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.33k stars 2.76k forks source link

[HOLD for payment 2024-09-09][$250] Track tax - Tax rate is no longer assigned to the distance rate after changing the tax code #45861

Open lanitochka17 opened 1 month ago

lanitochka17 commented 1 month 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: 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:

  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Distance rates
  3. Click on any distance rate
  4. Click Tax rate
  5. Assign a tax rate
  6. Go to Taxes
  7. Click on the tax rate in Step 5
  8. Click Tax code
  9. Edit tax code and save it
  10. Go back to Distance rates
  11. Click on the distance rate from Step 3

Expected Result:

The tax rate will remain assigned to the distance rate after changing the tax code

Actual Result:

The tax rate is no longer assigned to the distance rate after changing the tax code

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/cac00e2f-c721-4526-9bfe-94feac12e510

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01487f230385609e0a
  • Upwork Job ID: 1815359905732178734
  • Last Price Increase: 2024-07-22
Issue OwnerCurrent Issue Owner: @abdulrahuman5196
melvin-bot[bot] commented 1 month ago

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

github-actions[bot] commented 1 month 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.
lanitochka17 commented 1 month ago

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

mountiny commented 1 month ago

Does this work in production or in OldDot? We might have to optimistically change the tax rate code in the distance rates to make this work, but I wonder if we do this in the backend even.

AS this applies for Control policies, I think we could treat this as NAB

mountiny commented 1 month ago

I dont think this is a blocker as its quite a minor case, but we should explore it, marking as external

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~01487f230385609e0a

melvin-bot[bot] commented 1 month ago

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

mountiny commented 1 month ago

Asked if this works well in prod or if this is a new feature

daledah commented 1 month ago

Proposal

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

The tax rate is no longer assigned to the distance rate after changing the tax code

What is the root cause of that problem?

When updating the tax code here https://github.com/Expensify/App/blob/b4c5ac57873b50dfe87bf9c28d4a41cba879d47c/src/libs/actions/TaxRate.ts#L486, we're not updating the taxRateExternalID of the distance rates (example usage of taxRateExternalID here https://github.com/Expensify/App/blob/b4c5ac57873b50dfe87bf9c28d4a41cba879d47c/src/pages/workspace/distanceRates/PolicyDistanceRateTaxRateEditPage.tsx#L30), so the taxRateExternalID of those distance rates remain the old code, which no longer exists.

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

In https://github.com/Expensify/App/blob/b4c5ac57873b50dfe87bf9c28d4a41cba879d47c/src/libs/actions/TaxRate.ts#L486, go through the list of policy distance rates, check if the taxRateExternalID of the distance rate is equal to the old tax code, if true we should create optimistic data to update that taxRateExternalID to the new tax code.

We also need to revert to the old taxRateExternalID in failureData

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 month ago

Current assignee @marcochavezf is eligible for the DeployBlockerCash assigner, not assigning anyone new.

github-actions[bot] commented 1 month 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.
mountiny commented 1 month ago

@daledah Can you please confirm which PR has caused this regression?

mountiny commented 1 month ago

QA confirmed this works well in production so given that (I thought its more of a new feature) we should fix this

daledah commented 1 month ago

which PR has caused this regression?

@mountiny It's this one https://github.com/Expensify/App/pull/43156

daledah commented 1 month ago

@mountiny I can raise a PR asap to fix this

mountiny commented 1 month ago

@daledah that PR is already in production though so some other change caused this issue cc @rushatgabhane @dangrous

daledah commented 1 month ago

@mountiny I only see the option to edit tax code in staging. In production, it doesn't display. The policy in both videos is control WS.

https://github.com/user-attachments/assets/1e44f66d-59eb-405d-91c2-c26bdb8f8b3e

https://github.com/user-attachments/assets/6898a092-f28c-431e-84bd-457e2767d747

mountiny commented 1 month ago

Huh, thats odd as the mentioned PR is now in production for sure

daledah commented 1 month ago

@mountiny I checked the production branch of the App and I also saw the tax code page is added but not sure why I can't see it in production website.

rushatgabhane commented 1 month ago

@mountiny @dangrous this should be handled on backend, right?

mountiny commented 1 month ago

@daledah, with your fix, if you sign out and sign back in, are the distance rates correctly updated to the new tax code?

I also feel like it might need a backend fix.

Its a new feature though so I dont thing we have to block app deploy on this

mountiny commented 1 month ago

Assigning Daniel (I hope thats ok) as you have been working on the project

daledah commented 1 month ago

with your fix, if you sign out and sign back in, are the distance rates correctly updated to the new tax code?

@mountiny Yes, my change will work in the offline cases and we also need the BE change to update data correctly in the database

dangrous commented 1 month ago

oh this is actually weirder than I expected. I'm writing a test that should help us fix this on the back end. I think we will need optimistic changes though right?

daledah commented 1 month ago

@dangrous Yes we will need optimistic changes in FE here as I mentioned in my proposal https://github.com/Expensify/App/issues/45861#issuecomment-2242836763. I can raise the PR to handle the FE change.

dangrous commented 1 month ago

backend fix is in review, then we can go from there.

dangrous commented 1 month ago

Backend PR still waiting on reviews, but hopefully we'll be able to get this moving this week.

dangrous commented 1 month ago

Okay backend fix is in production now. @rushatgabhane / @daledah can you check if the front end (non optimistic) is behaving as expected now, and if so, evaluate the proposal for updating optimistic data appropriately?

Thanks!

daledah commented 1 month ago

Okay backend fix is in production now. @rushatgabhane / @daledah can you check if the front end (non optimistic) is behaving as expected now

@dangrous The checked and the non optimistic data is correct now.

evaluate the proposal for updating optimistic data appropriately

I've had the proposal here . Can you please assign me here, thanks.

dangrous commented 1 month ago

@rushatgabhane can you review @daledah's proposal and we can go from there? Thanks!

daledah commented 1 month ago

@rushatgabhane friendly bump

rushatgabhane commented 1 month ago

@dangrous do we need to do this optimistically?

@daledah what happens when taxCode update fails, do we reset the optimistic data for distance rate?

Can a race condition happen here?

daledah commented 1 month ago

@rushatgabhane I mentioned the case when taxCode update fails in my proposal

We also need to revert to the old taxRateExternalID in failureData.

rushatgabhane commented 1 month ago

@dangrous let's hire @daledah for their proposal 🎀 👀 🎀

melvin-bot[bot] commented 1 month ago

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

dangrous commented 1 month ago

Assigned!

melvin-bot[bot] commented 1 month ago

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

daledah commented 1 month ago

@rushatgabhane This PR is ready for review.

daledah commented 3 weeks ago

friendly bump @rushatgabhane

dangrous commented 1 week ago

I think this was deployed to prod on 2024-09-02 but we should confirm and adjust payment timing as needed

melvin-bot[bot] commented 1 week ago

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

dangrous commented 1 week ago

Automation didn't fire but this seems to have been deployed on 9/2, so payment on 9/9. Grabbed you @garrettmknight for when payment is ready!

melvin-bot[bot] commented 5 days ago

Payment Summary

Upwork Job

BugZero Checklist (@garrettmknight)

JmillsExpensify commented 3 days ago

I went ahead and filled out the payment summary above, though repeating here for posterity:

garrettmknight commented 3 days ago

@daledah offer sent in Upwork (linking here)

garrettmknight commented 3 days ago

$250 approved for @rushatgabhane