-
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
Changes edit view to json read-only view #112034
Changes edit view to json read-only view #112034
Conversation
…ts inspect functional tests
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.
Overall looking good. A few remarks and question. The most significant one is regarding what we want to do with editUrl
.
<Route path={'/:type/:id'} exact={true}> | ||
<RedirectToHomeIfUnauthorized> | ||
<Suspense fallback={<EuiLoadingSpinner />}> | ||
<SavedObjectsEditionPage | ||
coreStart={coreStart} |
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 see serviceRegistry
is still passed down to <SavedObjectsTablePage>
, to then be propagated to <Flyout>
, where it apparently no longer have any usage. Do we want to perform the final SOL cleanup in this PR or in a follow-up?
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.
Um, good point! I'll double check to make sure we're not using the serviceRegistry
in some sub-component and can probably remove it in this PR if we want to go that way (or at least remove it from the top most UI layer, meaning the <Flyout>
and <SavedObjectsTablePage>
).
We have an open issue to handle decoupling the SOM from the SOL, so maybe we should defer the cleanup to a different PR to play it safe?
I've realized small changes in one area have some unexpected downstream consequences (as with most things related to legacy code and saved objects 😆 ).
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 taken a stab at removing the serviceRegistry
up to a point in the top-most UI. I'd prefer to delay removing the rest to a follow up PR.
...defaultProps, | ||
canEdit: false, | ||
}); | ||
expect(mounted.find('h1').text()).toMatchInlineSnapshot(`"View search"`); | ||
expect(mounted.find('h1').text()).toMatchInlineSnapshot(`"Inspect saved object"`); |
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.
canEdit
is no longer used in the component, so I think we can remove the duplicate tests that were testing for title depending on it.
pageTitle={i18n.translate('savedObjectsManagement.view.inspectTitle', { | ||
defaultMessage: 'Inspect saved object', | ||
})} |
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 wonder if we should pass down the object
to the <Header>
component to display it's name in the title (obj.meta.title
)
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.
Do we need the whole object? What about just passing down the title if there is one, as I've done here?
<CodeEditor | ||
languageId={XJsonLang.ID} | ||
value={objectAsJsonString} | ||
onChange={() => {}} |
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.
NIT: is onChange
required? The prop seems optional and we're using readOnly:true
in the options
...ved_objects_management/public/management_section/object_view/saved_object_view.test.mocks.ts
Show resolved
Hide resolved
coreStart.application.navigateToUrl( | ||
coreStart.http.basePath.prepend( | ||
`/app/management/kibana/objects/${savedObject.type}/${savedObject.id}` | ||
) | ||
); |
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.
Are we sure no types were registering custom editUrls (allowing to edit the object via a page specific to the type instead of directing to /app/management/kibana/objects/{...}
)? We may be breaking things here.
Also, if we do know that all specified editUrl
were directing to the SOM edit page, we can now get rid of SavedObjectsTypeManagementDefinition.getEditUrl
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.
Are we sure no types were registering custom editUrls (allowing to edit the object via a page specific to the type instead of directing to /app/management/kibana/objects/{...})? We may be breaking things here.
@pgayvallet You're absolutely right about breaking things here, the functional tests needed to be updated and that should have made me question this! I quick search through the code shows that index-patterns
define editUrl: /management/kibana/indexPatterns/patterns/<id>
.
I was working towards the PoC, where the change to goInspectObject
was proposed.
@timroes can you remember why you proposed the change to the redirect?
The idea behind the change was to make all saved objects "Inspectable", not just ones that had the editUrl
defined.
If we need to keep the original implementation but also make other types 'Inspectable', would a combination of the two methods work?, e.g.
goInspectObject={(savedObject) => {
const { editUrl } = savedObject.meta;
if (editUrl) {
return coreStart.application.navigateToUrl(
coreStart.http.basePath.prepend(`/app${editUrl}`)
);
} else {
return coreStart.application.navigateToUrl(
coreStart.http.basePath.prepend(`/app/management/kibana/objects/${savedObject.type}/${savedObject.id}`)
);
}
}}
If not, do you have some other suggestion we could try?
Also, if we do know that all specified editUrl were directing to the SOM edit page, we can now get rid of SavedObjectsTypeManagementDefinition.getEditUrl
Removing SavedObjectsTypeManagementDefinition.getEditUrl
was also a remaining task in the PoC and I was planning on getting rid of that in a follow up PR.
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.
If we need to keep the original implementation but also make other types 'Inspectable', would a combination of the two methods work?
After syncing with @lukeelmers we realized we're using the loaders to get the saved object type using the custom editUrl
. Removing the deprecated SavedObjectLoader
means we'll need to get the actual SO type in some other way on the client side.
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.
Removing the deprecated SavedObjectLoader means we'll need to get the actual SO type in some other way on the client side.
Yeah this is the tricky part, basically there isn't a 1:1 relationship between the type
defined in client-side route, and the underlying SO type. (For example dashboard URLs in SOM have savedDashboard/:id
in the route). The flow looks something like this:
- A SO type has
getEditUrl
on the server, the returned value of which is included in the injectedMeta that's added to the response from the client's call to bulkGet - The edit URL is pulled from the response rendered in the list view
- When navigating to the edit url, we are using the provided
service
in the route params (which came from the injectedMeta) doing a lookup against the SOM service registry, which essentially provides a mapping between the id (savedDashboards
) and the underlying SO type (dashboards
) - The underlying SO type is then used to make the request to retrieve the actual object itself
This only works because it is relying on 2 things to be in sync:
- The service name that's in the path in the
editUrl
on the server must match the ID that's in the SOM service registry on the client - The SO type that's in the SOM service registry on the client must match the type that's used to register the SO on the server, which also must match the
type
included in the SO Loader
Trying to think of a way out of this, I see a couple options:
- Maintain a hardcoded list of mappings between the editUrl name & SO type on the client
- 👎 Implicit dependencies, no clear separation of concerns, not really better than status quo
- Expose a registry in SOM for plugins to register this info
- Basically the inverse of what we have today: other plugins would need to register with the SOM UI on the client to tell it about their SO types
- 👍 More explicit dependencies, clearer separation of concerns
- 👎 Adds a step to the process of adding a custom editUrl, because it wouldn't actually work unless you also registered this info on the client
- Find a way to get this info from the server back to the client, perhaps by having some sort of preflight call
- For this we'd probably create a route in SOM that returns a list of all available types and returns meta info such as their editUrls
- This would need to be called up front whenever SOM is mounted so the mapping could be cached and referenced during routing
- 👍 No added dependencies or extension points
- 👎 Additional round trip to the server (but probably just one)
- Only use editUrls for navigating outside of SOM (e.g. index patterns)
- If no editUrl is provided, we'd fallback to defining this route internally in SOM
- This means breaking existing routes so they are forced to do something like
app/management/kibana/objects/dashboards/123456
- 👍 Results in the simplest implementation
- 👎 Requires breaking change to routes (unless we build in some redirects for affected types: dashboards, vis, saved searches)
Overall I am torn on this. Options 3 and 4 are probably least-bad? WDYT @TinaHeiligers @pgayvallet, any other ideas?
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.
After chatting with @pgayvallet we've decided to go with option 4:
Only use editUrls for navigating outside of SOM (e.g. index patterns)
👍 What this means is that from the saved objects management UI, we'll be able to inspect (view the object) for all so types.
👎 The down side is that to get this to work, we'll need to remove the getEditUrl
property of the type registrations that were directing to the SOM edit page:
savedDashboards
savedVisualizations
savedSearches
This is a breaking change to routes but will allow us to decouple the plugin from the loaders.
The next question is: Do we introduce these changes now or wait for 8.0?
@timroes does this seem reasonable to you?
src/plugins/saved_objects_management/public/management_section/saved_objects_edition_page.tsx
Outdated
Show resolved
Hide resolved
test/functional/apps/saved_objects_management/edit_saved_object.ts
Outdated
Show resolved
Hide resolved
test/functional/apps/saved_objects_management/edit_saved_object.ts
Outdated
Show resolved
Hide resolved
x-pack/test/functional/apps/saved_objects_management/spaces_integration.ts
Outdated
Show resolved
Hide resolved
…eateDashboardSavedObjectType and visualizationSavedObjectType. Adds redirect to valid editUrl on Inspect if one is defined
); | ||
const { editUrl } = savedObject.meta; | ||
if (editUrl) { | ||
coreStart.application.navigateToUrl(coreStart.http.basePath.prepend(`/app${editUrl}`)); |
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.
Redirect to a custom editUrl
for editing in another app if the route is defined.
} else { | ||
coreStart.application.navigateToUrl( | ||
coreStart.http.basePath.prepend( | ||
`/app/management/kibana/objects/${savedObject.type}/${savedObject.id}` |
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.
Allows all saved objects to be viewed as read-only json.
@@ -27,9 +27,6 @@ export const createDashboardSavedObjectType = ({ | |||
getTitle(obj) { | |||
return obj.attributes.title; | |||
}, | |||
getEditUrl(obj) { |
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.
Allows us to decouple the loaders from the saved objects management page and still render the underlying saved object in the SOM Inspect view.
We were using the loaders to map from savedDashboards
back to dashboard
but we're aiming at removing the dependency on the loaders in the saved objects management plugin.
@@ -20,9 +20,6 @@ export const searchSavedObjectType: SavedObjectsType = { | |||
getTitle(obj) { | |||
return obj.attributes.title; | |||
}, | |||
getEditUrl(obj) { |
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.
Allows us to decouple the loaders from the saved objects management page and still render the underlying saved object in the SOM Inspect view.
@@ -20,9 +20,6 @@ export const visualizationSavedObjectType: SavedObjectsType = { | |||
getTitle(obj) { | |||
return obj.attributes.title; | |||
}, | |||
getEditUrl(obj) { |
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.
Allows us to decouple the loaders from the saved objects management page and still render the underlying saved object in the SOM Inspect view.
…ctions as true based on object type and id being present
@@ -298,7 +298,7 @@ export class Relationships extends Component<RelationshipsProps, RelationshipsSt | |||
icon: 'inspect', | |||
'data-test-subj': 'relationshipsTableAction-inspect', | |||
onClick: (object: SavedObjectWithMetadata) => goInspectObject(object), | |||
available: (object: SavedObjectWithMetadata) => !!object.meta.editUrl, | |||
available: (object: SavedObjectWithMetadata) => !!(object.type && object.id), |
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.
The inspect action is now available for all saved object types (we check that the object provided has a type and id to make sure that routing to the Inspect page will be valid). It used to only be available for saved objects that have an editUrl
defined but is not the case anymore.
@elasticmachine merge upstream |
dcd44bf
to
121283e
Compare
@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.
Presentation team changes LGTM! Code only review, and removing the editUrl makes a lot of sense.
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.
Sass changes LGTM
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsReferences to deprecated APIs
History
To update your PR or re-run it, just comment with: |
💔 Backport failed
To backport manually run: |
@lukeelmers we can't actually backport this to 7.16. It depends on removing support for legacy exports, which we decided not to backport. Did we paint ourselves into a corner here? |
@TinaHeiligers Ah, shoot. 😞 I think this means our only option is making a second version of this PR targeting In hindsight, what would have saved us from this would be splitting the PR into two phases: the first just updating the UI component to be read-only, and the second updating all of the plumbing to remove Edit: That is, assuming the app team's work that needs to coincide with this still lands in 7.16. |
Manual backports with conflicts are always painful, but that seems to be the only option. Note that we only need to preserve the FWIW, in that situation I would personally manually pick file by file from this PR's diff instead of trying to cherry pick commits (but that's just how I would be doing it, it's not necessarily the easiest/correct way) |
I reached out to the app team about their plans for removing the loaders and heard back that they're only targeting 8.0. They assumed that everything would stay as is in 7.16, keeping support for legacy exports and keeping the edit capability using the loaders. They consider the change from edit -> readonly view a breaking change, hence not implementing in 7.16. |
To add to that - if the core team decides to drop one of these features in 7.16, the vis editors team wouldn’t object; we are fine both ways. It’s just that I’m not seeing a good reason to drop the edit ui in 7.16 vs 8.0 - we need the SO loader anyway for legacy import and it would give the users a bit more time to change their Workflows away from the so edit UI. Jsut a suggestion, but what about keeping the edit UI in 7.16 with a big warning banner that it will be removed in 8.0? |
Adding a warning that the edit UI will change in 8.0 seems like a reasonable alternative. We could then also include the link to the saved objects API docs to get folks looking at using those rather than the UI. The API's have validation, something that's been sorely missing in the UI workflow. @elastic/kibana-core do you think we need to warn folks that editing from the UI is being removed in 8.0? It's a UI change and, IIRC, we thought of this as more of a feature than a breaking change. |
If the app team doesn't need this in 7.16, I see no reason why we should invest the time on manually backporting
TBH I am not sure if it's worth adding a warning. We agreed in the original issue #59588 (comment) that we didn't want to go out of our way to show users workarounds for manually changing their SOs due to the consequences of messing this up. We could add a general "this is going away" warning, but it probably would just annoy people if we aren't giving them a workaround. Just my two cents though, I don't have particularly strong feelings on this & would support adding a warning if others felt it was important (we'd just need to wordsmith it carefully) Edit: just found #112864, will move my comments there |
Actually it uncomfortable, because sometimes it was useful, to change some pieces of object directly in kibana editor. |
Resolves #59588
Historically, we needed a way to update index pattern references after importing visualizations and added a way to edit the raw JSON of a saved object.
Now that import handles updating index pattern references, we don't need to support editing raw JSON in the UI anymore.
Given that there wasn't any validation on the edits in the first place, removing the option to edit the raw JSON should help to reduce the chance of Kibana breaking and/or migration failures during upgrades.
This PR changes the Inspect view in saved objects management to a read-only JSON view that's available for all saved objects.
Table view:
Flyout view:
View with all privileges:
View with read-only privileges:
Other changes:
The following is a list of changes that should be handled as follow up issues. making the changes in this PR would make it more difficult to review.
Not done:
*_edit_*
to*_inspect_*
(this will make the PR more difficult to review)saved_objects_page
for functional tests (we no longer conditionally render the context menu, it is always available)Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
For maintainers
Release note
Changes saved objects management inspect view to a read-only json view of the whole saved object.