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

[$500] IOU - Total amount is not updated when change the different currency IOU in offline #35209

Closed
4 of 6 tasks
lanitochka17 opened this issue Jan 25, 2024 · 27 comments
Closed
4 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 25, 2024

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.32-2
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: Applause - Internal Team
Slack conversation:

Issue found when executing PR #34011

Action Performed:

  1. Go to staging.new.expensify.com.
  2. Go offline
  3. Click FAB > Request money > Manual.
  4. Request money from a user that has no existing chat.
  5. In 1:1 DM, click + > Request money.
  6. Create another manual request in a different currency
  7. Open the detail IOU page from second request
  8. Change the currency into same as first IOU
  9. Go to 1:1 conversation

Expected Result:

Total from two IOS request should be present, since both requests are in same currency

Actual Result:

Only amount of the first request is displayed in total

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6355235_1706222767182.Recording__1927.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0110932b60eddd10f4
  • Upwork Job ID: 1750654949608632320
  • Last Price Increase: 2024-01-25
  • Automatic offers:
    • cubuspl42 | Reviewer | 28131429
    • dukenv0307 | Contributor | 28131430
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 25, 2024
@melvin-bot melvin-bot bot changed the title IOU - Total amount is not updated when change the different currency IOU in offline [$500] IOU - Total amount is not updated when change the different currency IOU in offline Jan 25, 2024
Copy link

melvin-bot bot commented Jan 25, 2024

Job added to Upwork: https://www.upwork.com/jobs/~0110932b60eddd10f4

Copy link

melvin-bot bot commented Jan 25, 2024

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 25, 2024
Copy link

melvin-bot bot commented Jan 25, 2024

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

@dukenv0307
Copy link
Contributor

dukenv0307 commented Jan 26, 2024

Proposal

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

Only amount of the first request is displayed in total

What is the root cause of that problem?

In here, when updating money request, we only recalculate the total of the iouReport when the amount changes. This makes it not working for the case when user changes transaction currency from same as iouReport to different, or vice versa.

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

Update this block to account for the cases of changing currency as well.

There're 3 cases:

  • Currency is changed from same as iouReport to different from iouReport: the transaction amount (retrieved from TransactionUtils.getAmount(updatedTransaction, ...)) should be subtracted from the iouReport total amount, because now the total amount won't include the total until the user goes online and the conversion rate is determined
  • Currency is changed from different from iouReport to same as iouReport: the transaction amount should be added to the iouReport total amount, because previously the amount was not included in total and now it's included
  • Currency is changed from different from iouReport to another currency different from iouReport: do nothing

When subtracted/added is mentioned above, it's understood that if the iouReport is stored as negative (in some cases), then subtracted/added will be added/subtracted.

When currency is mentioned above, it means modifiedCurrency field for updatedTransaction and transaction.modifiedCurrency || transaction.currency for transaction (or can just use TransactionUtils.getCurrency). It seems in here we're not using the correct currency field for updatedTransaction, we might want to fix that.

What alternative solutions did you explore? (Optional)

We can recalculate the iouReport total based on its money requests, when currency/amount is updated. That means we don't calculate the diff/changes and add to the iouReport total, but we'll calculate based on all the money requests from the ground up, this might help reduce chances of error.

Not related, but we can also reconsider the location of the Total will update when you're back online message, currently it seems to always show at the last money request in the iou report (even if that money request is not the one pending currency conversion), which is a bit odd to me.

@melvin-bot melvin-bot bot added the Overdue label Jan 29, 2024
Copy link

melvin-bot bot commented Jan 29, 2024

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

@jacobkim9881
Copy link
Contributor

jacobkim9881 commented Jan 29, 2024

Proposal

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

When money requests are being made with different currencies offline, amount total of report thread is not calculated based on each currency.

What is the root cause of that problem?

The frontend app should wait for response with calculated amount total from server when currencies are different. If it is online then the server calculates amount total but if it is offline and then amount total is shown as un-calculated without any filter.

When offline the amount total is calculated but it is false calculation for different currencies to money request thread. Technically the calculation is not from the server but frontend.

