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

Migrate policy API to remove policy members #10452

Merged
merged 27 commits into from
Sep 30, 2022
Merged

Migrate policy API to remove policy members #10452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
e6f1619
Remove unnneded code for DeleteMembersFromWorkspace
iwiznia Aug 18, 2022
a1ae9f5
Merge from main
iwiznia Sep 2, 2022
fc96430
Use employeeList in members page
iwiznia Sep 2, 2022
ec7e20d
Merge branch 'main' into ionatan_DeleteMembersFromWorkspace
iwiznia Sep 7, 2022
9ad6ec9
Transalate error
iwiznia Sep 7, 2022
41ad34c
Update to new Onyx version and fix editing comments
iwiznia Sep 13, 2022
c92093d
Fix active clients
iwiznia Sep 13, 2022
48c9543
Merge branch 'main' into ionatan_DeleteMembersFromWorkspace
iwiznia Sep 13, 2022
f1a5ba4
Address review comments
iwiznia Sep 13, 2022
faae0c7
Use each instead of map
iwiznia Sep 15, 2022
77cd2f3
Merge branch 'main' into ionatan_DeleteMembersFromWorkspace
iwiznia Sep 15, 2022
ff75e67
Merge branch 'main' into ionatan_DeleteMembersFromWorkspace
iwiznia Sep 16, 2022
11b8404
Fix activeClients bug and inviting new users
iwiznia Sep 16, 2022
8a1c3e9
Lint
iwiznia Sep 19, 2022
e32fc79
Remove unnecessary code
iwiznia Sep 19, 2022
c351e69
Update src/libs/ActiveClientManager/index.js
iwiznia Sep 20, 2022
f995ec4
Simplify ActiveClientManager
iwiznia Sep 20, 2022
d20176e
Merge branch 'ionatan_DeleteMembersFromWorkspace' of github.com:Expen…
iwiznia Sep 20, 2022
42bd676
Move promise to correct place
iwiznia Sep 20, 2022
4bc6ad9
Linting
iwiznia Sep 20, 2022
10b97ca
Add comment about init method
iwiznia Sep 20, 2022
2f08c2c
Merge branch 'main' into ionatan_DeleteMembersFromWorkspace
iwiznia Sep 23, 2022
c694ae7
Show no access page when you lose access to a chat
iwiznia Sep 28, 2022
57b6cb8
Merge branch 'main' into ionatan_DeleteMembersFromWorkspace
iwiznia Sep 28, 2022
93b977c
Lint
iwiznia Sep 28, 2022
67e8509
Merge with main
iwiznia Sep 30, 2022
fea93ab
Fix not found page showing close
iwiznia Sep 30, 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
14 changes: 7 additions & 7 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@
"react-native-image-picker": "^4.8.5",
"react-native-image-size": "git+https://github.com/Expensify/react-native-image-size#6b5ab5110dc3ed554f8eafbc38d7d87c17147972",
"react-native-modal": "^13.0.0",
"react-native-onyx": "1.0.15",
"react-native-onyx": "1.0.17",
"react-native-pdf": "^6.6.2",
"react-native-performance": "^2.0.0",
"react-native-permissions": "^3.0.1",
Expand Down
29 changes: 25 additions & 4 deletions src/components/BlockingViews/FullPageNotFoundView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,30 @@ const propTypes = {

/** If true, child components are replaced with a blocking "not found" view */
shouldShow: PropTypes.bool,

/** The key in the translations file to use for the title */
titleKey: PropTypes.string,

/** The key in the translations file to use for the subtitle */
subtitleKey: PropTypes.string,

/** Whether we should show a back icon */
shouldShowBackButton: PropTypes.bool,

/** Whether we should show a close button */
shouldShowCloseButton: PropTypes.bool,

/** Method to trigger when pressing the back button of the header */
onBackButtonPress: PropTypes.func,
};

const defaultProps = {
shouldShow: false,
titleKey: 'notFound.notHere',
subtitleKey: 'notFound.pageNotFound',
shouldShowBackButton: true,
shouldShowCloseButton: true,
onBackButtonPress: () => Navigation.dismissModal(),
};

// eslint-disable-next-line rulesdir/no-negated-variables
Expand All @@ -30,15 +50,16 @@ const FullPageNotFoundView = (props) => {
return (
<>
<HeaderWithCloseButton
shouldShowBackButton
onBackButtonPress={() => Navigation.dismissModal()}
shouldShowBackButton={props.shouldShowBackButton}
shouldShowCloseButton={props.shouldShowCloseButton}
onBackButtonPress={props.onBackButtonPress}
onCloseButtonPress={() => Navigation.dismissModal()}
/>
<View style={styles.flex1}>
<BlockingView
icon={Expensicons.QuestionMark}
title={props.translate('notFound.notHere')}
subtitle={props.translate('notFound.pageNotFound')}
title={props.translate(props.titleKey)}
subtitle={props.translate(props.subtitleKey)}
/>
</View>
</>
Expand Down
2 changes: 2 additions & 0 deletions src/languages/en.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,7 @@ export default {
iouReportNotFound: 'The payment details you are looking for cannot be found.',
notHere: "Hmm... it's not here",
pageNotFound: 'That page is nowhere to be found.',
noAccess: 'You don\'t have access to this chat',
},
setPasswordPage: {
enterPassword: 'Enter a password',
Expand Down Expand Up @@ -822,6 +823,7 @@ export default {
error: {
genericAdd: 'There was a problem adding this workspace member.',
cannotRemove: 'You cannot remove yourself or the workspace owner.',
genericRemove: 'There was a problem removing that workspace member.',
},
},
card: {
Expand Down
2 changes: 2 additions & 0 deletions src/languages/es.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,7 @@ export default {
iouReportNotFound: 'Los detalles del pago que estás buscando no se pudieron encontrar.',
notHere: 'Hmm… no está aquí',
pageNotFound: 'La página que buscas no existe.',
noAccess: 'No tienes acceso a este chat',
},
setPasswordPage: {
enterPassword: 'Escribe una contraseña',
Expand Down Expand Up @@ -824,6 +825,7 @@ export default {
error: {
genericAdd: 'Ha ocurrido un problema al agregar el miembro al espacio de trabajo',
cannotRemove: 'No puedes eliminarte ni a ti mismo ni al dueño del espacio de trabajo.',
genericRemove: 'Ha ocurrido un problema al eliminar al miembro del espacio de trabajo.',
},
},
card: {
Expand Down
39 changes: 27 additions & 12 deletions src/libs/ActiveClientManager/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
/**
* When you have many tabs in one browser, the data of Onyx is shared between all of them. Since we persist write requests in Onyx, we need to ensure that
* only one tab is processing those saved requests or we would be duplicating data (or creating errors).
* This file ensures exactly that by tracking all the clientIDs connected, storing the most recent one last and it considers that last clientID the "leader".
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Love the description 👍


import _ from 'underscore';
import Onyx from 'react-native-onyx';
import Str from 'expensify-common/lib/str';
Expand All @@ -6,38 +12,47 @@ import * as ActiveClients from '../actions/ActiveClients';

const clientID = Str.guid();
const maxClients = 20;

let activeClients;

let resolveIsReadyPromise;
const isReadyPromise = new Promise((resolve) => {
resolveIsReadyPromise = resolve;
let activeClients = [];
let resolveSavedSelfPromise;
const savedSelfPromise = new Promise((resolve) => {
resolveSavedSelfPromise = resolve;
});

/**
* Determines when the client is ready. We need to wait both till we saved our ID in onyx AND the init method was called
* @returns {Promise}
*/
function isReady() {
return isReadyPromise;
return savedSelfPromise;
}

Onyx.connect({
key: ONYXKEYS.ACTIVE_CLIENTS,
callback: (val) => {
activeClients = !val ? [] : val;
if (activeClients.length >= maxClients) {
activeClients = val;

// Remove from the beginning of the list any clients that are past the limit, to avoid having thousands of them
let removed = false;
while (activeClients.length >= maxClients) {
activeClients.shift();
removed = true;
}

// Save the clients back to onyx, if they changed
if (removed) {
ActiveClients.setActiveClients(activeClients);
}
},
});

/**
* Add our client ID to the list of active IDs
* Add our client ID to the list of active IDs.
* We want to ensure we have no duplicates and that the activeClient gets added at the end of the array (see isClientTheLeader)
*/
function init() {
ActiveClients.addClient(clientID)
.then(resolveIsReadyPromise);
activeClients = _.without(activeClients, clientID);
activeClients.push(clientID);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the intention of doing it like this is to ensure that the clientID is always added at the end of the array and its only added once. Would you mind updating the method description to talk about that? I know it's mentioned pretty well in the comment at the top of the file, but having the method doc comment talk about it is a little better IMO since it's closer to the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually don't get why are we concerned with it only being added once.

We only add more clients and take them away to keep the list of IDs from growing infinitely. The active client will always be the last one regardless of whether we clean the current user clientID.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Marc, but it's a NAB for me on this PR. Maybe a follow-up

ActiveClients.setActiveClients(activeClients).then(resolveSavedSelfPromise);
}

/**
Expand Down
2 changes: 0 additions & 2 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import KeyboardShortcut from '../../KeyboardShortcut';
import Navigation from '../Navigation';
import * as User from '../../actions/User';
import * as Modal from '../../actions/Modal';
import * as Policy from '../../actions/Policy';
import modalCardStyleInterpolator from './modalCardStyleInterpolator';
import createCustomModalStackNavigator from './createCustomModalStackNavigator';

Expand Down Expand Up @@ -100,7 +99,6 @@ class AuthScreens extends React.Component {
authEndpoint: `${CONFIG.EXPENSIFY.URL_API_ROOT}api?command=AuthenticatePusher`,
}).then(() => {
User.subscribeToUserEvents();
Policy.subscribeToPolicyEvents();
});

// Listen for report changes and fetch some data we need on initialization
Expand Down
13 changes: 3 additions & 10 deletions src/libs/actions/ActiveClients.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,13 @@ import ONYXKEYS from '../../ONYXKEYS';

/**
* @param {Array} activeClients
* @return {Promise}
*/
function setActiveClients(activeClients) {
Onyx.set(ONYXKEYS.ACTIVE_CLIENTS, activeClients);
}

/**
* @param {Number} clientID
* @returns {Promise}
*/
function addClient(clientID) {
return Onyx.merge(ONYXKEYS.ACTIVE_CLIENTS, [clientID]);
return Onyx.set(ONYXKEYS.ACTIVE_CLIENTS, activeClients);
}

export {
// eslint-disable-next-line import/prefer-default-export
setActiveClients,
addClient,
};
64 changes: 13 additions & 51 deletions src/libs/actions/Policy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,11 @@ import Str from 'expensify-common/lib/str';
import * as DeprecatedAPI from '../deprecatedAPI';
import * as API from '../API';
import ONYXKEYS from '../../ONYXKEYS';
import Growl from '../Growl';
import CONFIG from '../../CONFIG';
import CONST from '../../CONST';
import * as Localize from '../Localize';
import Navigation from '../Navigation/Navigation';
import ROUTES from '../../ROUTES';
import * as OptionsListUtils from '../OptionsListUtils';
import * as Report from './Report';
import * as Pusher from '../Pusher/pusher';
import DateUtils from '../DateUtils';
import * as ReportUtils from '../ReportUtils';

Expand Down Expand Up @@ -209,29 +205,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')}})),
Copy link
Contributor

Choose a reason for hiding this comment

The 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 errorMessage.
Since the API is a failure, the failureData here will get triggered to insert all members including removed ones with the error. Additionally the workspace.people.error.genericRemove is also inserted.
So if you removed 3 users, and the PHP layer failed on user 2.

  1. Won't all 3 users get workspace.people.error.genericRemove added to them since the API is considered a failure?
  2. Won't user 2 show workspace.people.error.genericRemove and errorMessage i.e., two error messages which could be confusing?

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.

Copy link
Contributor

@chiragsalian chiragsalian Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The 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 workspace.people.error.genericRemove in Spanish? and then the app would display both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 workspace.people.error.genericRemove in Spanish? and then the app would display both.

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):
image

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:

  • If we add the generic error, we get this behavior
  • If we don't add the generic error, we risk not showing any error at all if what we got from the request was a failure with no data back

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

NAB, but does failureData still set this generic error if one is provided from the server? I haven't personally relied on any default error messages like this yet. It's pretty unclear to me when I should use a default error message or when I should rely on the server. But maybe something we could be more consistent about or provide guidance on in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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});
}

/**
Expand Down Expand Up @@ -653,31 +641,6 @@ function updateLastAccessedWorkspace(policyID) {
Onyx.set(ONYXKEYS.LAST_ACCESSED_WORKSPACE_POLICY_ID, policyID);
}

/**
* Subscribe to public-policyEditor-[policyID] events.
*/
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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

@iwiznia iwiznia Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The 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)

});
}

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

