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.54k stars 2.88k forks source link

[$250] Tax code - Error shows up when attempting to save tax code without modifying it #45770

Closed lanitochka17 closed 2 months ago

lanitochka17 commented 3 months 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.9-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: N/A Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

Expected Result:

User will be able to save existing tax code when no changes are made

Actual Result:

Error shows up when attempting to save tax code without modifying it - This tax code is already in use

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/38d088e0-a3b1-44e5-a625-8aaebf1030c6

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0100ee19e52f8bb5b6
  • Upwork Job ID: 1814285898085752540
  • Last Price Increase: 2024-07-19
Issue OwnerCurrent Issue Owner: @rayane-djouah
melvin-bot[bot] commented 3 months ago

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

github-actions[bot] commented 3 months 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.
etCoderDysto commented 3 months ago

Proposal

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

Error shows up when attempting to save tax code without modifying it

What is the root cause of that problem?

On submit, we call validateTaxCode passing current input value and policy as params. And validateTaxCode, compares current value with existing tax code keys and returns error when finding a matching key in policy.taxRates.taxes https://github.com/Expensify/App/blob/b8b99d0a994e3ccf8b91564df0f92a7c440aa6c7/src/pages/workspace/taxes/WorkspaceTaxCodePage.tsx#L50-L55

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

We should check if the input value is equal to the currentTaxCode tax code. Then return empty object if they are equal before returning result of validateTaxCode(policy, values);

if (values?.taxCode.trim() === currentTaxCode) return {};
return validateTaxCode(policy, values);

What alternative solutions did you explore? (Optional)

trjExpensify commented 3 months ago

@dangrous @Gonals @rushatgabhane @dylanexpensify - heads up!

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0100ee19e52f8bb5b6

melvin-bot[bot] commented 3 months ago

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

trjExpensify commented 3 months ago

FWIW @puneetlath, I don't think this is a deploy blocker. I'll let you make that call, but get the proposal triaged to move in the direction of fixing it. 👍

nyomanjyotisa commented 3 months ago

Proposal

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

Error shows up when attempting to save tax code without modifying it

What is the root cause of that problem?

We validated it with isExistingTaxCode so it return an error when we submit with the original value https://github.com/Expensify/App/blob/b8b99d0a994e3ccf8b91564df0f92a7c440aa6c7/src/libs/ValidationUtils.ts#L481-L484

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

Update the isExistingTaxCode function to return false if the submitted taxCode is equal to originalTaxCode

function isExistingTaxCode(taxCode: string, taxRates: TaxRates, originalTaxCode: string): boolean {
    const trimmedTaxCode = taxCode.trim();
    if (trimmedTaxCode === originalTaxCode.trim()) {
        return false;
    }
    return !!Object.keys(taxRates).find((taxID) => taxID === trimmedTaxCode);
}

Update this to include currentTaxCode

const validateTaxCode = (policy: Policy, values: FormOnyxValues<typeof ONYXKEYS.FORMS.WORKSPACE_TAX_CODE_FORM>, currentTaxCode: string) => {
    const errors = ValidationUtils.getFieldRequiredErrors(values, [INPUT_IDS_TAX_CODE.TAX_CODE]);

    const taxCode = values[INPUT_IDS_TAX_CODE.TAX_CODE];
    if (policy?.taxRates?.taxes && ValidationUtils.isExistingTaxCode(taxCode, policy.taxRates.taxes, currentTaxCode)) {
        errors[INPUT_IDS_TAX_CODE.TAX_CODE] = translateLocal('workspace.taxes.error.taxCodeAlreadyExists');
    }

    return errors;
};

Update this to include currentTaxCode

return validateTaxCode(policy, values, currentTaxCode);

Additionally we can do the same for validateTaxName here and isExistingTaxName here

What alternative solutions did you explore? (Optional)

rushatgabhane commented 3 months ago

Fixing it as part of https://github.com/Expensify/App/issues/44258

rushatgabhane commented 3 months ago

cc: @mollfpr for c+

dangrous commented 3 months ago

removing deploy blocker label, minor issue

dangrous commented 3 months ago

How are we looking on the fix for this? I know there's that other bug too, that feels backend to me so I think it's separate? But I'll confirm today.

rushatgabhane commented 3 months ago

How are we looking on the fix for this

@dangrous fixing it in PR - https://github.com/Expensify/App/pull/45866 because it was a one liner

dangrous commented 3 months ago

oh okay great! ping me as a reviewer when it's ready if you could - thanks!

melvin-bot[bot] commented 3 months ago

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

dangrous commented 3 months ago

not overdue, solution has been deployed to prod. Didn't get the automation because the issue link didn't have $. I added it so hopefully we'll get it, but if not - payment due 8/9 i believe!

dangrous commented 2 months ago

This one is all set in terms of timeline. Are we treating this as a regression, @rushatgabhane / @mollfpr ? Or do we need to grab a BZ for payment?

rushatgabhane commented 2 months ago

this would be a regression @dangrous

dangrous commented 2 months ago

okay cool i will go ahead and close then, if you need anything else let me know!