search

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.32k stars 2.75k forks source link

[HOLD for payment 2024-09-07] [$250] Taxes - Name field is no longer grayed out after editing tax code offline #47747

Closed IuliiaHerets closed 3 days 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.22-5 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Exp https://expensify.testrail.io/index.php?/tests/view/4874241 Email or phone of affected tester (no customers): 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. Go offline.
  4. Click on any tax rate.
  5. Click Name.
  6. Edit the name and save it.
  7. Click Tax code.
  8. Edit Tax code and save it.

Expected Result:

Name field will remain grayed out after editing tax code offline.

Actual Result:

Name field is no longer grayed out after editing tax code offline.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/096e04a3-79a3-46e9-9c65-a91e1e7cf762

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01da26942e57c47d6b
  • Upwork Job ID: 1826075814524858613
  • Last Price Increase: 2024-08-21
Issue OwnerCurrent Issue Owner: @kadiealexander
melvin-bot[bot] commented 3 weeks ago

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

IuliiaHerets commented 3 weeks ago

We think that this bug might be related to #wave-collect - Release 1

IuliiaHerets commented 3 weeks ago

@kadiealexander FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

Krishna2323 commented 3 weeks ago

Edited by proposal-police: This proposal was edited at 2024-08-20 20:40:06 UTC.

Proposal

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

Taxes - Name field is no longer grayed out after editing tax code offline

What is the root cause of that problem?

Current pending field are not spread when updating the tax code. When updating tax code we create a key/value pair with the new tax code name so the pending fields are not merged in this case like it does when updating name or value, that's why we don't need to spread the pending fields when updating name or value.

https://github.com/Expensify/App/blob/b2eb922823d491996faa5d50275bfce540e9b288/src/libs/actions/TaxRate.ts#L498-L503

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

Spread the pending fields. We should also check for other update actions for tax details edit.

EDIT: We don't need to do that when editing name or value.

What alternative solutions did you explore? (Optional)



Result

https://github.com/user-attachments/assets/c75a04de-8dea-43d1-997b-9858f86ee627

abzokhattab commented 3 weeks ago

Edited by proposal-police: This proposal was edited at 2024-08-20 21:06:07 UTC.

Proposal

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

Taxes - Name field is no longer grayed out after editing tax code offline

What is the root cause of that problem?

when updating the tax code it overrides the existing pending fields: https://github.com/Expensify/App/blob/b2eb922823d491996faa5d50275bfce540e9b288/src/libs/actions/TaxRate.ts#L496C21-L525C27

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

we should add the existing pending field from the originalTaxRate.pendingFields into the pending fields object... same applies to the success and the failure data

To do that we should change the pending fields property in the optimistic values to:

pendingFields: {...(originalTaxRate?.pendingFields ?? {}), code: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE},

also, modify that in the success and failure objects

What alternative solutions did you explore? (Optional)

N.A

Krishna2323 commented 3 weeks ago

Proposal Updated

Krishna2323 commented 3 weeks ago

Proposal Updated

abzokhattab commented 3 weeks ago

Proposal Updated

Added the code implementation

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

c3024 commented 3 weeks ago

@Krishna2323 's first posted proposal comment shows the offending code block correctly in the RCA and the suggested solution is clear enough. I think we can proceed with his proposal because his is first.

🎀 👀 🎀 C+ Reviewed

melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @mountiny, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

Krishna2323 commented 2 weeks ago

@mountiny, bump for assignment if you agree with @c3024's decision.

mountiny commented 2 weeks ago

Go ahead please @Krishna2323

Krishna2323 commented 2 weeks ago

@c3024, PR ready for review ^

c3024 commented 1 week ago

Automation failed here. This was deployed to production yesterday. Payment due on 7-Sep or 8-Sep.

cc: @kadiealexander

mountiny commented 1 week ago

Updated

$250 to @Krishna2323 and to @c3024

kadiealexander commented 1 week ago

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:

melvin-bot[bot] commented 5 days ago

Payment Summary

Upwork Job

BugZero Checklist (@kadiealexander)

c3024 commented 4 days ago

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:

  • [x] [@c3024] The PR that introduced the bug has been identified. Link to the PR: #43156
  • [x] [@c3024] 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: https://github.com/Expensify/App/commit/bee9d44584a82ad6efa1b689f1aac65b9884b387#r146398000
  • [x] [@c3024] 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: No discussion was started because this could not have identified earlier.
  • [x] [@c3024] Determine if we should create a regression test for this bug. Yes
  • [x] [@c3024] 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.

Regression Test Proposal

  1. Go to workspace settings > Taxes.
  2. Go offline.
  3. Click on any tax rate.
  4. Click on Name.
  5. Edit the name and save it.
  6. Click on Tax code.
  7. Edit the Tax code and save it.
  8. Verify that the name field remains greyed out.
kadiealexander commented 4 days ago

Not overdue, offers sent!

Krishna2323 commented 4 days ago

Offer accepted.