You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/02/04 22:59:42 UTC

[GitHub] [incubator-superset] graceguo-supercat opened a new pull request #9087: [logging] Add Auto Refresh Dashboard user event into dashboard logging

graceguo-supercat opened a new pull request #9087: [logging] Add Auto Refresh Dashboard user event into dashboard logging
URL: https://github.com/apache/incubator-superset/pull/9087
 
 
   ### CATEGORY
   
   Choose one
   
   - [x] Bug Fix
   - [ ] Enhancement (new features, refinement)
   - [x] Refactor
   - [ ] Add tests
   - [ ] Build / Development Environment
   - [ ] Documentation
   
   ### SUMMARY
   In Dashboard, we allow dashboard owner set a `refresh_frequency` and save it with dashboard metadata. A dashboard with refresh_frequency (> 0) will auto force-refresh itself periodically once it is loaded in browser. But this auto-refresh event is not logged correctly in current dashboard logging mechanism. 
   
   This PR is to add auto refresh event, and a little code refactor for the period rendering function.
   
   ### TEST PLAN
   Test is covered by existed integration tests.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   ### REVIEWERS
   @etr2460 @serenajiang 

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging
URL: https://github.com/apache/incubator-superset/pull/9087#discussion_r375414461
 
 

 ##########
 File path: superset/assets/src/dashboard/components/Header.jsx
 ##########
 @@ -188,7 +188,7 @@ class Header extends React.PureComponent {
 
   forceRefresh() {
     if (!this.props.isLoading) {
-      const chartList = Object.values(this.props.charts);
+      const chartList = Object.keys(this.props.charts);
 
 Review comment:
   I noticed this because when dashboard periodically refreshing for a long time, the dashboard state/chart state might be changed, like added filter etc. User will expect periodically load the latest query params.
   
   If I pass chart object here, its state is not updated. So i prefer to only pass the affected chart id.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging
URL: https://github.com/apache/incubator-superset/pull/9087#discussion_r375414461
 
 

 ##########
 File path: superset/assets/src/dashboard/components/Header.jsx
 ##########
 @@ -188,7 +188,7 @@ class Header extends React.PureComponent {
 
   forceRefresh() {
     if (!this.props.isLoading) {
-      const chartList = Object.values(this.props.charts);
+      const chartList = Object.keys(this.props.charts);
 
 Review comment:
   I noticed this because when dashboard periodically refreshing for a long time, the dashboard state/chart state might be changed, like add filter etc.  User will expect periodically load the latest query params.
       If I pass chart object here, its state is not updated. So i prefer to only pass the affected chart id.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging
URL: https://github.com/apache/incubator-superset/pull/9087#discussion_r375414461
 
 

 ##########
 File path: superset/assets/src/dashboard/components/Header.jsx
 ##########
 @@ -188,7 +188,7 @@ class Header extends React.PureComponent {
 
   forceRefresh() {
     if (!this.props.isLoading) {
-      const chartList = Object.values(this.props.charts);
+      const chartList = Object.keys(this.props.charts);
 
 Review comment:
   I noticed this because when dashboard periodically refreshing for a long time, the dashboard state/chart state might be changed, like add filter etc.  User will expect periodically load the latest query params.
   If I pass chart object here, its state is not updated. So i prefer to only pass the affected chart id.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging
URL: https://github.com/apache/incubator-superset/pull/9087#discussion_r375412083
 
 

 ##########
 File path: superset/assets/src/dashboard/util/setPeriodicRunner.js
 ##########
 @@ -0,0 +1,36 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+const stopPeriodicRender = refreshTimer => {
 
 Review comment:
   Stop interval is here.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging
URL: https://github.com/apache/incubator-superset/pull/9087#discussion_r375444022
 
 

 ##########
 File path: superset/assets/src/dashboard/util/setPeriodicRunner.js
 ##########
 @@ -0,0 +1,36 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+const stopPeriodicRender = refreshTimer => {
 
 Review comment:
   oh duh, sorry!

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging
URL: https://github.com/apache/incubator-superset/pull/9087#discussion_r375383102
 
 

 ##########
 File path: superset/assets/src/dashboard/components/Header.jsx
 ##########
 @@ -188,7 +188,7 @@ class Header extends React.PureComponent {
 
   forceRefresh() {
     if (!this.props.isLoading) {
-      const chartList = Object.values(this.props.charts);
+      const chartList = Object.keys(this.props.charts);
 
 Review comment:
   what's the value in only passing around the chart keys instead of the chart object? It looks like we still need to get the whole object from state when we actually fetch the chart, so does this save much?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging
URL: https://github.com/apache/incubator-superset/pull/9087#discussion_r375384004
 
 

 ##########
 File path: superset/assets/src/dashboard/actions/dashboardState.js
 ##########
 @@ -238,46 +236,12 @@ export function fetchCharts(chartList = [], force = false, interval = 0) {
     const delay = meta.stagger_refresh
       ? refreshTime / (chartList.length - 1)
       : 0;
-    chartList.forEach((chart, i) => {
-      setTimeout(
-        () => dispatch(refreshChart(chart, force, timeout)),
-        delay * i,
-      );
+    chartList.forEach((chartKey, i) => {
+      setTimeout(() => dispatch(refreshChart(chartKey, force)), delay * i);
     });
   };
 }
 
-let refreshTimer = null;
-export function startPeriodicRender(interval) {
-  const stopPeriodicRender = () => {
-    if (refreshTimer) {
-      clearTimeout(refreshTimer);
-      refreshTimer = null;
-    }
-  };
-
-  return (dispatch, getState) => {
-    stopPeriodicRender();
 
 Review comment:
   did we lose the code where we cancel the periodic render function? i didn't see it anywhere here (but might've missed it)

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-io commented on issue #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging
URL: https://github.com/apache/incubator-superset/pull/9087#issuecomment-582183550
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9087?src=pr&el=h1) Report
   > Merging [#9087](https://codecov.io/gh/apache/incubator-superset/pull/9087?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/66fd177000beabea33dadcb4e27bae35a32f3dbc?src=pr&el=desc) will **decrease** coverage by `0.49%`.
   > The diff coverage is `17.64%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9087/graphs/tree.svg?width=650&token=KsB0fHcx6l&height=150&src=pr)](https://codecov.io/gh/apache/incubator-superset/pull/9087?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##           master    #9087     +/-   ##
   =========================================
   - Coverage   59.44%   58.94%   -0.5%     
   =========================================
     Files         369      371      +2     
     Lines       11749    11821     +72     
     Branches     2888     2903     +15     
   =========================================
   - Hits         6984     6968     -16     
   - Misses       4586     4669     +83     
   - Partials      179      184      +5
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9087?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ssets/src/dashboard/containers/DashboardHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29udGFpbmVycy9EYXNoYm9hcmRIZWFkZXIuanN4) | `100% <ø> (ø)` | :arrow_up: |
   | [superset/assets/src/logger/LogUtils.js](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9sb2dnZXIvTG9nVXRpbHMuanM=) | `100% <ø> (ø)` | :arrow_up: |
   | [...set/assets/src/dashboard/actions/dashboardState.js](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvYWN0aW9ucy9kYXNoYm9hcmRTdGF0ZS5qcw==) | `31.72% <0%> (-9.24%)` | :arrow_down: |
   | [superset/assets/src/chart/chartAction.js](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9jaGFydC9jaGFydEFjdGlvbi5qcw==) | `43.24% <0%> (-4.71%)` | :arrow_down: |
   | [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29tcG9uZW50cy9ncmlkQ29tcG9uZW50cy9DaGFydC5qc3g=) | `67.41% <100%> (ø)` | :arrow_up: |
   | [...set/assets/src/dashboard/util/setPeriodicRunner.js](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvdXRpbC9zZXRQZXJpb2RpY1J1bm5lci5qcw==) | `15.38% <15.38%> (ø)` | |
   | [...uperset/assets/src/dashboard/components/Header.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29tcG9uZW50cy9IZWFkZXIuanN4) | `41.48% <23.07%> (-0.59%)` | :arrow_down: |
   | [...sets/src/explore/components/DisplayQueryButton.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvRGlzcGxheVF1ZXJ5QnV0dG9uLmpzeA==) | `51.38% <0%> (-1.74%)` | :arrow_down: |
   | [...rset/assets/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL3JlZHVjZXJzL2V4cGxvcmVSZWR1Y2VyLmpz) | `25% <0%> (-0.93%)` | :arrow_down: |
   | ... and [6 more](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9087?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9087?src=pr&el=footer). Last update [66fd177...b3a0d7b](https://codecov.io/gh/apache/incubator-superset/pull/9087?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat merged pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging

Posted by GitBox <gi...@apache.org>.
graceguo-supercat merged pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging
URL: https://github.com/apache/incubator-superset/pull/9087
 
 
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-io edited a comment on issue #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging
URL: https://github.com/apache/incubator-superset/pull/9087#issuecomment-582183550
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9087?src=pr&el=h1) Report
   > Merging [#9087](https://codecov.io/gh/apache/incubator-superset/pull/9087?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/66fd177000beabea33dadcb4e27bae35a32f3dbc?src=pr&el=desc) will **decrease** coverage by `0.43%`.
   > The diff coverage is `34.48%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9087/graphs/tree.svg?width=650&token=KsB0fHcx6l&height=150&src=pr)](https://codecov.io/gh/apache/incubator-superset/pull/9087?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##           master   #9087      +/-   ##
   =========================================
   - Coverage   59.44%     59%   -0.44%     
   =========================================
     Files         369     371       +2     
     Lines       11749   11816      +67     
     Branches     2888    2902      +14     
   =========================================
   - Hits         6984    6972      -12     
   - Misses       4586    4662      +76     
   - Partials      179     182       +3
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9087?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ssets/src/dashboard/containers/DashboardHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29udGFpbmVycy9EYXNoYm9hcmRIZWFkZXIuanN4) | `100% <ø> (ø)` | :arrow_up: |
   | [superset/assets/src/logger/LogUtils.js](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9sb2dnZXIvTG9nVXRpbHMuanM=) | `100% <ø> (ø)` | :arrow_up: |
   | [...set/assets/src/dashboard/actions/dashboardState.js](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvYWN0aW9ucy9kYXNoYm9hcmRTdGF0ZS5qcw==) | `31.72% <0%> (-9.24%)` | :arrow_down: |
   | [superset/assets/src/chart/chartAction.js](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9jaGFydC9jaGFydEFjdGlvbi5qcw==) | `43.24% <0%> (-4.71%)` | :arrow_down: |
   | [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29tcG9uZW50cy9ncmlkQ29tcG9uZW50cy9DaGFydC5qc3g=) | `67.41% <100%> (ø)` | :arrow_up: |
   | [...uperset/assets/src/dashboard/components/Header.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29tcG9uZW50cy9IZWFkZXIuanN4) | `41.79% <25%> (-0.28%)` | :arrow_down: |
   | [...set/assets/src/dashboard/util/setPeriodicRunner.js](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvdXRpbC9zZXRQZXJpb2RpY1J1bm5lci5qcw==) | `66.66% <66.66%> (ø)` | |
   | [...sets/src/explore/components/DisplayQueryButton.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvRGlzcGxheVF1ZXJ5QnV0dG9uLmpzeA==) | `51.38% <0%> (-1.74%)` | :arrow_down: |
   | [...rset/assets/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL3JlZHVjZXJzL2V4cGxvcmVSZWR1Y2VyLmpz) | `25% <0%> (-0.93%)` | :arrow_down: |
   | ... and [6 more](https://codecov.io/gh/apache/incubator-superset/pull/9087/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9087?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9087?src=pr&el=footer). Last update [66fd177...bab5097](https://codecov.io/gh/apache/incubator-superset/pull/9087?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging
URL: https://github.com/apache/incubator-superset/pull/9087#discussion_r375414461
 
 

 ##########
 File path: superset/assets/src/dashboard/components/Header.jsx
 ##########
 @@ -188,7 +188,7 @@ class Header extends React.PureComponent {
 
   forceRefresh() {
     if (!this.props.isLoading) {
-      const chartList = Object.values(this.props.charts);
+      const chartList = Object.keys(this.props.charts);
 
 Review comment:
   I noticed this because when dashboard periodically refreshing for a long time, the dashboard state/chart state might be changed, like add filter etc. If I pass chart object here, its state is not updated. So i prefer to only pass the affected chart id.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9087: [logging] Add Auto Refresh Dashboard event into dashboard logging
URL: https://github.com/apache/incubator-superset/pull/9087#discussion_r375414461
 
 

 ##########
 File path: superset/assets/src/dashboard/components/Header.jsx
 ##########
 @@ -188,7 +188,7 @@ class Header extends React.PureComponent {
 
   forceRefresh() {
     if (!this.props.isLoading) {
-      const chartList = Object.values(this.props.charts);
+      const chartList = Object.keys(this.props.charts);
 
 Review comment:
   I noticed this because when dashboard periodically refreshing for a long time, the dashboard state/chart state might be changed, like add filter etc.  User will expect periodically load the latest query params.
   
   If I pass chart object here, its state is not updated. So i prefer to only pass the affected chart id.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org