refactor(dashboard): rename supersetCanCSV to supersetCanDownload (#2…#39118
refactor(dashboard): rename supersetCanCSV to supersetCanDownload (#2…#39118Pawansingh3889 wants to merge 2 commits intoapache:masterfrom
Conversation
| roles, | ||
| ), | ||
| superset_can_csv: findPermission('can_csv', 'Superset', roles), | ||
| superset_can_download: findPermission('can_csv', 'Superset', roles), |
There was a problem hiding this comment.
Suggestion: The new superset_can_download field still checks only can_csv, which breaks permission behavior when granular export controls are enabled. In that mode, download access is driven by can_export_data (as used elsewhere), so users with the new export permission but without legacy CSV permission will be incorrectly blocked on dashboards. [logic error]
Severity Level: Major ⚠️
- ❌ Dashboard header Download menu hidden for permitted users.
- ⚠️ Results pane disables CSV/XLSX export in dashboards.
- ⚠️ Inconsistent export behavior: dashboard header vs chart context menu.| superset_can_download: findPermission('can_csv', 'Superset', roles), | |
| superset_can_download: window.featureFlags?.GRANULAR_EXPORT_CONTROLS | |
| ? findPermission('can_export_data', 'Superset', roles) | |
| : findPermission('can_csv', 'Superset', roles), |
Steps of Reproduction ✅
1. Enable granular export controls in the backend by setting `GRANULAR_EXPORT_CONTROLS` to
true in `superset/config.py` (see definition at `superset/config.py:570-573`) and restart
Superset so the feature flag propagates to the frontend.
2. Configure a user role that has `("can_export_data", "Superset")` but not `("can_csv",
"Superset")` (permissions are created in `superset/security/manager.py:1231-1234` and
migration `add_granular_export_permissions.py:42-52` adds the granular ones).
3. Log in as that user and open any dashboard; `DashboardPage.tsx` dispatches
`hydrateDashboard` on load
(`superset-frontend/src/dashboard/containers/DashboardPage.tsx:33` import and `203-210`
call), which sets `dashboardInfo.superset_can_download` from `findPermission('can_csv',
'Superset', roles)` in `hydrate.ts:356`. Because the role lacks `can_csv`,
`superset_can_download` is false in Redux state.
4. On the same dashboard, open the chart header menu for a slice: `Chart.tsx` reads
`superset_can_download` from `dashboardInfo`
(`superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:87-90`),
passes it through `SliceHeader` (`SliceHeader/index.tsx:364`) into `SliceHeaderControls`,
which only renders the Download submenu and CSV/XLSX options when
`props.supersetCanDownload` is truthy (`SliceHeaderControls/index.tsx:502-76`). Observe
that no Download menu is shown, even though backend export checks now rely on
`can_export_data` when `GRANULAR_EXPORT_CONTROLS` is enabled
(`superset/views/core.py:321-325` and `superset/charts/data/api.py:409-414`), so the user
is actually authorized to export.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/actions/hydrate.ts
**Line:** 356:356
**Comment:**
*Logic Error: The new `superset_can_download` field still checks only `can_csv`, which breaks permission behavior when granular export controls are enabled. In that mode, download access is driven by `can_export_data` (as used elsewhere), so users with the new export permission but without legacy CSV permission will be incorrectly blocked on dashboards.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
Code Review Agent Run #9f1086
Actionable Suggestions - 1
-
superset-frontend/src/dashboard/actions/hydrate.ts - 1
- Inconsistent Permission Check · Line 356-356
Review Details
-
Files reviewed - 8 · Commit Range:
c98d255..c98d255- superset-frontend/src/dashboard/actions/hydrate.ts
- superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx
- superset-frontend/src/dashboard/components/SliceHeader/index.tsx
- superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx
- superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
- superset-frontend/src/dashboard/components/SliceHeaderControls/types.ts
- superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.tsx
- superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx
-
Files skipped - 0
-
Tools
- 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
| roles, | ||
| ), | ||
| superset_can_csv: findPermission('can_csv', 'Superset', roles), | ||
| superset_can_download: findPermission('can_csv', 'Superset', roles), |
There was a problem hiding this comment.
The superset_can_download permission check still uses the deprecated 'can_csv' permission instead of the new granular 'can_export_data' when the feature flag is enabled. This creates inconsistency with explore permissions and may cause access issues once 'can_csv' is removed in v3.0.0. Update to match the explore logic: use 'can_export_data' if GranularExportControls is enabled, else 'can_csv'.
Code suggestion
Check the AI-generated fix before applying
- import { DataMaskStateWithId, JsonObject } from '@superset-ui/core';
+ import { DataMaskStateWithId, JsonObject, isFeatureEnabled, FeatureFlag } from '@superset-ui/core';
@@ -356,1 +356,4 @@
- superset_can_download: findPermission('can_csv', 'Superset', roles),
+ superset_can_download: isFeatureEnabled(FeatureFlag.GranularExportControls)
+ ? findPermission('can_export_data', 'Superset', roles)
+ : findPermission('can_csv', 'Superset', roles),
Code Review Run #9f1086
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
jaymasiwal
left a comment
There was a problem hiding this comment.
Great initiative on the rename! However, as the automated checks flagged below in hydrate.ts, leaving the permission check hardcoded to 'can_csv' introduces a regression for workspaces using Granular Export Controls.
Please apply the bot's suggested fix to use 'can_export_data' when the feature flag is enabled. Once that's updated, I'll be happy to approve this!
|
Fixed — now checks can_export_data when GranularExportControls is enabled, falls back to can_csv otherwise. |
Code Review Agent Run #ffa816Actionable Suggestions - 0Review 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 |
|
Thanks for adding the GranularExportControls check! The permission logic looks perfect now. LGTM! |
|
Adding a Before this PR (current master): After this PR: The breaking scenario:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39118 +/- ##
==========================================
- Coverage 64.52% 64.52% -0.01%
==========================================
Files 2536 2536
Lines 131208 131210 +2
Branches 30457 30459 +2
==========================================
Hits 84661 84661
- Misses 45084 45086 +2
Partials 1463 1463
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
SUMMARY
Closes #24290. Rename supersetCanCSV → supersetCanDownload across frontend (props, state, tests). Backend can_csv permission unchanged.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No visual changes — rename only.
TESTING INSTRUCTIONS
Run cd superset-frontend && npm run test -- --testPathPattern="SliceHeader|Chart"
ADDITIONAL INFORMATION
Has associated issue: #24290
Required feature flags:
Changes UI
Includes DB Migration
Removes existing feature or API