Bug 23925 - Value builder plugin to check ISBN validity
Summary: Value builder plugin to check ISBN validity
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: paxed
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-10-30 09:17 UTC by paxed
Modified: 2020-04-21 13:17 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 23925: Value builder plugin to check ISBN validity (8.96 KB, patch)
2019-10-30 09:19 UTC, paxed
Details | Diff | Splinter Review
Bug 23925: Value builder plugin to check ISBN validity (8.99 KB, patch)
2020-03-03 08:29 UTC, paxed
Details | Diff | Splinter Review
Bug 23925: Value builder plugin to check ISBN validity (9.05 KB, patch)
2020-03-06 12:12 UTC, Devinim
Details | Diff | Splinter Review
Bug 23925: Value builder plugin to check ISBN validity (9.09 KB, patch)
2020-03-06 21:38 UTC, Andreas Roussos
Details | Diff | Splinter Review
incorrect display of input field when the ISBN is wrong (6.13 KB, image/png)
2020-03-06 21:43 UTC, Andreas Roussos
Details
correct display of input field when the ISBN is wrong (6.11 KB, image/png)
2020-03-06 21:44 UTC, Andreas Roussos
Details
Bug 23925: (follow-up) Add missing CSS classes (1.10 KB, patch)
2020-03-06 21:55 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 23925: Value builder plugin to check ISBN validity (9.14 KB, patch)
2020-03-08 21:58 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 23925: (follow-up) Add missing CSS classes (1.15 KB, patch)
2020-03-08 21:58 UTC, Michal Denar
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description paxed 2019-10-30 09:17:44 UTC
This patch adds a new value builder plugin ISBN.pl, which should
be attached to field 020a. The plugin will perform a simple
checksum checking when trying to save the record, and inform the
user that the ISBN is illegal if the checksum doesn't match.

The ISBN plugin is a test case for additional value builder plugin
functionality that lets the plugins check field values and
prevent saving if so desired. The example plugin has been updated
with information on how to write a pre-save check function.

To test:
1) Add the ISBN.pl plugin to the field 020a in MARC subfield
   structure admin.
2) Go to cataloguing and try to save the record, no complaints
   about the ISBN (Because the plugin will not complain about
   empty field)
3) Enter something in the 020a field and try to save, you should
   receive a complaint about illegal ISBN.
4) Enter a valid ISBN (eg. 978-951-1-27641-8) and try to save,
   there should be no complaints about the ISBN.
Comment 1 paxed 2019-10-30 09:19:24 UTC Comment hidden (obsolete)
Comment 2 Bernardo Gonzalez Kriegel 2020-02-29 19:41:14 UTC
Sorry, needs rebase

CONFLICT (content): Merge conflict in koha-tmpl/intranet-CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
Comment 3 paxed 2020-03-03 08:29:41 UTC
Created attachment 99977 [details] [review]
Bug 23925: Value builder plugin to check ISBN validity

This patch adds a new value builder plugin ISBN.pl, which should
be attached to field 020a. The plugin will perform a simple
checksum checking when trying to save the record, and inform the
user that the ISBN is illegal if the checksum doesn't match.

The ISBN plugin is a test case for additional value builder plugin
functionality that lets the plugins check field values and
prevent saving if so desired. The example plugin has been updated
with information on how to write a pre-save check function.

To test:
1) Add the ISBN.pl plugin to the field 020a in MARC subfield
   structure admin.
2) Go to cataloguing and try to save the record, no complaints
   about the ISBN (Because the plugin will not complain about
   empty field)
3) Enter something in the 020a field and try to save, you should
   receive a complaint about illegal ISBN.
4) Enter a valid ISBN (eg. 978-951-1-27641-8) and try to save,
   there should be no complaints about the ISBN.
Comment 4 Devinim 2020-03-06 12:12:04 UTC
Created attachment 100267 [details] [review]
Bug 23925: Value builder plugin to check ISBN validity

This patch adds a new value builder plugin ISBN.pl, which should
be attached to field 020a. The plugin will perform a simple
checksum checking when trying to save the record, and inform the
user that the ISBN is illegal if the checksum doesn't match.

The ISBN plugin is a test case for additional value builder plugin
functionality that lets the plugins check field values and
prevent saving if so desired. The example plugin has been updated
with information on how to write a pre-save check function.

To test:
1) Add the ISBN.pl plugin to the field 020a in MARC subfield
   structure admin.
2) Go to cataloguing and try to save the record, no complaints
   about the ISBN (Because the plugin will not complain about
   empty field)
3) Enter something in the 020a field and try to save, you should
   receive a complaint about illegal ISBN.
4) Enter a valid ISBN (eg. 978-951-1-27641-8) and try to save,
   there should be no complaints about the ISBN.

Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Signed-off-by: Devinim <kohadevinim@devinim.com.tr>

Nice plugin.
Tested for both 13 and 10 digits ISBNs
Comment 5 Andreas Roussos 2020-03-06 21:38:20 UTC
Created attachment 100296 [details] [review]
Bug 23925: Value builder plugin to check ISBN validity

