Description
Eric Phetteplace
2019-09-16 19:10:16 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) }) } Created attachment 92846 [details] [review] Bug 23625: ArticleRequestsMandatoryFields* only affects field labels, does not make inputs required 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. Created attachment 92922 [details] [review] Bug 23625: Make new 'required' attributes match the currently used syntax in other templates 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> 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> 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> 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> Nice work! Pushed to master for 19.11.00 Pushed to 19.05.x for 19.05.05 backported to 18.11.x for 18.11.11 Hmm I don't know if this is working in newer versions... (In reply to David Cook from comment #12) > Hmm I don't know if this is working in newer versions... I've filed a new bug report after testing this in master: Bug 31294 - Article requests: Mandatory subfields in OPAC don't show they are required The visual is missing, bit it complains when saving. |