Skip to content
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 Solution][Entity details] - move managed user code to flyout folder #190107

Merged
merged 3 commits into from
Aug 23, 2024

Conversation

PhilippeOberti
Copy link
Contributor

Summary

This PR continues the effort in removing all old flyout components, hooks and utils functions. It moves all the code related to ManagedUsers hooks, components, types and constants from the timeline side_panel folder to the alert details expandable flyout folder

The goal of this PR is to:

  • move the code and update the imports
  • add unit tests if they were not present

The purpose of the PR was never to full refactor the code, so things that aren't necessarily super clean might still be present.

No UI changes should be visible whatsoever!

Checklist

https://github.com/elastic/security-team/issues/7462

@PhilippeOberti PhilippeOberti added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.16.0 labels Aug 8, 2024
@PhilippeOberti PhilippeOberti force-pushed the manage-user-to-flyout branch 2 times, most recently from 22d5a6f to b076821 Compare August 8, 2024 10:17
@PhilippeOberti
Copy link
Contributor Author

buildkite test this

@PhilippeOberti PhilippeOberti marked this pull request as ready for review August 8, 2024 10:25
@PhilippeOberti PhilippeOberti requested review from a team as code owners August 8, 2024 10:25
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations)

@@ -10,7 +10,7 @@ import { useObservedHostFields } from './use_observed_host_fields';
import { mockObservedHostData } from '../../mocks';
import { TestProviders } from '../../../../common/mock';

describe('useManagedUserItems', () => {
describe('useObservedHostFields', () => {
it('returns managed user items for Entra user', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe update the text below to reference host too?
it('returns managed host items for Entra host', () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

* 2.0.
*/

export const INSTALL_EA_INTEGRATIONS_HREF = `browse/security?q=entityanalytics`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be merged with the other constants in shared folder

Copy link
Contributor Author

@PhilippeOberti PhilippeOberti Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason I kept it separate is because this constant is used in a single place within the user_right folder, while the others are used in both user_right and user_left (if I'm not mistaken)

@PhilippeOberti PhilippeOberti force-pushed the manage-user-to-flyout branch 3 times, most recently from 553e2a4 to 364c45d Compare August 19, 2024 13:28
Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

Thank you, Philippe, for moving the files and improving the code! 🙇‍♂️

I left a minor comment. Please take a look.

});

export const FAIL_MANAGED_USER = i18n.translate(
'xpack.securitySolution.timeline.userDetails.failManagedUserDescription',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should rename the translation keys in this file because they still reference the timeline.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup I had missed those. I fixed them as well as another translation file I moved!

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #15 / discover/context_awareness extension getDefaultAppState data view mode should merge and dedup configured default columns with default profile columns

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 5676 5677 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 18.0MB 18.0MB -1.7KB

History

  • 💔 Build #229562 failed 9ba2d40d8c394dbaf13e667d90934d4dcf52cc44
  • 💚 Build #228241 succeeded 364c45d8f1899c7b05e264a33965d257da1f7717
  • 💔 Build #228177 failed 553e2a4b722143cba286aa1cb7eb2d5397b77655
  • 💔 Build #228130 failed 30b6929a16c0e23e2ed87556d4c81e31a102b9ef
  • 💛 Build #226476 was flaky b0768216b7e881d3e5265e0088af8afcd24241d9

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@PhilippeOberti PhilippeOberti merged commit 168f564 into elastic:main Aug 23, 2024
42 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 23, 2024
@PhilippeOberti PhilippeOberti deleted the manage-user-to-flyout branch August 23, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants