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!