-
Notifications
You must be signed in to change notification settings - Fork 84
Eng 1262 custom field improvements #6898
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
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.
Greptile Overview
Greptile Summary
This PR extends custom fields to support custom taxonomies by migrating resource_type from an enum to a string in the database and frontend.
Key changes:
- Migration converts
resource_typecolumn from PostgreSQL enum to String type inplus_custom_field_definitionandplus_custom_fieldtables - Backend models updated to accept any string value for
resource_typeinstead of restricted enum - Frontend adds
useGetCustomFieldLocationsQuery()to dynamically fetch available locations - Resource type normalization logic added to handle display format conversions (e.g.,
taxonomy:data_category→data_category) - Taxonomy helpers updated to pass through custom taxonomy types instead of returning
undefined
Critical issue:
- The frontend calls a new endpoint
/plus/custom-fields/locationsbut this backend endpoint implementation is not included in this PR and does not appear to exist in the codebase, which will cause the custom field form to fail when loading
Confidence Score: 1/5
- This PR cannot be safely merged due to a missing backend endpoint
- Score reflects a critical blocking issue: the frontend introduces a call to
useGetCustomFieldLocationsQuery()which expects a/plus/custom-fields/locationsbackend endpoint, but this endpoint is not implemented in this PR or the existing codebase. This will cause runtime errors when users try to create or edit custom fields. - Pay close attention to
clients/admin-ui/src/features/plus/plus.slice.tsandclients/admin-ui/src/features/custom-fields/CustomFieldForm.tsx- they reference a missing backend endpoint
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/fides/api/alembic/migrations/versions/303287c70600_migrate_resource_type_from_enum_to_.py | 5/5 | Converts resource_type from Enum to String in custom field tables to support custom taxonomies. Migration is clean with proper PostgreSQL casting. |
| clients/admin-ui/src/features/custom-fields/useCreateOrUpdateCustomField.ts | 4/5 | Adds normalizeResourceType function to convert UI display format (e.g., taxonomy:data_category) to backend format before submission. |
| clients/admin-ui/src/features/custom-fields/CustomFieldForm.tsx | 2/5 | Calls useGetCustomFieldLocationsQuery() to dynamically populate location options. CRITICAL: Backend endpoint appears to be missing. |
| clients/admin-ui/src/features/plus/plus.slice.ts | 2/5 | Adds getCustomFieldLocations RTK Query endpoint and changes getCustomFieldDefinitionsByResourceType to accept string instead of enum. CRITICAL: Backend endpoint /plus/custom-fields/locations appears to be missing. |
9 files reviewed, 2 comments
| const normalizeResourceType = (rt: string): string => { | ||
| if (rt.startsWith("taxonomy:")) { | ||
| return rt.split(":", 2)[1]; | ||
| } | ||
| if (rt === "system:information") return "system"; | ||
| if (rt === "system:data_use") return "privacy_declaration"; | ||
| return rt; | ||
| }; | ||
|
|
||
| values.resource_type = normalizeResourceType( | ||
| values.resource_type as unknown as string, | ||
| ) as any; |
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.
style: This transformation logic duplicates the mapping that should be handled by the backend. The UI should send the key as-is (e.g., taxonomy:data_category) and let the backend handle the normalization, making the contract clearer and avoiding frontend/backend logic duplication.
8b53758 to
455ec40
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6898 +/- ##
==========================================
- Coverage 87.03% 87.03% -0.01%
==========================================
Files 528 528
Lines 34668 34666 -2
Branches 4005 4005
==========================================
- Hits 30174 30172 -2
Misses 3620 3620
Partials 874 874 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b24c38a to
006dbed
Compare
Approved by another reviewer because Lucano is OOO.
…om/ethyca/fides into ENG-1262-custom-field-improvements
Co-authored-by: Jeremy Pople <jeremy@ethyca.com>
https://ethyca.atlassian.net/browse/ENG-1262 (fides portion)
Fidesplus portion is here- https://github.com/ethyca/fidesplus/pull/2722
Description Of Changes
Adds support for adding custom fields to custom taxonomies and for creating custom fields with values of custom taxonomy keys.
Code Changes
Steps to Confirm
Run a Fidesplus backend using the branch linked above.
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works