Choose a reason for hiding this comment

The 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 null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Added a screenshot of how it looks to the description

}
});
});
}

/**
* Removes an error after trying to delete a member
*
Expand Down Expand Up @@ -974,7 +937,6 @@ export {
updateWorkspaceCustomUnit,
updateCustomUnitRate,
updateLastAccessedWorkspace,
subscribeToPolicyEvents,
clearDeleteMemberError,
clearAddMemberError,
clearDeleteWorkspaceError,
Expand Down
18 changes: 1 addition & 17 deletions src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,14 +331,6 @@ function fetchChatReportsByIDs(chatList, shouldRedirectIfInaccessible = false) {
// Fetch the personal details if there are any
PersonalDetails.getFromReportParticipants(_.values(simplifiedReports));
return simplifiedReports;
})
.catch((err) => {
if (err.message !== CONST.REPORT.ERROR.INACCESSIBLE_REPORT) {
return;
}

// eslint-disable-next-line no-use-before-define
handleInaccessibleReport();
});
}

Expand Down Expand Up @@ -1086,6 +1078,7 @@ function editReportComment(reportID, originalReportAction, textForNewComment) {
isEdited: true,
html: htmlForNewComment,
text: textForNewComment,
type: originalReportAction.message[0].type,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change in this PR? It seems unrelated.

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 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 fragment.text here

}],
},
};
Expand Down Expand Up @@ -1212,14 +1205,6 @@ function navigateToConciergeChat() {
Navigation.navigate(ROUTES.getReportRoute(conciergeChatReportID));
}

/**
* Handle the navigation when report is inaccessible
*/
function handleInaccessibleReport() {
Growl.error(Localize.translateLocal('notFound.chatYouLookingForCannotBeFound'));
navigateToConciergeChat();
}

/**
* Creates a policy room, fetches it, and navigates to it.
* @param {String} policyID
Expand Down Expand Up @@ -1544,7 +1529,6 @@ export {
getSimplifiedIOUReport,
syncChatAndIOUReports,
navigateToConciergeChat,
handleInaccessibleReport,
setReportWithDraft,
createPolicyRoom,
addPolicyReport,
Expand Down
Loading
Loading