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

Money is sent/requested multiple times if the enter key is pressed multiple times reported by @adeel0202 #12714

Closed
kavimuru opened this issue Nov 14, 2022 · 71 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Nov 14, 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!


Actions Performed:

  1. Go to any chat
  2. Click on the plus icon
  3. Click Send money or Request Money
  4. Enter any amount
  5. Click Next
  6. Press enter key multiple times

Expected Result:

Money is sent only once

Actual Result:

Money is sent multiple times

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • Desktop App
  • Mobile Web
  • iOS
  • Android (should be reproducible, right now it's crashing in the latest build which is a kI)

Version Number: 1.2.27-3
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:

https://user-images.githubusercontent.com/43996225/201730915-2bfd3b1f-8e65-4290-bf28-0151ad0a578a.mov
https://user-images.githubusercontent.com/43996225/201730938-ef44e1a4-8ef5-42f9-95ee-f9c3ae4383a7.mp4

Expensify/Expensify Issue URL:
Issue reported by: @adeel0202
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1668413790082549

View all open jobs on GitHub

@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 14, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 14, 2022

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

@Puneet-here
Copy link
Contributor

Proposal

The issue is also reproducible while splitting bill or requesting money

To fix this we can show a loader at button like this

                    ? (
                        <SettlementButton
                            isDisabled={shouldDisableButton}
                            onPress={this.confirm}
                            shouldShowPaypal={Boolean(recipient.payPalMeAddress)}
                            enablePaymentsRoute={ROUTES.IOU_SEND_ENABLE_PAYMENTS}
                            addBankAccountRoute={ROUTES.IOU_SEND_ADD_BANK_ACCOUNT}
                            addDebitCardRoute={ROUTES.IOU_SEND_ADD_DEBIT_CARD}
                            currency={this.props.iou.selectedCurrencyCode}
+                           isLoading={this.props.iou.loading}
                        />
                    ) : (
                        <ButtonWithMenu
+                           isLoading={this.props.iou.loading}
                            isDisabled={shouldDisableButton}
                            onPress={(_event, value) => this.confirm(value)}
                            options={this.splitOrRequestOptions}
                        />
                    )}

