Migrate policy API to remove policy members #10452
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -218,29 +218,21 @@ function removeMembers(members, policyID) { | |
| if (members.length === 0) { | ||
| return; | ||
| } | ||
| const membersListKey = `${ONYXKEYS.COLLECTION.POLICY_MEMBER_LIST}${policyID}`; | ||
| const optimisticData = [{ | ||
| onyxMethod: CONST.ONYX.METHOD.MERGE, | ||
| key: membersListKey, | ||
| value: _.object(members, Array(members.length).fill({pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE})), | ||
| }]; | ||
| const failureData = [{ | ||
| onyxMethod: CONST.ONYX.METHOD.MERGE, | ||
| key: membersListKey, | ||
| value: _.object(members, Array(members.length).fill({errors: {[DateUtils.getMicroseconds()]: Localize.translateLocal('workspace.people.error.genericRemove')}})), | ||
|
There was a problem hiding this comment. I have a question with this failureData. It looks good to me, but assume it fails when removing a user. Assume the the failure happens in PHP/auth layer. Now your PHP code here will throw an ExpError with an
Anyway my concern is mostly point 1, showing user1 and user3 back even though they were successfully removed, because push notification would run first to remove them and then failureData from the ExpError of user2 would add them back, right? Let me know if that makes sense, if so I think maybe we should not have failureData over here. There was a problem hiding this comment. Also for any es device won't it be weird that the PHP layer is providing the ExpError in English while the app adds There was a problem hiding this comment.
We don't care. We are not supporting multiple languages. As for the other comment, you are incorrect on point 1, here's a test I did by adding a throw before calling sharePolicy on a specific user (I had removed 2 users): As you can see, you are correct on point 2. I thought we would replace it, but that's not correct since we merge... this is a problem in many other APIs, since this is the pattern we are following in lots of them... I don't have a great solution for it right now, because:
Any ideas? In any case, I don't think I would want to block on this, given this problem is endemic and must probably be fixed in many other places. There was a problem hiding this comment. NAB, but does |
||
| }]; | ||
| API.write('DeleteMembersFromWorkspace', { | ||
| emailList: members.join(','), | ||
| policyID, | ||
| }, {optimisticData, failureData}); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -699,21 +691,14 @@ function updateLastAccessedWorkspace(policyID) { | |
| function subscribeToPolicyEvents() { | ||
| _.each(allPolicies, (policy) => { | ||
| const pusherChannelName = `public-policyEditor-${policy.id}${CONFIG.PUSHER.SUFFIX}`; | ||
|
There was a problem hiding this comment. NAB, I did not see the Web PR, but curious if we kept this logic here to fetch the report data and report history for any policyExpenseChats? There was a problem hiding this comment. Policy expense chats should be sent via onyx when adding, removing or changing the role of users (and AFAIK we are doing that, at least the remove part is done in my web PR) |
||
| Pusher.subscribe(pusherChannelName, 'policyEmployeeRemoved', ({removedEmails, defaultRoomChatIDs}) => { | ||
| // Remove the default chats if we are one of the users getting removed | ||
|
There was a problem hiding this comment. NAB, are we doing this part via the server now? What does it look like for the user if they are looking at a chat and it is set to There was a problem hiding this comment. Yes. Added a screenshot of how it looks to the description |
||
| if (!removedEmails.includes(sessionEmail) || _.isEmpty(defaultRoomChatIDs)) { | ||
|
There was a problem hiding this comment. Is it possible to do this logic in PHP so that the logic on the client can be more simple? Also, why isn't PHP sending Onyx instructions that are passed to There was a problem hiding this comment. Ah yes, we are doing this in PHP already, we can now remove all this |
||
| return; | ||
| } | ||
| _.each(defaultRoomChatIDs, (chatID) => { | ||
| Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatID}`, null); | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1263,6 +1263,7 @@ function editReportComment(reportID, originalReportAction, textForNewComment) { | |
| isEdited: true, | ||
| html: htmlForNewComment, | ||
| text: textForNewComment, | ||
| type: originalReportAction.message[0].type, | ||
|
There was a problem hiding this comment. Why is this change in this PR? It seems unrelated. There was a problem hiding this comment. It is not. Messages is an array and we were not sending the type in the optimistic action (neither in the action from the server), so when we replace the message fully now, this property was not set and was causing issues (we would render the comment as |
||
| }], | ||
| }, | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -283,14 +283,20 @@ class WorkspaceMembersPage extends React.Component { | |
| } | ||
|
|
||
| render() { | ||
| const policyMemberList = lodashGet(this.props, 'policyMemberList', {}); | ||
| const removableMembers = []; | ||
| let data = []; | ||
| _.each(policyMemberList, (policyMember, email) => { | ||
|
There was a problem hiding this comment. The update here to each alone is not correct. Notice in the previous code we filter to remove empty values but that's not here. So over here I think you should not update data if details is empty otherwise you'll still get the same error when testing locally because it cannot do lowercase if value is empty. Sample data provided here. This will result in an app crash when members page is clicked which is bad, So my suggestion is to update this, to be, if (details) {
data.push({
...policyMember,
...details,
});
}There was a problem hiding this comment. That was a hack that someone added because the app crashed and instead of fixing the real issue, they just removed the detail from the list, which is wrong. This works if you are using the latest version of auth, that returns personal details for all emails, even if they don't have explicitly set a name. Well, at least I hope the crash you saw was due to outdated auth, sounds like it because it's the same I was getting before sending the auth fix. There was a problem hiding this comment. Oh, you also need the web PR of course There was a problem hiding this comment. Odd, im sure i updates all repos and did a makeall yesterday. Maybe its buggy on an existing workspace, I'll try again on a fresh workspace. There was a problem hiding this comment. Should not be buggy on existing data. Maybe.you need to sign out and back in though, not sure. |
||
| if (email !== this.props.session.email && email !== this.props.policy.owner) { | ||
| removableMembers.push(email); | ||
| } | ||
| const details = this.props.personalDetails[email]; | ||
| data.push({ | ||
| ...policyMember, | ||
| ...details, | ||
| }); | ||
| }); | ||
| data = _.sortBy(data, value => value.displayName.toLowerCase()); | ||
| const policyID = lodashGet(this.props.route, 'params.policyID'); | ||
| const policyName = lodashGet(this.props.policy, 'name'); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats this change for? When i test your PR the command
DeleteMembersFromWorkspacenever gets called in the network tab and I think its because of the code here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to refactor this a bit because this was using the array merging feature of onyx that we changed. It is not called in the flow of that api command.