This is an interesting bug to review the current status of the "defaults strategy" , see summary below. However I think, this specific case (a non-root number object property) setting an empty string as the modelValue for the field when the user completely removes the input content, and do no validation on it, seems a client bug.

Default values strategy

I can't find the specific phab issue and comments where more relevant arguments about the way we should handle defaults were discussed, see some context in T360042 and T359193. The idea of making default values mandatory for root properties in the schema comes from the fact we cannot ensure configs to be available at all times. In the eventual situation of that happening we thought being able to fallback to a valid value was preferred over completely crashing, see T363669: CommunityConfiguration: eswiki beta fatals with Help panel enabled and related T363736. That being said, unless we want to revisit this decision, we should probably prioritize T363414.

The problem here is we should figure out what's the intent of the user when completely removing the value of a field. I think it's fair to assume the user is trying to "remove" that config option expecting it to make no effect, eg: emptying maxTasksPerDay the administrator expectation would be that the limit is not applied at all but that is not a supported use case in CC1.0 Special:EditGrowthConfig where trying that operation yields:

Invalid suggested edits configuration for task type "maxTasksPerDay": "section-image-recommendation" must be larger than 1

The request actually contains an empty string sent as form data wpnewcomertasks-section-image-recommendationMaxTasksPerDay: "". If the error message would have not mentioned the String type which is indeed confusing but would have yield a number specific message we would achieve the same UX than in CC1.0. I think we can achieve that quickly, see 1031871.

Also, this is a good moment to revisit T360921 and maybe prioritize it. If we don't want to support the "removing to unlimit the number of tasks" use case, setting this field as required would be fair.

Note: In this case the error is specific to the section_image_recommendation.maxTasksPerDay property which is a property nested in the section_image_recommendation, which does not have a default neither, this is probably a bug and we should correct it(?), cc @Urbanecm_WMF. Even if we had some default there, eg: [ .. 'maxTasksPerDay' => 25 ... ] we should figure out what to do when the user completely removes the value of a field. I think the convention of allowing to remove a property needs to be backed in the json schema with a nullable type, eg: "type": ['int', null]. If the type is not nullable the validator will fairly complain on server validation on submission. If we signal the field being required in the schema, the form will complain before submission about the missing value.

Thank you, @Sgs, for writing out these thoughts. I agree that, for this specific field, it makes sense to use the default it has in the GrowthExperiments extension. It probably also makes sense to add the requirement that its value needs to be larger than one, if we can make that possible. Ideally, that would then directly lead to also a frontend form error, not just a backend validation error, if that value is smaller than 2 or not a number.
In general, if all the values in the GrowthExperiments community configuration have a default, then we very likely also want that default to be in effect in the GE Schema for CC2.0 configuration.

Though that notion is distinct from saying that it is per-se impossible to have a truly optional field, if that is what is desired by the schema authors. But it makes sense to not code that until we genuinely have that use-case.

Change #1031898 had a related patch set uploaded (by Michael Große; author: Michael Große):

[mediawiki/extensions/GrowthExperiments@master] Add validation to max number of image validation tasks per day config

https://gerrit.wikimedia.org/r/1031898

Change #1031871 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/CommunityConfiguration@master] Number control: avoid empty string as a valid modelValue

https://gerrit.wikimedia.org/r/1031871

Hi all, I think this task hints at a couple of different issues. Firstly, the form should not attempt to save a string when the schema clearly requires an integer (the form should be generated in a way that makes saving invalid data pretty much impossible, thus making T351879: Support i18n in validation errors not as important.

But, the empty string issue is not the only problem this highlights. There is also a great deal of confusion in the empty state (rather than not introducing any maximum limit of links, GrowthExperiments limits it to 10, which is counterintutiive). I logged this confusing behavior as T365142: Empty state of Special:CommunityConfiguration/GrowthSuggestedEdits is confusing.

We also generally do not support nullable fields at all, I filled T365145: Community configuration: Support nullable types to track that.

With all of this in mind, I think what is left for this task is fixing the empty string saves. I rephrased the description and task title to make that more clear. @Etonkovidova, could you review my changes and comment whether the newly-filled tickets and the new description/title sufficiently covers the bug(s) this was meant to report? I'm not sure if I covered everything, or if there is something that should still be dealt with one way or another.

In T364056#9799599, @Michael wrote:

[...] It probably also makes sense to add the requirement that its value needs to be larger than one, if we can make that possible.

Why? We've seen some communities complaining about the difficulty of reverting only one link (out of many added in a single edit), and this being one of the patrolling problems for GrowthExperiments. In those situations, it might make sense to lower this down to 1. That being said, we should likely not allow negative numbers (since that does not make any sense in this context), but that seems to be out of scope for this task.

In general, if all the values in the GrowthExperiments community configuration have a default, then we very likely also want that default to be in effect in the GE Schema for CC2.0 configuration.

Absolutely. This is now covered in T365142.

Though that notion is distinct from saying that it is per-se impossible to have a truly optional field, if that is what is desired by the schema authors. But it makes sense to not code that until we genuinely have that use-case.

Having a truly optional config field is indeed hard, since the configured component needs to behave in a certain way when the field is omitted. It can use its own default value for that field (=the field has a default), it can disable a part of its functionality (=the field has a default of null) or it can error out gracefully (=the field is required). Whatever the behaviour should be probably specified via the defaults.

Change #1031871 merged by jenkins-bot:

[mediawiki/extensions/CommunityConfiguration@master] Number control: avoid empty string as a valid modelValue

https://gerrit.wikimedia.org/r/1031871