Bug 6209 - Standardize on one javascript library for client-side form validation
Summary: Standardize on one javascript library for client-side form validation
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Owen Leonard
QA Contact: Paul Poulain
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-19 16:15 UTC by Owen Leonard
Modified: 2014-05-26 21:04 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 6209 - Standardize on one plugin for client-side form validation (28.15 KB, patch)
2012-03-22 09:34 UTC, Owen Leonard
Details | Diff | Splinter Review
Another form validation plugin (Ketchup) (169.36 KB, patch)
2012-03-22 14:40 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6209 - Standardize on one plugin for client-side form validation (31.91 KB, patch)
2013-02-01 17:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 6209 [Revised] Standardize on one plugin for client-side form validation (32.33 KB, patch)
2013-02-07 14:44 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 6209 [Revised] Standardize on one plugin for client-side form validation (32.21 KB, patch)
2013-03-01 15:17 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6209 [Revised] Standardize on one plugin for client-side form validation (32.31 KB, patch)
2013-03-01 21:21 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6209 [Revised] Standardize on one plugin for client-side form validation (32.07 KB, patch)
2013-03-22 01:14 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 6209 follow-up removing tabs (62.71 KB, patch)
2013-03-22 11:15 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 6209 follow-up, make strings translatable (2.82 KB, patch)
2013-03-22 11:15 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2011-04-19 16:15:11 UTC
We handle client-side validation in a piecemeal fashion. It would simplify things for the user and developers if we standardized on one JavaScript library for handling all client-side form validation.

I propose we use this one:

http://bassistance.de/jquery-plugins/jquery-plugin-validation/

Syntax is pretty simple:

In the JavaScript:

  $(document).ready(function(){
    $("#commentForm").validate();
  });

In the markup:

 <form id="commentForm" method="get" action="">
 <input id="cname" name="name" size="25" class="required" minlength="2" />

 "class='required'" tells the script to validate that field as required. Other options include email, url, date, number, specific length, etc.
Comment 1 Paul Poulain 2011-04-27 09:15:42 UTC
Owen, this idea sounds interesting. But what about CSS3 and html5 and advanced form validation ? won't your proposal result in some work to be done again once browser accept those advanced new features ?
Comment 2 Owen Leonard 2012-03-20 13:36:31 UTC
I'm taking this up again today and doing tests with the plugin. In the release notes for the latest version (http://bassistance.de/2011/10/07/release-validation-plugin-1-9-0/):

"Most notable is heavily improved compability with HTML5 controls: You can apply validation rules to input types like number, email or url, it’ll get picked up by the plugin if the type matches a rule, and a required attribute (with the argument) also works with both jQuery 1.6+ (prop) and previous versions (attr)."

I will keep this in mind as I do my tests.
Comment 3 Owen Leonard 2012-03-22 09:34:52 UTC Comment hidden (obsolete)
Comment 4 Julian Maurice 2012-03-22 14:40:25 UTC Comment hidden (obsolete)
Comment 5 Julian Maurice 2012-03-22 14:45:34 UTC
Note about the patch above: branchcode and branchname have been marked as 'required', Email as 'email', and url as 'url'.
Submitting a form with errors inside causes window to scroll to the top of the page and all errors are displayed in front of corresponding inputs.
Comment 6 Paul Poulain 2012-03-22 14:49:15 UTC
(In reply to comment #4)
> I just wanted to propose another form validation plugin, Ketchup.

I have no opinion on which one is the best, but, as you're in the same room, maybe it's worth a small discussion to share arguments!
Comment 7 Owen Leonard 2012-03-22 16:22:50 UTC
> I have no opinion on which one is the best, but, as you're in the same room,
> maybe it's worth a small discussion to share arguments!

We did, and I asked Julian to submit a simple patch demonstrating his version so that I could test and so that others could give their opinions. I'd be very interested in hearing what others think.
Comment 8 Owen Leonard 2012-08-14 16:24:31 UTC
I'm still trying to make a decision on this, and at the moment I'm still on the side of Jörn Zaefferer's Validation plugin if only because it's is under more active development. Points also go to Zaefferer for creating the original tabs plugin which was incorporated into jQueryUI.

However, I want to fully understand these concerns:

(In reply to comment #4)
> 
> but I don't like the way we have to declare
> "validation instructions" in HTML tags with Validation plugin: some of them
> can be declared in class attribute (which is designed for styling)

I don't think we should worry about "abusing" the class attribute. It's perfectly reasonable to use the class attribute for semantically-meaningful purposes. We use class selectors in many places in Koha jQuery scripts.

> others have to be declared in separate non-standard attributes (eg:
> minlength="2") (by the way, how do we declare "instructions" with more than
> 1 parameter? the documentation is not clear about this).

The Validation plugin lets you define multiple parameters if you're doing the configuration in your JavaScript.

It sounds like Ketchup would give us the benefit of being able to define more complex validation parameters directly in HTML tag attributes. Validation would require that we do so in JavaScript. Does that get to the essence of the difference?
Comment 9 Owen Leonard 2013-02-01 17:52:27 UTC Comment hidden (obsolete)
Comment 10 Galen Charlton 2013-02-07 00:34:06 UTC
Looking good overall, but I've got a nit to pick:

--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/branches.tt
@@ -31,6 +31,7 @@ tinyMCE.init({
     height : "300",
     width : "700"
 });
+    //]]>
 </script>

That added line doesn't appear to correspond to a //<![CDATA[

That aside, I'm close to being ready to sign off on this.
Comment 11 Owen Leonard 2013-02-07 14:44:15 UTC Comment hidden (obsolete)
Comment 12 Owen Leonard 2013-03-01 15:17:19 UTC Comment hidden (obsolete)
Comment 13 Bernardo Gonzalez Kriegel 2013-03-01 21:21:58 UTC Comment hidden (obsolete)
Comment 14 Paul Poulain 2013-03-21 16:31:59 UTC
Sorry owen, but this patch does not apply anymore and it's out of my skills to fix the conflict in branches.tt

Please rebase & set back to "signed off", i'll take care (hopefully tomorrow if you rebase during my european night ;-) )
Comment 15 Owen Leonard 2013-03-22 01:14:36 UTC
Created attachment 16701 [details] [review]
[SIGNED-OFF] Bug 6209 [Revised] Standardize on one plugin for client-side form validation

This patch adds the jQuery "Validation" plugin
(http://docs.jquery.com/Plugins/Validation) and includes it by
default in the staff client along with an include file containing
translatable strings for all of the plugin's possible messages.

The staff client's main JavaScript file has been modified to perform
automatic validation of any form with the class "validated."

For a simple example I have modified the library entry form
to mark required fields as required (something which had not
been done before). Each required field has been given a class,
"required" and the HTML5 attribute "required." The former adds
the hook for the plugin, the latter adds browser-based validation
in recent browsers (and which will be superceded by the plugin
if JavaScript is enabled).

This will check for inputs classed "required" without any
additional configuration. More complex forms (with specific
data types, for instance) will require more inline JavaScript
configuration.

$("#FormID").validate({ /* configuration details */ });

Also added to the library entry form are checks on the email and URL
fields via classes ("email" and "url", respectively). This demonstrates
validation of content without forcing the field to be required.

To test, go to Administration -> Libraries and Groups and choose
"New library." Submit the form without entering a library code
or name. You should be prevented from submitting the form and be
shown an error message by each field. Perform the same check with an
existing entry.

To test browser-based validation, disable JavaScript and follow
the same procedure.

Revision rebases the patch against current master and adds missing
"//<![CDATA["

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Comment: Work as described with new or existing library.
No errors.
Comment 16 Paul Poulain 2013-03-22 11:13:40 UTC
QA comment:
 * passes koha-qa.pl
 * work fine

BUT : it seems the strings can't be translated. I've updated the french po, and strings like "This field is required." are not in the .po file.
I've made a follow-up to fix this mistake, the feature work with it and the strings appear in the .po file

passed QA
Comment 17 Paul Poulain 2013-03-22 11:15:27 UTC Comment hidden (obsolete)
Comment 18 Paul Poulain 2013-03-22 11:15:33 UTC
Created attachment 16772 [details] [review]
[PASSED QA] Bug 6209 follow-up, make strings translatable

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Comment 19 Jared Camins-Esakov 2013-03-23 04:13:47 UTC
This patch has been pushed to master. I did not push the tab follow-up because it did not apply. As the tabs were in Javascript, I didn't care so much.