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
+69 −3
Merged

fix: optimistically add personal details #22904

Changes from 2 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
52 changes: 51 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 UserUtils from '../UserUtils';
import Log from '../Log';
import Permissions from '../Permissions';

Expand Down Expand Up @@ -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<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 buildPolicyPersonalDetails(logins, accountIDs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The function name is not the most accurate, this data is not explicit to policies. Can we move this function to somewhere more fitting? e.g. PersonalDetails.js. Also I think we should indicate that the function will only return onyx data for non existing users.

Copy link
Contributor Author

@samh-nl samh-nl Jul 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have:

  1. Moved it to PersonalDetailsUtils.js.
  2. Renamed it to getNewPersonalDetailsOnyxData.
  3. Updated the JSDoc.
  4. Made a small modification to ensure compatibility with PersonalDetailsUtils.js, where personalDetails is an array instead of an object.
  5. It now returns complete Onyx data (including onyxMethod and key) instead of just the personal detail objects. This makes it consistent with other functions that have "OnyxData" in their name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move that to PersonalDetails instead? Having personalDetails as an object is faster to access.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would moving it to PersonalDetails.js be semantically correct? It's placed in the actions/ folder and primarily contains functions triggering API calls, although also a few helper functions so it might be reasonable.

Yes iterating over the array is less efficient. It may have been done as a speed optimization if the other util functions are called often, so it's more efficient to cast it to an array at Onyx.connect instead of within the util function (executed many times). I would therefore be hesitant to change it to an object and put an extra cast to array inside the other existing functions, without further testing the performance implications.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should add another value to PersonalDetailsUtils? keeping personalDetails as array and adding allPersonalDetails as object.

Then we make make a follow up PR to rewrite the util functions that use the array to use the object and remove the array variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems like a good way forward. I have committed the change and will work on the refactor in a separate PR.

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this cause a flicker?

  1. Optimistically set account
  2. On success, remove account
  3. On success (server response), add account (probably another account)

Copy link
Contributor Author

@samh-nl samh-nl Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The swapping of the optimistic data with the server data on success should happen within the same rendering cycle.
I did not observe a flickering in the sense that members are removed and then reappear.
Avatars and display names, however, can update upon retrieving the server data. This behavior is consistent with other instances where one adds a user, for example if you create a report with or assign a task to a user that you have not interacted with yet.

The reason is that the optimistically set data is derived from the search result, and the search result is not an accurate depiction as no backend calls are made. In an ideal situation the search result is accurate and contains all information, but there may be performance/UX/privacy reasons why this was not pursued.

Copy link
Contributor Author

@samh-nl samh-nl Jul 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see that when creating a report with a new user, both the optimistic user and the BE user remain in personalDetailsList.
If you subsequently search for the user, you will see 2 search results instead of 1.

My addition of policyPersonalDetails.successData[accountID] = null; prevents such problem by ensuring the optimistic user is cleaned up. So I think in other parts of the codebase a similar cleanup mechanism needs to be put in place, however this is outside the scope of this issue (in my opinion).

Alternatively, we could add a boolean flag to the optimistic user and filter these out in the search results.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, same thing goes for policyMembers_ We will have redundant members. I think we can keep that outside the scope as well. Let's just make sure we have optimistic data for new non existing members.

policyPersonalDetails.failureData[accountID] = null;
}
});

return policyPersonalDetails;
}

/**
* Adds members to the specified workspace/policyID
*
Expand All @@ -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);
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.


const optimisticData = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
value: policyPersonalDetails.optimisticData,
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: membersListKey,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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}))),

Expand Down