[HOLD for payment 2024-04-03] [$500] IOU - CMD+ENTER command takes you to the IOU confirmation page without selecting members #35377

kavimuru · 2024-01-30T10:57:51Z

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

Version Number: 1.4.33-0
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
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

Open https://staging.new.expensify.com/
Log in under your HT account
Tap on the green plus button (FAB)
Select Request money/Manual
Select any currency and amount
Press the CMD + ENTER key combination

Expected Result:

The CMD + ENTER command should not work in the member selection menu without selected members.

Actual Result:

The CMD + ENTER command takes you to the IOU confirmation page without selecting members.

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?

Screenshots/Videos

Add any screenshot/video evidence

Bug6360685_1706608112809.Recording__1215.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01072c9895579ae1ce
Upwork Job ID: 1752285003381895168
Last Price Increase: 2024-02-20
Automatic offers:

mollfpr | Reviewer | 0
Krishna2323 | Contributor | 0

melvin-bot · 2024-01-30T10:57:56Z

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

melvin-bot · 2024-01-30T10:57:57Z

Job added to Upwork: https://www.upwork.com/jobs/~01072c9895579ae1ce

melvin-bot · 2024-01-30T10:58:02Z

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

ZhenjaHorbach · 2024-01-30T11:07:34Z

Proposal

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

IOU - CMD+ENTER command takes you to the IOU confirmation page without selecting members

What is the root cause of that problem?

The main problem with issue is that we don't have any condition for confirm selection
And when we press CMD + ENTER we just call the function regardless of the number of participants

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

Lines 274 to 280 in 8c0eee2

    
           const handleConfirmSelection = useCallback(() => { 
        
               if (shouldShowSplitBillErrorMessage) { 
        
                   return; 
        
               } 
        
               navigateToSplit(); 
        
           }, [shouldShowSplitBillErrorMessage, navigateToSplit]);

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

To fix this issue we can add new condition

        if (!participants.length) {
            return;
        }

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

Lines 274 to 280 in 8c0eee2

    
           const handleConfirmSelection = useCallback(() => { 
        
               if (shouldShowSplitBillErrorMessage) { 
        
                   return; 
        
               } 
        
               navigateToSplit(); 
        
           }, [shouldShowSplitBillErrorMessage, navigateToSplit]);

And here

App/src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js

Lines 262 to 268 in c53276d

    
           const handleConfirmSelection = useCallback(() => { 
        
               if (shouldShowSplitBillErrorMessage) { 
        
                   return; 
        
               } 
        
               onFinish(); 
        
           }, [shouldShowSplitBillErrorMessage, onFinish]);

What alternative solutions did you explore? (Optional)

As an alternative, we can select a current participant and use logic during selecting an element when we have selectedOptions === 0

In order to preserve the old logic and make these changes for all lists for uniformity (Which is logical for me, but we can always add a param if we want to disable new logic)
We can update the code like:

    useKeyboardShortcut(
        CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER,
        (e) => {
            if (!flattenedSections.selectedOptions.length) {
                selectFocusedOption();
                return;
            }

												
            onConfirm ? onConfirm(e) : selectFocusedOption();
        },
        {
            captureOnInputs: true,
            shouldBubble: !flattenedSections.allOptions[focusedIndex],
            isActive: !disableKeyboardShortcuts && isFocused,
        },
    );

App/src/components/SelectionList/BaseSelectionList.tsx

Lines 372 to 376 in 82ae62f

    
           useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, onConfirm ?? selectFocusedOption, { 
        
               captureOnInputs: true, 
        
               shouldBubble: !flattenedSections.allOptions[focusedIndex], 
        
               isActive: !disableKeyboardShortcuts && isFocused, 
        
           });

Krishna2323 · 2024-01-30T11:17:49Z

Proposal

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

IOU - CMD+ENTER command takes you to the IOU confirmation page without selecting members

What is the root cause of that problem?

We return from onConfirm when the participants length is not greater than one.

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

Lines 274 to 280 in 8c0eee2

    
           const handleConfirmSelection = useCallback(() => { 
        
               if (shouldShowSplitBillErrorMessage) { 
        
                   return; 
        
               } 
        
               navigateToSplit(); 
        
           }, [shouldShowSplitBillErrorMessage, navigateToSplit]);

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

