https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17979 --- Comment #2 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #1)

I guess the regex should be m|^[\d-]*$|; Instead of just /\d/

That doesn't account for ISBN-10 where the check digit can be X or x: https://en.wikipedia.org/wiki/Check_digit#ISBN_10 We could use Business::ISBN to check if it's an ISBN and then if it's not, we could treat it as a title. (We could even do ISBN search only for valid ISBNs and do title searches for everything else.) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17979 --- Comment #3 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 95673 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=95673&action=edit Bug 17979: Fatal error when searching from addbooks.pl due to ISBN flaw This patch adds true ISBN detection to cataloging searches so that ISBNs can be correctly identified and passed to queries of the reservoir. To test, apply the patch and go to Cataloging. - In the "Cataloging search" form at the top of the page, test various searches to confirm that results are returned in both the catalog and the reservoir: - Any title search - A search for a title which begins with a numeral, e.g. "1st to die" - A search by 10-digit ISBN - A search by 13-digit ISBN -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17979 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de Status|Needs Signoff |Failed QA --- Comment #4 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Owen, why not search the ISBN as is? Or in different formats according to our ISBN variations prefs? I am not sure about the line using the as_isbn10. + if( $isbn = Business::ISBN->new( $query ) ) { + $isbn = $isbn->as_isbn10->as_string; + $isbn =~ s/-//g; As I understand it, if the ISBN is a new one (13), this will return undef. as_isbn10 Returns a new ISBN object. If the object is already ISBN-10, this method clones it. If it is an ISBN-13 with the prefix 978, it returns the ISBN-10 equivalent. For all other cases it returns undef. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17979 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #95673|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17979 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |NEW -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17979 --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 119373 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119373&action=edit Bug 17979: Fix cataloguing search if ISBN 13 is passed This patch fixes the following error: C4::Breeding::BreedingSearch(): DBI Exception: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use ne ar '' at line 5 [for Statement "SELECT import_record_id, file_name, isbn, title, author FROM import_biblios JOIN import_records USING (import_record_id) JOIN import_batches USING (import_batch_id) WHERE "] at /kohadevbox/koha/cataloguing/addbooks.pl line 126 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 It happens when our GetNormalizedISBN sub does not return an ISBN, the SQL query generated by BreedingSearch is invalid Test plan: Try a cataloguing search with "400638133393x" -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17979 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17979 --- Comment #10 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Would be good if the function didn't error on invalid isbn or title at all, the reproducer from bug 27008 still gives the error with this patch applied:

perl -e 'use C4::Context; use C4::Breeding; C4::Breeding::BreedingSearch();'

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17979 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com Status|Signed Off |Failed QA --- Comment #12 from Nick Clemens <nick@bywatersolutions.com> --- This certainly improves things, however, the initial problem reported still exists - a search for '1st to die' searches the isbn field -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17979 --- Comment #14 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- It would probably make sense to split this to two functions: one for title search and another for isbn? Makes also unit tests easier, less cases to iterate. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17979 --- Comment #16 from David Cook <dcook@prosentient.com.au> --- (In reply to Joonas Kylmälä from comment #14)

It would probably make sense to split this to two functions: one for title search and another for isbn? Makes also unit tests easier, less cases to iterate.

+1 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17979 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |DUPLICATE Status|Failed QA |RESOLVED --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- I believe the patches on bug 29437 resolve the issue here (if a slightly different implementation) *** This bug has been marked as a duplicate of bug 29437 *** -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17979 --- Comment #19 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #18)

(In reply to Nick Clemens from comment #17)

...

I believe the patches on bug 29437 resolve the issue here (if a slightly different implementation)

*** This bug has been marked as a duplicate of bug 29437 ***

Not the second patch.

Doesn't it? The second patch here pushed the conditions to an array depending on whether we think we have an ISBN or not Bug 29437 removes the attempt to determine which kind of query we have and instead uses 'OR' and builds the same condition each time. As the overlap between title/author and ISBN is very small I think one condition that searches all the fields resolves the issue -- You are receiving this mail because: You are watching all bug changes.