[HOLD for payment 2024-10-25] [$250] [P2P Distance] Distance rate - Error when selecting distance rate that no longer has tax rate

[IuliiaHerets]

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.21-3
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

Precondition:

• Track tax is enabled.
• Create a Distance rate X that has Tax rate and Tax reclaimable on.

1. Go to staging.new.expensify.com
2. Go to workspace settings > Taxes.
3. Delete tha tax rate that is assigned to Distance rate X from the precondition.
4. Go to workspace chat.
5. Submit a distance expense with another distance rate that is not Distance rate X.
6. Go to transaction thread.
7. Click Rate.
8. Select Distance rate X.

Expected Result:

App will not throw error when selecting a distance rate that no longer has tax rate (tax rate is deleted after it is assigned to distance rate).

Actual Result:

App throws error when selecting a distance rate that no longer has tax rate.

Workaround:

Unknown

Platforms:

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• [x ] MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6575129_1723970878058.20240818_164022.mp4

---

#47613 (comment)

On NewDot in the workspace settings, I propose this change to make it clearer that the Tax rate and Tax reclaimable on set on a distance rate are interlinked:

Don’t show the Tax reclaimable on field until a Tax rate is selected.
Note: This would be similar to how we show certain fields in settings after a selection that comes before it, i.e monthly for scheduled submit reveals a Date field etc).

If the Tax rate being used on the distance rate is deleted in Taxes, clear both the Tax rate and Tax reclaimable on values. We would then also hide the Tax reclaimable on field again as Tax rate is empty.
Note: This would prevent an “unexpected error” from occurring, as you can’t get into a situation where there’s a Tax reclaimable on value without a Tax rate selected.

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01def12cd4fe837196
• Upwork Job ID: 1828100234986953227
• Last Price Increase: 2024-08-26
• Automatic offers:
• DylanDylann | Reviewer | 103922844

Issue Owner

Current Issue Owner: @OfstadC

[melvin-bot]

Triggered auto assignment to @OfstadC (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]

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

[IuliiaHerets]

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

[daledah]

Proposal

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

App throws error when selecting a distance rate that no longer has tax rate.

The problem also happens when the linked tax rate is disabled.

What is the root cause of that problem?

DeletePolicyTaxes API does not remove the deleted tax rate from the attached distance rate, it only clears the tax rate itself:

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

We should:

1. not display; or
2. display as disabled

the distance rate with deleted tax rate in distance rate selector.

First, modify the getMileageRates here to include a new param includeDisabledTrackTaxRates to filter rates with deleted or disabled tax rates.

Secondly, we need to know whether its corresponding tax rate is deleted or disabled by PolicyUtils.getTaxByID here.

const taxRate = PolicyUtils.getTaxByID(rate.attributes.taxRateExternalID);
const isTaxRateAvailable = taxRate && !taxRate.isDisabled;

Then:

1. Filter those distance rates accordingly by appending isTaxRateAvailable to the early return condition here.

2. Introduce a new attribute disabled to the returned millage rates here. Then show it as disabled in the sections here by isInteractive: !rate.disabled.

disabled: PolicyUtils.isTaxTrackingEnabled && !isTaxRateAvailable

[OfstadC]

I feel like it should show an error, no? Going to discuss in Slack.

[OfstadC]

Slack discussion ongoing

[OfstadC]

@trjExpensify Could you confirm expected behavior here? I don't have the tax option on Distance rates in NewDot 🤔

[trjExpensify]

Commented in thread!

[trjExpensify]

Adding @MonilBhavsar here per the thread.

[trjExpensify]

On NewDot in the workspace settings, I propose this change to make it clearer that the Tax rate and Tax reclaimable on set on a distance rate are interlinked:

• Don’t show the Tax reclaimable on field until a Tax rate is selected.

Note: This would be similar to how we show certain fields in settings after a selection that comes before it, i.e monthly for scheduled submit reveals a Date field etc).

• If the Tax rate being used on the distance rate is deleted in Taxes, clear both the Tax rate and Tax reclaimable on values. We would then also hide the Tax reclaimable on field again as Tax rate is empty.

Note: This would prevent an “unexpected error” from occurring, as you can’t get into a situation where there’s a Tax reclaimable on value without a Tax rate selected.

[MonilBhavsar]

@daledah if you want to update your proposal based on this^

[daledah]

Edited by proposal-police: This proposal was edited at 2024-08-19T10:00:00Z.

Updated Proposal

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

