[$500] Workspace - Invite members to workspace error but user can still join the workspace

lanitochka17 commented 9 months ago

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:

Open 2 users on 2 browsers
B send message to A
A create a workspace
A invite B and invalid user (+252 3 234211), observed an error appearing in 2 users when inviting
A go to announce room and send message
From B, Observe that B can access the room

Expected Result:

When the invite user fails, B cannot access the workspace

Actual Result:

B can access the workspace during invite error

Workaround:

Unknown

Platforms:

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

[ ] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[x] Windows / Chrome
[ ] MacOS / Desktop

Version Number: 1.3.55-7

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/78819774/971f2b38-d29e-48ec-8b3c-3979ff760643

https://github.com/Expensify/App/assets/78819774/a7ea2029-b25d-42d8-a67d-99eecd853888

Expensify/Expensify Issue URL:

Issue reported by: @namhihi237

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1691655815906349

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~017361e55ce3e1ac2d
Upwork Job ID: 1693979555238805504
Last Price Increase: 2024-05-02
Automatic offers:

s77rt | Reviewer | 102505066
tienifr | Contributor | 102505069

Issue Owner

Current Issue Owner: @bondydaa

bondydaa commented 3 months ago

posted here in wave8 https://expensify.slack.com/archives/C036QM0SLJK/p1707841028741169

bondydaa commented 2 months ago

looking into this again today and retested it locally and I think something has slightly changed, now it seems none of the members are added to the workspace if a single one errors out which I guess is slightly better than what was happening before but still not great IMO.

https://github.com/Expensify/App/assets/4073354/f5d31b2b-76b2-4664-875a-061ee0126bd5

But I'm going to test out a change locally to see if I can make this better so that users that can be added are and we only then show RBR errors for users who could not be added

bondydaa commented 2 months ago

i'm like halfway through figuring this out but hit some small blockers locally. I'll keep plugging on away on this tomorrow 🙏

bondydaa commented 2 months ago

I've got a draft PR up here for the API fixes https://github.com/Expensify/Web-Expensify/pull/41268

asking for help on how I can now send on to the clients/onyx the values for the users who had errors, I've got this mostly working so far 🎉

bondydaa commented 2 months ago

okay my PRs for the API changes have been fully deployed so I think we can have this go back to a contributor to finish off the new dot changes.

What I've done on the API side is update the response a bit so we need to update the optimistic/failure/success data a bit on the client side and better handle the response.

Previously if you'd tried to add more than 1 user and any user failed we would throw an error which would trigger the failure data to get rendered which resulted in the OP bug where then some users were actually added the workspace even though we showed the failureData for them.

Now if any user was successfully added to the workspace we won't throw an error at all so the response back to client will be as if everything worked properly. If a user in the list did fail though then the response will have the errors key that we can still pick up and RBR to show.

here's an example of the response data where 1 member was added and 1 member failed

I've also updated the API to expect the request to have look for accountID instead of just email here https://github.com/Expensify/App/blob/e542ba7c2d9e216c2cd401de0de8ed225fff4a7b/src/libs/actions/Policy.ts#L1344-L1348

so that we can then properly return any optimisticAccountID that failed to get created back to the clients directly from the request with the errors to render them in the UI. Here is the diff I'd used locally when testing my API changes for anyone who wants to take it.

diff --git a/src/libs/actions/Policy.ts b/src/libs/actions/Policy.ts
index 9e6a50f3f7..9cf4653d5c 100644
--- a/src/libs/actions/Policy.ts
+++ b/src/libs/actions/Policy.ts
@@ -1342,7 +1342,7 @@ function addMembersToWorkspace(invitedEmailsToAccountIDs: InvitedEmailsToAccount
     ];

     const params: AddMembersToWorkspaceParams = {
-        employees: JSON.stringify(logins.map((login) => ({email: login}))),
+        employees: JSON.stringify(Object.entries(invitedEmailsToAccountIDs).map(([login, accountID]) => ({email: PhoneNumber.addSMSDomainIfPhoneNumber(login), accountID}))),
         welcomeNote: new ExpensiMark().replace(welcomeNote),
         policyID,
     };
(END)

Let me know if there's anything else that needs clarifying but open for proposals on how to update this.

melvin-bot[bot] commented 2 months ago

Upwork job price has been updated to $500

melvin-bot[bot] commented 2 months ago

Current assignee @s77rt is eligible for the External assigner, not assigning anyone new.

s77rt commented 2 months ago

Not overdue. Looking for proposals

laurenreidexpensify commented 2 months ago

Not overdue, looking for proposals

shubham1206agra commented 2 months ago

