-
Notifications
You must be signed in to change notification settings - Fork 14.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(report): allow capturing dashboard reports in specific state #20552
Conversation
99b7a08
to
2692a98
Compare
Codecov Report
@@ Coverage Diff @@
## master #20552 +/- ##
==========================================
- Coverage 66.23% 66.09% -0.14%
==========================================
Files 1757 1758 +1
Lines 66978 66987 +9
Branches 7117 7117
==========================================
- Hits 44360 44273 -87
- Misses 20801 20897 +96
Partials 1817 1817
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. |
2df55eb
to
5d6ae1d
Compare
f05debb
to
b04fd2e
Compare
f7adb55
to
723e9d9
Compare
723e9d9
to
dee64d5
Compare
dee64d5
to
aae0516
Compare
aae0516
to
605cec6
Compare
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.
Thanks for adding the feature. I few comments, but otherwise LGTM.
.../migrations/versions/2022-07-11_11-26_ffa79af61a56_rename_report_schedule_extra_to_extra_.py
Show resolved
Hide resolved
superset/reports/commands/create.py
Outdated
anchor = dashboard_state.get("anchor") | ||
invalid_tab_ids = { | ||
tab_id for tab_id in active_tabs if tab_id not in position_data | ||
} |
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.
You could inline this as two set comprehensions via { ... } | { ... }
.
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.
I think |
is actually union. What I needed is set(a) - set(b)
.
605cec6
to
aa9e399
Compare
8f39f04
to
9377aa5
Compare
9377aa5
to
3047bdb
Compare
SUMMARY
Depends on: #20632 , please review after it is merged.(merged)We want to allow users to specify which tabs are selected (and potentially which filters has been applied, too) when generating dashboard reports (see #19354 ). This PR is API changes requires for this to be possible.
Previous attempts (#17749) implemented this feature with only 1 tab being selectable (and highlighted), but will generate multiple screenshots in one report.
This isn't ideal---because there would be no way to compose a view where multiple specific tabs are selected (whether nested or not). We want to generate a screenshot where the whole dashboard state is preserved, hence this approach in this PR---the report_schedule.extra saves only one dashboard state at a time (including which tabs are selected, which filters are applied, etc), then we make only one screenshot for each report that corresponds to the specific dashboard state.
If users want reports for multiple dashboard tabs, they can create multiple reports---currently we don't allow multiple reports for each user on each report, which we would need to change. This PR only changes the report schedule schema and screenshot generation process. There will be more PRs to follow to fully enable the desired feature.
Future TODOs:
#17749 isn't used by the frontend in any way, so it should be safe to just replace it.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
We rely on unit tests.
ADDITIONAL INFORMATION