When performing a cataloging search we take the query and test to see if it is a valid isbn: If not, we perform a SQL search for the term as author or title If so, we pass it to BreedingSearch as an ISBN - we then normailize it to ISBN 10 - then perform aSQL search for ISBN In the ISBN case, if it cannot be converted to a 10 digit ISBN we nullify the term - the SQL then has no where statement and fails in plack-error.log: 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 near '' at line 5 at /kohadevbox/koha/cataloguing/addbooks.pl line 117
Created attachment 127458 [details] [review] Bug 29437: Search reservoir for term as title, author, or variations of ISBN The code in the script and the module attempt to determine whether a term is an isbn, or not. Rather than try to do this, we can simply search it on the three fields: isbn, title, author Additionally, we should search as any of the ISBN variations to broaden our matches Note: Curently only an ISBN 10 is stored in import biblios, so for an ISBN13 that doesn't convert the value will be blank - this is another bug To test: 1 - Perform a cataloging search for a valid ISBN 13 with no ISBN10 counterpart: 9798200834976 2 - 500 error 3 - Apply patch 4 - Repeat, no results 5 - Import some records 6 - Search by title/author/isbn 7 - Confirm searching works as expected
Needs unit tests for QA, but can be tested
Created attachment 127460 [details] [review] Bug 29437: Search reservoir for term as title, author, or variations of ISBN The code in the script and the module attempt to determine whether a term is an isbn, or not. Rather than try to do this, we can simply search it on the three fields: isbn, title, author Additionally, we should search as any of the ISBN variations to broaden our matches Note: Curently only an ISBN 10 is stored in import biblios, so for an ISBN13 that doesn't convert the value will be blank - this is another bug To test: 1 - Perform a cataloging search for a valid ISBN 13 with no ISBN10 counterpart: 9798200834976 2 - 500 error 3 - Apply patch 4 - Repeat, no results 5 - Import some records 6 - Search by title/author/isbn 7 - Confirm searching works as expected Signed-off-by: David Nind <david@davidnind.com>
(In reply to Nick Clemens from comment #2) > Needs unit tests for QA, but can be tested Changed status back to needs signoff - everything work as per the test plan.
Created attachment 127512 [details] [review] Bug 29437: Unit tests
Created attachment 127513 [details] [review] Bug 29437: Search reservoir for term as title, author, or variations of ISBN The code in the script and the module attempt to determine whether a term is an isbn, or not. Rather than try to do this, we can simply search it on the three fields: isbn, title, author Additionally, we should search as any of the ISBN variations to broaden our matches Note: Curently only an ISBN 10 is stored in import biblios, so for an ISBN13 that doesn't convert the value will be blank - this is another bug To test: 1 - Perform a cataloging search for a valid ISBN 13 with no ISBN10 counterpart: 9798200834976 2 - 500 error 3 - Apply patch 4 - Repeat, no results 5 - Import some records 6 - Search by title/author/isbn 7 - Confirm searching works as expected WNC amended to fix spelling Signed-off-by: David Nind <david@davidnind.com>
Created attachment 127516 [details] [review] Bug 29437: Unit tests Signed-off-by: David Nind <david@davidnind.com>
Created attachment 127517 [details] [review] Bug 29437: Search reservoir for term as title, author, or variations of ISBN The code in the script and the module attempt to determine whether a term is an isbn, or not. Rather than try to do this, we can simply search it on the three fields: isbn, title, author Additionally, we should search as any of the ISBN variations to broaden our matches Note: Curently only an ISBN 10 is stored in import biblios, so for an ISBN13 that doesn't convert the value will be blank - this is another bug To test: 1 - Perform a cataloging search for a valid ISBN 13 with no ISBN10 counterpart: 9798200834976 2 - 500 error 3 - Apply patch 4 - Repeat, no results 5 - Import some records 6 - Search by title/author/isbn 7 - Confirm searching works as expected WNC amended to fix spelling Signed-off-by: David Nind <david@davidnind.com>
QAing
Created attachment 127842 [details] [review] Bug 29437: Unit tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 127843 [details] [review] Bug 29437: Search reservoir for term as title, author, or variations of ISBN The code in the script and the module attempt to determine whether a term is an isbn, or not. Rather than try to do this, we can simply search it on the three fields: isbn, title, author Additionally, we should search as any of the ISBN variations to broaden our matches Note: Curently only an ISBN 10 is stored in import biblios, so for an ISBN13 that doesn't convert the value will be blank - this is another bug To test: 1 - Perform a cataloging search for a valid ISBN 13 with no ISBN10 counterpart: 9798200834976 2 - 500 error 3 - Apply patch 4 - Repeat, no results 5 - Import some records 6 - Search by title/author/isbn 7 - Confirm searching works as expected WNC amended to fix spelling Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> AMENDED: Useless call of ISBNs (plural) when you only pass one parameter.
Created attachment 127844 [details] [review] Bug 29437: (QA follow-up) Remove Business::ISBN from addbooks Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 127845 [details] [review] Bug 29437: (QA follow-up) Update TODO in Breeding.t We now have some tests for BreedingSearch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to David Nind from comment #8) > Note: Curently only an ISBN 10 is stored in import biblios, so for an ISBN13 > that doesn't convert > the value will be blank - this is another bug Did you open a new report ?
Looks good enough to me. A funny side effect is that we find reservoir books via the ISBN variations 10/13/hypens/no hyphens. We wont find them in the catalog. But if we search for a partial isbn 545 (as being part of eg 0-545-01022-5), we will not find anything in the reservoir, but Zebra finds it in the catalog.
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.05
Depends on Bug 29319 not in 20.11.x
*** Bug 17979 has been marked as a duplicate of this bug. ***
(In reply to Fridolin Somers from comment #18) > Depends on Bug 29319 not in 20.11.x Actually a pity that we didnt backport something. Quite easy to generate this in the logs: [2021/12/08 15:49:20] [WARN] 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 near '' 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 /usr/share/koha/C4/Breeding.pm line 100. [2021/12/08 15:49:20] [WARN] DBD::mysql::st fetchrow_hashref failed: fetch() without execute() [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 /usr/share/koha/C4/Breeding.pm line 101.
This is the most minimal maintenance solution I could come up with for 20.11: diff --git a/C4/Breeding.pm b/C4/Breeding.pm index 6b103b3..5b0d83a 100644 --- a/C4/Breeding.pm +++ b/C4/Breeding.pm @@ -96,6 +96,7 @@ sub BreedingSearch { $query .= "isbn like ?"; push(@bind,"$isbn%"); } + return 0 unless @bind; $sth = $dbh->prepare($query); $sth->execute(@bind); while (my $data = $sth->fetchrow_hashref) { Since Koha returns a 200, it only gives you a few log lines less.
Hi, I'll try to backport this for 20.11. Thanks Marcel for digging into this, how should this change be used? How can I use it to solve the conflicts due to bug 29319 missing? Or maybe it's a whole alternative patch? Another way: does bug 29319 seems like a risky backport to you? The odd thing is that I can catalogue search stuff that begins with digits without getting errors. In other words, I don't reproduce the issue.
(In reply to Victor Grousset/tuxayo from comment #22) > Hi, I'll try to backport this for 20.11. > > Thanks Marcel for digging into this, how should this change be used? How can > I use it to solve the conflicts due to bug 29319 missing? > Or maybe it's a whole alternative patch? > > Another way: does bug 29319 seems like a risky backport to you? The odd > thing is that I can catalogue search stuff that begins with digits without > getting errors. In other words, I don't reproduce the issue. 29319 seems quite innocent to me
Backported: Pushed to 20.11.x branch for 20.11.14