Conversation
Removes feature flag restrictions to allow people admins to access and edit employee documents in the directory UI. Facilitates document management for authorized users.
Allows document editing only for super admins or people admins who are also e-sign senders, improving access control and aligning permissions with organizational requirements.
There was a problem hiding this comment.
Pull request overview
Enables the “Documents” section in the People edit flow and conditionally exposes a “Documents” step in the directory stepper based on admin/e-sign sender permissions.
Changes:
- Enabled
EditPeopleFormTypes.documentsto renderIndividualEmployeeDocumentViewinPeopleFormSections. - Added role-based visibility for the “Documents” step in
DirectorySteppersusingisSuperAdmin/isPeopleAdmin+isESignSender.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| frontend/src/community/people/components/organisms/PeopleFormSections/PeopleFormSections.tsx | Adds rendering for the “Documents” edit section. |
| frontend/src/community/people/components/molecules/DirectorySteppers/DirectorySteppers.tsx | Adds permission-based inclusion of the “Documents” step in the stepper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| : []), | ||
| // Feature flagged | ||
| // ...(isPeopleAdmin ? [translateText(["editAllInfo", "documents"])] : []) | ||
| ...(isSuperAdmin || (isPeopleAdmin && isESignSender) |
There was a problem hiding this comment.
The new "documents" step can be rendered even when isIndividualView or isAccountView is true. Those views render PeopleIndividualSection / PeopleAccountSection, which don't handle EditPeopleFormTypes.documents, so clicking the step will set currentStep to "Documents" and the content area will render nothing. Consider gating the documents step the same way as other edit-only steps (e.g., require !isIndividualView && !isAccountView), or add documents support to those sections if it should be available there.
| ...(isSuperAdmin || (isPeopleAdmin && isESignSender) | |
| ...(!isIndividualView && | |
| !isAccountView && | |
| (isSuperAdmin || (isPeopleAdmin && isESignSender)) |
| ...(isSuperAdmin || (isPeopleAdmin && isESignSender) | ||
| ? [translateText(["editAllInfo", "documents"])] | ||
| : []) |
There was a problem hiding this comment.
This new permission logic for the "documents" step is not covered by the existing DirectorySteppers tests. Adding/adjusting tests for (a) when the documents step should/shouldn't appear (super admin vs people admin + eSign sender vs others) and (b) clicking it should help prevent regressions in role-based visibility.
| case EditPeopleFormTypes.documents: | ||
| return ( | ||
| <IndividualEmployeeDocumentView selectedUser={Number(employeeId)} /> | ||
| ); |
There was a problem hiding this comment.
EditPeopleFormTypes.documents now renders IndividualEmployeeDocumentView unconditionally. Since currentStep is stored in a persisted zustand store, it's possible for a user without the required roles to end up on the "Documents" step (e.g., stale persisted state) even if the stepper hides it. Consider enforcing the same permission check here as in DirectorySteppers (e.g., require isSuperAdmin || (isPeopleAdmin && isESignSender)), and fallback to a safe default step or null when unauthorized.
Unifies conditions for displaying stepper steps and full-width layout using a dedicated variable, improving clarity and maintainability. Ensures step visibility and layout are consistent across views.
prasad-rtc
force-pushed
the
feat/42-admins-envelope-permissions
branch
from
b767299 to
f26360b
Compare
|
1 participant
PR checklist
TaskId: (https://github.com/SkappHQ/skapp/issues/[id])
Summary
How to test
Project Checklist
npm run formatnpm run check-lintOther
PR Checklist
ready-for-code-review)Additional Information