Conversation
mihalikv
marked this pull request as ready for review
July 3, 2026 13:07
Contributor
Author
|
@codex review |
MartinCervenkaSB
left a comment
MartinCervenkaSB
left a comment
Collaborator
There was a problem hiding this comment.
Review of SBAdminPermissionWidget (permission tree widget, groups mode, JS search, tests).
Concerns
- Unbound/add forms preselect every permission in the queryset — a fail-open default that is also undocumented in the new AGENTS.md section.
- Re-rendering a bound form after a tampered POST (non-numeric IDs) raises an uncaught
ValueError→ 500. _standard_actionsplits the codename from the right, so standard permissions of models with underscores in their name are silently dropped from rendering.- Checkbox DOM ids are not prefixed with the widget id — two permission widgets on one page collide.
- Global
.tooltip, .popover { z-index: 1020 }puts overlays below Bootstrap modals (1055), which can hide this widget's own body-container tooltips inside modals. {{ form.media }}is now rendered for all modal forms — widget scripts re-execute on every modal open; other widgets' media may not be idempotent.
Nits
group_by_modelparameter of_build_auto_contextis never used withTrue(dead branch).- Duplicated empty
<label>for the section select-all toggle. - Section count is server-rendered as
0/Neven when selections exist (JS corrects it after init). - Duplicated
view_dashboardget_or_createblock in testsetUpTestData. - Two consecutive identical CSS selectors
.permission-tree__custom-row > input[type="checkbox"].toggle + labelcould be merged into one rule. sb_admin_widget = Trueis already inherited fromSBAdminBaseWidget.
Questions for the author
- Is select-all-by-default on add forms deliberate (e.g. for the Neoship account use case)? If so, please document it in AGENTS.md; if not, consider an opt-in flag.
- The section "select all" toggle also flips permissions currently hidden by an active search filter — intended?
- What motivated the global tooltip/popover z-index change to 1020 (below Bootstrap's modal at 1055)?
What looks good
- Single-query context building, locked in with an
assertNumQueries(1)test. - JS search highlighting escapes HTML before
innerHTML— no XSS through permission names. - Strict
app_label.model:codenamerefs with clearImproperlyConfigurederrors, and group-referenced permissions are deduplicated from the automatic leftover sections. - Diacritics-insensitive, astral-safe search index with correct offset mapping.
- PEP 562 lazy export in
widgets.pyavoids the import cycle cleanly. - Comprehensive test suite covering both modes, indeterminate state, and misconfiguration errors.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4809f636ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Simplify permission tree handling by removing indeterminate states, improving search functionality, and refactoring selection logic. Update related tests and documentation.
…cted permission counts
…mission-tree.css`
…rmission tree component This new CSS file consolidates and organizes styles for the permission tree, enhancing the UI structure and ensuring consistent styling across various elements within the component.
Contributor
Author
|
@codex review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46515b99ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… handling Added a detailed explanation regarding the all-or-nothing behavior of group options in the permission system, including guidance on managing existing groups and users during updates to permission sets.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.