-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 custom fields: List type #194236
base: main
Are you sure you want to change the base?
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please also update the tests in x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases
to include examples of list custom fields?
const CaseCustomFieldListWithValidationRt = rt.strict({ | ||
key: rt.string, | ||
type: CustomFieldListTypeRt, | ||
value: rt.union([CaseCustomFieldListWithValidationValueRt('value'), rt.null]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is the right approach. Why not just save the key?
Testing this with the API led me to some weird scenarios.
Try the following:
- Create a list custom field with a few different options.
- Send a case POST request where the custom field value has:(1) a correct
key
, and(2) a wrongvalue/label
.
The case detail page renders the correct label but the case GET API returns the wrong label that does not exist in the configuration.
Some screenshots illustrating this scenario:
My configuration
The POST request
The case detail page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is the right approach. Why not just save the key?
Already decided on in #168378
Text of the custom field labels need to be searchable and queryable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Send a case POST request where the custom field value has:(1) a correct key, and(2) a wrong value/label.
Let me take a look and see if I can get the POST to only contain the key and derive the label from it on the server side. That's probably the safest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For type system simplicity it looks like the easiest solution is to validate both that the correct key exists and that the label matches the key.
works fine 👍
++ custom_field_with_template.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM except some minor comments.
Agree with @adcoelho's concern about saving the option key instead of value for case's custom field.
type: customFieldsConfigurationMock[5].type, | ||
value: null, | ||
}, | ||
...customFieldsConfigurationMock.map(({ key, type, defaultValue, required }) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! 😄
// When updating the field, store the selected value while waiting for the response | ||
// Display the selected value in the view component while waiting, and only switch it back to the | ||
// previous value if the response fails | ||
const [optimisticDisplayField, setOptimisticDisplayField] = useState<CaseCustomFieldList | null>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice name 😄 Does it take lot of time to get response here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It takes a couple seconds, not long but long enough to bother me personally 🤣
@adcoelho @js-jankisalvi I also had concerns about saving the option key and the label instead of just saving the option key. As per @cnasikas's comment here this is the schema we have to use on this feature. |
…ustom-field # Conflicts: # x-pack/platform/plugins/shared/cases/common/types/api/configure/v1.ts # x-pack/platform/plugins/shared/cases/common/types/domain/configure/v1.ts # x-pack/platform/plugins/shared/cases/public/components/custom_fields/toggle/configure_text_field.test.ts # x-pack/platform/plugins/shared/cases/public/components/custom_fields/toggle/configure_toggle_field.test.ts # x-pack/platform/plugins/shared/cases/server/custom_fields/list.ts # x-pack/platform/plugins/shared/cases/server/services/cases/transform.ts # x-pack/plugins/cases/public/components/custom_fields/toggle/configure_text_field.test.ts
💔 Build Failed
Failed CI StepsHistory
|
Summary
Closes #176491
Adds the List custom field type to Cases. Also makes some changes to the custom field configuration type system to handle the new features added in this field type.
The List field type creates a
<select>
dropdown with a set ofoptions
determined by the custom field's configuration. Options are defined as{ key: UUID, label: String }
.When the user selects a value, it is saved to the case with the schema
{ key: ${Field UUID}.{Option UUID}, label: Option Label }
. When fetching the case, thiskey
is transformed into a{key: Field UUID, value: Option UUID}
pair so that the UI can fetch current option label from the field configuration schema. Therefore, option labels can be renamed and display their correct label in the UI, but updating thelabel
in the case SavedObject will still require a followup issue. Fortunately the only thing this affects is sorting, which is not yet implemented at the UI level for any custom field type.This PR also allows List fields to be filtered. This required a change from using static
filterOptions
in custom field configurations to a dynamicgetFilterOptions
function. The Toggle field was updated to use this new dynamic function.Known Issues
Due to elastic/eui#8027 drag-and-drop on the newUPDATE: This should be fixed<OptionsField>
doesn't correctly render the current draggable element inside the flyout. EUI is aiming to fix this before 8.16.Checklist