Access policies controls UI + new description field#7918
Access policies controls UI + new description field#7918lucanovera wants to merge 15 commits intomainfrom
Conversation
…y-groups-based-on-Control-adding-editing
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
lucanovera
requested review from
erosselli,
kruulik,
speaker-ender and
thabofletcher
and removed request for
a team,
erosselli and
speaker-ender
|
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (83.25%) is below the target coverage (85.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7918 +/- ##
==========================================
- Coverage 85.06% 83.25% -1.81%
==========================================
Files 629 629
Lines 40859 40860 +1
Branches 4748 4748
==========================================
- Hits 34757 34020 -737
- Misses 5029 5641 +612
- Partials 1073 1199 +126 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lucanovera
added
db-migration
|
/code-review re-review after changes |
…y-groups-based-on-Control-adding-editing
There was a problem hiding this comment.
Code Review: Controls Management UI (#7918)
Overall this is a clean, well-structured feature. The page/component decomposition is sensible, the RTK Query endpoints follow existing patterns, the MSW mocks are thorough, and the UX flow (list → create → edit → delete with confirmation) is cohesive. The tag invalidation strategy (deleteControl invalidating both "Controls" and "Access Policies") is a nice touch.
Issues to address before merge
Migration chain (must fix)
The down_revision = "b3c8d5e7f2a1" in the new migration needs to be verified against the actual HEAD revision on main before this lands. The related checklist items are still unchecked. Additionally, the xx_ filename prefix is non-standard for this repo — migrations typically use a revision-hash prefix; xx_ may cause Alembic sort/discovery issues.
router.query type safety (minor but real)
router.query.controlKey is string | string[] | undefined in Next.js. The as string cast in [controlKey].tsx bypasses this silently. A simple Array.isArray guard (see inline) makes it safe. The same pattern appears in the MSW handlers for req.params.key.
MSW PATCH mock semantics (minor)
The PATCH handler overwrites label and description from the body even when they are undefined, which would corrupt mock state for future contributors who send partial PATCH bodies. In practice today it's fine, but the mock semantics diverge from a real PATCH. See inline comment.
Nits / low-priority
useDeleteControlMutationloading state isn't captured in the list page, so nothing prevents a double-click during deletion. Low impact since RTK Query deduplicates, but the UX can stutter.ControlFormusesinitialValuesto populate the edit form. Ant Design doesn't re-applyinitialValueson prop changes — this is safe for the current page-per-route setup, but worth a comment in case the form gets reused elsewhere.- The "Manage controls" button on the access policies page is unconditionally rendered when
hasPoliciesis true, with no feature flag. If controls is being rolled out progressively, it may need a flag guard consistent withalphaPrivacyDocUpload.
The .fides/db_dataset.yml addition and the description field on the Control type/model are straightforward and correct.
🔬 Codegraph: connected (46725 nodes)
💡 Write /code-review in a comment to re-run this review.
| title: "Delete control", | ||
| content: ( | ||
| <span className="text-gray-500"> | ||
| Are you sure you want to delete the control "{control.label} |
There was a problem hiding this comment.
clients/admin-ui/src/pages/access-policies/controls/index.tsx:31
useDeleteControlMutation is destructured without capturing the loading state:
const [deleteControl] = useDeleteControlMutation();Nothing prevents a second delete click while the first is in-flight. Consider capturing isLoading and disabling the Delete button inside the confirm onOk, or at least preventing double-submission. RTK Query will deduplicate, but the UX is still jarring.
| const [form] = Form.useForm<ControlFormValues>(); | ||
| const isEditing = !!control; | ||
|
|
||
| const initialValues = useMemo<ControlFormValues>( |
There was a problem hiding this comment.
clients/admin-ui/src/features/access-policies/ControlForm.tsx:28
When control changes (e.g. if the edit page ever remounts with a different key without unmounting), initialValues will update via useMemo but Ant Design's Form ignores initialValues after the first render. If this form is ever reused in a context where the control prop can change in-place, you'll need to call form.setFieldsValue(initialValues) inside a useEffect. Not a problem for the current page-per-route setup, but worth a comment or defensive useEffect.
kruulik
left a comment
kruulik
left a comment
There was a problem hiding this comment.
No comments - code looks great! Tested it out locally with no issues.
…y-groups-based-on-Control-adding-editing
3 participants
Ticket ENG-2961
Description Of Changes
Adds a dedicated Controls management UI for creating, editing, and deleting controls (regulatory framework groupings for access policies). Controls now have a
descriptionfield. The key is auto-generated from the label on the backend usingget_key_from_data.The backend CRUD endpoints (POST/PATCH/DELETE/GET single for
/controls) are scaffolded in the fidesplus repo in a companion PR — this PR covers the fides-side changes (model, migration, and all frontend work).Code Changes
descriptioncolumn toControlORM model and alembic migration (d6e7f8a9b0c1)/access-policies/controls) using Ant DesignListcomponent withList.Item.Meta/access-policies/controls/new) and edit page (/access-policies/controls/[controlKey]) with sharedControlFormcomponent (Name + Description textarea)getControl(dedicated GET),createControl,updateControl,deleteControlCONTROLS_ROUTE,CONTROLS_NEW_ROUTE,CONTROLS_EDIT_ROUTEuseModal().confirm()withokButtonProps: { danger: true }NextLink; submit buttons show loading state during mutationsSteps to Confirm
Use
npm run dev:mocklocally to test/access-policies/controlslist pagenpx tsc --noEmitinclients/admin-ui/— should pass with no errorsPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works