Page MenuHomePhabricator

Add error handling for the undocumented parameter input
Closed, ResolvedPublic5 Estimated Story Points

Description

Background

There are certain values which are not accepted and currently it simply seems like the input is broken. The same values are not allowed, but we will display error messages so that the user understands what is happening.

Requirements

  • Display errors when:
    • the user enters a parameter name already existing in the template (either documented or not)
    • the user enters a parameter name already existing in the template (either documented or not) that is unselected
    • the user enters the alias of a parameter already existing in the template (can only be a documented param)
    • the user enters the name/alias of a deprecated parameter
    • the user enters a forbidden character {, =, |, and }.
  • Error displayed using OOUI component "FieldLayout with error message"
  • Button to add the parameter is disabled

Mocks

Error1.png (642×800 px, 40 KB)
Undoc params.png (642×800 px, 43 KB)
Error2.png (642×800 px, 43 KB)
Error3.png (642×800 px, 41 KB)

Error message text

Param exists: "Cannot add two parameters of the same name."
Alias: "“$input” is an alias of the parameter “$conflicting parameter name” and cannot be added because it would be a duplicate."
Deprecated: "“$input” cannot be added because the parameter has been marked as deprecated."
Forbidden character: ""$inputsymbol" is a forbidden character. Please remove it to add the parameter."

Event Timeline

@Sarai-WMDE to update description and add mock up representing 4th error state: when the users search for a documented parameter or an undocumented parameter that has been added but is not selected (not present in the main form).

Lena_WMDE set the point value for this task to 5.Jul 21 2021, 1:48 PM

Minor question, maybe this should be a "parent" task of the input, ie. we first create the input and then implement error handling?

Change 715755 had a related patch set uploaded (by Andrew-WMDE; author: Andrew-WMDE):

[mediawiki/extensions/VisualEditor@master] Add input validation to the add parameter page

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

Change 717385 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/VisualEditor@master] Don't allow parameter names that break the wikitext syntax

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

@thiemowmde

Just disable the button when the input is not even a valid parameter name. Forbidden characters are {, =, |, and }.

If this is another case where the user is blocked, I think it makes sense to include an error message here as well while we are adding the rest. Such as "($symbol) is a forbidden character. Please remove it to add the parameter." Otherwise I'm not sure it would be totally clear as to why the button is disabled.

Change 717439 had a related patch set uploaded (by Andrew-WMDE; author: Andrew-WMDE):

[mediawiki/extensions/VisualEditor@master] Improve input validation for the add parameter page

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

Sure, we can show a message. However, there is still the edge-case when the input field is empty. What to do then?

Change 717466 had a related patch set uploaded (by Andrew-WMDE; author: Andrew-WMDE):

[mediawiki/extensions/VisualEditor@master] Don't allow users to add parameters that contain forbidden chars

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

Change 717385 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Don't allow parameter names that break the wikitext syntax

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

the user enters the alias of a parameter already existing in the template (can only be a documented param)

This one is slightly annoying when a required parameter is aliased. Since the required parameter cannot be unchecked, the alias can never be used.

Change 719250 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/VisualEditor@master] Add test for MWTemplateModel.getOriginalParameterName()

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

Change 719483 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/VisualEditor@master] Simplify check for forbidden characters

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

Change 719507 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/VisualEditor@master] Add test for ve.ui.MWAddParameterPage.getValidationErrors()

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

Change 719483 abandoned by Thiemo Kreuz (WMDE):

[mediawiki/extensions/VisualEditor@master] Simplify check for forbidden characters

Reason:

Squashed. Now part if I1011949.

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

Change 719507 abandoned by Thiemo Kreuz (WMDE):

[mediawiki/extensions/VisualEditor@master] Add test for ve.ui.MWAddParameterPage.getValidationErrors()

Reason:

Squashed. Now part of Iebb982e.

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

While testing the current implementation I came across an edge case:

  • a parameter (e.g. Foo) has an alias (e.g. fooalias)
  • and the alias is being used as the name of the parameter
  • if you enter the original name (Foo)
  • the error message says there is a duplicate name (although Foo is not displayed)

Screenshot from 2021-09-09 11-51-22.png (719×1 px, 139 KB)

Possible solution: display a different error message for aliases in usage

Picking up what @lilients_WMDE wrote above. For demo time or @ECohen_WMDE to look at.

Cannot add two parameters of the same name.
There is a special case when a parameter is already used, but via an alias, and the alias is shown in the sidebar. Suggestion: Cannot add a duplicate of the parameter "x". Show this messages in all cases, no matter if the sidebar shows the primary name or an alias. The "x" will repeat the user's input most of the time, but this doesn't hurt, I believe.

"a" is an alias of the parameter "y" and cannot be added because it would be a duplicate.
This is currently shown in two situations. When the parameter is already used, and when it's not. It makes a lot of sense to talk about a duplicate when the parameter is already used. But what's the reason we don't want users to add a known parameter via an alias? Isn't it more like this: "x" is an alias of the parameter "y", which is already available for use. Please check the options in the sidebar." Is this worth a separate message, or is the existing one good enough?

Change 715755 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Add input validation to the add parameter page

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

Change 717466 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Don't allow users to add parameters that contain forbidden chars

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

@ECohen_WMDE It turns out we need to specify the priority of these different cases. The issue that's been a sticking point in code review is that we can get alias errors when the alias or primary parameter is already present in the template invocation. In this case, should we show the alias error (third attached mock), or the duplicate parameter error (first attached mock)?

Change 717439 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Improve input validation for the add parameter page

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

Change 719250 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Add test for MWTemplateModel.getOriginalParameterName()

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

In this case, should we show the alias error (third attached mock), or the duplicate parameter error (first attached mock)?

After reading through, I think for this case we can simply show the alias error (third mock F34533178). From my understanding, the text still makes sense. I think this resolves all the above issues but if there is still something outstanding, let me know

Change 720991 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/VisualEditor@master] [WIP] Improve messages about duplicate template parameters

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

[…] simply show the alias error (third mock F34533178).

This would make the message claim that the actual parameter name is an "alias". Technically this is not wrong. It might still be confusing.

We reviewed this ticket during demo time. It was implemented according the requirements and can be closed. A few behaviors were noted as confusing and will be dealt with in a follow-up ticket T291059: [DRAFT] Adjust messages for special alias edge cases (currently a draft, ideal behavior TBD).