[$250] Tax - Toggle becomes unlocked, Delete button appears briefly after saving tax code of default rate

[lanitochka17]

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: 9.0.10-2 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause -Internal Team

Action Performed:

Precondition:

• Workspace is under Control plan
• Taxes feature is enabled
  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Taxes
  3. Click on the default tax rate
  4. Click Tax code
  5. Edit the tax code and save it

Expected Result:

The toggle will remain locked and Delete button will not appear after saving the tax code of the default tax rate

Actual Result:

The toggle becomes unlocked and Delete button appears briefly after saving the tax code of the default tax rate

Workaround:

Unknown

Platforms:

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

• [x] Android: Native
• [x] Android: mWeb Chrome
• [x] iOS: Native
• [x] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/eb97f566-3f25-4d88-82f4-97ebcad424b3

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017da022f8b49cd8bf
• Upwork Job ID: 1815084149483432900
• Last Price Increase: 2024-07-21
• Automatic offers:
• ahmedGaber93 | Reviewer | 103216881

Issue Owner

Current Issue Owner: @ahmedGaber93

[melvin-bot[bot]]

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

[github-actions[bot]]

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

[lanitochka17]

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

[etCoderDysto]

Proposal

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

Tax - Toggle becomes unlocked, Delete button appears briefly after saving tax code of default rate

What is the root cause of that problem?

When updating tax code, defaultExternalID is storing the old value, while newTaxCode is updated optimistically to a new value. https://github.com/Expensify/App/blob/f3a8f736208166286e6f7da3562f1503d5ef2a3d/src/libs/actions/TaxRate.ts#L486-L495 Then canEditTaxRate returns true comparing the old defaultExternalID with the new tax code here https://github.com/Expensify/App/blob/f3a8f736208166286e6f7da3562f1503d5ef2a3d/src/libs/PolicyUtils.ts#L408-L409 Then shouldShowDeleteMenuItem becomes true we display delete button https://github.com/Expensify/App/blob/f3a8f736208166286e6f7da3562f1503d5ef2a3d/src/pages/workspace/taxes/WorkspaceEditTaxPage.tsx#L42-L44 Then BE udpates defaultExternalID with new value and delete button becomes hidden since the new defaultExternalID is equal to newTaxCode

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

We should set defaultExternalID to the new value in optimisticData

taxRates: {
  defaultExternalID: oldTaxCode === policy?.taxRates?.defaultExternalID ? newTaxCode : policy?.taxRates?.defaultExternalID,

https://github.com/Expensify/App/blob/f3a8f736208166286e6f7da3562f1503d5ef2a3d/src/libs/actions/TaxRate.ts#L494-L496 And add for failureData to

defaultExternalID:  policy?.taxRates?.defaultExternalID;

Note: We should apply the same pattern on foreignTaxDefault

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

Applying the same pattern on foreignTaxDefault, makes sure that Default badge is displayed on the default tax rate rather than Workspace currency default https://github.com/Expensify/App/blob/f3a8f736208166286e6f7da3562f1503d5ef2a3d/src/pages/workspace/taxes/WorkspaceTaxesPage.tsx#L82-L86

What alternative solutions did you explore? (Optional)

Result:

https://github.com/user-attachments/assets/11451ed3-9873-4db1-a9bf-78969a4e408e

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[NaveedShaikh78]

Contributor details Your Expensify account email: naveed.ajaj@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0147b6419f14a69c1b

[melvin-bot[bot]]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[codeapexdev]

Contributor details Your Expensify account email: akashftg@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/akashvashishtha

[melvin-bot[bot]]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[ahmedGaber93]

@etCoderDysto's proposal LGTM!

When we update the default tax rate code, We missed updating defaultExternalID in optimisticData and this causes the issue briefly until the backend data received. We also should do the same for the default foreign tax.

🎀 👀 🎀 C+ reviewed

[melvin-bot[bot]]

Current assignee @cead22 is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[cead22]

@ahmedGaber93 @etCoderDysto what PR was this introduced in / was this a regression?

[melvin-bot[bot]]

📣 @ahmedGaber93 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @etCoderDysto 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 📖

[etCoderDysto]

@ahmedGaber93 @etCoderDysto what PR was this introduced in / was this a regression?

The issue comes for this PR. The PR introduces a new feature where the user can edit tax code.

[etCoderDysto]

I will raise a PR ASAP.

[ahmedGaber93]

what PR was this introduced in / was this a regression?

@cead22 Yes it seems a regression from a new feature PR https://github.com/Expensify/App/pull/43156, should we continue to fix it here?

[ahmedGaber93]

@etCoderDysto please let's wait @cead22 decision here https://github.com/Expensify/App/issues/45858#issuecomment-2244652303 before create the PR

[cead22]

Let's follow our normal process for regressions cc @rushatgabhane @mollfpr @Gonals

[cead22]

@rushatgabhane can you please confirm this was caused by https://github.com/Expensify/App/pull/43156 and fix it?

[rushatgabhane]

@cead22 yes this is caused by the ^ PR

[rushatgabhane]

fix on the way

[rushatgabhane]

half way done.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@cead22, @mollfpr, @rushatgabhane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot[bot]]

@cead22, @mollfpr, @rushatgabhane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[cead22]

@rushatgabhane do you have an ETA? Just curious

[melvin-bot[bot]]

@cead22, @mollfpr, @rushatgabhane Eep! 4 days overdue now. Issues have feelings too...

[cead22]

@rushatgabhane how that PR coming along?

[melvin-bot[bot]]

@cead22, @mollfpr, @rushatgabhane Huh... This is 4 days overdue. Who can take care of this?

[cead22]

DM'ed @rushatgabhane on slack to ask for an update

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot[bot]]

This issue has not been updated in over 15 days. @cead22, @mollfpr, @rushatgabhane 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!