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
[dashboard fix] force refresh charts under tabs #5291
[dashboard fix] force refresh charts under tabs #5291
Conversation
@graceguo-supercat what do you think about a different fix where we simply don't fetch data for charts that haven't been rendered? that would reduce the query time as well. |
3168a26
to
3a93201
Compare
@@ -202,5 +202,10 @@ export function runQuery(formData, force = false, timeout = 60, key) { | |||
} | |||
|
|||
export function refreshChart(chart, force, timeout) { | |||
return dispatch => dispatch(runQuery(chart.latestQueryFormData, force, timeout, chart.id)); | |||
return dispatch => { | |||
if (Object.keys(chart.latestQueryFormData).length === 0) { |
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.
should there also be a check for whether chart.latestQueryFormData
is defined or is that guaranteed to be there?
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.
it is initialized from getInitialState in line 79:
https://github.com/apache/incubator-superset/blob/c06531950869d84a50e7914b945ba6ceb2a5c179/superset/assets/src/dashboard/reducers/getInitialState.js#L79
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.
can we add the check JIC since there is no downside to adding it?
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.
added.
3a93201
to
34b6f79
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.
LGTM assuming build passes. thanks for being extra careful on the check 👍
34b6f79
to
54cd99b
Compare
Codecov Report
@@ Coverage Diff @@
## master #5291 +/- ##
==========================================
- Coverage 61.3% 61.29% -0.01%
==========================================
Files 368 368
Lines 23455 23457 +2
Branches 2715 2716 +1
==========================================
Hits 14379 14379
- Misses 9064 9066 +2
Partials 12 12
Continue to review full report at Codecov.
|
(cherry picked from commit 4ee984c)
(cherry picked from commit 4ee984c)
(cherry picked from commit 4ee984c)
(cherry picked from commit 4ee984c)
(cherry picked from commit 4ee984c)
(cherry picked from commit 4ee984c)
(cherry picked from commit 4ee984c)
(cherry picked from commit 4ee984c)
(cherry picked from commit 4ee984c)
(cherry picked from commit 4ee984c)
(cherry picked from commit 4ee984c)
(cherry picked from commit 4ee984c)
when user created top level tabs and place charts under it, initial page load won't fetch those charts data. But if user force refresh dashboard, currently we force refresh all charts even not visible ones. It cause js exception because some attributes are not well initialized.
updated solution: when user force refresh whole dashboard, don't fetch data for charts that haven't been rendered.