-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Dashboard] Use SavedObjectResolve #111040
Conversation
4fe0a26
to
d98ad63
Compare
Pinging @elastic/kibana-presentation (Team:Presentation) |
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.
Tested this locally, and everything works as expected. Went over the code again, and couldn't find anything to comment on! It's a clever solution for keeping the Saved Objects class in there for the time being - can't wait until we migrate away. LGTM!
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.
A couple of minor problems need to be addressed, but overall this looks good!
Perhaps a dumb question, but I noticed that if you navigate to a dashboard with a full URL ( /app/dashboards#/view/foo?_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now))
), when you get redirected, it drops the _g
query parameter and then the URL quickly changes to show it again.
I'm not that familiar with how the global filter works, but is that reliable behavior? Or, should we think about changing the redirect so it includes query params?
if (screenshotModeService?.isScreenshotMode()) { | ||
scopedHistory().replace(savedDashboard.getFullPath()); | ||
} else { | ||
spacesService?.ui.redirectLegacyUrl(savedDashboard.getFullPath()); |
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.
This uses navigateToApp
with the current appId
under the hood.
That means that when you go to this URL:
/s/test2/app/dashboards#/view/7c0a5590-10e5-11ec-8e89-e5edd366fc1d-test2
Kibana detects the aliasMatch outcome and redirects you here:
/s/test2/app/dashboards/app/dashboards#/view/7c0a5590-10e5-11ec-8e89-e5edd366fc1d-test2-newId
So you're actually at the /app/dashboards/app/dashboards
page, which actually loads because of a quirk in our router, but we should change it so we don't have the app show up in the path twice 😅
Perhaps you can expose a getLocalPath()
method?
if (screenshotModeService?.isScreenshotMode()) { | ||
scopedHistory().replace(savedDashboard.getFullPath()); |
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.
How can I test this? When I load the dashboard and click the "Full screen" button, it doesn't add anything to the URL. And obviously by the point the page has loaded for an aliasMatch, if we aren't already in screenshot mode, the other redirect has already happened.
Do I actually need to generate a report to exercise this?
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.
Screenshot mode is a standalone plugin. There's a method on it to set whether you are in screenshot mode or not. Reporting sets it when it's doing its thing so you can generate a report.
Or, I think you can fake it by setting this key in localStorage __KBN_SCREENSHOT_MODE_ENABLED_KEY__
to 'true'
? spacesService?.ui.components.getLegacyUrlConflict({ | ||
currentObjectId: dashboardAppState.savedDashboard.id, | ||
otherObjectId: dashboardAppState.savedDashboard.aliasId, | ||
otherObjectPath: dashboardAppState.savedDashboard.getFullPath(), |
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.
Similar to my other comment about the app path --
If I navigate here:
/s/test3/app/dashboards#/view/7c0a5590-10e5-11ec-8e89-e5edd366fc1d-test3
Then I click the "Go to other object" button, I get redirected here:
/joe/s/test3/app/dashboards/app/dashboards#/view/7c0a5590-10e5-11ec-8e89-e5edd366fc1d-test3-newId
So I think we want to use the local path, not the full path here.
33e8fdb
to
382712f
Compare
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
I've encountered two issues while testing this locally.
App State
I tested the redirect using global state and app state, because a lot of users use URLs with both and expect everything to be forwarded correctly. Add the following state onto the ends of the test URLs:
?_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-25m,to:now))&_a=(query:(language:kuery,query:'test%20me%20please'))
The time picker should read from 25 minutes ago until now, and the query bar should read "test me please". This works correctly with test1, but test2 forwards the global state, and not the app state, so the query bar is not properly populated.
Conflict on default index pattern
I'm not entirely sure if this is a problem with your PR, or a caveat of the test data, but when creating a new dashboard, the query bar and filters disappear, and I get an error that reads DataViewSavedObjectConflictError
. I think this is the default index pattern having a conflict. Lens entirely fails to open as well. I wonder if there is a plan in place to handle conflicts like this @jportner
Yeah, so:
You can test with this subset of the staging data provided above, which will not result in any index pattern conflicts:
|
Amazing, thank you for the explanation, Joe! So it looks like only the one problem of forwarding App state is remaining in this PR - will re-review once it's added in! |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
I've fixed the issue with carrying through url params for the auto redirect case, but still struggling to get it working for the conflict banner case. I'm going to go ahead with this and follow up with an issue outlining what's going on with that conflict case. |
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.
Tested again, all is working great!
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
* Switch Dashboard to use savedobjects.resolve when loading * Don't use LegacyURI Redirect if in screenshot mode * Pass query string on redirects * Remove unused import * Fix carrying query params through redirect Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
* Switch Dashboard to use savedobjects.resolve when loading * Don't use LegacyURI Redirect if in screenshot mode * Pass query string on redirects * Remove unused import * Fix carrying query params through redirect Co-authored-by: Kibana Machine <[email protected]>
* [Dashboard] Use SavedObjectResolve (#111040) * Switch Dashboard to use savedobjects.resolve when loading * Don't use LegacyURI Redirect if in screenshot mode * Pass query string on redirects * Remove unused import * Fix carrying query params through redirect Co-authored-by: Kibana Machine <[email protected]> * Fix issue with importing json dashboards Co-authored-by: Kibana Machine <[email protected]>
This PR makes Dashboards Share Capable (part of #100489)
Some important highlights of changes here.
Changes the saved_dashboard class to use
savedObjectsClient.resolve
if requested. Will also store the outcome and the aliasId on the resulting saved dashboard object.An Alias Match will redirect and show the notification toast to let the user know the object has moved
A conflict match will show the conflict banner.
Testing
To setup your environment for testing, install the ecommerce sample data and then run the contents of this file in dev tools
resolve-staging-data.txt
What to test?
/s/test1/app/dashboards#/view/7c0a5590-10e5-11ec-8e89-e5edd366fc1d-test1
should load normally/s/test2/app/dashboards#/view/7c0a5590-10e5-11ec-8e89-e5edd366fc1d-test2
should load and redirect to/s/test2/app/dashboards#/view/7c0a5590-10e5-11ec-8e89-e5edd366fc1d-test2-newid
and you should see the "We redirected you toast"/s/test3/app/dashboards#/view/7c0a5590-10e5-11ec-8e89-e5edd366fc1d-test3
should load and show the Conflict Banner. The conflict banner should not show when you go to full screen mode.7c0a5590-10e5-11ec-8e89-e5edd366fc1d-test2
should no have the "Redirected" toast show up on the report.