App icon indicating copy to clipboard operation
App copied to clipboard

[$250] Workspace - WS chat is created for invalid phone number

Open IuliiaHerets opened this issue 1 year ago β€’ 15 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: v9.0.52-5 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): [email protected] **Issue reported by:**Applause Internal Team

Action Performed:

  1. Login to an account
  2. create a WS
  3. Invite a user with invalid Phone number(Invalid Phone: +1 631-791-8378) Notice error is shown and can't invite user.
  4. Go to inbox

Expected Result:

WS chat with invalid Phone number is not created

Actual Result:

WS chat with invalid Phone number is created

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/5a47eb49-3cfb-4446-8a6d-69da70913a7e

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021849978282313703906
  • Upwork Job ID: 1849978282313703906
  • Last Price Increase: 2024-10-26
  • Automatic offers:
    • akinwale | Reviewer | 104685790
Issue OwnerCurrent Issue Owner: @bernhardoj

IuliiaHerets avatar Oct 23 '24 09:10 IuliiaHerets

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] avatar Oct 23 '24 09:10 melvin-bot[bot]

@stephanieelliott 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

IuliiaHerets avatar Oct 23 '24 09:10 IuliiaHerets

Edited by proposal-police: This proposal was edited at 2024-10-23 10:33:54 UTC.

Proposal

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

WS chat is still available for failed to add workspace members.

What is the root cause of that problem?

When we invite a member, we optimistically create a report for the invited member. However, we don't clear the report when fails, we only set the isLoadingInitialReportActions metadata to false. https://github.com/Expensify/App/blob/f251dbcdfdafc492d9a9908cbf40e4630b7b6d9d/src/libs/actions/Policy/Policy.ts#L1007-L1013

Also, I see another issue. The optimistic member is added to the #announce room but never removed from the room if failed to be added. We already have a failure data that should remove the optimistic member when fails. https://github.com/Expensify/App/blob/4f0ca943a51e340652885c3c1fa3dd4434b91e3f/src/libs/actions/Policy/Member.ts#L149-L165

However, participants is an object, so merging the new participants object that contains the optimistic members with the previous participants object will do nothing.

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

We can clear the optimistic report when fails (and it's metadata).

workspaceMembersChats.onyxFailureData.push({
    onyxMethod: Onyx.METHOD.SET,
    key: `${ONYXKEYS.COLLECTION.REPORT_METADATA}${optimisticReport.reportID}`,
    value: null,
});

workspaceMembersChats.onyxFailureData.push({
    onyxMethod: Onyx.METHOD.SET,
    key: `${ONYXKEYS.COLLECTION.REPORT}${optimisticReport.reportID}`,
    value: null,
});

OR

We can show an error instead of clearing it like this: https://github.com/Expensify/App/blob/f251dbcdfdafc492d9a9908cbf40e4630b7b6d9d/src/libs/actions/IOU.ts#L804-L809

workspaceMembersChats.onyxFailureData.push({
    onyxMethod: Onyx.METHOD.MERGE,
    key: `${ONYXKEYS.COLLECTION.REPORT}${optimisticReport.reportID}`,
    value: {
        errorFields: {
            createChat: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('report.genericCreateReportFailureMessage'),
        },
    },
});

This way, the user will see an error on the chat and can manually remove it. image

To fix the #announce participant issue, we need to set null to each member.

value: {
    participants: accountIDs.reduce((acc, curr) => {
        Object.assign(acc, {[curr]: null});
        return acc;
    }, {}),
    pendingChatMembers: announceReport?.pendingChatMembers ?? null,
},

bernhardoj avatar Oct 23 '24 10:10 bernhardoj

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

melvin-bot[bot] avatar Oct 26 '24 00:10 melvin-bot[bot]

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

melvin-bot[bot] avatar Oct 26 '24 00:10 melvin-bot[bot]

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

melvin-bot[bot] avatar Oct 29 '24 18:10 melvin-bot[bot]

Hey @akinwale we have a proposal here, can you please review?

stephanieelliott avatar Oct 30 '24 08:10 stephanieelliott

Hey @akinwale we have a proposal here, can you please review?

Sure, I'll review today.

akinwale avatar Oct 30 '24 08:10 akinwale

We can move forward with @bernhardoj's proposal here.

@bernhardoj Let's go with the option that is already an established pattern in other parts of the app.

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed.

akinwale avatar Oct 30 '24 15:10 akinwale

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

melvin-bot[bot] avatar Oct 30 '24 15:10 melvin-bot[bot]

Sounds good, thanks @akinwale for the review! Assigning @bernhardoj πŸš€

marcochavezf avatar Oct 31 '24 20:10 marcochavezf

πŸ“£ @akinwale πŸŽ‰ 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] avatar Oct 31 '24 20:10 melvin-bot[bot]

Let's go with the option that is already an established pattern in other parts of the app.

@akinwale sorry, which one you mean? πŸ˜… The clear report one or show an error?

bernhardoj avatar Nov 01 '24 03:11 bernhardoj

Let's go with the option that is already an established pattern in other parts of the app.

@akinwale sorry, which one you mean? πŸ˜… The clear report one or show an error?

Display an error.

akinwale avatar Nov 01 '24 17:11 akinwale

Ok, thanks!

PR is ready

cc: @akinwale

bernhardoj avatar Nov 02 '24 04:11 bernhardoj

Hey @akinwale the PR is waiting for your review, can you take a look?

stephanieelliott avatar Nov 05 '24 08:11 stephanieelliott

@stephanieelliott I will review today.

akinwale avatar Nov 05 '24 16:11 akinwale

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

melvin-bot[bot] avatar Nov 11 '24 18:11 melvin-bot[bot]

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.59-3 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/51923

If no regressions arise, payment will be issued on 2024-11-18. :confetti_ball:

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

  • @akinwale requires payment automatic offer (Reviewer)
  • @bernhardoj requires payment through NewDot Manual Requests

melvin-bot[bot] avatar Nov 11 '24 18:11 melvin-bot[bot]

@akinwale @stephanieelliott The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

melvin-bot[bot] avatar Nov 11 '24 18:11 melvin-bot[bot]

@stephanieelliott I am eligible to be paid through ND on this one so I'll decline the automatic offer.

akinwale avatar Nov 15 '24 15:11 akinwale

Summarizing payment on this issue:

  • Contributor: @akinwale $250 - please request via ND
  • Contributor+: @bernhardoj $250 - please request via ND

stephanieelliott avatar Nov 19 '24 07:11 stephanieelliott

Requested in ND.

bernhardoj avatar Nov 19 '24 08:11 bernhardoj

$250 approved for @akinwale

JmillsExpensify avatar Nov 19 '24 09:11 JmillsExpensify

$250 approved for @bernhardoj

JmillsExpensify avatar Nov 21 '24 11:11 JmillsExpensify