[Cases] Case action enhancements and fixes by cnasikas · Pull Request #180758 · elastic/kibana

cnasikas · 2024-04-13T14:01:49Z

Summary

In this PR:

Address @adcoelho comments regarding documentation.
Fix @js-jankisalvi bug about unsupported consumers ([Cases] Case action #168369 (review)).
Address @shanisagiv1 feedback regarding the title and the description. Specifically:
- The title changed to "<rule_name> - Grouping by <grouping_by_value> (Auto-created)".
- The description changed to "This case was created by the Case action in <rule_name_link>. The assigned alerts are grouped by <grouping_by_key>:<grouping_by_value>".
Add the grouping key as a tag.

The issue about the "Unknown" user will be fixed in another PR.

rule_grouping_UI_issue.mov

I think it is fine to leave it as it is because a) the value will not be saved even if they are added b) an error is being shown c) the only way to do it properly is to validate while the user is typing which is going to lead to bad UX. If you feel otherwise let me know.

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

cnasikas · 2024-04-13T14:02:07Z

/ci

elasticmachine · 2024-04-15T08:37:01Z

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

elasticmachine · 2024-04-15T08:37:02Z

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

adcoelho · 2024-04-15T08:48:18Z

x-pack/plugins/cases/server/connectors/cases/cases_connector_executor.ts

-      .map(([key, value]) => {
-        const keyAsCodeBlock = `\`${key}\``;
-        const valueAsCodeBlock = `\`${convertValueToString(value)}\``;
+    const description = `This case was created by the Case action in ${ruleName}.`;


What if we find a way to highlight/differentiate the rule name? Maybe by wrapping it in quotation marks or having it in italics?

I think currently it looks a bit messy:

Good point. If you see my screenshot in the description if you have set the server.publicBaseUrl: 'https://localhost:5601' in your kibana.dev.yml the name of the rule would be a URL. Quotation marks seem more suitable with links. Wdyt?

Ah I like how it looks in the screenshot, imo that is ideal and we dont need the quotation marks.

Is there any scenario where no URL is rendered and we just have the name?

Yes if the user is not set server.publicBaseUrl it will be shown as in your screenshot.

adcoelho

I tested the changes and the fixes work.

I left one small comment about the Case description.

Regarding the bug I found previously you mention the following in this PR's Summary:

the only way to do it properly is to validate while the user is typing which is going to lead to bad UX

Why only during typing? We already have an error being shown, can't we prevent the user from clicking Save if the form has errors?

I tried other fields and saving is not allowed if they have errors. This behavior is not consistent with the rest of the sidepanel.

Screen.Recording.2024-04-15.at.10.57.49.mov

js-jankisalvi

Verified again and it works as expected. 👍

We already have an error being shown, can't we prevent the user from clicking Save if the form has errors?

Agree with @adcoelho We prevent to save when there is an error. We should keep similar behaviour for group by alert field.

x-pack/plugins/cases/server/connectors/cases/cases_connector_executor.ts

jcger · 2024-04-15T09:35:37Z

x-pack/plugins/cases/server/connectors/cases/cases_connector_executor.ts

-    const ruleNameTrimmed = ruleName.slice(0, MAX_TITLE_LENGTH - suffix.length - 1);
+    const suffix = `${
+      groupingDescription.length > 0 ? ` - Grouping by ${groupingDescription}` : ''
+    }${oracleCounter > INITIAL_ORACLE_RECORD_COUNTER ? ` (${oracleCounter})` : ''} (Auto-created)`;


Should (Auto-created) be translated? What about the description or the grouping description?

Hey! I do not think we should translate them because this content is generated dynamically on the server side. It is not a static text on the UI.

ok but if I might nitpick a bit, I think we should answer another question. Should the user be seeing it in their default set language or in english? I disagree that depending where it comes from it should be translated or not. IMO it's better UX.

To be honest, I am not sure what is the best practice here and what we do in Kibana. @lcawl what we should do here?

I'm not sure if either of these links are helpful: https://eui.elastic.co/#/utilities/i18n or https://github.com/elastic/kibana/tree/main/packages/kbn-i18n. However I agree that we should try to translate these phrases if it's possible.

Done in 81667dd (#180758)

jcger · 2024-04-15T09:43:27Z

x-pack/plugins/cases/server/connectors/cases/cases_connector_executor.ts

+      return `${description} The assigned alerts are grouped by ${groupingDescription}.`;
+    }
+
+    return `${description}`;


shouldn't this be just return description?

Done af0baec (#180758)

cnasikas · 2024-04-15T11:31:49Z

@adcoelho @js-jankisalvi We can only disable the Save button if the groupingBy field changes and the validation runs. In this scenario, the onChange callback is not called by the EuiCombobox so the field does not change and the validation will not run. I think it is an issue with how the actions UI framework works. The only functions that give us what the user types is onSearch or onKeyDown. They are fired on every keystroke typed by the user. If we change the value as the user types, so we can run the validation, we can run into weird performance issues and bugs. The work on the new rule form would probably fix this. I cannot find a way to do it properly. Any suggestions/alternatives I can use? cc @Zacqary

x-pack/plugins/cases/server/connectors/cases/cases_connector_executor.ts

kibana-ci · 2024-04-16T12:10:25Z

💚 Build Succeeded

Buildkite Build
Commit: dc91440

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
`alerting`	825	826	+1

Unknown metric groups

API count

id	before	after	diff
`alerting`	857	858	+1

History

💔 Build #203968 failed af0baec
💔 Build #203453 failed 2937b4a

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

cc @cnasikas

cnasikas added 5 commits April 12, 2024 13:31

Remove unused translations

c2825e2

Add the key as tag to cases

affa94f

Improve the tags, title, and the description

94989f8

Pass the producer to the connector adapter

b7fece7

Use producer to get the if the consumer is not found

2937b4a

cnasikas added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// Feature:Cases Cases feature v8.14.0 labels Apr 13, 2024

cnasikas self-assigned this Apr 13, 2024

cnasikas added 2 commits April 15, 2024 10:13

Previous PR feedback

78ce0ea

Fix integration tests

5700eea

cnasikas marked this pull request as ready for review April 15, 2024 08:36

cnasikas requested a review from a team as a code owner April 15, 2024 08:36

adcoelho reviewed Apr 15, 2024

View reviewed changes

adcoelho approved these changes Apr 15, 2024

View reviewed changes

js-jankisalvi approved these changes Apr 15, 2024

View reviewed changes

x-pack/plugins/cases/server/connectors/cases/cases_connector_executor.ts Outdated Show resolved Hide resolved

jcger approved these changes Apr 15, 2024

View reviewed changes

lcawl reviewed Apr 15, 2024

View reviewed changes

x-pack/plugins/cases/server/connectors/cases/cases_connector_executor.ts Outdated Show resolved Hide resolved

cnasikas added 6 commits April 16, 2024 12:30

Merge branch 'main' into ca_fixes

7b0fc81

Fix merge

9291a95

Add eclipses at the end of the trimmed rule name

c23f48f

Translate the title and the description

81667dd

Improve rule context in description

18cf2d2

PR feedback

af0baec

cnasikas enabled auto-merge (squash) April 16, 2024 10:13

Fix types

dc91440

cnasikas merged commit e9266f3 into elastic:main Apr 16, 2024

cnasikas deleted the ca_fixes branch April 16, 2024 12:10

kibanamachine added the backport:skip This PR does not require backporting label Apr 16, 2024

Conversation

cnasikas commented Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

cnasikas commented Apr 13, 2024

Uh oh!

elasticmachine commented Apr 15, 2024

Uh oh!

elasticmachine commented Apr 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adcoelho left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

js-jankisalvi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnasikas Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnasikas commented Apr 15, 2024

Uh oh!

Uh oh!

kibana-ci commented Apr 16, 2024

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

cnasikas commented Apr 13, 2024 •

edited

Loading

adcoelho left a comment •

edited

Loading

js-jankisalvi left a comment •

edited

Loading

cnasikas Apr 15, 2024 •

edited

Loading