Skip to content
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.

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

[$250] Track tax - Tax rate does not update when editing distance rate offline #48293

Open
6 tasks done
lanitochka17 opened this issue Aug 29, 2024 · 23 comments
Open
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 29, 2024

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.26-2
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4902670
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.
  • Workspace has at least two distance rates that have tax rate and tax reclaimable on
  • Each distance rate has different tax rate
  1. Go to staging.new.expensify.com
  2. Go to workspace chat
  3. Submit a distance expense (for Step 8 )
  4. Go offline
  5. Start another submit distance flow and proceed to confirmation page
  6. Click Rate and select a different rate
  7. Note that Tax rate field still updates according to the selected distance rate in offline mode
  8. Go to the distance expense thread in Step 3
  9. Click Rate
  10. Select a different rate

Expected Result:

  • Tax rate will update after changing distance rate offline in transaction thread, as it works on confirmation page (Step 7)
  • Tax rate and tax amount field will gray out

Actual Result:

  • Tax rate does not update after changing distance rate offline in transaction thread. The only thing that updates is the removal of "default" label in tax rate field
  • Tax rate and tax amount field are not gray out

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

Bug6586512_1724933599343.20240829_200404.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c25cfebc45c34fc3
  • Upwork Job ID: 1830995712415705839
  • Last Price Increase: 2024-09-10
  • Automatic offers:
    • Krishna2323 | Contributor | 103905995
Issue OwnerCurrent Issue Owner: @aimane-chnaif
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 29, 2024
Copy link

melvin-bot bot commented Aug 29, 2024

Triggered auto assignment to @bfitzexpensify (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
Copy link
Author

@bfitzexpensify 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
Copy link
Contributor

Krishna2323 commented Aug 29, 2024

Edited by proposal-police: This proposal was edited at 2024-08-30 00:03:36 UTC.

Proposal

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

Track tax - Tax rate does not update when editing distance rate offline

What is the root cause of that problem?

  • We use getTaxName to get the tax and when distance transaction is created, the taxCode is filled by the backend. When we update the rate after the transaction is created, taxCode is used, which will be only updated the backend api call is successful.
    /**
    * Gets the name corresponding to the taxCode that is displayed to the user
    */
    function getTaxName(policy: OnyxEntry<Policy>, transaction: OnyxEntry<Transaction>) {
    const defaultTaxCode = getDefaultTaxCode(policy, transaction);
    return Object.values(transformedTaxRates(policy, transaction)).find((taxRate) => taxRate.code === (transaction?.taxCode ?? defaultTaxCode))?.modifiedName;
    }
  • This does not happen when we are creating a new distance request because in the transaction draft transaction?.taxCode is not present, so we are using the defaultTaxCode, getDefaultTaxCode will return the correct tax using the customUnitRateID.
    function getDefaultTaxCode(policy: OnyxEntry<Policy>, transaction: OnyxEntry<Transaction>, currency?: string | undefined): string | undefined {
    if (isDistanceRequest(transaction)) {
    const customUnitRateID = getRateID(transaction) ?? '';
    const customUnitRate = getCustomUnitRate(policy, customUnitRateID);
    return customUnitRate?.attributes?.taxRateExternalID ?? '';
    }
    const defaultExternalID = policy?.taxRates?.defaultExternalID;
    const foreignTaxDefault = policy?.taxRates?.foreignTaxDefault;
    return policy?.outputCurrency === (currency ?? getCurrency(transaction)) ? defaultExternalID : foreignTaxDefault;
    }

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

  • In selectDistanceRate, we are already calculating taxAmount and taxRateExternalID (taxCode), we can modify the updateMoneyRequestDistanceRate util function to accept 2 new parameters taxAmount and taxRateExternalID (taxCode).
    function selectDistanceRate(customUnitRateID: string) {
    if (shouldShowTax) {
    const policyCustomUnitRate = getCustomUnitRate(policy, customUnitRateID);
    const taxRateExternalID = policyCustomUnitRate?.attributes?.taxRateExternalID ?? '-1';
    const taxableAmount = DistanceRequestUtils.getTaxableAmount(policy, customUnitRateID, TransactionUtils.getDistanceInMeters(transaction, unit));
    const taxPercentage = TransactionUtils.getTaxValue(policy, transaction, taxRateExternalID) ?? '';
    const taxAmount = CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, taxableAmount, rates[customUnitRateID].currency ?? CONST.CURRENCY.USD));
    IOU.setMoneyRequestTaxAmount(transactionID, taxAmount);
    IOU.setMoneyRequestTaxRate(transactionID, taxRateExternalID);
    }
    if (currentRateID !== customUnitRateID) {
    IOU.setMoneyRequestDistanceRate(transactionID, customUnitRateID, policy?.id ?? '-1', !isEditing);
    if (isEditing) {
    IOU.updateMoneyRequestDistanceRate(transaction?.transactionID ?? '-1', reportID, customUnitRateID, policy, policyTags, policyCategories);
    }
    }
    navigateBack();
    }
  • Then from selectDistanceRate we will pass the taxAmount and taxRateExternalID (taxCode) to updateMoneyRequestDistanceRate.
  • Update the transactionChanges object in updateMoneyRequestDistanceRate function to also include taxAmount & taxRate.

    App/src/libs/actions/IOU.ts

    Lines 3122 to 3145 in 489312e

    /** Updates the distance rate of an expense */
    function updateMoneyRequestDistanceRate(
    transactionID: string,
    transactionThreadReportID: string,
    rateID: string,
    policy: OnyxEntry<OnyxTypes.Policy>,
    policyTagList: OnyxEntry<OnyxTypes.PolicyTagLists>,
    policyCategories: OnyxEntry<OnyxTypes.PolicyCategories>,
    ) {
    const transactionChanges: TransactionChanges = {
    customUnitRateID: rateID,
    };
    const allReports = ReportConnection.getAllReports();
    const transactionThreadReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${transactionThreadReportID}`] ?? null;
    const parentReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${transactionThreadReport?.parentReportID}`] ?? null;
    let data: UpdateMoneyRequestData;
    if (ReportUtils.isTrackExpenseReport(transactionThreadReport) && ReportUtils.isSelfDM(parentReport)) {
    data = getUpdateTrackExpenseParams(transactionID, transactionThreadReportID, transactionChanges, true, policy);
    } else {
    data = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, policy, policyTagList, policyCategories, true);
    }
    const {params, onyxData} = data;
    API.write(WRITE_COMMANDS.UPDATE_MONEY_REQUEST_DISTANCE_RATE, params, onyxData);
    }
  • This will also grey out the taxAmount & taxRate field without modifying the fields to look for change in customUnitRateID field.
Code changes
    function selectDistanceRate(customUnitRateID: string) {
        let taxAmount;
        let taxRateExternalID;
        if (shouldShowTax) {
            const policyCustomUnitRate = getCustomUnitRate(policy, customUnitRateID);
            taxRateExternalID = policyCustomUnitRate?.attributes?.taxRateExternalID ?? '-1';
            const taxableAmount = DistanceRequestUtils.getTaxableAmount(policy, customUnitRateID, TransactionUtils.getDistanceInMeters(transaction, unit));
            const taxPercentage = TransactionUtils.getTaxValue(policy, transaction, taxRateExternalID) ?? '';
            taxAmount = CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, taxableAmount, rates[customUnitRateID].currency ?? CONST.CURRENCY.USD));
            IOU.setMoneyRequestTaxAmount(transactionID, taxAmount);
            IOU.setMoneyRequestTaxRate(transactionID, taxRateExternalID);
        }

        if (currentRateID !== customUnitRateID) {
            IOU.setMoneyRequestDistanceRate(transactionID, customUnitRateID, policy?.id ?? '-1', !isEditing);

            if (isEditing) {
                IOU.updateMoneyRequestDistanceRate(
                    transaction?.transactionID ?? '-1',
                    reportID,
                    customUnitRateID,
                    policy,
                    policyTags,
                    policyCategories,
                    shouldShowTax ? taxRateExternalID : transaction?.taxCode,
                    shouldShowTax ? taxAmount : transaction?.taxAmount,
                );
            }
        }

        navigateBack();
    }

