Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mWeb - Group chat - Search is not blocked when user reached max participants #7679

Closed
kbecciv opened this issue Feb 10, 2022 · 14 comments
Closed
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Feb 10, 2022

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. Go to https://staging.new.expensify.com and sign in
  2. Tap the green plus button
  3. Select New group option
  4. Start the group creation flow and add 8 participants
  5. Verify you're showed a message that you reached the max participants
  6. Enter a new user in the search field and tap Return button on the keyboard

Expected Result:

The search is blocked. The user is unable to add another user to the group.

Actual Result:

The user is able to search for a new user. The search is not blocked after reaching max of participants for group chat. The first added user in the participants list disappears from the list of participants and new searched user can be added to the group.

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Mobile Web

Version Number: 1.1.38.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

Bug5447628_0-02-05-defd673bf7b5b056556a0bf71b7e59cc101b76c4ce36a7dba386607e31b403df_6c2337c7d65e23f3.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause

Slack conversation:

View all open jobs on GitHub

@MelvinBot
Copy link

Triggered auto assignment to @AndrewGable (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@AndrewGable AndrewGable added Weekly KSv2 External Added to denote the issue can be worked on by a contributor and removed Daily KSv2 labels Feb 11, 2022
@MelvinBot
Copy link

Triggered auto assignment to @bfitzexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Feb 11, 2022
@sobitneupane
Copy link
Contributor

sobitneupane commented Feb 11, 2022

This issue exists in all platforms.
Proposal
Pass maxParticipantsReached from NewChatPage.js to OptionSelectors as a prop.

In OptionSelectors, Use maxParticipantsReached prop to deactivate the text input.

<TextInput
ref={el => this.textInput = el}
value={this.props.value}
onChangeText={this.props.onChangeText}
onKeyPress={this.handleKeyPress}
placeholder={this.props.placeholderText
|| this.props.translate('optionsSelector.nameEmailOrPhoneNumber')}
/>

<TextInput
    ref={el => this.textInput = el}
    value={this.props.value}
    onChangeText={this.props.onChangeText}
    onKeyPress={this.handleKeyPress}
    placeholder={this.props.placeholderText
        || this.props.translate('optionsSelector.nameEmailOrPhoneNumber')}
+   editable = {!this.props.maxParticipantsReached}
/>

We should also add following lines of codes in handleKeyPress().

handleKeyPress(e) {
// We are mapping over all the options to combine them into a single array and also saving the section index
// index within that section so we can navigate
const allOptions = _.reduce(this.props.sections, (options, section, sectionIndex) => (
[...options, ..._.map(section.data, (option, index) => ({
...option,

        if (this.props.maxParticipantsReached) {
            return;
        }

@bfitzexpensify
Copy link
Contributor

Upwork job here

@MelvinBot
Copy link

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

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 11, 2022
@MelvinBot
Copy link

Current assignee @AndrewGable is eligible for the Exported assigner, not assigning anyone new.

@parasharrajat
Copy link
Member

@sobitneupane
No disabling the search does not sound like a good option. And if we disable the handleKeyPress Then the user won't be able to deselect the user and navigate via arrow keys.

@thesahindia
Copy link
Member

thesahindia commented Feb 11, 2022

Here's what I found. When we enable the user to navigate using arrow keys (we haven't on new group right now) The first user is always highlighted so let's say someone reached the max participants limit and searches for a new user, now if the user pressed the enter/return key the first user that is highlighted will get removed(as on enter we select/unselect the user).
Here's a vid

Screen.Recording.2022-02-11.at.11.59.36.PM.mov

cc: @parasharrajat

@Yoel-amri
Copy link

Yoel-amri commented Feb 11, 2022

Hello,
I think I solved this,
You basically add maxParticipantsReached prop to OptionsSelector and then check on it in the case of 'Enter' in handleKeyPress

<OptionsSelector
maxParticipantsReached={maxParticipantsReached}
canSelectMultipleOptions={this.props.isGroupChat}
sections={sections} .....

and inside OptionsSelector

switch (e.nativeEvent.key) {
case 'Enter': {
if (this.props.maxParticipantsReached)
break;
this.selectRow(allOptions[this.state.focusedIndex]);
e.preventDefault();
break;
}

thats is

@mdneyazahmad
Copy link
Contributor

Proposal

Do not search when max participant is reached. In this case user can enter text but it will not search.

<OptionsSelector
canSelectMultipleOptions={this.props.isGroupChat}
sections={sections}
selectedOptions={this.state.selectedOptions}
value={this.state.searchValue}
onSelectRow={this.toggleGroupOptionOrCreateChat}
onChangeText={(searchValue = '') => {
const {
recentReports,
personalDetails,
userToInvite,
} = OptionsListUtils.getNewChatOptions(

                                        onChangeText={(searchValue = '') => {

+                                           if(maxParticipantsReached) {
+                                               return;
+                                          }

                                            const {
                                                recentReports,
                                                personalDetails,
                                                userToInvite,
                                            } = OptionsListUtils.getNewChatOptions(

Result

Screen.Recording.2022-02-13.at.9.19.24.AM.mov

@parasharrajat
Copy link
Member

It does not seem like a valid issue. For more, you can look at Sahil's comment.

I would discuss this on slack to understand the required change before accepting proposals. I will do this on Monday. Thanks, everyone for the proposal. There are a couple of PRs affecting this behavior indirectly. So, I would prefer to wait before we handle this issue.

@parasharrajat
Copy link
Member

No update...I haven't discussed this. I don't understand the issue here. It seems to work fine from the video. And I don't know what should be discussed for this on slack. cc: @bfitzexpensify

When we deselect a user from the list and there is some search text in the input. you should see the search results. Right?

@MelvinBot MelvinBot removed the Overdue label Feb 21, 2022
@parasharrajat
Copy link
Member

Question ⬆️

@bfitzexpensify
Copy link
Contributor

Yes, I also can't quite see the issue here. Tested on iOS and desktop, and the behaviour looks correct to me:

  • you can search and add participants until you hit the max number of participants
  • when you hit the limit, search doesn't return any results
  • if you remove one of the participants, search and adding works again

That runs counter to the original post — the behaviour right now is as expected. So, I think we are fine to close this one out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants