[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$1000] IOU - Selecting currency in BNP and pressing "CMD+ENTER" results in accidental redirection #23395

lanitochka17 · 2023-07-22T13:05:34Z

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:

Open https://staging.new.expensify.com/
Select any valid 1:1 chat room
Click on the + button in the compose box.
Click on "Request money" in the action menu
Enter any amount of currency
Click on the currency
In the currency selection menu, press the "CMD+ENTER" key combination

Expected Result:

The modal window of currency selection should close and the user should get to the previous window of currency amount input with correctly selected currency

Actual Result:

The modal currency selection window closes and the user is redirected to a random chat

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.3.44.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug6137428_Recording__104.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Inernal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~015204edc11a150c2b
Upwork Job ID: 1684299342131027968
Last Price Increase: 2023-08-02
Automatic offers:

mollfpr | Reviewer | 25995133
samh-nl | Contributor | 25995135

melvin-bot · 2023-07-22T13:05:39Z

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

melvin-bot · 2023-07-22T13:05:41Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

rayane-djouah · 2023-07-22T14:08:18Z

Proposal

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

The existing issue lies in the currency selection functionality of the app. After selecting currency for a money request in a chat room, a combination of "CMD+ENTER" keys results in an unwanted redirection to a random chat room, instead of just closing the currency selection menu and returning to the previous window.

What is the root cause of that problem?

The cause of the problem appears to be tied to the handling of the "CMD+ENTER" key combination in the app. Currently, the event listener in the currency selection menu is not configured to properly handle this key combination, leading to the accidental redirection.

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

To address this issue, we need to modify the code in the CurrencySymbolButton.js file.

App/src/components/CurrencySymbolButton.js

Lines 20 to 32 in b7d17e0

    
           function CurrencySymbolButton(props) { 
        
               return ( 
        
                   <Tooltip text={props.translate('iOUCurrencySelection.selectCurrency')}> 
        
                       <PressableWithoutFeedback 
        
                           onPress={props.onCurrencyButtonPress} 
        
                           accessibilityLabel={props.translate('iOUCurrencySelection.selectCurrency')} 
        
                           accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON} 
        
                       > 
        
                           <Text style={styles.iouAmountText}>{props.currencySymbol}</Text> 
        
                       </PressableWithoutFeedback> 
        
                   </Tooltip> 
        
               ); 
        
           }

Here's the proposed code modification to handle the "CMD+ENTER" key combination, prevent the default behavior (which is triggering the form submission) and close the currency selection modal:

function CurrencySymbolButton(props) {
    const handleKeyDown = (event) => {
        if (event.metaKey && event.key === 'Enter') {
            event.preventDefault();
            props.onCurrencyButtonPress();
        }
    };

    return (
        <Tooltip text={props.translate('iOUCurrencySelection.selectCurrency')}>
            <PressableWithoutFeedback
                onPress={props.onCurrencyButtonPress}
                accessibilityLabel={props.translate('iOUCurrencySelection.selectCurrency')}
                accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
                onKeyDown={handleKeyDown} // Added the keydown event listener here
            >
        ...

What alternative solutions did you explore? (Optional)

None.

samh-nl · 2023-07-22T15:01:03Z

Proposal

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

Pressing CMD+Enter while selecting the currency closes the modal.

What is the root cause of that problem?

In the BaseOptionsSelector component, CMD+Enter is subscribed to for selecting rows:

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 91 to 93 in b7d17e0

    
           const CTRLEnterConfig = CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER; 
        
           this.unsubscribeCTRLEnter = KeyboardShortcut.subscribe( 
        
               CTRLEnterConfig.shortcutKey,

The callback is executed twice due to a bug in react-native-key-command, where both Enter and CMD+Enter are processed for the CMD+Enter shortcut and executed. This is due to a missing check for the correct modifierFlags, which should also match in the case of no modifier flags present (i.e. just Enter) in the input, but expected in the listener.

https://github.com/Expensify/react-native-key-command/blob/5b093ed48c9bb90f047c7a3f1267e384298c1018/src/index.js#L96-L102

Subsequently, confirmCurrencySelection is called twice in IOUCurrencySelection leading to a double navigation, therefore closing the modal.

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

We should ensure that the input modifiers are correctly checked in react-native-key-command (this line):

- if (!validatedKeyCommand.modifierFlags && isInputMatched) {
+ if (!validatedKeyCommand.modifierFlags && isInputMatched && isCommandMatched) {

Instead of the two if-statements contained in this function, we can also simplify this by combining them to: if (isInputMatched && isCommandMatched) {

What alternative solutions did you explore? (Optional)

Alternatively, we can add a debounce time to prevent the callback from being executed twice.

MitchExpensify · 2023-07-24T18:01:00Z

@mountiny buddy check on this keyboard command actually being a platform wide expectation the same as hitting "Enter" on its own

mountiny · 2023-07-25T11:25:55Z

@MitchExpensify I am not sure about this one, seems like a niche edge case where I am not sure if we should worry about.

mountiny · 2023-07-25T11:29:17Z

@azimgd from your experience, have we been catching such flows before?

samh-nl · 2023-07-25T12:00:34Z

@MitchExpensify Only pressing Enter does not trigger the event listening to CMD+Enter. It's when the combination is pressed that both are processed, leading to a double execution of the callback.

It's a general problem when listening to a combination of keys where modifiers are involved.

mountiny · 2023-07-25T12:30:41Z

Yeah so I think if we want to solve this it would have to be in general

samh-nl · 2023-07-25T12:33:42Z

I traced it back to the react-native-key-command library, feel free to read the details in my proposal above.

MitchExpensify · 2023-07-26T20:27:03Z

Ok cool, as long as we solve in general this seems worth fixing

melvin-bot · 2023-07-26T20:27:12Z

Job added to Upwork: https://www.upwork.com/jobs/~015204edc11a150c2b

melvin-bot · 2023-07-26T20:27:15Z

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

melvin-bot · 2023-07-26T20:27:21Z

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

MitchExpensify · 2023-07-26T20:33:46Z

Not overdue, Melvin

mollfpr · 2023-07-31T11:55:14Z

@samh-nl The solution seems good and works well on Web and Desktop. I need to test the shortcut on the native platform based on this test case #17708.

melvin-bot · 2023-08-02T16:00:23Z

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

melvin-bot · 2023-10-13T21:57:26Z

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

build: upgrade to react-native-key-command 1.0.6 #28733

If no regressions arise, payment will be issued on 2023-10-20. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

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

@mollfpr requires payment offer (Reviewer)
@samh-nl requires payment offer (Contributor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-10-13T21:57:28Z

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:

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

melvin-bot · 2023-10-13T22:50:15Z

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

build: upgrade to react-native-key-command 1.0.6 #28733

If no regressions arise, payment will be issued on 2023-10-20. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

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

@mollfpr requires payment offer (Reviewer)
@samh-nl requires payment offer (Contributor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-10-13T22:50:17Z

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:

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

melvin-bot · 2023-10-16T21:12:55Z

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

build: upgrade to react-native-key-command 1.0.6 #28733

If no regressions arise, payment will be issued on 2023-10-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

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

@mollfpr requires payment offer (Reviewer)
@samh-nl requires payment offer (Contributor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-10-16T21:12:57Z

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:

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

melvin-bot · 2023-10-16T21:38:16Z

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

build: upgrade to react-native-key-command 1.0.6 #28733

If no regressions arise, payment will be issued on 2023-10-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

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

@mollfpr requires payment offer (Reviewer)
@samh-nl requires payment offer (Contributor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-10-16T21:38:18Z

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:

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

MitchExpensify · 2023-10-18T21:07:43Z

Wow whats up Melvin! Is this actually being held for payment at this point?

mollfpr · 2023-10-20T16:43:18Z

[@johnmlee101] The PR that introduced the bug has been identified. Link to the PR:
[@johnmlee101] 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:

Expensify/react-native-key-command#5

There's also another PR in the E/App that causing the issue, because it got resolve before the fix in upstream merged.

[@johnmlee101] 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:

The regression step should be enough.

[@mollfpr / @samh-nl] Determine if we should create a regression test for this bug.
[@mollfpr / @samh-nl] 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.

Click on the FAB
Go to Request money
Click on the currency symbol to open the currency selector
Press CMD+Enter
Verify that the currency selector is closed, returning the user to the enter money amount page
👍 or 👎

MitchExpensify · 2023-10-22T18:37:18Z

Mind helping me understand if the fix has been on prod for 7 days without regression @mollfpr? The Melvin updates are causing me doubt

mollfpr · 2023-10-23T02:34:57Z

@MitchExpensify The PR is included in the production release: https://github.com/Expensify/App/releases/tag/1.3.83-11, which is on Oct 14th.

MitchExpensify · 2023-10-25T17:10:50Z

Payment summary:

$500 (50% 8+ day penalty) @mollfpr requires payment offer (Reviewer)
$500 (50% 8+ day penalty) @samh-nl requires payment offer (Contributor)

MitchExpensify · 2023-10-25T17:11:19Z

Mind taking a peak at the BZ steps @johnmlee101 #23395 (comment) 👀

mollfpr · 2023-10-25T17:18:31Z

@MitchExpensify I already have the Upwork contract.

MitchExpensify · 2023-10-28T21:22:40Z

Paid and contracts ended!

lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 22, 2023

melvin-bot bot assigned MitchExpensify Jul 22, 2023

melvin-bot bot added the Overdue label Jul 26, 2023

melvin-bot bot removed the Overdue label Jul 26, 2023

MitchExpensify added External Added to denote the issue can be worked on by a contributor Overdue labels Jul 26, 2023

melvin-bot bot changed the title ~~IOU - Selecting currency in BNP and pressing "CMD+ENTER" results in accidental redirection~~ [$1000] IOU - Selecting currency in BNP and pressing "CMD+ENTER" results in accidental redirection Jul 26, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 26, 2023

melvin-bot bot assigned mollfpr Jul 26, 2023

melvin-bot bot added Overdue and removed Overdue labels Jul 26, 2023

melvin-bot bot removed the Overdue label Jul 31, 2023

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 13, 2023

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 16, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 20, 2023

melvin-bot bot added the Overdue label Oct 25, 2023

melvin-bot bot removed the Overdue label Oct 25, 2023

MitchExpensify closed this as completed Oct 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$1000] IOU - Selecting currency in BNP and pressing "CMD+ENTER" results in accidental redirection #23395

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$1000] IOU - Selecting currency in BNP and pressing "CMD+ENTER" results in accidental redirection #23395

lanitochka17 commented Jul 22, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Jul 22, 2023

melvin-bot bot commented Jul 22, 2023

rayane-djouah commented Jul 22, 2023

samh-nl commented Jul 22, 2023 •

edited

Loading

MitchExpensify commented Jul 24, 2023

mountiny commented Jul 25, 2023

mountiny commented Jul 25, 2023

samh-nl commented Jul 25, 2023 •

edited

Loading

mountiny commented Jul 25, 2023

samh-nl commented Jul 25, 2023

MitchExpensify commented Jul 26, 2023

melvin-bot bot commented Jul 26, 2023

melvin-bot bot commented Jul 26, 2023

melvin-bot bot commented Jul 26, 2023

MitchExpensify commented Jul 26, 2023

mollfpr commented Jul 31, 2023

melvin-bot bot commented Aug 2, 2023

melvin-bot bot commented Oct 13, 2023

melvin-bot bot commented Oct 13, 2023

melvin-bot bot commented Oct 13, 2023

melvin-bot bot commented Oct 13, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023 •

edited by MitchExpensify

Loading

MitchExpensify commented Oct 18, 2023

mollfpr commented Oct 20, 2023

MitchExpensify commented Oct 22, 2023 •

edited

Loading

mollfpr commented Oct 23, 2023

MitchExpensify commented Oct 25, 2023

MitchExpensify commented Oct 25, 2023

mollfpr commented Oct 25, 2023

MitchExpensify commented Oct 28, 2023

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$1000] IOU - Selecting currency in BNP and pressing "CMD+ENTER" results in accidental redirection #23395

[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$1000] IOU - Selecting currency in BNP and pressing "CMD+ENTER" results in accidental redirection #23395

Comments

lanitochka17 commented Jul 22, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Jul 22, 2023

melvin-bot bot commented Jul 22, 2023

Bug0 Triage Checklist (Main S/O)

rayane-djouah commented Jul 22, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

samh-nl commented Jul 22, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

MitchExpensify commented Jul 24, 2023

mountiny commented Jul 25, 2023

mountiny commented Jul 25, 2023

samh-nl commented Jul 25, 2023 • edited Loading

mountiny commented Jul 25, 2023

samh-nl commented Jul 25, 2023

MitchExpensify commented Jul 26, 2023

melvin-bot bot commented Jul 26, 2023

melvin-bot bot commented Jul 26, 2023

melvin-bot bot commented Jul 26, 2023

MitchExpensify commented Jul 26, 2023

mollfpr commented Jul 31, 2023

melvin-bot bot commented Aug 2, 2023

melvin-bot bot commented Oct 13, 2023

melvin-bot bot commented Oct 13, 2023

melvin-bot bot commented Oct 13, 2023

melvin-bot bot commented Oct 13, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023

melvin-bot bot commented Oct 16, 2023 • edited by MitchExpensify Loading

MitchExpensify commented Oct 18, 2023

mollfpr commented Oct 20, 2023

MitchExpensify commented Oct 22, 2023 • edited Loading

mollfpr commented Oct 23, 2023

MitchExpensify commented Oct 25, 2023

MitchExpensify commented Oct 25, 2023

mollfpr commented Oct 25, 2023

MitchExpensify commented Oct 28, 2023

lanitochka17 commented Jul 22, 2023 •

edited by melvin-bot bot

Loading

samh-nl commented Jul 22, 2023 •

edited

Loading

samh-nl commented Jul 25, 2023 •

edited

Loading

melvin-bot bot commented Oct 16, 2023 •

edited by MitchExpensify

Loading

MitchExpensify commented Oct 22, 2023 •

edited

Loading