[HOLD for payment 2023-05-25] [$1000] Pressing enter twice on selecting currency from search closes the send money/request money option

[kavimuru]

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. Open the app
2. Click on plus and click on send money
3. Select currency
4. Search for any currency, eg: USD
5. Press enter twice

Expected Result:

App should display enter amount page with selected currency as it does on single enter click

Actual Result:

App exits the send money/ request money option when we press enter twice to select the currency

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.8.8
Reproducible in staging?: y
Reproducible in production?: y
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

twice.enter.exits.send.money.mp4

Recording.449.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1682793162634169

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d1daa29666491c62
• Upwork Job ID: 1655705158516649984
• Last Price Increase: 2023-05-08

[MelvinBot]

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

[MelvinBot]

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

[AmjedNazzal]

Proposal

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

Pressing enter twice on selecting currency from search closes the send money/request money option

What is the root cause of that problem?

The problem begins in how we handle the keyboard enter event

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 59 to 67 in 846a50c

	this.unsubscribeEnter = KeyboardShortcut.subscribe(
	enterConfig.shortcutKey,
	() => {
	const focusedOption = this.state.allOptions[this.state.focusedIndex];
	if (!focusedOption) {
	return;
	}
	this.selectRow(focusedOption);

The problem with this is that the Enter key will call selectRow for as many times as it's pressed quickly which will eventually lead to Navigation.goBack to be called twice which will exit the entire modal

Screen.Recording.2023-05-02.at.11.15.18.PM.mov

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

We can set up a condition to wait for the excution of the selectRow function before calling it again thust resolving the issue while keeping the component flexible for handing multipleOptions and selection changing.

if(!isEnterDisabled){
    isEnterDisabled = true;
    let result = Promise.resolve(this.selectRow(focusedOption));
    InteractionManager.runAfterInteractions(() => {
        result.finally(() => isEnterDisabled = false);
    });
}

Result

Screen.Recording.2023-05-02.at.11.10.33.PM.mov

[MelvinBot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[alexpensify]

I'm catching up from being OOO on Friday and Monday, I'll try to test this one soon.

[alexpensify]

I've run out of time this week; I will test over the weekend to continue the process.

[alexpensify]

No update

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01d1daa29666491c62

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[alexpensify]

I tested and was able to verify. You can select any currency and click enter twice on your keyboard which will get the Send Money box to disappear.

[abdulrahuman5196]

@AmjedNazzal As per guidelines we don't recommended to

Posting large multi-line diffs (this is basically a PR).

Instead you could point to where you want to change(similar to the one you done in the root cause section) and what change you want to make.

Could you kindly remove the actual code diff in the proposal and update the proposal accordingly and respond here once it is ready to be reviewed?

Reference:
https://raw.githubusercontent.com/Expensify/App/main/contributingGuides/PROPOSAL_TEMPLATE.md
https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md

[abdulrahuman5196]

@AmjedNazzal And on the existing proposal

Finally in selectRow before resolving selectedOption and passing the selected option to onSelectRow, we flip the value of rowSelected to true which will prevent the next Enter press from calling selectRow again until the component is mounted again and the value of rowSelected is reset.

This would regress other flows like Multiple Row selection cases, as once we select a row it will block enter being pressed again until component mounts again

[AmjedNazzal]

Thank you @abdulrahuman5196 I've edited the proposal accordingly

[abdulrahuman5196]

@AmjedNazzal Thank you for updating the proposal. Your RCA is correct.

But it seems, "as once we select a row it will block 'enter' being pressed again until component mounts again unless it is a canSelectMultipleOptions enabled component" holds true. "unless it is a canSelectMultipleOptions enabled component" seems be the new update.

I wouldn't agree on this solution fully. Since it might cause a issue, in case we might want the user being able to switch his selection before confirming on the selection. Currently for currency selection its not the case, but we shouldn't limit BaseOptionsSelector.js component functionality to be capable of selecting only once during its mounted time.

The 'BaseOptionsSelector.js' is a common component which is being used in multiple places, so I would suggest to have our solution without regressing its functionality.

[alitoshmatov]

Proposal

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

Pressing enter twice on selecting currency from search closes the send money/request money option

What is the root cause of that problem?

When we first time press enter confirmCurrencySelection method is getting called

App/src/pages/iou/IOUCurrencySelection.js

Lines 128 to 131 in d2d9ce3

	confirmCurrencySelection(option) {
	IOU.setIOUSelectedCurrency(option.currencyCode);
	Navigation.goBack();
	}

And it is calling Navigation.goBack() which closes current screen but it is doing it with animation and delaying componentWillUnmount which is responsible for unsubscribing from keyboard.

Thus resulting in multiple same actions

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

We can listen to whether transition is happening in our modal stack

componentDidMount() {
        this.unsubscribeStart = this.props.navigation.addListener('transitionStart', (e) => {
            this.isNavigating = true;
        });

        this.unsubscribeEnd = this.props.navigation.addListener('transitionEnd', (e) => {
            this.isNavigating = false;
        });
    }

and can prevent any confirmCurrencySelection from firing again.

What alternative solutions did you explore? (Optional)

We can also rather than just going back here

App/src/pages/iou/IOUCurrencySelection.js

Line 130 in d2d9ce3

Navigation.goBack();

Manually show where to go to previous stack

   Navigation.drawerGoBack(navigation.getState().routes[lastElement].name);

[alitoshmatov]

Proposal

Updated with alternative solutions - #18265 (comment)

[AmjedNazzal]

@abdulrahuman5196 that's an excellent point, I didn't think of that. I've updated my proposal to account for that moving the checking logic to the currency selection component itself leaving BaseOptionSelector untouched to preserve it's flexibility for use in multiple components.

[abdulrahuman5196]

@alitoshmatov Thank you for proposing solution.
But I don't think it would be the best solution to block at Currency Selection page from navigation back twice.

The reason is, this issue is not limited to Currency selection page, any page using BaseOptionsSelector.js component(or any new page we implement) with a similar requirement would be affected. So we should be thinking of fixing at the source of the issue which in this case might be BaseOptionsSelector.js without limiting its functionality.

[abdulrahuman5196]

BugZero checklist:

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

This logic handling of multiple presses was never implemented and the requirement was also not identified in this case, so I don't we can point to a specific PR as bug creation PR.

Determine if we should create a regression test for this bug.

Yes

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.

Regression Test:

1. Open the app
2. Click on plus icon on LHN and click on send money
3. Click on the currency in the send money input page
4. Search for any currency form the list, eg: USD
5. Press enter twice
6. Verify that user should be navigated to send money input page like a currency being selected and the currency should have updated in the send money input page.

[melvin-bot]

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

[dylanexpensify]

I'm heading OOO tomorrow for about a week so adding another BZ member while I'm away to help keep the train moving! 🚂 Thanks @kadiealexander!!

[kadiealexander]

Regression test GH: https://github.com/Expensify/Expensify/issues/285621

[alexpensify]

@kadiealexander and @dylanexpensify - I appreciate your help here! I'm back online and removing your assignment.

[abdulrahuman5196]

@alexpensify Gentle ping on the payment.

[alexpensify]

It's payment time! @abdulrahuman5196 and @dhanashree-sawant please apply to the job here: https://www.upwork.com/jobs/~01d1daa29666491c62. After, I can continue with the payment process. Thank you!

@AmjedNazzal - I see you applied, so I'm working on the payment process in Upwork now.

[abdulrahuman5196]

@alexpensify Applied for the job in upwork.

[alexpensify]

Alright, offers have been sent to @AmjedNazzal and @abdulrahuman5196-- please accept in Upwork. Thank you!

@dhanashree-sawant when you apply, please let me know. Thank you!

[abdulrahuman5196]

@alexpensify ⏩ 🐎 Accepted the offer

[AmjedNazzal]

@alexpensify Accepted the offer :)

[dhanashree-sawant]

Hi @alexpensify, thanks I have applied to the job.

[alexpensify]

@AmjedNazzal and @abdulrahuman5196 have been paid via upwork.

Now waiting for @dhanashree-sawant to accept to complete that payment, then I'll close this GH.

[dhanashree-sawant]

Thanks @alexpensify, I have accepted the offer.

[alexpensify]

Perfect, everyone has been paid via Upwork and I closed the job. Great work!