I have talked to @bondydaa about another change in BE here https://expensify.slack.com/archives/C01GTK53T8Q/p1711666219473119?thread_ts=1711581709.271689&cid=C01GTK53T8Q. So that I can do the required changes in FE to make it working.

@laurenreidexpensify Maybe hold on to this.

cc @bondydaa for confirmation.

shubham1206agra commented 2 months ago

Proposal

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

Workspace - Invite members to workspace error, but user can still join the workspace

What is the root cause of that problem?

As per @bondydaa's comment here https://github.com/Expensify/App/issues/25592#issuecomment-2024143007, the BE is partially fixed.

We just need to make the changes to the API call as stated in the comment.

Ref: https://github.com/Expensify/App/blob/e542ba7c2d9e216c2cd401de0de8ed225fff4a7b/src/libs/actions/Policy.ts#L1344-L1348

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

Make the following change

diff --git a/src/libs/actions/Policy.ts b/src/libs/actions/Policy.ts
index 9e6a50f3f7..9cf4653d5c 100644
--- a/src/libs/actions/Policy.ts
+++ b/src/libs/actions/Policy.ts
@@ -1342,7 +1342,7 @@ function addMembersToWorkspace(invitedEmailsToAccountIDs: InvitedEmailsToAccount
     ];

     const params: AddMembersToWorkspaceParams = {
-        employees: JSON.stringify(logins.map((login) => ({email: login}))),
+        employees: JSON.stringify(Object.entries(invitedEmailsToAccountIDs).map(([login, accountID]) => ({email: PhoneNumber.addSMSDomainIfPhoneNumber(login), accountID}))),
         welcomeNote: new ExpensiMark().replace(welcomeNote),
         policyID,
     };
(END)

Right now, the error will not shown just yet, this requires another BE change discussed here https://expensify.slack.com/archives/C01GTK53T8Q/p1711615570179409?thread_ts=1711581709.271689&cid=C01GTK53T8Q.

Once we get the changes, we need to remove conditional logic here

https://github.com/Expensify/App/blob/e542ba7c2d9e216c2cd401de0de8ed225fff4a7b/src/libs/actions/Policy.ts#L1315-L1325

And change to

value: accountIDs.reduce((accountIDsWithClearedPendingAction, accountID) => {
                const value = {pendingAction: null};
                return {...accountIDsWithClearedPendingAction, [accountID]: value};
            }, {}),

bondydaa commented 2 months ago

I don't think we need to wait for the rest of the backend changes to go out here, there's other higher priority things on my plate so not sure how quickly I'll be able to get those out either.

I think we can make these changes to new dot incrementally anyways so that we can update it now to work with the existing API response as well as the idealized one we've suggested in slack.

I believe the current problem that is preventing us from rendering the updated errors from the API when we get them is that we just set value to null here https://github.com/Expensify/App/blob/ee5ab95f2c893738fb48756a9002e9d9dadd9a31/src/libs/actions/Policy.ts#L1321 https://github.com/Expensify/App/blob/ee5ab95f2c893738fb48756a9002e9d9dadd9a31/src/libs/actions/Policy.ts#L1328

We need to having something "smarter" that looks at the response and if there are accountIDs with errors keys we need to render them with RBRs, like how the failureData works.

The failureData then too needs to be updated to check for accountIDs with errors and render the message instead of the default that is set optimistically.

s77rt commented 2 months ago

@shubham1206agra Thanks for the proposal. Your RCA is correct. Regarding the solution we shouldn't clear the errors for all the accounts as some may have failed to get added.

melvin-bot[bot] commented 1 month ago

📣 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] commented 1 month ago

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

s77rt commented 1 month ago

Still looking for proposals

laurenreidexpensify commented 1 month ago

Still waiting on proposals

melvin-bot[bot] commented 1 month ago

📣 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] commented 1 month ago

Upwork job price has been updated to $1000

laurenreidexpensify commented 1 month ago

Updated bounty to see if we can find any proposals

tienifr commented 1 month ago

Proposal

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

There's no error showing for the users that were not added to the workspace due to errors like invalid phone number.

What is the root cause of that problem?

In here, we're always clearing the optimistic member in successData however some of them could have errors returned from the back-end and we need to display them.

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

First we need to update this to

employees: JSON.stringify(Object.entries(invitedEmailsToAccountIDs).map(([login, accountID]) => ({email: PhoneNumber.addSMSDomainIfPhoneNumber(login), accountID}))),

Then we need to have something like successDataPreprocessor that will make adjustments to successData (with inputs from the onyxData returned from back-end here).

The step will be (below is pseudo code):

Define the requestDataProcessor in a separate file that will be responsible for processing the successData

const requestDataProcessor = {
[WRITE_COMMANDS.ADD_MEMBERS_TO_WORKSPACE]: ({successData, onyxData}) => {
    // ... processing the `successData` not to remove the item in `policyMembers_` that has `errors` in `onyxData` 
    // also should keep their personal details
    // The rest of the optimistic member in `policyMembers_` can safely be removed
    return {successData}
}
}

When calling API.write here, pass the processor we want to use with this request. The processor will be stored alongside the Onyx data of the request.

API.write(WRITE_COMMANDS.ADD_MEMBERS_TO_WORKSPACE, params, {
optimisticData, 
successData, 
failureData,
processor: WRITE_COMMANDS.ADD_MEMBERS_TO_WORKSPACE
});

In here, if there's a processor defined in the Onyx data, we will get the successData after applying the processor, otherwise use the successData defined in request as usual
Then that successData will be applied here and the errors will show correctly

For failureData, we might need to do similarly to address the below

The failureData then too needs to be updated to check for accountIDs with errors and render the message instead of the default that is set optimistically.

Or we can just do not set the generic errors in failureData here so it will use the back-end's errors.

Note: This processor pattern is the same pattern we're using to parse the html/markdown in expensify-common

What alternative solutions did you explore? (Optional)

Do not clear the optimistic personal details in policyMembers_ list in both successData and failureData
Add a Onyx.connect to listen to policyMembers_ of the policy, if there're member records with errors defined, we'll then proceed to remove the optimistic personal detail that was successfully added as members (because they have their actual accountIDs now).

Another alternative:

Update the back-end to remove personal details/chats/... of optimistic users that were successfully deleted as members. But for those that failed to be added, do not remove personal details/chats/...
Do not remove personal details/chats/... of optimistic users in successData and failureData in the front-end

But I don't really prefer this since the back-end should not know or be responsible to clear the optimistic data in front-end side. That clearing logic should ideally be front-end only.

Update: There's currently an issue that when we add a phone number as member, it will not show in the members list, it's because we store the phone number in employeeList without emails, so we're unable to search for it in the emailToPersonalDetailsCache. To fix it we need to add email to phone number before searching in emailToPersonalDetailsCache. Or add email to phone number before saving to employeeList

melvin-bot[bot] commented 1 month ago

Upwork job price has been updated to $500

laurenreidexpensify commented 1 month ago

Catching up with @bondydaa - we think this job's price at $500 is probably more accurate, as we can largely apply this recommended solution here - https://github.com/Expensify/App/issues/25592#issuecomment-2032969494 that has already been suggested

s77rt commented 1 month ago

@tienifr Thanks for the proposal. Regarding the RCA, the code did change, now we are not removing any member, we are just clearing their pendingAction and errors. https://github.com/Expensify/App/blob/228e7696d98817bc6052a52abd39dbc06fbda926/src/libs/actions/Policy.ts#L1320-L1340

To prevent the errors from being cleared we should move the errors clearing logic to optimisticMembersState (that's the correct place).

Now onyx is holding the expected data, but still the user is not being rendered, can you look into that?

mvtglobally commented 1 month ago

Issue not reproducible during KI retests. (First week)

laurenreidexpensify commented 1 month ago

Given not reproducible, am closing

tienifr commented 1 month ago

@laurenreidexpensify The issue we're trying to fix here is still reproducible in latest staging. Could you help reopen the issue (more info below)?

After inviting the invalid number, it will disappear from the member list after a few seconds, and the RBR will appear in the Members tab without any way to dismiss it. Please see below reproduction video:

https://github.com/Expensify/App/assets/113963320/9b137a2e-a9a9-4401-b015-d531608486b8

tienifr commented 1 month ago

Now onyx is holding the expected data, but still the user is not being rendered, can you look into that?

@s77rt The personal details of the optimistic members were also cleared, see here and here.

When there's no personal details, the user will not be rendered (as there's no data on that user), see here. That's why we need to keep the personal details data of the optimistic user if that user failed to be added, as suggested in my proposal.

s77rt commented 1 month ago

@laurenreidexpensify We still have unfinished work here as outlined in @tienifr's video we are missing rendering errors for users that were failed to be invited.

s77rt commented 1 month ago

@tienifr Thanks for looking into this. Here are my thoughts:

we need to have something like successDataPreprocessor

Let's try avoid adding a new pattern i.e. successDataProcessors. At first I thought we can use API.makeRequestWithSideEffects to wait on the server's response but I see that this function execute the request immediately without being added to the queue.

we need to keep the personal details data of the optimistic user if that user failed to be added

