[$250] [Track Tax] Tax currency is editable for employee, and it reverts to default currency after creation

[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.63-0
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:

Precondition:

• User is an employee of Collect workspace
• Collect workspace has some tax rates

1. Go to staging.new.expensify.com
2. Go to workspace chat
3. Go to + > Submit expense > Distance
4. Proceed to confirmation page
5. Note that distance amount cannot be edited
6. Click Show more > Tax amount
7. Click on the currency

Expected Result:

Since workspace employee is unable to edit the distance amount, they should not be able to edit tax currency

Actual Result:

Workspace employee is able to edit tax currency for distance request. After the request is created, the tax currency reverts to the workspace default currency

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

Bug6454202_1713469646447.20240419_034127.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bf4f1d5c88c460fe
• Upwork Job ID: 1782480122275028992
• Last Price Increase: 2024-05-28

[melvin-bot]

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

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

[lanitochka17]

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

[ZhenjaHorbach]

Proposal

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

Tax - Tax currency is editable for employee, and it reverts to default currency after creation

What is the root cause of that problem?

The main problem with issue is that we can't change amount when it's DistanceRequest

App/src/components/MoneyRequestConfirmationList.tsx

Lines 665 to 689 in 48f64d0

	{
	item: (
	<MenuItemWithTopDescription
	key={translate('iou.amount')}
	shouldShowRightIcon={!isReadOnly && !isDistanceRequest}
	title={formattedAmount}
	description={translate('iou.amount')}
	interactive={!isReadOnly}
	onPress={() => {
	if (isDistanceRequest) {
	return;
	}
	Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_AMOUNT.getRoute(action, iouType, transactionID, reportID, Navigation.getActiveRouteWithoutParams()));
	}}
	style={[styles.moneyRequestMenuItem, styles.mt2]}
	titleStyle={styles.moneyRequestConfirmationAmount}
	disabled={didConfirm}
	brickRoadIndicator={shouldDisplayFieldError && TransactionUtils.isAmountMissing(transaction ?? null) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined}
	error={shouldDisplayFieldError && TransactionUtils.isAmountMissing(transaction ?? null) ? translate('common.error.enterAmount') : ''}
	/>
	),
	shouldShow: shouldShowSmartScanFields,
	isSupplementary: false,
	},

But we don't have this condition for taxAmount

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

To fix this issue we can disable taxAmount (optional taxRate) when it's DistanceRequest using the same logic as for amount

App/src/components/MoneyRequestConfirmationList.tsx

Lines 838 to 854 in 48f64d0

	{
	item: (
	<MenuItemWithTopDescription
	key={`${taxRates?.name}${formattedTaxAmount}`}
	shouldShowRightIcon={!isReadOnly}
	title={formattedTaxAmount}
	description={translate('iou.taxAmount')}
	style={[styles.moneyRequestMenuItem]}
	titleStyle={styles.flex1}
	onPress={() => Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_TAX_AMOUNT.getRoute(action, iouType, transactionID, reportID, Navigation.getActiveRouteWithoutParams()))}
	disabled={didConfirm}
	interactive={!isReadOnly}
	/>
	),
	shouldShow: shouldShowTax,
	isSupplementary: true,
	},

What alternative solutions did you explore? (Optional)

NA

[allgandalf]

The expected result is wrong IMO, we should indeed be able to edit taxes on any distance request, so I will propose my solution accordingly.

Proposal

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

There is a mismatch in currency of amount and tax rate.

What is the root cause of that problem?

When we update tax rate currency at the same time we need to update the currency of the amount as well, currently we do not update the currency of amount, this happens because we have a check for formattedAmount which will set the currency to currency prop, instead of iouCurrencyCode:

App/src/components/MoneyRequestConfirmationList.tsx

Line 264 in 48f64d0

isDistanceRequest ? currency : iouCurrencyCode,

This formattedAmount is used to display the amount of distance request.

Observe the currency of tax amount and amount, you'll see currency mismatch after we update the currency of tax request.

Now these values are sent to the BE with different currencies and hence we see weird texts on the money report details page. This is the RCA for the actual bug.

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