function updateMoneyRequestDistanceRate(
    transactionID: string,
    transactionThreadReportID: string,
    rateID: string,
    policy: OnyxEntry<OnyxTypes.Policy>,
    policyTagList: OnyxEntry<OnyxTypes.PolicyTagLists>,
    policyCategories: OnyxEntry<OnyxTypes.PolicyCategories>,
    taxCode?: string,
    taxAmount?: number,
) {
    const transactionChanges: TransactionChanges = {
        customUnitRateID: rateID,
        ...(taxCode ? {taxCode} : {}),
        ...(taxAmount ? {taxAmount} : {}),
    };
   // ...
}

What alternative solutions did you explore? (Optional)



We can do few things here:

  • If the transaction is a distance request then use defaultTaxCode as the primary value.
  • If the transaction is a distance request and has pending action for customUnitRateID then use defaultTaxCode as the primary value..
  • We can also create a function for the above steps.
  • To grey out the taxRate and taxAmount fields we can check if the request is of type distance and if true we will also use customUnitRateID for pending action.
 <OfflineWithFeedback pendingAction={isDistanceRequest ? getPendingFieldAction('taxCode') ?? getPendingFieldAction('customUnitRateID') : getPendingFieldAction('taxCode')}>

{shouldShowTax && (
<OfflineWithFeedback pendingAction={getPendingFieldAction('taxCode')}>
<MenuItemWithTopDescription
title={taxRateTitle ?? ''}
description={taxRatesDescription}
interactive={canEditTaxFields}
shouldShowRightIcon={canEditTaxFields}
titleStyle={styles.flex1}
onPress={() =>
Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_TAX_RATE.getRoute(CONST.IOU.ACTION.EDIT, iouType, transaction?.transactionID ?? '-1', report?.reportID ?? '-1'))
}
brickRoadIndicator={getErrorForField('tax') ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined}
errorText={getErrorForField('tax')}
/>
</OfflineWithFeedback>
)}
{shouldShowTax && (
<OfflineWithFeedback pendingAction={getPendingFieldAction('taxAmount')}>
<MenuItemWithTopDescription
title={formattedTaxAmount ? formattedTaxAmount.toString() : ''}
description={translate('iou.taxAmount')}
interactive={canEditTaxFields}
shouldShowRightIcon={canEditTaxFields}
titleStyle={styles.flex1}
onPress={() =>
Navigation.navigate(
ROUTES.MONEY_REQUEST_STEP_TAX_AMOUNT.getRoute(CONST.IOU.ACTION.EDIT, iouType, transaction?.transactionID ?? '-1', report?.reportID ?? '-1'),
)
}
/>
</OfflineWithFeedback>
)}

Result

@Krishna2323
Copy link
Contributor

Proposal Updated

  • Added alternative

@nkdengineer
Copy link
Contributor

Proposal

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

Tax rate does not update after changing distance rate offline in transaction thread. The only thing that updates is the removal of "default" label in tax rate field
Tax rate and tax amount field are not gray out

What is the root cause of that problem?

Tax rate does not update after changing distance rate offline in transaction thread. The only thing that updates is the removal of "default" label in tax rate field

  1. We have the logic here to update the tax rate and tax amount when we update the distance rate if tax tracking is enabled. It updates the draft transaction data used in the create flow.