App throws error when selecting a distance rate that no longer has tax rate.

The problem also happens when the linked tax rate is disabled.

What is the root cause of that problem?

DeletePolicyTaxes API does not remove the deleted tax rate from the attached distance rate, it only clears the tax rate itself:

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

Don’t show the Tax reclaimable on field until a Tax rate is selected.

Add the condition of having a tax rate selected here:

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

Line 157 in d48a513

{isDistanceTrackTaxEnabled && (

isDistanceTrackTaxEnabled && !!taxRate

If the Tax rate being used on the distance rate is deleted in Taxes, clear both the Tax rate and Tax reclaimable on values.

We need to update the BE to handle this and implement optimistic data in the FE.

In deletePolicyTaxes action here, we need to check if there's any linked rate with tax IDs from taxesToDelete:

const customUnits = policy?.customUnits ?? {};
const ratesToUpdate = Object.values(customUnits?.[Object.keys(customUnits)[0]]?.rates).filter((rate) => !!rate.attributes?.taxRateExternalID && taxesToDelete.includes(rate.attributes?.taxRateExternalID));

Then if there are, build the onyx data to clear the tax rate and value from the corresponding distance rate:

const optimisticRates: Record<string, Rate> = {};
const successRates: Record<string, Rate> = {};
const failureRates: Record<string, Rate> = {};

ratesToUpdate.forEach((rate) => {
    const rateID = rate.customUnitRateID ?? '';
    optimisticRates[rateID] = {
        ...rate,
        attributes: {
            ...rate?.attributes,
            taxRateExternalID: undefined,
            taxClaimablePercentage: undefined,
        },
        pendingFields: {
            taxRateExternalID: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
            taxClaimablePercentage: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
        },
    };
    successRates[rateID] = {...rate, pendingFields: {taxRateExternalID: null}};
    failureRates[rateID] = {
        ...rate,
        pendingFields: {taxRateExternalID: null, taxClaimablePercentage: null},
        errorFields: {
            taxRateExternalID: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('common.genericErrorMessage'),
            taxClaimablePercentage: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('common.genericErrorMessage'),
        },
    };
});

And use them inside onyxData here.

[melvin-bot]

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

[melvin-bot]

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

[MonilBhavsar]

@DylanDylann could you please take a look at the proposal based on this expected outcome #47613 (comment)
bas

[DylanDylann]

@daledah proposal looks good to me. I see some minor problems in your implementation. However the detailed implementation will be discussed more in the PR phase.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

@OfstadC @MonilBhavsar @DylanDylann this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[MonilBhavsar]

Sounds good 👍 Thanks for clarifying

[melvin-bot]

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

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

[MonilBhavsar]

@trjExpensify a quick question here. If user deletes a tax rate associated with distance rate, should we display an error/confirmation if they really want to delete it and deleting it would also clear tax rate from distance rate, or we should delete it without any notification

[daledah]

@DylanDylann PR is up.

[melvin-bot]

This issue has not been updated in over 15 days. @OfstadC, @MonilBhavsar, @DylanDylann, @daledah eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[DylanDylann]

Update: Waiting for BE change from @MonilBhavsar

[MonilBhavsar]

Submitted PR for review. It is expected to be live on staging early next week

[MonilBhavsar]

App PR is merged!

[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 9.0.50-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: Error when selecting distance rate that no longer has tax rate #49044

If no regressions arise, payment will be issued on 2024-10-25. 🎊

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

• @DylanDylann requires payment automatic offer (Reviewer)
• @daledah 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:

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

[OfstadC]

@DylanDylann Could you please complete the BZ checklist? Thanks! 😃

[DylanDylann]

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:

[@DylanDylann] The PR that introduced the bug has been identified. Link to the PR: NA
[@DylanDylann] 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: NA
[@DylanDylann] 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: NA
[@DylanDylann] Determine if we should create a regression test for this bug. Yes
[@DylanDylann] 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
Precondition:

• Track tax is enabled.
• Create a Distance rate X that has Tax rate and Tax reclaimable on.

1. Go to workspace settings > Taxes
2. Delete the tax rate that is assigned to Distance Rate X from the precondition
3. Go to the distance rate page
4. Select Distance rate X
5. Verify that: the tax rate is empty and Tax reclaimable on is hidden

Do we agree 👍 or 👎

[OfstadC]

Payment Summary

• @DylanDylann paid $250 via Upwork
• @daledah offer sent here - please accept so I can issue payment😃

Had to create new job post here