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:
• [x] 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
• [x] Introduces new feature or API
• [ ] Removes existing feature or API

[codecov[bot]]

Codecov Report

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

:exclamation: 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%)	:arrow_down:
javascript	55.65% <86.20%> (+0.02%)	:arrow_up:
mysql	?
postgres	79.32% <65.38%> (-0.07%)	:arrow_down:
presto	53.78% <30.76%> (-0.16%)	:arrow_down:
python	83.25% <65.38%> (-0.11%)	:arrow_down:
sqlite	77.82% <65.38%> (-0.07%)	:arrow_down:
unit	54.54% <30.76%> (+0.03%)	:arrow_up:

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%)	:arrow_down:
...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

:mega: 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.

[EugeneTorap]

Hi @cemremengu. Any updates?

[cemremengu]

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

[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 Can you also add Excel support in Download on explore page for consistency?

[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 https://github.com/apache/superset/pull/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[bot]]

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

[rusackas]

/testenv up FEATURE_ALERT_REPORTS=true

[rusackas]

@cemremengu it looks like there was a PR by @lyndsiWilliams where some translations are moved into constants in one of the files. Would you mind rebasing off master to conform to this change? Sorry we didn't get this one merged first ;)

[github-actions[bot]]

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

[cemremengu]

@EugeneTorap here is my attempt, still needs to be checked for runtime errors in a test-env though.

I (if I got you correctly) basically combined _get_embedded_data and _get_csv_data into one function as _get_data to obtain the dataframe directly. Then from there, I applied the conversion according to ReportDataFormat

[EugeneTorap]

@geido @dpgaspar Can you review it? Maybe you have a good suggestion to improve the code?

[kakoni]

@cemremengu Btw, python integration tests are currently failing for this, superset.reports.commands.exceptions.ReportScheduleUnexpectedError: argument of type 'method' is not iterable

This is because there is line self._report_schedule.report_format in ReportDataFormat.table_like: it should be self._report_schedule.report_format in ReportDataFormat.table_like():

[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.