chore: Fix/remove hardcode of admin role#27779
Conversation
|
@supersetbot orglabel |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #27779 +/- ##
==========================================
+ Coverage 69.71% 69.77% +0.05%
==========================================
Files 1920 1920
Lines 75234 75244 +10
Branches 8423 8425 +2
==========================================
+ Hits 52447 52498 +51
+ Misses 20725 20684 -41
Partials 2062 2062
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
How I can add orglabel?) |
rusackas
changed the title
@mistercrunch can probably help. Not sure if there's documentation for all this somewhere... we should probably add a page to the docs site or the wiki (probably the wiki, since that's where more process/dev content is going, while the docs are more end-user focused). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #27779 +/- ##
==========================================
+ Coverage 63.95% 64.36% +0.41%
==========================================
Files 2664 2681 +17
Lines 146217 148096 +1879
Branches 33731 33982 +251
==========================================
+ Hits 93512 95323 +1811
- Misses 50988 51008 +20
- Partials 1717 1765 +48
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:
|
|
@supersetbot unlabel TechAudit-BI |
|
@supersetbot orglabel |
|
The bot was down for some reason, revived it in recent PRs. More about the bot here -> https://github.com/apache-superset/supersetbot . It even has a logo now 🤖 🤖 🤖 |
|
Thank you @mistercrunch! |
|
@john-bodley Hi! Fixed code by your review. Please take a look! |
|
Rerunning the mysql test in hopes that it's just flaky, but if it fails again, the PR might need a rebase (there were some glitches with these DB tests that got resolved on master not long ago). |
|
@rusackas Test cases has beed passed without rebase |
|
The main thing I'm wondering about this PR now that I look more closely at the code, is if there's a better place to store this information. I'm a little nervous about storing bootstrap data on the window object, which opens up a new "junk drawer" pattern where people might store anything there. I'm not sure if we should have the Feature Flags there either, really. Is it possible to access this via the |
|
@rusackas I see, ok. I have pushed commit with the fix 👍, please take a look! |
|
I'll fix frontend errors soon |
|
Looks like this one just... never finished CI for some reason. Closing/reopening to kick start this now... let's see if we can get it merged! |
| // but is hardcoded in backend logic already, so... | ||
| const ADMIN_ROLE_NAME = 'admin'; | ||
| const bootstrapData = getBootstrapData(); | ||
| const ADMIN_ROLE_NAME = bootstrapData.common.conf.AUTH_ROLE_ADMIN || 'Admin'; |
There was a problem hiding this comment.
Mmmmh, seems ADMIN_ROLE_NAME != AUTH_ROLE_ADMIN. AUTH_ROLE_ADMIN probably exist do it can diverge from the global ADMIN_ROLE - assuming this exists. Maybe FAB exposes it somewhere? Would have to be carried to the frontend?
|
This is coming together nicely, @Always-prog. Pulling |
rusackas
force-pushed
the
fix/remove-hardcode-of-admin-role
branch
from
9c1ac86 to
95ef7cc
Compare
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #080cad
Actionable Suggestions - 1
-
superset-frontend/src/dashboard/util/permissionUtils.ts - 1
- Incomplete test coverage for configurable admin role · Line 29-30
Additional Suggestions - 1
-
superset/views/base.py - 1
-
Missing test for new config exposure · Line 76-76New config key `AUTH_ROLE_ADMIN` is exposed to frontend but lacks a corresponding unit test. Existing pattern at line 73-76 tests `DEFAULT_MAP_RENDERER` exposure. Per rule [11730], new features should include unit tests covering success paths and validation scenarios.
-
Review Details
-
Files reviewed - 3 · Commit Range:
b7e9497..95ef7cc- superset-frontend/src/dashboard/util/permissionUtils.ts
- superset/config.py
- superset/views/base.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| const bootstrapData = getBootstrapData(); | ||
| const ADMIN_ROLE_NAME = bootstrapData.common.conf.AUTH_ROLE_ADMIN || 'Admin'; |
There was a problem hiding this comment.
The module-level getBootstrapData() call is not mocked in tests, creating a testing gap for the new configurable ADMIN_ROLE_NAME functionality. Tests rely on DEFAULT_BOOTSTRAP_DATA which has conf: {}, so the custom AUTH_ROLE_ADMIN path is never exercised. Consider adding mock setup in beforeAll to control the admin role name and verify case-insensitive matching behavior for both default and custom values.
Code Review Run #080cad
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
5 participants
SUMMARY
Just removed hardcode of admin role from perm utils and using
AUTH_ROLE_ADMINconstant in the Superset config instead.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION