Skip to content

Conversation

@Kelsey-Ethyca
Copy link
Contributor

@Kelsey-Ethyca Kelsey-Ethyca commented Nov 18, 2025

Ticket ENG-1739

Description Of Changes

This PR enables the Data stewards field in the System Information form and ensures that only eligible users (excluding approvers and external respondents) can be assigned as data stewards. The field is now a searchable, multiselect dropdown that allows users to select multiple data stewards directly in the form.

Previously, the Data stewards field was disabled and data stewards could only be assigned through the "Assign data steward" action menu. This change provides a more intuitive way to manage data stewards directly in the system form.

Key improvements:

  • Data stewards field is now enabled and functional in the System Information form
  • Searchable multiselect dropdown with filtering
  • Automatically filters out approvers and external respondents (who cannot be data stewards)
  • Properly handles adding and removing data stewards when the form is saved
  • System data is refreshed after steward assignments to show updated list

Backend changes:

  • Added exclude_approvers query parameter to the get_users endpoint
  • Filters users with the approver role when exclude_approvers=true is passed
  • Uses PostgreSQL array operator (@>) to efficiently filter by role

Code Changes

Backend:

  • src/fides/api/api/v1/endpoints/user_endpoints.py:
    • Added exclude_approvers parameter to get_users endpoint
    • Added filtering logic using INNER JOIN with FidesUserPermissions and PostgreSQL @> operator to exclude approvers

Frontend:

  • clients/admin-ui/src/features/system/form.ts:

    • Changed data_stewards type from string to string[] in FormValues
    • Updated default value to []
    • Updated transformSystemToFormValues to extract usernames as an array from system response
  • clients/admin-ui/src/features/system/SystemInformationForm.tsx:

    • Added useGetAllUsersQuery hook with exclude_approvers: true and include_external: false
    • Created dataStewardOptions using useMemo to transform eligible users into select options
    • Replaced disabled CustomTextInput with ControlledSelect component (multiselect, searchable)
    • Added useBulkAssignStewardMutation and useRemoveUserManagedSystemMutation hooks
    • Added logic in handleSubmit to:
      • Compare current vs desired stewards
      • Assign new stewards using bulkAssignSteward
      • Remove deselected stewards using removeUserManagedSystem (with proper user ID mapping)
      • Refetch system data after changes to show updated stewards list
    • Fixed linter error by wrapping option?.label with String() in filterOption
  • clients/admin-ui/src/features/system/SystemActionsMenu.tsx:

    • Updated useGetAllUsersQuery to include exclude_approvers: true parameter
  • clients/admin-ui/src/features/user-management/types.ts:

    • Added exclude_approvers?: boolean to UsersListParams interface
  • clients/admin-ui/src/features/user-management/user-management.slice.ts:

    • Added "System" to invalidatesTags for removeUserManagedSystem mutation to refresh system cache after steward removal

Steps to Confirm

  1. Navigate to a system in the Admin UI and open the System Information form
  2. Scroll to the "Administrative properties" section
  3. Verify the "Data stewards" field is enabled (not disabled) and shows a searchable multiselect dropdown
  4. Click on the dropdown and verify:
    • Only eligible users are shown (no approvers or external respondents)
    • You can search/filter users by typing
    • You can select multiple users
    • Selected users display their usernames
  5. Select one or more data stewards and save the form
  6. Verify the form saves successfully and shows a success message
  7. Refresh the page and verify the selected data stewards are persisted
  8. Edit the system again and remove a data steward (deselect them)
  9. Save the form and verify the steward is removed
  10. Verify that approvers cannot be selected (they should not appear in the dropdown list)
  11. Verify that external respondents cannot be selected (they should not appear in the dropdown list)
  12. Test the "Assign data steward" action menu to ensure it still works and also excludes approvers

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@vercel
Copy link

vercel bot commented Nov 18, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Nov 28, 2025 3:55pm
fides-privacy-center Ignored Ignored Nov 28, 2025 3:55pm

@Kelsey-Ethyca Kelsey-Ethyca changed the title add data stewards input to system info and exclude approvers from opt… Enabled data stewards field with searchable multiselect in system information form Nov 18, 2025
@Kelsey-Ethyca Kelsey-Ethyca marked this pull request as ready for review November 18, 2025 14:35
@Kelsey-Ethyca Kelsey-Ethyca requested review from a team as code owners November 18, 2025 14:35
@Kelsey-Ethyca Kelsey-Ethyca requested review from adamsachs and gilluminate and removed request for a team November 18, 2025 14:35
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 18, 2025

Greptile Summary

  • Enabled data stewards field in System Information form as searchable multiselect dropdown
  • Added backend filtering to exclude approvers and external respondents from eligible data stewards
  • Implemented automatic steward assignment/removal on form save with proper cache invalidation

Confidence Score: 4/5

  • This PR is generally safe to merge with one toast handling issue to address
  • The implementation is well-structured with proper type changes, backend filtering, and cache invalidation. However, there's a toast handling bug where toast.closeAll() can hide warning messages from steward assignment/removal failures, which should be fixed before merge.
  • SystemInformationForm.tsx needs attention for the toast handling issue

Important Files Changed

Filename Overview
clients/admin-ui/src/features/system/SystemInformationForm.tsx Enabled data stewards field with multiselect dropdown and added logic to assign/remove stewards, but has toast handling issue with warning messages

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (1)

  1. clients/admin-ui/src/features/system/SystemInformationForm.tsx, line 286 (link)

    logic: toast.closeAll() will hide warning toasts from steward assignment/removal failures that occur before handleResult is called. Move this call to happen before the steward operations (after line 314), or wait for all steward operations to complete before deciding which toasts to display.

    Context Used: Rule from dashboard - When handling multiple async operations that can fail independently, avoid using toast.closeAll() ... (source)

6 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Copy link
Contributor

@gilluminate gilluminate left a comment

Choose a reason for hiding this comment

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

Approved with 2 very minor comments. Overall FE code looks great! Looks like there are some Cypress tests that need cleaning up though.

Co-authored-by: Jason Gill <jason.gill@ethyca.com>
@codecov
Copy link

codecov bot commented Nov 28, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.00%. Comparing base (7a391d0) to head (81d7e66).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/fides/api/api/v1/endpoints/user_endpoints.py 50.00% 1 Missing and 2 partials ⚠️

❌ Your patch status has failed because the patch coverage (50.00%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6993      +/-   ##
==========================================
- Coverage   87.01%   87.00%   -0.02%     
==========================================
  Files         528      528              
  Lines       34674    34678       +4     
  Branches     4008     4010       +2     
==========================================
  Hits        30172    30172              
- Misses       3628     3629       +1     
- Partials      874      877       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Kelsey-Ethyca Kelsey-Ethyca added this pull request to the merge queue Nov 28, 2025
Merged via the queue into main with commit 171f009 Nov 28, 2025
74 of 75 checks passed
@Kelsey-Ethyca Kelsey-Ethyca deleted the ENG-1739-manually-updating-data-stewards-does-not-work-for-systems branch November 28, 2025 17:44
jjdaurora pushed a commit that referenced this pull request Dec 5, 2025
…ormation form (#6993)

Co-authored-by: Jason Gill <jason.gill@ethyca.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants