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

[HOLD for payment 2023-05-22] [$1000] The placeholder of rate doesn't dynamically update on one platform when default currency is changed from the other platform (works well with the rate input and the unit) #18206

Closed
1 of 6 tasks
kavimuru opened this issue Apr 30, 2023 · 35 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering 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

Comments

@kavimuru
Copy link

kavimuru commented Apr 30, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open web chrome and login with User A (Let's called it as Platform 1)
  2. Go to any workspace
  3. Go to reimburse expenses
  4. Now login with the same User A on another web chrome window or android (Platform 2) and go to the same workspace > reimburse expenses
  5. From Platform 1, try to change the value in the Unit dropdown or try to update the value in the rate field
  6. On Platform 2 , if you notice , the unit field and the input value in the rate field updates properly
  7. But now, on Platform 1 , try to change the default currency to some other values in general settings of the workspace
  8. Now go to Platform 2 , in the rate field , delete all the values and notice that it still shows the old currency and not the updated one in the placeholder
  9. Thus, in the same page, if we try to input values in rate field and if we try to change the option in unit field , they update dynamically without problem, but changing the currency on one platform and trying to view it from the other platform in the placeholder of the rate field doesn't updates dynamically

Expected Result:

The currency should be updated dynamically in the placeholder of the rate field if the default currency is changed from the other platform in the general settings

Actual Result:

The currency is not updated dynamically in the placeholder of the rate field if the default currency is changed from the other platform in the general settings

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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.8.8
Reproducible in staging?: y
Reproducible in production?: y
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

error-2023-04-29_18.30.58.mp4
Recording.421.mp4

Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1682772795937269

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015c0176549b561f82
  • Upwork Job ID: 1653946271666044929
  • Last Price Increase: 2023-05-04
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 30, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

MelvinBot commented Apr 30, 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

@Prince-Mendiratta
Copy link
Contributor

Proposal

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

In this issue, we can notice that the currency code placeholder for the unit does not update when the default currency for a workspace is modified.

What is the root cause of that problem?

In the WorkspaceReimburseView, we set the outputCurrency for the policy in the state. When the default currency changes, we however do not update the state with the new outputCurrency, which leads to the currency code not updating.

outputCurrency: lodashGet(props, 'policy.outputCurrency', ''),

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

I do not see any reason why we should keep the outputCurrency in the state. We can get rid of the key from state and instead directly pass the value using the props. We need to use lodashGet directly to set the placeholder value.

placeholder={this.state.outputCurrency}

This will ensure that if the default currency updates, the placeholder value will do so as well.

What alternative solutions did you explore? (Optional)

If we still want to keep the outputCurrency in the state, we will need to add a new condition in the componentDidUpdate such that if prevProps.policy.outputCurrency is not equal to this.props.policy.outputCurrency, then we update the state with the new outputCurrency value.

@sophiepintoraetz
Copy link
Contributor

Yup - I'm able to recreate this!
2023-05-01_17-20-08 (1)

@MelvinBot
Copy link

Triggered auto assignment to @roryabraham (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@melvin-bot melvin-bot bot added the Overdue label May 3, 2023
@sophiepintoraetz
Copy link
Contributor

Bump @roryabraham!

@melvin-bot melvin-bot bot removed the Overdue label May 4, 2023
@roryabraham roryabraham added the External Added to denote the issue can be worked on by a contributor label May 4, 2023
@melvin-bot melvin-bot bot changed the title The placeholder of rate doesn't dynamically update on one platform when default currency is changed from the other platform (works well with the rate input and the unit) [$1000] The placeholder of rate doesn't dynamically update on one platform when default currency is changed from the other platform (works well with the rate input and the unit) May 4, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

Current assignee @sophiepintoraetz is eligible for the External assigner, not assigning anyone new.

@MelvinBot
Copy link

📣 @Prince-Mendiratta You have been assigned to this job by @roryabraham!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@MelvinBot
Copy link

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

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

Current assignee @roryabraham is eligible for the External assigner, not assigning anyone new.

@roryabraham
Copy link
Contributor

roryabraham commented May 4, 2023

@Prince-Mendiratta totally agree, there's no reason outputCurrency needs to be in state. My only feedback is that instead of defaulting to '' for the outputCurrency, we should default to CONST.CURRENCY.USD

@Prince-Mendiratta
Copy link
Contributor

PR is up for review!

cc @roryabraham @fedirjh

@melvin-bot
Copy link

melvin-bot bot commented May 11, 2023

@fedirjh, @sophiepintoraetz, @roryabraham, @Prince-Mendiratta Whoops! This issue is 2 days overdue. Let's get this updated quick!

@fedirjh
Copy link
Contributor

fedirjh commented May 11, 2023

PR was deployed to staging

@melvin-bot melvin-bot bot removed the Daily KSv2 label May 15, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 15, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented May 15, 2023

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

If no regressions arise, payment will be issued on 2023-05-22. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented May 15, 2023

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
  • [@fedirjh] 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:
  • [@fedirjh] 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:
  • [@fedirjh] Determine if we should create a regression test for this bug.
  • [@fedirjh] 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.
  • [@sophiepintoraetz] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@Prince-Mendiratta
Copy link
Contributor

Sharing the timeline of this issue to help with the eventual analysis, calculated with this tool.

🐛 Issue created at: Sun, 30 Apr 2023 04:30:29 GMT
🧯 Help Wanted at: Thu, 04 May 2023 02:15:10 GMT
🕵🏻 Contributor assigned at: Thu, 04 May 2023 02:15:03 GMT
🛸 PR created at: Thu, 04 May 2023 13:09:23 GMT
🎯 PR merged at: Tue, 09 May 2023 03:36:19 GMT
⌛ Business Days Elapsed between assignment and PR merge: 3

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels May 21, 2023
@sophiepintoraetz
Copy link
Contributor

All right, I believe the payments are as follows:
@priya-za = $250
@Prince-Mendiratta = $1500
@fedirjh = $1500 (which I can pay out once you have completed these steps here)

@sophiepintoraetz
Copy link
Contributor

Offers are here.

@apollo-man

This comment was marked as spam.

@melvin-bot
Copy link

melvin-bot bot commented May 22, 2023

📣 @apollo-man! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@sophiepintoraetz sophiepintoraetz removed the Bug Something is broken. Auto assigns a BugZero manager. label May 22, 2023
@sophiepintoraetz sophiepintoraetz removed their assignment May 22, 2023
@sophiepintoraetz sophiepintoraetz added the Bug Something is broken. Auto assigns a BugZero manager. label May 22, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 22, 2023

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

@melvin-bot

This comment was marked as duplicate.

@sophiepintoraetz
Copy link
Contributor

sophiepintoraetz commented May 22, 2023

@miljakljajic - all you have to do is hit the payment for the three contributors (job link) - I've made the offers (I'm about to go OOO for the afternoon/week) and just waiting on them to accept/ C+ complete the bug checklist review.

@sophiepintoraetz sophiepintoraetz self-assigned this May 22, 2023
@priya-zha
Copy link

@sophiepintoraetz Accepted the offer. Thanks

@miljakljajic
Copy link
Contributor

Has everyone been paid? I made two payments but I'm not seeing the option to pay @Prince-Mendiratta - has payment already been sent?

@miljakljajic
Copy link
Contributor

I am also OOO today travelling back from EC3 so @roryabraham if any additional payments still need to be made, can you re-assign the BZ label?

@Prince-Mendiratta
Copy link
Contributor

@miljakljajic I haven't received the contract yet, so payment is pending for me.

@fedirjh
Copy link
Contributor

fedirjh commented May 23, 2023

  1. Link to the PR: Add new capability to set tracking rate and unit #6839
  2. Link to comment: Not sure , the PR is very old ( it's 20 months old )
  3. Link to discussion: https://expensify.slack.com/archives/C049HHMV9SM/p1684885698137059
  4. Determine if we should create a regression test for this bug: ❌ IT feels like an extreme case , where the user have to simultaneously open the app on two different devices and update the currency at the same time. IMO it doesn’t seems to be a real use case.

@melvin-bot melvin-bot bot added the Overdue label May 26, 2023
@sophiepintoraetz
Copy link
Contributor

sophiepintoraetz commented May 29, 2023

@Prince-Mendiratta - you need to accept the offer in order to initiate payment. The offer was made on 22 May - please check your emails from Upwork.
image

@melvin-bot melvin-bot bot removed the Overdue label May 29, 2023
@Prince-Mendiratta
Copy link
Contributor

@sophiepintoraetz woops, that's the wrong Prince you sent the contract to. This is my upwork profile, kindly send a contract to this one - https://www.upwork.com/freelancers/~013fecfd3cf5741ae0

@sophiepintoraetz
Copy link
Contributor

Ah, that will be where we're going wrong! offer resent!

@sophiepintoraetz
Copy link
Contributor

All set here!
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering 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
Projects
None yet
Development

No branches or pull requests

9 participants