-
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
[Cases] User profile hooks #137830
[Cases] User profile hooks #137830
Conversation
* User profiles | ||
*/ | ||
|
||
export const DEFAULT_USER_SIZE = 100; |
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.
What do you think about defaulting it to 10? I believe that's what the Elasticsearch default is. 100 seems like it could result in a pretty long list.
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.
Ok! Sounds good to me.
uids, | ||
}: BulkGetUserProfilesArgs): Promise<UserProfile[]> => { | ||
if (security == null) { | ||
return Promise.resolve([]); |
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 it give us an error if we just do return []
? I thought async functions would automatically wrap the return value in a promise 🤔
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.
Oh, you are right. I missed that.
}); | ||
}, | ||
{ | ||
enabled: !isEmpty(debouncedName), |
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.
hmm does this mean we won't show anything if the user hasn't typed anything? I wonder what Elasticsearch will return if we don't send in a name
, maybe the most recently logged in users? I can test that.
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, that means that the query will not run if the user hasn't typed anything. You are right. Let's test it together offline and see what the results will be.
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.
Ok, I tested it and ES returns a suggestion of users even with an empty string. I removed the enabled
to simulate the same behavior.
@@ -215,11 +215,11 @@ export const getManagementFilteredLinks = async ( | |||
plugins: StartPlugins | |||
): Promise<LinkItem> => { | |||
try { | |||
const currentUserResponse = await plugins.security.authc.getCurrentUser(); | |||
const currentUserResponse = await plugins.security?.authc.getCurrentUser(); |
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: It turns out that the security plugin will always be defined even if security is disabled within Elasticsearch. I think we could probably leave this the way it was.
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 is that possible? I thought that if you put it in the kibana.json
as optional then it maybe not be there.
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.
Yeah that's what I thought too, but it's no longer the case. I think it's because you can't actually disable the security plugin itself. It is always enabled. You can disable security though, but it is now done within Elasticsearch directly. So for example if you attempt to call the suggestions API, Elasticsearch will throw a 500 I think saying that the API does not exist.
That's why we now have to check for the license to determine if security is disabled: https://github.com/elastic/kibana/blob/main/x-pack/plugins/cases/server/services/user_profiles/index.ts#L109
#137861 -> I still need to clean up the undefined
checks we have in cases.
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 explanation. I reverted back.
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/response-ops-cases (Feature:Cases) |
@elasticmachine merge upstream |
…o suggestion_api_query
daf1989
to
a036f26
Compare
@jonathan-buttner I made the |
@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.
LGTM
…o suggestion_api_query
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
History
To update your PR or re-run it, just comment with: cc @cnasikas |
Summary
This PR created user profile hooks. One for the suggest API and one for the bulk get API.
Depends on: #137346
Checklist
Delete any items that are not applicable to this PR.
For maintainers