App icon indicating copy to clipboard operation
App copied to clipboard

[$500] Room - Inconsistency in behaviour while adding VOIP number as contact via start chat&room

Open kbecciv opened this issue 1 year ago β€’ 33 comments

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.21-1 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:

  1. Go to https://staging.new.expensify.com/
  2. Tap on a room chat
  3. Tap on header---members----Invite
  4. Paste the VOIP number (+18183305299)
  5. Tap invite
  6. Note user can add VOIP number as contact
  7. Navigate to LHN
  8. Tap fab---Start chat
  9. Paste the VOIP number (+18183305299)
  10. Select the VOIP number (+18183305299) from the list
  11. Note error message is displayed

Expected Result:

User must be not be allowed to add a landline or VOIP contact to a room or other places in the app. The error message that should be returned is the same as if you try to invite them to a chat via steps 7-11 above. "The provided phone number belongs to a landline or VoIP, please use your email address instead."

Actual Result:

In room, Voip number allowed to add as contact but in start chat adding voip number showing error.Inconsistency in behaviour displayed.

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/dc656da1-24d7-4059-8131-3d6f460edbaa

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01907badd5c1783209
  • Upwork Job ID: 1742844313960140800
  • Last Price Increase: 2024-01-18

kbecciv avatar Jan 04 '24 09:01 kbecciv

Job added to Upwork: https://www.upwork.com/jobs/~01907badd5c1783209

melvin-bot[bot] avatar Jan 04 '24 09:01 melvin-bot[bot]

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

melvin-bot[bot] avatar Jan 04 '24 09:01 melvin-bot[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[bot] avatar Jan 04 '24 09:01 melvin-bot[bot]

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

melvin-bot[bot] avatar Jan 04 '24 09:01 melvin-bot[bot]

Proposal

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

Discrepancy in behavior when adding a VoIP user in a workspace and starting a chat.

What is the root cause of that problem?

1 - Allow creating server request for invalid phone numbers (landline or VoIP), 2 - Inconsistency server response when dealing with invalid phone numbers (landline or VoIP).

Where users can invite/add others members/users: 1 - Invite member/user to a workspace 2 - Search a user and 'start chat' 3 - Invite member/user to a regular room 4 - FAB > Start chat 5 - FAB > Request money 6 - FAB > Send Money 7 - FAB > Assign Task

Backend response for then: 1 - "onyxData": [], - "message": "The provided phone number belongs to a landline or VoIP..." 2 - "onyxData": [ { ... createChat": { "1705430486982131": "The provided phone number belongs to a landline or VoIP...

3 - "onyxData": [ { "key": "report_3251340957676970", "onyxMethod": "merge", "value": { "participantAccountIDs": [ 16258318, 16177509, 16256252, 16256248, 16258226, 16205380 ], "participants": [ "[email protected]", "+18183305299", "+18183305295", "+18183305296", "+18173305299", "[email protected]" ], "visibleChatMemberAccountIDs": [ 16258318, 16177509, 16256252, 16256248, 16258226, 16205380 ] } },

4 -"onyxData": [ { ... createChat": { "1705430486982131": "The provided phone number belongs to a landline or VoIP... 5 - "onyxData": [], - "message": "The provided phone number belongs to a landline or VoIP..." 6 - "onyxData": [], - "message": "The provided phone number belongs to a landline or VoIP..."

7 - "onyxData": [ { "key": "report_8859913735881996", "onyxMethod": "merge", "value": { "description": "", "lastVisibleActionCreated": "2024-01-16 19:27:01.357", "managerID": 16258666, "ownerAccountID": 16205380, "parentReportID": "1566167401886507", "reportID": "8859913735881996", "reportName": "tes2", "state": "OPEN", "stateNum": 0, "statusNum": 0, "type": "task" } }, { "key": "personalDetailsList", "onyxMethod": "merge", "value": { "16205380": { "accountID": 16205380, "avatar": "https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/default-avatar_22.png", "displayName": "[email protected]", "firstName": "", ...

Online - UI behavior: 1 - Almost good - a "There was a problem adding ..." instead of "The provided phone number belongs to a landline or VoIP...". 2 - Looks good - Open a disabled report chat with a "The provided phone number belongs to a landline or VoIP..." message. 3 - Big Issue - We can add the member successfully. 4 - Looks good - Open a disabled report chat with a "The provided phone number belongs to a landline or VoIP..." message. 5 - Issue - Open a report full with only skeleton loading. 6 - Issue - Open a report full with only skeleton loading. 7 - Big Issue - We create a task and assign the member successfully.

Off - UI behavior: 1 - Looks good - Able to invite, disable look. 2 - Looks good - Able to start a chat and a conversation, disabled look. 3 - Issue - Able to invite, 'pressed' look after invite instead of disabled. https://github.com/Expensify/App/issues/34671 image 4 - Looks good - Able to start a chat and a conversation, disabled look. 5 - Looks good - Able to create a iou report and a conversation, disabled look. 6 - Looks good - Able to create a iou report and a conversation, disabled look. 7 - Looks good - Able to create a task report and a conversation, disabled look.

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

~Create a landline or VoIP form validation.~ https://github.com/Expensify/App/issues/24519#issuecomment-1687078956

Since we need the backend to verify a number, we need Standardize backend responses when the check for VOIP and landline is true.

What alternative solutions did you explore? (Optional)

brunovjk avatar Jan 04 '24 19:01 brunovjk

Updated the Expected behaviour in the OP

Expected Result:

User must be not be allowed to add a landline or VOIP contact to a room or other places in the app. The error message that should be returned is the same as if you try to invite them to a chat via steps 7-11 above. "The provided phone number belongs to a landline or VoIP, please use your email address instead."

@brunovjk @0xmiroslav and others, let's make sure we're testing any other ways/places a user can be added to ensure we surface the "The provided phone number belongs to a landline or VoIP, please use your email address instead." error everywhere. I want to fix all instances at once and not manage multiple GH issues.

mallenexpensify avatar Jan 09 '24 00:01 mallenexpensify

I got this error while trying to add the number to a room. Is this still reproducible?

expensify-error-adding-voip-number-to-room

Victor-Nyagudi avatar Jan 10 '24 11:01 Victor-Nyagudi

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

melvin-bot[bot] avatar Jan 11 '24 16:01 melvin-bot[bot]

@mallenexpensify, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Jan 12 '24 17:01 melvin-bot[bot]

d'oh, tagged wrong person and not you @0xmiroslav , can you please review my comment above and the proposals? Thx

mallenexpensify avatar Jan 15 '24 22:01 mallenexpensify

Proposal

Updated

brunovjk avatar Jan 17 '24 17:01 brunovjk

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

melvin-bot[bot] avatar Jan 18 '24 16:01 melvin-bot[bot]

@mallenexpensify @0xmiroslav this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar Jan 18 '24 17:01 melvin-bot[bot]

@brunovjk Thanks for the proposal. I see you directly linked to https://github.com/Expensify/App/issues/24519#issuecomment-1687078956. Can you add some context behind it?

0xmiroslav avatar Jan 18 '24 17:01 0xmiroslav

Can you add some context behind it?

@0xmiroslav although 'I think on top of the proposal, they should implement some sort of check in the front-end for whether the provided phone number is valid before creating an' report https://github.com/Expensify/App/issues/24519#issuecomment-1686742822 I understood that 'these validations are meant to happen on the backend since we can't easily verify if a phone number is VOIP (from its format, we can check but it's not dependable)' https://github.com/Expensify/App/issues/24519#issuecomment-1687078956

This is what I found on Github+Slack, maybe there is a better discussion on the subject, I don't know, but I understand the difficulty of trusting these validators on the frontend, and I wonder about the feasibility of standardizing responses when validation for an invalid number is true.

brunovjk avatar Jan 18 '24 19:01 brunovjk

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

melvin-bot[bot] avatar Jan 24 '24 03:01 melvin-bot[bot]

@cubuspl42 reassigning, please take over as C+. If you don't have bandwidth, unassign yourself. Thanks

mallenexpensify avatar Jan 24 '24 03:01 mallenexpensify

We have one proposal so far

jakub-trzebiatowski avatar Jan 24 '24 10:01 jakub-trzebiatowski

@brunovjk Do I understand correctly that you're suggesting no frontend changes, but to handle this on the backend instead?

jakub-trzebiatowski avatar Jan 24 '24 10:01 jakub-trzebiatowski

@cubuspl42 Yes :) Does that make sense? I'm new here, still trying to understand how it all works.

brunovjk avatar Jan 24 '24 11:01 brunovjk

It makes sense. Unfortunately, in such a case, the issue might have to be fixed internally without external compensation.

πŸŽ€ πŸ‘€ πŸŽ€

jakub-trzebiatowski avatar Jan 24 '24 11:01 jakub-trzebiatowski

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

melvin-bot[bot] avatar Jan 24 '24 11:01 melvin-bot[bot]

@danieldoglas Would you take a look?

jakub-trzebiatowski avatar Jan 24 '24 11:01 jakub-trzebiatowski

Very good, thank you @cubuspl42 :D

brunovjk avatar Jan 24 '24 11:01 brunovjk

I agree with you. @mallenexpensify can you please turn this into an internal issue? I think this could probably be a part of vip-split.

danieldoglas avatar Jan 24 '24 18:01 danieldoglas

Current assignee @cubuspl42 is eligible for the Internal assigner, not assigning anyone new.

melvin-bot[bot] avatar Jan 27 '24 17:01 melvin-bot[bot]

Moved to Internal, @danieldoglas what's your reasoning for #vip-split? I want to post in that room and could use some 'ammo'/words to sell it. Thx

mallenexpensify avatar Jan 27 '24 17:01 mallenexpensify

@danieldoglas can you comment on the above when you have a min?

what's your reasoning for #vip-split? I want to post in that room and could use some 'ammo'/words to sell it. Thx

mallenexpensify avatar Jan 31 '24 01:01 mallenexpensify

Oh my bad, didn't see that comment before.

#vip-split is where I'm seeing the focus on 1:1 and group chats being created, as well as universal chat. So I think that's a good place where we should focus on limiting voip numbers.

danieldoglas avatar Jan 31 '24 19:01 danieldoglas

@cubuspl42 @mallenexpensify this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!

melvin-bot[bot] avatar Feb 01 '24 15:02 melvin-bot[bot]