Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.34k stars 2.77k forks source link

[$250] Taxes - "Default" label changes to "Workspace currency default" after changing tax code #48088

Closed IuliiaHerets closed 1 week ago

IuliiaHerets commented 3 weeks ago

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


Version Number: v9.0.25-1 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: applausetester+kh050806@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Taxes.
  3. Click on the default tax code.
  4. Click Tax code.
  5. Edit tax code and save it.
  6. Note that "Default" label in the default tax code in the background changes to "Workspace currency default" briefly.

Expected Result:

"Default" label in the default tax code in the background will not change after changing tax code.

Actual Result:

"Default" label in the default tax code in the background changes to "Workspace currency default" briefly.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/f915175d-0c36-4b38-9717-d9dec3518008

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e55f0f655d87efe9
  • Upwork Job ID: 1828439699555129867
  • Last Price Increase: 2024-08-27
  • Automatic offers:
    • etCoderDysto | Contributor | 103702547
    • shubham1206agra | Contributor | 103702642
Issue OwnerCurrent Issue Owner: @akinwale
melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @OfstadC (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] commented 3 weeks ago

Triggered auto assignment to @MariaHCD (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 3 weeks ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
IuliiaHerets commented 3 weeks ago

We think that this bug might be related to #wave-control

etCoderDysto commented 3 weeks ago

Proposal

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

"Default" label changes to "Workspace currency default" after changing tax code

What is the root cause of that problem?

Here were are not setting foreignTaxDefault to the newTaxCode when editing tax code https://github.com/Expensify/App/blob/d5cfecfc53a2abf926a1dd9be8bc72c0b373ff81/src/libs/actions/TaxRate.ts#L486-L496 And here default badge is only displayed when taxID === defaultExternalID && taxID === foreignTaxDefault condition holds to be true. And it is not being true, since we are not updating foreignTaxDefault https://github.com/Expensify/App/blob/d5cfecfc53a2abf926a1dd9be8bc72c0b373ff81/src/pages/workspace/taxes/WorkspaceTaxesPage.tsx#L87-L91

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

We should update foreignTaxDefault with the new tax code in optimistic data here

foreignTaxDefault: oldTaxCode === policy?.taxRates?.foreignTaxDefault ? newTaxCode : policy?.taxRates?.foreignTaxDefault,

And set foreignTaxDefault to the existing foreignTaxDefault in failure data

foreignTaxDefault: policy?.taxRates?.foreignTaxDefault,

Also we shouldn't update defaultExternalID in successData since BE returns the updated value

What alternative solutions did you explore? (Optional)

etCoderDysto commented 3 weeks ago

Offending pr https://github.com/Expensify/App/pull/47819

cc: @rushatgabhane @cead22

MariaHCD commented 3 weeks ago

@etCoderDysto's proposal looks good to me πŸ‘πŸΌ

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

πŸ“£ @etCoderDysto πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer 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 πŸ“–

etCoderDysto commented 3 weeks ago

Thanks! I will raise a PR in an hour.

melvin-bot[bot] commented 3 weeks ago

πŸ“£ @shubham1206agra πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer 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 πŸ“–

MariaHCD commented 3 weeks ago

Assigning @shubham1206agra for C+ review

MariaHCD commented 3 weeks ago

Reassigned the C+ since this was a regression:

https://expensify.slack.com/archives/C02NK2DQWUX/p1724769162552359

MariaHCD commented 3 weeks ago

@etCoderDysto any updates on the PR just yet?

etCoderDysto commented 3 weeks ago

@etCoderDysto any updates on the PR just yet?

Sorry, I was on a road. I am working on the PR now. It will be ready for review under 30 minutes.

puneetlath commented 3 weeks ago

I confirmed this is fixed on staging. Thanks!

OfstadC commented 1 week ago

I think we are just waiting for this to be deployed to production in order to issue payment, but please correct me if I'm wrong.

etCoderDysto commented 1 week ago

@OfstadC this PR was cped on Aug 28. I think the automation is failing. It should be ready for payment.

OfstadC commented 1 week ago

Thanks for clarifying @etCoderDysto ! Sorry for the confusion here πŸ˜…

Payment Summary

shubham1206agra commented 1 week ago

@OfstadC I did not reviewed the PR. @rushatgabhane did that as it was a regression from his PR.

rushatgabhane commented 1 week ago

no c+ pay due here πŸ˜„

OfstadC commented 1 week ago

Ah okay perfect! Thanks for letting me know @shubham1206agra!