The un-matched amount total go through not only report preview but also report header, LHN, report setting page, share code page and browser notification.

스크린샷 2024-01-29 오후 8 11 18

스크린샷 2024-01-29 오후 8 13 00

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

When it is offline and when the report thread with more than 2 reports has different currencies, total amount will be shown as 'TBD'. For this convertToDisplayString function will be used for the solution. The functions gives 'TBD' when amountInCents is 0 and when shouldFallbackToTbd is true.

If it has different currencies, as optimistic data total should be 0 for amountInCents should be 0 to show 'TBD' at convertToDisplayString. Because with server response, success data change total into calculated amount currently. To build this, total will be given 0 with the condition when deciding optimistic data.

To check offline, we should import default function from src/hooks/useNetwork.ts at ReportUtils.getReportName function and at src/components/ReportActionItem/ReportPreview.js.

On report preview page, getDisplayAmount is used to decide total. We use the same logic for the issue.

On such tags at LHN, money report header, ReportUtils.getReportName is used to decide total amount for showing. It has convertToDisplayString function and the same logic will be applied.

What alternative solutions did you explore? (Optional)

Fetch currency information from web and use in frontend.

@cubuspl42
Copy link
Contributor

@jacobkim9881 So, effectively you're just suggesting to stop calculating any total sum while offline? This is not an acceptable behavior.

@melvin-bot melvin-bot bot removed the Overdue label Jan 30, 2024
@cubuspl42
Copy link
Contributor

I approve the proposal by @dukenv0307

C+ reviewed 🎀 👀 🎀

Copy link

melvin-bot bot commented Jan 30, 2024

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

@jacobkim9881
Copy link
Contributor

Hi, @cubuspl42. Thanks for reviewing. I want to apologize for the short and unclear proposal. I would like to reply to your question shortly. For suggesting to stop calculating, it was only for limited money requests. Based on the proposal, only problematic report threads will be stopped to calculate.

@jacobkim9881
Copy link
Contributor

With my proposal, the PR will solve un-calculated total at report's name on LHN too. I hope your reconsideration.

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

melvin-bot bot commented Jan 30, 2024

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

Copy link

melvin-bot bot commented Jan 30, 2024

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

@jacobkim9881
Copy link
Contributor

I want to say my proposal doesn't stop to calculate the amount. My proposal makes un-calculated amount total shown as TBD. The amount total can't be calculated same from the server's calculation response. If the amount should be calculated then it should be cared an internal issue as a new feature.

@cubuspl42
Copy link
Contributor

@jacobkim9881 Thanks for your proposal and the explanations. I understand what you're suggesting. Out of these two proposals, I decided that the other is a better way forward.

@jacobkim9881
Copy link
Contributor

@cubuspl42 I see and respect your decision. I hope the issue will be solved well.

@melvin-bot melvin-bot bot added the Overdue label Feb 2, 2024
@bfitzexpensify
Copy link
Contributor

How is this one going @dukenv0307?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Feb 2, 2024
@bfitzexpensify
Copy link
Contributor

Bump @dukenv0307, can you please provide an update?

@melvin-bot melvin-bot bot removed the Overdue label Feb 5, 2024
@dukenv0307
Copy link
Contributor

I'm working on the PR, thanks for your bump.

Copy link

melvin-bot bot commented Feb 20, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Copy link

melvin-bot bot commented Feb 20, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Copy link

melvin-bot bot commented Feb 20, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@cubuspl42
Copy link
Contributor

This is officially a disaster. I believe we underestimated the impact of the change.

@tylerkaraszewski
Copy link
Contributor

I think all the deploy blockers related to this have been resolved now. Maybe we can close this out?

@dukenv0307
Copy link
Contributor

@bfitzexpensify I think we're ready for payment now.

@cubuspl42
Copy link
Contributor

I think we're ready for payment now.

For whatever is left of it, anyway 😅

@bfitzexpensify
Copy link
Contributor

OK, there was a lot to try and follow here. I'm going to bring the price down to $125 - it should be halved again (3 regressions in total), but $62.50 feels too low for the work put in.

Payments complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

6 participants