Group chat - User can proceed without Start group button via CMD+Enter shortcut

[lanitochka17]

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.58-4 Reproducible in staging?: Y Reproducible in production?: N **If this was caught during regression testing, add the test name, ID and link from TestRail:**N/A Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to FAB > Start chat
3. Select two users
4. Click Next
5. Unselect all the users
6. Press CMD+Enter

Expected Result:

User will not be able to proceed with CMD+Enter

Actual Result:

User is able to proceed with CMD+Enter when there is no Start group button

Workaround:

Unknown

Platforms:

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

• [ ] Android: Native
• [ ] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/dc766479-4144-44b4-abf2-db0db2575f49

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @blimpich (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[github-actions[bot]]

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[lanitochka17]

@blimpich FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[ShridharGoel]

Proposal

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

User can proceed without Start group button via CMD+Enter shortcut

What is the root cause of that problem?

There's no check for minimum number of participants in createGroup method. We are removing the confirm button but shortcut can still be used to continue.

https://github.com/Expensify/App/blob/14ff9445d22bd92d0645abd456ac805cedc06c28/src/pages/NewChatConfirmPage.tsx#L99-L105

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

Add the below check to ensure there are at least 2 users (though it's not technically a group with 2 users, it's a DM which is going to be handled by this proposal).

const createGroup = () => {
        if (!newGroupDraft || newGroupDraft.participants.length <= 1) {
            return;
        }
        const logins: string[] = newGroupDraft.participants.map((participant) => participant.login);
        Report.navigateToAndOpenReport(logins, true, groupName);
    };

The above code is tested.

[abzokhattab]

Proposal

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

user can create a group chat when with no users when clicking on CMD+ENTER

What is the root cause of that problem?

The issue occurs because, within the SelectionList, the CMD+ENTER keyboard shortcut remains active even when the Confirm button is not displayed.

https://github.com/Expensify/App/blob/3cf22d1b41ce44109e81f4d5baa841b6a53cf436/src/components/SelectionList/BaseSelectionList.tsx#L458-L473

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

To resolve this inconsistency, we should align keyboard actions with the UI's state by deactivating the keyboard shortcut when the Confirm button is hidden.

• This can be achieved by adding && showConfirmButton to the condition above, ensuring that the confirmation action is triggered only when the Confirm button is visible:

            if (onConfirm && showConfirmButton) {

or

    if (onConfirm) {
        if (!showConfirmButton) {
            return;
        }
        onConfirm(e, focusedOption);
        return;
    }

alternatively:

• Alternatively, we can change the isActive condition to ensure it is only true when both onConfirm and showConfirmButton are either true or false simultaneously:

            isActive: !disableKeyboardShortcuts && isFocused && !!showConfirmButton === !!onConfirm,

[blimpich]

this probably due to the new group chats PR https://github.com/Expensify/App/pull/37458. Not big enough of an issue to revert, so lets just open this to contributors.

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~013017903b692fbc8f

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Upwork job price has been updated to $250

[allgandalf]

Proposal

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

Use can create a group even when only 1 participant exists

What is the root cause of that problem?

For the shortcut CMD + Enter we have code in BaseSelectionList to allow us to submit the chat without checking for length: https://github.com/Expensify/App/blob/99cc0b6f34c477dee40934b0ffb294e5dde1f58a/src/components/SelectionList/BaseSelectionList.tsx#L459-L464

But here we don't have a check which checks if showConfirmButton is enabled or not, we already pass this prop to the component and the showConfirmButton is used to display/hide the create group button

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

Add a check inside useKeyboardShortcut to return if showConfirmButton is false as the case for that to be false is the the participant list length is greater than 1: https://github.com/Expensify/App/blob/14ff9445d22bd92d0645abd456ac805cedc06c28/src/pages/NewChatConfirmPage.tsx#L130-L135

So in useKeyboardShortcut we can add a check to return if this is set to false, so the updated code would be:

/** Calls confirm action when pressing CTRL (CMD) + Enter */
    useKeyboardShortcut(
        CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER,
        (e) => {
            const focusedOption = flattenedSections.allOptions[focusedIndex];
            if(!showConfirmButton){
                return;
            }

This way we make sure that we make use of the existing prop and not cause any additional regression as showConfirmButton is already used to display the create group button.

What alternative solutions did you explore? (Optional)

N/A

[nkdengineer]

Proposal

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

• Group chat - User can proceed without Start group button via CMD+Enter shortcut

What is the root cause of that problem?

• Below is the condition we used to check whether we should display the "Start group" button or not: https://github.com/Expensify/App/blob/a4d01a87b242eef06af37db965bd135745e342e5/src/pages/NewChatConfirmPage.tsx#L135
• But in the case of this bug, selectedOptions.length: 0, we do not have a logic to disable the shortcut button CMD + ENTER: https://github.com/Expensify/App/blob/a4d01a87b242eef06af37db965bd135745e342e5/src/components/SelectionList/BaseSelectionList.tsx#L464

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

• Generally, we need to make sure we only allow to call onConfirm if there is a button in SelectionList.
• If we fix the issue by updating this to:

        if (!newGroupDraft || newGroupDraft.participants.length <= 1) {

it will violate the above rule "make sure we only allow to call onConfirm if there is a button in SelectionList"

• If we fix the issue by updating this to:

            if (onConfirm && showConfirmButton) {

it can lead to regression because with other page that uses footerContent instead of showConfirmButton, CMD+ENTER does not call onConfirm function. For example, this page

• So the solution I suggest is to update this to:

                onConfirm={selectedOptions.length > 1 ? createGroup :undefined}

What alternative solutions did you explore? (Optional)

• NA

[jjcoffee]

Thanks for the proposals everyone! I agree with the analysis in @nkdengineer's proposal and their solution seems good. Taking into account separation of concern, it doesn't really makes sense for BaseSelectionList to decide whether or not to call the onConfirm function. It should always call it and leave it up to the function to decide behaviour.

:ribbon::eyes::ribbon: C+ reviewed

[melvin-bot[bot]]

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

[abzokhattab]

because with other page that uses footerContent instead of showConfirmButton, CMD+ENTER does not call onConfirm function. For example, this page

Correct i agree, i didn't notice this case. sometimes we pass the onConfirm without passing the shouldShowConfirm prop

[bondydaa]

guessing i got assigned here b/c Ben is now OOO, going to remove him.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

📣 @jjcoffee 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @nkdengineer You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

[nkdengineer]

@jjcoffee PR https://github.com/Expensify/App/pull/39553 is ready to review

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

• https://github.com/Expensify/App/pull/39553

If no regressions arise, payment will be issued on 2024-04-25. :confetti_ball:

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

• @jjcoffee requires payment automatic offer (Reviewer)
• @nkdengineer requires payment (Needs manual offer from BZ)

[melvin-bot[bot]]

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:

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

[anmurali]

Not due for another couple of days but @nkdengineer - you can accept the offer here.

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/37458/
• 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: https://github.com/Expensify/App/pull/37458/files#r1581099685
• 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: N/A
• 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 Proposal

1. Go to FAB > Start chat
2. Select any two users
3. Click Next
4. Unselect all the users
5. Press CMD+Enter and verify that you cannot proceed

Do we agree 👍 or 👎

[melvin-bot[bot]]

@bondydaa, @anmurali, @jjcoffee, @nkdengineer Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[anmurali]

Paid, added regression test.