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: dashboard page xlsx export #24005

Merged
merged 6 commits into from
May 19, 2023

Conversation

vitoldi
Copy link
Contributor

@vitoldi vitoldi commented May 10, 2023

SUMMARY

The Superset 2.1.0 release has new feature: excel export. But the feature is only available on the Chart page. Our customers have no access to the Chart page, so the Dashboard page is a primary place where we wanted Excel Export option.

This PR adds the ability to export to Excel on the Dashboard page.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
before

After:
after

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

Resolves #23711

@EugeneTorap
Copy link
Contributor

EugeneTorap commented May 10, 2023

Hello @vitoldi!
Can you use Excel instead of .XLSX in Export to .XLSX ?
Can you add #23711 issue to the description and write: Resolves #23711 in order to automatically close the issue.
cc: @villebro @dpgaspar

@vitoldi
Copy link
Contributor Author

vitoldi commented May 10, 2023

Hello @vitoldi! Can you use Excel instead of .XLSX in Export to .XLSX ? Can you add #23711 issue to the description and write: Resolves #23711 in order to automatically close the issue. cc: @villebro @dpgaspar

Hello @EugeneTorap, it's done.

@EugeneTorap
Copy link
Contributor

Hi @vitoldi, can you merge master branch -> your branch in order to fix CI tests

@EugeneTorap
Copy link
Contributor

@vitoldi, pls, fix the prettier error

prettier.................................................................Failed
- hook id: prettier
- files were modified by this hook

superset-frontend/src/dashboard/components/gridComponents/Chart.jsx

@vitoldi
Copy link
Contributor Author

vitoldi commented May 11, 2023

@EugeneTorap, prettier error fixed.

1

2

renderWrapper(props);
expect(props.exportXLSX).toBeCalledTimes(0);
userEvent.hover(screen.getByText('Download'));
userEvent.click(await screen.findByText('Export to .XLSX'));
Copy link
Contributor

Choose a reason for hiding this comment

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

@vitoldi pls, fix the unit test. Use Excel instead of .XLSX

Error: Unable to find an element with the text: Export to .XLSX.
This could be because the text is broken up by multiple elements.
In this case, you can provide a function for your text matcher to make your matcher more flexible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1

@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Merging #24005 (444cac4) into master (6b54591) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 444cac4 differs from pull request most recent head 67945ae. Consider uploading reports for the commit 67945ae to get more accurate results

@@           Coverage Diff           @@
##           master   #24005   +/-   ##
=======================================
  Coverage   68.27%   68.28%           
=======================================
  Files        1952     1952           
  Lines       75468    75476    +8     
  Branches     8202     8203    +1     
=======================================
+ Hits        51529    51537    +8     
  Misses      21832    21832           
  Partials     2107     2107           
Flag Coverage Δ
javascript 54.68% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...end/src/dashboard/components/SliceHeader/index.tsx 90.74% <100.00%> (+0.17%) ⬆️
...dashboard/components/SliceHeaderControls/index.tsx 69.79% <100.00%> (+0.64%) ⬆️
.../src/dashboard/components/gridComponents/Chart.jsx 57.52% <100.00%> (+1.55%) ⬆️
superset-frontend/src/logger/LogUtils.ts 97.22% <100.00%> (+0.07%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@EugeneTorap
Copy link
Contributor

@villebro @dpgaspar @michael-s-molina Can you review this PR?

@rusackas
Copy link
Member

@villebro @dpgaspar @michael-s-molina Can you review this PR?

Officially requested these reviewers.

}

exportXLSX() {
this.exportTable('xlsx', false);
Copy link
Member

Choose a reason for hiding this comment

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

I think this is totally fine for now, but just wondering if folks will also want to do a "Full Excel" export like we do with CSV (when that feature flag is enabled). I'm totally fine skipping it for now, since it might cause performance issues, but it just made me wonder ¯\_(ツ)_/¯

Copy link
Member

@rusackas rusackas May 15, 2023

Choose a reason for hiding this comment

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

While I'm on the topic, I wonder if the logger should also discern between regular and "full" CSV exports, in case anyone is trying to discern performance impact. Maybe an additional log for full CSV exports?

Again, not the point of this PR, but mentioning the bycatch...

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

Code LGTM, but haven't tested it yet. Couple of comments about bycatch that might be worth exploring in a subsequent PR.

@rusackas
Copy link
Member

@vitoldi I think we can merge this, but now there's a conflict. Would you mind doing a quick rebase?

@vitoldi
Copy link
Contributor Author

vitoldi commented May 19, 2023

@rusackas, I have resolved the conflict.

1 (1)

2

1

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

@dpgaspar dpgaspar merged commit d0687d0 into apache:master May 19, 2023
26 checks passed
@vitoldi vitoldi deleted the feat/dashboard-page-xlsx-export branch May 19, 2023 12:17
@vitoldi vitoldi restored the feat/dashboard-page-xlsx-export branch May 30, 2023 14:05
@vitoldi vitoldi deleted the feat/dashboard-page-xlsx-export branch June 5, 2023 07:54
@sebastianliebscher
Copy link
Contributor

This feature is released with version 3.0.0.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.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 size/M 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Excel export option missing in dashboard
7 participants