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][Case] ServiceNow ITSM: Add category & subcategory fields #90547

Merged
merged 1 commit into from
Feb 11, 2021

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Feb 6, 2021

Summary

The PR adds two new fields for the ServiceNow ITSM connector. Specifically, category and subcategory. It also fixes a bug when changing categories and subcategories in ServiceNow SIR connector.

Create Case

ITSM.-.Create.Case.mov

Alerts & Detections

Screenshot 2021-02-09 at 2 34 17 PM

Meta: https://github.com/elastic/security-team/issues/477
Depends on: #88655

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@cnasikas cnasikas added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Feb 6, 2021
@cnasikas cnasikas requested review from a team as code owners February 6, 2021 10:26
@cnasikas cnasikas self-assigned this Feb 6, 2021
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

Alerting related changes LGTM

@cnasikas cnasikas requested review from a team as code owners February 9, 2021 10:34
@cnasikas cnasikas requested a review from a team as a code owner February 9, 2021 11:10
@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Feb 9, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -119,6 +119,8 @@ function getServiceNowActionParams(): ServiceNowActionParams {
impact: '2',
severity: '2',
urgency: '2',
category: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want to update our UI to allow users to actually set these without using the API directly. I'm creating a follow-up issue.

elastic/uptime#300

Copy link
Member Author

@cnasikas cnasikas Feb 9, 2021

Choose a reason for hiding this comment

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

@justinkambic FWIW, there is an API call (subaction) where you can get dynamically all available choices (impact, priority, severity, category, subcategory) for a specific instance. If you are interested to this please ping me on Slack!

Example: https://github.com/cnasikas/kibana/blob/itms_category/x-pack/plugins/security_solution/public/cases/components/connectors/servicenow/servicenow_itsm_case_fields.tsx

@cnasikas
Copy link
Member Author

cnasikas commented Feb 9, 2021

@elasticmachine merge upstream

@cnasikas cnasikas force-pushed the itms_category branch 2 times, most recently from ba6e698 to 8962df3 Compare February 10, 2021 08:29
@cnasikas cnasikas force-pushed the itms_category branch 3 times, most recently from b745780 to 0a794b6 Compare February 10, 2021 13:12
Comment on lines 202 to 219
test('it should set subcategory to null when changing category', async () => {
await waitFor(() => {
const select = wrapper.find(EuiSelect).filter(`[data-test-subj="categorySelect"]`)!;
select.prop('onChange')!({
target: {
value: 'network',
},
} as React.ChangeEvent<HTMLSelectElement>);
});
wrapper.update();
expect(onChange).toHaveBeenCalledWith({
...fields,
subcategory: null,
category: 'network',
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I know the other tests here dont follow this and it passes as is without, but the correct way would be:

Suggested change
test('it should set subcategory to null when changing category', async () => {
await waitFor(() => {
const select = wrapper.find(EuiSelect).filter(`[data-test-subj="categorySelect"]`)!;
select.prop('onChange')!({
target: {
value: 'network',
},
} as React.ChangeEvent<HTMLSelectElement>);
});
wrapper.update();
expect(onChange).toHaveBeenCalledWith({
...fields,
subcategory: null,
category: 'network',
});
});
test('it should set subcategory to null when changing category', async () => {
const select = wrapper.find(EuiSelect).filter(`[data-test-subj="categorySelect"]`)!;
select.prop('onChange')!({
target: {
value: 'network',
},
} as React.ChangeEvent<HTMLSelectElement>);
wrapper.update();
await waitFor(() => {
expect(onChange).toHaveBeenCalledWith({
...fields,
subcategory: null,
category: 'network',
});
});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

the await waitFor should be surrounding the expect after the async change. it definitely does not always work this way, but alas we're using enzyme with react-testing-library

disabled={isLoadingChoices}
options={categoryOptions}
value={incident.category ?? undefined}
onChange={(e) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd do a useCallback here since you have the incident value

Copy link
Member Author

@cnasikas cnasikas Feb 11, 2021

Choose a reason for hiding this comment

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

There is no difference if we use the incident or not. The callback function will always be a new reference for the prop no matter what we do inside the function.

Copy link
Contributor

@stephmilovic stephmilovic left a comment

Choose a reason for hiding this comment

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

Manual testing went perfectly. Just a few nits on the code, other than that LGTM. very clean, nicely done! 🎸

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2180 2181 +1
triggersActionsUi 337 338 +1
total +2

Async chunks

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

id before after diff
securitySolution 7.5MB 7.5MB +3.8KB
triggersActionsUi 1.4MB 1.4MB +3.4KB
uptime 909.2KB 909.2KB +31.0B
total +7.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 234.0KB 234.1KB +105.0B

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting Security Solution Threat Hunting Team Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants