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

fix: admin can access member details page after member leaves #41492

Merged
merged 7 commits into from
May 16, 2024
Merged

fix: admin can access member details page after member leaves #41492

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e724493
fix: admin can access member details page after member leaves
tienifr May 2, 2024
e834286
Merge branch 'main' of https://github.com/tienifr/App into fix/40416
tienifr May 4, 2024
f1d86f0
not found page with wrapper
tienifr May 4, 2024
fc15f14
Merge branch 'main' of https://github.com/tienifr/App into fix/40416
tienifr May 15, 2024
83b9cc7
do not use negated variable name
tienifr May 15, 2024
f913df5
Revert "do not use negated variable name"
tienifr May 16, 2024
59918af
Merge branch 'main' of https://github.com/tienifr/App into fix/40416
tienifr May 16, 2024
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
19 changes: 18 additions & 1 deletion src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@ import Text from '@components/Text';
import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import usePrevious from '@hooks/usePrevious';
import useStyleUtils from '@hooks/useStyleUtils';
import useThemeStyles from '@hooks/useThemeStyles';
import * as UserUtils from '@libs/UserUtils';
import Navigation from '@navigation/Navigation';
import type {SettingsNavigatorParamList} from '@navigation/types';
import NotFoundPage from '@pages/ErrorPage/NotFoundPage';
import AccessOrNotFoundWrapper from '@pages/workspace/AccessOrNotFoundWrapper';
import type {WithPolicyAndFullscreenLoadingProps} from '@pages/workspace/withPolicyAndFullscreenLoading';
import withPolicyAndFullscreenLoading from '@pages/workspace/withPolicyAndFullscreenLoading';
Expand Down Expand Up @@ -56,6 +58,7 @@ function WorkspaceMemberDetailsPage({personalDetails, policy, route}: WorkspaceM

const memberLogin = personalDetails?.[accountID]?.login ?? '';
const member = policy?.employeeList?.[memberLogin];
const prevMember = usePrevious(member);
const details = personalDetails?.[accountID] ?? ({} as PersonalDetails);
const avatar = details.avatar ?? UserUtils.getDefaultAvatar();
const fallbackIcon = details.fallbackIcon ?? '';
Expand Down Expand Up @@ -95,14 +98,20 @@ function WorkspaceMemberDetailsPage({personalDetails, policy, route}: WorkspaceM
}
}, [accountID, policy?.errorFields?.changeOwner, policy?.isChangeOwnerSuccessful, policyID]);

useEffect(() => {
if (!prevMember || prevMember?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE || member?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {
return;
}
Navigation.goBack();
}, [member, prevMember]);

const askForConfirmationToRemove = () => {
setIsRemoveMemberConfirmModalVisible(true);
};

const removeUser = useCallback(() => {
Policy.removeMembers([accountID], policyID);
setIsRemoveMemberConfirmModalVisible(false);
Navigation.goBack();
}, [accountID, policyID]);

const navigateToProfile = useCallback(() => {
Expand All @@ -127,6 +136,14 @@ function WorkspaceMemberDetailsPage({personalDetails, policy, route}: WorkspaceM
Navigation.navigate(ROUTES.WORKSPACE_OWNER_CHANGE_CHECK.getRoute(policyID, accountID, 'amountOwed' as ValueOf<typeof CONST.POLICY.OWNERSHIP_ERRORS>));
}, [accountID, policyID]);

// eslint-disable-next-line rulesdir/no-negated-variables
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 don't think you should use the lint rule here.

You can just change the conditional

Copy link
Contributor Author

@tienifr tienifr May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the lint rule is a bit too strict, it should only block actual negated variable names like shouldNotShow but not shouldShowNotFoundPage because the not here is not really a negated adverb. I think it's fine since we're already using it many places in App.

But I still updated as your feedback.

const shouldShowPage = member && (member.pendingAction !== DELETE || prevMember?.pendingAction !== DELETE);

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, that makes sense, I guess that OK
Maybe you can add a comment to explain why the lint exception is appropriate.
I agree that shouldShowPage is a confusing name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted!

const shouldShowNotFoundPage =
!member || (member.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE && prevMember?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Coming from #54938, we missed the case of removing a member whilst offline. The useNetwork hook triggers an extra render, so member.pendingAction and prevMember.pendingAction are both delete as the modal is closing. See the proposal for more details.


if (shouldShowNotFoundPage) {
return <NotFoundPage />;
}

return (
<AccessOrNotFoundWrapper
policyID={policyID}
Expand Down
Loading