Closed lanitochka17 closed 3 days ago
Triggered auto assignment to @blimpich (DeployBlockerCash
), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.
: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:
@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
We think that this bug might be related to #vip-vsp
User can proceed without Start group button via CMD+Enter shortcut
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.
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.
user can create a group chat when with no users when clicking on CMD+ENTER
The issue occurs because, within the SelectionList
, the CMD+ENTER
keyboard shortcut remains active even when the Confirm
button is not displayed.
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.
&& 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;
}
isActive
condition to ensure it is only true when both onConfirm
and showConfirmButton
are either true or false simultaneously: isActive: !disableKeyboardShortcuts && isFocused && !!showConfirmButton === !!onConfirm,
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.
Job added to Upwork: https://www.upwork.com/jobs/~013017903b692fbc8f
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee (External
)
Upwork job price has been updated to $250
Use can create a group even when only 1 participant exists
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
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.
N/A
Group chat - User can proceed without Start group button via CMD+Enter shortcut
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
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}
NA
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
Triggered auto assignment to @bondydaa, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
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
guessing i got assigned here b/c Ben is now OOO, going to remove him.
Triggered auto assignment to @anmurali (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
π£ @jjcoffee π An offer has been automatically sent to your Upwork account for the Reviewer role π Thanks for contributing to the Expensify app!
π£ @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 π
@jjcoffee PR https://github.com/Expensify/App/pull/39553 is ready to review
Reviewing
label has been removed, please complete the "BugZero Checklist".
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:
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:
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:
Not due for another couple of days but @nkdengineer - you can accept the offer here.
Regression Test Proposal
Do we agree π or π
@bondydaa, @anmurali, @jjcoffee, @nkdengineer Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Paid, added regression test.
If you havenβt already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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:
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?
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
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @anmurali