Page MenuHomePhabricator

Don't drop fields with incorrect format values
Closed, ResolvedPublic3 Story Points

Description

If a user enters a field value which does not match the specified field format, we shouldn't silently drop the input but instead give them a popup -
"One or more of your inputs don't match the expected format. Do you wish to edit it?"
Options: "Edit template", "Insert template"
When the user hits "Edit template", we take them back to the form with a red border on fields with a mismatch.
When the user hits "Insert template", we insert the template without dropping the mismatch field.

Event Timeline

Niharika created this task.Apr 2 2018, 11:47 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 2 2018, 11:47 PM
JJMC89 added a subscriber: JJMC89.Apr 3 2018, 12:44 AM

When the user hits "Insert template", we insert the template without dropping the mismatch field.

Why would we want to allow the insertion of malformed values?

Users should have to fix it, or it should be removed.

When the user hits "Insert template", we insert the template without dropping the mismatch field.

Why would we want to allow the insertion of malformed values?
Users should have to fix it, or it should be removed.

There is always the possibility that TemplateData does not reflect what the field should contain accurately. See example pointed out by @Tacsipacsi.
In that case, we should trust the editor's judgement.

I tend to agree, although I think there are a few instances of templatedata that are overly-restrictive (e.g. ISBN as 'number') and that maybe the real fix is to ensure that the templatedata is correct.

JJMC89 added a comment.Apr 3 2018, 1:11 AM

The TemplateData should be corrected when it is incorrect. ISBN should be a string, not a number.

The TemplateData should be corrected when it is incorrect. ISBN should be a string, not a number.

I agree with that but not all users can be expected to know what is causing the format mismatch or how to fix it. TemplateData is fairly complex to edit and can cause significant problems if edited incorrectly. Hence I am not very inclined to ask users to fix TemplateData if they think it is wrong.

Niharika set the point value for this task to 3.Apr 3 2018, 11:03 PM
Samwilson edited projects, added Community-Tech-Sprint; removed Community-Tech.

Change 425479 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/extensions/TemplateWizard@master] Add verification message when inserting invalid fields

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

The above patch depends on T192057 being resolved.

@Samwilson I pulled the patch on commtech wiki after updating the extension repo, and updated the Test template to have an age field which is numeric only. I still see the field being dropped if I enter anything other than numbers in it.

This seems to be a deeper bug than it first looks (see also T150455). Number fields are type="number" and browsers hide non-numerical values for those (i.e. .val() returns an empty string).

It seems like the best option is to change NumberInputWidget to act like other widgets and not even allow non-numeric data to be entered e.g. try typing something into a date field, and it just gets emptied when you move away from the field. Oh, no actually I'm wrong: it *sort of* works like that, but if you enter a valid date, and then change it to a invalid string, the valid date is what's kept — even though, when you click back into the date field, the invalid string is what's there. This feels a bit messy! But it works in practice to prevent invalid dates from being entered.

Basically, out of the box, we don't seem to get access to the invalid values of these things, but only the validated values. This is fine for most inputs, because page names and things are just text fields, but for date and number there is format validation happening. I think numbers should be treated as dates are (or sort-of are), and we shouldn't permit non-numerical dates to be entered. Does that sound correct?

This seems to be a deeper bug than it first looks (see also T150455). Number fields are type="number" and browsers hide non-numerical values for those (i.e. .val() returns an empty string).
It seems like the best option is to change NumberInputWidget to act like other widgets and not even allow non-numeric data to be entered e.g. try typing something into a date field, and it just gets emptied when you move away from the field. Oh, no actually I'm wrong: it *sort of* works like that, but if you enter a valid date, and then change it to a invalid string, the valid date is what's kept — even though, when you click back into the date field, the invalid string is what's there. This feels a bit messy! But it works in practice to prevent invalid dates from being entered.
Basically, out of the box, we don't seem to get access to the invalid values of these things, but only the validated values. This is fine for most inputs, because page names and things are just text fields, but for date and number there is format validation happening. I think numbers should be treated as dates are (or sort-of are), and we shouldn't permit non-numerical dates to be entered. Does that sound correct?

That sounds good to me. Let's do that.

Aside, how is it that image-file fields attempt to show possible images but wiki-page fields don't attempt to auto-complete page names?

Change 427592 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/core@master] Set number inputs to empty if their DOM value is empty

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

Change 427835 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[oojs/ui@master] Set number inputs to empty if their DOM value is empty

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

Change 427592 abandoned by Samwilson:
Set number inputs to empty if their DOM value is empty

Reason:
In favour of: https://gerrit.wikimedia.org/r/#/c/427835/

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

Change 427835 merged by jenkins-bot:
[oojs/ui@master] NumberInputWidget: Set inputs to empty if their DOM value is empty

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

Aside, how is it that image-file fields attempt to show possible images but wiki-page fields don't attempt to auto-complete page names?

Type wiki-page-name should be working with autocomplete. What template are you testing with?

https://gerrit.wikimedia.org/r/#/c/425479 is ready for review, if anyone's got a mo'.

The number field issue is fixed (in ooui; if you're testing locally, make sure you've got the latest core and run composer update).

Change 425479 merged by jenkins-bot:
[mediawiki/extensions/TemplateWizard@master] Add verification message when inserting invalid fields

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

Niharika closed this task as Resolved.May 1 2018, 4:39 PM
Niharika moved this task from Needs Review/Feedback to Q1 2018-19 on the Community-Tech-Sprint board.

Thanks, Sam.