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

[$250] Search - Total amount does not auto update after changing the expense currency #49493

Open
2 of 6 tasks
IuliiaHerets opened this issue Sep 19, 2024 · 40 comments
Open
2 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors Internal Requires API changes or must be handled by Expensify staff Needs Reproduction Reproducible steps needed Overdue retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Sep 19, 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: 9.0.38-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to DM.
  3. Submit two expenses.
  4. Go to Search.
  5. Go to Outstanding.
  6. Click View next to the submitted expense.
  7. Click Amount.
  8. Change the currency and save it.
  9. Dismiss the RHP.

Expected Result:

The total amount should auto update after changing the expense currency.

Actual Result:

The total amount does not auto update after changing the expense currency.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6608678_1726739142065.bandicam_2024-09-19_17-41-06-770.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021845906959284211554
  • Upwork Job ID: 1845906959284211554
  • Last Price Increase: 2024-10-21
Issue OwnerCurrent Issue Owner: @marcochavezf
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 19, 2024
Copy link

melvin-bot bot commented Sep 19, 2024

Triggered auto assignment to @CortneyOfstad (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.

@IuliiaHerets
Copy link
Author

We think that this bug might be related to #wave-control

@IuliiaHerets
Copy link
Author

@CortneyOfstad FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@CyberAndrii
Copy link
Contributor

Proposal

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

Total amount does not auto update after changing the expense currency

What is the root cause of that problem?

We compute the total here

const formattedTotal = TransactionUtils.getAmount(transactionItem, isExpenseReport);

However, this function (getTransactionItemCommonFormattedProperties) is only called if the route/query changes. When we change the currency the route/query doesn't change, and we read the outdated total value.

const {type, status, sortBy, sortOrder, hash} = queryJSON;
const [currentSearchResults] = useOnyx(`${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`);

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

Instead of reading from the formattedTotal property, we can calculate the total based on the updated transaction list.

Replace this line

let amount = CurrencyUtils.convertToDisplayString(transactionItem.formattedTotal, currency);

with the following code

const isExpenseReport = transactionItem.reportType === CONST.REPORT.TYPE.EXPENSE;
const total = TransactionUtils.getAmount(transactionItem, isExpenseReport);
let amount = CurrencyUtils.convertToDisplayString(total, currency);

A similar approach is already implemented for reports.

Finally, we can remove the formattedTotal property from here

const formattedTotal = TransactionUtils.getAmount(transactionItem, isExpenseReport);

@melvin-bot melvin-bot bot added the Overdue label Sep 23, 2024
Copy link

melvin-bot bot commented Sep 23, 2024

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

@CortneyOfstad CortneyOfstad added Needs Reproduction Reproducible steps needed retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause labels Sep 23, 2024
@CortneyOfstad
Copy link
Contributor

So one aspect here is missing — the default currency in the workspace. If the default currency is set to MHR, then the total amount did update.

I am not able to recreate this as the expense converts to the home currency of the workspace.

Going to have this retested.

@melvin-bot melvin-bot bot removed the Overdue label Sep 23, 2024
@MelvinBot
Copy link

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

@melvin-bot melvin-bot bot added the Overdue label Sep 26, 2024
Copy link

melvin-bot bot commented Sep 27, 2024

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

Copy link

melvin-bot bot commented Oct 1, 2024

@CortneyOfstad 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@CortneyOfstad
Copy link
Contributor

Still waiting for this to be retested!

@melvin-bot melvin-bot bot removed the Overdue label Oct 1, 2024
Copy link

melvin-bot bot commented Oct 3, 2024

@CortneyOfstad 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 melvin-bot bot added the Overdue label Oct 3, 2024
@CortneyOfstad
Copy link
Contributor

Not overdue as waiting for this to be retested!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 4, 2024
@CortneyOfstad
Copy link
Contributor

Again, still waiting for this to be retested!

@melvin-bot melvin-bot bot removed the Overdue label Oct 8, 2024
@CyberAndrii
Copy link
Contributor

This issue is still reproducible. To clarify as the title is a bit misleading - the total amount does update, but not to the correct value. It should be around 1500 but it's showing as 1212. Refreshing the page displays the correct amount.

@melvin-bot melvin-bot bot added the Overdue label Oct 10, 2024
Copy link

melvin-bot bot commented Oct 11, 2024

@CortneyOfstad Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@CortneyOfstad
Copy link
Contributor

Thanks @CyberAndrii!

@situchan
Copy link
Contributor

reviewing

@melvin-bot melvin-bot bot removed the Overdue label Oct 21, 2024
@situchan
Copy link
Contributor

@huult I think you got confused. This happens on Search page but your demo video is on Inbox tab.

@huult
Copy link
Contributor

huult commented Oct 22, 2024

@situchan , I apologize for sending the video with the wrong case, but they have the same RCA. Here is a video related to this ticket after my update:

Screen.Recording.2024-10-23.at.02.35.38.mp4

@situchan
Copy link
Contributor

@huult this only happens on Search page. No issue on Inbox

@huult
Copy link
Contributor

huult commented Oct 22, 2024

@situchan I apologize for misunderstanding this ticket. As a result, my proposal was incorrect. I will update the proposal shortly

@huult
Copy link
Contributor

huult commented Oct 22, 2024

@situchan I have identified the root cause and solutions to resolve this issue. I will post my proposal soon.

@huult
Copy link
Contributor

huult commented Oct 23, 2024

Edited by proposal-police: This proposal was edited at 2024-10-28 17:24:33 UTC.

Proposal

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

Total amount does not auto update after changing the expense currency

What is the root cause of that problem?

This issue occurred because the report data in the snapshot was not updated, so the total did not change after the currency was changed.

When we called UpdateMoneyRequestAmountAndCurrency to update the currency, the response from this API did not include the snapshot data, so the snapshot was not updated after completing UpdateMoneyRequestAmountAndCurrency, meaning the report was not updated either.

The total displayed is taken from the report in the snapshot. Since the data was not updated, the total still shows the old value after the currency update, and this issue occurs.

I shared video to describe it:

Screen.Recording.2024-10-23.at.06.42.51.mp4

The report data in snapshot will be automatically updated when report changes in react-native-onyx.

The updatedData of the report in snapshot is assigned a value twice. The first time, it is set to the value from the request response. The second time, it is updated with the value after the request is successful, as we assign it in the success data. During this second update, lodashPick returns an empty object, so the report data in snapshot is not updated

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

To fix this issue, we need to update react-native-onyx so that it checks if the value is an empty object and, if so, returns without assigning {} to updatedData.

https://github.com/Expensify/react-native-onyx/blob/36ff87b26a2751d0818462cceba8edacf9d3f923/lib/Onyx.ts#L573C51

//.lib/Onyx.ts#L273
-      updatedData = {...updatedData, [key]: lodashPick(value, Object.keys(snapshotData[key]))};
+      const newValue = lodashPick(value, Object.keys(snapshotData[key]))
+      if (utils.isEmptyObject(newValue)) {
+          return
+      }

+      updatedData = {...updatedData, [key]: newValue};

@situchan
Copy link
Contributor

I think this requires backend change. 🎀👀🎀
cc: @luacmartins since this is related to search

Copy link

melvin-bot bot commented Oct 23, 2024

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

@huult
Copy link
Contributor

huult commented Oct 23, 2024

That's correct. We need a backend change to update the snapshot data. If they request the searchHash, we need to ensure it is included in the request, as mentioned in my proposal.

@CortneyOfstad
Copy link
Contributor

Thanks @situchan and @huult for the information!

@marcochavezf — does that all look correct/achievable for the BE?

@marcochavezf
Copy link
Contributor

Yeah, it requires a BE change to update the snapshot, changing to Internal

@melvin-bot melvin-bot bot added the Overdue label Oct 25, 2024
@marcochavezf marcochavezf added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Overdue labels Oct 25, 2024
@marcochavezf
Copy link
Contributor

Not overdue. @luacmartins feel free to take it since you have more context than me about this one 😄

@melvin-bot melvin-bot bot removed the Overdue label Oct 25, 2024
@luacmartins
Copy link
Contributor

We shouldn't be updating the snapshot directly in this case (both when it comes to optimistic and API return data). Onyx automatically applies updates to the snapshot data if it's present in the local Onyx db. Does the response from UpdateMoneyRequestAmountAndCurrency include an update to the report total?

@huult
Copy link
Contributor

huult commented Oct 26, 2024

@luacmartins

Does the response from UpdateMoneyRequestAmountAndCurrency include an update to the report total?

The response from UpdateMoneyRequestAmountAndCurrency includes the report total in the report_ data, but the snapshot_ does not. In this case, we need the backend response to include the snapshot_ data with the updated report total like json example I included in my proposal

@luacmartins
Copy link
Contributor

That shouldn't be necessary. Onyx should automatically apply any changes with a matching snapshot_ key. See this code in Onyx - https://github.com/Expensify/react-native-onyx/blob/36ff87b26a2751d0818462cceba8edacf9d3f923/lib/Onyx.ts#L541-L584

@melvin-bot melvin-bot bot added the Overdue label Oct 28, 2024
@huult
Copy link
Contributor

huult commented Oct 28, 2024

@luacmartins , Based on your information, I found the RCA and solution. May I ask if this ticket is still open for external proposals?

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. Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors Internal Requires API changes or must be handled by Expensify staff Needs Reproduction Reproducible steps needed Overdue retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause
Projects
Status: Polish
Development

No branches or pull requests

8 participants