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

[ResponseOps][Cases] Allow users to create templates #184104

Merged

Conversation

js-jankisalvi
Copy link
Contributor

@js-jankisalvi js-jankisalvi commented May 23, 2024

Summary

Merging into feature branch.

Implements create functionality #181864

Add template from case settings page.

image

How to test

  • Go to Cases > Settings
  • Click on add templates
  • Fill the form
  • Save

Scenarios:

  • Create template with different custom fields
  • Create template with connector

Flaky test runner here

Checklist

Delete any items that are not applicable to this PR.

@js-jankisalvi js-jankisalvi added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature labels May 23, 2024
@js-jankisalvi js-jankisalvi self-assigned this May 23, 2024
@js-jankisalvi js-jankisalvi added the release_note:skip Skip the PR/issue when compiling release notes label May 30, 2024
@js-jankisalvi js-jankisalvi marked this pull request as ready for review May 30, 2024 17:02
@js-jankisalvi js-jankisalvi requested a review from a team as a code owner May 30, 2024 17:02
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@cnasikas cnasikas self-requested a review May 31, 2024 07:45
Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

No code review. I tested the UI first. I will test the code soon. Finding:

  • I noticed that the template tags field provides options coming from the cases tags. Given that the tags are related to tags I think we should provide them from the template tags. We have all tags from all templates in the configuration. No need for an API call. Do we have a limit on the total number of tags per template?
Screenshot 2024-05-31 at 11 07 49 AM
  • The description is a text field. I think it should be a text area or markdown editor. Let's start with a text area for this MVP.

  • The description is required. What about making it optional?

Screenshot 2024-05-31 at 11 12 05 AM
  • The description is persisted to local storage. Given that this is a flyout and not a page I think we should not persist it. Otherwise, every time I open the flyout the description of the template I created previously will appear.
Screenshot 2024-05-31 at 11 09 46 AM

@js-jankisalvi
Copy link
Contributor Author

js-jankisalvi commented May 31, 2024

  • I noticed that the template tags field provides options coming from the cases tags. Given that the tags are related to tags I think we should provide them from the template tags. We have all tags from all templates in the configuration. No need for an API call.

Ahh, good point. Will load the template tags here.

Do we have a limit on the total number of tags per template?

Yes, it's 10

  • The description is a text field. I think it should be a text area or markdown editor. Let's start with a text area for this MVP.

Max description length is 1000 characters and it should be informative only, so agree with text area.

  • The description is required. What about making it optional?

It was mandatory in issue, but I am fine with making it optional.

  • The description is persisted to local storage. Given that this is a flyout and not a page I think we should not persist it. Otherwise, every time I open the flyout the description of the template I created previously will appear.

Hmm.. yes, this will need some modification in the existing description component. That's why I thought of doing it in next PR, but it makes sense to do it here to test the create template feature properly.

Fixed in a468d94

@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#6176

[❌] x-pack/test/functional_with_es_ssl/apps/cases/group2/config.ts: 0/25 tests passed.
[❌] x-pack/test_serverless/functional/test_suites/security/config.ts: 0/25 tests passed.
[❌] x-pack/test_serverless/functional/test_suites/observability/config.ts: 0/25 tests passed.

see run history

…, make description optional, show template tags options
@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#6182

[❌] x-pack/test/functional_with_es_ssl/apps/cases/group2/config.ts: 0/25 tests passed.
[❌] x-pack/test_serverless/functional/test_suites/security/config.ts: 0/25 tests passed.
[❌] x-pack/test_serverless/functional/test_suites/observability/config.ts: 0/25 tests passed.

see run history

Copy link
Contributor

@doakalexi doakalexi left a comment

Choose a reason for hiding this comment

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

I tested locally and worked as expected, LGTM! I was able to create templates with custom fields and a connector. This is really small, but now that the description is optional it'd be good to add the OptionalFieldLabel to match the other optional fields.

@js-jankisalvi
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#6190

[❌] x-pack/test/functional_with_es_ssl/apps/cases/group2/config.ts: 0/25 tests passed.
[❌] x-pack/test_serverless/functional/test_suites/security/config.ts: 0/25 tests passed.
[❌] x-pack/test_serverless/functional/test_suites/observability/config.ts: 0/25 tests passed.

see run history

@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6213

[✅] x-pack/test/functional_with_es_ssl/apps/cases/group2/config.ts: 30/30 tests passed.
[✅] x-pack/test_serverless/functional/test_suites/observability/config.ts: 30/30 tests passed.
[✅] x-pack/test_serverless/functional/test_suites/security/config.ts: 30/30 tests passed.

see run history

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Great work and great test coverage 🚀 ! I left a couple of comments.

x-pack/plugins/cases/public/components/templates/index.tsx Outdated Show resolved Hide resolved

const { emptyField, maxLengthField } = fieldValidators;

export const schema: FormSchema<TemplateFormProps> = {
Copy link
Member

Choose a reason for hiding this comment

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

A portion of this schema seems identical with the schema defined here x-pack/plugins/cases/public/components/create/schema.tsx. Wdyt about having one base schema common to both? Then, in this file, we can iterate the common schema and add the optional label to the fields we want.

Copy link
Contributor Author

@js-jankisalvi js-jankisalvi Jun 7, 2024

Choose a reason for hiding this comment

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

Created schema for case_form_fields and used it in template schema to append optional label. here
We can use the same in create case form as well.

@js-jankisalvi js-jankisalvi requested review from JiaweiWu and removed request for JiaweiWu June 5, 2024 15:29
Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Code LGMT! I tested and everything is working as expected. Thank you for your patience with the review.

@kibana-ci
Copy link
Collaborator

💔 Build Failed

Failed CI Steps

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [5986cc9]

History

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

cc @js-jankisalvi

@js-jankisalvi js-jankisalvi merged commit c511009 into elastic:feat/case_templates Jun 7, 2024
23 of 24 checks passed
@js-jankisalvi js-jankisalvi deleted the case-templates-ui branch June 7, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants