New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(explore): CSV Export Permission is incorrect on Explore page #13713
fix(explore): CSV Export Permission is incorrect on Explore page #13713
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13713 +/- ##
==========================================
+ Coverage 76.98% 76.99% +0.01%
==========================================
Files 955 955
Lines 48085 48095 +10
Branches 6036 6038 +2
==========================================
+ Hits 37020 37033 +13
+ Misses 10867 10864 -3
Partials 198 198
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up |
@junlincc Ephemeral environment spinning up at http://34.222.8.128:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, LGTM! Sorry for the delay and thanks for the fix. do you mind rebasing since it has been quite a while. 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix! You should probably add it here, too (it's the new chart data endpoint that will ultimately replace explore_json
):
superset/superset/common/query_context.py
Lines 152 to 160 in a3b41e2
def get_data(self, df: pd.DataFrame,) -> Union[str, List[Dict[str, Any]]]: | |
if self.result_format == ChartDataResultFormat.CSV: | |
include_index = not isinstance(df.index, pd.RangeIndex) | |
result = csv.df_to_escaped_csv( | |
df, index=include_index, **config["CSV_EXPORT"] | |
) | |
return result or "" | |
return df.to_dict(orient="records") |
d879566
to
6860027
Compare
5347655
to
bbb4767
Compare
bbb4767
to
b26cc92
Compare
@villebro PR is ready, please review it again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - stellar PR with both frontend and backend tests! 🚀 Big thanks!
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
TEST PLAN
{ "error": "You don't have the rights to download as csv" }
ADDITIONAL INFORMATION