Bug 29980 - Validate ISBN when cataloguing bibliographic records
Summary: Validate ISBN when cataloguing bibliographic records
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-01-31 03:27 UTC by Aleisha Amohia
Modified: 2022-05-04 11:34 UTC (History)
3 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement adds a new system preference ValidateISBN. When enabled, Koha will apply a valid checksum algorithm to ISBN fields (020$a for MARC21 and 010$a for UNIMARC) to validate the ISBN when cataloguing bibliographic records. The user will be prompted to fix their invalid ISBN, or have the option to bypass the ISBN check and save anyway. This will work for both 10-digit and 13-digit ISBNs. This patch also cleans up some bad indentation in cataloguing/addbiblio.pl.
Version(s) released in:


Attachments
Bug 29980: Add ValidateISBN system preference (3.35 KB, patch)
2022-01-31 04:20 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 29980: Validate ISBNs when saving biblio records (10.52 KB, patch)
2022-01-31 04:20 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 29980: Add ValidateISBN system preference (3.31 KB, patch)
2022-04-30 04:09 UTC, David Nind
Details | Diff | Splinter Review
Bug 29980: Validate ISBNs when saving biblio records (10.56 KB, patch)
2022-04-30 04:09 UTC, David Nind
Details | Diff | Splinter Review
Screenshot - placement of error messages when other errors (48.48 KB, image/png)
2022-04-30 04:32 UTC, David Nind
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2022-01-31 03:27:29 UTC
https://metacpan.org/pod/Business::ISBN

Business::ISBN has a valid_isbn_checksum method that implements an ISBN check digit algorithm for both 10-digit and 13-digit ISBNs. Koha could adopt this to validate ISBNs when cataloguing biblios.
Comment 1 Aleisha Amohia 2022-01-31 04:20:02 UTC
Created attachment 129984 [details] [review]
Bug 29980: Add ValidateISBN system preference
Comment 2 Aleisha Amohia 2022-01-31 04:20:06 UTC
Created attachment 129985 [details] [review]
Bug 29980: Validate ISBNs when saving biblio records

This enhancement adds a new system preference ValidateISBN. When
enabled, Koha will apply a valid checksum algorithm to ISBN fields
(020$a for MARC21 and 010$a for UNIMARC) to validate the ISBN when
cataloguing bibliographic records. The user will be prompted to fix
their invalid ISBN, or have the option to bypass the ISBN check and
save anyway. This will work for both 10-digit and 13-digit ISBNs.

This patch also cleans up some bad indentation in
cataloguing/addbiblio.pl.

To test:
1) Apply patch, update database.
2) Go to Koha administration -> global system preferences -> cataloguing
tab -> Record structure heading. Note the new ValidateISBN system
preference. It should be disabled by default. Leave it disabled.
3) Set marcflavour system preference to MARC21.
4) In a new tab, go to add a new biblio record, or edit an existing one.
5) Find the 020$a field and input an incorrect ISBN. Save the record and
confirm the ISBN is not validated and does not block saving the record.
6) Go back to your system preferences tab. Set the ValidateISBN tab to
"Validate" to enable it.
7) Go back to your cataloguing tab. Edit the record. Save the record
without making any changes.
8) Confirm an Invalid ISBN error pops up, blocking you from saving.
Click Continue to bypass the ISBN check and save the record with the
invalid ISBN.
9) Edit the record again. Save the record without making any changes.
10) When the Invalid ISBN error pops up, this time fix the invalid ISBN,
then save the record again. Confirm you are able to save the record.
11) Go back to the system preferences tab and change the marcflavour
system preference to UNIMARC. Repeat the above steps, but this time use
the 010$a field instead of 020$a for ISBN.

Sponsored-by: Education Services Australia SCIS
Comment 3 Shi Yao Wang 2022-03-10 21:47:15 UTC
Everything works until I switch marcflavour from MARC21 to UNIMARC. Error 500 appears when I search the catalog to continue the steps. Did the switch on origin/master and the same happens.
Comment 4 Katrin Fischer 2022-03-10 21:59:36 UTC
If you system hasn't been set up for UNIMARC from the start, it#s to be expected that the search does not work. What will probably work: Keep your record open in cataloguing and refresh the page after switching the marcflavor. This is just a workaround so you can test this on a MARC21 system, but it should be ok.
Comment 5 Aleisha Amohia 2022-03-10 22:03:30 UTC
(In reply to Katrin Fischer from comment #4)
> If you system hasn't been set up for UNIMARC from the start, it#s to be
> expected that the search does not work. What will probably work: Keep your
> record open in cataloguing and refresh the page after switching the
> marcflavor. This is just a workaround so you can test this on a MARC21
> system, but it should be ok.

Thanks for this suggestion Katrin!
Comment 6 Shi Yao Wang 2022-03-10 22:14:04 UTC
I kept one tab with a record open and another tab with system preferences open. Then I switched marcflavour to UNIMARC, went to the other tab, clicked edit and then error 500 appears once again.
Comment 7 Aleisha Amohia 2022-03-10 22:15:37 UTC
Maybe try clicking Edit and then changing the marcflavour, so you're refreshing the page already in the editor?

If that still doesn't work I'll take another look.
Comment 8 Shi Yao Wang 2022-03-10 22:24:24 UTC
I tried it and modified 010$a field but when I clicked save the same error 500 pops up. Maybe my system HAS to support UNIMARC for this to work but I don't really know. Sorry I couldn't test it.
Comment 9 Aleisha Amohia 2022-03-10 22:25:45 UTC
No worries I'll take a look.
Comment 10 Aleisha Amohia 2022-03-10 23:07:14 UTC
Looks like that error occurs because you don't have any UNIMARC records or frameworks in your Koha system.

When testing for UNIMARC, try to import a UNIMARC record and test with that!
Comment 11 David Nind 2022-04-30 04:09:35 UTC
Created attachment 134432 [details] [review]
Bug 29980: Add ValidateISBN system preference

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2022-04-30 04:09:40 UTC
Created attachment 134433 [details] [review]
Bug 29980: Validate ISBNs when saving biblio records

This enhancement adds a new system preference ValidateISBN. When
enabled, Koha will apply a valid checksum algorithm to ISBN fields
(020$a for MARC21 and 010$a for UNIMARC) to validate the ISBN when
cataloguing bibliographic records. The user will be prompted to fix
their invalid ISBN, or have the option to bypass the ISBN check and
save anyway. This will work for both 10-digit and 13-digit ISBNs.

This patch also cleans up some bad indentation in
cataloguing/addbiblio.pl.

To test:
1) Apply patch, update database.
2) Go to Koha administration -> global system preferences -> cataloguing
tab -> Record structure heading. Note the new ValidateISBN system
preference. It should be disabled by default. Leave it disabled.
3) Set marcflavour system preference to MARC21.
4) In a new tab, go to add a new biblio record, or edit an existing one.
5) Find the 020$a field and input an incorrect ISBN. Save the record and
confirm the ISBN is not validated and does not block saving the record.
6) Go back to your system preferences tab. Set the ValidateISBN tab to
"Validate" to enable it.
7) Go back to your cataloguing tab. Edit the record. Save the record
without making any changes.
8) Confirm an Invalid ISBN error pops up, blocking you from saving.
Click Continue to bypass the ISBN check and save the record with the
invalid ISBN.
9) Edit the record again. Save the record without making any changes.
10) When the Invalid ISBN error pops up, this time fix the invalid ISBN,
then save the record again. Confirm you are able to save the record.
11) Go back to the system preferences tab and change the marcflavour
system preference to UNIMARC. Repeat the above steps, but this time use
the 010$a field instead of 020$a for ISBN.

Sponsored-by: Education Services Australia SCIS

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2022-04-30 04:32:21 UTC
Created attachment 134434 [details]
Screenshot - placement of error messages when other errors

Testing notes (using koha-testing-docker):

- I have tested in both MARC21 and UNIMARC environments.

- When using koha-testing-docker you can't just change the MARC flavour. Before starting it up you need to change KOHA_MARC_FLAVOUR=marc21 in your .env file to KOHA_MARC_FLAVOUR=unimarc ; to change back to a MARC21 environment shut everything down (CTRL-C and then kd), and change it back to marc21. It is also possible to get a UNIMARC installation using the sandboxes - change the MARC flavour when you create the sandbox (see https://wiki.koha-community.org/wiki/Sandboxes). Basically, I found that I had to test in a MARC21 environment, then set up fresh in a UNIMARC one.

- One thing I noted is that if there are other errors, they display ABOVE the title - see the screenshot. It may pay to check with Owen about this. To me the errors should be below the title, like happens with the invalid ISBN error message. So I don't think that is an issue for this bug.

- The other thing I noted was that if a record is saved with an invalid ISBN and you then 1) edit it again 2) introduce other errors (such as deleting the 942$c Koha item type) 3) save, the invalid ISBN error doesn't display until after the other errors are resolved.
Comment 14 Marcel de Rooy 2022-05-03 12:18:15 UTC
QA:
Please remove indentation corrections out of your regular patches. They obscure the review. Put them separately (always). Thanks :)

Why dont you use Koha::NormalizeISBN and friends ?

FQA
Comment 15 Marcel de Rooy 2022-05-03 12:21:16 UTC
Just another thought, but you may leave it for another report (since it may be adjusting your scope):
What about checking if this ISBN is already used in another bib record?
Comment 16 Katrin Fischer 2022-05-03 16:03:32 UTC
(In reply to Marcel de Rooy from comment #15)
> Just another thought, but you may leave it for another report (since it may
> be adjusting your scope):
> What about checking if this ISBN is already used in another bib record?

I think it would not necessarily be a duplicate, if you treat different editions. I think mixing in duplicate checking would be out of scope here.

We already use Business::ISBN in other places - it seems ok to me. Where would you use NormalizeIBSN?
Comment 17 Marcel de Rooy 2022-05-04 11:14:41 UTC
(In reply to Katrin Fischer from comment #16)
> (In reply to Marcel de Rooy from comment #15)
> > Just another thought, but you may leave it for another report (since it may
> > be adjusting your scope):
> > What about checking if this ISBN is already used in another bib record?
> 
> I think it would not necessarily be a duplicate, if you treat different
> editions. I think mixing in duplicate checking would be out of scope here.

Fine
Comment 18 Marcel de Rooy 2022-05-04 11:22:46 UTC
Found another thing too:

Look at this piece of code:
my $r = MARC::Record->new;
my $f1 = MARC::Field->new('020','','', a => 'isbn1' );
my $f2 = MARC::Field->new('020','','', a => 'isbn2' );
$r->append_fields( $f1, $f2 );
my $scal = $r->subfield( '020', 'a' );
my @list = $r->subfield( '020', 'a' );
print Dumper( $scal, '-', @list );

It prints:
$VAR1 = 'isbn1';
$VAR2 = '-';
$VAR3 = 'isbn1';

Which means that the following code does not work:
+    my @input_isbns = ( C4::Context->preference('marcflavour') eq "UNIMARC" ) ? $record->subfield('010','a') : $record->subfield('020','a');

The @input_isbns will just contain the first ISBN only.
Comment 19 Marcel de Rooy 2022-05-04 11:30:58 UTC
The previous comment illustrates why we need code that can be tested easily. We do not achieve with putting all code in scripts but by using modules.

(In reply to Katrin Fischer from comment #16)
> We already use Business::ISBN in other places - it seems ok to me. Where
> would you use NormalizeIBSN?

The intention of this remark is: let's not call Business::ISBN everywhere in scripts. Use a module where we already have that functionality.
The above remark serves to illustrate my point.
Comment 20 Marcel de Rooy 2022-05-04 11:34:34 UTC
Another thing while revisiting this bug:
We are building tabs again, passing an invalid_isbn flag, etc. It would be better imo to put this code in a (default) cataloguing plugin with a very small ajax call (or just javascript, but then we cannot use the perl module).

Earlier validation, not just at time of saving biblio record.