feat: call screenshot to store query_context
#15846
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
betodealmeida
requested review from
villebro,
lilykuang,
eschutho and
hughhhh
and removed request for
lilykuang
Codecov Report
@@ Coverage Diff @@
## master #15846 +/- ##
==========================================
- Coverage 77.05% 76.76% -0.30%
==========================================
Files 986 986
Lines 51944 51993 +49
Branches 7081 7090 +9
==========================================
- Hits 40025 39911 -114
- Misses 11693 11856 +163
Partials 226 226
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
betodealmeida
force-pushed
the
report_populate_query_context
branch
from
869cfa4
to
c089468
Compare
betodealmeida
commented
Jul 26, 2021
eschutho
reviewed
Jul 26, 2021
superset/reports/commands/execute.py
Outdated
| # get a screenshot to force the chart to produce and save the query | ||
| # context. | ||
| if self._report_schedule.chart.query_context is None: | ||
| self._get_screenshot() |
There was a problem hiding this comment.
do you think there may be a case where the screenshot finishes before the put? Do we need some error handling for that case?
There was a problem hiding this comment.
Good point, I'll add some error handling.
eschutho
approved these changes
Jul 26, 2021
opus-42
pushed a commit
to opus-42/incubator-superset
that referenced
this pull request
Nov 14, 2021
* feat: call screenshot to store query_context * Add unit test * Move updateQueryContext to ExploreChartPanel * Add error handling * Fix code * Fix logic
cccs-RyanS
pushed a commit
to CybercentreCanada/superset
that referenced
this pull request
Dec 17, 2021
* feat: call screenshot to store query_context * Add unit test * Move updateQueryContext to ExploreChartPanel * Add error handling * Fix code * Fix logic
QAlexBall
pushed a commit
to QAlexBall/superset
that referenced
this pull request
Dec 29, 2021
* feat: call screenshot to store query_context * Add unit test * Move updateQueryContext to ExploreChartPanel * Add error handling * Fix code * Fix logic
9 tasks
cccs-rc
pushed a commit
to CybercentreCanada/superset
that referenced
this pull request
Mar 6, 2024
* feat: call screenshot to store query_context * Add unit test * Move updateQueryContext to ExploreChartPanel * Add error handling * Fix code * Fix logic
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
#15830 modified the
_get_csv_datafunctionality in the reports executor to use a new chart API introduced in #15827. One problem is that the new API can only be called for charts that were saved with theirquery_context, a change that was introduced in #15824.Pre-existing charts have
query_contextset to null, so we can't use the new API with them. As a workaround, @hughhhh implemented logic to savequery_contextwhen a chart that doesn't have it saved is loaded (#15865). This PR leverages that work by taking a screenshot of the chart using Selenium whenever we need to call the CSV API butquery_contextis null.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
query_contextof the chart (UPDATE slices SET query_context = NULL WHERE id=XXX)query_contextshould be back.ADDITIONAL INFORMATION