-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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(reports): xlsx attachments #19810
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
2b556b2
to
5c04c2a
Compare
Any update on this? |
@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! |
You are welcome! I am happy to continue once maintainers have time to chime in. |
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.
This looks great. Just a few small suggestions.
5c04c2a
to
92805d1
Compare
@EugeneTorap thanks for the heads up! Just pushed the updates, my testing environment is limited now so apologies for any inconvenience |
@cemremengu you forgot to add to the file superset/reports/commands/execute.py |
92805d1
to
d22cdbf
Compare
@cemremengu Thanks. Can you format a file with black to fix CI: Python Misc / pre-commit (3.8)?
|
@cemremengu We use |
@EugeneTorap you mean replacing all occurrences of |
I think yes, need to replace all UI label from |
Just to clarify @EugeneTorap should something like this be
or
|
@cemremengu Value must be
|
@EugeneTorap Fixed 2 of them, I agree it is better like this. |
/testenv up |
@rusackas Ephemeral environment spinning up at http://35.86.251.160:8080. Credentials are |
@cemremengu Btw, python integration tests are currently failing for this, This is because there is line |
@kakoni thank you for the tip! |
@cemremengu did you get time to fix this ? |
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? |
Easiest way to get CI logs again is to close/reopen it, or apply new commits (like fixing the merge conflicts would do) |
The
the |
f7678b1
to
ab7fb9c
Compare
@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 |
@rusackas can we please unblock this by answering @cemremengu's question in last comment. |
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! |
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