Bug 906

Suppose we check ISBN (ISBN10 or ISBN13) is invalid. What should be the system behaviour?

(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)

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?

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.

(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.

(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.

(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?

(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)

(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.

Is anyone planning on following up on this bug?

By the way, this still needs to happen. Though, there is a nice NormalizeISBN function in C4::Koha now. :)

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?

(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?

Is anyone planning on following up on this bug?

Regards,

Marjorie

We, at Mines ParisTech, aren't involved anymore in this bug, because we don't catalog anymore in Koha.

(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.