Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: [search query view] edit link is broken #10128

Merged
merged 6 commits into from
Jun 25, 2020
Merged

fix: [search query view] edit link is broken #10128

merged 6 commits into from
Jun 25, 2020

Conversation

mistercrunch
Copy link
Member

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

Screen Shot 2020-06-21 at 9 56 06 PM

after
Screen Shot 2020-06-21 at 9 55 39 PM

TEST PLAN

Relying on existing tests, looking to add more coverage

mistercrunch
mistercrunch commented Jun 22, 2020
View reviewed changes
superset/app.py Outdated
@@ -241,14 +241,6 @@ def init_views(self) -> None:
category_label=__("Manage"),
category_icon="",
)
appbuilder.add_view(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to hide this as it is redundant with the search query page

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@villebro
Copy link
Member

FYI @mistercrunch needs a rebase

willbarrett
willbarrett requested changes Jun 24, 2020
View reviewed changes
Copy link
Member

@willbarrett willbarrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's split out the view removal from the bugfix. Two separate logical changes with different levels of risk.

superset/app.py Outdated
@@ -241,14 +241,6 @@ def init_views(self) -> None:
category_label=__("Manage"),
category_icon="",
)
appbuilder.add_view(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@mistercrunch
Copy link
Member Author

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

@willbarrett
Copy link
Member

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.

@mistercrunch mistercrunch force-pushed the fix_open_query_link branch from 80cbf91 to ad0cf1d Compare June 25, 2020 07:48
@mistercrunch
Copy link
Member Author

Realized digging deeper that /api/v1/query/{pk} exists! Thanks to @dpgaspar, using that instead.

villebro
villebro approved these changes Jun 25, 2020
View reviewed changes
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Just needed this today.

@mistercrunch
Copy link
Member Author

Related: #10162

@mistercrunch mistercrunch merged commit 1781ebb into apache:master Jun 25, 2020
@mistercrunch mistercrunch deleted the fix_open_query_link branch June 25, 2020 19:20
@nytai nytai mentioned this pull request Sep 23, 2020
Closed
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@mistercrunch @auxten
fix: [search query view] edit link is broken (apache#10128)
ec299bc 
* fix: [search query view] edit link is broken

* eslint + mypy

* rever app.py changes

* addressing comments

* use api/v1/query

* fix test
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willbarrett willbarrett willbarrett approved these changes

@villebro villebro villebro approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 0.37.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mistercrunch @villebro @willbarrett