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

Web - IOU - Numbers added after decimal point are appearing preceding the decimal point #10928

Closed
kavimuru opened this issue Sep 9, 2022 · 26 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Reviewing Has a PR in review Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Sep 9, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Go to staging.new.expensify.com and login
  2. Select a group and select option "Split bill"
  3. Enter a decimal point and then numbers

Expected Result:

Numbers are added after the decimal point

Actual Result:

Numbers are added before the decimal point, user can't add decimal amount
(Same scenario with Request money and Send money as well)

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web

Version Number: v1.1.99-2
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Recording.518.mp4
@melvin-bot
Copy link

melvin-bot bot commented Sep 9, 2022

Triggered auto assignment to @techievivek (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@melvin-bot melvin-bot bot added the Overdue label Sep 12, 2022
@techievivek
Copy link
Contributor

@eVoloshchak Is this in any way related to your changes?

@melvin-bot melvin-bot bot removed the Overdue label Sep 12, 2022
@eVoloshchak
Copy link
Contributor

@eVoloshchak Is this in any way related to your changes?

Yes, I think it's a regression from #10028. I'll be able to revert the PR and verify this in ±14 hours.

@techievivek
Copy link
Contributor

@eVoloshchak Any updates on this one?

@melvin-bot melvin-bot bot removed the Overdue label Sep 15, 2022
@eVoloshchak
Copy link
Contributor

@eVoloshchak Any updates on this one?

Sorry, having trouble with free time
It is related to #10028 simply because this PR introduced adding preceding zero if the user entered the decimal point, so it was an edge case I missed in this PR
Somehow this doesn't happen if you have currency after the amount

Screen.Recording.2022-09-15.at.21.13.34.mov

I'll look into this on the weekend

@parasharrajat
Copy link
Member

Hmm, I am sure I tested it on that PR. Check the attached web video on that PR. Could this be related to #10147

@eVoloshchak
Copy link
Contributor

Could this be related to #10147?

I don't think so, this PR changed only the updateAmountNumberPad method (the one used for on-screen keyboard), while this bug is on Web, so the problem is with updateAmount method, which was unchanged in #10147

@eVoloshchak
Copy link
Contributor

Now that I think about it more, yes, it could be related to #10147. It introduced some changes to the TextInput, mainly the selection being controlled by state, there could be bugs associated with this
I'll check it, thank you

@eVoloshchak
Copy link
Contributor

I checked it, #10147 was the PR that caused this, thanks @parasharrajat for pointing this out.

Solution:
This is a controlled input, but currently in updateAmount we update amount, but don't update the selection. Let's fix that

updateAmount(text) {
    this.setState((prevState) => {
        const amount = this.addLeadingZero(this.replaceAllDigits(text, this.props.fromLocaleDigit));
+       const selection = this.getNewSelection(prevState.selection, prevState.amount.length, amount.length);
        return this.validateAmount(amount)
-            ? {amount: this.stripCommaFromAmount(amount)}
+            ? {amount: this.stripCommaFromAmount(amount), selection}
            : prevState;
    });
}
Screen.Recording.2022-09-18.at.17.19.46.mov

@melvin-bot melvin-bot bot added the Overdue label Sep 18, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 2022

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

@techievivek
Copy link
Contributor

Just checked with the team on how we need to handle this. I will put the updates here. Thanks

@melvin-bot melvin-bot bot removed the Overdue label Sep 19, 2022
@rushatgabhane
Copy link
Member

I'll work on the RCA

@techievivek
Copy link
Contributor

I just discussed it with the team and since this is a clear case of regression we would want @eVoloshchak to push a PR fixing this.

@eVoloshchak
Copy link
Contributor

eVoloshchak commented Sep 19, 2022

If the solution I proposed looks good to you, I can push a PR

@rushatgabhane
Copy link
Member

@eVoloshchak yea, let's do it!

@techievivek
Copy link
Contributor

@eVoloshchak Hi, have you pushed a PR fixing it?

@eVoloshchak
Copy link
Contributor

@eVoloshchak Hi, have you pushed a PR fixing it?

PR is up

@melvin-bot melvin-bot bot added the Overdue label Sep 23, 2022
@techievivek techievivek added the Reviewing Has a PR in review label Sep 23, 2022
@melvin-bot melvin-bot bot removed the Overdue label Sep 23, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2022

@techievivek Eep! 4 days overdue now. Issues have feelings too...

@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2022

@techievivek Still overdue 6 days?! Let's take care of this!

@melvin-bot
Copy link

melvin-bot bot commented Oct 7, 2022

@techievivek Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

@melvin-bot
Copy link

melvin-bot bot commented Oct 11, 2022

@techievivek 12 days overdue now... This issue's end is nigh!

@melvin-bot melvin-bot bot removed the Daily KSv2 label Oct 14, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 14, 2022

This issue has not been updated in over 14 days. @techievivek eroding to Weekly issue.

@melvin-bot melvin-bot bot added the Weekly KSv2 label Oct 14, 2022
@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@puneetlath puneetlath added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 19, 2022
@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Third week) @techievivek Are we ok to close this one?

@techievivek
Copy link
Contributor

Sorry, I agree we can close this now.

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. Engineering Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants