Repository navigation
feat(themes): add per-theme editors (Subject model) - #42404
Merged
Merged
Conversation
yousoph
requested review from
betodealmeida,
eschutho,
michael-s-molina,
mistercrunch,
rusackas and
sadpandajoe
as code owners
July 24, 2026 23:30
dosubot
Bot
added
api
github-actions
Bot
added
risk:db-migration
Contributor
|
Bito Review Skipped - Source Branch Not Found |
yousoph
marked this pull request as draft
July 24, 2026 23:36
sadpandajoe
reviewed
Jul 28, 2026
Contributor
|
The migration UPDATING.md |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42404 +/- ##
==========================================
+ Coverage 80.17% 80.18% +0.01%
==========================================
Files 2925 2926 +1
Lines 172670 172808 +138
Branches 40087 40131 +44
==========================================
+ Hits 138440 138570 +130
+ Misses 31633 31628 -5
- Partials 2597 2610 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
yousoph
added
the
🎪 ⚡ showtime-trigger-start
github-actions
Bot
added
🎪 30ea099 🚦 building
and removed
🎪 ⚡ showtime-trigger-start
Backfill each non-system theme's creator as an editor so existing theme authors keep edit access after upgrade (empty editors = admin-only would otherwise regress non-admin authors). System themes and null-creator themes are skipped. Also fix a ThemeModal save-payload type, a mypy Optional return in the integration test, and update the theme API column-set and update-command unit tests for the new editors field / editorship check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-point down_revision to the current single migration head so the branch does not introduce a second alembic head after syncing with master.
…format Add admin login to the command-level theme-deletion integration tests now that delete enforces editorship; update the migration revision-chain test for the rebased down_revision; ruff-format the migration.
Mirror CreateThemeCommand and the dashboard/chart/dataset importers so a non-admin who imports a new theme can still edit/delete it (the new editorship checks require editorship, which import previously never set).
Editor add/remove now marks the modal dirty, so closing without saving triggers the discard confirmation instead of silently dropping editor changes (previously only theme_name/json_data were compared).
POST /api/v1/theme/ built the Theme row directly instead of going through CreateThemeCommand, so the creator was never added as an editor. That left the assertion in test_create_theme_adds_creator_as_editor failing before the test's own cleanup ran, which in turn left the theme_writer fixture's user and role dangling and broke every sibling test in the class (FK violation deleting the user, then duplicate ab_user_role rows on subsequent setups). Also fixes the list/show columns (missing "editors"), the related/editors endpoint (missing allowed_rel_fields/filters wiring), PUT silently dropping the "editors" payload, and PUT/DELETE not translating ThemeForbiddenError into a 403. Fixes a pre-existing unit test that only worked by accident because a Mock's unconfigured .editors attribute happened not to be exercised until CreateThemeCommand was wired up correctly.
Master gained migration 39097d124752 (add_reason_to_purge_audit_log) with the same down_revision this branch's migration was cut from, producing two Alembic heads. Re-point down_revision to the new head so the branch merges into a single linear chain. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The migration was rebased onto 39097d124752 in a prior commit, but the test's revision-chain assertion still expected the old down_revision, failing unit-tests CI on every run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ose extra_editors The MCP create_theme tool created themes directly via ThemeDAO, bypassing CreateThemeCommand's editor-seeding step, so themes created through MCP left the creating user without edit access afterward. Route it through the same command the REST API uses. Also surface extra_editors on the theme GET/GET-list responses (matching dashboards/charts) and return 403 instead of an unhandled error when a non-editor attempts a bulk delete. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…in RBAC test The create_theme tool no longer imports db directly since it now routes through CreateThemeCommand, so the RBAC control test's patch.object(create_theme_module.db.session, "commit") broke with AttributeError. Patch superset.db.session.commit directly instead, matching the convention used by other command-based MCP tool tests.
The API already attached extra_editors (editorship granted via EXTRA_EDITORS_RESOLVER) to theme GET responses, matching charts/dashboards, but ThemeModal only checked the persisted editors list when deciding read-only state. A user granted editorship solely through the resolver could update the theme via the API yet was shown a read-only modal. Also adds regression coverage confirming the MCP create_theme tool persists through CreateThemeCommand (its editor-seeding step) rather than bypassing it, matching the REST create path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ThemeModal already factors extra_editors (editorship granted via EXTRA_EDITORS_RESOLVER) into its read-only check, and the API already attaches extra_editors to each row in the theme list response, but the list-row action still checked only the persisted editors list. A user granted editorship solely through the resolver could update the theme via the API yet saw a read-only 'View' action. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A migration landed on master with the same down_revision this branch's add_theme_editors_table migration was cut from, producing two Alembic heads. Add a no-op merge revision joining them.
…ns note Restore .github/workflows to the merge-base state so the PR no longer rolls back action pins, the ubuntu-slim runners, and the liccheck pkg_resources shim (the cause of the python-dependency-liccheck failure). Reword the UPDATING.md entry: themes were never admin-only to edit, since Theme is in GAMMA_READ_ONLY_MODEL_VIEWS and Alpha holds can_write. The change this PR introduces is tightening update/delete to editors or Admins. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The related/editors endpoint used the base related-field serialization, returning the raw Subject<...> str() with no label, type, or active fields. Add the same text_field_rel_fields, extra_fields_rel_fields, and order_rel_fields config used by the dashboard/chart editors endpoints so the theme editor picker gets proper labels/icons and can filter out inactive subjects. Adds a response-shape test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A force-push to this branch dropped the merge migration that had previously joined the task_dependencies and theme_editors Alembic heads, leaving two divergent heads again. Add a fresh no-op merge revision joining them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The Theme REST API gained an `editors` field on post/put plus a 403 response, but the committed spec was never regenerated to match.
Contributor
|
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
…editorship The ThemeList Delete row action was gated purely on the blanket Theme:can_write permission, so a non-editor saw an enabled Delete icon for themes they could not actually delete (the server-side raise_for_editorship check correctly blocked the request, but the UI was misleading). Similarly, the Edit row action's edit/view toggle didn't account for the admin-only restriction on the active system default/dark theme slot, so a non-admin editor saw an editable icon for a theme they could not actually save. Both actions now share a single per-row guard that mirrors the server's UpdateThemeCommand/DeleteThemeCommand checks: editorship (or admin) for regular themes, admin-only for the active system default/dark theme. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Code Review Agent Run #56fd90Actionable Suggestions - 0Additional Suggestions - 14
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
sadpandajoe
reviewed
Sep 11, 2026
Resolves a UPDATING.md ordering conflict by keeping both this branch's and master's changelog entries. Verified the theme_editors migration chain still resolves to a single Alembic head (0884f655c0a6) after the merge.
…admins ThemeModal only checked the blanket is_system flag to decide read-only state, so a non-admin editor of a regular theme that had been promoted to the active system-default/dark slot saw Save enabled and got a 403 from UpdateThemeCommand.validate() on submit, which enforces admin-only on is_system_default/is_system_dark independently of is_system. Mirror that guard in the modal and adjust the read-only notice text so it doesn't claim the user isn't an editor when they are one.
Member
|
Re: the "Additional Suggestions" from the 2026-09-11 review run (0 actionable, 14 additional) — the modal/list gating mismatch is now fixed (same defect independently flagged by a human reviewer, see the
Not blocking merge on these three; happy to file follow-up issues if maintainers want them tracked separately. |
rusackas
approved these changes
Sep 16, 2026
Member
|
Your call on fix here vs merge/follow-up @sadpandajoe - looks good enough to me, but happy to push some commits, or open a follow-up PR ¯\_(ツ)_/¯ |
Member
|
Merging this, thanks for the thorough writeup @yousoph, and for the careful triage on those three @sadpandajoe. Opening a follow-up PR for them now rather than letting them go untracked. |
4 of 9 tasks
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
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.
SUMMARY
Extends the Subject model / entity editors introduced in #38831 to Themes. Themes previously had no ownership concept and were editable only by admins. This PR adds a per-theme
editorslist (Subjects) so edit access can be scoped per theme instead of all-or-nothing, following the same editors-only pattern already used by alerts/reports (no viewers for themes).Key behaviors:
theme_editorsjunction table and aTheme.editorsrelationship (mirrorsreport_schedule_editors).CreateThemeCommand; the creating user is automatically added as an editor.PUT/DELETE/bulk-delete now enforceraise_for_editorshipon the theme (admins always pass). System themes and the system default/dark themes keep their existing admin-only protections.editors(create/update schemas +list/showcolumns) and a/api/v1/theme/related/editorsendpoint, with a new optionalSUBJECTS_RELATED_TYPES_THEMESconfig to control which subject types appear in the picker (falls back to the global default).Backwards compatibility: no feature flag. On upgrade the migration backfills each existing non-system theme's creator as an editor so authors keep edit access; system themes and themes with no creator receive no editors and stay admin-only to edit (an empty editors list means admin-only). Theme visibility is deliberately unchanged: the list endpoint has no new access filter, so every user who can read themes today still can, and themes applied to a dashboard continue to render for anyone who can view that dashboard.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Themes list gains an "Editors" column; the theme modal gains an "Editors" picker.
TESTING INSTRUCTIONS
superset db upgradecreates thetheme_editorstable (migrationf7e8d9c0b1a2).can_writeonTheme: create a theme → you are automatically its editor and can edit it; you cannot edit a theme you are not an editor of (403); you cannot add yourself as editor viaPUTto gain access.tests/integration_tests/themes/test_theme_editors.py,tests/unit_tests/migrations/test_add_theme_editors.py, plusThemeList/ThemeModaljest tests.ADDITIONAL INFORMATION
Documents the behavior change in
UPDATING.md. Builds on #38831.🤖 Generated with Claude Code