We should always set currency to iouCurrencyCode, whatever the request type may be, so we should remove isDistanceRequest from the above.

This is safe to do as the currency does change for tax amount as well as amount if any one of the two is edited for manual requests:

Screen.Recording.2024-04-20.at.6.49.29.AM.mov

Hence, we should allow the same for distance requests as well.

So we should change the currency of amount as well if we update the currency of Tax Rate

What alternative solutions did you explore? (Optional)

N/A

NOTE:

If we decide to go with original bug expectation then here's my solution:

The RCA is that we do not mark the tax rate and tax amount as non editable if the request is of distance type

Solution:

I propose that we shouldn't disable(this is done using shouldShow field) the tax rate and tax amount but instead we should mark both these field as non editable by adding an extra condition of !isDistanceRequest for shouldShowRightIcon (shouldShowRightIcon is used to mark the field as editable) if the request type is Distance below:

App/src/components/MoneyRequestConfirmationList.tsx

Lines 823 to 825 in 48f64d0

	<MenuItemWithTopDescription
	key={`${taxRates?.name}${taxRateTitle}`}
	shouldShowRightIcon={!isReadOnly}

App/src/components/MoneyRequestConfirmationList.tsx

Lines 840 to 842 in 48f64d0

	<MenuItemWithTopDescription
	key={`${taxRates?.name}${formattedTaxAmount}`}
	shouldShowRightIcon={!isReadOnly}

[dragnoir]

Proposal

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

Tax currency is editable for employee, and it reverts to default currency after creation

What is the root cause of that problem?

We disable the shouldShowRightIcon here

App/src/components/MoneyRequestConfirmationList.tsx

Line 698 in a04f40e

shouldShowRightIcon={!isReadOnly && !isDistanceRequest}

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

We should do the same for "Tax Amount" here
and return if (isDistanceRequest)

shouldShowRightIcon={!isReadOnly && !isDistanceRequest}

onPress={() => {
  if (isDistanceRequest) {
    return;
  }
  // ...
  }}

POC:

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[CortneyOfstad]

Posted in Wave Collect and will follow up when we have an urgency.

@mananjadhav we have some proposals above for you to review when you have a chance 👍 Thanks!

[trjExpensify]

@MonilBhavsar discussion about this here for you to catch up on.

[MonilBhavsar]

It will be fixed by #31673

[mananjadhav]

Should we close this one then?

[trjExpensify]

Maybe, it will be good to point to which PR in that linked issue will fix this because there's a bunch of them, a lot of which are merged.

[CortneyOfstad]

@MonilBhavsar to echo TRJ, there are quite a few PRs linked in the GH here — can you point out the PR I should be keeping an eye on as the on-hold for this GH? TIA!

[MonilBhavsar]

It's this one #31673

[melvin-bot]

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

[CortneyOfstad]

Draft PR is here

[melvin-bot]

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

[CortneyOfstad]

Still waiting on draft PR. Will be delayed due to merge freeze 👍

[CortneyOfstad]

Merge freeze ends on Monday 🎉

[CortneyOfstad]

This is actively being worked on via the on-hold GH, as there was an issue with the distance rates. Discussed here

[melvin-bot]

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

[CortneyOfstad]

Per the comment here, someone else is going to take this on 👍 They are not online now, but will keep an eye on this to get them assigned ASAP.

[mananjadhav]

@CortneyOfstad Do you need a C+ assigned here?

[CortneyOfstad]

@mananjadhav — one is automatically assigned via the External label, but I will unassign, as it is not needed at this stage. Thanks!

[melvin-bot]

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

[CortneyOfstad]

PR is here. We're on a merge freeze, so will be a bit before it will be released

[melvin-bot]

@CortneyOfstad Huh... This is 4 days overdue. Who can take care of this?

[CortneyOfstad]

Merge freeze extended

[melvin-bot]

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

[trjExpensify]

#42342 has been deployed to staging, taking this off hold.

[trjExpensify]

@MonilBhavsar what are the next steps here?

[MonilBhavsar]

This is fixed now. We can close it.
User is no longer able to edit tax amount or currency