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.

Sign up for GitHub

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

Jump to bottom

Fix: Cannot remove RBR when invite an invalid member to workspace #23157

Merged
merged 8 commits into from
Jul 26, 2023
Merged

Fix: Cannot remove RBR when invite an invalid member to workspace #23157

Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a5a6b64
fix: cannot remove rbr when invite an invalid workspace member
tienifr Jul 19, 2023
304a5e6
exclude current account id
tienifr Jul 19, 2023
012b2aa
refactor using chain
tienifr Jul 19, 2023
9d734b9
dummy account ID length
tienifr Jul 19, 2023
ca996a6
isDummy account
tienifr Jul 21, 2023
78f8ba1
change isDummy to isOptimisticData
tienifr Jul 26, 2023
b3998dc
remove isOptimisticData
tienifr Jul 26, 2023
9eba1dd
remove isOptimisticData
tienifr Jul 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions src/libs/actions/Policy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ import {escapeRegExp} from 'lodash';
import * as API from '../API';
import ONYXKEYS from '../../ONYXKEYS';
import CONST from '../../CONST';
import * as LocalePhoneNumber from '../LocalePhoneNumber';
import * as OptionsListUtils from '../OptionsListUtils';
import * as ErrorUtils from '../ErrorUtils';
import * as ReportUtils from '../ReportUtils';
import Log from '../Log';
import Permissions from '../Permissions';
import * as UserUtils from '../UserUtils';

const allPolicies = {};
Onyx.connect({
Expand Down Expand Up @@ -355,6 +357,25 @@ function addMembersToWorkspace(invitedEmailsToAccountIDs, welcomeNote, policyID,
// create onyx data for policy expense chats for each new member
const membersChats = createPolicyExpenseChats(policyID, invitedEmailsToAccountIDs, betas);

// Optimistic personal details for the new accounts invited
const optimisticPersonalDetails = _.chain(invitedEmailsToAccountIDs)
.map(
(accountID, memberLogin) =>
!_.has(allPersonalDetails, accountID) && [
accountID,
{
accountID,
avatar: UserUtils.getDefaultAvatarURL(accountID),
displayName: LocalePhoneNumber.formatPhoneNumber(memberLogin),
login: OptionsListUtils.addSMSDomainIfPhoneNumber(memberLogin),
isDummy: true,
Copy link
Contributor

@NikkiWines NikkiWines Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually need isDummy? We are able to get the personalDetails to be removed in clearDeleteMemberError clearAddMemberError by accountID. Tested locally and seemed to work fine without including this

(edited function name)

Copy link
Contributor

@NikkiWines NikkiWines Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I guess for the cases where we have an error we want to dismiss but where we don't want to remove that user from the policy details 🤔 Though... do we have a case like that? I don't think so?

If we do (and it's entirely possible I'm missing/unaware of some flow like that) I guess I would call this something a little more descriptive like isOptimisticDetails / isOptimisicData or something like that.

Copy link
Contributor Author

@tienifr tienifr Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is cleared in clearAddMemberError, not delete. The point is to remove the invalid personal details on clearing the error. Otherwise, those invalid details would still be shown in the invite suggestion page. And isDummy is to check for those invalid ones. I've tried removing isDummy but it didn't work.

Also, isOptimisticData sounds great to me, thanks for the suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, sorry, you're right about that, I mistyped the function name. But the comment still holds, why do we need isDummy/isOptimisticData if we can remove the optimistically added member by accountID?

Copy link
Contributor Author

@tienifr tienifr Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, sorry I might not understand your point. Inside clearAddMemberError we only remove the accountID from the policy's members list ONYXKEYS.COLLECTION.POLICY_MEMBERS but we haven't remove it from personal details list, which we should do as explained above:

Otherwise, those invalid details would still be shown in the invite suggestion page

Also as you've mentioned here:

Tested locally and seemed to work fine without including this

Can you elaborate on the test you've made?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, I'm not explaining things very clearly 😅 Effectively, I'm not sure I understand why we need to include the isOptimisticData value at all.

The logic you have here conditionally retrieves the personal details that match the accountID and where isOptimisticData = true, and then removes those details from ONYXKEYS.PERSONAL_DETAILS_LIST, right?

But whenever we call clearAddMemberError, we're going to want to update ONYXKEYS.PERSONAL_DETAILS_LIST to remove the details for the provided accountID, because that user won't have been added to the workspace successfully. We can do this based on accountID alone, so why do we need isOptimisticData = true as a condition to be met?

Hopefully, that's a bit clearer? Let me know if I'm misunderstanding something or if it's still unclear.

Can you elaborate on the test you've made?

Yeah, for sure. I just removed this line and adjusted this line to be as follows:

-    if (lodashGet(allPersonalDetails, [accountID, 'isOptimisticData'])) {
+    if (lodashGet(allPersonalDetails, [accountID])) {

Then retested using the testing steps for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Thanks for your patience and detailed explanation. At first we tried to only remove the "dummy" account personal details but there's no way to do it. But as mentioned above, we should remove all errored account from personal details list.

Please check again!

},
],
)
.compact()
.object()
.value();

const optimisticData = [
{
onyxMethod: Onyx.METHOD.MERGE,
Expand All @@ -364,6 +385,11 @@ function addMembersToWorkspace(invitedEmailsToAccountIDs, welcomeNote, policyID,
value: _.object(accountIDs, Array(accountIDs.length).fill({pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD})),
},
...membersChats.onyxOptimisticData,
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
value: optimisticPersonalDetails,
},
];

const successData = [
Expand All @@ -376,6 +402,11 @@ function addMembersToWorkspace(invitedEmailsToAccountIDs, welcomeNote, policyID,
value: _.object(accountIDs, Array(accountIDs.length).fill({pendingAction: null, errors: null})),
},
...membersChats.onyxSuccessData,
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
value: _.object(_.keys(optimisticPersonalDetails), Array(_.size(optimisticPersonalDetails)).fill(null)),
},
];

const failureData = [
Expand Down Expand Up @@ -743,6 +774,13 @@ function clearAddMemberError(policyID, accountID) {
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY_MEMBERS}${policyID}`, {
[accountID]: null,
});

// Remove draft accountID
if (lodashGet(allPersonalDetails, [accountID, 'isDummy'])) {
Onyx.merge(`${ONYXKEYS.PERSONAL_DETAILS_LIST}`, {
[accountID]: null,
});
}
}

/**
Expand Down
Loading