-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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.
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
fix: Revert to old endpoint temporarily to enable sharing saved queries #24434
fix: Revert to old endpoint temporarily to enable sharing saved queries #24434
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24434 +/- ##
==========================================
- Coverage 68.93% 68.93% -0.01%
==========================================
Files 1904 1904
Lines 73879 73879
Branches 8119 8119
==========================================
- Hits 50928 50927 -1
- Misses 20840 20841 +1
Partials 2111 2111
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -1372,12 +1372,11 @@ export function popStoredQuery(urlId) { | |||
export function popSavedQuery(saveQueryId) { | |||
return function (dispatch) { | |||
return SupersetClient.get({ | |||
endpoint: `/api/v1/saved_query/${saveQueryId}`, | |||
endpoint: `/savedqueryviewapi/api/get/${saveQueryId}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfrag1 thanks for the change. Is there any reason you opted for this approach as opposed to keeping the new endpoint and relaxing the SavedQueryFilter
filter criterion? I guess the merit with the later is it's clearer how this works as opposed to the somewhat magical (and obscure) FAB REST API endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@john-bodley The main reason I did it this way is that I'm pretty sure there are several FAB-generated endpoints under the SavedQueryRestApi
that utilize the SavedQueryFilter
(set as the base_filter
on the class), and I didn't want to change the behavior for those. For example, the endpoint that fetches a list of a user's saved queries could start returning a list of all saved queries with that change.
…es (apache#24434) (cherry picked from commit 86ce440)
SUMMARY
This PR is an alternative option to #24301 for re-enabling sharing saved queries; it simply reverts the change that broke it.
The long term fix for this will depend on the outcome of a future SIP
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION