Skip to content

Conversation

@mgorsk1
Copy link

@mgorsk1 mgorsk1 commented Dec 19, 2022

SUMMARY

Disables saving artifacts to JSON when canDownloadCSV is not allowed. This is required to prevent leaking data from superset. can csv on Superset is a permission aiming to prevent downloading structured data from Superset but it's not covering JSON exports. This PR aims to fix this.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: Not possible to disable export to json on charts #19535
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although I think the linter may complain about the long line

@villebro
Copy link
Member

@mgorsk1 can you check if you were actually able to download the JSON data? I assume you should get an error if you don't have the right perms, and if you're not, we need to also fix this in the API.

@mgorsk1
Copy link
Author

mgorsk1 commented Dec 20, 2022

hey villebro, on a second thought I think that downloading JSON itself might be always valuable and we shall just strip it of data section when canDownloadCSV is false. wdyt?

@mgorsk1
Copy link
Author

mgorsk1 commented Jan 10, 2023

bump @villebro

@villebro
Copy link
Member

Hi @mgorsk1 - CI is failing due to a linting issue:
image

@rusackas
Copy link
Member

@mgorsk1 feel free to hit us up on Slack if we can help with linting. It also appears there are some conflicts to mitigate, hopefully that's straightforward-ish :) Would love to see #19535 closed. @Larissa-Rocha might also want to take a look at this PR on that note.

@mgorsk1 mgorsk1 force-pushed the fix/disable_json_download branch from 26ae6fe to c9eebfb Compare February 3, 2023 13:56
@mgorsk1
Copy link
Author

mgorsk1 commented Feb 3, 2023

fixed linting (I hope), feel free to re-run the CI @rusackas

@rusackas
Copy link
Member

rusackas commented Feb 6, 2023

Resolved the conflict... hopefully CI passes ;)

@rusackas
Copy link
Member

Ugh... I can't reboot the unit tests (which were probably just flaky) for some reason, so I'm going to close/reopen this to kick-start CI (again)

@rusackas rusackas closed this Apr 27, 2023
@rusackas rusackas reopened this Apr 27, 2023
@rusackas
Copy link
Member

rusackas commented May 8, 2023

Unit tests seem to be failing pretty reliably... can you run/address the tests locally? I also believe that a unit tests was fixed on master not long ago, so you might get lucky with a rebase.

@rusackas
Copy link
Member

@mgorsk1 just checking to see if you have any interest in pursuing this still.

@rusackas
Copy link
Member

rusackas commented Nov 6, 2023

Closing and re-opening to see if anything magically changes, but unless @mgorsk1 wants to check/fix unit tests, we might wind up closing this one for good.

@rusackas rusackas closed this Nov 6, 2023
@rusackas rusackas reopened this Nov 6, 2023
@mgorsk1 mgorsk1 force-pushed the fix/disable_json_download branch from 294ab90 to 478699f Compare November 7, 2023 10:38
@mgorsk1
Copy link
Author

mgorsk1 commented Nov 7, 2023

rebased @rusackas

@rusackas
Copy link
Member

rusackas commented Feb 5, 2024

Closing/reopening to kick CI.

@rusackas rusackas closed this Feb 5, 2024
@rusackas rusackas reopened this Feb 5, 2024
@rusackas
Copy link
Member

Tests still seem to be failing on this. Any chance you can run them locally and see if you can get them to pass? I wonder if it's because the UI changed since the PR was opened.

@michael-s-molina
Copy link
Member

Ci is failing with:

SyntaxError: /home/runner/work/superset/superset/superset-frontend/src/types/dom-to-pdf.d.ts: A 'const' initializer in an ambient context must be a string or numeric literal or literal enum reference.

#27236 was merged recently, so I think this PR just needs a rebase and it will be good to go.

@rusackas
Copy link
Member

rusackas commented May 3, 2024

@mgorsk1 would you mind rebasing this so we can merge it?

@rusackas
Copy link
Member

rusackas commented May 9, 2024

Still in need of a rebase here, I think, but I'll close/reopen again in a bit of a "hail mary" attempt.

@rusackas rusackas closed this May 9, 2024
@rusackas rusackas reopened this May 9, 2024
@rusackas
Copy link
Member

Looks like this still needs a rebase to pass CI. It's been a long while since it was touched, so I'll convert it to Draft. We may circle back and close it in time if it doesn't get any touch-ups. Please mark it as ready for review if/when it's ready, and we'll happily merge it.

@rusackas rusackas marked this pull request as draft March 20, 2025 22:33
@codecov
Copy link

codecov bot commented Mar 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.00%. Comparing base (d6fde3c) to head (478699f).
Report is 2959 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #22454   +/-   ##
=======================================
  Coverage   69.00%   69.00%           
=======================================
  Files        1938     1938           
  Lines       75831    75831           
  Branches     8427     8427           
=======================================
  Hits        52328    52328           
  Misses      21334    21334           
  Partials     2169     2169           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sadpandajoe
Copy link
Member

Closed in favor of #28429

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants