[$500] Room - Can invite valid phone account multiple times into the room

[kbecciv]

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.3.89.6
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: @namhihi237
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1698051749271339

Action Performed:

1. Create a WS
2. Create a room with workspace
3. Click room header => Members => Invite
4. Input an invalid phone (eg: +3233232432) then select and click invite
5. Repeat step 4

Expected Result:

The app should show an error when inviting an invalid phone number

Actual Result:

The system does not show errors and may invite invalid phone numbers multiple times

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Android: Native

Screen.Recording.2023-10-23.at.19.42.26.mov

Android: mWeb Chrome

Screen.Recording.2023-10-23.at.19.42.26.mov

iOS: Native

Screen.Recording.2023-10-23.at.19.42.26.1.mov

iOS: mWeb Safari

Screen.Recording.2023-10-23.at.19.43.59.mov

MacOS: Chrome / Safari

Screen.Recording.2023-10-23.at.16.06.33.mov

Recording.5119.mp4

MacOS: Desktop

Screen.Recording.2023-10-23.at.19.36.43.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01da7f12cf9ed3d23f
• Upwork Job ID: 1716509714359189504
• Last Price Increase: 2023-10-30
• Automatic offers:
• DylanDylann | Contributor | 27539247
• namhihi237 | Reporter | 27539248

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[alitoshmatov]

I think number provided number(+3233232432) is valid number

[namhihi237]

@alitoshmatov you can try with a new chat. the backend return invalid

[namhihi237]

I think issue should fixed in backend.

[FitseTLT]

Proposal

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

Room allows multiple invitation for phone number which is already a member.

What is the root cause of that problem?

The problem is not invalid phone numbers can be invited multiple times (as commented in the title of the issue). It correctly validates when you input wrong number but for valid phone numbers it let's you invite it multiple times without validating and informing the user that the phone number is already a member of the room as it correctly does for emails.
This is because of a bug which lies in this line of code

App/src/libs/OptionsListUtils.js

Lines 1237 to 1238 in 844b3b9

	!_.find(optionsToExclude, (optionToExclude) => optionToExclude.login === addSMSDomainIfPhoneNumber(searchValue).toLowerCase()) &&
	(searchValue !== CONST.EMAIL.CHRONOS || Permissions.canUseChronos(betas)) &&

Because it is comparing existing login list of members of the room (optionsToExclude) with the phone number input (searchInput) of the user passed to the addSMSDomainIfPhoneNumber function which concatinates '@expensify.sms' to the searchInput. But as I observed the login value for the phone number only holds the phone number without '@expensify.sms' concatination. Hence, existing phone numbers will never be filtered out as the comparison will always be false.

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

Change

App/src/libs/OptionsListUtils.js

Lines 1237 to 1238 in 844b3b9

	!_.find(optionsToExclude, (optionToExclude) => optionToExclude.login === addSMSDomainIfPhoneNumber(searchValue).toLowerCase()) &&
	(searchValue !== CONST.EMAIL.CHRONOS || Permissions.canUseChronos(betas)) &&

to

        !_.find(optionsToExclude, (optionToExclude) => optionToExclude.login === searchValue.toLowerCase()) &&

Result:

10.New.Expensify.mp4

What alternative solutions did you explore? (Optional)

May be if this code is also necessary to support for previous login info (if there was time where we used to save phone number concatinated with '@expensify.sms' ) we could add an or condition to support for both situations like

!_.find(optionsToExclude, (optionToExclude) => optionToExclude.login === searchValue.toLowerCase() || optionToExclude.login === addSMSDomainIfPhoneNumber(searchValue).toLowerCase()) &&

[FitseTLT]

Updated comment And I think this bug is worthy of fixing.

[DylanDylann]

Proposal

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

• Room - Can invite invalid phone account multiple times into the room

What is the root cause of that problem?

• From my side, the +3233232432 is a valid phone number, so we need to update the issue `s title.
• When we select the phone number A (for example A is +3233232432), we send it to BE without adding sms domain (@expensify.sms). Then BE will merge phone number A to report.participants without sms domain as well. Now the report.participants is [..., A].
• Then we search A again, because the excludeUser is get from report.participants, so now the excludeUser is [..., A] as well.
• In getOptions function, the below logic:
  

  App/src/libs/OptionsListUtils.js

  Line 1317 in a5e1e3f

  !_.find(optionsToExclude, (optionToExclude) => optionToExclude.login === addSMSDomainIfPhoneNumber(searchValue).toLowerCase()) &&

  
  will return true because _find([..., {login: A}], (optionToExclude) => optionsToExculde.login === [email protected] return false. That leads to the new user being created and appearing in the search result

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

• We can add the @expensify.sms with phone number when call API "InviteToRoom" as we did when call API "AddMembersToWorkspace" by updating:
  

  App/src/libs/actions/Report.js

  Line 2031 in b335f2f

  const inviteeEmails = _.keys(inviteeEmailsToAccountIDs);

  
  to:

const inviteeEmails = _.keys(inviteeEmailsToAccountIDs).map((login) => addSMSDomainIfPhoneNumber(login));

Additionally, I found out there is a related bug that needs to be fixed (encounters in WorkspaceInvitePage as well):

• If the member list already contains A, then we type A in the search input, it will display as No results found rather than [email protected] is already a member of Workspace.
• RCA:
  

  App/src/pages/RoomInvitePage.js

  Lines 191 to 193 in dae3c5a

  	if (!userToInvite && excludedUsers.includes(searchValue)) {
  	return translate('messages.userIsAlreadyMember', {login: searchValue, name: reportName});
  	}

  
  Based on the above logic, we are displaying messages.userIsAlreadyMember only when usersToInvite is empty and excludeUsers includes searchValue. In case phone number A already existed, in excludeUsers it is [email protected], in searchValue it is just A, so the conditions usersToInvite.length === 0 && excludedUsers.includes(searchValue) will be false, leads to the bug.
• Solution: We should add the trailing @expensify.sms to phone number A when checking if it existed in excludeUsers or not by:
  update:
  

  App/src/pages/RoomInvitePage.js

  Line 191 in dae3c5a

  if (!userToInvite && excludedUsers.includes(searchValue)) {

  
  to:

if (usersToInvite.length === 0 && (excludedUsers.includes(searchValue) || excludedUsers.includes(OptionsListUtils.addSMSDomainIfPhoneNumber(searchValue).toLowerCase()))) 

What alternative solutions did you explore? (Optional)

• NA

Result

Screencast.from.27-10-2023.02.46.47.webm

[melvin-bot]

@greg-schroeder, @rushatgabhane Whoops! This issue is 2 days overdue. Let's get this updated quick!

[greg-schroeder]

awaiting proposal review from @rushatgabhane

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

@greg-schroeder, @rushatgabhane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[greg-schroeder]

Bump @rushatgabhane

[rushatgabhane]

C+ reviewed 🎀 👀 🎀

I like @DylanDylann's propsoal #30190 (comment)

[melvin-bot]

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

[DylanDylann]

@greg-schroeder I am working on it, I need to double-check because the latest main has a lot of changes

[greg-schroeder]

Okay got it, thanks!

[DylanDylann]

The new PR is ready to review

[greg-schroeder]

New PR was merged, awaiting deploy to staging -> prod

[melvin-bot]

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

[melvin-bot]

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

• Fix: Duplicate phone number can be invited #33928

If no regressions arise, payment will be issued on 2024-02-07. 🎊

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

• @rushatgabhane requires payment through NewDot Manual Requests
• @DylanDylann requires payment automatic offer (Contributor)
• @namhihi237 requires payment automatic offer (Reporter)

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

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

[Beamanator]

Seems like the PR for this was reverted again 😬 #35335 so no payment due yet

[greg-schroeder]

RIP

[greg-schroeder]

Are we even going to move forward here at all? It seems this might not actually go forward with another PR based on what @mountiny is saying in the linked PRs

[DylanDylann]

@rushatgabhane @mountiny I cannot reproduce this issue in staging anymore. Can anyone help confirm?

[Beamanator]

Added retest-weekly to get more eyes from applause, would be great if this got fixed automagically somehow 😅 😳

[greg-schroeder]

I also couldn't repro, I think I'm going to close this given two failures to reproduce and also #30190 (comment). Reopen if you disagree