Bug 23625 - ArticleRequestsMandatoryFields* only affects field labels, does not make inputs required
Summary: ArticleRequestsMandatoryFields* only affects field labels, does not make inpu...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Eric Phetteplace
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-16 19:10 UTC by Eric Phetteplace
Modified: 2019-10-17 20:07 UTC (History)
4 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.05


Attachments
Bug 23625: ArticleRequestsMandatoryFields* only affects field labels, does not make inputs required (6.58 KB, patch)
2019-09-16 19:58 UTC, Eric Phetteplace
Details | Diff | Splinter Review
Bug 23625: Make new 'required' attributes match the currently used syntax in other templates (5.58 KB, patch)
2019-09-17 19:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23625: ArticleRequestsMandatoryFields* only affects field labels, does not make inputs required (6.66 KB, patch)
2019-09-17 19:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23625: Make new 'required' attributes match the currently used syntax in other templates (5.64 KB, patch)
2019-09-17 19:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23625: ArticleRequestsMandatoryFields* only affects field labels, does not make inputs required (6.75 KB, patch)
2019-09-27 07:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23625: Make new 'required' attributes match the currently used syntax in other templates (5.72 KB, patch)
2019-09-27 07:01 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 Eric Phetteplace 2019-09-16 19:10:16 UTC
Steps to recreate:

1) Enable article requests ( syspref: ArticleRequests => Enable, Circ and fine rules ALL/ALL (or a given category/itemtype): Article requests => Yes )
2) Set one or more fields to require in the ArticleRequestsMandatoryFields, ArticleRequestsMandatoryFieldsItemOnly, and/or ArticleRequestsMandatoryFieldsRecordOnly settings (all these settings are affected in the same manner)
3) Search for a title and select the "Request Article" button from the OPAC search results
4) Authenticate as a patron
5) Neglect to fill out at least one of the mandatory fields (their <label>s will have a "required" class making them appear red in the default theme)
6) Select the "Place Request" button at the bottom of the form
7) The form should not submit without the required fields but it does anyways.

The basic issue is that the opac-request-article.tt template https://github.com/Koha-Community/Koha/blob/master/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-request-article.tt creates the same non-required <input> elements regardless of whether a field is required or not. Here is one example:

<li>
    [% IF mandatory_fields.search('title') %]
        <label for="title" class="required">Title:</label>
    [% ELSE %]
        <label for="title">Title:</label>
    [% END %]
    <input type="text" name="title" id="title" size="50"/>
</li>

I believe that all of the form fields should be modified such that both the <label> AND <input> are different depending on whether a field is required or not:

<li>
    [% IF mandatory_fields.search('title') %]
        <label for="title" class="required">Title:</label>
        <input type="text" required name="title" id="title" size="50"/>
    [% ELSE %]
        <label for="title">Title:</label>
        <input type="text" name="title" id="title" size="50"/>
    [% END %]
</li>
Comment 1 Eric Phetteplace 2019-09-16 19:23:19 UTC
If it's useful for others, we're using the following snippet in OPACUserJS to work around this bug: https://github.com/cca/koha_snippets/blob/master/catalog-js/opac-request-article.js

// fields made mandatory in settings only have <label> with a "required" class
// we need to make their corresponding inputs required, too
if (location.pathname.match('/cgi-bin/koha/opac-request-article.pl')) {
    $('#place-article-request label.required').each((idx, el) => {
        let input = $(el).attr('for')
        $(`#${input}`).prop('required', true)
    })
}
Comment 2 Eric Phetteplace 2019-09-16 19:58:48 UTC
Created attachment 92846 [details] [review]
Bug 23625: ArticleRequestsMandatoryFields* only affects field labels, does not make inputs required
Comment 3 Eric Phetteplace 2019-09-16 20:16:00 UTC
I created a patch sticking closely to what the template already looked like. But I do wonder if incredibly repetitive templating like this isn't better as a loop:

[% FOREACH field IN ['title', 'author', 'volume', 'issue', 'date', 'pages' , 'chapters'] %]
    <li>
        [% IF mandatory_fields.search(field) %]
            <label for="[% field %]" class="required">[% field FILTER ucfirst %]:</label>
            <input type="text" required name="[% field %]" id="[% field %]" size="50"/>
        [% ELSE %]
            <label for="[% field %]">[% field FILTER ucfirst %]:</label>
            <input type="text" name="[% field %]" id="[% field %]" size="50"/>
        [% END %]
    </li>
[% END %]

Is there a reason that pattern isn't used here? Does it not play well with internationalization? I don't know template toolkit that well so forgive me if it doesn't work, I'm just speculating based on reading some documentation.
Comment 4 Kyle M Hall 2019-09-17 19:41:27 UTC
Created attachment 92922 [details] [review]
Bug 23625: Make new 'required' attributes match the currently used syntax in other templates
Comment 5 Kyle M Hall 2019-09-17 19:49:25 UTC
Created attachment 92923 [details] [review]
Bug 23625: ArticleRequestsMandatoryFields* only affects field labels, does not make inputs required

Test plan:

1. Enable article requests ( syspref: ArticleRequests => Enable, Circ and fine rules ALL/ALL (or a given category/itemtype): Article requests => Yes )
2. Set one or more fields to require in the ArticleRequestsMandatoryFields, ArticleRequestsMandatoryFieldsItemOnly, and/or ArticleRequestsMandatoryFieldsRecordOnly settings (all these settings are affected in the same manner)
3. Search for a title and select the "Request Article" button from the OPAC search results
4. Authenticate as a patron
5. Neglect to fill out at least one of the mandatory fields
6. Select the "Place Request" button at the bottom of the form
7. The form submits despite the empty mandatory fields
8. Apply patch
9. Repeat steps 3, 5, & 6
10 . The form should refuse to submit, show a browser-native message about the missing required fields.

Sponsored-by: California College of the Arts

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Kyle M Hall 2019-09-17 19:49:35 UTC
Created attachment 92924 [details] [review]
Bug 23625: Make new 'required' attributes match the currently used syntax in other templates

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Marcel de Rooy 2019-09-27 07:01:35 UTC
Created attachment 93182 [details] [review]
Bug 23625: ArticleRequestsMandatoryFields* only affects field labels, does not make inputs required

Test plan:

1. Enable article requests ( syspref: ArticleRequests => Enable, Circ and fine rules ALL/ALL (or a given category/itemtype): Article requests => Yes )
2. Set one or more fields to require in the ArticleRequestsMandatoryFields, ArticleRequestsMandatoryFieldsItemOnly, and/or ArticleRequestsMandatoryFieldsRecordOnly settings (all these settings are affected in the same manner)
3. Search for a title and select the "Request Article" button from the OPAC search results
4. Authenticate as a patron
5. Neglect to fill out at least one of the mandatory fields
6. Select the "Place Request" button at the bottom of the form
7. The form submits despite the empty mandatory fields
8. Apply patch
9. Repeat steps 3, 5, & 6
10 . The form should refuse to submit, show a browser-native message about the missing required fields.

Sponsored-by: California College of the Arts

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2019-09-27 07:01:40 UTC
Created attachment 93183 [details] [review]
Bug 23625: Make new 'required' attributes match the currently used syntax in other templates

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Martin Renvoize 2019-09-27 12:59:38 UTC
Nice work!

Pushed to master for 19.11.00
Comment 10 Fridolin SOMERS 2019-10-08 07:05:21 UTC
Pushed to 19.05.x for 19.05.05
Comment 11 Lucas Gass 2019-10-17 20:07:11 UTC
backported to 18.11.x for 18.11.11