<SettlementButton
isDisabled={shouldDisableButton}
onPress={this.confirm}
shouldShowPaypal={Boolean(recipient.payPalMeAddress)}
enablePaymentsRoute={ROUTES.IOU_SEND_ENABLE_PAYMENTS}
addBankAccountRoute={ROUTES.IOU_SEND_ADD_BANK_ACCOUNT}
addDebitCardRoute={ROUTES.IOU_SEND_ADD_DEBIT_CARD}
currency={this.props.iou.selectedCurrencyCode}
/>
) : (
<ButtonWithMenu
isDisabled={shouldDisableButton}

@miljakljajic
Copy link
Contributor

Adding external label

@miljakljajic miljakljajic added the External Added to denote the issue can be worked on by a contributor label Nov 15, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 15, 2022

Current assignee @miljakljajic is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Nov 15, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 15, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 15, 2022

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

@melvin-bot melvin-bot bot changed the title Money is sent multiple times if the enter key is pressed multiple times reported by @adeel0202 [$250] Money is sent multiple times if the enter key is pressed multiple times reported by @adeel0202 Nov 15, 2022
@miljakljajic
Copy link
Contributor

@Beamanator , can you confirm this can be worked on by an external engineer?

@aimane-chnaif - take a look at Puneet's proposal when you have a chance. Thank you!

@Emz27
Copy link

Emz27 commented Nov 15, 2022

Proposal

  • add debounce/throttle to onPress event using lodash

Cause

  • because of render lag, the javascript code cant keep up with native events

@Puneet-here proposal will help but It can still be replicated if the QA use their magic fingers since react state render is asynchronous

This can be fully solved with debounce/throttle on client + loader + backend limit

@Beamanator
Copy link
Contributor

@miljakljajic Yep I agree this should be able to be worked externally 👍

@aimane-chnaif
Copy link
Contributor

@Puneet-here Your proposal still doesn't work, especially on mobile. I tested your solution. When press button with multiple fingers at the same time, I could send 10+ times from my android phone. Easy reproducible on HT account, slow device. @Emz27's analysis is correct.

@Emz27 can you elaborate more detail how to apply debounce/throttle to onPress? Please share code snippet what you're trying to achieve.

@Emz27
Copy link

Emz27 commented Nov 17, 2022

@aimane-chnaif heres the diff's

  • we create debounce/throttle func instance upon creation of the page
  • I chose throttle here because the debounce spawns a timer while throttle will just use timestamp
  • the 800ms throttle time will give the component enough time to render loading and disabled button property changes which prevents any succeeding press

throttle reference

diff --git a/src/components/IOUConfirmationList.js b/src/components/IOUConfirmationList.js
index 4f4eb415e..d7f81de40 100755
--- a/src/components/IOUConfirmationList.js
+++ b/src/components/IOUConfirmationList.js
@@ -118,6 +118,7 @@ class IOUConfirmationList extends Component {
 
         this.toggleOption = this.toggleOption.bind(this);
         this.confirm = this.confirm.bind(this);
+        this.confirmThrottle = _.throttle(this.confirm, 800, {leading: true, trailing: false});
     }
 
     /**
@@ -273,7 +274,7 @@ class IOUConfirmationList extends Component {
     render() {
         const selectedParticipants = this.getSelectedParticipants();
         const shouldShowSettlementButton = this.props.iouType === CONST.IOU.IOU_TYPE.SEND;
-        const shouldDisableButton = selectedParticipants.length === 0;
+        const shouldDisableButton = selectedParticipants.length === 0 || this.props.iou.loading;
         const recipient = this.state.participants[0];
         const canModifyParticipants = !this.props.isIOUAttachedToExistingChatReport && this.props.hasMultipleParticipants;
 
@@ -282,7 +283,7 @@ class IOUConfirmationList extends Component {
                 sections={this.getSections()}
                 value={this.props.comment}
                 onSelectRow={canModifyParticipants ? this.toggleOption : undefined}
-                onConfirmSelection={this.confirm}
+                onConfirmSelection={this.confirmThrottle}
                 onChangeText={this.props.onUpdateComment}
                 textInputLabel={this.props.translate('iOUConfirmationList.whatsItFor')}
                 placeholderText={this.props.translate('common.optional')}
@@ -300,18 +301,20 @@ class IOUConfirmationList extends Component {
                     ? (
                         <SettlementButton
                             isDisabled={shouldDisableButton}
-                            onPress={this.confirm}
+                            onPress={this.confirmThrottle}
                             shouldShowPaypal={Boolean(recipient.payPalMeAddress)}
                             enablePaymentsRoute={ROUTES.IOU_SEND_ENABLE_PAYMENTS}
                             addBankAccountRoute={ROUTES.IOU_SEND_ADD_BANK_ACCOUNT}
                             addDebitCardRoute={ROUTES.IOU_SEND_ADD_DEBIT_CARD}
                             currency={this.props.iou.selectedCurrencyCode}
+                            isLoading={this.props.iou.loading}
                         />
                     ) : (
                         <ButtonWithMenu
                             isDisabled={shouldDisableButton}
-                            onPress={(_event, value) => this.confirm(value)}
+                            onPress={(_event, value) => this.confirmThrottle(value)}
                             options={this.splitOrRequestOptions}
+                            isLoading={this.props.iou.loading}
                         />
                     )}
             />

@bondydaa
Copy link
Contributor

why does our API and Auth allow this? This should be fixed at every level of the stack not just in JS.

@bondydaa bondydaa assigned bondydaa and unassigned Beamanator and aimane-chnaif Nov 17, 2022
@bondydaa
Copy link
Contributor

going to take this over and investigate it a bit more since I suspect we need to update the API and Auth.

@bondydaa bondydaa removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Nov 17, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 21, 2022

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

@melvin-bot melvin-bot bot added the Overdue label Nov 21, 2022
@bondydaa
Copy link
Contributor

sorry haven't had a chance to dig into this yet, been focused on the Upwork Integration auth command and had a bunch of ring1/IFR chores last week. Will get started on this tomorrow at the latest

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Dec 20, 2022
@0xmiros
Copy link
Contributor

0xmiros commented Dec 20, 2022

PR is ready for review

@bondydaa bondydaa removed their assignment Dec 26, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jan 2, 2023

@youssef-lr, @miljakljajic, @aimane-chnaif, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot
Copy link

melvin-bot bot commented Jan 4, 2023

@youssef-lr, @miljakljajic, @aimane-chnaif, @0xmiroslav Huh... This is 4 days overdue. Who can take care of this?

@aimane-chnaif
Copy link
Contributor

PR was deployed to production on Dec 27.
Issue title was not updated automatically.

@aimane-chnaif
Copy link
Contributor

@miljakljajic kindly bump ^

@miljakljajic
Copy link
Contributor

Apologies, @aimane-chnaif. I will make the relevant payments today.

@dylanexpensify
Copy link
Contributor

job post here! cc @aimane-chnaif @adeel0202

@aimane-chnaif
Copy link
Contributor

I think bounty can be $1000 because it's now base price and worthy of it based on difficulty

@adeel0202
Copy link
Contributor

Thanks, @dylanexpensify. I have applied.

@miljakljajic
Copy link
Contributor

Contracts extended @adeel0202 @0xmiroslav @aimane-chnaif

@adeel0202
Copy link
Contributor

Thanks @miljakljajic, but I'm eligible for reporting bonus only i.e. $250

@miljakljajic
Copy link
Contributor

That's no problem, thank you for accepting the contract. I've paid you now and I'll end the contract.

@miljakljajic
Copy link
Contributor

@aimane-chnaif once you accept the offer we can close this out. Thank you!

@aimane-chnaif
Copy link
Contributor

I think bounty can be $1000 because it's now base price and worthy of it based on difficulty

did we confirm final amount including timeline bonus?
cc: @youssef-lr

@miljakljajic
Copy link
Contributor

Confirmed! Sent offer including timeline bonus.

@miljakljajic
Copy link
Contributor

@aimane-chnaif if you accept the offer I can pay you and get this closed out!

@aimane-chnaif
Copy link
Contributor

@aimane-chnaif if you accept the offer I can pay you and get this closed out!

@miljakljajic Accepted, thanks.
Also, I got 2 additional offers that are not needed. I haven't accepted them yet so please feel free to cancel them.

@miljakljajic
Copy link
Contributor

Paid and incorrect offers withdrawn, thank you @aimane-chnaif.

@adeel0202
Copy link
Contributor

@miljakljajic, kindly end my contract as well. Thank you

@0xmiros
Copy link
Contributor

0xmiros commented Jan 16, 2023

@miljakljajic I only got $250

@miljakljajic
Copy link
Contributor

miljakljajic commented Jan 16, 2023

Apologies @0xmiroslav - this GH was originally created before we increased the bounty from 250 to 1000 so that is technically the correct payment. However, I've extended another offer to bring the payment up to 1000 + timeline bonus given that it's a rather unique situation. Thank you for your work - please accept the new offer.

@miljakljajic miljakljajic reopened this Jan 16, 2023
@0xmiros
Copy link
Contributor

0xmiros commented Jan 16, 2023

No problem. Accepted new offer. Thanks @miljakljajic

@miljakljajic
Copy link
Contributor

Paid, contract ended - closing out again.

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

No branches or pull requests