Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.32k stars 2.75k forks source link

[$250] Negative amount displayed in Pay with Expensify Dropdown Button and Confirm payment amount Modal #42325

Closed m-natarajan closed 1 week ago

m-natarajan commented 3 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 1.4.74-4 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @ikevin127 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1715813045792699

Action Performed:

Prerequisite : Have one existing account (A) and one newly created account (B)

  1. Login as new user (B) in a browser
  2. On the “What do you want to do today?” modal choose “Manage my team's expenses” and Continue. 3, Input business name and Continue.
  3. Input First and Last Name and Continue.
  4. On the “Welcome” modal click Get Started.
  5. Log in as an existing user (A) in incognito or in another browser
  6. Click FAB > Submit expense > Manual > enter any amount and select the user B
  7. Create one more IOU with user B to have 2 expenses report
  8. Hold one of the expenses
  9. Login to user B account and look for the IOU request from user A (If the report with the existing user does not show-up in LHN then click FAB > Start chat > Input existing user email and start a chat)
  10. Once the chat opens, we should see the two report expenses requested by the existing user with the “Pay with Expensify” green dropdown button.
  11. Click on the report preview showing “2 expenses” at the bottom in order to see the two expenses. Notice that the report header “Pay with Expensify” green dropdown button shows negative amount like -$7.00.
  12. Click on the “Pay with Expensify” green dropdown arrow button and notice that both options are showing negative amount. After clicking on the dropdown arrow button > Select “Pay elsewhere”.
  13. Click on the header “Pay elsewhere” button and notice that the “Confirm what to pay” modal also shows negative amount for both options.

    Expected Result:

    -Report header “Pay with Expensify” green dropdown button shows the amount as positive amount -Clicking on the header “Pay elsewhere” button “Confirm what to pay” modal also shows positive amount for both options.

Actual Result:

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence Screenshot 2024-05-10 at 21 54 05

Screenshot 2024-05-10 at 21 53 52

https://github.com/Expensify/App/assets/38435837/a9a56919-16d4-4320-b4d8-159d256cc0c5

https://github.com/Expensify/App/assets/38435837/3b939592-6fea-49b7-9310-ae0bd73ce4ab

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01cc7f63f52440c4db
  • Upwork Job ID: 1800107311798006347
  • Last Price Increase: 2024-06-17
  • Automatic offers:
    • alitoshmatov | Reviewer | 102858943
    • dominictb | Contributor | 102858945
Issue OwnerCurrent Issue Owner: @zanyrenney
melvin-bot[bot] commented 3 months ago

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.

ikevin127 commented 3 months ago

To me this looks BE-related if we look at the transaction amounts (see C+ Slack 🧵) being a positive number, considering it should be negative as that's how we store it in BE and since we have logic here, here and other places in FE in order to make it positive.

[!note] If this is BE related, I'd look at the “What do you want to do today?” Modal -> “Manage my team's expenses” flow.

But could also be some mix-up on the FE, not entirely sure 🤔

zanyrenney commented 3 months ago

Thanks for adding these thoughts @ikevin127 !

zanyrenney commented 3 months ago

checking here - https://expensify.slack.com/archives/C01GTK53T8Q/p1716227156290099

melvin-bot[bot] commented 3 months ago

@zanyrenney Whoops! This issue is 2 days overdue. Let's get this updated quick!

zanyrenney commented 3 months ago

Seeing as this is a BE issue and noone has volunteered for it in slack. I think we should reopen if we encounter this via a real user flow rather than the slightly complex reproduction steps above.

lanitochka17 commented 3 months ago

Issue is still reproducible on the latest build 1.4.76-2 Steps: Submitter

  1. Create an IOU with several expenses in a 1:1 with another user you have access to Participant
  2. Log in as the other user
  3. Navigate to a expense, click the three dots in the header, and choose Hold
  4. Leave a hold comment and confirm
  5. Navigate up to the report conversation

https://github.com/Expensify/App/assets/78819774/e76f33be-0243-4fbb-acac-f96aff6f879e

melvin-bot[bot] commented 3 months ago

@zanyrenney this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 3 months ago

@zanyrenney Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 3 months ago

@zanyrenney Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

zanyrenney commented 3 months ago

Been OOO, this reopened whilst I was out.

zanyrenney commented 3 months ago

As it's still reproducible, assigning external.

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

dominictb commented 3 months ago

Proposal

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

Notice that the report header “Pay with Expensify” green dropdown button shows negative amount like -$7.00. Click on the “Pay with Expensify” green dropdown arrow button and notice that both options are showing negative amount. After clicking on the dropdown arrow button > Select “Pay elsewhere”. Click on the header “Pay elsewhere” button and notice that the “Confirm what to pay” modal also shows negative amount for both options.

What is the root cause of that problem?

We show the displayed amount as the nonHeldAmount here if the iouReport has held request and nonHeldAmount is returned by getNonHeldAndFullAmount

In this function, we always *-1 here without checking the iouReport is expense report or not. And then because the iou report has positive amount, the negative amount is displayed.

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

For here and here we should check if the iouReport is expense report, we will * -1 otherwise we will * 1.

What alternative solutions did you explore? (Optional)

NA

zanyrenney commented 3 months ago

@alitoshmatov please can you review the proposal?

melvin-bot[bot] commented 3 months ago

@zanyrenney @alitoshmatov this issue is now 4 weeks old, please consider:

Thanks!

melvin-bot[bot] commented 3 months ago

@zanyrenney, @alitoshmatov Whoops! This issue is 2 days overdue. Let's get this updated quick!

zanyrenney commented 2 months ago

bump @alitoshmatov please review the proposal?

zanyrenney commented 2 months ago

bumped in open source: https://expensify.slack.com/archives/C01GTK53T8Q/p1718634671704649

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

@zanyrenney, @alitoshmatov Still overdue 6 days?! Let's take care of this!

alitoshmatov commented 2 months ago

This issue is reproducable without any additional steps. You just need to hold one of the iou transactions and receiving user sees total amount as negative

To expand more I do agree with @ikevin127 's take here

It looks like we are treating expense amount as negative numbers in multiple instances, also this comment indicates the same conclusion.

But this positive amount is working completely fine when using iou normally without holding any transaction.

Before we proceed we should make clear expected values here, @zanyrenney we internal engineer to look at this comments and answer to the following question:

Screenshot 2024-06-18 at 3 02 13 PM
dominictb commented 2 months ago

But this positive amount is working completely fine when using iou normally without holding any transaction.

@alitoshmatov Can you review my proposal here . It will explain why the positive amount is not working with holding transaction.

Additional information, the amount value of transaction in expense report will be negative amount and it will be positive amount for iou report.

melvin-bot[bot] commented 2 months ago

@zanyrenney, @alitoshmatov Whoops! This issue is 2 days overdue. Let's get this updated quick!

alitoshmatov commented 2 months ago

@dominictb Oh I see, I understand now. I confused iou and expense reports with each other. I was focused on 1:1 chats and testing iou report only. I forgot to test workspace chats where expanse reports are created.

alitoshmatov commented 2 months ago

We can go with @dominictb 's proposal

C+ reviewed 🎀 👀 🎀

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @blimpich, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 2 months ago

📣 @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

melvin-bot[bot] commented 2 months ago

📣 @dominictb 🎉 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 📖

blimpich commented 2 months ago

This confused me for a minute and I had to read through the discussions and look at the code for awhile but it does make sense to me that this is a frontend issue. Appears that we overlooked this particular scenario when implementing the hold functionality for reports.

alitoshmatov commented 2 months ago

@blimpich Sorry missed your comment.

Can you elaborate, which part you think we have a problem. Is this the fact that amounts are in negative when in expense reports and positive in iou reports?

Should we pause PR or go on with proposed solution?

blimpich commented 2 months ago

Go with the proposed solution, sorry, my comment was worded poorly. When I say "we overlooked" I don't mean you and @dominictb, I mean the original implementors of this code that we are now fixing.

Please proceed forward with the proposed solution.

melvin-bot[bot] commented 1 month ago

This issue has not been updated in over 15 days. @blimpich, @zanyrenney, @alitoshmatov, @dominictb eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

blimpich commented 1 month ago

Did we ever pay out for this issue? @zanyrenney can you make sure we paid for the work done here? I think maybe the automation failed?

alitoshmatov commented 1 month ago

The PR was deployed to production on july 16th

alitoshmatov commented 1 month ago

We can complete the payment and close the issue

cc: @blimpich @zanyrenney

melvin-bot[bot] commented 3 weeks ago

This issue has not been updated in over 15 days. @blimpich, @zanyrenney, @alitoshmatov, @dominictb eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

alitoshmatov commented 1 week ago

@blimpich @zanyrenney

Bump

blimpich commented 1 week ago

Sorry for the delay @alitoshmatov!

@zanyrenney can we get payment completed and close out this issue soon?

zanyrenney commented 1 week ago

Sure thing @blimpich - on it now.

zanyrenney commented 1 week ago

Payment summary

paid @alitoshmatov $250 via upwork paid @dominictb $250 via upwork.