-
Notifications
You must be signed in to change notification settings - Fork 1.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
Register dashboards in entity title service. #19751
Register dashboards in entity title service. #19751
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
✅
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this to a separate PR: #19761
21a0141
to
a02a71c
Compare
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.
Code looks good.
Thanks for adding the test! It's failing though and needs a bit of work to get it passing.
this.api = api; | ||
} | ||
|
||
@Test |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.statusCode(201) | |
.statusCode(200) |
.log().ifValidationFails() | ||
.assertThat() | ||
.statusCode(200) | ||
.body("pagination.total", equalTo(2)) |
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.
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
dashboards
view, 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: