fix(explore): explain disabled chart overwrite option#39796
Conversation
Code Review Agent Run #d3f2b5Actionable 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 AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39796 +/- ##
==========================================
+ Coverage 63.83% 64.38% +0.54%
==========================================
Files 2589 2573 -16
Lines 137821 135606 -2215
Branches 31928 31655 -273
==========================================
- Hits 87978 87306 -672
+ Misses 48327 46803 -1524
+ Partials 1516 1497 -19
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:
|
|
Hey @yeaight7, this is a nice UX improvement 👍 The message makes sense and definitely helps explain why the overwrite option is disabled. One small thought — do you think it would help to also hint at the alternative (like saving as a new chart)? Might make it a bit more actionable for users. Also just checking — is this string covered by translation like the rest of the UI? Overall looks clean and well scoped. Nice work! |
yeaight7
force-pushed
the
yeaight7/explore-save-owner-message
branch
from
3f2dcc4 to
2cbc37c
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #d6a694Actionable 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 AI Code Review powered by |
|
hey @Mayankaggarwal8055, forgot to reply. Yes, everything is covered by translation. I agree with your suggestion, that's why I added the hint to "Save as new chart". Thanks for the feedback |
sfirke
left a comment
sfirke
left a comment
There was a problem hiding this comment.
Thanks for this PR @yeaight7 !
One meaningful gap to address before merging: isChartOwner and canOverwriteSlice are not equivalent, and the PR gates the message on only one of the two conditions that can disable the button.
canOverwriteSlice() returns false in two distinct situations:
- The user is not an owner of the chart
- The chart has
is_managed_externally: true(managed by an external tool like a git-sync pipeline)
The PR introduces isChartOwner — which only checks condition 1 — and uses that to decide whether to show the message. This means: if a user is an owner but the chart is externally managed, the overwrite button is still disabled but no message appears. The user sees a disabled control with no explanation.
The fix is straightforward — replace isExistingChartNonOwner (and the redundant isChartOwner variable) with a condition based on canOverwriteSlice itself, and distinguish the two cases in the message text. This matches the pattern already used in PropertiesModal for the same flag:
{this.props.slice && !canOverwriteSlice && (
<div>
<Typography.Text type="secondary">
{this.props.slice.is_managed_externally
? t("This chart is managed externally and can't be overwritten in Superset.")
: t('Must be a chart owner to overwrite this chart. Save as a new chart instead.')}
</Typography.Text>
</div>
)}A test for the externally-managed case should be added alongside the existing non-owner test.
Minor style note: "Save as a new chart instead." may be unnecessary since the radio button is right there — existing hint text in the codebase tends toward single-sentence messages.
|
Addressed in the next update: the helper text now uses Validation: |
yeaight7
force-pushed
the
yeaight7/explore-save-owner-message
branch
from
2fee52d to
7e196be
Compare
Code Review Agent Run #556f4fActionable 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 AI Code Review powered by |
|
Noting that HoltzTomas#33 is also open... from a bot (but not targeting this repo)! Anyone care to assess how they stack up? |
|
@yeaight7 in the original issue I wrote:
Is that something you're able to add? |
|
Nice, the updated logic looks much cleaner now, especially handling both the ownership and externally-managed cases separately. The additional test coverage was a good catch too 👍 |
|
Took a look at the Devin PR (not on our repo) — this one is the better fix and worth pushing over the finish line.... I think it's close enough. The key difference is that the other one gates the message on a separate The one open thread is the style note about "Save as a new chart instead." being potentially redundant given the radio button is right there. That seems like a quick editorial call to unblock this. Otherwise the logic looks solid and the test coverage is good. Approving, and (unless there are ninja-edits to be made) will merge imminently. |
|
Meh... I'm not worried about the redundant messaging, on second thought. Easy follow-up PR if people want to wordsmith... this seems to be the right fix. In it goes! |
|
Happy to help 🙌 |
4 participants
SUMMARY
Adds explanatory helper text under the disabled Explore save modal overwrite option when an existing chart cannot be overwritten. The message now distinguishes charts the user does not own from charts managed externally.
Fixes #39786
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not included. Cosmetic text-only change in the existing Save chart modal.
TESTING INSTRUCTIONS
is_managed_externally: true.Automated/local validation run:
npx jest --config jest.config.js --maxWorkers=80% --silent --testRegex ".*SaveModal\\.test\\.tsx$"(passes: 23 tests)npx prettier --check src/explore/components/SaveModal.tsx src/explore/components/SaveModal.test.tsx(passes)pre-commit run --all-files(passes)ADDITIONAL INFORMATION