We should check the length of the participants, if there is only one participant we will call addSingleParticipant.

Steps:

Update callback for CTRL_ENTER keypress to pass the focusedOption to onConfirm callback.

App/src/components/SelectionList/BaseSelectionList.tsx

Line 372 in 82ae62f

    
           useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, onConfirm ?? selectFocusedOption, {

Update second parameter to:

        () => {
            if (onConfirm && flattenedSections.selectedOptions.length) {
                onConfirm();
                return;
            }

            selectFocusedOption();
        },

In MoneyTemporaryForRefactorRequestParticipantsSelector update handleConfirmSelection function to call addSingleParticipant(lastfocusedOption); if the participants list is empty.

App/src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js

Line 262 in 82ae62f

const handleConfirmSelection = useCallback(() => {

Add a if check here and use the parameter passed from SelcetionList:

    const handleConfirmSelection = useCallback(
        (lastfocusedOption) => {
            
            if (participants.length === 0 && lastfocusedOption) {
                addSingleParticipant(lastfocusedOption);
            }

Update the types file for BaseSelectionListProps, to also TItem as onConfirm callback parameter.

App/src/components/SelectionList/types.ts

Line 187 in 82ae62f

onConfirm?: (e?: GestureResponderEvent | KeyboardEvent | undefined) => void;

Add TItem here as a optional parameter:

onConfirm?: (e?: GestureResponderEvent | KeyboardEvent | TItem | undefined) => void;

We pass onConfirm callback in from multiple components, we should also update there if needed, we use in RoomInvitePage, RoomMembersPage, MoneyTemporaryForRefactorRequestParticipantsSelector, MoneyRequestParticipantsSelector, WorkspaceInvitePage & WorkspaceMembersPage.

Result

access.mp4

Alternative

We can call selectFocusedOption directly from BaseSelectionList, if the selected options length is 0 and we pressed CMD+Enter. Or if we have selected options and onConfirm callback then we will call onConfirm callback

Add a if check here and use the parameter passed from SelcetionList:

        () => {
            if (onConfirm && flattenedSections.selectedOptions.length) {
                onConfirm();
                return;
            }

            selectFocusedOption();
        },

Krishna2323 · 2024-01-30T11:28:32Z

@ZhenjaHorbach, your proposal was updated with alternative after mine initial solution.
cc: @mollfpr

ZhenjaHorbach · 2024-01-30T11:29:45Z

@ZhenjaHorbach, your proposal was updated with alternative after mine initial solution. cc: @mollfpr

Oh, sorry
I thought that you closed your proposition ))

Krishna2323 · 2024-01-30T11:30:45Z

Yeah, the first one was closed.

mollfpr · 2024-01-30T14:36:48Z

@ZhenjaHorbach I think it's better we alternatively select the focused option instead blocking it.

@Krishna2323 Could you elaborate on this?

Update callback for CTRL_ENTER keypress to pass the focusedOption to onConfirm callback.

Update the types file for BaseSelectionListProps, to also TItem as onConfirm callback parameter.

We pass onConfirm callback in from multiple components, we should also update there if needed, we use in RoomInvitePage, RoomMembersPage, MoneyTemporaryForRefactorRequestParticipantsSelector, MoneyRequestParticipantsSelector, WorkspaceInvitePage & WorkspaceMembersPage.

What to update?

Krishna2323 · 2024-01-30T14:48:22Z

@mollfpr, proposal updated to include more details.

ZhenjaHorbach · 2024-01-30T15:38:31Z

In fact, being able to select the current element creates a lot of questions

How should it behave when we have already selected several elements?
When we try to select the selected element, what is the behavior?
Are these changes needed within the BaseSelectionList for all screens or only within the IOUConfirmationPage ?)

But if we want to save old logic and only add the ability to choose an element when we don't have selected elements
I updated proposal

mollfpr · 2024-01-30T15:51:37Z

How should it behave when we have already selected several elements?

Go to confirm page.

When we try to select the selected element, what is the behavior?

Select the focused element and go to the confirm page.

Both scenarios conclude the same instinct.

Krishna2323 · 2024-01-30T15:57:33Z

@mollfpr, the only change here will be to select and go to confirm page with the currently focused option if we haven't selected any participant and we press cmd+enter, currently we go to confirm page without selecting any member. Everything else will work the same way.

Krishna2323 · 2024-01-30T16:01:11Z

@mollfpr, sorry I thought you were asking.

Krishna2323 · 2024-01-30T16:03:38Z

@ZhenjaHorbach, I already mentioned that in my alternative solution.

mollfpr · 2024-02-01T17:15:18Z

@Krishna2323 I try your main solution but it's not working well when multiple member selected and press CMD+Enter.

Screen.Recording.2024-02-01.at.23.52.19.mp4

@ZhenjaHorbach Have you encountered a similar issue on the other options list? I might be considering your first proposal. It's similar to the member invite page where it does nothing on pressing CMD+Enter when there's no member selected.

ZhenjaHorbach · 2024-02-01T17:25:40Z

@Krishna2323 I try your main solution but it's not working well when multiple member selected and press CMD+Enter.

Screen.Recording.2024-02-01.at.23.52.19.mp4
@ZhenjaHorbach Have you encountered a similar issue on the other options list? I might be considering your first proposal. It's similar to the member invite page where it does nothing on pressing CMD+Enter when there's no member selected.

Actually yes )
member invite page has the same behavior )
that CMD+Enter is not active when there's no members

Plus I found an additional place where this should be fixed)
FAB - send money - amount page - list of members

Krishna2323 · 2024-02-01T17:29:30Z

@mollfpr, I think blocking isn't a great idea and in the result section in my proposal, you can see that it works fine with multiple participants. We should edit the behaviour of blocking if some component already uses it.

mollfpr · 2024-02-01T18:28:00Z

@Krishna2323 Yeah, in your video, it works fine without any focused option.

I think blocking isn't a great idea

Also, your proposal seems to not resolve the main issue. I still navigate to the confirm page without selecting any members.

Screen.Recording.2024-02-02.at.01.25.33.mp4

It's not entirely blocking, but there's nothing to process. Also, the expected result seems toward it.

@muttmuure May I get your opinion on the expected behavior? Is pressing CMD+Enter on no member selected and there's an option focused should process the selection or not?

Krishna2323 · 2024-02-01T18:32:14Z

@mollfpr, yeah, thanks for the feedback, will update proposal for that fix once we confirm the expected result.

muttmuure · 2024-02-28T17:16:29Z

@Krishna2323 please raise your PR when ready

Krishna2323 · 2024-03-05T09:46:32Z

@mollfpr, PR ready for review.

Krishna2323 · 2024-03-12T13:28:27Z

@mollfpr friendly bump for review.

melvin-bot · 2024-03-27T04:44:48Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2024-03-27T04:44:51Z

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

fix: IOU - CMD+ENTER command takes you to the IOU confirmation page without selecting members #37460

If no regressions arise, payment will be issued on 2024-04-03. 🎊

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

@mollfpr requires payment automatic offer (Reviewer)
@Krishna2323 requires payment automatic offer (Contributor)

melvin-bot · 2024-03-27T04:44:52Z

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.
[@muttmuure] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

Krishna2323 · 2024-04-04T06:44:17Z

@muttmuure, friendly bump for payments :)

mollfpr · 2024-04-05T20:01:47Z

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

I couldn't find the offending PR.

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

The regression step should be enough.

[@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.

Open the App
Tap on the green plus button (FAB)
Select Request Money/Manual
Select any currency and amount
Press the CMD + ENTER key combination without focusing on any option, verify it doesn't move to confirmation page
Go back, focus on any option, and press the CMD + ENTER, verify it moves to the confirmation page with a focused option
Go back, add any number of options to split, and press CMD + ENTER, verify it takes you to the confirmation page with selected options.
👍 or 👎

@muttmuure Could you create the payment summary? I'll do manual request in NewDot

Krishna2323 · 2024-04-09T09:04:13Z

@muttmuure, bump for payments 🙇🏻

mollfpr · 2024-04-10T17:18:17Z

Friendly bump @muttmuure

muttmuure · 2024-04-16T10:04:56Z

Looking!

muttmuure · 2024-04-25T08:16:53Z

$500 for @mollfpr for C+

$500 for @Krishna2323 C (paid in Upwork

muttmuure · 2024-04-25T08:18:33Z

QA added, payment summary above. Sorry for the delay.

JmillsExpensify · 2024-04-29T17:37:17Z

$500 approved for @mollfpr

kavimuru added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 30, 2024

melvin-bot bot assigned muttmuure Jan 30, 2024

melvin-bot bot changed the title ~~IOU - CMD+ENTER command takes you to the IOU confirmation page without selecting members~~ [$500] IOU - CMD+ENTER command takes you to the IOU confirmation page without selecting members Jan 30, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 30, 2024

melvin-bot bot assigned mollfpr Jan 30, 2024

This comment was marked as off-topic.

Sign in to view

melvin-bot bot added the Overdue label Feb 1, 2024

melvin-bot bot removed the Overdue label Feb 1, 2024

melvin-bot bot added the Overdue label Feb 28, 2024

melvin-bot bot removed the Overdue label Feb 28, 2024

Krishna2323 mentioned this issue Feb 29, 2024

fix: IOU - CMD+ENTER command takes you to the IOU confirmation page without selecting members #37460

Merged

50 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 Weekly KSv2 labels Feb 29, 2024

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 27, 2024

melvin-bot bot changed the title ~~[$500] IOU - CMD+ENTER command takes you to the IOU confirmation page without selecting members~~ [HOLD for payment 2024-04-03] [$500] IOU - CMD+ENTER command takes you to the IOU confirmation page without selecting members Mar 27, 2024

melvin-bot bot removed the Reviewing Has a PR in review label Mar 27, 2024

melvin-bot bot added the Overdue label Apr 24, 2024

melvin-bot bot removed the Overdue label Apr 25, 2024

muttmuure closed this as completed Apr 25, 2024

[HOLD for payment 2024-04-03] [$500] IOU - CMD+ENTER command takes you to the IOU confirmation page without selecting members #35377

[HOLD for payment 2024-04-03] [$500] IOU - CMD+ENTER command takes you to the IOU confirmation page without selecting members #35377

Comments

kavimuru commented Jan 30, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Jan 30, 2024

melvin-bot bot commented Jan 30, 2024

melvin-bot bot commented Jan 30, 2024

ZhenjaHorbach commented Jan 30, 2024 • 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)

This comment was marked as off-topic.

Krishna2323 commented Jan 30, 2024 • 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?

Steps:

Result

Alternative

Krishna2323 commented Jan 30, 2024

ZhenjaHorbach commented Jan 30, 2024

Krishna2323 commented Jan 30, 2024

mollfpr commented Jan 30, 2024

Krishna2323 commented Jan 30, 2024

ZhenjaHorbach commented Jan 30, 2024 • edited Loading

mollfpr commented Jan 30, 2024

Krishna2323 commented Jan 30, 2024

Krishna2323 commented Jan 30, 2024

Krishna2323 commented Jan 30, 2024

mollfpr commented Feb 1, 2024

ZhenjaHorbach commented Feb 1, 2024

Krishna2323 commented Feb 1, 2024 • edited Loading

mollfpr commented Feb 1, 2024 • edited Loading

Krishna2323 commented Feb 1, 2024

muttmuure commented Feb 28, 2024

Krishna2323 commented Mar 5, 2024

Krishna2323 commented Mar 12, 2024

melvin-bot bot commented Mar 27, 2024

melvin-bot bot commented Mar 27, 2024

melvin-bot bot commented Mar 27, 2024

Krishna2323 commented Apr 4, 2024

mollfpr commented Apr 5, 2024

Krishna2323 commented Apr 9, 2024

mollfpr commented Apr 10, 2024

muttmuure commented Apr 16, 2024

muttmuure commented Apr 25, 2024

muttmuure commented Apr 25, 2024

JmillsExpensify commented Apr 29, 2024

kavimuru commented Jan 30, 2024 •

edited by melvin-bot bot

Loading

ZhenjaHorbach commented Jan 30, 2024 •

edited

Loading

Krishna2323 commented Jan 30, 2024 •

edited

Loading

ZhenjaHorbach commented Jan 30, 2024 •

edited

Loading

Krishna2323 commented Feb 1, 2024 •

edited

Loading

mollfpr commented Feb 1, 2024 •

edited

Loading