Page MenuHomePhabricator

Filters without name or pattern should not be saved
Closed, ResolvedPublic

Description

Recently, when in a hurry, I created a filter without name. I believe this shouldn't be allowed.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 23 2017, 3:23 PM
Huji added a subscriber: Huji.Sep 2 2017, 3:05 PM

Why is it a problem? It does not stop any functionality, does it?

The purpose of the field is to give each filter a public description. Most of the extension expects each filter has one.

The trouble here is actually bigger. Right now, you can easily save an empty filter: no title, no pattern, no selected options, anything. Of course this makes no sense. I believe we should mark as required some basic fields, i.e. title and pattern. My reasons: title is needed since it's the only public description available: people would have no idea of the broken rule. As for the pattern, it would make no sense not to write one, since it's the core of the filter itself. I'll see if I can make a commit to fix this.

Change 417842 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Mark title and pattern as required

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

Daimona claimed this task.Mar 9 2018, 12:26 PM

Added clientside requirement which should prevent any unvoluntary click. If we also want a serverside validation, we should first build a proper warning system.

Huji added a comment.Mar 9 2018, 1:55 PM

Daimona, the HTML "required" attribute doesn't do much on its own. We should implement that along with a validation system.

BTW I'm quite worried for this, since when Ace Editor will be implemented, we will almost surely need some JS trick to make the editor required: being a DIV element, it cannot take "required" property, and by putting a "required" on the hidden textarea the error message won't show (right because it's hidden).
@Huji: the "required" attribute warns the user when they try to submit the form with empty values by showing localised error messages (and prevents submitting the data). This is probably enough for someone who forgot a field. However, as I was saying above (which I wrote before reading your reply), it would probably be better if we build an independent validation system, which we may flexibly use with Ace div once ready.

Daimona renamed this task from Filters without name should not be saved to Filters without name or pattern should not be saved.Mar 9 2018, 2:27 PM

Reworked the patch to use serverside validation, basically copying the syntax check.

Huji added a comment.Mar 9 2018, 11:59 PM

@Daimona you might want to also work no T189349, now that you have figured out this part of the code!

@Huji I'd be happy to do it, but I think that we shouldn't do it. Check comment there.

Change 417842 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add requirement for title and pattern fields

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

Daimona closed this task as Resolved.Apr 2 2018, 6:35 PM