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

[$500] Request money: Changing default currency for workspace is not refelected when requesting a money if there is previous unpaid money #27060

Closed
1 of 6 tasks
m-natarajan opened this issue Sep 8, 2023 · 66 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering 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

Comments

@m-natarajan
Copy link

m-natarajan commented Sep 8, 2023

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


Action Performed:

  1. Open the website
  2. Click the + icon from LHN
  3. Click request money
  4. Enter the amount and select a workspace from the list
  5. Confirm the requested money
  6. Observe the currency from the IOU report
  7. Go to Settings > workspace
  8. Select the workspace from the list which should be the same as the one money request that has been sent
  9. Click setting
  10. Change the default currency
  11. Do the same steps from 2 to 6
  12. The new default currency is not reflected in the IOU report

Expected Result:

A report not in the same currency as the workspace should be marked as a violation

Actual Result:

Nothing points out the different currency

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.66-3
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screenshare.-.2023-09-04.7_34_06.PM.1.mp4
Recording.427.mp4

Expensify/Expensify Issue URL:
Issue reported by: @misgana96
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693845090453449

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b90f99e542d09e16
  • Upwork Job ID: 1700249522279403520
  • Last Price Increase: 2023-11-15
@m-natarajan m-natarajan added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 8, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 8, 2023

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Sep 8, 2023

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

@melvin-bot melvin-bot bot changed the title Request money: Changing default currency for workspace is not refelected when requesting a money if there is previous unpaid money [$500] Request money: Changing default currency for workspace is not refelected when requesting a money if there is previous unpaid money Sep 8, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 8, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 8, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 8, 2023

Triggered auto assignment to @abekkala (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Sep 8, 2023

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

@jeet-dhandha
Copy link
Contributor

jeet-dhandha commented Sep 9, 2023

Seems like a BE change as mostly merging of money requests is done there.

@melvin-bot melvin-bot bot added the Overdue label Sep 11, 2023
@tienifr
Copy link
Contributor

tienifr commented Sep 11, 2023

Proposal

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

Request money: Changing default currency for workspace is not refelected when requesting a money if there is previous unpaid money

What is the root cause of that problem?

When open request money page, we reset the request money info

currency: lodashGet(currentUserPersonalDetails, 'localCurrencyCode', CONST.CURRENCY.USD),

and the currency is always the local currency instead of the currency of WS

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

If users open request money page from FAB button in LHN, we should show the local currency
But if they open request money page from plus button in chat, we should show the WS currency. In order to archive that in

App/src/libs/actions/IOU.js

Lines 1787 to 1790 in ef0eb33

function startMoneyRequest(iouType, reportID = '') {
resetMoneyRequestInfo(`${iouType}${reportID}`);
Navigation.navigate(ROUTES.getMoneyRequestRoute(iouType, reportID));
}

we get the report from reportID -> get the policyID from report -> get the policy from policyID -> get the outputCurrency from policy

function startMoneyRequest(iouType, reportID = '') {
    const report = allReports[`report_${reportID}`] || {}
    const policy = allPolicies[`policy_${report.policyID}`] || {}
    resetMoneyRequestInfo(`${iouType}${reportID}`, policy.outputCurrency);
    Navigation.navigate(ROUTES.getMoneyRequestRoute(iouType, reportID));
}

and in resetMoneyRequestInfo we pass the currency as the second argument. Then update currency here

currency: lodashGet(currentUserPersonalDetails, 'localCurrencyCode', CONST.CURRENCY.USD),

to

        currency: currency ? currency: lodashGet(currentUserPersonalDetails, 'localCurrencyCode', CONST.CURRENCY.USD),

We should do the same with SplitBill and SendMoney

Result

Screen.Recording.2023-09-11.at.17.29.37.mp4

@abekkala
Copy link
Contributor

@aimane-chnaif we've already received a proposal for this if you can give that a review!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Sep 11, 2023
@aimane-chnaif
Copy link
Contributor

aimane-chnaif commented Sep 14, 2023

@tienifr from your demo video, I don't see the main fix of this GH. Please check OP. It's not about default currency on request money modal.

Awaiting proposals

@melvin-bot melvin-bot bot removed the Overdue label Sep 14, 2023
@abekkala
Copy link
Contributor

@JmillsExpensify there is an issue occurring with double assignments happening after the initial Bug label. I'm unassigning myself as the second person.

@abekkala abekkala removed their assignment Sep 14, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 2023

📣 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 Sep 18, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

@JmillsExpensify, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot
Copy link

melvin-bot bot commented Sep 20, 2023

@JmillsExpensify, @aimane-chnaif Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot
Copy link

melvin-bot bot commented Sep 22, 2023

@JmillsExpensify @aimane-chnaif 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
Copy link

melvin-bot bot commented Sep 22, 2023

@JmillsExpensify, @aimane-chnaif Still overdue 6 days?! Let's take care of this!

@melvin-bot
Copy link

melvin-bot bot commented Sep 22, 2023

📣 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 removed the Overdue label Nov 24, 2023
@aldo-expensify
Copy link
Contributor

aldo-expensify commented Nov 24, 2023

@JmillsExpensify I think the labels didn't work here as expected. An Expensify engineer should have been assigned with Internal, right?

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Nov 24, 2023

@JmillsExpensify can you help me confirm that I'm understanding correctly the problem and the expected outcome?

I did this:

  1. Created a workspace (default currency USD)
  2. Created a money request in the policyExpenseChat
  3. I see the money request's preview in USD
  4. Change the currency of the workspace to CAD
  5. I see the money request's preview still showing in USD (wrong?)

Is the expectation that the UNPAID money requests update their previews to be shown in CAD?

I think the step of creating a second money request is irrelevant here.

@JmillsExpensify
Copy link

Is the expectation that the UNPAID money requests update their previews to be shown in CAD?

Yes, correct.

@aldo-expensify aldo-expensify self-assigned this Nov 24, 2023
@melvin-bot melvin-bot bot added the Overdue label Nov 27, 2023
@aldo-expensify
Copy link
Contributor

Working on this...

@melvin-bot melvin-bot bot removed the Overdue label Nov 27, 2023
@aldo-expensify
Copy link
Contributor

I noticed that in OldDot we also don't update the currency for Open/Processing reports, do we want this to also apply for workspaces that are not Free? (OldDot workspaces)

@JmillsExpensify
Copy link

I think we do.

@melvin-bot melvin-bot bot added the Overdue label Dec 1, 2023
@aldo-expensify
Copy link
Contributor

Started working on this here: https://github.com/Expensify/Auth/pull/9317

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Dec 1, 2023
@aldo-expensify
Copy link
Contributor

Working on this now.

@melvin-bot melvin-bot bot removed the Overdue label Dec 4, 2023
@aldo-expensify
Copy link
Contributor

Made PR ready for review: https://github.com/Expensify/Auth/pull/9317

@melvin-bot melvin-bot bot added the Overdue label Dec 7, 2023
@aldo-expensify
Copy link
Contributor

Verified that new reports are created with the default currency of the workspace

Considering the conversation here https://expensify.slack.com/archives/C03TQ48KC/p1701801905528039?thread_ts=1701750393.653449&cid=C03TQ48KC, I think the expected result of this issue is outdated. I'll chat with Carlos about how can we implement this in the form of a violation.

@melvin-bot melvin-bot bot removed the Overdue label Dec 7, 2023
@aldo-expensify
Copy link
Contributor

I'll have a look at the Violations design doc to decide how to implement this. Meanwhile... should create a new GH issue? or just update this one? I'm guessing the there will be a frontend and a backend part of this.

@melvin-bot melvin-bot bot added the Overdue label Dec 11, 2023
Copy link

melvin-bot bot commented Dec 11, 2023

@JmillsExpensify, @aldo-expensify, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

@aldo-expensify
Copy link
Contributor

I still haven't read the violations design doc, I'll get to it as soon as possible

@melvin-bot melvin-bot bot removed the Overdue label Dec 12, 2023
@JmillsExpensify
Copy link

All good, thanks!

@melvin-bot melvin-bot bot added the Overdue label Dec 15, 2023
@aldo-expensify
Copy link
Contributor

aldo-expensify commented Dec 15, 2023

Chatted with @JmillsExpensify about the priority of this issue, and we agreed with closing it.

My thoughts about it is that this is really a feature request and not a bug, since this wasn't supposed to work as the expected results describe, and at the moment, we are focusing in issues tied to waves.

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 Engineering 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
Projects
None yet
Development

No branches or pull requests

9 participants