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/08/03 20:45:52 UTC

[GitHub] [superset] eschutho opened a new pull request, #20965: fix: save dataset and repopulate state

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

   ### SUMMARY
   This PR fixes a bug in the sqllab to explore chart with a query flow where upon saving, there was an error saying that the form fields had not been transferred when they had. 
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   
   ### 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:
   - [ ] 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] hughhhh commented on a diff in pull request #20965: fix: save dataset and repopulate state

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


##########
superset/views/core.py:
##########
@@ -2135,7 +2135,12 @@ def sqllab_viz(self) -> FlaskResponse:  # pylint: disable=no-self-use
         table.columns = cols
         table.metrics = [SqlMetric(metric_name="count", expression="count(*)")]
         db.session.commit()
-        return json_success(json.dumps({"table_id": table.id}))
+
+        return json_success(
+            json.dumps(
+                {"table_id": table.id, "data": sanitize_datasource_data(table.data)}

Review Comment:
   πŸ‘ŒπŸΎ



-- 
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 #20965: fix: save dataset and repopulate state

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

   @yousoph Ephemeral environment spinning up at http://35.88.30.184: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] github-actions[bot] commented on pull request #20965: fix: save dataset and repopulate state

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

   @eschutho Ephemeral environment spinning up at http://34.219.161.252: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] eschutho merged pull request #20965: fix: save dataset and repopulate state

Posted by GitBox <gi...@apache.org>.
eschutho merged PR #20965:
URL: https://github.com/apache/superset/pull/20965


-- 
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] eschutho commented on a diff in pull request #20965: fix: save dataset and repopulate state

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


##########
superset-frontend/src/explore/actions/hydrateExplore.ts:
##########
@@ -119,7 +120,7 @@ export const hydrateExplore =
       controls: initialControls,
       form_data: initialFormData,
       slice: initialSlice,
-      controlsTransferred: [],
+      controlsTransferred: explore.controlsTransferred,

Review Comment:
   @michael-s-molina do you see any problems with us pulling in state here on hydrate? It's not a property that exists in the persisted db. 



-- 
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 #20965: fix: save dataset and repopulate state

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


##########
superset-frontend/src/explore/actions/hydrateExplore.ts:
##########
@@ -119,7 +120,7 @@ export const hydrateExplore =
       controls: initialControls,
       form_data: initialFormData,
       slice: initialSlice,
-      controlsTransferred: [],
+      controlsTransferred: explore.controlsTransferred,

Review Comment:
   Yes. The `hydrateExplore` is the action responsible for mounting the Explore state so it shouldn't have a reference to itself. It will only work because there's a known problem where the save action is firing `hydrateExplore` again but it shouldn't. We're calling `hydrateExplore` on save intentionally because during the SPA work we wanted to avoid reloading the page but many of the existing logic depended on that. It would require a major refactor to change this behavior. Currently, `explore.controlsTransferred` is being updated by `UPDATE_FORM_DATA_BY_DATASOURCE` action. You'll probably won't need all the logic inside that action, so I suggest creating a new action to update this piece of state and call this new action from your code.
   
   Ping me or @kgabryje if you need more assistance on this.



-- 
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 #20965: fix: save dataset and repopulate state

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

   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] eschutho commented on a diff in pull request #20965: fix: save dataset and repopulate state

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


##########
superset/models/helpers.py:
##########
@@ -1331,7 +1334,8 @@ def get_sqla_query(  # pylint: disable=too-many-arguments,too-many-locals,too-ma
 
         columns_by_name: Dict[str, "TableColumn"] = {
             col.get("column_name"): col
-            for col in self.columns  # col.column_name: col for col in self.columns
+            # col.column_name: col for col in self.columns
+            for col in self.columns

Review Comment:
   It looks like it's the end of a dictionary comprehension, not a for loop, so no colon needed. The comment change btw was just part of my automated linting on line length. I'm not actually sure what the comment means, too. :)



-- 
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] eschutho commented on pull request #20965: fix: save dataset and repopulate state

Posted by GitBox <gi...@apache.org>.
eschutho commented on PR #20965:
URL: https://github.com/apache/superset/pull/20965#issuecomment-1206644167

   /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] eschutho commented on a diff in pull request #20965: fix: save dataset and repopulate state

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


##########
superset-frontend/packages/superset-ui-core/src/api/types/core.ts:
##########
@@ -0,0 +1,31 @@
+/**
+ * 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.
+ */
+
+// /superset/sqllab_viz
+interface SqlLabPostRequest {
+  data: {
+    schema: string;
+    sql: string;
+    dbId: number;
+    templateParams?: string;
+    datasourceName: string;
+    metrics?: string[];
+    columns?: string[];
+  };
+}

Review Comment:
   This is a first attempt at creating a set of types of that map to the api. cc @hughhhh cc @eric-briscoe. We talked about auto-generating this with Marshmallow, but this seemed like a decent start. Thoughts?



-- 
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] eschutho commented on pull request #20965: fix: save dataset and repopulate state

Posted by GitBox <gi...@apache.org>.
eschutho commented on PR #20965:
URL: https://github.com/apache/superset/pull/20965#issuecomment-1205529413

   @yousoph thanks for catching that. I just pushed up a fix for the dashboard button too.


-- 
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 #20965: fix: save dataset and repopulate state

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

   # [Codecov](https://codecov.io/gh/apache/superset/pull/20965?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 [#20965](https://codecov.io/gh/apache/superset/pull/20965?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b1699a2) into [master](https://codecov.io/gh/apache/superset/commit/ac585821d8d6810d76a244a322cd1e24d15c265a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ac58582) will **decrease** coverage by `11.66%`.
   > The diff coverage is `57.14%`.
   
   > :exclamation: Current head b1699a2 differs from pull request most recent head 532561f. Consider uploading reports for the commit 532561f to get more accurate results
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #20965       +/-   ##
   ===========================================
   - Coverage   66.38%   54.71%   -11.67%     
   ===========================================
     Files        1766     1766               
     Lines       67224    67225        +1     
     Branches     7135     7137        +2     
   ===========================================
   - Hits        44625    36782     -7843     
   - Misses      20774    28617     +7843     
   - Partials     1825     1826        +1     
   ```
   
   | Flag | Coverage Ξ” | |
   |---|---|---|
   | hive | `?` | |
   | mysql | `?` | |
   | postgres | `?` | |
   | presto | `53.11% <100.00%> (+0.01%)` | :arrow_up: |
   | python | `57.48% <100.00%> (-24.09%)` | :arrow_down: |
   | sqlite | `?` | |
   | unit | `50.52% <0.00%> (ΓΈ)` | |
   
   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/20965?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Ξ” | |
   |---|---|---|
   | [...BigNumber/BigNumberWithTrendline/transformProps.ts](https://codecov.io/gh/apache/superset/pull/20965/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-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlcldpdGhUcmVuZGxpbmUvdHJhbnNmb3JtUHJvcHMudHM=) | `45.45% <0.00%> (-1.52%)` | :arrow_down: |
   | [...dashboard/components/AddSliceCard/AddSliceCard.tsx](https://codecov.io/gh/apache/superset/pull/20965/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0FkZFNsaWNlQ2FyZC9BZGRTbGljZUNhcmQudHN4) | `65.90% <0.00%> (-1.54%)` | :arrow_down: |
   | [...rset-frontend/src/explore/components/SaveModal.tsx](https://codecov.io/gh/apache/superset/pull/20965/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9TYXZlTW9kYWwudHN4) | `37.61% <0.00%> (ΓΈ)` | |
   | [...et-frontend/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/superset/pull/20965/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZXhwbG9yZVJlZHVjZXIuanM=) | `40.00% <0.00%> (ΓΈ)` | |
   | [superset-frontend/src/views/App.tsx](https://codecov.io/gh/apache/superset/pull/20965/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0FwcC50c3g=) | `0.00% <ΓΈ> (ΓΈ)` | |
   | [superset/views/sql\_lab.py](https://codecov.io/gh/apache/superset/pull/20965/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-c3VwZXJzZXQvdmlld3Mvc3FsX2xhYi5weQ==) | `56.57% <ΓΈ> (-3.95%)` | :arrow_down: |
   | [...set-frontend/src/explore/actions/exploreActions.ts](https://codecov.io/gh/apache/superset/pull/20965/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvYWN0aW9ucy9leHBsb3JlQWN0aW9ucy50cw==) | `58.33% <50.00%> (ΓΈ)` | |
   | [...frontend/src/explore/actions/datasourcesActions.ts](https://codecov.io/gh/apache/superset/pull/20965/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvYWN0aW9ucy9kYXRhc291cmNlc0FjdGlvbnMudHM=) | `100.00% <100.00%> (ΓΈ)` | |
   | [...set-frontend/src/explore/actions/hydrateExplore.ts](https://codecov.io/gh/apache/superset/pull/20965/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvYWN0aW9ucy9oeWRyYXRlRXhwbG9yZS50cw==) | `42.10% <100.00%> (ΓΈ)` | |
   | [...t-frontend/src/explore/actions/saveModalActions.js](https://codecov.io/gh/apache/superset/pull/20965/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvYWN0aW9ucy9zYXZlTW9kYWxBY3Rpb25zLmpz) | `98.33% <100.00%> (ΓΈ)` | |
   | ... and [298 more](https://codecov.io/gh/apache/superset/pull/20965/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: Codecov can now indicate which changes are the most critical in Pull Requests. [Learn more](https://about.codecov.io/product/feature/runtime-insights/?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] github-actions[bot] commented on pull request #20965: fix: save dataset and repopulate state

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

   @yousoph Ephemeral environment spinning up at http://35.167.113.206: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] hughhhh commented on a diff in pull request #20965: fix: save dataset and repopulate state

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


##########
superset/models/helpers.py:
##########
@@ -1749,12 +1753,20 @@ def get_sqla_query(  # pylint: disable=too-many-arguments,too-many-locals,too-ma
                 inner_time_filter = []
 
                 if dttm_col and not db_engine_spec.time_groupby_inline:
-                    inner_time_filter = [
-                        dttm_col.get_time_filter(
-                            inner_from_dttm or from_dttm,
-                            inner_to_dttm or to_dttm,
-                        )
-                    ]
+                    if isinstance(dttm_col, dict):

Review Comment:
   Use this pattern instead for grabbing the time_filter:
   https://github.com/preset-io/superset/blob/master/superset/models/helpers.py#L1550



-- 
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] yousoph commented on pull request #20965: fix: save dataset and repopulate state

Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #20965:
URL: https://github.com/apache/superset/pull/20965#issuecomment-1205614076

   /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] yousoph commented on pull request #20965: fix: save dataset and repopulate state

Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #20965:
URL: https://github.com/apache/superset/pull/20965#issuecomment-1204727888

   /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] hughhhh commented on a diff in pull request #20965: fix: save dataset and repopulate state

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


##########
superset/models/helpers.py:
##########
@@ -1331,7 +1334,8 @@ def get_sqla_query(  # pylint: disable=too-many-arguments,too-many-locals,too-ma
 
         columns_by_name: Dict[str, "TableColumn"] = {
             col.get("column_name"): col
-            for col in self.columns  # col.column_name: col for col in self.columns
+            # col.column_name: col for col in self.columns
+            for col in self.columns

Review Comment:
   shouldn't this be `for col in self.columns:` it's missing the colon



-- 
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] jinghua-qa commented on pull request #20965: fix: save dataset and repopulate state

Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20965:
URL: https://github.com/apache/superset/pull/20965#issuecomment-1206693929

   LGTM


-- 
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] yousoph commented on pull request #20965: fix: save dataset and repopulate state

Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #20965:
URL: https://github.com/apache/superset/pull/20965#issuecomment-1205577427

   /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 #20965: fix: save dataset and repopulate state

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

   @yousoph Ephemeral environment spinning up at http://54.244.60.192: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] yousoph commented on pull request #20965: fix: save dataset and repopulate state

Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #20965:
URL: https://github.com/apache/superset/pull/20965#issuecomment-1204738457

   ![image](https://user-images.githubusercontent.com/10627051/182762229-83252d87-b1cb-4562-9edf-796a6f54282b.png)
   Seeing that if a chart name and dashboard are selected but no dataset, the Save & Go to Dashboard button is enabled - it should be disabled when the Dataset name is empty


-- 
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] pkdotson commented on pull request #20965: fix: save dataset and repopulate state

Posted by GitBox <gi...@apache.org>.
pkdotson commented on PR #20965:
URL: https://github.com/apache/superset/pull/20965#issuecomment-1205914928

   looks good to me baring any of Michael's feedback! I'l approve!


-- 
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] pkdotson commented on a diff in pull request #20965: fix: save dataset and repopulate state

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


##########
superset-frontend/src/explore/actions/datasourcesActions.test.ts:
##########
@@ -83,3 +98,42 @@ test('change datasource action', () => {
     updateFormDataByDatasource(CURRENT_DATASOURCE, NEW_DATASOURCE),
   );
 });
+
+test('saveDataset handles success', async () => {
+  const datasource = { id: 1 };
+  const saveDatasetResponse = {
+    data: datasource,
+  };
+  fetchMock.reset();
+  fetchMock.post(saveDatasetEndpoint, saveDatasetResponse);
+  const dispatch = sinon.spy();
+  const getState = sinon.spy(() => ({ explore: { datasource } }));
+  const dataset = await saveDataset(SAVE_DATASET_POST_ARGS)(dispatch);

Review Comment:
   nice way to test this!



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