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] Remove alerts from cases #143457

Merged
merged 12 commits into from Oct 24, 2022
Merged

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Oct 17, 2022

Summary

This PR allows users to remove alerts from a case through the case activity. It also:

  • Refactors the action components used in the case activity
  • Improve event titles in user actions when deleting attachments
  • Fix a UI bug with the actions comment type icon
  • Improve callout titles when deleting attachments

Screenshots

Screenshot 2022-10-18 at 6 09 47 PM

Screenshot 2022-10-18 at 2 31 26 PM

Screenshot 2022-10-17 at 7 28 16 PM

Screenshot 2022-10-17 at 7 13 53 PM

Screenshot 2022-10-17 at 7 28 09 PM

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Release notes

Add the ability to remove alerts attached to a case

@cnasikas cnasikas added backport:skip This commit does not require backporting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature release_note:feature Makes this part of the condensed release notes labels Oct 17, 2022
@cnasikas cnasikas self-assigned this Oct 17, 2022
@cnasikas cnasikas marked this pull request as ready for review October 18, 2022 16:21
@cnasikas cnasikas requested a review from a team as a code owner October 18, 2022 16:21
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

const { comment } = userAction.payload;

if (comment.type === CommentType.alert) {
const alertLabel =
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, would it be better to use a single translation for this? I suppose we'd need to force comment.alertId to be an array for that to work though. I believe the translations can handle the plural vs singular.

<UserCommentPropertyActions
isLoading={isLoading}
commentContent={comment.comment}
onEdit={() => handleManageMarkdownEditId(comment.id)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note, this will recreate the anonymous function on each render, is it worth creating a useCallback for these? Probably not that much of a performance improvement though

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, this is not a hook so I cannot use useCallback. I need to create another component that wraps the actions so I can use useCallback. Do you mind if we leave it as it is and do the performance improvement if needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's fine, leave it as it is.

const builderArgs = getMockBuilderArgs();

beforeEach(() => {
jest.clearAllMocks();
});

it('renders correctly a description', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there's an extra space between correctly and a

@peteharverson
Copy link
Contributor

For a user with read only access to Cases, the actions popover menu is shown, but is empty. We should hide this if there are no actions:

image

@cnasikas
Copy link
Member Author

cnasikas commented Oct 21, 2022

@peteharverson Fixed in 7a9fec0

@cnasikas
Copy link
Member Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #3 / task_manager scheduling and running tasks should disable and reenable task and not run it when runSoon = false

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 500 507 +7

Async chunks

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

id before after diff
cases 372.9KB 374.5KB +1.6KB

History

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

cc @cnasikas

@cnasikas cnasikas merged commit ee130f1 into elastic:main Oct 24, 2022
@cnasikas cnasikas deleted the unlink_alerts_comments branch October 24, 2022 11:33
jennypavlova pushed a commit to jennypavlova/kibana that referenced this pull request Oct 24, 2022
* Remove alerts from cases

* Refactor property actions

* Add tests

* Fix i18n

* Fix tests

* PR feedback

* Do not show property action if empty

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
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:feature Makes this part of the condensed release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants