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 2022/10/24 22:05:38 UTC

[GitHub] [superset] zhaorui2022 opened a new pull request, #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

zhaorui2022 opened a new pull request, #21924:
URL: https://github.com/apache/superset/pull/21924

   ### SUMMARY
   The original dashboard auto-fresh intervals were hard-coded. However, based on the back-end query engine architecture, some options might not work. Move these options to config file so that they can be customized.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   With default option, there is no change on the UI. Tested with customized options in config file. 
   
   https://user-images.githubusercontent.com/105950525/197638482-e4e84f58-e9ae-4945-a7b3-ba54cfd6beff.mp4
   
   Also verified the auto refresh function is still working as expected.
   
   ### TESTING INSTRUCTIONS
   If testing with original options, no change is needed. Otherwise, change the options in the config file. Run `docker-compose up` and then view auto refresh options in any dashboard.
   
   
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [X] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] michael-s-molina commented on a diff in pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21924:
URL: https://github.com/apache/superset/pull/21924#discussion_r1004808935


##########
superset-frontend/src/configs.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * 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 function configs() {
+  // read superset config using multiple sources
+  // currently it only reads from bootstrap data
+  return configsFromBootstrapData();
+}
+
+function configsFromBootstrapData() {

Review Comment:
   I think there was an error with the previous link. I update it to point to https://github.com/apache/superset/blob/1388f21ee34251b6ef83beb009ba0901e4067848/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx#L352
   
   > So if I want to read bootstrap data in this RefreshIntervalModal which belongs to Dashboard, does that mean this file should contain some logic to save bootstrapData?
   
   You don't need to save `bootstrapData`, you just need to retrieve the values from Redux instead of parsing `data-bootstrap` again like you're doing in https://github.com/apache/superset/pull/21924/files#diff-9cbbe9c430f0761d26c381b58358e44e5c4f0ac353f223832e9150d4070413c4



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] michael-s-molina commented on a diff in pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21924:
URL: https://github.com/apache/superset/pull/21924#discussion_r1004779293


##########
superset-frontend/src/configs.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * 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 function configs() {
+  // read superset config using multiple sources
+  // currently it only reads from bootstrap data
+  return configsFromBootstrapData();
+}
+
+function configsFromBootstrapData() {

Review Comment:
   Your reference is correct. In this file, the `common` state will be initialized in the Redux store with contents from `data-bootstrap`. This means that you don't need to parse the content again, you can access it using Redux like [this](https://github.com/apache/superset/blob/1388f21ee34251b6ef83beb009ba0901e4067848/superset-frontend/src/dashboard/containers/Dashboard.ts). If any component dynamically changes the value of that configuration, your component would be automatically updated.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] michael-s-molina commented on a diff in pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21924:
URL: https://github.com/apache/superset/pull/21924#discussion_r1005987272


##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -57,6 +44,7 @@ type RefreshIntervalModalProps = {
   editMode: boolean;
   refreshLimit?: number;
   refreshWarning: string | null;
+  refreshIntervalOptions: [[number, string]];

Review Comment:
   ```suggestion
     refreshIntervalOptions: [number, string][];
   ```



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] zhaorui2022 commented on a diff in pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
zhaorui2022 commented on code in PR #21924:
URL: https://github.com/apache/superset/pull/21924#discussion_r1004755870


##########
superset-frontend/src/configs.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * 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 function configs() {
+  // read superset config using multiple sources
+  // currently it only reads from bootstrap data
+  return configsFromBootstrapData();
+}
+
+function configsFromBootstrapData() {

Review Comment:
   Is there any example. I have found several but they all read from bootstrap data https://sourcegraph.com/search?q=getAttribute%28%27data-bootstrap%27%29+repo%3Aapache%2Fsuperset%24



##########
superset-frontend/src/configs.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * 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 function configs() {
+  // read superset config using multiple sources
+  // currently it only reads from bootstrap data
+  return configsFromBootstrapData();
+}
+
+function configsFromBootstrapData() {

Review Comment:
   Is there any example? I have found several but they all read from bootstrap data https://sourcegraph.com/search?q=getAttribute%28%27data-bootstrap%27%29+repo%3Aapache%2Fsuperset%24



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] codecov[bot] commented on pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21924:
URL: https://github.com/apache/superset/pull/21924#issuecomment-1289750134

   # [Codecov](https://codecov.io/gh/apache/superset/pull/21924?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#21924](https://codecov.io/gh/apache/superset/pull/21924?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4237c92) into [master](https://codecov.io/gh/apache/superset/commit/f41d0b0cbf47042bf510dc2b0b24b68e3fa11d37?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f41d0b0) will **decrease** coverage by `0.12%`.
   > The diff coverage is `100.00%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #21924      +/-   ##
   ==========================================
   - Coverage   66.82%   66.70%   -0.13%     
   ==========================================
     Files        1805     1805              
     Lines       69070    69086      +16     
     Branches     7373     7373              
   ==========================================
   - Hits        46159    46081      -78     
   - Misses      21005    21099      +94     
     Partials     1906     1906              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | mysql | `78.37% <100.00%> (+0.02%)` | :arrow_up: |
   | postgres | `?` | |
   | presto | `?` | |
   | python | `80.99% <100.00%> (-0.28%)` | :arrow_down: |
   | sqlite | `?` | |
   | unit | `51.06% <100.00%> (+<0.01%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/21924?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../src/dashboard/components/RefreshIntervalModal.tsx](https://codecov.io/gh/apache/superset/pull/21924/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1JlZnJlc2hJbnRlcnZhbE1vZGFsLnRzeA==) | `80.00% <ø> (ø)` | |
   | [superset/views/base.py](https://codecov.io/gh/apache/superset/pull/21924/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvYmFzZS5weQ==) | `75.70% <ø> (ø)` | |
   | [superset/config.py](https://codecov.io/gh/apache/superset/pull/21924/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.71% <100.00%> (+0.07%)` | :arrow_up: |
   | [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/21924/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
   | [superset/sql\_validators/\_\_init\_\_.py](https://codecov.io/gh/apache/superset/pull/21924/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvX19pbml0X18ucHk=) | `80.00% <0.00%> (-20.00%)` | :arrow_down: |
   | [superset/sqllab/sql\_json\_executer.py](https://codecov.io/gh/apache/superset/pull/21924/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsbGFiL3NxbF9qc29uX2V4ZWN1dGVyLnB5) | `64.63% <0.00%> (-9.76%)` | :arrow_down: |
   | [superset/sqllab/execution\_context\_convertor.py](https://codecov.io/gh/apache/superset/pull/21924/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsbGFiL2V4ZWN1dGlvbl9jb250ZXh0X2NvbnZlcnRvci5weQ==) | `81.48% <0.00%> (-7.41%)` | :arrow_down: |
   | [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/21924/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `89.28% <0.00%> (-7.15%)` | :arrow_down: |
   | [superset/db\_engine\_specs/postgres.py](https://codecov.io/gh/apache/superset/pull/21924/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Bvc3RncmVzLnB5) | `89.83% <0.00%> (-6.78%)` | :arrow_down: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/21924/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `81.05% <0.00%> (-6.73%)` | :arrow_down: |
   | ... and [24 more](https://codecov.io/gh/apache/superset/pull/21924/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] michael-s-molina commented on a diff in pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21924:
URL: https://github.com/apache/superset/pull/21924#discussion_r1004779293


##########
superset-frontend/src/configs.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * 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 function configs() {
+  // read superset config using multiple sources
+  // currently it only reads from bootstrap data
+  return configsFromBootstrapData();
+}
+
+function configsFromBootstrapData() {

Review Comment:
   Your reference is correct. In this file, the `common` state will be initialized in the Redux store with contents from `data-bootstrap`. This means that you don't need to parse the content again, you can access it using Redux like [this](https://github.com/apache/superset/blob/1388f21ee34251b6ef83beb009ba0901e4067848/superset-frontend/src/dashboard/containers/Dashboard.ts#L53). If any component dynamically changes the value of that configuration, your component would be automatically updated.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] zhaorui2022 commented on a diff in pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
zhaorui2022 commented on code in PR #21924:
URL: https://github.com/apache/superset/pull/21924#discussion_r1004762315


##########
superset-frontend/src/configs.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * 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 function configs() {
+  // read superset config using multiple sources
+  // currently it only reads from bootstrap data
+  return configsFromBootstrapData();
+}
+
+function configsFromBootstrapData() {

Review Comment:
   One example I used for reference was https://github.com/apache/superset/blob/3311128c5e6c5de2ea5d6a2dfeb01ea3179e9af8/superset-frontend/src/views/store.ts#L65 It does have some redux related code but it still reads data-bootstrap



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] zhaorui2022 commented on a diff in pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
zhaorui2022 commented on code in PR #21924:
URL: https://github.com/apache/superset/pull/21924#discussion_r1005055516


##########
superset-frontend/src/configs.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * 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 function configs() {
+  // read superset config using multiple sources
+  // currently it only reads from bootstrap data
+  return configsFromBootstrapData();
+}
+
+function configsFromBootstrapData() {

Review Comment:
   Updated



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] michael-s-molina commented on a diff in pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21924:
URL: https://github.com/apache/superset/pull/21924#discussion_r1005564578


##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -57,6 +44,7 @@ type RefreshIntervalModalProps = {
   editMode: boolean;
   refreshLimit?: number;
   refreshWarning: string | null;
+  refreshIntervalOptions: any;

Review Comment:
   ```suggestion
     refreshIntervalOptions: [[number, string]];
   ```



##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -99,13 +87,19 @@ class RefreshIntervalModal extends React.PureComponent<
   }
 
   handleFrequencyChange(value: number) {
+    const { refreshIntervalOptions } = this.props;
     this.setState({
-      refreshFrequency: value || options[0].value,
+      refreshFrequency: value || (refreshIntervalOptions[0][0] as number),

Review Comment:
   ```suggestion
         refreshFrequency: value || refreshIntervalOptions[0][0],
   ```



##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -120,7 +114,10 @@ class RefreshIntervalModal extends React.PureComponent<
             <FormLabel>{t('Refresh frequency')}</FormLabel>
             <Select
               ariaLabel={t('Refresh interval')}
-              options={options}
+              options={refreshIntervalOptions.map((o: any[]) => ({
+                value: o[0] as number,
+                label: t(o[1]),
+              }))}

Review Comment:
   ```suggestion
                 options={refreshIntervalOptions.map(option => ({
                   value: option[0],
                   label: t(option[1]),
                 }))}
   ```



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] github-actions[bot] commented on pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21924:
URL: https://github.com/apache/superset/pull/21924#issuecomment-1293339696

   @michael-s-molina Ephemeral environment spinning up at http://54.244.1.135:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] michael-s-molina commented on a diff in pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21924:
URL: https://github.com/apache/superset/pull/21924#discussion_r1004808935


##########
superset-frontend/src/configs.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * 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 function configs() {
+  // read superset config using multiple sources
+  // currently it only reads from bootstrap data
+  return configsFromBootstrapData();
+}
+
+function configsFromBootstrapData() {

Review Comment:
   I think there was an error with the previous link. I update it to point to https://github.com/apache/superset/blob/1388f21ee34251b6ef83beb009ba0901e4067848/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx#L352
   
   > So if I want to read bootstrap data in this RefreshIntervalModal which belongs to Dashboard, does that mean this file should contain some logic to save bootstrapData?
   
   You don't need to save `bootstrapData`, you just need to retrieve the values from Redux instead of parsing `data-bootstrap` again like you're doing in `configsFromBootstrapData`.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] michael-s-molina commented on a diff in pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21924:
URL: https://github.com/apache/superset/pull/21924#discussion_r1004400693


##########
superset-frontend/src/configs.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * 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 function configs() {
+  // read superset config using multiple sources
+  // currently it only reads from bootstrap data
+  return configsFromBootstrapData();
+}
+
+function configsFromBootstrapData() {

Review Comment:
   We generally get these configurations from Redux to only parse `data-bootstrap` once and also to have the ability to dynamically change the original settings and allow components to react accordingly.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] michael-s-molina merged pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
michael-s-molina merged PR #21924:
URL: https://github.com/apache/superset/pull/21924


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] michael-s-molina commented on pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #21924:
URL: https://github.com/apache/superset/pull/21924#issuecomment-1293332914

   /testenv up


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] github-actions[bot] commented on pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21924:
URL: https://github.com/apache/superset/pull/21924#issuecomment-1293390288

   Ephemeral environment shutdown and build artifacts deleted.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] michael-s-molina commented on a diff in pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21924:
URL: https://github.com/apache/superset/pull/21924#discussion_r1004779293


##########
superset-frontend/src/configs.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * 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 function configs() {
+  // read superset config using multiple sources
+  // currently it only reads from bootstrap data
+  return configsFromBootstrapData();
+}
+
+function configsFromBootstrapData() {

Review Comment:
   Your reference is correct. In this file, the `common` state will be initialized in the Redux store with contents from `data-bootstrap`. This means that you don't need to parse the content again, you can access it using Redux like [this](https://github.com/apache/superset/blob/1388f21ee34251b6ef83beb009ba0901e4067848/superset-frontend/src/SqlLab/components/TabbedSqlEditors/index.jsx#L352). If any component dynamically changes the value of that configuration, your component would be automatically updated.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] zhaorui2022 commented on a diff in pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
zhaorui2022 commented on code in PR #21924:
URL: https://github.com/apache/superset/pull/21924#discussion_r1004804503


##########
superset-frontend/src/configs.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * 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 function configs() {
+  // read superset config using multiple sources
+  // currently it only reads from bootstrap data
+  return configsFromBootstrapData();
+}
+
+function configsFromBootstrapData() {

Review Comment:
   I see. So if I want to read bootstrap data in this RefreshIntervalModal which belongs to Dashboard, does that mean this file should contain some logic to save bootstrapData? https://sourcegraph.com/github.com/apache/superset/-/blob/superset-frontend/src/dashboard/components/Dashboard.jsx In the file you linked in the previous comment, it reads from dashboardInfo but I don't see Dashboard.jsx saves the data. I'm wondering if I miss anything
   



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] michael-s-molina commented on a diff in pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21924:
URL: https://github.com/apache/superset/pull/21924#discussion_r1004816152


##########
superset-frontend/src/configs.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * 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 function configs() {
+  // read superset config using multiple sources
+  // currently it only reads from bootstrap data
+  return configsFromBootstrapData();
+}
+
+function configsFromBootstrapData() {

Review Comment:
   If you inspect the Redux state when you're inside any dashboard you'll see that the configuration values are already there.
   
   <img width="1785" alt="Screen Shot 2022-10-25 at 3 03 42 PM" src="https://user-images.githubusercontent.com/70410625/197848619-ddd6969d-9a8b-4b5b-9f01-f3444b587a41.png">
   



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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

[GitHub] [superset] zhaorui2022 commented on a diff in pull request #21924: feat(dashboard): Move dashboard auto refresh intervals options to config

Posted by GitBox <gi...@apache.org>.
zhaorui2022 commented on code in PR #21924:
URL: https://github.com/apache/superset/pull/21924#discussion_r1005027607


##########
superset-frontend/src/configs.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * 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 function configs() {
+  // read superset config using multiple sources
+  // currently it only reads from bootstrap data
+  return configsFromBootstrapData();
+}
+
+function configsFromBootstrapData() {

Review Comment:
   I see. I did a bit more research and I think I know how to retrieve the values. I feel I didn't make myself clear in the previous message as well. Actually I was wondering how those values were saved in redux at the first place.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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