From 9cbe95d63eef3070d22954db8887d149aa26fb3f Mon Sep 17 00:00:00 2001 From: Sam Hariri <137707942+samh-nl@users.noreply.github.com> Date: Fri, 14 Jul 2023 14:39:03 +0200 Subject: [PATCH 1/6] fix: optimistically add personal details --- src/libs/actions/Policy.js | 52 +++++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/src/libs/actions/Policy.js b/src/libs/actions/Policy.js index 5b0afa1caf69..968c46dd6661 100644 --- a/src/libs/actions/Policy.js +++ b/src/libs/actions/Policy.js @@ -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 UserUtils from '../UserUtils'; import Log from '../Log'; import Permissions from '../Permissions'; @@ -344,6 +345,39 @@ function createPolicyExpenseChats(policyID, invitedEmailsToAccountIDs, betas) { return workspaceMembersChats; } +/** + * Build personal details objects for a given list of logins and accountIDs + * + * @param {Array} logins Array of user logins + * @param {Array} accountIDs Array of user accountIDs + * @returns {Object} - object with optimisticData, successData and failureData (object of personal details objects) + */ +function buildPolicyPersonalDetails(logins, accountIDs) { + const policyPersonalDetails = { + optimisticData: {}, + successData: {}, + failureData: {}, + }; + + _.map(logins, (login, index) => { + const accountID = accountIDs[index]; + + if (_.isEmpty(personalDetails[accountID])) { + policyPersonalDetails.optimisticData[accountID] = { + login, + accountID, + avatar: UserUtils.getDefaultAvatarURL(accountID), + displayName: login, + }; + + policyPersonalDetails.successData[accountID] = null; + policyPersonalDetails.failureData[accountID] = null; + } + }); + + return policyPersonalDetails; +} + /** * Adds members to the specified workspace/policyID * @@ -354,12 +388,19 @@ 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 policyPersonalDetails = buildPolicyPersonalDetails(logins, accountIDs); // create onyx data for policy expense chats for each new member const membersChats = createPolicyExpenseChats(policyID, invitedEmailsToAccountIDs, betas); const optimisticData = [ + { + onyxMethod: Onyx.METHOD.MERGE, + key: ONYXKEYS.PERSONAL_DETAILS_LIST, + value: policyPersonalDetails.optimisticData, + }, { onyxMethod: Onyx.METHOD.MERGE, key: membersListKey, @@ -371,6 +412,11 @@ function addMembersToWorkspace(invitedEmailsToAccountIDs, welcomeNote, policyID, ]; const successData = [ + { + onyxMethod: Onyx.METHOD.MERGE, + key: ONYXKEYS.PERSONAL_DETAILS_LIST, + value: policyPersonalDetails.successData, + }, { onyxMethod: Onyx.METHOD.MERGE, key: membersListKey, @@ -383,6 +429,11 @@ function addMembersToWorkspace(invitedEmailsToAccountIDs, welcomeNote, policyID, ]; const failureData = [ + { + onyxMethod: Onyx.METHOD.MERGE, + key: ONYXKEYS.PERSONAL_DETAILS_LIST, + value: policyPersonalDetails.failureData, + }, { onyxMethod: Onyx.METHOD.MERGE, key: membersListKey, @@ -399,7 +450,6 @@ function addMembersToWorkspace(invitedEmailsToAccountIDs, welcomeNote, policyID, ...membersChats.onyxFailureData, ]; - const logins = _.map(_.keys(invitedEmailsToAccountIDs), (memberLogin) => OptionsListUtils.addSMSDomainIfPhoneNumber(memberLogin)); const params = { employees: JSON.stringify(_.map(logins, (login) => ({email: login}))), From 571da944a78fdfc98e362f4e59f1c8974a546bc0 Mon Sep 17 00:00:00 2001 From: Sam Hariri <137707942+samh-nl@users.noreply.github.com> Date: Sat, 15 Jul 2023 10:56:21 +0200 Subject: [PATCH 2/6] refactor: use each instead of map --- src/libs/actions/Policy.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/actions/Policy.js b/src/libs/actions/Policy.js index 968c46dd6661..d73b147be76f 100644 --- a/src/libs/actions/Policy.js +++ b/src/libs/actions/Policy.js @@ -359,7 +359,7 @@ function buildPolicyPersonalDetails(logins, accountIDs) { failureData: {}, }; - _.map(logins, (login, index) => { + _.each(logins, (login, index) => { const accountID = accountIDs[index]; if (_.isEmpty(personalDetails[accountID])) { From f0d3a67dae6ea06002c622d15610ca44880db683 Mon Sep 17 00:00:00 2001 From: Sam Hariri <137707942+samh-nl@users.noreply.github.com> Date: Sun, 16 Jul 2023 12:46:19 +0200 Subject: [PATCH 3/6] refactor: moved to PersonalDetailsUtils --- src/libs/PersonalDetailsUtils.js | 57 +++++++++++++++++++++++++++++++- src/libs/actions/Policy.js | 55 +++--------------------------- 2 files changed, 61 insertions(+), 51 deletions(-) diff --git a/src/libs/PersonalDetailsUtils.js b/src/libs/PersonalDetailsUtils.js index 26c0a67aae48..2fdf3deae82b 100644 --- a/src/libs/PersonalDetailsUtils.js +++ b/src/libs/PersonalDetailsUtils.js @@ -91,4 +91,59 @@ 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} logins Array of user logins + * @param {Array} 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]; + const currentDetail = _.find(personalDetails, (detail) => Number(detail.accountID) === Number(accountID)); + + if (_.isUndefined(currentDetail)) { + optimisticData[accountID] = { + login, + accountID, + avatar: UserUtils.getDefaultAvatarURL(accountID), + displayName: login, + }; + + // Cleanup the optimistic user to ensure it does not permanently persist + 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}; diff --git a/src/libs/actions/Policy.js b/src/libs/actions/Policy.js index d73b147be76f..0548b6c10ddb 100644 --- a/src/libs/actions/Policy.js +++ b/src/libs/actions/Policy.js @@ -12,7 +12,7 @@ import ROUTES from '../../ROUTES'; import * as OptionsListUtils from '../OptionsListUtils'; import * as ErrorUtils from '../ErrorUtils'; import * as ReportUtils from '../ReportUtils'; -import * as UserUtils from '../UserUtils'; +import * as PersonalDetailsUtils from '../PersonalDetailsUtils'; import Log from '../Log'; import Permissions from '../Permissions'; @@ -345,39 +345,6 @@ function createPolicyExpenseChats(policyID, invitedEmailsToAccountIDs, betas) { return workspaceMembersChats; } -/** - * Build personal details objects for a given list of logins and accountIDs - * - * @param {Array} logins Array of user logins - * @param {Array} accountIDs Array of user accountIDs - * @returns {Object} - object with optimisticData, successData and failureData (object of personal details objects) - */ -function buildPolicyPersonalDetails(logins, accountIDs) { - const policyPersonalDetails = { - optimisticData: {}, - successData: {}, - failureData: {}, - }; - - _.each(logins, (login, index) => { - const accountID = accountIDs[index]; - - if (_.isEmpty(personalDetails[accountID])) { - policyPersonalDetails.optimisticData[accountID] = { - login, - accountID, - avatar: UserUtils.getDefaultAvatarURL(accountID), - displayName: login, - }; - - policyPersonalDetails.successData[accountID] = null; - policyPersonalDetails.failureData[accountID] = null; - } - }); - - return policyPersonalDetails; -} - /** * Adds members to the specified workspace/policyID * @@ -390,17 +357,12 @@ function addMembersToWorkspace(invitedEmailsToAccountIDs, welcomeNote, policyID, const membersListKey = `${ONYXKEYS.COLLECTION.POLICY_MEMBERS}${policyID}`; const logins = _.map(_.keys(invitedEmailsToAccountIDs), (memberLogin) => OptionsListUtils.addSMSDomainIfPhoneNumber(memberLogin)); const accountIDs = _.values(invitedEmailsToAccountIDs); - const policyPersonalDetails = buildPolicyPersonalDetails(logins, accountIDs); + const newPersonalDetailsOnyxData = PersonalDetailsUtils.getNewPersonalDetailsOnyxData(logins, accountIDs); // create onyx data for policy expense chats for each new member const membersChats = createPolicyExpenseChats(policyID, invitedEmailsToAccountIDs, betas); const optimisticData = [ - { - onyxMethod: Onyx.METHOD.MERGE, - key: ONYXKEYS.PERSONAL_DETAILS_LIST, - value: policyPersonalDetails.optimisticData, - }, { onyxMethod: Onyx.METHOD.MERGE, key: membersListKey, @@ -408,15 +370,11 @@ 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, ]; const successData = [ - { - onyxMethod: Onyx.METHOD.MERGE, - key: ONYXKEYS.PERSONAL_DETAILS_LIST, - value: policyPersonalDetails.successData, - }, { onyxMethod: Onyx.METHOD.MERGE, key: membersListKey, @@ -425,15 +383,11 @@ 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, ]; const failureData = [ - { - onyxMethod: Onyx.METHOD.MERGE, - key: ONYXKEYS.PERSONAL_DETAILS_LIST, - value: policyPersonalDetails.failureData, - }, { onyxMethod: Onyx.METHOD.MERGE, key: membersListKey, @@ -447,6 +401,7 @@ function addMembersToWorkspace(invitedEmailsToAccountIDs, welcomeNote, policyID, }), ), }, + ...newPersonalDetailsOnyxData.failureData, ...membersChats.onyxFailureData, ]; From e6881a1c9b248114d49105e8de8659d49aee5148 Mon Sep 17 00:00:00 2001 From: Sam Hariri <137707942+samh-nl@users.noreply.github.com> Date: Sun, 16 Jul 2023 13:14:36 +0200 Subject: [PATCH 4/6] style: prettify --- src/libs/PersonalDetailsUtils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/PersonalDetailsUtils.js b/src/libs/PersonalDetailsUtils.js index 2fdf3deae82b..2cf263dbce7b 100644 --- a/src/libs/PersonalDetailsUtils.js +++ b/src/libs/PersonalDetailsUtils.js @@ -106,7 +106,7 @@ function getNewPersonalDetailsOnyxData(logins, accountIDs) { _.each(logins, (login, index) => { const accountID = accountIDs[index]; const currentDetail = _.find(personalDetails, (detail) => Number(detail.accountID) === Number(accountID)); - + if (_.isUndefined(currentDetail)) { optimisticData[accountID] = { login, From 67258bf0efc6e46b0fc9444821dfcbb5a18a8dfd Mon Sep 17 00:00:00 2001 From: Sam Hariri <137707942+samh-nl@users.noreply.github.com> Date: Mon, 17 Jul 2023 08:39:48 +0200 Subject: [PATCH 5/6] refactor: use personal details object --- src/libs/PersonalDetailsUtils.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/libs/PersonalDetailsUtils.js b/src/libs/PersonalDetailsUtils.js index 2cf263dbce7b..9860aa84aa80 100644 --- a/src/libs/PersonalDetailsUtils.js +++ b/src/libs/PersonalDetailsUtils.js @@ -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; + }, }); /** @@ -105,9 +109,8 @@ function getNewPersonalDetailsOnyxData(logins, accountIDs) { _.each(logins, (login, index) => { const accountID = accountIDs[index]; - const currentDetail = _.find(personalDetails, (detail) => Number(detail.accountID) === Number(accountID)); - if (_.isUndefined(currentDetail)) { + if (_.isEmpty(allPersonalDetails[accountID])) { optimisticData[accountID] = { login, accountID, From 1ee57d830cc277ea836a7adf7208726454fa0774 Mon Sep 17 00:00:00 2001 From: Sam Hariri <137707942+samh-nl@users.noreply.github.com> Date: Mon, 17 Jul 2023 21:08:59 +0200 Subject: [PATCH 6/6] docs: expanded comment --- src/libs/PersonalDetailsUtils.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libs/PersonalDetailsUtils.js b/src/libs/PersonalDetailsUtils.js index 9860aa84aa80..d3d7d6b84af7 100644 --- a/src/libs/PersonalDetailsUtils.js +++ b/src/libs/PersonalDetailsUtils.js @@ -118,7 +118,10 @@ function getNewPersonalDetailsOnyxData(logins, accountIDs) { displayName: login, }; - // Cleanup the optimistic user to ensure it does not permanently persist + /** + * Cleanup the optimistic user to ensure it does not permanently persist. + * This is done to prevent duplicate entries (upon success) since the BE will return other personal details with the correct account IDs. + */ successData[accountID] = null; failureData[accountID] = null; }