We are moving to Git Issues for bug tracking in future releases. During transition, content will be in both tools. If you'd like to file a new bug, please create an issue.

Bug 8433 - form validations
form validations
Status: NEW
Product: OJS
Classification: Unclassified
Component: General
3.0b
All All
: P3 normal
Assigned To: PKP Support
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-28 04:54 PDT by Bozana Bokan
Modified: 2013-10-09 07:48 PDT (History)
1 user (show)

See Also:
Version Reported In:
Also Affects:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Bozana Bokan 2013-09-28 04:54:19 PDT
Hello,

I think forms i.e. form validation in OJS 3.0 is not consistent and not totally correct. I will only consider input fields and drop-down lists/selections here:
1. If the attribute "required=true" is used for example in fbvElement:
Input Fields:
The char "*" will be displayed. 
There will be no JavaScript validation (with the message "This field is required").
Drop-down list:
The char "*" will be displayed.
There will be the JavaScript validation.

2. If a FormValidator (and no "required=true") is used:
Input Fields:
The element will get the attribute "validation=required" and in connection with that there will be first the JavaScript validation and then the FormValidator validation. I.e. the message "This field is required." will be shortly displayed and overridden with the correct FormValidator message.
Drop-down lists:
There will be no JavaScript validation, just the FormValidator validation.

Using both form elements in one form would thus lead to different results/reactions.
Furthermore, I think it is wrong to bind the JavaScript validation to the FormValidator, as this is the case for input fields now. An input field doesn't necessarily have to contain something, if it has a FormValidator -- For example in a case of a CustomFormValidator, where the content of the input field for example depends on another form element. Thus the FormValidator should only decide/validate it.

Thus, I think there should be a consistent solution for everything. Maybe:
a) If it would be possible to first check, if there is a FormValidator for an element, in order not to allow the JavaScript validation in that case, then:
Use "required=true" to display "*" and for the JavaScript validation. As I said, if there is a FormValidator don't use the JavaScript but the FormValidator validation.
b) If it is not possible to prevent the JavaScript validation, if a FormValidator exists, which I am afraid is the case, then:
b1) Use an attribute just to display "*" (e.g. "required=true" or something else).
b2) Use another attribute to bind the JavaScript validation (e.g. "validation=true/required" or "required=true" if not used in #b1 for "*" or something else).
b3) Don't bind the JavaScript validation to FormValidators (at the moment using/inserting "validation=required").

I definitely have no overview of the system and what is possible and what is not possible and why the decisions are as they are, so I could maybe be totally wrong. Sorry, sorry, sorry if it is so! :-\ -- It is just what I've realized trying/struggling to construct the DOI/URN settings form to function correctly and looking at a few other form examples in the system.

I would be happy about an explanation and even more about a better way to solve this :-)

Thanks a lot!!!
Bozana
Comment 1 Bozana Bokan 2013-09-30 08:00:53 PDT
Hmmm... Apparently, there is also a difference if you define/add the FormValidators in the form constructor or in the method "validate". 
If defined in the constructor, the behavior is like I mentioned above, in my first comment. 
If defined in the method "validate" the form validation functions better, I think, but there are still some strange (JavaScript?) things to consider. You could see it for example in the IssueDataForm:
1. Select Volume, Number and Year check-box. Insert some numbers into the Volume and Number input field, leave the Year input field empty. Save the form. 
2. Now you only see the message "Year is required and must be a positive, numeric value.", which is correct, I think. /* And not first the JavaScript message "This field is required.", that is immediately overridden with this correct one, as when the FormValidator would be defined/added in the form constructor. */
3. Go to the Number input field and delete the content from the input field. Now you see the JavaScrit message "This field is required.". Which is OK here and in this way, I suppose.
4. Got to the Year field and insert a number into it. The label "Year" disappears.
I.e. only this disappearing problem seems to be here.

And maybe something else:
Lets say the editor first wanted to have all numbers (Volume, Number, Year) for the issues identifications, thus for example the check-box Volume is selected and there is a number in the Volume input field. But then the editor decides that the volume number is not needed, goes to the form and deletes the content of the Volume input field, but forgets to deactivate the Volume check-box and saves the form. The editor becomes the message "Volume is required and must be a positive, numeric value.". Then the editor deactivates the Volume check-box and saves the form. Now the editor sees also the message "This field is required." close to the Volume check-box and is not able to do anything except to start the form anew.
Comment 2 Jason Nugent 2013-10-09 06:49:49 PDT
Hi Bozana,

Thanks for the detailed bug report.  You're right, form validation is a difficult thing to solve correctly.  Although we have tried to be consistent about it, there are a few different ways that it gets implemented and maybe we need to think about standardizing it to make it more robust and consistent. A few of my own observations.

1. The client-side validation done in JavaScript only works if the necessary bits are used when building the templates with the form builder vocabulary.  In particular, there seems to be two ways to mark a field as required.  Using 'required=true' does mark a field as required (and therefore, it must be filled in) but we also support things like required=number. The FBV parses out the 'required' parameter from the fbvElement and adds it to the generated HTML element.  I am pretty sure this gets interpreted by plugins to jQuery for validation.

2.  There are situations when form elements depend on each other and are either required or not, depending on the context in which they are available.  This would obviously need to be handled client side, probably with custom Form handlers, since the elements would need to be enabled or disabled based on context.  Server side validation through custom form validators after this are probably also necessary in order to make sure that the data stays consistent, but perhaps those form handlers should be backups and cause the form to generate a fatalError instead of returning a validation message.  Maybe we need to pick one mechanism for returning form validations and use the server side one for data consistency.  After all, all OJS 3.0 and OMP forms now have client side form handlers that support validation.

3.  I also like the idea of putting validation handlers in the validate() method.  We need to make sure that we call it.  I haven't checked recently, but there might be places in grid controllers where forms are instantiated through constructors (which would normally add a validator if it was defined in the constructor) but might be used without validate() being called.

4. In any case, it does sound like the IssueDataForm is a bit broken. I'll take a look at that specific example and see if I can make it consistent, and perhaps come back with better ideas.

Cheers,
Jason
Comment 3 Jason Nugent 2013-10-09 07:48:53 PDT
Okay, so an update on the IssueForm, specifically.  This is why the validation behaviour seems inconsistent.

That form has no required-true elements in the template.  This is because the elements for the issue data are dependent on the checkboxes that determine which ones should be used for issue identification.

When the form is submitted, the validate() method is called by the grid handler.  This looks at the form elements and and then adds FormValidator checks for the text fields that are required, based on which checkboxes are selected.

The problem is that if the issue form fails validation at this point, the form is fetched() back to the client side and re-displayed.  Because the validate() method has been called (now), the form is reloaded with the checks added and those get handed to the client side JS validator. If you examine the CSS on the elements before and after a form re-submit you will see the difference.

I guess that this is the problem with putting the addition of checks in the validate() method.  They do not exist when the form is first loaded, only after it is submitted at least once.  And in this case, each time the form is submitted, the validation checks may change depending on what checkboxes are chosen. 

The grid handler does this:

if ($issueForm->validate($request)) {
    $issueId = $issueForm->execute($request);
    return DAO::getDataChangedEvent($issueId);
} else {
    $json = new JSONMessage(true, $issueForm->fetch($request));
    return $json->getString();
}

I think that in order to ensure consistent behaviour here, the else statement needs to do something besides just calling fetch().  It probably needs to reset with a new IssueForm() call, followed by a readInputData() instead of initData(), and then a fetch().  

Or, we change this pattern to something else and do the checkbox/input field validation completely client side, in a custom form handler.  

Regards,
Jason