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/09/16 21:46:41 UTC

[GitHub] [incubator-superset] riahk opened a new pull request #10922: feat: update saved query backend routing + add savedquery list

riahk opened a new pull request #10922:
URL: https://github.com/apache/incubator-superset/pull/10922


   ### SUMMARY
   - [x] Add `list` route for Saved Queries, that serves React page when both `ENABLE_REACT_CRUD_VIEWS` and `SIP_34_DATABASE_UI` feature flags are enabled
   - [x] Add `SavedQueryList` skeleton with same `SubMenu` as other data pages and enable react routing functionality
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <img width="1267" alt="Screen Shot 2020-09-16 at 2 35 56 PM" src="https://user-images.githubusercontent.com/8216382/93394973-f1111e80-f829-11ea-85c2-06bd05fc45e1.png">
   
   ### TEST PLAN
   - [x] Add basic `SavedQueryList` unit tests
   
   ### ADDITIONAL INFORMATION
   - [ ] Has associated issue:
   - [x] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] 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.

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] [incubator-superset] nytai commented on a change in pull request #10922: feat: update saved query backend routing + add savedquery list

Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10922:
URL: https://github.com/apache/incubator-superset/pull/10922#discussion_r489806143



##########
File path: superset-frontend/src/views/CRUD/data/common.ts
##########
@@ -36,8 +36,8 @@ export const commonMenuData = {
     {
       name: 'Saved Queries',
       label: t('Saved Queries'),
-      url: '/sqllab/my_queries/',
-      usesRouter: false,
+      url: '/savedqueryview/list/',
+      usesRouter: true,

Review comment:
       Yup, since databases is already done this shouldn't be a problem. I'm planning to remove the `SIP_34_DATABASE_UI ` flag soon. This is mainly to avoid users landing on a blank/under construction page when `ENABLE_REACT_CRUD_VIEWS=True`. If you want to add a feature flag check here you're welcome to, but I'd say for now it's fine if it routes through the backend and we'll switch this on once the saved queries ui is done. 




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

[GitHub] [incubator-superset] nytai commented on a change in pull request #10922: feat: update saved query backend routing + add savedquery list

Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #10922:
URL: https://github.com/apache/incubator-superset/pull/10922#discussion_r489778259



##########
File path: superset-frontend/src/views/CRUD/data/common.ts
##########
@@ -36,8 +36,8 @@ export const commonMenuData = {
     {
       name: 'Saved Queries',
       label: t('Saved Queries'),
-      url: '/sqllab/my_queries/',
-      usesRouter: false,
+      url: '/savedqueryview/list/',
+      usesRouter: true,

Review comment:
       does this still work if the feature flags are not enabled? 

##########
File path: superset/views/sql_lab.py
##########
@@ -75,6 +76,17 @@ class SavedQueryView(
         "changed_on": _("Changed on"),
     }
 
+    @expose("/list/")
+    @has_access
+    def list(self) -> FlaskResponse:
+        if not (
+            app.config["ENABLE_REACT_CRUD_VIEWS"]
+            and feature_flag_manager.is_feature_enabled("SIP_34_DATABASE_UI")

Review comment:
       let's add a new feature flag specific to this one, maybe `SIP_34_SAVED_QUERIES_UI` 




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

[GitHub] [incubator-superset] riahk commented on a change in pull request #10922: feat: update saved query backend routing + add savedquery list

Posted by GitBox <gi...@apache.org>.
riahk commented on a change in pull request #10922:
URL: https://github.com/apache/incubator-superset/pull/10922#discussion_r489802911



##########
File path: superset-frontend/src/views/CRUD/data/common.ts
##########
@@ -36,8 +36,8 @@ export const commonMenuData = {
     {
       name: 'Saved Queries',
       label: t('Saved Queries'),
-      url: '/sqllab/my_queries/',
-      usesRouter: false,
+      url: '/savedqueryview/list/',
+      usesRouter: true,

Review comment:
       Turns out it does not, I think I misunderstood how we were using the feature flags. In which case the new Databases view feature flag also got broken with my last routing update




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

[GitHub] [incubator-superset] riahk commented on a change in pull request #10922: feat: update saved query backend routing + add savedquery list

Posted by GitBox <gi...@apache.org>.
riahk commented on a change in pull request #10922:
URL: https://github.com/apache/incubator-superset/pull/10922#discussion_r489804123



##########
File path: superset-frontend/src/views/CRUD/data/common.ts
##########
@@ -36,8 +36,8 @@ export const commonMenuData = {
     {
       name: 'Saved Queries',
       label: t('Saved Queries'),
-      url: '/sqllab/my_queries/',
-      usesRouter: false,
+      url: '/savedqueryview/list/',
+      usesRouter: true,

Review comment:
       To be clear, routing from the nav bar still works with the feature flag, it's just routing within the `SubMenu` that needs an extra feature flag check




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

[GitHub] [incubator-superset] mistercrunch merged pull request #10922: feat: update saved query backend routing + add savedquery list

Posted by GitBox <gi...@apache.org>.
mistercrunch merged pull request #10922:
URL: https://github.com/apache/incubator-superset/pull/10922


   


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