Bug 22657 - Remove JavaScript from OPAC suggestion validation of required fields
Summary: Remove JavaScript from OPAC suggestion validation of required fields
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Marcel de Rooy
URL:
Keywords: rel_19_05_candidate
Depends on:
Blocks:
 
Reported: 2019-04-08 14:47 UTC by Owen Leonard
Modified: 2019-05-30 19:30 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22657: Remove JavaScript from OPAC suggestion validation of required fields (18.09 KB, patch)
2019-04-08 14:56 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22657: Remove JavaScript from OPAC suggestion validation of required fields (18.14 KB, patch)
2019-04-12 19:55 UTC, Bin Wen
Details | Diff | Splinter Review
Screenshot of wandering required message (53.01 KB, image/png)
2019-04-17 05:58 UTC, Katrin Fischer
Details
Bug 22657: Remove JavaScript from OPAC suggestion validation of required fields (17.90 KB, patch)
2019-05-09 12:06 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22657: (QA follow-up) Restore previous version of regex (987 bytes, patch)
2019-05-09 18:04 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22657: Remove JavaScript from OPAC suggestion validation of required fields (18.01 KB, patch)
2019-05-10 07:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22657: (QA follow-up) Restore previous version of regex (1.07 KB, patch)
2019-05-10 07:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2019-04-08 14:47:07 UTC
The OPAC suggestion form required JavaScript in order to make required fields mandatory. I think we should replace this with HTML5 'required' attributes and more processing in the template. This makes the form fit better with the general goal of progressive enhancement for the OPAC.
Comment 1 Owen Leonard 2019-04-08 14:56:40 UTC Comment hidden (obsolete)
Comment 2 Bin Wen 2019-04-12 19:55:19 UTC
Created attachment 87939 [details] [review]
Bug 22657: Remove JavaScript from OPAC suggestion validation of required fields

This patch modifies the OPAC suggestion form so that it doesn't require
JavaScript for form validation. This change doesn't alter the behavior
of the form for users but does fit with the general goal of progressive
enhancement for the OPAC.

The patch adds "required" labels to required fields to better identify
required fields.

To test, apply the patch and make sure there are required fields
specified in the OPACSuggestionMandatoryFields system preference.

 - Log in to the OPAC and go to Your purchase suggestions -> New
   purchase suggestion.
 - Verify that the fields specified in OPACSuggestionMandatoryFields are
   highlighted and marked "required."
 - Verify that you can't submit the form without filling out these
   fields.

Signed-off-by: Bin Wen <bin.wen@inlibro.com>
Comment 3 Katrin Fischer 2019-04-17 05:58:52 UTC
Created attachment 88132 [details]
Screenshot of wandering required message

Something is a bit odd here: 

- Go to the OPAC
- New purchase suggestion (title is required)
- Submit
- Please fill out this field is shown
- Scroll
- Verify the note stays in the same place while the form moves

Owen, can you please check?
Comment 4 Owen Leonard 2019-04-17 12:39:29 UTC
(In reply to Katrin Fischer from comment #3)

> - Scroll
> - Verify the note stays in the same place while the form moves

The validation is done using the browser's built-in interpretation of the HTML5 attribute "required." What you see appears to be a browser bug. I see it in Firefox but not Chrome.
Comment 5 Katrin Fischer 2019-04-17 13:16:46 UTC
(In reply to Owen Leonard from comment #4)
> (In reply to Katrin Fischer from comment #3)
> 
> > - Scroll
> > - Verify the note stays in the same place while the form moves
> 
> The validation is done using the browser's built-in interpretation of the
> HTML5 attribute "required." What you see appears to be a browser bug. I see
> it in Firefox but not Chrome.

Actually, I was kind of afraid you'd say that. Should we push or wait a bit until this is fixed? on the form it's really confusing.
Comment 6 Jonathan Druart 2019-05-08 17:52:55 UTC
Sorry Owen, bug 22800 created a conflict with this one:
CONFLICT (content): Merge conflict in opac/opac-suggestions.pl
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-suggestions.tt
Comment 7 Owen Leonard 2019-05-09 12:06:13 UTC
Created attachment 89504 [details] [review]
Bug 22657: Remove JavaScript from OPAC suggestion validation of required fields

This patch modifies the OPAC suggestion form so that it doesn't require
JavaScript for form validation. This change doesn't alter the behavior
of the form for users but does fit with the general goal of progressive
enhancement for the OPAC.

The patch adds "required" labels to required fields to better identify
required fields.

To test, apply the patch and make sure there are required fields
specified in the OPACSuggestionMandatoryFields system preference.

 - Log in to the OPAC and go to Your purchase suggestions -> New
   purchase suggestion.
 - Verify that the fields specified in OPACSuggestionMandatoryFields are
   highlighted and marked "required."
 - Verify that you can't submit the form without filling out these
   fields.

Signed-off-by: Bin Wen <bin.wen@inlibro.com>
Comment 8 Jonathan Druart 2019-05-09 15:31:38 UTC
A good reason for that change:
-    @mandatoryfields = sort split(/\s*\,\s*/, $fldsreq_sp);
+    @mandatoryfields = split( ",", $fldsreq_sp );

?

I think the regex must be kept as it, just in case.
Comment 9 Owen Leonard 2019-05-09 18:04:58 UTC
Created attachment 89520 [details] [review]
Bug 22657: (QA follow-up) Restore previous version of regex

This patch restores the previous version of the regular expression which
parses the OPACSuggestionMandatoryFields system preference value.
Comment 10 Marcel de Rooy 2019-05-10 07:34:01 UTC
Created attachment 89525 [details] [review]
Bug 22657: Remove JavaScript from OPAC suggestion validation of required fields

This patch modifies the OPAC suggestion form so that it doesn't require
JavaScript for form validation. This change doesn't alter the behavior
of the form for users but does fit with the general goal of progressive
enhancement for the OPAC.

The patch adds "required" labels to required fields to better identify
required fields.

To test, apply the patch and make sure there are required fields
specified in the OPACSuggestionMandatoryFields system preference.

 - Log in to the OPAC and go to Your purchase suggestions -> New
   purchase suggestion.
 - Verify that the fields specified in OPACSuggestionMandatoryFields are
   highlighted and marked "required."
 - Verify that you can't submit the form without filling out these
   fields.

Signed-off-by: Bin Wen <bin.wen@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2019-05-10 07:34:06 UTC
Created attachment 89526 [details] [review]
Bug 22657: (QA follow-up) Restore previous version of regex

This patch restores the previous version of the regular expression which
parses the OPACSuggestionMandatoryFields system preference value.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Nick Clemens 2019-05-10 19:17:35 UTC
This is cleanup I would like to see move ahead if the browser issue is reasonable, will test next week so considering for 19.05
Comment 13 Nick Clemens 2019-05-15 17:04:33 UTC
Awesome work all!

Pushed to master for 19.05
Comment 14 Nick Clemens 2019-05-15 17:05:57 UTC
(In reply to Katrin Fischer from comment #5)
> > The validation is done using the browser's built-in interpretation of the
> > HTML5 attribute "required." What you see appears to be a browser bug. I see
> > it in Firefox but not Chrome.
> 
> Actually, I was kind of afraid you'd say that. Should we push or wait a bit
> until this is fixed? on the form it's really confusing.

I don't think this is too bad, the red highlight makes it fairly obvious which field and any click clears. Still keeping my fingers crossed for firefox to fix it
Comment 15 Martin Renvoize 2019-05-30 19:30:09 UTC
Enhancement will not be backported to 18.11.x series.