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

[Cases] Delete alerts when deleting all comments #154202

Merged
merged 3 commits into from
Apr 4, 2023

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Apr 1, 2023

Summary

This PR remove the case id from the alerts when deleting all cases comments

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@cnasikas 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 labels Apr 1, 2023
@cnasikas cnasikas self-assigned this Apr 1, 2023
@cnasikas cnasikas requested a review from a team as a code owner April 1, 2023 18:02
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@cnasikas cnasikas mentioned this pull request Apr 1, 2023
8 tasks
@@ -387,35 +386,6 @@ export default ({ getService }: FtrProviderContext): void => {
});
});

it('should delete multiple comments from the appropriate owner', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved all tests about deleteAllComments to x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comments.ts

Copy link
Member Author

Choose a reason for hiding this comment

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

I preferred to create another file for the delete all cases comments API even though the code is very similar to the delete comment API. If you think otherwise let me know.

describe('alerts', () => {
type Alerts = Array<{ _id: string; _index: string }>;

const createCaseAttachAlertAndDeleteAlert = async ({
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is the same function as in delete_comment.ts could we move it to a common place and use it in both files?

});
});

describe('alerts', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not too painful could we have a test that deletes all comments for a case that has some alerts and other comments?

And could we also add an expect that ensures all comments are removed (like delete comments, then do a find and make sure there are no comments for the case or something).

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Unknown metric groups

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

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

cc @cnasikas

Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

@cnasikas cnasikas merged commit 66ad9e0 into elastic:main Apr 4, 2023
@cnasikas cnasikas deleted the remove_alerts_delete_all branch April 4, 2023 12:24
@kibanamachine kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Apr 4, 2023
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 Feature:Cases Cases feature release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants