Skip to content

Move policy employee list logic to ONYXKEYS.COLLECTION.POLICY_MEMBER_LIST #10461

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

Merged
nkuoch merged 12 commits into from
Aug 24, 2022
Merged

Move policy employee list logic to ONYXKEYS.COLLECTION.POLICY_MEMBER_LIST #10461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
60c3bec
load policy employee list into new onyx key
arosiclair Aug 19, 2022
097f78e
connect policyMemberList in withFullPolicy
arosiclair Aug 19, 2022
0712c4a
use policyMemberList in WorkspaceMembersPage
arosiclair Aug 19, 2022
c134503
use policyMemberList in WorkspaceInvitePage
arosiclair Aug 19, 2022
2a7ef48
use new member list key when creating policies
arosiclair Aug 19, 2022
94f4aee
use new member list key when removing members
arosiclair Aug 19, 2022
cd5e71f
use new member list key when inviting members
arosiclair Aug 19, 2022
f775f69
use existing policyMemberPropType
arosiclair Aug 19, 2022
78730c6
use policyMemberList for removableMembers
arosiclair Aug 19, 2022
f95bd12
updated getSimplifiedEmployeeList jsdoc
arosiclair Aug 19, 2022
ca233bf
use empty object for default employee list entry
arosiclair Aug 23, 2022
f05e06c
simplify getSimplifiedEmployeList
arosiclair Aug 23, 2022
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
78 changes: 35 additions & 43 deletions src/libs/actions/Policy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,18 @@ Onyx.connect({
});

/**
* Simplifies the employeeList response into an object containing an array of emails
* Simplifies the employeeList response into an object mapping employee email to a default employee list entry
*
* @param {Object} employeeList
* @returns {Array}
* @returns {Object}
*/
function getSimplifiedEmployeeList(employeeList) {
const employeeListEmails = _.chain(employeeList)
return _.chain(employeeList)
.pluck('email')
.flatten()
.unique()
.reduce((map, email) => ({...map, [email]: {}}), {})
.value();

return employeeListEmails;
}

/**
Expand Down Expand Up @@ -92,7 +91,6 @@ function getSimplifiedPolicyObject(fullPolicyOrPolicySummary, isFromFullPolicy)
// "GetFullPolicy" and "GetPolicySummaryList" returns different policy objects. If policy is retrieved by "GetFullPolicy",
// avatarUrl will be nested within the key "value"
avatarURL: fullPolicyOrPolicySummary.avatarURL || lodashGet(fullPolicyOrPolicySummary, 'value.avatarURL', ''),
employeeList: getSimplifiedEmployeeList(lodashGet(fullPolicyOrPolicySummary, 'value.employeeList')),
customUnit: customUnitSimplified,
};
}
Expand Down Expand Up @@ -143,14 +141,16 @@ function create(name = '') {
Report.fetchChatReportsByIDs([response.policy.chatReportIDAdmins, response.policy.chatReportIDAnnounce, response.ownerPolicyExpenseChatID]);

// We are awaiting this merge so that we can guarantee our policy is available to any React components connected to the policies collection before we navigate to a new route.
return Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${response.policyID}`, {
employeeList: getSimplifiedEmployeeList(response.policy.employeeList),
id: response.policyID,
type: response.policy.type,
name: response.policy.name,
role: CONST.POLICY.ROLE.ADMIN,
outputCurrency: response.policy.outputCurrency,
});
return Promise.all(
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${response.policyID}`, {
Comment on lines +144 to +145

@parasharrajat parasharrajat Aug 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI, There is a regression from this change #10667

Promise.all accepts a single param as iterable.

@Beamanator Beamanator Aug 31, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI @arosiclair I'm planning to hire a contributor to fix this, in #10665

@Beamanator Beamanator Sep 2, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

JK it was fixed elsewhere :D

id: response.policyID,
type: response.policy.type,
name: response.policy.name,
role: CONST.POLICY.ROLE.ADMIN,
outputCurrency: response.policy.outputCurrency,
}),
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY_MEMBER_LIST}${response.policyID}`, getSimplifiedEmployeeList(response.policy.employeeList)),

@tgolen tgolen Aug 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it necessary to have both of these inside the promise.all? I don't think it matters when the member list is merged, does it? It also doesn't seem like it matters when the policy object is merged.

The only time the promise returned from create() is used is to navigate to the policy, but I don't think any of that requires the policy stuff to be merged into Onyx yet, does it?

@arosiclair arosiclair Aug 25, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The policy data would need to be merged otherwise the workspace page would briefly open as "Not Found" until the Onyx update finishes. I included the members list here also just to be safe. The functionality here is very temporary since it'll be replaced by incoming workspace refactors so I wouldn't lose too much sleep optimizing it.

@tgolen tgolen Aug 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, while I am fine not doing anything since it's only temporary, I do think it's important to always make clear and appropriate choices with code, regardless of how long it's going to live.

);
})
.then(() => Promise.resolve(lodashGet(res, 'policyID')));
}
Expand Down Expand Up @@ -259,10 +259,8 @@ function loadFullPolicy(policyID) {
return;
}

Onyx.set(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, {
...allPolicies[`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`],
...getSimplifiedPolicyObject(policy, true),
});
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, getSimplifiedPolicyObject(policy, true));
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY_MEMBER_LIST}${policy.id}`, getSimplifiedEmployeeList(lodashGet(policy, 'value.employeeList', {})));
});
}

Expand Down Expand Up @@ -291,14 +289,9 @@ function removeMembers(members, policyID) {
return;
}

const key = `${ONYXKEYS.COLLECTION.POLICY}${policyID}`;

// Make a shallow copy to preserve original data and remove the members
const policy = _.clone(allPolicies[key]);
policy.employeeList = _.without(policy.employeeList, ...members);

// Optimistically remove the members from the policy
Onyx.set(key, policy);
const employeeListUpdate = {};
_.each(members, login => employeeListUpdate[login] = null);
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY_MEMBER_LIST}${policyID}`, employeeListUpdate);

// Make the API call to remove a login from the policy
DeprecatedAPI.Policy_Employees_Remove({
Expand All @@ -309,9 +302,10 @@ function removeMembers(members, policyID) {
if (data.jsonCode === 200) {
return;
}
const policyDataWithMembersRemoved = _.clone(allPolicies[key]);
policyDataWithMembersRemoved.employeeList = [...policyDataWithMembersRemoved.employeeList, ...members];
Onyx.set(key, policyDataWithMembersRemoved);

// Rollback removal on failure
_.each(members, login => employeeListUpdate[login] = {});
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY_MEMBER_LIST}${policyID}`, employeeListUpdate);

// Show the user feedback that the removal failed
const errorMessage = data.jsonCode === 666 ? data.message : Localize.translateLocal('workspace.people.genericFailureMessage');
Expand All @@ -327,16 +321,15 @@ function removeMembers(members, policyID) {
* @param {String} policyID
*/
function invite(logins, welcomeNote, policyID) {
const key = `${ONYXKEYS.COLLECTION.POLICY}${policyID}`;
const newEmployeeList = _.map(logins, login => OptionsListUtils.addSMSDomainIfPhoneNumber(login));

// Make a shallow copy to preserve original data, and concat the login
const policy = _.clone(allPolicies[key]);
policy.employeeList = [...policy.employeeList, ...newEmployeeList];
policy.alertMessage = '';
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, {
alertMessage: '',
});

// Optimistically add the user to the policy
Onyx.merge(key, policy);
const newEmployeeLogins = _.map(logins, login => OptionsListUtils.addSMSDomainIfPhoneNumber(login));
const employeeUpdate = {};
_.each(newEmployeeLogins, login => employeeUpdate[login] = {});
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY_MEMBER_LIST}${policyID}`, employeeUpdate);

// Make the API call to merge the login into the policy
DeprecatedAPI.Policy_Employees_Merge({
Expand All @@ -356,16 +349,15 @@ function invite(logins, welcomeNote, policyID) {
}

// If the operation failed, undo the optimistic addition
const policyDataWithoutLogin = _.clone(allPolicies[key]);
policyDataWithoutLogin.employeeList = _.without(allPolicies[key].employeeList, ...newEmployeeList);
_.each(newEmployeeLogins, login => employeeUpdate[login] = null);
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY_MEMBER_LIST}${policyID}`, employeeUpdate);

// Show the user feedback that the addition failed
policyDataWithoutLogin.alertMessage = Localize.translateLocal('workspace.invite.genericFailureMessage');
let alertMessage = Localize.translateLocal('workspace.invite.genericFailureMessage');
if (data.jsonCode === 402) {
policyDataWithoutLogin.alertMessage += ` ${Localize.translateLocal('workspace.invite.pleaseEnterValidLogin')}`;
alertMessage += ` ${Localize.translateLocal('workspace.invite.pleaseEnterValidLogin')}`;
}

Onyx.set(key, policyDataWithoutLogin);
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, {alertMessage});
});
}

Expand Down
4 changes: 2 additions & 2 deletions src/pages/workspace/WorkspaceInvitePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ class WorkspaceInvitePage extends React.Component {
}

getExcludedUsers() {
const policyEmployeeList = lodashGet(this.props, 'policy.employeeList', []);
return [...CONST.EXPENSIFY_EMAILS, ...policyEmployeeList];
const policyMemberList = _.keys(lodashGet(this.props, 'policyMemberList', {}));
return [...CONST.EXPENSIFY_EMAILS, ...policyMemberList];
}

/**
Expand Down
9 changes: 5 additions & 4 deletions src/pages/workspace/WorkspaceMembersPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ class WorkspaceMembersPage extends React.Component {
*/
toggleAllUsers() {
this.setState({showTooltipForLogin: ''});
const removableMembers = _.without(this.props.policy.employeeList, this.props.session.email, this.props.policy.owner);
const policyMemberList = _.keys(lodashGet(this.props, 'policyMemberList', {}));
const removableMembers = _.without(policyMemberList, this.props.session.email, this.props.policy.owner);
this.setState(prevState => ({
selectedEmployees: removableMembers.length !== prevState.selectedEmployees.length
? removableMembers
Expand Down Expand Up @@ -263,9 +264,9 @@ class WorkspaceMembersPage extends React.Component {
}

render() {
const policyEmployeeList = lodashGet(this.props, 'policy.employeeList', []);
const removableMembers = _.without(this.props.policy.employeeList, this.props.session.email, this.props.policy.owner);
const data = _.chain(policyEmployeeList)
const policyMemberList = _.keys(lodashGet(this.props, 'policyMemberList', {}));
const removableMembers = _.without(policyMemberList, this.props.session.email, this.props.policy.owner);
const data = _.chain(policyMemberList)
.map(email => this.props.personalDetails[email])
.filter()
.sortBy(person => person.displayName.toLowerCase())
Expand Down
15 changes: 10 additions & 5 deletions src/pages/workspace/withFullPolicy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import CONST from '../../CONST';
import getComponentDisplayName from '../../libs/getComponentDisplayName';
import * as Policy from '../../libs/actions/Policy';
import ONYXKEYS from '../../ONYXKEYS';
import policyMemberPropType from '../policyMemberPropType';

let previousRouteName = '';
let previousRoutePolicyID = '';
Expand All @@ -35,7 +36,7 @@ function isPreviousRouteInSameWorkspace(routeName, policyID) {
}

const fullPolicyPropTypes = {
/** The full policy object for the current route (as opposed to the policy summary object) */
/** The policy object for the current route */
policy: PropTypes.shape({
/** The ID of the policy */
id: PropTypes.string,
Expand All @@ -57,10 +58,10 @@ const fullPolicyPropTypes = {

/** The URL for the policy avatar */
avatarURL: PropTypes.string,

/** A list of emails for the employees on the policy */
employeeList: PropTypes.arrayOf(PropTypes.string),
}),

/** The policy member list for the current route */
policyMemberList: PropTypes.objectOf(policyMemberPropType),
};

const fullPolicyDefaultProps = {
Expand Down Expand Up @@ -98,13 +99,14 @@ export default function (WrappedComponent) {
previousRouteName = currentRoute.name;
previousRoutePolicyID = policyID;

const rest = _.omit(props, ['forwardedRef', 'policy']);
const rest = _.omit(props, ['forwardedRef', 'policy', 'policyMemberList']);

@tgolen tgolen Aug 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are policy and policyMemberList removed from props, only to be added directly as props below? I think that forwardedRef is the only one that needs removed from props because it needs to be renamed to ref

@arosiclair arosiclair Aug 25, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Honestly not sure about this one I just followed the existing pattern. I'll see if it makes a difference

return (
<WrappedComponent
// eslint-disable-next-line react/jsx-props-no-spreading
{...rest}
ref={props.forwardedRef}
policy={props.policy}
policyMemberList={props.policyMemberList}
/>
);
};
Expand All @@ -121,6 +123,9 @@ export default function (WrappedComponent) {
policy: {
key: props => `${ONYXKEYS.COLLECTION.POLICY}${getPolicyIDFromRoute(props.route)}`,
},
policyMemberList: {
key: props => `${ONYXKEYS.COLLECTION.POLICY_MEMBER_LIST}${getPolicyIDFromRoute(props.route)}`,
},
})(withFullPolicy);
}

Expand Down