rusackas commented on PR #39796: URL: https://github.com/apache/superset/pull/39796#issuecomment-4434233442
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 `isNonOwnerOfExistingSlice()` helper that only checks ownership, so it silently fails to explain the disabled button when `is_managed_externally` is `true`. This PR correctly uses `!canOverwriteSlice` — the same condition that disables the button — and shows a distinct message for each case. It's also a smaller, cleaner diff. 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
