Implement multi-page "select all" for Okta monitor results#7307
Implement multi-page "select all" for Okta monitor results#7307
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Greptile OverviewGreptile Summaryimplements multi-page "select all" functionality for Okta monitor results with two selection modes: explicit (individual selections) and all (select everything with exclusions) Key Changes:
Issues Found:
Confidence Score: 3/5
Important Files Changed
|
clients/admin-ui/src/features/data-discovery-and-detection/discovery-detection.slice.ts
Outdated
Show resolved
Hide resolved
speaker-ender
left a comment
There was a problem hiding this comment.
Tested locally and works as expected.
Minor nits and questions
| url: `/plus/identity-provider-monitors/${monitor_config_key}/results/bulk-mute`, | ||
| body: urns, | ||
| }), | ||
| query: ({ monitor_config_key, urns, bulkSelection }) => { |
There was a problem hiding this comment.
Is there a reason why passing both args is disallowed?
There was a problem hiding this comment.
The backend will error if both are provided and nonempty so I preferred enforcing that here. Technically the UI shouldn't ever be in a state where we have a value for both, but if the selection itself is working correctly it shouldn't cause any harm, if you think just passing both is better.
There was a problem hiding this comment.
Oof that's not good api design imo bc there is not way to know that without looking at comments in the docs (if they even exist).
I'm fine with leaving as is to help reduce invalid queries but I'd add some comments about why.
Alternatively you could make a discriminated union type that would disallow certain combinations of filters lol.
| bulkSelection?: { | ||
| filters?: { | ||
| search?: string; | ||
| diff_status?: DiffStatus | DiffStatus[]; | ||
| vendor_id?: string | string[]; | ||
| data_uses?: string[]; | ||
| }; | ||
| exclude_urns?: string[]; | ||
| }; |
There was a problem hiding this comment.
Is there a need to abstract part of the raw filters?
There was a problem hiding this comment.
Yeah, good callout-- I've made a new type for these filter values and applied it to other areas where they're used.
| excludedKeys, | ||
| excludedUrns, | ||
| selectionMode, |
There was a problem hiding this comment.
Is there a reason why the useBulkListSelect hook wasn't able to work here? (I realize this hook wasn't added in the PR)
There was a problem hiding this comment.
I messed with this a good bit and it seems impractical to integrate useBulkListSelect with the existing custom selection hook. I think it could probably be rewritten to use useBulkListSelect directly and obviate the custom hook entirely but it'd be a larger undertaking and I'd rather not let that block merging at this point.
Ticket ENG-2222
Description Of Changes
Implements multi-page select-all and bulk action functionality for Okta monitors.
Code Changes
Steps to Confirm
Probably easiest to test using the new mocks, so run
npm run dev:mock(I ran into some strange issues trying to do this with Turbo, but maybe that's just my machine) to get a multi-page responsePre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works