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.44k stars 2.8k forks source link

IOU - Currency in the amount editor is USD when the request is created in local currency #40543

Closed izarutskaya closed 5 months ago

izarutskaya commented 5 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.63-5 Reproducible in staging?: Y Reproducible in production?: N Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to 1:1 DM.
  3. Create a manual request in local currency.
  4. Go to transaction thread.
  5. Click Amount.

Expected Result:

The currency in Amount editor should be the local currency,.

Actual Result:

The currency in Amount editor is USD, when the request is created in local currency.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/95f49bee-4c0c-4803-a28a-02afa03c0fae

View all open jobs on GitHub

melvin-bot[bot] commented 5 months ago

Triggered auto assignment to @Beamanator (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

melvin-bot[bot] commented 5 months ago

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

github-actions[bot] commented 5 months ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
izarutskaya commented 5 months ago

@joekaufmanexpensify 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.

izarutskaya commented 5 months ago

We think this issue might be related to the #collect project.

izarutskaya commented 5 months ago

Production

https://github.com/Expensify/App/assets/115492554/c79b040c-a514-4f31-985d-2ec3e301aac3

nkdengineer commented 5 months ago

Proposal

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

The currency in Amount editor is USD, when the request is created in local currency.

What is the root cause of that problem?

We're passing wrong transaction data to get the currency here. we should pass it as transaction when we're editing instead of draftTransaction

https://github.com/Expensify/App/blob/305f12c52ef228e7ed4845735a1794942a2b8d87/src/pages/iou/request/step/IOURequestStepAmount.tsx#L82

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

Update this line to

const {currency: originalCurrency} = ReportUtils.getTransactionDetails(isEditing ? transaction : draftTransaction) ?? {currency: CONST.CURRENCY.USD};

https://github.com/Expensify/App/blob/305f12c52ef228e7ed4845735a1794942a2b8d87/src/pages/iou/request/step/IOURequestStepAmount.tsx#L82

What alternative solutions did you explore? (Optional)

NA

joekaufmanexpensify commented 5 months ago

Seems like a legit bug. I could see this being a legit blocker. @Beamanator you agree?

Beamanator commented 5 months ago

Oof ya i would say this is a blocker, it's not great to show it's $100 when it's actually 100 in some other currency 😬

Beamanator commented 5 months ago

Hmm this one looks pretty sus: https://github.com/Expensify/App/pull/38892

cc @roryabraham @sobitneupane @bernhardoj

Beamanator commented 5 months ago

Posted in slack & tagged ^ those people - https://expensify.slack.com/archives/C01GTK53T8Q/p1713538343234679?thread_ts=1713521084.907369&cid=C01GTK53T8Q

Sadly I'm unable to quickly revert the PR to test, probably b/c the PR was started a while ago

bernhardoj commented 5 months ago

Hmm, based on @nkdengineer root cause and solution which I already tested, it shouldn't be coming from our PR because we don't change the isEditing ? draftTransaction : transaction code/logic.

image
bernhardoj commented 5 months ago

I think it's coming from https://github.com/Expensify/App/pull/39144. In amount page, we are creating a backup transaction if we are editing a money request. https://github.com/Expensify/App/blob/4654d6ed3389916e750ecdfdbdad08d637535b8d/src/pages/iou/request/step/IOURequestStepAmount.tsx#L107-L114

createBackupTransaction will take the current transaction data and copy it to transactionsDraft, but the PR above changes the name to transactionsBackup.

That's why when we are editing, we pass the draftTransaction instead of transaction to get the data from the backup data. https://github.com/Expensify/App/blob/4654d6ed3389916e750ecdfdbdad08d637535b8d/src/pages/iou/request/step/IOURequestStepAmount.tsx#L82

Rename the key to transactionsBackup and the issue is fixed. https://github.com/Expensify/App/blob/4654d6ed3389916e750ecdfdbdad08d637535b8d/src/pages/iou/request/step/IOURequestStepAmount.tsx#L269-L272

ONYXKEYS.COLLECTION.TRANSACTION_BACKUP

Beamanator commented 5 months ago

Thanks @bernhardoj ! I agree it does look like https://github.com/Expensify/App/pull/39144 is the real culprit - they modified createBackupTransaction which now updates a new onyx key, and they didn't look where else that function was used & if we needed to subscribe to the new onyx key there (which it looks like we do need to)

Beamanator commented 5 months ago

Ok I have a PR up for the fix - note I used @bernhardoj 's solution

melvin-bot[bot] commented 5 months ago

⚠️ 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.

shahinyan11 commented 5 months ago

My proposal will cover this issue . @Beamanator please take a look

DylanDylann commented 5 months ago

@Beamanator @allroundexperts The new field ONYXKEYS.COLLECTION.TRANSACTION_BACKUP should only be used in distance requests as explained here. With the manual request, we should use draftTransaction as before.

cc @mountiny @bernhardoj

joekaufmanexpensify commented 5 months ago

@DylanDylann I see you were already paid for this PR in https://github.com/Expensify/App/issues/40631#issuecomment-2089321279, so I don't think any payment is due here. Closing this out for now, but LMK if you think that is not correct!