This patch adds a new value builder plugin ISBN.pl, which should
be attached to field 020a. The plugin will perform a simple
checksum checking when trying to save the record, and inform the
user that the ISBN is illegal if the checksum doesn't match.

The ISBN plugin is a test case for additional value builder plugin
functionality that lets the plugins check field values and
prevent saving if so desired. The example plugin has been updated
with information on how to write a pre-save check function.

To test:
1) Add the ISBN.pl plugin to the field 020a in MARC subfield
   structure admin.
2) Go to cataloguing and try to save the record, no complaints
   about the ISBN (Because the plugin will not complain about
   empty field)
3) Enter something in the 020a field and try to save, you should
   receive a complaint about illegal ISBN.
4) Enter a valid ISBN (eg. 978-951-1-27641-8) and try to save,
   there should be no complaints about the ISBN.

Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Signed-off-by: Devinim <kohadevinim@devinim.com.tr>
Signed-off-by: Andreas Roussos <a.roussos@dataly.gr>
Comment 6 Andreas Roussos 2020-03-06 21:43:49 UTC
Created attachment 100299 [details]
incorrect display of input field when the ISBN is wrong
Comment 7 Andreas Roussos 2020-03-06 21:44:36 UTC
Created attachment 100300 [details]
correct display of input field when the ISBN is wrong
Comment 8 Andreas Roussos 2020-03-06 21:49:54 UTC
I've tested the plugin on both a MARC21 and a UNIMARC installation
and it works fine, except for a minor hiccup in the display of the
highlighted input field when the ISBN is incorrect (see attached
screenshots).

This is due to a couple of missing CSS classes for the HTML <input>
element. I will provide a follow-up that addresses this issue.
Comment 9 Andreas Roussos 2020-03-06 21:55:44 UTC
Created attachment 100301 [details] [review]
Bug 23925: (follow-up) Add missing CSS classes

The highlighted input field for an incorrect ISBN was displayed
incorrectly -- this patch fixes that.
Comment 10 Michal Denar 2020-03-08 21:58:52 UTC
Created attachment 100318 [details] [review]
Bug 23925: Value builder plugin to check ISBN validity

This patch adds a new value builder plugin ISBN.pl, which should
be attached to field 020a. The plugin will perform a simple
checksum checking when trying to save the record, and inform the
user that the ISBN is illegal if the checksum doesn't match.

The ISBN plugin is a test case for additional value builder plugin
functionality that lets the plugins check field values and
prevent saving if so desired. The example plugin has been updated
with information on how to write a pre-save check function.

To test:
1) Add the ISBN.pl plugin to the field 020a in MARC subfield
   structure admin.
2) Go to cataloguing and try to save the record, no complaints
   about the ISBN (Because the plugin will not complain about
   empty field)
3) Enter something in the 020a field and try to save, you should
   receive a complaint about illegal ISBN.
4) Enter a valid ISBN (eg. 978-951-1-27641-8) and try to save,
   there should be no complaints about the ISBN.

Signed-off-by: Pasi Kallinen <pasi.kallinen@koha-suomi.fi>
Signed-off-by: Devinim <kohadevinim@devinim.com.tr>
Signed-off-by: Andreas Roussos <a.roussos@dataly.gr>

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 11 Michal Denar 2020-03-08 21:58:55 UTC
Created attachment 100319 [details] [review]
Bug 23925: (follow-up) Add missing CSS classes

The highlighted input field for an incorrect ISBN was displayed
incorrectly -- this patch fixes that.

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 12 Jonathan Druart 2020-03-17 13:55:39 UTC
"Illegal ISBN" will not be translatable.
Comment 13 Andreas Roussos 2020-04-21 06:43:05 UTC
(In reply to Jonathan Druart from comment #12)
> "Illegal ISBN" will not be translatable.
Hello, Jonathan. Can you elaborate, please? From what I can see Pasi has
followed the coding guideline (JS2: Enabling translation of language
strings in embedded JavaScript) and wrapped the string in the special
function _(), like so:

return { funcname: id, msg: _("Illegal ISBN") };
Comment 14 Jonathan Druart 2020-04-21 09:00:33 UTC
Hi Andreas,

This is not an "embedded javascript" (in .tt). With bug 21156 it is now possible to have translated string in .js, but the function is different.
The correct guideline is JS5 (but make sure you read the note at the beginning).

Basically you need to replace _() with __() (simple vs double underscore).
Comment 15 Andreas Roussos 2020-04-21 13:08:06 UTC
Thanks for your prompt reply, Jonathan.
I tried to write a follow-up and replaced the _() with __()
However, when I run:

cd misc/translator && ./translate update fr-FR

Then, none of the files in misc/translator/po/fr-FR-*.po contain the
string "Illegal ISBN" for translation.

Is this because the string is part of embedded JS in a .pl file?
(cataloguing/value_builder/ISBN.pl)
Comment 16 Jonathan Druart 2020-04-21 13:17:40 UTC
Ha! Yes!
I was wondering why I did not follow-up myself for such simple fix.

Actually what needs to be done is: pass an error code to the template, then test it and make the string translatable from there.
Does it make sense?