function selectDistanceRate(customUnitRateID: string) {
if (shouldShowTax) {
const policyCustomUnitRate = getCustomUnitRate(policy, customUnitRateID);
const taxRateExternalID = policyCustomUnitRate?.attributes?.taxRateExternalID ?? '-1';
const taxableAmount = DistanceRequestUtils.getTaxableAmount(policy, customUnitRateID, TransactionUtils.getDistanceInMeters(transaction, unit));
const taxPercentage = TransactionUtils.getTaxValue(policy, transaction, taxRateExternalID) ?? '';
const taxAmount = CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, taxableAmount, rates[customUnitRateID].currency ?? CONST.CURRENCY.USD));
IOU.setMoneyRequestTaxAmount(transactionID, taxAmount);
IOU.setMoneyRequestTaxRate(transactionID, taxRateExternalID);
}

When we're in the edit flow, we don't update the tax rate and tax amount in optimistic data in getUpdateMoneyRequestParams function

Tax rate and tax amount field are not gray out

Tax rate/tax amount is only grayed out if we have pending action on tax rate/tax amount

<OfflineWithFeedback pendingAction={getPendingFieldAction('taxCode')}>

<OfflineWithFeedback pendingAction={getPendingFieldAction('taxAmount')}>

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

Tax rate will update after changing distance rate offline in transaction thread, as it works on confirmation page (Step 7)

For this bug, we can follow the same way we do in IOURequestStepDistanceRate to update the tax rate and tax amount in optimistic data when we edit the distance rate after creating. We can create a util

function getTaxRateAndTaxAmountForTrackTax(transaction: OnyxInputOrEntry<Transaction>, policy: OnyxInputOrEntry<Policy>, isPolicyExpenseChat: boolean, customUnitRateID = '') {
    if (!transaction || !policy || !customUnitRateID || !PolicyUtils.isTaxTrackingEnabled(isPolicyExpenseChat, policy, true)) {
        return {
            taxCode: undefined,
            taxAmount: undefined,
        };
    }

    const mileageRates = DistanceRequestUtils.getMileageRates(policy, true);
    const {unit} = mileageRates[customUnitRateID] ?? DistanceRequestUtils.getDefaultMileageRate(policy);
    const policyCustomUnitRate = getCustomUnitRate(policy, customUnitRateID);
    const taxRateExternalID = policyCustomUnitRate?.attributes?.taxRateExternalID ?? '-1';
    const taxableAmount = DistanceRequestUtils.getTaxableAmount(policy, customUnitRateID, getDistanceInMeters(transaction, unit));
    const taxPercentage = getTaxValue(policy, transaction ?? undefined, taxRateExternalID) ?? '';
    const taxAmount = convertToBackendAmount(calculateTaxAmount(taxPercentage, taxableAmount, mileageRates[customUnitRateID].currency ?? CONST.CURRENCY.USD));
    return {taxCode: taxRateExternalID, taxAmount};
}

function selectDistanceRate(customUnitRateID: string) {
if (shouldShowTax) {
const policyCustomUnitRate = getCustomUnitRate(policy, customUnitRateID);
const taxRateExternalID = policyCustomUnitRate?.attributes?.taxRateExternalID ?? '-1';
const taxableAmount = DistanceRequestUtils.getTaxableAmount(policy, customUnitRateID, TransactionUtils.getDistanceInMeters(transaction, unit));
const taxPercentage = TransactionUtils.getTaxValue(policy, transaction, taxRateExternalID) ?? '';
const taxAmount = CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, taxableAmount, rates[customUnitRateID].currency ?? CONST.CURRENCY.USD));
IOU.setMoneyRequestTaxAmount(transactionID, taxAmount);
IOU.setMoneyRequestTaxRate(transactionID, taxRateExternalID);
}

const {taxCode, taxAmount} = getTaxRateAndTaxAmountForTrackTax(transaction, policy, isFromExpenseReport, customUnitRateID);

if (hasModifiedRouteWithPendingWaypoints || hasModifiedRateWithPendingWaypoints) {
    return {
        amount: CONST.IOU.DEFAULT_AMOUNT,
        modifiedAmount: CONST.IOU.DEFAULT_AMOUNT,
        modifiedMerchant: Localize.translateLocal('iou.fieldPending'),
        ...(taxCode ? {taxCode} : {}),
        ...(taxAmount ? {taxAmount} : {}),
    };
}

