[Cases] Remove case info from alerts when deleting an alert attachment #154024

cnasikas · 2023-03-30T08:27:48Z

Summary

Users can remove alerts from a case by deleting the whole alert attachment. This PR removes the case id from the alerts when deleting an attachment of type alert. It does not remove the case info from all alerts attached to a case when deleting a case. It also fixes a bug where the success toaster will not show when deleting an attachment.

Related: #146864, #140800

Testing

Create a case and attach some alerts to the case.
Verify that the alerts table (in security or in o11y) shows the case the alert is attached to. You can enable the cases column by pressing "Fields", searching for "Cases", and then selecting the field.
Go to the case and find the alerts' user activity.
Press the ... and press "Remove alerts(s)"
Go back to the alert table and verify that the case is not shown in the Cases column for each alert.

Please check that when you remove alert(s), attachments (ml, etc), and comments you get a success toaster with the correct text.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2023-03-30T14:26:43Z

Pinging @elastic/response-ops (Team:ResponseOps)

elasticmachine · 2023-03-30T14:26:44Z

Pinging @elastic/response-ops-cases (Feature:Cases)

js-jankisalvi

Looks good. 👍
Verified locally, case id is removed from Cases columns in Alerts and success toaster is also visible once removed Alert from case. 🎉

js-jankisalvi · 2023-03-30T15:57:08Z

x-pack/plugins/cases/common/utils/attachments.ts

+
+export const isAlertAttachment = (
+  attachment: CommentAttributes
+): attachment is AttributesTypeAlerts => attachment.type === CommentType.alert;


Should we add unit test for this function?

js-jankisalvi · 2023-03-30T16:26:44Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+            },
+          },
+          {
+            script: { source: painlessScript, lang: 'painless' },


nice script name 😄

Yes because is a pain to write them hahhaha.

CoenWarmer

AO owned changes LGTM

jonathan-buttner

Great work, I left a few comments and I still need to test things. I'll do that shortly.

jonathan-buttner · 2023-03-31T14:42:29Z

x-pack/plugins/cases/common/utils/attachments.ts

@@ -29,3 +31,7 @@ export const isCommentRequestTypePersistableState = (
 ): context is CommentRequestPersistableStateType => {
  return context.type === CommentType.persistableState;
 };
+
+export const isAlertAttachment = (


Can we use the type guard here: https://github.com/elastic/kibana/blob/main/x-pack/plugins/cases/server/common/utils.ts#L254

It's slightly different as it operates on the request rather than attributes.

Ah, thanks. And I was searching for an existing one.

jonathan-buttner · 2023-03-31T15:10:51Z

x-pack/plugins/cases/server/services/alerts/index.ts

+        return;
+      }
+
+      await this.alertsClient.removeAlertsFromCase({


removeAlertsFromCase makes me think this is removing the alert ids from the case but if I understand this correctly we're removing the case id from the alerts right? Maybe rename this to removeCaseIdFromAlerts

We'd probably want a similar change for bulkUpdateCases

jonathan-buttner · 2023-03-31T15:17:29Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+        operation: ReadOperations.Get,
+      });
+
+      const painlessScript = `if (ctx._source['${ALERT_CASE_IDS}'] != null && ctx._source['${ALERT_CASE_IDS}'].length > 0) {


nit: I think we can remove ctx._source['${ALERT_CASE_IDS}'].length > 0 as the for-loop ensures that it won't execute the loop since i = 0 is not less than 0 or a negative number.

jonathan-buttner · 2023-03-31T15:31:30Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+
+      const painlessScript = `if (ctx._source['${ALERT_CASE_IDS}'] != null && ctx._source['${ALERT_CASE_IDS}'].length > 0) {
+        for (int i=0; i < ctx._source['${ALERT_CASE_IDS}'].length; i++) {
+            if (ctx._source['${ALERT_CASE_IDS}'][i] == '${caseId}') {


Painless might be figuring this out automatically but I think we want the string equality operator here:

if (ctx._source['${ALERT_CASE_IDS}'][i].equals('${caseId}'))

jonathan-buttner · 2023-03-31T15:35:07Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+
+      const mgetRes = await this.ensureAllAlertsAuthorized({
+        alerts,
+        operation: ReadOperations.Get,


Just double checking that we want the deletion operation to be a read here?

Yes, the same as when we attach an alert to a case (add a case id to the alert schema).

jonathan-buttner · 2023-03-31T16:53:42Z

x-pack/plugins/cases/server/services/alerts/index.ts

+      });
+    } catch (error) {
+      throw createCaseError({
+        message: `Failed to remove alerts from case ${caseId}: ${error}`,


Maybe update this to say Failed to remove case id from alerts...

jonathan-buttner · 2023-03-31T16:54:44Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+        body: bulkUpdateRequest,
+      });
+    } catch (error) {
+      this.logger.error(`Error removing alerts from case ${caseId}: ${error}`);


Maybe update this and the function name to removing case id from alerts.

jonathan-buttner · 2023-03-31T17:27:52Z

x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comment.ts

+          const updatedCases = [];
+
+          for (const theCase of cases) {
+            const updatedCase = await createComment({


Could we do a Promise.all here?

The Promise.all produces some weird race conditions/conflicts and the alerts are not updated correctly with the case ids.

jonathan-buttner · 2023-03-31T17:36:48Z

x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comment.ts

+      });
+
+      describe('observability', () => {
+        const createCaseAttachAlertAndDeleteAlert = async ({


Seems like this and the security solution function are pretty close do you think it'd make sense to move it to a function that both could call? I think the differences are the alerts that get created and owner, maybe those could be parameters?

jonathan-buttner · 2023-03-31T17:37:46Z

x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comment.ts

+          totalCases: number;
+          indexOfCaseToDelete: number;
+          expectedHttpCode?: number;
+          auth?: { user: User; space: string | null };


How about we rename this to deleteCommentAuth since I think that's the only time we're using it right?

jonathan-buttner · 2023-03-31T19:37:25Z

I tested it and everything is working great. The only slightly odd thing that could happen is if you attach an alert to the same case twice (as two separate attachments), then delete one of them, the case won't show in the alerts table but the case would still have an alert attached to it. I think this is fine though as I doubt users would add the same alert multiple times.

cnasikas · 2023-04-01T10:24:23Z

I tested it and everything is working great. The only slightly odd thing that could happen is if you attach an alert to the same case twice (as two separate attachments), then delete one of them, the case won't show in the alerts table but the case would still have an alert attached to it. I think this is fine though as I doubt users would add the same alert multiple times.

Very good point. Maybe we should start preventing users to attach the same alert twice in a case. I have an issue for that: #145514

cnasikas · 2023-04-01T13:00:35Z

@elasticmachine run elasticsearch-ci/docs

cnasikas · 2023-04-01T13:16:05Z

@elasticmachine merge upstream

kibana-ci · 2023-04-01T14:51:07Z

💚 Build Succeeded

Buildkite Build
Commit: c0cc933

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`ruleRegistry`	229	231	+2

Async chunks

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

id	before	after	diff
`cases`	363.7KB	364.3KB	+595.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`ruleRegistry`	14	15	+1

Unknown metric groups

API count

id	before	after	diff
`ruleRegistry`	258	260	+2

ESLint disabled line counts

id	before	after	diff
`securitySolution`	432	435	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	512	515	+3

History

💚 Build #116800 succeeded 476502b
💔 Build #116640 failed 7b218e8

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

cc @cnasikas

Remove alerts from case

7b218e8

cnasikas self-assigned this Mar 30, 2023

cnasikas added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.8.0 labels Mar 30, 2023

cnasikas mentioned this pull request Mar 30, 2023

[Cases] Link cases to alerts #146864

Closed

8 tasks

cnasikas added 2 commits March 30, 2023 14:27

Add tests

c7e2d82

Add integration tests

4735868

cnasikas marked this pull request as ready for review March 30, 2023 14:26

cnasikas requested review from a team as code owners March 30, 2023 14:26

Show a success toaster when deleting attachments

476502b

cnasikas force-pushed the remove_alerts branch from f76dbe6 to 476502b Compare March 30, 2023 15:05

cnasikas mentioned this pull request Mar 30, 2023

[Cases] Unlink alerts from a case #140800

Open

2 tasks

js-jankisalvi approved these changes Mar 30, 2023

View reviewed changes

CoenWarmer approved these changes Mar 31, 2023

View reviewed changes

PR feedback

7aaa9c2

jonathan-buttner reviewed Mar 31, 2023

View reviewed changes

jonathan-buttner approved these changes Mar 31, 2023

View reviewed changes

cnasikas added 2 commits April 1, 2023 13:28

PR feedback

0a62126

Merge branch 'main' into remove_alerts

623a999

cnasikas enabled auto-merge (squash) April 1, 2023 10:32

Merge branch 'main' into remove_alerts

c0cc933

cnasikas merged commit 3a6f721 into elastic:main Apr 1, 2023

cnasikas deleted the remove_alerts branch April 1, 2023 14:51

kibanamachine added the backport:skip This commit does not require backporting label Apr 1, 2023

cnasikas added release_note:breaking and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 3, 2023

cnasikas added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:breaking labels Apr 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cases] Remove case info from alerts when deleting an alert attachment #154024

[Cases] Remove case info from alerts when deleting an alert attachment #154024

cnasikas commented Mar 30, 2023 •

edited

Loading

elasticmachine commented Mar 30, 2023

elasticmachine commented Mar 30, 2023

js-jankisalvi left a comment

js-jankisalvi Mar 30, 2023

js-jankisalvi Mar 30, 2023

cnasikas Mar 31, 2023

CoenWarmer left a comment

jonathan-buttner left a comment

jonathan-buttner Mar 31, 2023

cnasikas Apr 1, 2023

jonathan-buttner Mar 31, 2023

jonathan-buttner Mar 31, 2023

jonathan-buttner Mar 31, 2023

jonathan-buttner Mar 31, 2023

cnasikas Apr 1, 2023

jonathan-buttner Mar 31, 2023

jonathan-buttner Mar 31, 2023

jonathan-buttner Mar 31, 2023

cnasikas Apr 1, 2023

jonathan-buttner Mar 31, 2023

jonathan-buttner Mar 31, 2023

jonathan-buttner commented Mar 31, 2023

cnasikas commented Apr 1, 2023 •

edited

Loading

cnasikas commented Apr 1, 2023

cnasikas commented Apr 1, 2023

kibana-ci commented Apr 1, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

[Cases] Remove case info from alerts when deleting an alert attachment #154024

[Cases] Remove case info from alerts when deleting an alert attachment #154024

Conversation

cnasikas commented Mar 30, 2023 • edited Loading

Summary

Testing

Checklist

For maintainers

elasticmachine commented Mar 30, 2023

elasticmachine commented Mar 30, 2023

js-jankisalvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoenWarmer left a comment

Choose a reason for hiding this comment

jonathan-buttner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-buttner commented Mar 31, 2023

cnasikas commented Apr 1, 2023 • edited Loading

cnasikas commented Apr 1, 2023

cnasikas commented Apr 1, 2023

kibana-ci commented Apr 1, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Public APIs missing exports

API count

ESLint disabled line counts

Total ESLint disabled count

History

cnasikas commented Mar 30, 2023 •

edited

Loading

cnasikas commented Apr 1, 2023 •

edited

Loading