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.

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

Optimistically create policy expense chats for new members of workspaces #16764

Merged
merged 11 commits into from
Apr 28, 2023
Merged
20 changes: 20 additions & 0 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -1544,6 +1544,25 @@ function getChatByParticipants(newParticipantList) {
});
}

/**
* Attempts to find a report in onyx with the provided list of participants in given policy
* @param {Array} newParticipantList
* @param {String} policyID
* @returns {object|undefined}
*/
function getChatByParticipantsAndPolicy(newParticipantList, policyID) {
newParticipantList.sort();
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the purpose of this sort? sort with no parameter? if it's necessary, can we add comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was modelling this function on the one above it: getChatByParticipants. But you're right, it looks irrelevant

Copy link
Contributor

Choose a reason for hiding this comment

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

ok we can remove that or leave it for now as long as it's not harmful
It was added here

return _.find(allReports, (report) => {
// If the report has been deleted, or there are no participants (like an empty #admins room) then skip it
if (!report || !report.participants) {
return false;
}

// Only return the room if it has all the participants and is not a policy room
return report.policyID === policyID && _.isEqual(newParticipantList, _.sortBy(report.participants));
});
}

/**
* @param {String} policyID
* @returns {Array}
Expand Down Expand Up @@ -1789,6 +1808,7 @@ export {
buildOptimisticAddCommentReportAction,
shouldReportBeInOptionList,
getChatByParticipants,
getChatByParticipantsAndPolicy,
getAllPolicyReports,
getIOUReportActionMessage,
getDisplayNameForParticipant,
Expand Down
111 changes: 110 additions & 1 deletion src/libs/actions/Policy.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import DateUtils from '../DateUtils';
import * as ReportUtils from '../ReportUtils';
import Log from '../Log';
import * as Report from './Report';
import Permissions from '../Permissions';

const allPolicies = {};
Onyx.connect({
Expand Down Expand Up @@ -227,17 +228,121 @@ function removeMembers(members, policyID) {
}, {optimisticData, successData, failureData});
}

/**
* Optimistically create a chat for each member of the workspace, creates both optimistic and success data for onyx.
*
* @param {String} policyID
* @param {Array} members
* @param {Array} betas
* @returns {Object} - object with onyxSuccessData, onyxOptimisticData, and optimisticReportIDs (map login to reportID)
*/
function createPolicyExpenseChats(policyID, members, betas) {
const workspaceMembersChats = {
onyxSuccessData: [],
onyxOptimisticData: [],
onyxFailureData: [],
reportCreationData: {},
};

// If the user is not in the beta, we don't want to create any chats
if (!Permissions.canUsePolicyExpenseChat(betas)) {
return workspaceMembersChats;
}

_.each(members, (login) => {
const oldChat = ReportUtils.getChatByParticipantsAndPolicy([sessionEmail, login], policyID);
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB for now since I think workspaces can only allow 1 admin anyways but as we expand to allow multiple admins this will break and only add the admin of the current user.

I think the members variable we pass to this method should have all users on the workspace right? I wonder if we could run something to check if they are a workspace admin and then add them to this array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

members variable stores only users which are going to be add to the workspace. But we can get the policy members from Onyx store and filter out members to get all admins. The downside is that while being offline there is possibility of not having this info stored, so we would still fallback to the [newMember, ourMail] situation like here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't be difficult to implement, so If you think it should support multiple admins already, I'm happy to help

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I think this is probably better as is for now, if/when we start allowing more than 1 admin on a workspace we can update this code as needed.


// If the chat already exists, we don't want to create a new one - just make sure it's not archived
if (oldChat) {
workspaceMembersChats.reportCreationData[login] = {
reportID: oldChat.reportID,
};
workspaceMembersChats.onyxOptimisticData.push({
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${oldChat.reportID}`,
value: {
stateNum: CONST.REPORT.STATE_NUM.OPEN,
statusNum: CONST.REPORT.STATUS.OPEN,
},
});
return;
}
const optimisticReport = ReportUtils.buildOptimisticChatReport(
[sessionEmail, login],
undefined,
CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT,
policyID,
login,
);
const optimisticCreatedAction = ReportUtils.buildOptimisticCreatedReportAction(optimisticReport.ownerEmail);

workspaceMembersChats.reportCreationData[login] = {
reportID: optimisticReport.reportID,
reportActionID: optimisticCreatedAction.reportActionID,
};

workspaceMembersChats.onyxOptimisticData.push({
onyxMethod: CONST.ONYX.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.REPORT}${optimisticReport.reportID}`,
value: {
...optimisticReport,
pendingFields: {
createChat: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
},
isOptimisticReport: true,
},
});
workspaceMembersChats.onyxOptimisticData.push({
onyxMethod: CONST.ONYX.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${optimisticReport.reportID}`,
value: {[optimisticCreatedAction.reportActionID]: optimisticCreatedAction},
});

workspaceMembersChats.onyxSuccessData.push({
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${optimisticReport.reportID}`,
value: {
pendingFields: {
createChat: null,
},
errorFields: {
createChat: null,
},
isOptimisticReport: false,
},
});
workspaceMembersChats.onyxSuccessData.push({
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${optimisticReport.reportID}`,
value: {[optimisticCreatedAction.reportActionID]: {pendingAction: null}},
});

workspaceMembersChats.onyxFailureData.push({
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${optimisticReport.reportID}`,
value: {
isLoadingReportActions: false,
},
});
});
return workspaceMembersChats;
}

/**
* Adds members to the specified workspace/policyID
*
* @param {Array<String>} memberLogins
* @param {String} welcomeNote
* @param {String} policyID
* @param {Array<String>} betas
*/
function addMembersToWorkspace(memberLogins, welcomeNote, policyID) {
function addMembersToWorkspace(memberLogins, welcomeNote, policyID, betas) {
const membersListKey = `${ONYXKEYS.COLLECTION.POLICY_MEMBER_LIST}${policyID}`;
const logins = _.map(memberLogins, memberLogin => OptionsListUtils.addSMSDomainIfPhoneNumber(memberLogin));

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

Choose a reason for hiding this comment

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

NAB maybe a better variable name would be like membersChatData


const optimisticData = [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
Expand All @@ -246,6 +351,7 @@ function addMembersToWorkspace(memberLogins, welcomeNote, policyID) {
// Convert to object with each key containing {pendingAction: ‘add’}
value: _.object(logins, Array(logins.length).fill({pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD})),
},
...membersChats.onyxOptimisticData,
];

const successData = [
Expand All @@ -257,6 +363,7 @@ function addMembersToWorkspace(memberLogins, welcomeNote, policyID) {
// need to remove the members since that will be handled by onClose of OfflineWithFeedback.
value: _.object(logins, Array(logins.length).fill({pendingAction: null, errors: null})),
},
...membersChats.onyxSuccessData,
];

const failureData = [
Expand All @@ -272,6 +379,7 @@ function addMembersToWorkspace(memberLogins, welcomeNote, policyID) {
},
})),
},
...membersChats.onyxFailureData,
];

API.write('AddMembersToWorkspace', {
Expand All @@ -280,6 +388,7 @@ function addMembersToWorkspace(memberLogins, welcomeNote, policyID) {
// Escape HTML special chars to enable them to appear in the invite email
welcomeNote: _.escape(welcomeNote),
policyID,
reportCreationData: JSON.stringify(membersChats.reportCreationData),
bondydaa marked this conversation as resolved.
Show resolved Hide resolved
}, {optimisticData, successData, failureData});
}

Expand Down
2 changes: 1 addition & 1 deletion src/pages/workspace/WorkspaceInvitePage.js
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ class WorkspaceInvitePage extends React.Component {
this.setState({shouldDisableButton: true}, () => {
const logins = _.map(this.state.selectedOptions, option => option.login);
const filteredLogins = _.uniq(_.compact(_.map(logins, login => login.toLowerCase().trim())));
Policy.addMembersToWorkspace(filteredLogins, this.state.welcomeNote, this.props.route.params.policyID);
Policy.addMembersToWorkspace(filteredLogins, this.state.welcomeNote, this.props.route.params.policyID, this.props.betas);
Navigation.goBack();
});
}
Expand Down