Bug 906 - ISBN Check
Summary: ISBN Check
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P2 enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Paul Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-01-22 02:15 UTC by Pascale Nalon
Modified: 2017-09-12 18:05 UTC (History)
11 users (show)

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


Attachments
Bug 906 - ISBN Check (8.52 KB, patch)
2013-03-15 19:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 906 - ISBN Check (8.66 KB, patch)
2013-03-15 19:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 906 - ISBN Check (8.98 KB, patch)
2013-03-16 15:54 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-20 23:36:37 UTC


---- Reported by pascale.nalon@ensmp.fr 2005-01-22 02:15:56 ----

Check the ISBN, in marc editor.
Sometimes ISBN is wrong on the book, so it should be nice to check it and report
it in 010z (in unimarc) if wrong.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-20 23:36 UTC  ---

This bug was previously known as _bug_ 906 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=906

Actual time not defined. Setting to 0.0

Comment 1 Tomás Cohen Arazi 2011-03-11 17:51:50 UTC
Suppose we check ISBN (ISBN10 or ISBN13) is invalid. What should be the system behaviour?
Comment 2 Pascale Nalon 2011-04-05 12:51:07 UTC
(In reply to comment #1)
> Suppose we check ISBN (ISBN10 or ISBN13) is invalid. What should be the system
> behaviour?

First, alert the user, and second ask it if he wants to put the wrong ISBN in the right field for it (010$z in unimarc)
Comment 3 Tomás Cohen Arazi 2013-03-15 19:22:05 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2013-03-15 19:27:32 UTC Comment hidden (obsolete)
Comment 5 Bernardo Gonzalez Kriegel 2013-03-16 15:54:56 UTC
Created attachment 16178 [details] [review]
[SIGNED-OFF] Bug 906 - ISBN Check

In the MARC editor, have the ISBN checked. This feature is syspref-driven [1].
The patch adds the following javascript functions to cataloguing/addbiblio.tt:

 - _NormalizeISBN (for sanitizing the ISBN before algorithm checking)
 - _CheckISBNChecksum10 (for validating the checksum digit)
 - _CheckISBNChecksum13 (idem)
 - IsISBNOk (extract the isbn from the corresponding field, call the proper checker functions, return the string message)

The Check function was hooked to call IsISBNOk and build a proper message dialog. AreMandatoriesNotOk got its message string rearranged too.

To test:
- Apply the patch, run updatedatabase.pl so the new syspref gets included on the DB.
- Try adding a record: no matter what you introduce on the corresponding ISBN field Koha will behave as usual.
- Go to the Sysprefs editor, under 'Cataloguing' set RequireValidISBN to 'Require'.
- Reload your marc editor
- Try different ISBN to see how it behaves:
  -- test cases --
  * Empty ISBN should not stop you from saving your record.
  * Valid ISBN idem [2].
  * Wrong ISBN should make Koha warn you and ask you to fix it [3].

[1] Added RequireValidISBN syspref.
[2] Valid ISBN10: 291409891X
          ISBN13: 9784873113685
[3] Try To+

Sponsored-by: Universidad Nacional de Córdoba
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Comment: Work as described. No errors.
Tested with 10 and 13 ISBN, and random string.

Suggestion: If problem is in check digit, could suggest
correct value?
Comment 6 Paul Poulain 2013-03-21 15:49:36 UTC
QA comment:
* I think I should fail QA the patch as it is written : there is a plugin/value builder system in Koha, that is used for a lot of things, like coded fields, but not only. Why didn't you use this system ? It would not need to check between MARC21/UNIMARC/NORMARC, and no need of a syspref. look in cataloguing/value_builder/, where plugins are. The addbiblio.pl part could easily be ported to value_builder_checkisbn.pl, so the change should not be too hard to do.

* passes koha-qa.pl

failing QA, but asking katrin's opinion, as she's the QA manager.
Comment 7 Jared Camins-Esakov 2013-03-21 16:05:00 UTC
(In reply to comment #6)
> QA comment:
> * I think I should fail QA the patch as it is written : there is a
> plugin/value builder system in Koha, that is used for a lot of things, like
> coded fields, but not only. Why didn't you use this system ? It would not
> need to check between MARC21/UNIMARC/NORMARC, and no need of a syspref. look
> in cataloguing/value_builder/, where plugins are. The addbiblio.pl part
> could easily be ported to value_builder_checkisbn.pl, so the change should
> not be too hard to do.
> 
> * passes koha-qa.pl
> 
> failing QA, but asking katrin's opinion, as she's the QA manager.

I don't think a check like this can be done with a value_builder plugin. If I am wrong, a value_builder plugin would definitely be better, but the plugins aren't -- so far as I understand -- used for validation but rather as helpers for setting fields.
Comment 8 Paul Poulain 2013-03-21 16:25:08 UTC
(In reply to comment #7)
> I don't think a check like this can be done with a value_builder plugin. If
> I am wrong, a value_builder plugin would definitely be better, but the
> plugins aren't -- so far as I understand -- used for validation but rather
> as helpers for setting fields.

You can put a hook on entering or leaving a field. If you put a hook on leaving ISBN field, then it will be a validation. We use that for some plugins already. The ... on the right are not necessary used to open a popup, then can be used to do a check as well.
Comment 9 Tomás Cohen Arazi 2013-03-21 19:58:42 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > I don't think a check like this can be done with a value_builder plugin. If
> > I am wrong, a value_builder plugin would definitely be better, but the
> > plugins aren't -- so far as I understand -- used for validation but rather
> > as helpers for setting fields.
> 
> You can put a hook on entering or leaving a field. If you put a hook on
> leaving ISBN field, then it will be a validation. We use that for some
> plugins already. The ... on the right are not necessary used to open a
> popup, then can be used to do a check as well.

How would the workflow be in case of bad ISBN Paul? Can't save unless put in the $z field? Do u still suggest validating it on the perl side or the onLeave thing should use the same javascript code?
Comment 10 Paul Poulain 2013-03-25 18:21:05 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > I don't think a check like this can be done with a value_builder plugin. If
> > > I am wrong, a value_builder plugin would definitely be better, but the
> > > plugins aren't -- so far as I understand -- used for validation but rather
> > > as helpers for setting fields.
> > 
> > You can put a hook on entering or leaving a field. If you put a hook on
> > leaving ISBN field, then it will be a validation. We use that for some
> > plugins already. The ... on the right are not necessary used to open a
> > popup, then can be used to do a check as well.
> 
> How would the workflow be in case of bad ISBN Paul? Can't save unless put in
> the $z field? Do u still suggest validating it on the perl side or the
> onLeave thing should use the same javascript code?

I would say "can't save an invalid ISBN".
So, if the subfield is empty, then it's OK.
That could be cool to move it to $z automatically ($z is also the invalid isbn for UNIMARC ;-) ), but could also be discussed, as it may trouble users.

I suggest validating on the onLeave, so at browser side, as your current patch do. (javascript is required to run staff interface, I'm fine with that)
Comment 11 Paul A 2013-03-27 14:36:39 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
[snip] 
> I would say "can't save an invalid ISBN".
> So, if the subfield is empty, then it's OK.
> That could be cool to move it to $z automatically ($z is also the invalid
> isbn for UNIMARC ;-) ), but could also be discussed, as it may trouble users.

Just talked to our cataloguers. They tell me that you *must* save invalid ISBNs (for whatever reason of 'invalid' - checksum, from a Z39.50 import, published in the book) but that it might be "cool to move it to $z automatically". They would see this immediately and decide (or not) to investigate. They suggest that they should have the option of recording it in the $a, particularly if it's a failed checksum, but printed in the book.

However, there would be a *presentation* problem: the ISBN field in many/most staff and OPAC pages comes up empty (only prints the $a, not the $z although the various search functions do appear to find the biblio with the $z.) We feel that (at least) the OPAC should display the ISBN, possibly with an "invalid" annotation although this might lead to end-user confusion. Savvy end-users might go to the 'MARC view', but both the 'Normal view' and the 'ISBD view' imply that the ISBN doesn't exist.
Comment 12 M. Tompsett 2014-08-29 13:47:29 UTC
Is anyone planning on following up on this bug?
Comment 13 M. Tompsett 2014-10-27 03:11:33 UTC
By the way, this still needs to happen. Though, there is a nice NormalizeISBN function in C4::Koha now. :)
Comment 14 M. Tompsett 2015-04-29 08:43:57 UTC
Having encountered horrific ISBN problems a while back, I get the feeling that warning people about bad ISBNs is the best we should do, because sometimes companies use invalid ISBNs.

Anyone going to follow up on this?
Comment 15 Tomás Cohen Arazi 2015-04-29 12:56:28 UTC
(In reply to M. Tompsett from comment #14)
> Having encountered horrific ISBN problems a while back, I get the feeling
> that warning people about bad ISBNs is the best we should do, because
> sometimes companies use invalid ISBNs.
> 
> Anyone going to follow up on this?

Mark, the unanswered question is what the workflow would be?
Comment 16 Marjorie Barry-Vila 2016-04-14 19:24:45 UTC
Is anyone planning on following up on this bug?

Regards,

Marjorie
Comment 17 Pascale Nalon 2016-04-20 12:35:33 UTC
We, at Mines ParisTech, aren't involved anymore in this bug, because we don't catalog anymore in Koha.
Comment 18 M. Tompsett 2016-04-20 13:14:30 UTC
(In reply to Marjorie Barry-Vila from comment #16)
> Is anyone planning on following up on this bug?

Until the work flow becomes clearly defined, something can't be written properly, as noted in comment #15. So, I don't think this bug is going to leave Failed QA purgatory, except to be snuffed out with a RESOLVED / WONT FIX, because fixing something without a clear definition doesn't work.