-
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
[Security Solution][Detections] Related Integrations & Required Fields Feedback & Fixes #133050
Conversation
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
…Rule Details if empty
…he Rules and Monitoring tables
I agree with the badge colour usage here to be consistent with what we use in ML Job enabled/disabled UI. On the different statuses itself though I wonder if it makes sense to combine the
|
@spong I’m on board with @yiyang’s suggestions as well. My only other thought/question is whether it makes sense to use |
…s installed and enabled badges
Thanks for the feedback @yiyangliu9286 and @jethr0null! 🙂 I've combined the |
...ution/public/detections/components/rules/related_integrations/use_installed_integrations.tsx
Outdated
Show resolved
Hide resolved
While testing locally, found a few bugs. Will post these right away and continue testing and reviewing the code. Version mismatch icon's color is not readableWhen the dark mode is off: Links include a semver requirement instead of a target versionLooks like it happens ndjson for repro:
|
Thanks @banderror! Should be resolved as of b0ba541. I've updated the icon color to be There might still be another bug in here with the version calculation, so will finish adding the remaining tests and push those next 👍 |
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.
Almost there! Reviewed most of the changes and have some suggestions, so please check the comments I left @spong.
Still have to carefully scan the getInstalledRelatedIntegrations
logic and review the test coverage. Will do it tomorrow morning.
...lic/detections/components/rules/related_integrations/integrations_description/index.test.tsx
Show resolved
Hide resolved
...lic/detections/components/rules/related_integrations/integrations_description/index.test.tsx
Show resolved
Hide resolved
...lic/detections/components/rules/related_integrations/integrations_description/index.test.tsx
Show resolved
Hide resolved
...lic/detections/components/rules/related_integrations/integrations_description/index.test.tsx
Show resolved
Hide resolved
export const integrationDetailsUninstalled: IntegrationDetails = { | ||
package_name: 'test', | ||
package_title: 'Test', | ||
package_version: '1.2.3', | ||
integration_name: 'integration', | ||
integration_title: 'Integration', | ||
is_enabled: false, | ||
is_installed: false, | ||
target_version: '1.2.3', | ||
version_satisfied: false, | ||
}; | ||
|
||
export const integrationDetailsInstalled: IntegrationDetails = { | ||
package_name: 'test', | ||
package_title: 'Test', | ||
package_version: '1.2.3', | ||
integration_name: 'integration', | ||
integration_title: 'Integration', | ||
is_enabled: false, | ||
is_installed: true, | ||
target_version: '1.2.3', | ||
version_satisfied: true, | ||
}; | ||
|
||
export const integrationDetailsEnabled: IntegrationDetails = { | ||
package_name: 'test', | ||
package_title: 'Test', | ||
package_version: '1.1.3', | ||
integration_name: 'integration', | ||
integration_title: 'Integration', | ||
is_enabled: true, | ||
is_installed: true, | ||
target_version: '1.3.3', | ||
version_satisfied: false, | ||
}; |
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.
Can we have an integration_details.ts
file and then move this mock data to integration_details.mock.ts
?
...ns/security_solution/public/detections/components/rules/related_integrations/translations.ts
Show resolved
Hide resolved
.../plugins/security_solution/public/detections/components/rules/related_integrations/utils.tsx
Show resolved
Hide resolved
.../plugins/security_solution/public/detections/components/rules/related_integrations/utils.tsx
Show resolved
Hide resolved
.../plugins/security_solution/public/detections/components/rules/related_integrations/utils.tsx
Show resolved
Hide resolved
💛 Build succeeded, but was flakyFailed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @spong |
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.
Last comments! Finished reviewing the code changes and tested it once again.
Couldn't find any new bugs, and the two bugs I reported above are fixed! 🙌
), | ||
type: 'boolean', | ||
category: [APP_ID], | ||
requiresPageReload: true, |
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.
When I disabled this setting and navigated to the Rules page without doing a page reload, it still worked (the column got hidden). Can we set requiresPageReload: false
here?
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 we can change to false
-- I had tested in multiple tabs and noticed the opposite behavior so had set as true
, but this appears to be a cache issue.
: integration.target_version; | ||
|
||
const integrationURL = | ||
version !== '' |
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.
In which case version
can be an empty string?
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's the error case when semver.valid(semver.coerce(i.version))
returns null
, so we will navigate to just the latest package version (redirected by fleet) in those instances. That said, as we saw, we still need to include the integrationName
if there is once so it doesn't just drop the user off on the base integration page.
// Version check e.g. fleet match `1.2.3` satisfies rule dependency `~1.2.1` | ||
const versionSatisfied = semver.satisfies(match.package_version, i.version); | ||
const packageVersion = versionSatisfied | ||
? i.version | ||
: semver.valid(semver.coerce(i.version)) ?? ''; |
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.
I'm thinking - if the version requirement is satisfied, shouldn't we use the installed version? Here in the code, it would be match.package_version
:
const targetVersion = versionSatisfied
? match.package_version
: semver.valid(semver.coerce(i.version)) ?? '';
i.version
here is the related integration's version
which is ~1.2.3
etc.
So to me, it looks like a bug, but it works as expected locally and there are some unit tests that seem to cover this code path, so I'm not sure about my sanity 😂
return integrationDetails.sort((a, b) => { | ||
if (a.integration_title != null && b.integration_title != null) { | ||
return a.integration_title.localeCompare(b.integration_title); | ||
} else if (a.integration_title != null) { | ||
return a.integration_title.localeCompare(b.package_title); | ||
} else if (b.integration_title != null) { | ||
return a.package_title.localeCompare(b.integration_title); | ||
} else { | ||
return a.package_title.localeCompare(b.package_title); | ||
} | ||
}); |
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: Could this do the same?
return integrationDetails.sort((a, b) => {
const aTitle = a.integration_title ?? a.package_title;
const bTitle = b.integration_title ?? b.package_title;
return aTitle.localeCompare(bTitle);
});
Also, if we always set integration_title
in IntegrationDetails
, could be even simpler:
return integrationDetails.sort((a, b) => {
return a.integration_title.localeCompare(b.integration_title);
});
Then, in getIntegrationLink
we wouldn't need to do ?? in const integrationTitle = integration.integration_title ?? integration.package_title;
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 I think we can always set integrationTitle
to clean this up and then keep integrationName
undefined
if it's a single integration package (i.e. system
package).
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.
Synced on the comments with @spong and decided to address them in a follow-up PR.
🚀 🚀 🚀
💔 All backports failed
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
…s Feedback & Fixes (elastic#133050) ## Summary Addressing the following feedback from elastic#132847: - [X] On the Rule Management page package name is used instead of package title when the package has only 1 integration: - [X] move integrations_popover to `related_integrations` directory - [X] update useInstalledIntegrations to always return array of installedIntegration - [X] move useInstalledIntegrations to `related_integrations` directory - [X] Slight update to copy in Rules Table popover - [X] Ok to use Rule Details UI within Rules Table popover content - [X] Sort integrations alphabetically - [X] Update left padding on version mis-match tooltip - [X] elastic#133291 - [X] elastic#133269 - [X] Add Kibana Advanced Setting for disabling integrations badge on Rules Table <p align="center"> <img width="500" src="https://user-images.githubusercontent.com/2946766/171750790-ffa2e3ef-dd7a-499c-9b08-89bafc06dd50.png" /> </p> - [ ] Adds tests - [x] `useInstalledIntegrations` hook - [X] relatedIntegrations utils - [x] IntegrationDescription - [ ] Add loaders where necessary since there can now be API delay - May hold off on loaders as transition from no installed integrations -> installed integrations isn't too bad as-is ##### Updated integrations popover content on Rules Table to match Rule Details design <p align="center"> <img width="500" src="https://user-images.githubusercontent.com/2946766/172263941-3e948b41-7ef7-4281-8354-57e77ddeb433.png" /> </p> In discussions with @banderror reviewing the different integration states (uninstalled, installed, enabled, and agents enrolled), we are now capturing the distinction between `Installed` and `Enabled` so that we don't confuse users when a package is installed but the integration isn't enabled/configured. I also added tooltips for clarifying each state and what action the user should perform to ensure compatibility. In collab with @yiyangliu9286 @jethr0null (comments below) -- we've consolidated to a single `Installed: enabled` badge, and updated `Uninstalled` to `Not installed` as well. ##### Tooltips <details><summary>Not installed</summary> <p align="center"> <img width="500" src="https://user-images.githubusercontent.com/2946766/172264210-00064485-2df9-408e-953b-9294f16dedf2.png" /> </p> </details> <details><summary>Installed</summary> <p align="center"> <img width="500" src="https://user-images.githubusercontent.com/2946766/172263672-67b641cd-5895-464a-8897-f26bd0a61073.png" /> </p> </details> <details><summary>Installed: enabled</summary> <p align="center"> <img width="500" src="https://user-images.githubusercontent.com/2946766/172263783-563ea48d-c96c-4519-87b4-7076582f5da2.png" /> </p> </details> ### Checklist Delete any items that are not applicable to this PR. - [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [X] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - Collaborating with docs teams on this dedicated docs issue: elastic/security-docs#2015 - [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [X] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) (cherry picked from commit 7bfcb52) # Conflicts: # x-pack/plugins/security_solution/public/common/components/integrations_popover/helpers.tsx # x-pack/plugins/security_solution/public/common/components/integrations_popover/index.tsx # x-pack/plugins/security_solution/public/detections/components/rules/description_step/required_integrations_description.tsx # x-pack/plugins/security_solution/public/detections/components/rules/related_integrations/use_installed_integrations.tsx
… Fields Feedback & Fixes (#133050) (#134148) * [Security Solution][Detections] Related Integrations & Required Fields Feedback & Fixes (#133050) ## Summary Addressing the following feedback from #132847: - [X] On the Rule Management page package name is used instead of package title when the package has only 1 integration: - [X] move integrations_popover to `related_integrations` directory - [X] update useInstalledIntegrations to always return array of installedIntegration - [X] move useInstalledIntegrations to `related_integrations` directory - [X] Slight update to copy in Rules Table popover - [X] Ok to use Rule Details UI within Rules Table popover content - [X] Sort integrations alphabetically - [X] Update left padding on version mis-match tooltip - [X] #133291 - [X] #133269 - [X] Add Kibana Advanced Setting for disabling integrations badge on Rules Table <p align="center"> <img width="500" src="https://user-images.githubusercontent.com/2946766/171750790-ffa2e3ef-dd7a-499c-9b08-89bafc06dd50.png" /> </p> - [ ] Adds tests - [x] `useInstalledIntegrations` hook - [X] relatedIntegrations utils - [x] IntegrationDescription - [ ] Add loaders where necessary since there can now be API delay - May hold off on loaders as transition from no installed integrations -> installed integrations isn't too bad as-is ##### Updated integrations popover content on Rules Table to match Rule Details design <p align="center"> <img width="500" src="https://user-images.githubusercontent.com/2946766/172263941-3e948b41-7ef7-4281-8354-57e77ddeb433.png" /> </p> In discussions with @banderror reviewing the different integration states (uninstalled, installed, enabled, and agents enrolled), we are now capturing the distinction between `Installed` and `Enabled` so that we don't confuse users when a package is installed but the integration isn't enabled/configured. I also added tooltips for clarifying each state and what action the user should perform to ensure compatibility. In collab with @yiyangliu9286 @jethr0null (comments below) -- we've consolidated to a single `Installed: enabled` badge, and updated `Uninstalled` to `Not installed` as well. ##### Tooltips <details><summary>Not installed</summary> <p align="center"> <img width="500" src="https://user-images.githubusercontent.com/2946766/172264210-00064485-2df9-408e-953b-9294f16dedf2.png" /> </p> </details> <details><summary>Installed</summary> <p align="center"> <img width="500" src="https://user-images.githubusercontent.com/2946766/172263672-67b641cd-5895-464a-8897-f26bd0a61073.png" /> </p> </details> <details><summary>Installed: enabled</summary> <p align="center"> <img width="500" src="https://user-images.githubusercontent.com/2946766/172263783-563ea48d-c96c-4519-87b4-7076582f5da2.png" /> </p> </details> ### Checklist Delete any items that are not applicable to this PR. - [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [X] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - Collaborating with docs teams on this dedicated docs issue: elastic/security-docs#2015 - [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [X] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) (cherry picked from commit 7bfcb52) # Conflicts: # x-pack/plugins/security_solution/public/common/components/integrations_popover/helpers.tsx # x-pack/plugins/security_solution/public/common/components/integrations_popover/index.tsx # x-pack/plugins/security_solution/public/detections/components/rules/description_step/required_integrations_description.tsx # x-pack/plugins/security_solution/public/detections/components/rules/related_integrations/use_installed_integrations.tsx * Fixes type from missing backport
Summary
Addressing the following feedback from #132847:
related_integrations
directoryrelated_integrations
directoryRequired fields
#133291useInstalledIntegrations
hookUpdated integrations popover content on Rules Table to match Rule Details design
In discussions with @banderror reviewing the different integration states (uninstalled, installed, enabled, and agents enrolled), we are now capturing the distinction between
Installed
andEnabled
so that we don't confuse users when a package is installed but the integration isn't enabled/configured. I also added tooltips for clarifying each state and what action the user should perform to ensure compatibility. In collab with @yiyangliu9286 @jethr0null (comments below) -- we've consolidated to a singleInstalled: enabled
badge, and updatedUninstalled
toNot installed
as well.Tooltips
Not installed
Installed
Installed: enabled
Checklist
Delete any items that are not applicable to this PR.
Related Integrations
,Required Fields
andSetup
security-docs#2015