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

[$500] [POLISH][Search v2.1] Amount filter - Native - The digit exceeding the max allowed decimal places is briefly inserted into the input #47875

Open
rayane-d opened this issue Aug 22, 2024 · 145 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@rayane-d
Copy link
Contributor

rayane-d commented Aug 22, 2024

This is a follow-up issue.
Coming from: #47343 (comment) and #47343 (comment)

Bug in iOS and Android native apps: When entering a number, the digit exceeding the maximum allowed decimal places is briefly inserted into the input field before being deleted. They should not be inserted at all.

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-08-15.at.15.52.28.mp4

cc @luacmartins @289Adam289

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~017b65a6e5c7b77ba2
  • Upwork Job ID: 1827006416140957560
  • Last Price Increase: 2025-01-08
  • Automatic offers:
    • brunovjk | Reviewer | 104524279
Issue OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @kirillzyusko
@rayane-d rayane-d added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Aug 22, 2024
Copy link

melvin-bot bot commented Aug 22, 2024

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

@luacmartins luacmartins self-assigned this Aug 22, 2024
@luacmartins luacmartins added the External Added to denote the issue can be worked on by a contributor label Aug 23, 2024
@melvin-bot melvin-bot bot changed the title [Search v2.1] Amount filter - Native - The digit exceeding the max allowed decimal places is briefly inserted into the input [$250] [Search v2.1] Amount filter - Native - The digit exceeding the max allowed decimal places is briefly inserted into the input Aug 23, 2024
Copy link

melvin-bot bot commented Aug 23, 2024

Job added to Upwork: https://www.upwork.com/jobs/~017b65a6e5c7b77ba2

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

melvin-bot bot commented Aug 23, 2024

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

@luacmartins luacmartins changed the title [$250] [Search v2.1] Amount filter - Native - The digit exceeding the max allowed decimal places is briefly inserted into the input [$250] [POLISH][Search v2.1] Amount filter - Native - The digit exceeding the max allowed decimal places is briefly inserted into the input Aug 23, 2024
@eyobai
Copy link

eyobai commented Aug 23, 2024

Validation Function: Create a function to check if the input exceeds the allowed decimal places.
Event Listener: Use the input event to validate each change to the input field.
Immediate Correction: If the input is invalid, immediately revert to the last valid state.

Copy link

melvin-bot bot commented Aug 23, 2024

📣 @eyobai! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@huult
Copy link
Contributor

huult commented Aug 24, 2024

Proposal

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

The digit exceeding the max allowed decimal places is briefly inserted into the input

What is the root cause of that problem?

This issue occurs because we use onChangeText to validate the amount between lines 37 and line 40. If the amount is not valid, we return and remove the text change in the text input

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 = stripSpacesFromAmount(newAmount);
const replacedCommasAmount = replaceCommasWithPeriod(newAmountWithoutSpaces);
const withLeadingZero = addLeadingZero(replacedCommasAmount);
if (!validateAmount(withLeadingZero, 2)) {
return;
}
onInputChange?.(withLeadingZero);
},
[onInputChange],
);

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

We should check that the decimal limit has been reached and no more decimals can be entered before the text changes.

// src/components/AmountWithoutCurrencyForm.tsx#L17
+ const decimals = 2;

// src/components/AmountWithoutCurrencyForm.tsx#L37
- if (!validateAmount(withLeadingZero, 2)) {
+ if (!validateAmount(withLeadingZero, decimals)) {

// src/components/AmountWithoutCurrencyForm.tsx#L45
+     const getDecimalLength = (numStr: string) => {
+        const parts = numStr.split('.');
+        return parts[1] ? parts[1].length : 0;
+    };

// src/components/AmountWithoutCurrencyForm.tsx#L46
    return (
        <TextInput
            value={formattedAmount}
            onChangeText={setNewAmount}
            inputID={inputID}
            name={name}
            label={label}
            defaultValue={defaultValue}
            accessibilityLabel={accessibilityLabel}
            role={role}
            ref={ref}
            keyboardType={CONST.KEYBOARD_TYPE.DECIMAL_PAD}
+           maxLength={getDecimalLength(formattedAmount) === decimals ? formattedAmount.length : undefined}
            // eslint-disable-next-line react/jsx-props-no-spreading
            {...rest}
        />
    );
POC
Screen.Recording.2024-08-25.at.01.13.48.mov

@brunovjk
Copy link
Contributor

Thank you for the proposal @huult, but I tested it and it doesn't seem to resolve the issue. The RCA focuses on maxLength, but the problem occurs only on native platforms. If you believe your solution fixes this, please share a test branch for review.
For context, I just reproduced the issue on web and native. On web, typing 1111111111111111111111.111111111111 shows 11111111.11 without issue. However, on native (I only tested on iOS so far), the number stays as 1111111111111111111111.111111111111 for a couple of seconds before correcting to 11111111.11. The issue may require further investigation into why native platforms are behaving differently.

@huult
Copy link
Contributor

huult commented Aug 26, 2024

@brunovjk , Thanks for reviewing my proposal. I double-checked and found that it works when the user types from the keyboard but does not work when pasting a number into the input field or entering multiple decimal points. I am investigating further.

@AlfredoAlc
Copy link
Contributor

This most likely is related to a known issue for the TextInput component from react-native, specially for iOS platforms. See this issue for reference: facebook/react-native#36494

Copy link

melvin-bot bot commented Aug 28, 2024

@luacmartins, @isabelastisser, @brunovjk Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Aug 28, 2024
@brunovjk
Copy link
Contributor

Thank you, @AlfredoAlc, for the reference. It aligns with our issue, but I haven’t found a fix yet, only workarounds.
@huult, your proposal appears reasonable as it directly enforces decimal limits. However, despite the straightforward nature of the suggested changes, they haven’t resolved the issue in my tests. Do you have any new findings or a test branch to share? Thank you all!

@melvin-bot melvin-bot bot removed the Overdue label Aug 28, 2024
@huult
Copy link
Contributor

huult commented Aug 29, 2024

@brunovjk , test branch for my proposal

POC
Screen.Recording.2024-08-29.at.09.27.32.mp4

@brunovjk
Copy link
Contributor

@brunovjk , test branch for my proposal

Thank you @huult :D I just tested. Almost there, but the problem is not just in the decimals:

Screen.Recording.2024-08-29.at.10.36.14.mov

If the video above is no longer available when you watch it, let me know.

@huult
Copy link
Contributor

huult commented Aug 29, 2024

@brunovjk , I've updated the test branch to fix the issue you tested.

Screen.Recording.2024-08-29.at.21.24.51.mp4

@brunovjk
Copy link
Contributor

Thank you for your efforts, @huult. Unfortunately, the solution still isn’t working as expected on my end. As we can't attack the root cause directly, we might need a "workaround" to address this effectively. I’m confident we can find one that won’t introduce regressions. Let’s keep exploring options—I’ll revisit this next week, and we can involve an internal engineer if necessary.

Screen.Recording.2024-08-29.at.16.57.22.mov

Copy link

melvin-bot bot commented Aug 30, 2024

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

Copy link

melvin-bot bot commented Sep 2, 2024

@luacmartins, @isabelastisser, @brunovjk Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added Overdue and removed Help Wanted Apply this label when an issue is open to proposals by contributors labels Jan 13, 2025
@luacmartins
Copy link
Contributor

We decided to give the lib a try. @kirillzyusko this is all yours for implementation!

@brunovjk
Copy link
Contributor

Not overdue

@melvin-bot melvin-bot bot removed the Overdue label Jan 13, 2025
Copy link

melvin-bot bot commented Jan 17, 2025

@luacmartins, @kirillzyusko, @isabelastisser, @brunovjk Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Jan 17, 2025
@kirillzyusko
Copy link
Contributor

@MelvinBot not overdue. I'm working on a fix in #53695 Last remaining thing is to make it able to paste values with commas and automatically transform it to dot format. Currently working on that!

@brunovjk
Copy link
Contributor

Not overdue :D

@melvin-bot melvin-bot bot removed the Overdue label Jan 17, 2025
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jan 20, 2025
@kirillzyusko
Copy link
Contributor

I'm working on a fix in #53695 Last remaining thing is to make it able to paste values with commas and automatically transform it to dot format. Currently working on that!

Fixed the problem with pasting. PR #53695 is ready for review 👀

@brunovjk
Copy link
Contributor

Update: We are still working on the PR.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Feb 6, 2025
@melvin-bot melvin-bot bot changed the title [$500] [POLISH][Search v2.1] Amount filter - Native - The digit exceeding the max allowed decimal places is briefly inserted into the input [Due for payment 2025-02-13] [$500] [POLISH][Search v2.1] Amount filter - Native - The digit exceeding the max allowed decimal places is briefly inserted into the input Feb 6, 2025
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Feb 6, 2025
Copy link

melvin-bot bot commented Feb 6, 2025

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Feb 6, 2025

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.94-25 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2025-02-13. 🎊

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

Copy link

melvin-bot bot commented Feb 6, 2025

@brunovjk @isabelastisser @brunovjk The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

@brunovjk
Copy link
Contributor

Do not proceed with payment yet, we are working on this additional PR.

@luacmartins luacmartins changed the title [Due for payment 2025-02-13] [$500] [POLISH][Search v2.1] Amount filter - Native - The digit exceeding the max allowed decimal places is briefly inserted into the input [$500] [POLISH][Search v2.1] Amount filter - Native - The digit exceeding the max allowed decimal places is briefly inserted into the input Feb 11, 2025
@luacmartins luacmartins added Daily KSv2 and removed Awaiting Payment Auto-added when associated PR is deployed to production Weekly KSv2 labels Feb 11, 2025
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. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Archived in project
Development

No branches or pull requests