[HOLD for payment 2024-04-05] [HOLD for payment 2024-04-03] [Simplified Collect][Taxes] Value row shows decimal point with no digit when saving amount with a dot and with no digit after

[m-natarajan]

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.54-0
Reproducible in staging?: y
Reproducible in production?: new feature
If this was caught during regression testing, add the test name, ID and link from TestRail: n/a
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: applause internal team
Slack conversation:

Action Performed:

Precondition:

• User is an admin of Collect workspace.

1. Go to staging.new.expensify.com.
2. Go to Profile > Workspaces > Collect workspace.
3. Go to Taxes.
4. Click Add rate.
5. Click Value.
6. Enter an amount, followed by a dot (decimal).
7. Save the value with empty amount after the decimal.

Expected Result:

The Value row will not display decimal since there is no value after the dot.

Actual Result:

The Value row shows decimal with no digit after it.

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

Bug6418140_1710783219469.20240319_012920.mp4

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[github-actions]

👋 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.

[melvin-bot]

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

[ShridharGoel]

Proposal

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

Value row shows decimal point with no digit when saving amount with a decimal and with no digit after.

What is the root cause of that problem?

There's no logic to remove the decimal if there's no value after it. If the currency supports decimals, then inputs like 2000. will be treated as valid, which is okay but we should remove the decimal for more clarity.

App/src/components/AmountForm.tsx

Lines 97 to 115 in e4aff64

	const setNewAmount = useCallback(
	(newAmount: string) => {
	// Remove spaces from the newAmount value because Safari on iOS adds spaces when pasting a copied value
	// More info: https://github.com/Expensify/App/issues/16974
	const newAmountWithoutSpaces = MoneyRequestUtils.stripSpacesFromAmount(newAmount);
	// Use a shallow copy of selection to trigger setSelection
	// More info: https://github.com/Expensify/App/issues/16385
	if (!MoneyRequestUtils.validateAmount(newAmountWithoutSpaces, decimals)) {
	setSelection((prevSelection) => ({...prevSelection}));
	return;
	}
	const strippedAmount = MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces);
	const isForwardDelete = currentAmount.length > strippedAmount.length && forwardDeletePressedRef.current;
	setSelection((prevSelection) => getNewSelection(prevSelection, isForwardDelete ? strippedAmount.length : currentAmount.length, strippedAmount.length));
	onInputChange?.(strippedAmount);
	},
	[currentAmount, decimals, onInputChange],
	);

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

Add logic to remove the decimal if there's nothing after it.

let strippedAmount = MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces);
        
+        // Remove the decimal if there's no value after it
+        if (strippedAmount.endsWith('.')) {
+            strippedAmount = strippedAmount.slice(0, -1);
+        }

[allgandalf]

Proposal

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

What is the root cause of that problem?

In setNewAmount we have a util stripCommaFromAmount which strips the commas in inputs, but we don't have any utility used to check if there is any value after the decimal point:

App/src/components/AmountForm.tsx

Line 109 in ed6fbbc

const strippedAmount = MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces);

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

We need to create a new util function which checks for the values after decimal,

Here we have two options:

1. As given in the bug description, remove the decimal point if there are no digits after the point by using the below funciton:

function stripDecimalPointIfNoDigitsAfter(amount: string): string {
    return amount.replace(/(\.\d*?)0*$/, '$1');
}

OR

2. To be consistent with the app behavior for IOU, Split bills, we can assign 0 to one decimal point, so 5. would become 5.0, etc:

function addZeroAfterDecimalIfNeeded(amount: string): string {
    return amount.replace(/(\.\d*)$/, '$10');
}

I would prefer option 2 more :), but okay with implementation of option 1 as well

[ahmedGaber93]

Proposal

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

Value row shows decimal point with no digit when saving amount with a dot and with no digit after

What is the root cause of that problem?

we display and send to backend what user type in amount field without clean it.

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

we can add parseFloat to value here to be return `${parseFloat(value)}%`;

App/src/libs/actions/TaxRate.ts

Lines 15 to 17 in ed6fbbc

	function getTaxValueWithPercentage(value: string): string {
	return `${value}%`;
	}

and this method is used in the title and before send tax to backend

What alternative solutions did you explore? (Optional)

[luacmartins]

This was introduced here. This is NAB since this feature is only available on Collect policies in NewDot and users don't have access to that yet. I think we can address that as a follow up to the other tax PRs. cc @kosmydel @filip-solecki

[ShridharGoel]

@luacmartins @chiragsalian Any chances of making this external?

[chiragsalian]

I leave it to @luacmartins since i belive he's working on this feature. Also @ShridharGoel, i think your proposal might be incorrect since it will block a user from entering a decimal unless they click on the left arrow to go somewhere in between a number and then enter a decimal which is not the best user experience.

[kosmydel]

Hey, I am the author of the original PR and I will take care of this one.

Can the value rates be decimal? If so, how many decimal places should we show?
@luacmartins

[luacmartins]

Thanks for taking this on @kosmydel! It seems like OldDot accepts 16 decimals 😅 @trjExpensify can you confirm if we should keep the same behavior?

[luacmartins]

Confirmed internally that we should support up to 16 decimals!

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

• [Simplified Collect][Taxes] Tax editing and bulk actions #38208

If no regressions arise, payment will be issued on 2024-04-03. 🎊

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

• @shubham1206agra requires payment (Needs manual offer from BZ)
• @kosmydel does not require payment (Contractor)

[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:

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

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

• [Taxes] Add extra decimal places, new API  #38733

If no regressions arise, payment will be issued on 2024-04-05. 🎊

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

• @shubham1206agra requires payment (Needs manual offer from BZ)
• @kosmydel does not require payment (Contractor)

[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:

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

[melvin-bot]

⚠️ 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.

[sonialiap]

@luacmartins did the linked PR cause a regression?

[luacmartins]

This was a new feature and it was under development at the time, so no regressions

[melvin-bot]

Payment Summary

Upwork Job

• ROLE: @shubham1206agra paid $(AMOUNT) via Upwork (LINK)
• Contributor: @kosmydel is from an agency-contributor and not due payment

BugZero Checklist (@sonialiap)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[sonialiap]

@shubham1206agra $500 - offer sent

[sonialiap]

@shubham1206agra let me know if you didn't receive the offer on Upwork. If you have, it's ready for you to accept and be paid :D

[sonialiap]

Shubham asked for payment to be delayed - slack

[sonialiap]

Melvin, not overdue

[sonialiap]

Paid