Makes sense but can we achieve that without that new pattern?

I also noticed that getPersonalDetailsOnyxDataForOptimisticUsers is buggy (race condition) where in the finallyData we will always clear the personal details of optimistic account even if we "generated/guessed" the correct account id. Maybe we can get rid of this and find a better way to prevent duplicate entries?

s77rt commented 1 month ago

The second alternative solution does not look that bad to me. If we assume that a user would have an account id A and the BE returns a different id (id B), I'd assume it's pretty okay that the BE tell us that id A is not correct i.e. clear personal details for account with id A.

tienifr commented 1 month ago

@laurenreidexpensify We still have unfinished work here as outlined in @tienifr's video we are missing rendering errors for users that were failed to be invited.

@laurenreidexpensify friendly bump to reopen the issue!

tienifr commented 1 month ago

The second alternative solution does not look that bad to me. If we assume that a user would have an account id A and the BE returns a different id (id B), I'd assume it's pretty okay that the BE tell us that id A is not correct i.e. clear personal details for account with id A.

Yes we can explore that, @bondydaa Do you think we can make further BE changes to support the following solution?

Another alternative: Update the back-end to remove personal details/chats/... of optimistic users that were successfully deleted as members. But for those that failed to be added, do not remove personal details/chats/... Do not remove personal details/chats/... of optimistic users in successData and failureData in the front-end

(We do have a front-end only alternative if we can not do the above in BE, more details in the proposal)

bondydaa commented 1 month ago

I'm pretty sure this needs to be open still.

I would advise we stop suggesting further back end changes for now, I don't exactly have the time to focus on those at the moment and I believe we can get this working better with the existing response from the API.

tienifr commented 1 month ago

I would advise we stop suggesting further back end changes for now, I don't exactly have the time to focus on those at the moment and I believe we can get this working better with the existing response from the API.

@s77rt Do you think we can move forward with the successDataProcessors then?

s77rt commented 1 month ago

@tienifr I'd prefer to not add a new pattern. Can we instead get rid of the part that clears the optimistic personal details? and fix the duplicate entries in another way?

melvin-bot[bot] commented 1 month ago

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

laurenreidexpensify commented 1 month ago

@tienifr do you have any revised proposals? thanks

tienifr commented 1 month ago

@tienifr do you have any revised proposals? thanks

I'll post an update tomorrow, thanks!

melvin-bot[bot] commented 1 month ago

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

laurenreidexpensify commented 1 month ago

@tienifr any updates? thanks

tienifr commented 1 month ago

Sorry I haven't been able to get to this earlier, I will post an update within 24 hours

tienifr commented 1 month ago

@tienifr I'd prefer to not add a new pattern. Can we instead get rid of the part that clears the optimistic personal details? and fix the duplicate entries in another way?

@s77rt What do you think about this approach?

Use Onyx to listen to changes in policyMembers
For each accountId in policyMembers, if it has errors, all good because we'll keep the personal details of errored member
If it doesn't have errors, that mean it was a member successfully added, now we get the login of the personalDetail of that member
Now we check to see if there's any optimistic personal detail matching login, if there is, remove it.

This will make sure to clear the personal details of the successfully added members, but not of errored members.

s77rt commented 1 month ago

@tienifr Handling this in Onyx connect callback feels disconnected from the logic where we add the optimistic data. It would probably work but it makes the code a little harder to understand.

s77rt commented 1 month ago

@tienifr Can we extend the successData type to () => OnyxUpdate[] | OnyxUpdate[]. If it's a function we pass the server response.

This is a new pattern too. Usually adding a new pattern just for one case is not the best but we can discuss this in Slack especially if the implementation is not complex

tienifr commented 1 month ago

@tienifr Can we extend the successData type to () => OnyxUpdate[] | OnyxUpdate[]. If it's a function we pass the server response.

@s77rt I don't think we can do that, because the API request data will be serialized and saved to Onyx so it works in offline mode. A function cannot be serialized.

That's why I suggested to get the processor based on the command name in my proposal. I think what you're suggesting is quite similar, just that you're suggesting to define the processor inline.

melvin-bot[bot] commented 1 month ago

@bondydaa, @s77rt, @laurenreidexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

s77rt commented 1 month ago

@tienifr You are right. Unfortunately I don't see a clear solution. Having a request success yet contains error is a new pattern too so perhaps we do need something different. I will raise a slack thread to discuss this

tienifr commented 1 month ago

@s77rt Just curious if you think using the processor based on command name like I suggested is not clear? Or you're just concerned that it's a new pattern?

melvin-bot[bot] commented 1 month ago

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

Expensify / App