simchaNielsen commented on a change in pull request #11880:
URL:
https://github.com/apache/incubator-superset/pull/11880#discussion_r534861251
##########
File path: superset-frontend/src/chart/chartAction.js
##########
@@ -360,28 +360,33 @@ export function exploreJSON(
// problem: response holds a list of results, when before we were just
getting one result.
// How to make the entire app compatible with multiple results?
// For now just use the first result.
- const result = response.result[0];
+ const queryResponse = response.result[0]; // deprecated
+ const queriesResponse = response.result;
- dispatch(
- logEvent(LOG_ACTIONS_LOAD_CHART, {
- slice_id: key,
- applied_filters: result.applied_filters,
- is_cached: result.is_cached,
- force_refresh: force,
- row_count: result.rowcount,
- datasource: formData.datasource,
- start_offset: logStart,
- ts: new Date().getTime(),
- duration: Logger.getTimestamp() - logStart,
- has_extra_filters:
- formData.extra_filters && formData.extra_filters.length > 0,
- viz_type: formData.viz_type,
- data_age: result.is_cached
- ? moment(new Date()).diff(moment.utc(result.cached_dttm))
- : null,
- }),
+ queriesResponse.forEach(resultItem =>
+ dispatch(
+ logEvent(LOG_ACTIONS_LOAD_CHART, {
+ slice_id: key,
+ applied_filters: resultItem.applied_filters,
+ is_cached: resultItem.is_cached,
+ force_refresh: force,
+ row_count: resultItem.rowcount,
+ datasource: formData.datasource,
+ start_offset: logStart,
+ ts: new Date().getTime(),
+ duration: Logger.getTimestamp() - logStart,
+ has_extra_filters:
+ formData.extra_filters && formData.extra_filters.length > 0,
+ viz_type: formData.viz_type,
+ data_age: resultItem.is_cached
+ ? moment(new Date()).diff(moment.utc(resultItem.cached_dttm))
+ : null,
+ }),
+ ),
+ );
+ return dispatch(
+ chartUpdateSucceeded(queryResponse, queriesResponse, key),
Review comment:
@robdiciuccio hmm... good point, actually I wanted avoid refactoring of
a lot places in code, but now I see I think I can handle it, I also will add
someone more to review, where I see that my refactor will affect his parts of
code to be sure. I tests these changes in explore/dashboard in areas that I
know, but if there are some other places, that I need to test please update me.
I pushed now refactored code as you proposed and asked @etr2460 to review
also...
I also see that affected some code that @suddjian implemented, may be it can
be good to add him as reviewer?
Thanks
##########
File path: superset-frontend/src/chart/chartAction.js
##########
@@ -360,28 +360,33 @@ export function exploreJSON(
// problem: response holds a list of results, when before we were just
getting one result.
// How to make the entire app compatible with multiple results?
// For now just use the first result.
- const result = response.result[0];
+ const queryResponse = response.result[0]; // deprecated
+ const queriesResponse = response.result;
- dispatch(
- logEvent(LOG_ACTIONS_LOAD_CHART, {
- slice_id: key,
- applied_filters: result.applied_filters,
- is_cached: result.is_cached,
- force_refresh: force,
- row_count: result.rowcount,
- datasource: formData.datasource,
- start_offset: logStart,
- ts: new Date().getTime(),
- duration: Logger.getTimestamp() - logStart,
- has_extra_filters:
- formData.extra_filters && formData.extra_filters.length > 0,
- viz_type: formData.viz_type,
- data_age: result.is_cached
- ? moment(new Date()).diff(moment.utc(result.cached_dttm))
- : null,
- }),
+ queriesResponse.forEach(resultItem =>
+ dispatch(
+ logEvent(LOG_ACTIONS_LOAD_CHART, {
+ slice_id: key,
+ applied_filters: resultItem.applied_filters,
+ is_cached: resultItem.is_cached,
+ force_refresh: force,
+ row_count: resultItem.rowcount,
+ datasource: formData.datasource,
+ start_offset: logStart,
+ ts: new Date().getTime(),
+ duration: Logger.getTimestamp() - logStart,
+ has_extra_filters:
+ formData.extra_filters && formData.extra_filters.length > 0,
+ viz_type: formData.viz_type,
+ data_age: resultItem.is_cached
+ ? moment(new Date()).diff(moment.utc(resultItem.cached_dttm))
+ : null,
+ }),
+ ),
+ );
+ return dispatch(
+ chartUpdateSucceeded(queryResponse, queriesResponse, key),
Review comment:
@robdiciuccio hmm... good point, actually I wanted avoid refactoring of
a lot places in code, but now I see I think I can handle it, I also will add
someone more to review, where I see that my refactor will affect his parts of
code to be sure. I tested these changes in explore/dashboard in areas that I
know, but if there are some other places, that I need to test please update me.
I pushed now refactored code as you proposed and asked @etr2460 to review
also...
I also see that affected some code that @suddjian implemented, may be it can
be good to add him as reviewer?
Thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
