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

Payment due 2024-09-23[$250] [P2P Distance] Add RBR logic for the bottom up flow #46753

Closed
neil-marcellini opened this issue Aug 2, 2024 · 45 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@neil-marcellini
Copy link
Contributor

neil-marcellini commented Aug 2, 2024

Implement the plan in this design doc section to add RBR logic for the bottom up flow.

Please branch off of this PR [App] feat: Edit Distance Rate flow if it hasn't been merged yet.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c65559d6ec0b66ea
  • Upwork Job ID: 1819470334389076893
  • Last Price Increase: 2024-08-02
  • Automatic offers:
    • paultsimura | Contributor | 103375763
    • brunovjk | Contributor | 103375768
Issue OwnerCurrent Issue Owner: @zanyrenney
@neil-marcellini neil-marcellini added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 2, 2024
@neil-marcellini neil-marcellini self-assigned this Aug 2, 2024
@neil-marcellini neil-marcellini added the External Added to denote the issue can be worked on by a contributor label Aug 2, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

Triggered auto assignment to @zanyrenney (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Copy link

melvin-bot bot commented Aug 2, 2024

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

@melvin-bot melvin-bot bot changed the title [P2P Distance] Add RBR logic for the bottom up flow [$250] [P2P Distance] Add RBR logic for the bottom up flow Aug 2, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 2, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

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

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

melvin-bot bot commented Aug 2, 2024

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

Copy link

melvin-bot bot commented Aug 2, 2024

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@paultsimura
Copy link
Contributor

@neil-marcellini I tried to reproduce the bottom-up flow, but it requires adding a bank account, and I assume enabling a wallet, which I cannot do as a contributor.

I have a test bank account added, but I can pay the request only elsewhere:
image

image

Is there any hack to enable the bank payments for testing the bottom-up flow?

@melvin-bot melvin-bot bot added the Overdue label Aug 5, 2024
Copy link

melvin-bot bot commented Aug 6, 2024

@paultsimura, @neil-marcellini, @zanyrenney, @brunovjk Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@neil-marcellini
Copy link
Contributor Author

Ah shoot. Luckily it looks like we are allowed to share the test account credentials. Try setting up a bank account using the information provided in the steps here. Lmk if that works or if you have further issues.

Oh, I see that maybe you have already done that. What are the conditions for App to show the bank account as a payment option?

@paultsimura
Copy link
Contributor

Seems like the issue is simpler than I expected: the bank payment is only allowed for USD:

const canUseWallet = !isExpenseReport && !isInvoiceReport && currency === CONST.CURRENCY.USD;

I'll comment it out and proceed with the development.

@brunovjk
Copy link
Contributor

brunovjk commented Aug 8, 2024

Not overdue -- @paultsimura let me know if you need any help :D

@melvin-bot melvin-bot bot removed the Overdue label Aug 8, 2024
@paultsimura
Copy link
Contributor

Some emotional support would be nice @brunovjk 😅

@brunovjk
Copy link
Contributor

brunovjk commented Aug 8, 2024

@paultsimura, I know these challenges can be tough, but you’ve faced similar ones before and come through. You’ve got this!

@paultsimura
Copy link
Contributor

@neil-marcellini is the "Rate not valid for this workspace" (taken from the doc) a verified copy?

@neil-marcellini neil-marcellini removed the Daily KSv2 label Aug 20, 2024
@melvin-bot melvin-bot bot removed the Overdue label Aug 20, 2024
@neil-marcellini
Copy link
Contributor Author

Hey @neil-marcellini any update on this one? Appreciate you're working 50% but if we can keep this updated according to the KSV2 cadence, that would be great.

@paultsimura if you can also contribute to the updates whilst Neil is working part time, that would really help. Cheers!

Good idea. I will drop this to a weekly since P2P distance is quite low priority as I understand it.

@neil-marcellini looks like we might be waiting on you for the BE changes before @paultsimura can finish off his portion.

Yes that is the case. I have been working on higher priority items. I will set a reminder to work on this on Thursday, if not sooner.

@neil-marcellini
Copy link
Contributor Author

I got started by writing a failing test for this flow. To be clear, I'm focusing on the edge case where a p2p distance expense is created towards the end of the year, then the default USD rate for workspaces is updated for the new year, after which the receiver pays the expense on a business bank account, creating a workspace. Then the submitter notices the violation and updates to the workspace default rate. The goal of the PR is to make sure the P2P distance rate data is properly updated/cleared and the violation is also cleared. I might need a Web-E PR to clear the violation. There might be other flows that could cause a similar violation, but this one is the most familiar to me so I'll focus on it.

@neil-marcellini
Copy link
Contributor Author

I worked on this a little more today. Fixed some mistakes in the test and implemented the solution. Now the test is erroring and I'll have to debug it.

Copy link

melvin-bot bot commented Aug 30, 2024

@paultsimura @jamesdeanexpensify @neil-marcellini @zanyrenney @brunovjk this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@melvin-bot melvin-bot bot added the Overdue label Sep 4, 2024
@neil-marcellini
Copy link
Contributor Author

Haven't had time recently, working on higher priority items

@melvin-bot melvin-bot bot removed the Overdue label Sep 4, 2024
@neil-marcellini
Copy link
Contributor Author

I finally got the backend PR up for review! I'm going to go ahead and start testing and potentially adding on to @paultsimura's PR because the flow we want to test requires changing the backend code part way through the test steps.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Sep 10, 2024
@neil-marcellini
Copy link
Contributor Author

@zanyrenney when it comes to payment time I think we should give @paultsimura the full amount, even though I jumped in and wrote some of the PR myself. The part he worked on was definitely substantial. I chose to jump in because the flow we want to test requires changing the backend as part of one of the steps, so it was the most efficient way to finish it off.

@neil-marcellini neil-marcellini removed the Reviewing Has a PR in review label Sep 18, 2024
@melvin-bot melvin-bot bot added the Overdue label Sep 18, 2024
@neil-marcellini neil-marcellini removed Overdue Waiting for copy User facing verbiage needs polishing labels Sep 18, 2024
@neil-marcellini neil-marcellini added Awaiting Payment Auto-added when associated PR is deployed to production and removed Overdue labels Sep 18, 2024
@melvin-bot melvin-bot bot added the Overdue label Sep 18, 2024
@paultsimura
Copy link
Contributor

Not overdue.
It was deployed to prod. Payment is due 2024-09-23.

@zanyrenney zanyrenney changed the title [$250] [P2P Distance] Add RBR logic for the bottom up flow Payment due 2024-09-23[$250] [P2P Distance] Add RBR logic for the bottom up flow Sep 19, 2024
@zanyrenney
Copy link
Contributor

payment due next week, not overdue.

@melvin-bot melvin-bot bot removed the Overdue label Sep 19, 2024
@brunovjk
Copy link
Contributor

I didn't review the PR - #47136 (comment)

@zanyrenney
Copy link
Contributor

payment summary

$250 to @paultsimura paid via upwork
no payment for review because this was reviewed internally by @youssef-lr and @neil-marcellini

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
No open projects
Status: No status
Development

No branches or pull requests

6 participants