[HOLD for payment 2024-06-13] [$250] mWeb - Tax - Saving tax name without altering displays error message

[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: 1.4.75
Reproducible in staging?: Y
Reproducible in production?: Y
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:

1. Go to https://staging.new.expensify.com/home
2. Tap on profile -- workspaces -- workspace
3. Tap more features
4. Toggle on taxes
5. Navigate to workspace settings and tap taxes
6. Tap add rate
7. Enter a tax name and value
8. Save it
9. Tap on new tax rate created
10. Tap on tax name
11. Without altering tax name, save it
12. Note error shown
13. Navigate back and note error displayed under new tax rate created

Expected Result:

When user saves without altering tax name, it must not display error message
Under new tax rate created error message must not be shown

Actual Result:

When user saves without altering tax name, it displays error message " An error occurred while updating tax rate. Please try again or ask concierge for help. " Under new tax rate created error message " Duplicate tax name" is shown

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6489742_1716489237727.tax.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f750a4457ca441b3
• Upwork Job ID: 1795375303903567872
• Last Price Increase: 2024-05-28
• Automatic offers:
• rojiphil | Reviewer | 102532639
• GandalfGwaihir | Contributor | 102532640

Issue Owner

Current Issue Owner: @laurenreidexpensify

[allgandalf]

Proposal

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

Saving tax name with no edit throws error

What is the root cause of that problem?

We currently do not have any check in place to check if the new tag name is same as current tag name, we directly call renamePolicyTax function.

App/src/pages/workspace/taxes/NamePage.tsx

Lines 47 to 50 in 791058a

	const submit = () => {
	renamePolicyTax(policyID, taxID, name);
	goBack();
	};

Then when we make this request to the BE it returns error of Duplicate Name

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

We should goBack and return early if there is no change between the current tag name and new tag name, we can get the current tag name from currentTaxRate?.name:

const submit = () => {
        if(name === currentTaxRate?.name)
        {
            goBack();
            return;
        }
        renamePolicyTax(policyID, taxID, name);
        goBack();
    };

What alternative solutions did you explore? (Optional)

[allgandalf]

@lanitochka17 , i guess automation didn't work, can you add those labels again ?

[melvin-bot]

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

[lanitochka17]

@laurenreidexpensify 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

[cretadn22]

Proposal

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

The API update continues to be triggered even when we update with the old value. This issue also persists on the tax value page.

What is the root cause of that problem?

App/src/pages/workspace/taxes/NamePage.tsx

Lines 47 to 50 in c19eea6

	const submit = () => {
	renamePolicyTax(policyID, taxID, name);
	goBack();
	};

App/src/pages/workspace/taxes/ValuePage.tsx

Lines 43 to 47 in c19eea6

	const submit = useCallback(
	(values: FormOnyxValues<typeof ONYXKEYS.FORMS.WORKSPACE_TAX_VALUE_FORM>) => {
	updatePolicyTaxValue(policyID, taxID, Number(values.value));
	goBack();
	},

In these instances, we don't check if the updated value is similar to the old value and thus do not prevent updating.

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

We should implement the same logic as we did in the EditTagPage.

This means applying it to the Name page and Value page as well.
For the Name page, we need to trim value before checking

const taxName = name.trim();
// Do not call the API if the edited tax name is the same as the current tag name
if (currentTaxRate?.name !== taxName) {
     renamePolicyTax(policyID, taxID, taxName);
}
Keyboard.dismiss();
goBack();

For the Value page:

const submit = useCallback(
        (values: FormOnyxValues<typeof ONYXKEYS.FORMS.WORKSPACE_TAX_VALUE_FORM>) => {
        // Do not call the API if the edited tax name is the same as the current tag name
        if (defaultValue !== values.value) {
            updatePolicyTaxValue(policyID, taxID, Number(values.value));
        }
        Keyboard.dismiss();
        goBack();
        },
        [goBack, policyID, taxID],
    );

We're considering adding Keyboard.dismiss(); to enhance the user experience on mobile devices

Optional: We should also address the same issue on other pages if it exists like PolicyDistanceRateEditPage

App/src/pages/workspace/distanceRates/PolicyDistanceRateEditPage.tsx

Lines 46 to 50 in c19eea6

	const submitRate = (values: FormOnyxValues<typeof ONYXKEYS.FORMS.POLICY_DISTANCE_RATE_EDIT_FORM>) => {
	Policy.updatePolicyDistanceRateValue(policyID, customUnit, [{...rate, rate: Number(values.rate) * CONST.POLICY.CUSTOM_UNIT_RATE_BASE_OFFSET}]);
	Keyboard.dismiss();
	Navigation.goBack();
	};

What alternative solutions did you explore? (Optional)

NA

[allgandalf]

We're considering adding Keyboard.dismiss(); to enhance the user experience on mobile devices

Keyboard is already dismissed on Native applications @cretadn22 :)

RPReplay_Final1716615262.mp4

For the Value page:

And the value page doesn’t have a keyboard popup as it is a numpad input.

[cretadn22]

@GandalfGwaihir, I recommend adding Keyboard.dismiss() because it aligns with our existing pattern of usage in scenarios like editing tags, categories, distance rates, and others. It's worth delving deeper to clarify why this function is necessary for consistency across our application.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[laurenreidexpensify]

Calm down melvin, not overdue, just waiting for proposals and review

[laurenreidexpensify]

@rojiphil bump for review ^^ thanks

[rojiphil]

Thanks for your proposals. Both proposals have got the RCA correct.
Regarding implementation, it is clear that we have to prevent the API request if the value has not changed. And both proposals have mentioned this.

@cretadn22 additionally proposed to trim the value but that is just a minor change and could have been identified during PR review. Calling Keyboard.dismiss() is not required here as there is no value-add and is not fixing any bug. Implementing the same logic to Value page is also not required as setting the same value is not an error in BE and does not result in a bug. Because of this, I don't find a good basis to give preference to this proposal.

@GandalfGwaihir proposal LGTM as that was the first one to get to the correct RCA and solution
🎀👀🎀 C+ reviewed

[melvin-bot]

📣 @rojiphil 🎉 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]

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

