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/22 04:57:05 UTC
[GitHub] [incubator-superset] mistercrunch opened a new pull request #10128: fix: [search query view] edit link is broken
mistercrunch opened a new pull request #10128:
URL: https://github.com/apache/incubator-superset/pull/10128
### SUMMARY
So somehow that link is using the `KV` model, and is broken when `KV_STORE = False`.
Why is that link using `KV` (the generic key/value model)? Well I think originally it just passed `?db_id={db_id}&sql={sql_string}`, but the SQL string can be too long as a GET param, so I'm guessing someone (maybe me, though I don't recall, didn't bother using `git blame` to see the history here), decided to push the query info to KV and reference it instead. Not sure why, probably to mimic the "share query" feature (?). Anyhow, here I decided to just pass the query id and let the frontend do the work.
Also did some minor cosmetic adjustements
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
**before**
<img width="635" alt="Screen Shot 2020-06-21 at 9 56 06 PM" src="https://user-images.githubusercontent.com/487433/85249963-07b20000-b40a-11ea-9e4e-cdac10f1783a.png">
**after**
<img width="632" alt="Screen Shot 2020-06-21 at 9 55 39 PM" src="https://user-images.githubusercontent.com/487433/85249964-084a9680-b40a-11ea-8dbc-994e54b32783.png">
### TEST PLAN
Relying on existing tests, looking to add more coverage
----------------------------------------------------------------
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] willbarrett commented on a change in pull request #10128: fix: [search query view] edit link is broken
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #10128:
URL: https://github.com/apache/incubator-superset/pull/10128#discussion_r445023599
##########
File path: superset/app.py
##########
@@ -241,14 +241,6 @@ def init_views(self) -> None:
category_label=__("Manage"),
category_icon="",
)
- appbuilder.add_view(
Review comment:
This changes a public API - are we sure this won't break anything? Should it go through deprecation before being removed? Could this be done as a separate PR from the bugfix?
----------------------------------------------------------------
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 commented on pull request #10128: fix: [search query view] edit link is broken
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10128:
URL: https://github.com/apache/incubator-superset/pull/10128#issuecomment-649328093
Realized digging deeper that `/api/v1/query/{pk}` exists! Thanks to @dpgaspar, using that instead.
----------------------------------------------------------------
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 #10128: fix: [search query view] edit link is broken
Posted by GitBox <gi...@apache.org>.
mistercrunch merged pull request #10128:
URL: https://github.com/apache/incubator-superset/pull/10128
----------------------------------------------------------------
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 commented on a change in pull request #10128: fix: [search query view] edit link is broken
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #10128:
URL: https://github.com/apache/incubator-superset/pull/10128#discussion_r443317142
##########
File path: superset/app.py
##########
@@ -241,14 +241,6 @@ def init_views(self) -> None:
category_label=__("Manage"),
category_icon="",
)
- appbuilder.add_view(
Review comment:
Decided to hide this as it is redundant with the search query page
----------------------------------------------------------------
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 commented on pull request #10128: fix: [search query view] edit link is broken
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10128:
URL: https://github.com/apache/incubator-superset/pull/10128#issuecomment-649334407
Related: https://github.com/apache/incubator-superset/pull/10162
----------------------------------------------------------------
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] willbarrett commented on pull request #10128: fix: [search query view] edit link is broken
Posted by GitBox <gi...@apache.org>.
willbarrett commented on pull request #10128:
URL: https://github.com/apache/incubator-superset/pull/10128#issuecomment-648983543
As you mention, extending the QueryView to retain the existing interface is simple - my vote would be to do that in this PR, then introduce another PR that changes the external API. I worry that a fix PR removing a publicly-available API won't receive the right level of scrutiny from organizations that might be leveraging it, and may be reverted later on.
----------------------------------------------------------------
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] villebro commented on pull request #10128: fix: [search query view] edit link is broken
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10128:
URL: https://github.com/apache/incubator-superset/pull/10128#issuecomment-648913210
FYI @mistercrunch needs a rebase
----------------------------------------------------------------
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 commented on pull request #10128: fix: [search query view] edit link is broken
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10128:
URL: https://github.com/apache/incubator-superset/pull/10128#issuecomment-648979706
@willbarrett the reason why I removed the view is that I'm polluting the list view with the `data` attribute. We need `/query/api/read` to show data, but we don't want the CRUD list view to show the blob.
Normally we would create a derivative `ModelView` (as in `class QueryRestModelView(QueryModelView)`) and override the list_columns.
I think it does make sense to kill the `QueryModelView` FAB CRUD as it is redundant to the query search view I'm fixing here, which is confusing.
I'm hoping we can make a call on `QueryModelView` here in this PR and move forward.
----------------------------------------------------------------
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