Register dashboards in entity title service. #19751
Conversation
There was a problem hiding this comment.
Looks good to me. Just nitpicks.
Imho, a solution that doesn't require a MongoDB view would be preferable. But as our entity suggestion mechanism seems to heavily rely on different entity types not sharing a common collection, the changes required to achieve that would be much more complex.
We should have tests, but I can't think of an easy way to test this. Maybe we can add a test when we actually use dashboards in suggestions?
| @@ -48,7 +50,9 @@ | |||
| @AutoValue | |||
| @JsonDeserialize(builder = ViewDTO.Builder.class) | |||
| @WithBeanGetter | |||
| @DbEntity(collection = "dashboards", readPermission = RestPermissions.DASHBOARDS_READ) | |||
There was a problem hiding this comment.
Could we add a comment here explaining that the entity is based on a MongoDB view? Without context, I think it is confusing that were are mentioning a different collection here than two lines below.
| @@ -48,7 +50,9 @@ | |||
| @AutoValue | |||
| @JsonDeserialize(builder = ViewDTO.Builder.class) | |||
| @WithBeanGetter | |||
| @DbEntity(collection = "dashboards", readPermission = RestPermissions.DASHBOARDS_READ) | |||
There was a problem hiding this comment.
Does this prevent us from adding an entity for saved searches? Maybe not a problem, but if we plan to add that any time soon, we should think about how that would work.
There was a problem hiding this comment.
Yes, I was also thinking about this. I took this as a chance to find out if it works and just implemented it in 927e2ec because it was trivial. Testing it for saved searches with a view that I created manually worked too.
There was a problem hiding this comment.
Moved this to a separate PR: #19761
dennisoelkers
force-pushed
the
feat/register-dashboards-in-entity-title-service
branch
from
21a0141
to
a02a71c
Compare
| this.api = api; | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
The test only starts for me if I apply the following change:
| @Test | |
| @ContainerMatrixTest |
| .post("/views") | ||
| .then() | ||
| .log().ifError() | ||
| .statusCode(201) |
There was a problem hiding this comment.
| .statusCode(201) | |
| .statusCode(200) |
| .log().ifValidationFails() | ||
| .assertThat() | ||
| .statusCode(200) | ||
| .body("pagination.total", equalTo(2)) |
There was a problem hiding this comment.
We also have the sources dashboard by default. So either we change the expectation to 3 and adjust the other assertions, or we remove the sources dashboard before running the test.
Description
Motivation and Context
This PR is registering dashboards in the entity title service, so they can be resolved (from id to title/listing titles) like other entities. For this, we need to create a
dashboardsview, which is just returning dashboard entities, as saved searches and dashboards are stored in the same collection./nocl No user-facing change.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: