The exception: [ERROR] GET /api/v1/acquisitions/orders: unhandled exception (DBIx::Class::Exception)<<DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'biblioitem.title' in 'where clause' at /usr/share/perl5/Data/Page.pm line 49 Because here is the experimental dump of what sub table_name_fixer replaces: in: 'q' => '{"-and":[{"basket.vendor_id":2},[[{"basket.name":{"like":"%f%"}}],[{"basket.basket_group.name":{"like":"%f%"}}],[{"me.order_id":{"like":"%f%"}}],[{"biblio.title":{"like":"%f%"}},{"biblio.author":{"like":"%f%"}},{"biblio.isbn":{"like":"%f%"}},{"biblio.publisher":{"like":"%f%"}},{"me.internal_note":{"like":"%f%"}},{"me.vendor_note":{"like":"%f%"}}],[{"me.replacement_price":{"like":"%f%"}}],[{"me.quantity":{"like":"%f%"}}],[{"me.ecost":{"like":"%f%"}}],[{"fund.name":{"like":"%f%"}}]]]}' }; out: 'q' => '{"-and":[{"basket.vendor_id":2},[[{"basket.name":{"like":"%f%"}}],[{"basket.basket_group.name":{"like":"%f%"}}],[{"me.order_id":{"like":"%f%"}}],[{"biblio.biblioitem.title":{"like":"%f%"}},{"biblio.biblioitem.author":{"like":"%f%"}},{"biblio.biblioitem.isbn":{"like":"%f%"}},{"biblio.biblioitem.publisher":{"like":"%f%"}},{"me.internal_note":{"like":"%f%"}},{"me.vendor_note":{"like":"%f%"}}],[{"me.replacement_price":{"like":"%f%"}}],[{"me.quantity":{"like":"%f%"}}],[{"me.ecost":{"like":"%f%"}}],[{"fund.name":{"like":"%f%"}}]]]}' }; That because this: $q =~ s|biblio\.|biblio\.biblioitem\.|g if $q =~ m/.*(isbn|ean|publisher).*/; should be with lookahead to exclude false matches: $q =~ s{biblio\.(?=isbn|ean|publisher)}{biblio\.biblioitem\.}g; Making a fix.
Created attachment 134612 [details] Bug 30677: Make s/// change only proper strings. To reproduce / test: 1. List form Test Koha all available acquisitions orders https://koha-kktest.lib.helsinki.fi/cgi-bin/koha/acqui/histsearch.pl with any (empty) search request to list them all, 2. open browser's JS console, to see Ajax errors (500), or run in parallel 'tail -F -n0 /var/log/koha/*/*-error.log', 3. start typing in Search field to sub-filter-out ajax-generated search results any existing word in orders below, 4. see that search dies with (in log: [ERROR] GET /api/v1/acquisitions/orders: unhandled exception (DBIx::Class::Exception)<<DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'biblioitem.title' in 'where clause' 5. apply the patch, 6. start typing in Search field to sub-filter-out ajax-generated search results: it should filter our what you want.
we need to add Tests, and some reproduction stuck: some settings, TBU...
There are some regression tests here: https://git.koha-community.org/Koha-community/Koha/src/branch/master/t/db_dependent/api/v1/acquisitions_orders.t#L115 you should extend them with your cases if you confirm them. Thanks!
Created attachment 134724 [details] [review] Bug 30677: Add tests
Created attachment 134900 [details] [review] Bug 30677: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 134901 [details] [review] Bug 30677: Use lookahead in regex for biblioitem replacement This patch takes Andrew's suggested fix using a lookahead regex to correct our biblio vs biblioitem table name replacements. Please use the preceeding unit test patch proposed by Jonathan to test. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
All looks good to me and both manual testing and the unit test appear happy. Signing of.. over to QA
Regarding comment 2 this is not ready. Andrew, any news on this?
I was stuck in the reproduction of this issue, but an only day ago I realized that I provided wrong place where this should be reproduced if team found (and I see it is) then it's all ok as for me,
I want to propose as QA-phase only this: That "if" is _probably_ not needed: ... if $q =~ m/.*(isbn|ean|publisher).*/; because this anyway matches only those places: $q =~ s{biblio\.(?=isbn|ean|publisher)}{biblio\.biblioitem\.}g; so having this: $q =~ s{biblio\.(?=isbn|ean|publisher)}{biblio\.biblioitem\.}g if $q =~ m/.*(isbn|ean|publisher).*/; is something like "double-check for the same thing", But this is more like 50/50 proposal to agree with you JD/Martin.
+ ->json_is( "/0/biblio/isbn", 'SOMETHING', 'Filtering by a biblioitem column works!' ) + ->json_is( "/0/biblio/title", $biblio->title, 'Filtering by a biblioitem and biblio columns works!' ); Descriptions refer tot biblioitem. I thought that was the problem in the first place.
Still looking
(In reply to Andrew Nugged from comment #10) > I want to propose as QA-phase only this: > > That "if" is _probably_ not needed: > > ... if $q =~ m/.*(isbn|ean|publisher).*/; > > because this anyway matches only those places: > > $q =~ s{biblio\.(?=isbn|ean|publisher)}{biblio\.biblioitem\.}g; > > so having this: > > $q =~ s{biblio\.(?=isbn|ean|publisher)}{biblio\.biblioitem\.}g > if $q =~ m/.*(isbn|ean|publisher).*/; > > is something like "double-check for the same thing", > But this is more like 50/50 proposal to agree with you JD/Martin. Yes that if is horrible for a purist. And note that matching .* is an insult already. We just wanted m/isbn|etc/ and that was not even very specific matching. Also note the separator switching here: Please use s/ a / b / and m/c/ for instance, but not do s| | | and m/ / It hurts my eyes :)
Please tidy both patches. FQA
(In reply to Marcel de Rooy from comment #13) > Yes that if is horrible for a purist. > And note that matching .* is an insult already. Language
(In reply to Tomás Cohen Arazi from comment #15) > (In reply to Marcel de Rooy from comment #13) > > Yes that if is horrible for a purist. > > And note that matching .* is an insult already. > > Language ?
This also affects table filtering in /cgi-bin/koha/acqui/parcel.pl?invoiceid=<number>.
Created attachment 136677 [details] [review] Bug 30677: Cleanup This patch cleans up the regular expression to remove the superflous double check and use standard delimiters
Will have a look right now ;)
Created attachment 136678 [details] [review] Bug 30677: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 136679 [details] [review] Bug 30677: Use lookahead in regex for biblioitem replacement This patch takes Andrew's suggested fix using a lookahead regex to correct our biblio vs biblioitem table name replacements. Please use the preceeding unit test patch proposed by Jonathan to test. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 136680 [details] [review] Bug 30677: Cleanup This patch cleans up the regular expression to remove the superflous double check and use standard delimiters Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 22.11. Nice work everyone, thanks!
Backported to 22.05.x for 22.05.03
pushed to 21.11.x for 21.11.11, thx!
Missing dependencies for 21.05.x, it shouldn't be affected, no backport. Nothing to document, marking resolved.
*** Bug 31193 has been marked as a duplicate of this bug. ***