...

return {
    amount: updatedAmount,
    modifiedAmount: updatedAmount,
    modifiedMerchant: updatedMerchant,
    modifiedCurrency: updatedCurrency,
    ...(taxCode ? {taxCode} : {}),
    ...(taxAmount ? {taxAmount} : {}),
};

and use this in calculateAmountForUpdatedWaypointOrRate function

Tax rate and tax amount field will gray out

For this bug, we can check if the request is distance request we will add the fallback pending action for tax rate and tax amount with the pending action of distance rate here

<OfflineWithFeedback pendingAction={isDistanceRequest ? getPendingFieldAction('taxCode') ?? getPendingFieldAction('customUnitRateID') : getPendingFieldAction('taxCode')}>

<OfflineWithFeedback pendingAction={getPendingFieldAction('taxCode')}>

<OfflineWithFeedback pendingAction={isDistanceRequest ? getPendingFieldAction('taxAmount') ?? getPendingFieldAction('customUnitRateID') : getPendingFieldAction('taxAmount')}>

<OfflineWithFeedback pendingAction={getPendingFieldAction('taxAmount')}>

What alternative solutions did you explore? (Optional)

NA

@Krishna2323
Copy link
Contributor

Krishna2323 commented Aug 30, 2024

Proposal Updated

  • Added new solution in the main solution section & moved the previous one in alternative section.

@melvin-bot melvin-bot bot added the Overdue label Sep 2, 2024
Copy link

melvin-bot bot commented Sep 2, 2024

@bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@bfitzexpensify bfitzexpensify added the External Added to denote the issue can be worked on by a contributor label Sep 3, 2024
@melvin-bot melvin-bot bot changed the title Track tax - Tax rate does not update when editing distance rate offline [$250] Track tax - Tax rate does not update when editing distance rate offline Sep 3, 2024
Copy link

melvin-bot bot commented Sep 3, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 3, 2024
Copy link

melvin-bot bot commented Sep 3, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Sep 3, 2024
@aimane-chnaif
Copy link
Contributor

Thanks for the proposals. Can you please share demo videos after applying your solution?
Please make sure that Tax rate and tax amount field are not gray out bug is also fixed.

@Krishna2323
Copy link
Contributor

@aimane-chnaif, test branch.

tax_rate_amount_offline.mp4

@nkdengineer
Copy link
Contributor

@aimane-chnaif The result here.

Screen.Recording.2024-09-05.at.10.59.36.mov

@bfitzexpensify
Copy link
Contributor

I am heading out of office until September 21st, so assigning a buddy to watch over this in my absence.

Current status: working through proposals

@bfitzexpensify bfitzexpensify removed their assignment Sep 6, 2024
@bfitzexpensify bfitzexpensify added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Sep 6, 2024
@bfitzexpensify bfitzexpensify self-assigned this Sep 6, 2024
Copy link

melvin-bot bot commented Sep 6, 2024

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

Copy link

melvin-bot bot commented Sep 9, 2024

@lschurr, @bfitzexpensify, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Sep 9, 2024
@trjExpensify
Copy link
Contributor

@aimane-chnaif what do you think of these proposals? CC: @MonilBhavsar for eyes.

Copy link

melvin-bot bot commented Sep 10, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@aimane-chnaif
Copy link
Contributor

@Krishna2323's main solution looks good.
🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Sep 10, 2024

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 10, 2024
Copy link

melvin-bot bot commented Sep 10, 2024

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

Copy link

melvin-bot bot commented Sep 12, 2024

@marcaaron @lschurr @bfitzexpensify @Krishna2323 @aimane-chnaif 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!

@Krishna2323
Copy link
Contributor

@aimane-chnaif, PR ready for review ^

Copy link

melvin-bot bot commented Sep 19, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: Polish
Development

No branches or pull requests

8 participants