-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Migrate policy API to remove policy members #10452
Changes from 8 commits
e6f1619
a1ae9f5
fc96430
ec7e20d
9ad6ec9
41ad34c
c92093d
48c9543
f1a5ba4
faae0c7
77cd2f3
ff75e67
11b8404
8a1c3e9
e32fc79
c351e69
f995ec4
d20176e
42bd676
4bc6ad9
10b97ca
2f08c2c
c694ae7
57b6cb8
93b977c
67e8509
fea93ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -218,29 +218,21 @@ function removeMembers(members, policyID) { | |
if (members.length === 0) { | ||
return; | ||
} | ||
|
||
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({ | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB, but does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, agree |
||
}]; | ||
API.write('DeleteMembersFromWorkspace', { | ||
emailList: members.join(','), | ||
policyID, | ||
}) | ||
.then((data) => { | ||
if (data.jsonCode === 200) { | ||
return; | ||
} | ||
|
||
// 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'); | ||
Growl.show(errorMessage, CONST.GROWL.ERROR, 5000); | ||
}); | ||
}, {optimisticData, failureData}); | ||
} | ||
|
||
/** | ||
|
@@ -699,21 +691,14 @@ function updateLastAccessedWorkspace(policyID) { | |
function subscribeToPolicyEvents() { | ||
_.each(allPolicies, (policy) => { | ||
const pusherChannelName = `public-policyEditor-${policy.id}${CONFIG.PUSHER.SUFFIX}`; | ||
Pusher.subscribe(pusherChannelName, 'policyEmployeeRemoved', ({removedEmails, policyExpenseChatIDs, defaultRoomChatIDs}) => { | ||
// Refetch the policy expense chats to update their state and their actions to get the archive reason | ||
if (!_.isEmpty(policyExpenseChatIDs)) { | ||
Report.fetchChatReportsByIDs(policyExpenseChatIDs); | ||
_.each(policyExpenseChatIDs, (reportID) => { | ||
Report.reconnect(reportID); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
if (removedEmails.includes(sessionEmail) && !_.isEmpty(defaultRoomChatIDs)) { | ||
_.each(defaultRoomChatIDs, (chatID) => { | ||
Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${chatID}`, null); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Added a screenshot of how it looks to the description |
||
if (!removedEmails.includes(sessionEmail) || _.isEmpty(defaultRoomChatIDs)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes, we are doing this in PHP already, we can now remove all this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this change in this PR? It seems unrelated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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,19 @@ class WorkspaceMembersPage extends React.Component { | |
} | ||
|
||
render() { | ||
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()) | ||
.map(person => ({...person})) // TODO: here we will add the pendingAction and errors prop | ||
.value(); | ||
const policyMemberList = lodashGet(this.props, 'policyMemberList', {}); | ||
const removableMembers = []; | ||
let data = _.map(policyMemberList, (value, email) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if (email !== this.props.session.email && email !== this.props.policy.owner) { | ||
removableMembers.push(email); | ||
} | ||
const details = this.props.personalDetails[email]; | ||
return { | ||
...value, | ||
...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.
Suggestion: Use
if (val && activeClients.indexOf(value) < 0)
instead of_.unique()
. I think it would be better to do if you're trying to prevent the same value being put into the array again.(sorry, I'm jaded against
_.unique()
after learning that it's not very performant).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.
Not that it matters much here, since at most there will be 20 items, but updated