Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD 36985][$500] [P2P Distance] Implement bottom up flow violations in App #36988

Closed
neil-marcellini opened this issue Feb 21, 2024 · 20 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Weekly KSv2

Comments

@neil-marcellini
Copy link
Contributor

neil-marcellini commented Feb 21, 2024

Implement bottom up flow violations following this plan. Please be sure to read and understand the relevant sections of the entire plan as well.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e3e6076dcf3ac3bb
  • Upwork Job ID: 1760136030900682752
  • Last Price Increase: 2024-03-06
  • Automatic offers:
    • alitoshmatov | Reviewer | 0
@neil-marcellini neil-marcellini changed the title [P2P Distance] Implement bottom up flow violations [P2P Distance] Implement bottom up flow violations in App Feb 21, 2024
@neil-marcellini neil-marcellini added External Added to denote the issue can be worked on by a contributor Engineering Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors labels Feb 21, 2024
Copy link

melvin-bot bot commented Feb 21, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01e3e6076dcf3ac3bb

@melvin-bot melvin-bot bot changed the title [P2P Distance] Implement bottom up flow violations in App [$500] [P2P Distance] Implement bottom up flow violations in App Feb 21, 2024
Copy link

melvin-bot bot commented Feb 21, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @alitoshmatov (External)

@neil-marcellini neil-marcellini added the NewFeature Something to build that is a new item. label Feb 21, 2024
Copy link

melvin-bot bot commented Feb 21, 2024

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Feb 21, 2024
@dukenv0307
Copy link
Contributor

dukenv0307 commented Feb 22, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Implement bottom up flow violations following this plan.

What is the root cause of that problem?

This is part of a new feature.

What changes do you think we should make in order to solve the problem?

This is dependent on #36985, so we should ideally start implementing this issue after the barebone from #36985 (with the rate field in MoneyRequestView) is ready.

  1. If the rate is out of workspace - show the error message “Rate not valid for this workspace”
  • Define the translations for the error message in the the translation files (Suggested translation - ES: Tasa inválida para este espacio de trabajo)
  • Add the translation for the violation to the getViolationTranslation function (This particular violation has name ‘customUnitOutOfPolicy’)
  1. Add the violation logic to the Rate field in MoneyRequestView
  • Prerequisite: This Rate field should already been built as part of [HOLD for payment 2024-05-02] CRITICAL: [P2P Distance] [$500] Create a new Rate field #36985
  • Associate the violation name customUnitOutOfPolicy to the rate field here
  • Add the errors to the Rate field by defining the brickRoadIndicator and error of the MenuItemWithTopDescription of the rate, just like we did for other fields like Category here
  • Clear the violation optimistically after the user selects a new correct rate. We should add logic to optimistically update transactionViolation error, using getViolationsOnyxData, so if we have customUnitOutOfPolicy violations, and the rate/custom unit now exists, and it's in the policy, we should clear (aka reject) the violation. We can do it similar to what we did with the categoryOutOfPolicy violation here

Those should be the main changes, other minor adjustments can be made during the PR phase.

What alternative solutions did you explore? (Optional)

NA

@koko57
Copy link
Contributor

koko57 commented Feb 22, 2024

Commenting to help with the review.

@neil-marcellini
Copy link
Contributor Author

Thanks for the proposal @dukenv0307, but I'm going to ask @waterim from Callstack to take this one.

Copy link

melvin-bot bot commented Feb 28, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Feb 29, 2024
@waterim
Copy link
Contributor

waterim commented Mar 1, 2024

Hello, Im Artem from Callstack and I would like to help with this one

Copy link

melvin-bot bot commented Mar 6, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@CortneyOfstad
Copy link
Contributor

Hi @waterim! Sorry for the delay, as I was OoO last week. Assigning you now!

@melvin-bot melvin-bot bot removed the Overdue label Mar 11, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 11, 2024
Copy link

melvin-bot bot commented Mar 11, 2024

📣 @alitoshmatov 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

@CortneyOfstad
Copy link
Contributor

@waterim can you provide an update on the ETA here? Do you think you'll have a PR or even a draft by EOD, EOW, etc? Thanks!

@waterim
Copy link
Contributor

waterim commented Mar 15, 2024

Hello @CortneyOfstad, as far as I know this one was on hold for some backend changes, is it off hold now?
if yes, I will prepare a PR during first half if the next week!

@CortneyOfstad
Copy link
Contributor

@neil-marcellini can you confirm if this is the on-hold issue — #36985? This doesn't appear to be BE related, so just wanted to get this clarified 👍

@neil-marcellini
Copy link
Contributor Author

Yes it should be on hold for #36985. I will update the title. @waterim if you want to collaborate with @koko57 to get started early, you could branch off of her draft PR.

@neil-marcellini neil-marcellini changed the title [$500] [P2P Distance] Implement bottom up flow violations in App [HOLD 36985][$500] [P2P Distance] Implement bottom up flow violations in App Mar 25, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 1, 2024
@CortneyOfstad
Copy link
Contributor

PR was linked in on-hold issue here 👍

@melvin-bot melvin-bot bot removed the Overdue label Apr 1, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 9, 2024
@CortneyOfstad
Copy link
Contributor

PR is still being reviewed, so no update on this side yet 👍

@melvin-bot melvin-bot bot removed the Overdue label Apr 10, 2024
@CortneyOfstad
Copy link
Contributor

PR is still open and being reviewed — will continue to keep an eye on this 👍

@CortneyOfstad
Copy link
Contributor

Deployed to production 14 hours ago!

@CortneyOfstad CortneyOfstad added Awaiting Payment Auto-added when associated PR is deployed to production and removed Awaiting Payment Auto-added when associated PR is deployed to production labels Apr 24, 2024
@CortneyOfstad
Copy link
Contributor

This can be closed as the on-hold issue finished the 7-day hold for production yesterday! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Weekly KSv2
Projects
No open projects
Development

No branches or pull requests

6 participants