feat(reports): xlsx attachments

[cemremengu]

SUMMARY

Most people and departments in enterprise companies are addicted to spreadsheets. Currently our customers are heavily demanding Excel reports and, unfortunately, we have failed to persuade them to continue with CSV.

So here I am with a PR that adds the ability to attach XLSX files as report data and notifications (slack). If this one gets approved, I will also attempt to implement PDF in a similar way.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Try sending an xlsx report or notification. I wasn't able to test slack since I don't use it actively but implemented it nevertheless for completeness.

Known Problems

Currently the first column is displayed like

It can be monkey patched but it should be fixed naturally once/if #19735 lands.

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

[codecov]

Codecov Report

Merging #19810 (77b9047) into master (62b4564) will decrease coverage by 0.05%.
The diff coverage is 76.36%.

❗ Current head 77b9047 differs from pull request most recent head 3edb8d5. Consider uploading reports for the commit 3edb8d5 to get more accurate results

@@            Coverage Diff             @@
##           master   #19810      +/-   ##
==========================================
- Coverage   68.98%   68.93%   -0.05%     
==========================================
  Files        1903     1904       +1     
  Lines       74126    74101      -25     
  Branches     8110     8120      +10     
==========================================
- Hits        51138    51085      -53     
- Misses      20876    20904      +28     
  Partials     2112     2112              

Flag	Coverage Δ
hive	53.85% <30.76%> (-0.16%)	⬇️
javascript	55.65% <86.20%> (+0.02%)	⬆️
mysql	?
postgres	79.32% <65.38%> (-0.07%)	⬇️
presto	53.78% <30.76%> (-0.16%)	⬇️
python	83.25% <65.38%> (-0.11%)	⬇️
sqlite	77.82% <65.38%> (-0.07%)	⬇️
unit	54.54% <30.76%> (+0.03%)	⬆️

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

Impacted Files	Coverage Δ
...acy-preset-chart-deckgl/src/components/Tooltip.tsx	0.00% <0.00%> (ø)
...lugins/plugin-chart-table/src/utils/formatValue.ts	66.66% <0.00%> (-3.93%)	⬇️
...c/components/Chart/DrillDetail/DrillDetailPane.tsx	75.32% <ø> (ø)
...rset-frontend/src/components/ReportModal/index.tsx	79.03% <ø> (ø)
...-frontend/src/components/TableCollection/index.tsx	100.00% <ø> (ø)
...mponents/DataTablesPane/components/SamplesPane.tsx	97.67% <ø> (ø)
...ataTablesPane/components/SingleQueryResultPane.tsx	85.71% <ø> (ø)
...-frontend/src/features/alerts/AlertReportModal.tsx	54.44% <ø> (ø)
superset/datasets/api.py	87.95% <ø> (ø)
superset/security/manager.py	93.83% <ø> (ø)
... and 13 more

... and 7 files with indirect coverage changes

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

[smartsense-as]

Any update on this?

[etadelta222]

@cemremengu , I've posted this in superset slack (https://apache-superset.slack.com/archives/CCKHMGRRB/p1662748936923259) and there's some feedback. Hopefully get some 👀 on it. Thank you for creating this PR!

[cemremengu]

You are welcome! I am happy to continue once maintainers have time to chime in.

[eschutho]

This looks great. Just a few small suggestions.

[cemremengu]

@EugeneTorap thanks for the heads up! Just pushed the updates, my testing environment is limited now so apologies for any inconvenience

[artemonsh]

@cemremengu you forgot to add

to the file superset/reports/commands/execute.py

Otherwise, email reports look like this:

[EugeneTorap]

@cemremengu Thanks. Can you format a file with black to fix CI: Python Misc / pre-commit (3.8)?

pip install black
black superset/tests/integration_tests/reports/commands_tests.py

[EugeneTorap]

@cemremengu We use Excel name instead of XLSX in our #22006 PR
Can we use the Excel name for email alert/report UI logic?

[cemremengu]

@EugeneTorap you mean replacing all occurrences of XLSX with Excel like EXPORT_TO_EXCEL right ?

[EugeneTorap]

I think yes, need to replace all UI label from XLSX to Excel in this PR for name consistency.
@villebro @zhaoyongjie @michael-s-molina What do you think?

[cemremengu]

Just to clarify @EugeneTorap should something like this be

<StyledRadio value="EXCEL">{t('Send as Excel')}</StyledRadio>

or

<StyledRadio value="Excel">{t('Send as Excel')}</StyledRadio>

[EugeneTorap]

@cemremengu Value must be XLSX but label is Excel

<StyledRadio value="XLSX">{t('Send as Excel')}</StyledRadio>

[cemremengu]

@EugeneTorap Fixed 2 of them, I agree it is better like this.

[rusackas]

/testenv up

[github-actions]

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

[cemremengu]

@kakoni thank you for the tip!

[mdeshmu]

@cemremengu did you get time to fix this ?

[cemremengu]

Not sure what I need to fix. As far as I remember failing tests were flaky (I cannot see the CI log anymore)

@EugeneTorap @rusackas how should we proceed with this PR?

[rusackas]

Easiest way to get CI logs again is to close/reopen it, or apply new commits (like fixing the merge conflicts would do)

[cemremengu]

@rusackas rebased, not sure what the failing tests are about.

@kakoni had doubts about _get_data vs _get_csv_data in previous discussions. Once we settle that I think we can merge?

[rusackas]

The test-mysql, test-postgres, and test-sqlite actions are failing due to this:

FAILED tests/integration_tests/reports/commands_tests.py::test_email_chart_report_schedule_with_csv - superset.reports.commands.exceptions.ReportScheduleDataFrameFailedError: Failed generating dataframe Input string must be text, not bytes

the pre-commit check is failing because of a linting error. You can probably just run pre-commit run --all-files and commit any changes to clear that up.

[cemremengu]

@rusackas thanks for pointing these out! Cypress tests are failing but those seem to be caused by something else?

I decided to revert back to the old _get_csv_data way to benefit from the post processing. The only problem now seems that the excel contains the index column even though I export it with Index=False. Should just force drop the first column? Any ideas about that?

[mdeshmu]

@rusackas can we please unblock this by answering @cemremengu's question in last comment.

[rusackas]

Hi all! Sorry for not answering the last question, but this came up in my "PRs that haven't been touched in forever" audit, and I didn't realize I was the one blocking things. I actually don't know the answer to the standing question, but if anyone/everyone wants to spin this up, let's at least start with a rebase. I'll convert this to Draft mode in the meantime, and ping @yousoph and @eschutho who may have interest in helping usher this along. Let's see if we can get it across the finish line!

[rusackas]

I'm OK with or without the index column, but this still needs a rebase to get anywhere. Converting it to draft for now, but it may be closed due to inactivity after a while if it's not made mergeable.

[rusackas]

Closing due to ongoing inactivity, but we appreciate the contribution and are more than happy to reopen this if you want to pick it back up.