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

[HOLD for payment 2022-09-07] [$2000] IOU - User is unable to modify the IOU amount - Reported by: @Santhosh-Sellavel #6154

Closed
isagoico opened this issue Nov 1, 2021 · 159 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@isagoico
Copy link

isagoico commented Nov 1, 2021

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. Navigate to a conversation
  2. Click on attachment button
  3. Click on request money
  4. On the amount, enter 1550
  5. Try to modify the amount

Expected Result:

User should be able to modify the amount by either using the < back caret from the end of the number, or highlighting any grouping of numbers and deleting them with a single keypress using the < caret.

Actual Result:

User is unable to tap to highlight the numbers on the BNP

Workaround:

Use the back caret < to delete/modify from the last number.

Expected Solution:

  1. Fix the issue on all platforms by using the selection and value prop of TextInput.
  2. Don't use setNativeProps as it won't be supported in Fabric
  3. Don't use any workaround like setTimeout, setImmediate, requestAnimationFrame etc.

Drawbacks of the expected solution:

  1. On Android, Copy / Paste menu will not work. But it'll eventually be fixed by [Ready for payment via newdot][$16000] Android - Copy / Paste / Cut menu is not displayed when selecting text in compose box - Reported by: @parasharrajat #8349
  2. From past experience, I'm assuming poor performance (at least on the dev build) when
    • typing quickly
    • or moving the cursor around

I believe that upgrading to the new Fabric architecture will fix both the drawbacks (anecdotal evidence).
We already have a ticket for the upgrade over here - #8503

Platform:

Where is this issue occurring?

  • iOS
  • Android

Version Number: 1.1.12-0

Reproducible in staging?: Yes
Reproducible in production?: Yes
Logs: https://stackoverflow.com/c/expensify/questions/4856
Upwork: https://www.upwork.com/jobs/~017b2406f1c8e307db

Notes/Photos/Videos: Any additional supporting documentation

Screen_Recording_20211030-033049_New.Expensify.mp4

Expensify/Expensify Issue URL:

Issue reported by: @Santhosh-Sellavel
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1635545417275400

View all open jobs on GitHub

@MelvinBot
Copy link

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

@deetergp
Copy link
Contributor

deetergp commented Nov 3, 2021

This can be external, but I want to verify that this is the bug and not the behavior we see on web/desktop.

@deetergp
Copy link
Contributor

deetergp commented Nov 3, 2021

The consensus is, let's do it 👍 Setting external…

@deetergp deetergp added the External Added to denote the issue can be worked on by a contributor label Nov 3, 2021
@MelvinBot
Copy link

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@deetergp deetergp removed their assignment Nov 3, 2021
@trjExpensify
Copy link
Contributor

The consensus is, let's do it 👍

👋 @deetergp, can you point me to this please? I can only find this thread with no discussion: https://expensify.slack.com/archives/C01GTK53T8Q/p1635545417275400

@deetergp
Copy link
Contributor

deetergp commented Nov 4, 2021

I believe this is the thread you want @trjExpensify.

@trjExpensify
Copy link
Contributor

Got it! So in terms of the expected behaviour then. Are we advocating for highlighting the number on press, and then tapping the caret < on the keyboard deletes just that selected number?

@MelvinBot
Copy link

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

@trjExpensify
Copy link
Contributor

^^ bump

@MelvinBot MelvinBot added Overdue and removed Overdue labels Nov 9, 2021
@MelvinBot
Copy link

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

@trjExpensify
Copy link
Contributor

@deetergp can you confirm this is the intended behaviour here, so I can update the OP accordingly?

@MelvinBot MelvinBot added Overdue and removed Overdue labels Nov 15, 2021
@deetergp
Copy link
Contributor

Got it! So in terms of the expected behaviour then. Are we advocating for highlighting the number on press, and then tapping the caret < on the keyboard deletes just that selected number?

Yes, that's correct. They should be able to either just < back from the end of the number, or highlight any grouping of numbers and delete them with a single keypress.

@MelvinBot
Copy link

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

@parasharrajat
Copy link
Member

This issue and #6285 are correlated. And the solution of other will directly affect this issue. So I suggest holding it until we are done with the other one which already has some proposals.

@trjExpensify
Copy link
Contributor

Cool, sounds good @parasharrajat. I'll pop this on hold and weekly for the time being.

@MelvinBot MelvinBot removed the Overdue label Nov 18, 2021
@trjExpensify trjExpensify added Weekly KSv2 and removed Daily KSv2 labels Nov 18, 2021
@trjExpensify trjExpensify changed the title IOU - User is unable to modify the IOU amount - Reported by: @Santhosh-Sellavel [Hold on #6285] IOU - User is unable to modify the IOU amount - Reported by: @Santhosh-Sellavel Nov 18, 2021
@rushatgabhane
Copy link
Member

awaiting a review from

@trjExpensify actually no, I'm yet to approve the PR
I'll ping @chiragsalian for a review after I've approved it 😃

@trjExpensify
Copy link
Contributor

Ah, I misinterpreted this then.. my bad!

@chiragsalian feel free to jump in for a review because we're 99% there.

@trjExpensify
Copy link
Contributor

@rushatgabhane, looks like you ran into a snag here with Storybook, right? #10147 (review)

@rushatgabhane
Copy link
Member

@trjExpensify yep, and it just got fixed in main.
Back to reviewing

@melvin-bot
Copy link

melvin-bot bot commented Aug 29, 2022

@eVoloshchak, @chiragsalian, @trjExpensify, @rushatgabhane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@trjExpensify
Copy link
Contributor

Happy Monday, Melvin! I think we're just waiting on @chiragsalian to review the PR now.

@melvin-bot
Copy link

melvin-bot bot commented Aug 31, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION 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 production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

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 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Aug 31, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 31, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.94-4 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 2022-09-07. 🎊

@melvin-bot melvin-bot bot changed the title [$2000] IOU - User is unable to modify the IOU amount - Reported by: @Santhosh-Sellavel [HOLD for payment 2022-09-07] [$2000] IOU - User is unable to modify the IOU amount - Reported by: @Santhosh-Sellavel Aug 31, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 4, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION 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 production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

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 Daily KSv2 and removed Weekly KSv2 labels Sep 7, 2022
@trjExpensify
Copy link
Contributor

👋 @chiragsalian @rushatgabhane I'm back from OOO. Confirming the breakdown in Slack.

@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION 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 production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

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.

@Christinadobrzyn
Copy link
Contributor

I followed up with @sig5 about compensation in our email thread.

@trjExpensify
Copy link
Contributor

@Santhosh-Sellavel - sent a contract for reporting
@rushatgabhane - sent a contract for C+
@eVoloshchak - settled up for the fix.

@trjExpensify
Copy link
Contributor

@rushatgabhane - settled up!

@trjExpensify
Copy link
Contributor

@Santhosh-Sellavel - settled up. Thanks everyone! 🎉

@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION 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 production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

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
Copy link

melvin-bot bot commented Oct 4, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION 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 production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

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.

@chiragsalian
Copy link
Contributor

No idea what regression this was linked to. I don't see any relevant regression posted related to this issue. Let me know if I'm missing something.

@trjExpensify
Copy link
Contributor

It's the first link in that message: #10693 (comment) CC: @parasharrajat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests