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

chore: Removes Saved Query old code #25007

Merged
merged 2 commits into from
Aug 18, 2023
Merged

chore: Removes Saved Query old code #25007

merged 2 commits into from
Aug 18, 2023

Conversation

michael-s-molina
Copy link
Member

SUMMARY

This PR removes old code related to the Saved Query module. This code was introduced in #7521 and it was later replaced by the new Saved Query module under SQL Lab.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

This is how Saved Queries were displayed back then
Screenshot 2023-08-16 at 14 54 55

This is the new Saved Query module
Screenshot 2023-08-16 at 14 55 22

TESTING INSTRUCTIONS

CI should be sufficient.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

dpgaspar
dpgaspar reviewed Aug 17, 2023
View reviewed changes
Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

can we remove SavedQueryView also, or at least of bit chunk of it?

@michael-s-molina
Copy link
Member Author

can we remove SavedQueryView also, or at least of bit chunk of it?

No. SavedQueryView is used to render the list of saved queries.

betodealmeida
betodealmeida approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Screenshot 2023-08-17 at 09-47-38 chore Removes Saved Query old code by michael-s-molina · Pull Request #25007 · apache_superset

john-bodley
john-bodley approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@john-bodley john-bodley left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up the code base.

@dpgaspar
Copy link
Member

can we remove SavedQueryView also, or at least of bit chunk of it?

No. SavedQueryView is used to render the list of saved queries.

right, but just for routing to savedqueryview/list then SavedQueryRestApi is the one that serves the necessary backend endpoints to render the React List CRUD view, it would nice to limit SavedQueryView exposed endpoints and attributes

@michael-s-molina
Copy link
Member Author

right, but just for routing to savedqueryview/list then SavedQueryRestApi is the one that serves the necessary backend endpoints to render the React List CRUD view, it would nice to limit SavedQueryView exposed endpoints and attributes

@dpgaspar this commit cleans up SavedQueryView

dpgaspar
dpgaspar reviewed Aug 18, 2023
View reviewed changes
superset/views/sql_lab/views.py
datamodel = SQLAInterface(SavedQuery)
include_route_methods = RouteMethod.CRUD_SET

class SavedQueryView(BaseSupersetView):
Copy link
Member

Choose a reason for hiding this comment

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

nice!!

dpgaspar
dpgaspar approved these changes Aug 18, 2023
View reviewed changes
Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for doing this!

@michael-s-molina michael-s-molina merged commit b428b06 into apache:master Aug 18, 2023
29 checks passed
sadpandajoe added a commit to preset-io/superset that referenced this pull request Aug 29, 2023
@sadpandajoe
Revert "chore: Removes Saved Query old code (apache#25007)"
da71322 
This reverts commit b428b06.
@sadpandajoe
Copy link
Member

@michael-s-molina found this issue that may be related to your PR.

michael-s-molina added a commit to michael-s-molina/superset that referenced this pull request Aug 30, 2023
@michael-s-molina
fix: Partially reverts apache#25007
c53a1bf
@michael-s-molina michael-s-molina mentioned this pull request Aug 30, 2023
Merged
9 tasks
@michael-s-molina
Copy link
Member Author

@michael-s-molina found this issue that may be related to your PR.

Thank you @sadpandajoe. I added a fix here.

michael-s-molina added a commit that referenced this pull request Aug 31, 2023
@michael-s-molina
fix: Partially reverts #25007 (#25124)
1144ac1
darwinsubramaniam pushed a commit to darwinsubramaniam/superset that referenced this pull request Sep 7, 2023
@michael-s-molina @darwinsubramaniam
fix: Partially reverts apache#25007 (apache#25124)
2289386
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@michael-s-molina
fix: Partially reverts apache#25007 (apache#25124)
9d9222e
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betodealmeida betodealmeida betodealmeida approved these changes

@dpgaspar dpgaspar dpgaspar approved these changes

@john-bodley john-bodley john-bodley 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/L 🚢 3.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@michael-s-molina @dpgaspar @sadpandajoe @betodealmeida @john-bodley @mistercrunch