-
Notifications
You must be signed in to change notification settings - Fork 8.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
[Security Solutions] Create all users tab on user page #128375
Conversation
b29a5c6
to
9eaddb9
Compare
9eaddb9
to
847ae05
Compare
skip?: boolean; | ||
/** | ||
* When the flag switches from `false` to `true`, it will abort any ongoing request. | ||
*/ | ||
abort?: boolean; |
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.
@stephmilovic I renamed it because it doesn't skip the request, but instead, it aborts any ongoing request.
if (error != null) { | ||
if (error != null && !(error instanceof AbortError)) { |
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.
It avoids showing annoying popups with errors when the request is aborted.
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 reviewed ✅ manual testing ✅ LGTM!
ec4c92f
to
50470bd
Compare
@elasticmachine merge upstream |
|
||
export const UsersTable = React.memo(UsersTableComponent); | ||
|
||
const getUsersColumns = (): UsersTableColumns => [ |
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.
shouldn't we place this const before the component definition, with the rowItems
?
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @machadoum |
⚪ Backport skippedThe pull request was not backported as there were no branches to backport to. If this is a mistake, please apply the desired version labels or run the backport tool manually. Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
}, | ||
]; | ||
|
||
const agg = { user_count: { cardinality: { field: 'user.name' } } }; |
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.
why is this aggregation stored in a const, but the other user_data
agg is hardcoded?
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.
added a couple of minor comments. tested locally and everything working as expected, data is displayed properly, useSearchStrategy with abort and query toggle is working well, and the code looks solid. LGTM! ✅
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Summary
Create 'All Users' tab on the User page. It also renames the authentication tab which was previously called 'All users'.
Checklist