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] Add Severity bulk action & row action #142826

Merged
merged 8 commits into from
Oct 10, 2022

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Oct 6, 2022

Summary

This PR adds the ability for users to be able to change the severity of a case through bulk actions or row actions

User flow

Screen.Recording.2022-10-06.at.1.04.09.PM.mov

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 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 v8.6.0 labels Oct 6, 2022
@cnasikas cnasikas self-assigned this Oct 6, 2022
@cnasikas cnasikas requested a review from a team as a code owner October 6, 2022 10:00
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@@ -158,10 +158,6 @@ describe('AllCasesListGeneric', () => {
.prop('value')
).toBe(useGetCasesMockState.data.cases[0].createdAt);

expect(
Copy link
Member Author

@cnasikas cnasikas Oct 6, 2022

Choose a reason for hiding this comment

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

Moved to its own test

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.

Looks good, left a few comments

i18n.translate('xpack.cases.actions.severity', {
values: { caseTitle, totalCases, severity },
defaultMessage:
'{totalCases, plural, =1 {Case "{caseTitle}" was} other {{totalCases} cases were}} set to {severity}',
Copy link
Contributor

Choose a reason for hiding this comment

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

if caseTitle is undefined will it show Case "undefined" was?

Copy link
Member Author

@cnasikas cnasikas Oct 6, 2022

Choose a reason for hiding this comment

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

The case is a mandatory field and it will never be undefined. Types also protect us from that, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I might be misunderstanding but I'm referring to caseTitle?: string;, so it's optional and can be undefined.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, sorry my mistake 🙂. It is a pattern we had before that I followed. The title will only show if you have one case. Indeed, it is possible to show "undefined" if totalCases === 1 and caseTitlte === undefined. Do you want me to change the pattern to something else? Any ideas? I am not super fun of it for the reason you mentioned.

{
name: severities[CaseSeverity.LOW].label,
icon: getSeverityIcon(CaseSeverity.LOW),
onClick: () => handleUpdateCaseSeverity(selectedCases, CaseSeverity.LOW),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we pass handleUpdateCaseSeverity directly? Instead of a function that calls it? I think passing a new function invalidates the memoization right?

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 need to get the selected cases somehow which are passed to getActions. It invalidates memorization but I think it is fine for this case. I am open to suggestions 🙂.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh I see, yeah that's fine. Or we could use two useCallback I think:

onClick: useCallback(() => handle..., [...])

Or does that not work?

Copy link
Member Author

Choose a reason for hiding this comment

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

This will not work because the onClick handler passes a MouseEvent and not the selected cases. Somehow I need to get them from the getActions function. I could

import { memoize } from 'lodash'

const onLowSeverityClick = memorize((selectedCases) => handleUpdateCaseSeverity(selectedCases, CaseSeverity.LOW))

//

onClick: () => onLowSeverityClick(selectedCases)

but I do not think it is worth the optimization until we have a strong indicator that it causes performance problems. The memorization cost in this scenario maybe be greater than the problem itself.

@cnasikas
Copy link
Member Author

cnasikas commented Oct 9, 2022

@elasticmachine merge upstream

@cnasikas
Copy link
Member Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 493 495 +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 369.9KB 372.6KB +2.7KB

History

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

cc @cnasikas

@cnasikas cnasikas merged commit b7de426 into elastic:main Oct 10, 2022
@cnasikas cnasikas deleted the severity_bulk_action branch October 10, 2022 13:50
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.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants