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] Custom field default values in configuration forms #174628

Conversation

adcoelho
Copy link
Contributor

@adcoelho adcoelho commented Jan 10, 2024

See #171747 for more info

Summary

Merging into a feature branch.

This PR handles only the cases configuration page.

With these changes, users can now configure default values for custom fields in the UI.

Text custom field Screenshot 2024-01-12 at 13 56 58
Toggle custom field Screenshot 2024-01-12 at 14 01 43

When clicking required the default value for a toggle custom field is false.

When clicking required the default value for a text custom field is an empty string.

Trying to save an empty `string` as the default value of a `text` custom field should display an error message on the form. Screenshot 2024-01-12 at 14 00 39

Viewing previously created custom fields should display their default values correctly.

Viewing previously created custom fields in a version where default values did not exist should display the forms correctly.

Other Notes

  1. I changed the property name from default_value to defaultValue
  2. I fixed some instances of expect(screen.getById... where I found them.
  3. I addressed most comments left from the previous PR. The last one will be a separate PR(moving some types around).

@adcoelho adcoelho 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 v8.13.0 labels Jan 10, 2024
@adcoelho adcoelho self-assigned this Jan 10, 2024
@adcoelho adcoelho marked this pull request as ready for review January 12, 2024 13:05
@adcoelho adcoelho requested a review from a team as a code owner January 12, 2024 13:05
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

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! I tested the code and I found the following:

  • On certain configurations, I can stuck on an "infinitive loop" where I cannot edit any custom field nor add a new one. This could happen if I have two custom fields with required fields created before the PR. If I try to set the default value in any of them add a new custom field or edit any of the others I get an error regarding missing default values. The reason is because on the request we put all the custom fields but I can edit only one per time in the UI. One easy solution would be to put our default (using the serializer) if a) the field is required and b) the default value is not set. Any other ideas?
  • For another PR, should we change the IDs in the toaster to the labels to make it easier to understand which field needs fixing?
  • The default value for the toggle feels a bit off. What about a select dropdown or putting labels next to the checkbox?

@adcoelho
Copy link
Contributor Author

adcoelho commented Jan 16, 2024

The reason is because on the request we put all the custom fields but I can edit only one per time in the UI. One easy solution would be to put our default (using the serializer) if a) the field is required and b) the default value is not set.

I like this idea, it is kind of what we will be doing in the update case iirc. I'll handle it 👍

For another PR, should we change the IDs in the toaster to the labels to make it easier to understand which field needs fixing?

Definitely, I found and fixed a similar error some time ago but I think it might have been in the update case page.

EDIT: We discussed this offline and I will address them in different PRs

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.

LGTM! About the bug I found in my previous review, we discussed offline and it will be addressed on another PR. What do you think about this (can be done on another PR):

The default value for the toggle feels a bit off. What about a select dropdown or putting labels next to the checkbox?

@adcoelho
Copy link
Contributor Author

@cnasikas

The default value for the toggle feels a bit off. What about a select dropdown or putting labels next to the checkbox?

Forgot to comment on this, how about writing false/true next to the checkbox? I also think it looks weird right now but didn't have a better idea.

@cnasikas
Copy link
Member

@adcoelho I rethink about it and I believe the EUI component we use for the defaultValue should be the same as the one we use for the value. So for the defaultValue we should use the EuiSwitch. This will avoid confusion. Wdyt?

@adcoelho
Copy link
Contributor Author

@cnasikas

@adcoelho I rethink about it and I believe the EUI component we use for the defaultValue should be the same as the one we use for the value. So for the defaultValue we should use the EuiSwitch. This will avoid confusion. Wdyt?

Just made the change in the last commit 👍

Screenshot 2024-01-17 at 14 13 15

@kibana-ci
Copy link
Collaborator

💔 Build Failed

Failed CI Steps

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [683fad8]

History

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

cc @adcoelho

@adcoelho adcoelho merged commit 8dc8647 into elastic:custom-fields-default-values Jan 18, 2024
20 of 21 checks passed
adcoelho added a commit that referenced this pull request Jan 23, 2024
## Summary

**Merging into a feature branch.**

Custom field error messages in the case configuration page now list the
custom field `label`s instead of `key`s.

The message was updated for the following errors:
1. Custom field type error (trying to change a custom field type)
2. Missing default value from required custom field
3. Trying to define a default value for an optional custom field

Additionally, the error message for custom field type errors in the
cases API was also updated.

This PR fixes a bug discussed [in a previous
comment](#174628 (review)).

> For another PR, should we change the IDs in the toaster to the labels
to make it easier to understand which field needs fixing?

<details><summary>Screenshot</summary>
<img width="1955" alt="Screenshot 2024-01-22 at 12 03 30"
src="https://github.com/elastic/kibana/assets/1533137/c4a2b83e-b40f-420d-9b74-575f3f92d819"></details>
adcoelho added a commit that referenced this pull request Feb 6, 2024
Closes #171747

Fixes #174285
Fixes #174286
Fixes #174287
Fixes #174288
Fixes #174289

## Summary

This feature branch introduces default values to the custom fields in
Cases.

Changes:
- The case configuration API now supports defining default values **for
required custom fields**.
  - The default value field is optional.
  - The default value field must have the same type as the custom field.
- Default values for required custom fields can be configured in the
cases settings page.
- The Create and Update Case pages now prefill required custom fields
with their default value(if it exists).
- Create and Update case API
  - required custom field missing from the request:
- if the default value exists -> set the custom field to the default
value
- if the default value is missing from the configuration -> throws an
error
- creating required custom fields with `value: null` -> always throws an
error

### Previous PRs

#174043 #174845 #174628
#175197 #175978 #175795

## Testing

I'll leave here a couple of important use cases to test:

- Migration scenario
  1. Create **multiple** required custom fields in 8.12.
  2. Migrate to the feature branch (future 8.13).
3. Defining default values in the settings page should work as expected.

- Update case UI
  1. Create an optional custom field
  2. Create a case and leave the custom field empty
  3. Edit the custom field to now be required
  4. Go to the case detail page and edit the custom field
  5. Is the form populated automatically with the default value?

## Release notes

Required custom fields now support default values. These default values
will be used to populate automatically the custom fields if they are not
defined when creating and updating cases.

---------

Co-authored-by: Kibana Machine <[email protected]>
fkanout pushed a commit to fkanout/kibana that referenced this pull request Feb 7, 2024
Closes elastic#171747

Fixes elastic#174285
Fixes elastic#174286
Fixes elastic#174287
Fixes elastic#174288
Fixes elastic#174289

## Summary

This feature branch introduces default values to the custom fields in
Cases.

Changes:
- The case configuration API now supports defining default values **for
required custom fields**.
  - The default value field is optional.
  - The default value field must have the same type as the custom field.
- Default values for required custom fields can be configured in the
cases settings page.
- The Create and Update Case pages now prefill required custom fields
with their default value(if it exists).
- Create and Update case API
  - required custom field missing from the request:
- if the default value exists -> set the custom field to the default
value
- if the default value is missing from the configuration -> throws an
error
- creating required custom fields with `value: null` -> always throws an
error

### Previous PRs

elastic#174043 elastic#174845 elastic#174628
elastic#175197 elastic#175978 elastic#175795

## Testing

I'll leave here a couple of important use cases to test:

- Migration scenario
  1. Create **multiple** required custom fields in 8.12.
  2. Migrate to the feature branch (future 8.13).
3. Defining default values in the settings page should work as expected.

- Update case UI
  1. Create an optional custom field
  2. Create a case and leave the custom field empty
  3. Edit the custom field to now be required
  4. Go to the case detail page and edit the custom field
  5. Is the form populated automatically with the default value?

## Release notes

Required custom fields now support default values. These default values
will be used to populate automatically the custom fields if they are not
defined when creating and updating cases.

---------

Co-authored-by: Kibana Machine <[email protected]>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
Closes elastic#171747

Fixes elastic#174285
Fixes elastic#174286
Fixes elastic#174287
Fixes elastic#174288
Fixes elastic#174289

## Summary

This feature branch introduces default values to the custom fields in
Cases.

Changes:
- The case configuration API now supports defining default values **for
required custom fields**.
  - The default value field is optional.
  - The default value field must have the same type as the custom field.
- Default values for required custom fields can be configured in the
cases settings page.
- The Create and Update Case pages now prefill required custom fields
with their default value(if it exists).
- Create and Update case API
  - required custom field missing from the request:
- if the default value exists -> set the custom field to the default
value
- if the default value is missing from the configuration -> throws an
error
- creating required custom fields with `value: null` -> always throws an
error

### Previous PRs

elastic#174043 elastic#174845 elastic#174628
elastic#175197 elastic#175978 elastic#175795

## Testing

I'll leave here a couple of important use cases to test:

- Migration scenario
  1. Create **multiple** required custom fields in 8.12.
  2. Migrate to the feature branch (future 8.13).
3. Defining default values in the settings page should work as expected.

- Update case UI
  1. Create an optional custom field
  2. Create a case and leave the custom field empty
  3. Edit the custom field to now be required
  4. Go to the case detail page and edit the custom field
  5. Is the form populated automatically with the default value?

## Release notes

Required custom fields now support default values. These default values
will be used to populate automatically the custom fields if they are not
defined when creating and updating cases.

---------

Co-authored-by: Kibana Machine <[email protected]>
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
Closes elastic#171747

Fixes elastic#174285
Fixes elastic#174286
Fixes elastic#174287
Fixes elastic#174288
Fixes elastic#174289

## Summary

This feature branch introduces default values to the custom fields in
Cases.

Changes:
- The case configuration API now supports defining default values **for
required custom fields**.
  - The default value field is optional.
  - The default value field must have the same type as the custom field.
- Default values for required custom fields can be configured in the
cases settings page.
- The Create and Update Case pages now prefill required custom fields
with their default value(if it exists).
- Create and Update case API
  - required custom field missing from the request:
- if the default value exists -> set the custom field to the default
value
- if the default value is missing from the configuration -> throws an
error
- creating required custom fields with `value: null` -> always throws an
error

### Previous PRs

elastic#174043 elastic#174845 elastic#174628
elastic#175197 elastic#175978 elastic#175795

## Testing

I'll leave here a couple of important use cases to test:

- Migration scenario
  1. Create **multiple** required custom fields in 8.12.
  2. Migrate to the feature branch (future 8.13).
3. Defining default values in the settings page should work as expected.

- Update case UI
  1. Create an optional custom field
  2. Create a case and leave the custom field empty
  3. Edit the custom field to now be required
  4. Go to the case detail page and edit the custom field
  5. Is the form populated automatically with the default value?

## Release notes

Required custom fields now support default values. These default values
will be used to populate automatically the custom fields if they are not
defined when creating and updating cases.

---------

Co-authored-by: Kibana Machine <[email protected]>
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.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants