Skip to content
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

feat(chart-data-api): download multiple csvs as zip #18618

Merged
merged 3 commits into from
Feb 8, 2022

Conversation

villebro
Copy link
Member

@villebro villebro commented Feb 8, 2022

SUMMARY

For charts that request more than one query, only the first result can be downloaded as CSV. This PR does the following:

  • When downloading a CSV, return a CSV file if there is only one result (current behavior), but return all results bundled in a ZIP file if there are more than one results. Also add a unit test to ensure that requesting a multi-query QueryContext returns a zip with the expected contents
  • If user tries to request CSV for a QueryContext with an empty queries property, return a 400. Also add a unit test for this case.
  • Add assertion for the mimetype to the pre-existing single result CSV integration test
  • Add assertion for the mimetype to the pre-existing JSON download integration test

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

  1. Create a Mixed Timeseries chart
  2. Click the "Export to .CSV format" button
  3. Verify that the downloaded ZIP file includes two CSVs with the correct contents

ADDITIONAL INFORMATION

  • Has associated issue: closes exported csv file from Mixed Time-Series chart only contains one series #17960
  • 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

@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #18618 (f838c5f) into master (4e2bdd4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head f838c5f differs from pull request most recent head 9246e81. Consider uploading reports for the commit 9246e81 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #18618   +/-   ##
=======================================
  Coverage   66.30%   66.31%           
=======================================
  Files        1595     1595           
  Lines       62632    62647   +15     
  Branches     6309     6309           
=======================================
+ Hits        41529    41545   +16     
+ Misses      19453    19452    -1     
  Partials     1650     1650           
Flag Coverage Δ
hive 52.14% <100.00%> (+0.07%) ⬆️
mysql 81.25% <100.00%> (+0.01%) ⬆️
postgres 81.30% <100.00%> (+0.01%) ⬆️
presto 51.99% <100.00%> (+0.07%) ⬆️
python 81.74% <100.00%> (+0.01%) ⬆️
sqlite 81.00% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/charts/data/api.py 89.80% <100.00%> (+0.99%) ⬆️
superset/utils/core.py 89.77% <100.00%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e2bdd4...9246e81. Read the comment docs.

@geido
Copy link
Member

geido commented Feb 8, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2022

@geido Ephemeral environment spinning up at http://52.12.219.152:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

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

LGTM!

@kgabryje
Copy link
Member

kgabryje commented Feb 8, 2022

When I use only 1 metric for a query, metric's name is not present in the csv
image

However, when I use more than 1 metric, I can see the metric names.
image

Is that intended behaviour?

Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

Left a small comment, but since that behaviour wasn't introduced by this PR, LGTM!

@srinify
Copy link
Contributor

srinify commented Feb 8, 2022

Looks good, just tested! Approving :]

@srinify srinify merged commit 125be78 into apache:master Feb 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2022

Ephemeral environment shutdown and build artifacts deleted.

@villebro villebro deleted the villebro/multi-csv branch February 8, 2022 20:19
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/M 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exported csv file from Mixed Time-Series chart only contains one series
5 participants