-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
chore(explore): Update chart save to use API endpoints #20498
chore(explore): Update chart save to use API endpoints #20498
Conversation
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.
Thanks for the PR! I left some first-pass comments.
Waiting on inclusion of Explore in SPA so redirection is no longer necessary and toasts appear. |
10ab7aa
to
622e053
Compare
437f5b7
to
faa7bf4
Compare
faa7bf4
to
d971b93
Compare
Codecov Report
@@ Coverage Diff @@
## master #20498 +/- ##
==========================================
- Coverage 66.85% 66.83% -0.02%
==========================================
Files 1753 1753
Lines 65833 65896 +63
Branches 7007 7013 +6
==========================================
+ Hits 44011 44044 +33
- Misses 20036 20069 +33
+ Partials 1786 1783 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
876b912
to
b6aed8c
Compare
Can we get rid of page refresh when you overwrite the chart? I removed Ideally we could also prevent redirecting when saving as a new chart, but that might be trickier... |
5a0e0f9
to
28406e7
Compare
Bugs found in manual testing:
|
@andrey-zayats I thought I had fixed it but I guess not :( will check it out and let you know when it's resolved, thanks! |
/testenv up |
@jinghua-qa Ephemeral environment spinning up at http://35.89.223.157:8080. Credentials are |
I have found an issue that when add a new chart from explore to the dashboard and apply native filter to the dashboard, the filter indicator is not appear Screen.Recording.2022-07-10.at.10.47.40.PM.mov |
@jinghua-qa Thanks, will look into this ASAP! |
Verified this issue is also exist in master, not related to this PR. So LGTM! |
2811945
to
fcb00d0
Compare
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
When a user saves a chart in Explore, the client sends one
POST /explore
request, then the server creates/updates the chart and optionally adds it to a new/existing dashboard as indicated. This PR updates Explore'sSaveModal
to instead use v1 API endpoints to accomplish the same. Here's the updated sequence of API calls:POST /api/v1/dashboard/
PUT /api/v1/dashboard/{dashboard_id}
POST /api/v1/chart/
{ dashboards: [..., dashboard_id] }
PUT /api/v1/chart/{chart_id}
{ dashboards: [..., dashboardId] }
In addition, this PR makes the Save and Save and Go to Dashboard buttons no longer trigger a page refresh.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Try performing the following tasks with and without proposed update and ensure that interface and user experience are identical (with the exception of there no longer being page refreshes):
ADDITIONAL INFORMATION