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/06/02 18:46:03 UTC
[GitHub] [incubator-superset] etr2460 commented on a change in pull request #9886: feat: [dashboard] notification and warning for auto force refresh
etr2460 commented on a change in pull request #9886:
URL: https://github.com/apache/incubator-superset/pull/9886#discussion_r434098118
##########
File path: superset-frontend/src/dashboard/components/Header.jsx
##########
@@ -218,6 +234,10 @@ class Header extends React.PureComponent {
interval,
chartCount: affectedCharts.length,
});
+ this.props.addWarningToast(
+ t(`This dashboard will force refresh at every ${intervalMessage}`),
Review comment:
This toast pops up when the dashboard starts refreshing right? It's a bit weird to say it `will force refresh` if it's currently force refreshing. Maybe the wording should be `This dashboard is currently force refreshing; the next force refresh will be in ${intervalMessage}.`?
cc @sylvia-tomiyama, @zuzana-vej for copy here
##########
File path: superset/views/base.py
##########
@@ -61,6 +61,8 @@
FRONTEND_CONF_KEYS = (
"SUPERSET_WEBSERVER_TIMEOUT",
"SUPERSET_DASHBOARD_POSITION_DATA_LIMIT",
+ "SUPERSET_DASHBOARD_PERIODICAL_REFRESH_LIMIT",
+ "SUPERSET_DASHBOARD_PERIODICAL_REFRESH_WARNING_MESSAGE",
Review comment:
can you add these two config options to the config.py file too and document them there? You can default it to 0/None if you'd like, or provide a generic message, but i think they should be documented there regardless
##########
File path: superset-frontend/src/dashboard/components/Header.jsx
##########
@@ -318,11 +345,17 @@ class Header extends React.PureComponent {
hasUnsavedChanges,
isLoading,
refreshFrequency,
+ isPersistentRefreshFrequency,
setRefreshFrequency,
} = this.props;
const userCanEdit = dashboardInfo.dash_edit_perm;
const userCanSaveAs = dashboardInfo.dash_save_perm;
+ const refreshLimit =
+ dashboardInfo?.common?.conf?.SUPERSET_DASHBOARD_PERIODICAL_REFRESH_LIMIT;
Review comment:
why do we need the optional chaining here? I'd expect the `conf` object always to be included in dashboardInfo since I believe it comes down with the initial bootstrap payload.
##########
File path: superset-frontend/src/dashboard/components/RefreshIntervalModal.jsx
##########
@@ -74,12 +102,32 @@ class RefreshIntervalModal extends React.PureComponent {
value={this.state.refreshFrequency}
onChange={this.handleFrequencyChange}
/>
+ {showRefreshWarning && (
+ <div className="refresh-warning-container">
+ <Alert bsStyle="warning">
+ <div>{refreshWarning}</div>
+ <br />
+ <div className="bold">
+ {t('Are you sure you still want to proceed?')}
Review comment:
I'd remove the `still` here because it's a bit more concise, but up to you
##########
File path: superset-frontend/src/dashboard/components/Header.jsx
##########
@@ -86,6 +89,7 @@ const propTypes = {
setMaxUndoHistoryExceeded: PropTypes.func.isRequired,
maxUndoHistoryToast: PropTypes.func.isRequired,
refreshFrequency: PropTypes.number.isRequired,
+ isPersistentRefreshFrequency: PropTypes.number.isRequired,
Review comment:
actually, i see this same variable set to booleans further down, is the proptypes wrong here?
##########
File path: superset-frontend/src/dashboard/components/Header.jsx
##########
@@ -86,6 +89,7 @@ const propTypes = {
setMaxUndoHistoryExceeded: PropTypes.func.isRequired,
maxUndoHistoryToast: PropTypes.func.isRequired,
refreshFrequency: PropTypes.number.isRequired,
+ isPersistentRefreshFrequency: PropTypes.number.isRequired,
Review comment:
this variable is a number but starts with `is` which would imply it's a boolean. Can we rename the variable?
##########
File path: superset-frontend/src/dashboard/util/getPersistentRefreshFrequency.ts
##########
@@ -0,0 +1,28 @@
+/**
+ * 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.
+ */
+export default function getPersistentRefreshFrequency({
+ currentRefreshFrequency = 0,
+ isPersistent = false,
+ dashboardInfo = {},
+}): number {
+ // get original persistent refresh frequency
+ const { metadata }: any = dashboardInfo;
Review comment:
We know dashboardInfo is an object at least, right? So maybe we can be more specific than `any` type here?
##########
File path: superset-frontend/src/dashboard/components/Header.jsx
##########
@@ -218,6 +234,10 @@ class Header extends React.PureComponent {
interval,
chartCount: affectedCharts.length,
});
+ this.props.addWarningToast(
+ t(`This dashboard will force refresh at every ${intervalMessage}`),
Review comment:
You should provide args for formatting this message instead of using string templates as per this api: https://github.com/apache-superset/superset-ui/tree/master/packages/superset-ui-translation#api
##########
File path: superset-frontend/src/dashboard/components/RefreshIntervalModal.jsx
##########
@@ -74,12 +102,32 @@ class RefreshIntervalModal extends React.PureComponent {
value={this.state.refreshFrequency}
onChange={this.handleFrequencyChange}
/>
+ {showRefreshWarning && (
+ <div className="refresh-warning-container">
+ <Alert bsStyle="warning">
+ <div>{refreshWarning}</div>
+ <br />
+ <div className="bold">
Review comment:
instead of this, why not use a `<strong>` tag instead? We wouldn't need the extra css class
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org