[allgandalf]

PR would be ready by EOD 👍

[allgandalf]

Android build is failing on main 😞 we have active slack convo too, if this doesn't get resolved by today i will still put up the PR on all other platforms except android native

[allgandalf]

Android finally builds 🥳 , PR ready for review c.c. @rojiphil

[melvin-bot]

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

[melvin-bot]

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

• [Fix]: Saving tax name without altering displays error message #42966

If no regressions arise, payment will be issued on 2024-06-13. 🎊

For reference, here are some details about the assignees on this issue:

• @rojiphil requires payment automatic offer (Reviewer)
• @allgandalf requires payment (Needs manual offer from BZ)

[melvin-bot]

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:

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

[laurenreidexpensify]

@allgandalf just to confirm - are you Rohan in Upwork ?

[allgandalf]

Yes @laurenreidexpensify , that is right 👍

[laurenreidexpensify]

Payment Summary:

• C+ $250 @rojiphil please accept offer in Upwork
• Contributor $250 @allgandalf paid via Upwork

@rojiphil pls confirm checklist and regression steps above thanks

[rojiphil]

• [@rojiphil] The PR that introduced the bug has been identified. Link to the PR: Offending PR
• [@rojiphil] 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: Added comment
• [@rojiphil] 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: Not Required. Existing checklist is good enough to capture such issues.
• [@rojiphil] Determine if we should create a regression test for this bug. : Not needed as this is an edge case and does not come in the critical path.
• [@rojiphil] 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. NA

[rojiphil]

@laurenreidexpensify BZ Checklist done and accepted offer. Thanks.

[laurenreidexpensify]

Payment Summary:

• C+ $250 @rojiphil paid via Upwork
• Contributor $250 @allgandalf paid via Upwork