feat(rls-frontend): Add new buttons to select/deselect all datasets/roles in RLS#32996
feat(rls-frontend): Add new buttons to select/deselect all datasets/roles in RLS#32996bmaquet wants to merge 2 commits into
Conversation
dosubot
Bot
added
the
authentication:row-level-security
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Complex Inline Event Handler ▹ view | 🧠 Not in scope |
 |
Missing error handling in API request ▹ view | 🧠 Not in standard |
 |
Redundant API Call for Select All ▹ view | 🧠 Not in scope |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/features/rls/RowLevelSecurityModal.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Feedback and Support
| SupersetClient.get({ | ||
| endpoint: `/api/v1/rowlevelsecurity/related/${entityType}?q=${query}`, | ||
| }).then(response => { | ||
| const allEntities = response.json.result.map( | ||
| (item: { value: number; text: string }) => ({ | ||
| key: item.value, | ||
| label: item.text, | ||
| value: item.value, | ||
| }), | ||
| ); | ||
| updateRuleState(entityType, allEntities || []); | ||
| }); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| SupersetClient.get({ | ||
| endpoint: `/api/v1/rowlevelsecurity/related/${entityType}?q=${query}`, | ||
| }).then(response => { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const createDropdownRender = useCallback( | ||
| (entityType: 'tables' | 'roles') => (menu: React.ReactNode) => ( | ||
| <> | ||
| <StyledDropdownContainer> | ||
| <StyledDropdownButton | ||
| type="button" | ||
| onClick={() => { | ||
| const query = rison.encode({ | ||
| page: 0, | ||
| page_size: -1, | ||
| }); | ||
| SupersetClient.get({ | ||
| endpoint: `/api/v1/rowlevelsecurity/related/${entityType}?q=${query}`, | ||
| }).then(response => { | ||
| const allEntities = response.json.result.map( | ||
| (item: { value: number; text: string }) => ({ | ||
| key: item.value, | ||
| label: item.text, | ||
| value: item.value, | ||
| }), | ||
| ); | ||
| updateRuleState(entityType, allEntities || []); | ||
| }); | ||
| }} | ||
| > |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| margin-top: ${({ theme }) => theme.gridUnit}px; | ||
| `; | ||
|
|
||
| const StyledDropdownContainer = styled.div` |
There was a problem hiding this comment.
This PR adds a lot of styled elements. Do you think that in support of Theming efforts underway, we could either:
a) Just go with "vanilla" AntD components and minimize style overrides
b) Move these styles into the root component behind a new prop so that they're reusable rather than bespoke for this one instance?
There was a problem hiding this comment.
Thanks for the comment @rusackas !
I wasn't aware of the new theming efforts. I can definitely do either of these. Which one would you suggest?
There was a problem hiding this comment.
"Vanilla" AntD would look like this (default type)
|
@michael-s-molina @geido actually, didn't we have a prop for the Select component to add a "Select All" option, rather than adding these buttons? |
Yes, it's the allowSelectAll which is available only to the sync select. Implementing Select All for asynchronous selects has many challenges as you can see in #20143. |
Thanks for the context about challenges with AsyncSelect @michael-s-molina What would be the suggested design decision here then? Go ahead with custom buttons for this use case without implementing Select All for the AsyncSelect component, or close the PR entirely and come back to it if/when the AsyncSelect challenges can be overcome? |
|
Oh! about theming, there's been a fair amount of work on the Would recommend either re-opening the PR against
Sorry about the confusion - but let's try to avoid merge conflicts. As far as ETA I think we're almost there for merging the theming / antd-v5 branch, waiting for v5.0 to be officially voted through to merge into master |
Thanks for the extra context. I'll close this PR and wait until the theming PR is merged to revisit. |
|
[... getting back to try and get that big feature branch merged so it's easier to accept this type of contribution ...] |
4 participants
SUMMARY
This PR introduces "Select All" and "Deselect All" buttons to the Table & Roles elements of the Row Level Security (RLS) configuration interface. This enhancement aims to improve user experience, particularly when managing a large number of tables or roles, by allowing bulk selection and deselection instead of requiring individual clicks for each item. The implementation adds two distinct buttons to trigger these actions, updating the selection state accordingly.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE
AFTER
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
Fixes #123)