App icon indicating copy to clipboard operation
App copied to clipboard

[$250] Workspace - Default workspace currency is changed to USD when changing a workspace name

Open IuliiaHerets opened this issue 1 year ago โ€ข 7 comments

If you havenโ€™t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: v9.0.73-6 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team

Action Performed:

Prerequisite User's location is not US and default currency is not USD

  1. Sign in with a same account both in ND and OD
  2. In OD create a new workspace which will automatically set a default currency which is not USD
  3. Navigate to new dot > Workspace settings > Profile > Name > Edit the name of the workspace > Save
  4. Now observe the default currency when saving the edited name

Expected Result:

Default currency stays the same when editing the name of the workspace

Actual Result:

Default currency is automatically changed to USD when editing the name of the workspace

Workaround:

Unknown

Platforms:

  • [ ] Android: Standalone
  • [ ] Android: HybridApp
  • [x] Android: mWeb Chrome
  • [ ] iOS: Standalone
  • [ ] iOS: HybridApp
  • [ ] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/5c7c63e6-bdee-4fba-ad77-e80bcf4ba79d

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021866457192804972214
  • Upwork Job ID: 1866457192804972214
  • Last Price Increase: 2024-12-10
Issue OwnerCurrent Issue Owner: @Pujan92

IuliiaHerets avatar Dec 10 '24 11:12 IuliiaHerets

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

melvin-bot[bot] avatar Dec 10 '24 11:12 melvin-bot[bot]

Proposal

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

Default currency is changed to USD if the policy is created from OD.

What is the root cause of that problem?

This has the same root cause as https://github.com/Expensify/App/issues/52190. When we create a policy from OD and we view it in ND, the outputCurrency of the policy is not available yet. Updating the workspace name also requires a currency param and we fallback to USD. https://github.com/Expensify/App/blob/5a0c6896ec681b0fb7d9371393d979b0a7e31633/src/libs/actions/Policy/Policy.ts#L1171

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

Since the API requires the currency param too, we can follow the same approach in https://github.com/Expensify/App/pull/53132 by fallback to other values first before fallback to USD.

const currency = currencyValue ?? policy?.outputCurrency ?? DistanceRequestUtils.getDefaultMileageRate(policy)?.currency ?? PolicyUtils.getPersonalPolicy()?.outputCurrency ?? CONST.CURRENCY.USD;

Or if we want a more complex approach, then the BE needs to update the API to accept undefined currency which indicates that we don't want to update the currency, so we don't need to fallback to USD and skip the currency optimistic data.

Or, BE can separate the UpdateWorkspaceGeneralSettings API into 2 APIs, 1 for name, 1 for currency.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We can merge a fake policy without outputCurrency to Onyx but it has the mileage rate data which contains the currency information. Then, assert that after calling updateGeneralSettings without currency param, the policy currency data isn't changed.

bernhardoj avatar Dec 10 '24 12:12 bernhardoj

So strange, I can repro this.

https://github.com/user-attachments/assets/6c4cc3be-8b3b-44bf-8e6c-3145a6425b7f

@MonilBhavsar as you worked on that change initially, can you take a look at this? If the user doesn't change the currency field, we don't want to update it to USD.

trjExpensify avatar Dec 10 '24 12:12 trjExpensify

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

melvin-bot[bot] avatar Dec 10 '24 12:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 10 '24 12:12 melvin-bot[bot]

I don't recall working on this ๐Ÿ˜… but happy to take a look

MonilBhavsar avatar Dec 10 '24 12:12 MonilBhavsar

I think it was an update to the UpdateGeneralWorkspaceSettings function.

trjExpensify avatar Dec 10 '24 13:12 trjExpensify

I think ND api response should include the outputCurrency data also for the respective policy. If it can't be and needs to be handled on the front-end side then we can proceed with @bernhardoj's proposal.

Screenshot 2024-12-11 at 22 20 39

Pujan92 avatar Dec 11 '24 16:12 Pujan92

@Pujan92, @trjExpensify, @MonilBhavsar Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] avatar Dec 17 '24 09:12 melvin-bot[bot]

@MonilBhavsar thoughts here?

trjExpensify avatar Dec 17 '24 12:12 trjExpensify

๐Ÿ“ฃ 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] avatar Dec 17 '24 16:12 melvin-bot[bot]

Made this internal. Working partially this week. I should have an update by EOW

MonilBhavsar avatar Dec 17 '24 16:12 MonilBhavsar

@Pujan92, @trjExpensify, @MonilBhavsar 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] avatar Dec 19 '24 09:12 melvin-bot[bot]

Looking tomorrow

MonilBhavsar avatar Dec 19 '24 18:12 MonilBhavsar

Trying to reproduce this issue. I set the location manually other than USD, but still the workspace's default currency is set as USD. Can anyone please help to quickly reproduce this

MonilBhavsar avatar Dec 20 '24 18:12 MonilBhavsar

What steps are you taking? As you're outside of the US, just creating a workspace should set your workspace currency to IDR (or wherever you are right now!) on creation.

  1. Sign in with a same account both in ND and OD
  2. In OD create a new workspace which will automatically set a default currency which is not USD
  3. Navigate to newDot > Workspace settings > Profile > Name > Edit the name of the workspace > Save
  4. Now observe the default currency when saving the edited name

trjExpensify avatar Dec 20 '24 20:12 trjExpensify

I think i got it. I was trying on dev and since we use US timezone by default, I was not able to reproduce ๐Ÿ‘

MonilBhavsar avatar Dec 23 '24 04:12 MonilBhavsar

@Pujan92 @trjExpensify @MonilBhavsar this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar Dec 24 '24 09:12 melvin-bot[bot]

๐Ÿ“ฃ 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] avatar Dec 24 '24 16:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 26 '24 09:12 melvin-bot[bot]

Switched back to this

MonilBhavsar avatar Dec 27 '24 19:12 MonilBhavsar

Found the bug, working on a fix

MonilBhavsar avatar Dec 27 '24 19:12 MonilBhavsar

Got a PR up to add outputCurrency field in onyx response as it was missing. Also, reverting the PR https://github.com/Expensify/App/pull/53132 as it should fix the issue https://github.com/Expensify/App/issues/52190 without adding any unnecessary fallback logic

MonilBhavsar avatar Dec 27 '24 20:12 MonilBhavsar

Got both PR's up

MonilBhavsar avatar Dec 27 '24 21:12 MonilBhavsar

Nice one! Assigning @Ollyws, as he reviewed the app PR it looks like: https://github.com/Expensify/App/pull/54631

trjExpensify avatar Jan 03 '25 11:01 trjExpensify

This backend PR fixes the issue https://github.com/Expensify/App/pull/54631

https://github.com/Expensify/App/pull/54631 reverts the previous PR and cleans up the code that attempted to fix similar issue on client side, which is not recommended

MonilBhavsar avatar Jan 03 '25 14:01 MonilBhavsar

Got it, so are you saying we don't need to handle payment for the revert?

trjExpensify avatar Jan 03 '25 18:01 trjExpensify

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] avatar Jan 08 '25 02:01 melvin-bot[bot]

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.81-6 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

  • https://github.com/Expensify/App/pull/54631

If no regressions arise, payment will be issued on 2025-01-15. :confetti_ball:

For reference, here are some details about the assignees on this issue:

  • @Ollyws requires payment through NewDot Manual Requests

melvin-bot[bot] avatar Jan 08 '25 02:01 melvin-bot[bot]

@Ollyws @trjExpensify @Ollyws The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

melvin-bot[bot] avatar Jan 08 '25 02:01 melvin-bot[bot]