SearchKit - Improve editable UX by not allowing required fields to be left blank

[colemanw]

Overview

Fixes dev/report#92 by enforcing required fields during in-place and bulk editing in SearchKit.

Before

SearchKit would allow you to save required fields with blank values.

After

Saving a blank value for a custom field marked "required" is prevented in the inline-edit UI:

It's also prevented in the bulk-update action:

Works similarly for select, date, and numeric inputs.

Technical Details

Unlike the 'required' field property, which only determines if the API requires a value to Create, the new 'nullable' property tells a UI whether a field is allowed to be set to NULL in Create OR Update. This value is now documented in APIv4 getFields and is returned consistently for both core and custom fields.

SearchKit uses this property during in-place edit and bulk edit operations to determine whether a field can be left blank.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[demeritcowboy]

Not sure if I'm understanding correctly with respect to non-custom fields: Would the next step be to update all the xml schema files to add a not_null setting? I'm confused because api4 getfields seems to ignore the existing <required> setting in the xml (both before and after patch), so required is always false (and so at the moment not_null is always false which is wrong).

And if that's correct, then this is going to be confusing terminology because GenCode etc uses <required> to determine whether the SQL should have NOT NULL in the SQL column definition, but there'd also be a <not_null> right beside it that it doesn't use.

Relatedly this line, while correct I think, is confusing: $ctrl.field.not_null ? null : ts('None') since it reads like "if not null, then null, otherwise none" where "none" means allow null.

Maybe "nullable" would be a better keyword?

[eileenmcnaughton]

Just noting that I tested this change and made the custom field marital status required but I can still remove it using edit in place with this patch (I logged a couple of issues in testing).

I'm with @demeritcowboy in struggling to get my head around this one - I think it needs to be documented as a confusing new 'getfields' value - I guess we have 3 things

1. required at the db level. is_deceased is a required field but it defaults to a reliable FALSE
2. required at the api level - I can't think of anything required at the api level but not at the db level at the moment but I can't think of things required at the db level and not required at the api level (eg is_deceased) and others that are required at both (financial_type_id)
3. quasi required - this applies to custom fields - they are required but ONLY if they are present on the form or another value in the same custom group is.

I think isnullable gets as as close as we are likely to get to something that makes sense

[colemanw]

@demeritcowboy I'm finally happy with this. Ready for a second round of review. See updated description.

[demeritcowboy]

Is this an "approximation" for now? For example I don't think activity date or status are really "nullable".

[colemanw]

Both of those fields are nullable in our schema. I think you could make a very good argument that they shouldn't be. Fixing it on the schema level can be a pain though because you have to deal with any existing data with null values. A quicker fix would be to set nullable=false just at the APIv4 layer for those fields (via the ActivitySpecProvider). I can do that as a followup.

[demeritcowboy]

Ok. My only other comment then was the one below about custom date fields. Otherwise looked good.

[demeritcowboy]

It doesn't seem to work for non-required custom date fields. I can't blank it out.

[colemanw]

It doesn't seem to work for non-required custom date fields. I can't blank it out.

@demeritcowboy I tracked that down to something pretty obscure. The API was returning a full date-time value for custom fields which only contain a date, and it was making the Angular field validation confused. Fixed in #22649.

[demeritcowboy]

Thanks @colemanw I tested both together and looks good. Will put merge-ready here.