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] Inconsistent Currency Code Representation for 'MRO' in LHN #46392

Closed
1 of 6 tasks
lanitochka17 opened this issue Jul 28, 2024 · 36 comments
Closed
1 of 6 tasks

[$250] Inconsistent Currency Code Representation for 'MRO' in LHN #46392

lanitochka17 opened this issue Jul 28, 2024 · 36 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Jul 28, 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: 9.0.13-3
Reproducible in staging?: Y
Reproducible in production?: Y
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:

  1. Go to Account Settings > Workspace
  2. Change the default currency to 'MRO' in Profile
  3. In the LHN, go to Workspace Chat > Plus Sign > Expense Request
  4. Add a merchant and confirm it
  5. Go to IOU and observe the LHN

Expected Result:

The currency code should be consistently represented as 'MRO' throughout the app

Actual Result:

The currency code is inconsistently represented as 'UM' instead of 'MRO' when submitting an expense and viewing the IOU in LHN

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

Bug6551142_1721786074503.Screen_Recording_2024-07-23_at_6.31.01_PM.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019389c6893f847c12
  • Upwork Job ID: 1817710932397878288
  • Last Price Increase: 2024-08-04
Issue OwnerCurrent Issue Owner: @mollfpr
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 28, 2024
Copy link

melvin-bot bot commented Jul 28, 2024

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

@lanitochka17
Copy link
Author

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

@jliexpensify jliexpensify added the External Added to denote the issue can be worked on by a contributor label Jul 28, 2024
@melvin-bot melvin-bot bot changed the title Inconsistent Currency Code Representation for 'MRO' in LHN [$250] Inconsistent Currency Code Representation for 'MRO' in LHN Jul 28, 2024
Copy link

melvin-bot bot commented Jul 28, 2024

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

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

melvin-bot bot commented Jul 28, 2024

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

@daledah
Copy link
Contributor

daledah commented Jul 29, 2024

Proposal

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

The currency code is inconsistently represented as 'UM' instead of 'MRO' when submitting an expense and viewing the IOU in LHN

What is the root cause of that problem?

The polifill for Intl.NumberFormat we're using does not have the correct symbol value of MRO, example in https://github.com/formatjs/formatjs/blob/f71ce48aaf8ada6a77d393d3f2293fc14f2c38c2/packages/intl-numberformat/test262-main.ts#L1019.

Perhaps it's because MRO is a retired currency and no longer in use since 2018, so no longer actively supported. Meanwhile back-end library still returns currency symbol as MU, hence the inconsistency.

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

Remove the retired currencies from the list of currencies in the app, namely here https://github.com/Expensify/App/blob/main/tests/unit/currencyList.json#L1 and here

App/src/CONST.ts

Line 4512 in 98d8a5a

"AED": {
because no user will use it (or should be allowed to use it)

Or, do not remove the currency completely but filtering out the retired currencies when displaying select options for the user (like in the currency list for submitting expense), this will allow user to view retired currencies in legacy reports/transactions, but not able to create new reports/transactions with retired currencies.

We can also hide it only if the retired currency is not currently selected (if it's already selected in previous requests, the user should be able to view the (maybe disabled) selection when clicking on the currency button, if the user selects another currency however, they won't be able to select the retired currency again.

What alternative solutions did you explore? (Optional)

  • Fix the symbol value in the @formatjs/intl-numberformat (there and in other places where symbol is set to MRO) to the correct value UM then update the lib version in our app, or we can create a patch. This seems to happen to other similar currencies too, such as "MRU" so might worth updating them too
  • In
    const initPolyfill = () => {};
    , init the same polyfill as in iOS , because we'd need the polyfill for all platforms for the above currency cases

Another solution could be that: Replace the currency formatting logic in the front-end by whatever is used in the back-end, so they are consistent.

Copy link

melvin-bot bot commented Aug 1, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Aug 1, 2024
@mollfpr
Copy link
Contributor

mollfpr commented Aug 1, 2024

Will review the proposal in the morning!

@melvin-bot melvin-bot bot removed the Overdue label Aug 1, 2024
@jliexpensify
Copy link
Contributor

Thank you @mollfpr

Copy link

melvin-bot bot commented Aug 4, 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 Aug 4, 2024
@mollfpr
Copy link
Contributor

mollfpr commented Aug 5, 2024

This issue is on the BE where it returns the lastMessageHtml or lastMessageText with UM currency. As mentioned in the proposal the currency MRU is also set as UM in the LHN's last message but the exchange is different between MRO and MRU.

@jliexpensify We might need an internal engineer to check why the report sends a different currency for the lastMessageHtml and lastMessageText values.

@melvin-bot melvin-bot bot removed the Overdue label Aug 5, 2024
@daledah
Copy link
Contributor

daledah commented Aug 5, 2024

@mollfpr The back-end returned data is correct. UM is the symbol representation of MRO (the code here).

UM in lastMessageHtml or lastMessageText is correct, just like $ is used in those places instead of USD.

The bug is in the front-end as explained in my proposal, it should show UM throughout just like $ is shown around the app.

@mollfpr
Copy link
Contributor

mollfpr commented Aug 5, 2024

Okay, I understand now. It's correct that MRO is obsolete.

Screenshot 2024-08-05 at 19 40 53

The currency code is inconsistently represented as 'UM' instead of 'MRO' when submitting an expense and viewing the IOU in LHN

@daledah @jliexpensify I think the actual result is expected where UM is a symbol representing currency MRO.

@jliexpensify
Copy link
Contributor

Thanks for the investigation @mollfpr - if MRO is now obsolete, could we just change it to MRU or even UM in-product? I think if we can get rid of the inconsistency, that would be good.

if it's not something that can be fixed, then we can close this one.

@daledah
Copy link
Contributor

daledah commented Aug 6, 2024

I think if we can get rid of the inconsistency, that would be good.

@jliexpensify The inconsistency can be fixed in the front-end using my proposal (both main and alternative solution)

@mollfpr Could you kindly review the proposal? Thx!

@mollfpr
Copy link
Contributor

mollfpr commented Aug 6, 2024

if MRO is now obsolete, could we just change it to MRU or even UM in-product? I think if we can get rid of the inconsistency, that would be good.

@jliexpensify We can filter out the obsolete currencies mentioned in the @daledah proposal.

@jliexpensify
Copy link
Contributor

Ok, I think we could probably fix this then? @mollfpr want to push it forward and we'll get an Internal Engineer to make the final decision?

@mollfpr
Copy link
Contributor

mollfpr commented Aug 7, 2024

The actual issue here is expected that UM is the symbol for MRO currency, but it's mentioned in the @daledah proposal we have obsolete currency (including MRO which replace by MRU) I think it's a better chance to filter them out from the list.

🎀 👀 🎀 C+ reviewed!

Copy link

melvin-bot bot commented Aug 11, 2024

@youssef-lr @mollfpr @jliexpensify 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 melvin-bot bot added the Overdue label Aug 11, 2024
@jliexpensify
Copy link
Contributor

Bumping @youssef-lr to review proposal, thanks!

Copy link

melvin-bot bot commented Aug 12, 2024

@youssef-lr, @mollfpr, @jliexpensify Eep! 4 days overdue now. Issues have feelings too...

@jliexpensify
Copy link
Contributor

Bumping @youssef-lr to review the proposal, cheers!

Copy link

melvin-bot bot commented Aug 14, 2024

@youssef-lr, @mollfpr, @jliexpensify Still overdue 6 days?! Let's take care of this!

@jliexpensify
Copy link
Contributor

DM-ed Youssef for a review.

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

melvin-bot bot commented Aug 15, 2024

📣 @daledah You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot removed the Overdue label Aug 15, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 19, 2024
@daledah
Copy link
Contributor

daledah commented Aug 19, 2024

@mollfpr This PR is ready for review.

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Sep 11, 2024
Copy link

melvin-bot bot commented Sep 11, 2024

This issue has not been updated in over 15 days. @youssef-lr, @mollfpr, @jliexpensify, @daledah eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@jliexpensify
Copy link
Contributor

Checked PR, this is deployed to staging but not to prod.

@mollfpr
Copy link
Contributor

mollfpr commented Sep 18, 2024

@jliexpensify The PR is deployed to production 3 weeks ago. I think Melvin failed to send the notification. #48035

Screenshot 2024-09-18 at 15 22 50

@mollfpr
Copy link
Contributor

mollfpr commented Sep 18, 2024

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

No offending PR.

[@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

The regression step should be enough.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

  1. Select FAB > Submit expense
    2 .Open Select currency list
  2. Verify that: Retired currencies (such as MRO) is not shown anymore
  3. 👍 or 👎

@jliexpensify
Copy link
Contributor

Dang ok - thanks @mollfpr!

@jliexpensify
Copy link
Contributor

jliexpensify commented Sep 18, 2024

Payment Summary

  • C: @daledah $250 - can you please add your Upworks name to your GH profile so I can hire you?
  • C+: @mollfpr $250 (ND Payment)

Upworks job - https://www.upwork.com/jobs/~021836326350630574505

@daledah
Copy link
Contributor

daledah commented Sep 18, 2024

Hey @jliexpensify here's my Upwork profile link https://www.upwork.com/freelancers/~0138d999529f34d33f

TIA

@jliexpensify
Copy link
Contributor

Thanks @daledah - offer sent

@garrettmknight
Copy link
Contributor

$250 approved for @mollfpr

@jliexpensify
Copy link
Contributor

Paid and job closed!

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 Monthly KSv2 Reviewing Has a PR in review
Projects
No open projects
Status: Done
Development

No branches or pull requests

6 participants