Skip to content

fix: maintain tax assign when tax code changes #47327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 29, 2024
Merged

fix: maintain tax assign when tax code changes #47327

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
30c56d1
fix: maintain tax assign when tax code changes
daledah Aug 13, 2024
2aef27d
refactor: simplify optimistic and failture data
daledah Aug 14, 2024
d43e7a8
refactor: simplify failureCustomUnits
daledah Aug 14, 2024
569e76a
Merge branch 'main' into fix/45861
daledah Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/libs/actions/TaxRate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import INPUT_IDS from '@src/types/form/WorkspaceNewTaxForm';
import {default as INPUT_IDS_TAX_CODE} from '@src/types/form/WorkspaceTaxCodeForm';
import type {Policy, TaxRate, TaxRates} from '@src/types/onyx';
import type * as OnyxCommon from '@src/types/onyx/OnyxCommon';
import type {CustomUnit, Rate} from '@src/types/onyx/Policy';
import type {OnyxData} from '@src/types/onyx/Request';

let allPolicies: OnyxCollection<Policy>;
Expand Down Expand Up @@ -486,6 +487,33 @@ function renamePolicyTax(policyID: string, taxID: string, newName: string) {
function setPolicyTaxCode(policyID: string, oldTaxCode: string, newTaxCode: string) {
const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`];
const originalTaxRate = {...policy?.taxRates?.taxes[oldTaxCode]};
const customUnits = Object.values(policy?.customUnits ?? {});
const optimisticCustomUnit = {
customUnits: {
...customUnits.reduce((units, customUnit) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm misunderstanding but wouldn't this be better as a map instead of a reduce? Same with the rates.reduce below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, we can't add properties to an object using map, so I need to use reduce here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

customUnits is an array though right? Could do something here like

customUnits: customUnits.map((customUnit) => {
	let newUnit = customUnit;
	// modify rates
	return newUnit
})

Seems a little cleaner? And could use a forEach for the rates object?

what do you think @rushatgabhane

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with ^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But since customUnits in optimistic data is of type Record<string, any>, after this we will have to either do an Array.reduce or Object.assign to the updated customUnits anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah, fair. Okay then I think this works for me! Thank you for the explanation, that was helpful!

// eslint-disable-next-line no-param-reassign
units[customUnit.customUnitID] = {
rates: {
...Object.keys(customUnit.rates).reduce((rates, rateID) => {
if (customUnit.rates[rateID].attributes?.taxRateExternalID === oldTaxCode) {
// eslint-disable-next-line no-param-reassign
rates[rateID] = {
attributes: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also do we need to spread the old rate here? to make sure we don't remove everything but attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a merge call, so I think we don't need to include everything inside.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yep

taxRateExternalID: newTaxCode,
},
};
}
return rates;
}, {} as Record<string, Rate>),
},
};
return units;
}, {} as Record<string, Partial<CustomUnit>>),
},
};
const failureCustomUnit = {
customUnits: policy?.customUnits,
};
const oldDefaultExternalID = policy?.taxRates?.defaultExternalID;
const oldForeignTaxDefault = policy?.taxRates?.foreignTaxDefault;
const onyxData: OnyxData = {
Expand All @@ -508,6 +536,7 @@ function setPolicyTaxCode(policyID: string, oldTaxCode: string, newTaxCode: stri
},
},
},
...(!!customUnits && optimisticCustomUnit),
},
},
],
Expand Down Expand Up @@ -552,6 +581,7 @@ function setPolicyTaxCode(policyID: string, oldTaxCode: string, newTaxCode: stri
},
},
},
...(!!customUnits && failureCustomUnit),
},
},
],
Expand Down
Loading