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] Workspace - Default workspace currency is changed to USD when changing a workspace name #53840

Open
2 of 8 tasks
IuliiaHerets opened this issue Dec 10, 2024 · 24 comments
Open
2 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor 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 Reviewing Has a PR in review

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Dec 10, 2024

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
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6689882_1733826538548.2024-12-10_13_23_39.mp4

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-24
Issue OwnerCurrent Issue Owner: @MonilBhavsar
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 10, 2024
Copy link

melvin-bot bot commented Dec 10, 2024

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.

@bernhardoj
Copy link
Contributor

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 #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.

const currency = currencyValue ?? policy?.outputCurrency ?? CONST.CURRENCY.USD;

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 #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.

@trjExpensify
Copy link
Contributor

So strange, I can repro this.

2024-12-10_13-03-22.mp4

@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 trjExpensify added the External Added to denote the issue can be worked on by a contributor label Dec 10, 2024
@melvin-bot melvin-bot bot changed the title Workspace - Default workspace currency is changed to USD when changing a workspace name [$250] Workspace - Default workspace currency is changed to USD when changing a workspace name Dec 10, 2024
Copy link

melvin-bot bot commented Dec 10, 2024

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

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

melvin-bot bot commented Dec 10, 2024

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

@MonilBhavsar
Copy link
Contributor

I don't recall working on this 😅 but happy to take a look

@MonilBhavsar MonilBhavsar self-assigned this Dec 10, 2024
@trjExpensify
Copy link
Contributor

I think it was an update to the UpdateGeneralWorkspaceSettings function.

@Pujan92
Copy link
Contributor

Pujan92 commented Dec 11, 2024

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

Copy link

melvin-bot bot commented Dec 17, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Dec 17, 2024
@trjExpensify
Copy link
Contributor

@MonilBhavsar thoughts here?

Copy link

melvin-bot bot commented Dec 17, 2024

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

@MonilBhavsar MonilBhavsar added the Internal Requires API changes or must be handled by Expensify staff label Dec 17, 2024
@MonilBhavsar
Copy link
Contributor

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

Copy link

melvin-bot bot commented Dec 19, 2024

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

@MonilBhavsar
Copy link
Contributor

Looking tomorrow

@melvin-bot melvin-bot bot removed the Overdue label Dec 19, 2024
@MonilBhavsar
Copy link
Contributor

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

@trjExpensify
Copy link
Contributor

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

@MonilBhavsar
Copy link
Contributor

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

Copy link

melvin-bot bot commented Dec 24, 2024

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

Copy link

melvin-bot bot commented Dec 24, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Dec 25, 2024
Copy link

melvin-bot bot commented Dec 26, 2024

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

@MonilBhavsar
Copy link
Contributor

Switched back to this

@melvin-bot melvin-bot bot removed the Overdue label Dec 27, 2024
@MonilBhavsar
Copy link
Contributor

Found the bug, working on a fix

@MonilBhavsar
Copy link
Contributor

MonilBhavsar commented Dec 27, 2024

Got a PR up to add outputCurrency field in onyx response as it was missing.
Also, reverting the PR #53132 as it should fix the issue #52190 without adding any unnecessary fallback logic

@MonilBhavsar
Copy link
Contributor

Got both PR's up

@MonilBhavsar MonilBhavsar added the Reviewing Has a PR in review label Dec 27, 2024
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 External Added to denote the issue can be worked on by a contributor 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 Reviewing Has a PR in review
Projects
Status: Bugs and Follow Up Issues
Development

No branches or pull requests

5 participants