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

fix: optimistically add personal details #22904

Merged
merged 6 commits into from
Jul 19, 2023
Merged

fix: optimistically add personal details #22904

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9cbe95d
fix: optimistically add personal details
samh-nl Jul 14, 2023
571da94
refactor: use each instead of map
samh-nl Jul 15, 2023
f0d3a67
refactor: moved to PersonalDetailsUtils
samh-nl Jul 16, 2023
e6881a1
style: prettify
samh-nl Jul 16, 2023
67258bf
refactor: use personal details object
samh-nl Jul 17, 2023
1ee57d8
docs: expanded comment
samh-nl Jul 17, 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
62 changes: 60 additions & 2 deletions src/libs/PersonalDetailsUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,13 @@ import * as Localize from './Localize';
import * as UserUtils from './UserUtils';

let personalDetails = [];
let allPersonalDetails = {};
Onyx.connect({
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
callback: (val) => (personalDetails = _.values(val)),
callback: (val) => {
personalDetails = _.values(val);
allPersonalDetails = val;
},
});

/**
Expand Down Expand Up @@ -91,4 +95,58 @@ function getLoginsByAccountIDs(accountIDs) {
);
}

export {getDisplayNameOrDefault, getPersonalDetailsByIDs, getAccountIDsByLogins, getLoginsByAccountIDs};
/**
* Given a list of logins and accountIDs, return Onyx data for users with no existing personal details stored
*
* @param {Array<string>} logins Array of user logins
* @param {Array<number>} accountIDs Array of user accountIDs
* @returns {Object} - Object with optimisticData, successData and failureData (object of personal details objects)
*/
function getNewPersonalDetailsOnyxData(logins, accountIDs) {
const optimisticData = {};
const successData = {};
const failureData = {};

_.each(logins, (login, index) => {
const accountID = accountIDs[index];

if (_.isEmpty(allPersonalDetails[accountID])) {
optimisticData[accountID] = {
login,
accountID,
avatar: UserUtils.getDefaultAvatarURL(accountID),
displayName: login,
};

// Cleanup the optimistic user to ensure it does not permanently persist
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is incomplete. Can you please indicate that this is done to prevent duplicated entries since the server will return other personal details with the correct account ids.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I've expanded it. Let me know what you think.

successData[accountID] = null;
failureData[accountID] = null;
}
});

return {
optimisticData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
value: optimisticData,
},
],
successData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
value: successData,
},
],
failureData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
value: failureData,
},
],
};
}

export {getDisplayNameOrDefault, getPersonalDetailsByIDs, getAccountIDsByLogins, getLoginsByAccountIDs, getNewPersonalDetailsOnyxData};
7 changes: 6 additions & 1 deletion src/libs/actions/Policy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import ROUTES from '../../ROUTES';
import * as OptionsListUtils from '../OptionsListUtils';
import * as ErrorUtils from '../ErrorUtils';
import * as ReportUtils from '../ReportUtils';
import * as PersonalDetailsUtils from '../PersonalDetailsUtils';
import Log from '../Log';
import Permissions from '../Permissions';

Expand Down Expand Up @@ -354,7 +355,9 @@ function createPolicyExpenseChats(policyID, invitedEmailsToAccountIDs, betas) {
*/
function addMembersToWorkspace(invitedEmailsToAccountIDs, welcomeNote, policyID, betas) {
const membersListKey = `${ONYXKEYS.COLLECTION.POLICY_MEMBERS}${policyID}`;
const logins = _.map(_.keys(invitedEmailsToAccountIDs), (memberLogin) => OptionsListUtils.addSMSDomainIfPhoneNumber(memberLogin));
const accountIDs = _.values(invitedEmailsToAccountIDs);
const newPersonalDetailsOnyxData = PersonalDetailsUtils.getNewPersonalDetailsOnyxData(logins, accountIDs);

// create onyx data for policy expense chats for each new member
const membersChats = createPolicyExpenseChats(policyID, invitedEmailsToAccountIDs, betas);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we're doing same work in different issues.
Is it fine to revert this PR and apply changes in #22410?
We should add members optimistically to policy expense chat as well (if policyExpenseChat beta enabled), not only to workspace.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc: @rezkiy37 @madmax330

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add members optimistically to policy expense chat as well (if policyExpenseChat beta enabled), not only to workspace.

@0xmiroslav can you update your PR to do this?

Then the two would complement each other right?

Copy link
Contributor

Choose a reason for hiding this comment

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

#22410 is more optimized code as reusing optimistic members from workspace chat.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, go ahead and resolve the conflict in #22410 and we will review.

Expand All @@ -367,6 +370,7 @@ function addMembersToWorkspace(invitedEmailsToAccountIDs, welcomeNote, policyID,
// Convert to object with each key containing {pendingAction: ‘add’}
value: _.object(accountIDs, Array(accountIDs.length).fill({pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD})),
},
...newPersonalDetailsOnyxData.optimisticData,
...membersChats.onyxOptimisticData,
];

Expand All @@ -379,6 +383,7 @@ function addMembersToWorkspace(invitedEmailsToAccountIDs, welcomeNote, policyID,
// need to remove the members since that will be handled by onClose of OfflineWithFeedback.
value: _.object(accountIDs, Array(accountIDs.length).fill({pendingAction: null, errors: null})),
},
...newPersonalDetailsOnyxData.successData,
...membersChats.onyxSuccessData,
];

Expand All @@ -396,10 +401,10 @@ function addMembersToWorkspace(invitedEmailsToAccountIDs, welcomeNote, policyID,
}),
),
},
...newPersonalDetailsOnyxData.failureData,
...membersChats.onyxFailureData,
];

const logins = _.map(_.keys(invitedEmailsToAccountIDs), (memberLogin) => OptionsListUtils.addSMSDomainIfPhoneNumber(memberLogin));
const params = {
employees: JSON.stringify(_.map(logins, (login) => ({email: login}))),

Expand Down