https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35729 --- Comment #39 from Aleisha Amohia <aleisha@catalyst.net.nz> --- (In reply to David Cook from comment #34)
After reviewing the code in C4::Koha and Business::ISBN, I think the patch is good but I think we need to update the unit tests in t/db_dependent/Koha.t for this one.
Looks like GetNormalizedISBN, which uses _isbn_cleanup, is used in quite a few places, so might be necessary to check out some other unit tests too.
This actually looks like it'll have fairly widespread repercussions. In theory, it should be a good move across the board, but... wise to check it out more thoroughly I think.
--
To avoid unintended consequences, it might not be a bad idea to scope this change more narrowly. Instead of using "normalized_isbn" in places like opac-detail.tt, it could be a good idea to use something like "isbn_for_coverimage".
Sometimes in Koha we end up making what looks like a sensible change but we don't take into account the wide-reaching effects. Again, I can't see the harm... but there's a lot of places that use GetNormalizedISBN so unless we check them all right now... I think maybe best to scope this one more narrowly.
Definitely take your point. I've amended the patch so that it only normalizes to ISBN-13 format if the ISBN begins with 979, in which case it has absolutely no ISBN-10 version. This should prevent unintended consequences for ^978 or other ISBNs which were already being normalized to ISBN-10 format (even if they could be ISBN-13), and solve the problem for our cover images. I've also written tests to confirm GetNormalizedISBN returns what we expect. Most of the logic is coming from Business::ISBN and therefore verified (probably), we just need to decide when to use it, and hopefully my solution is more targeted. -- You are receiving this mail because: You are watching all bug changes.