To reproduce, on k-t-d: 1) Run bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) 2) Create a FreeForm ILL request and input 23529000035676 in the cardnumber, that's "Henry Acevedo". 3) Go back to the ILL list table, input "henry" in the bottom "Patron" filter input and press the "Search" button, notice it fetches the request correctly. 4) Now do the the same search but with "henry acevedo", notice the request is not retrieved. The same issue also happens in the "keyword" filter input. Info: patron-search.inc is already doing what we need, but for patrons left side filters, it's handling all the patterns present in the #search_patron_filter input and adding each of those to the query, we need something similar here.
Created attachment 152701 [details] [review] Bug 34058: WIP Patron search is now considering all inputs using new buildSearchPatronQuery Still missing keyword input
Created attachment 152709 [details] [review] Bug 34058: Add table_prefix argument to buildPatronSearchQuery
Created attachment 152710 [details] [review] Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters
To test: Do the instructions on bug description, verify the issue occurs, apply patch, issue should not occur anymore.
Created attachment 152867 [details] [review] Bug 34058: Add table_prefix argument to buildPatronSearchQuery
Created attachment 152868 [details] [review] Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters
Created attachment 153391 [details] [review] Bug 34058: Add table_prefix argument to buildPatronSearchQuery Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 153392 [details] [review] Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This is working great.. I forgot Pedro had written it as I was about to come and write the same. Nice work, signing off to bump it into the QA queue :)
Really sad that things dont go faster..
Created attachment 156375 [details] [review] Bug 34058: Add table_prefix argument to buildPatronSearchQuery Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 156376 [details] [review] Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Thanks for looking, Marcel.
Happy to QA, but please write nicer commit messages (description, test plan etc.)
Created attachment 156732 [details] [review] Bug 34058: Preparation: buildPatronSearchQuery Add table_prefix argument to buildPatronSearchQuery. This allows the buildPatronSearchQuery function to also perform the search on a related table, instead of the default patron table. This is relevant because the query will be performed on the ILL API endpoint, so the table being searched will have to be prefixed with 'patron.', instead of the default 'me.' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 156733 [details] [review] Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters This will now use the centered logic from buildSearchPatronQuery, not only fixing the original issue of not being considering all the terms in the search input, but also reusing already established code. Test plan, k-t-d: 1) Install FreeForm, enable ILLModule sys pref, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) 2) Create a FreeForm ILL request and input 23529000035676 in the cardnumber, that's "Henry Acevedo". 3) Go back to the ILL list table, input "henry" in the bottom "Patron" filter input and press the "Search" button, notice it fetches the request correctly. 4) Now do the the same search but with "henry acevedo", notice the request is not retrieved. 5) Apply patch. Repeat. Do the same for the "patron" input field and the "keyword" input field. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Updated commit messages.
I think this is in a somewhat grey area for being a bug fix :)
Created attachment 158396 [details] [review] Bug 34058: Preparation: buildPatronSearchQuery Add table_prefix argument to buildPatronSearchQuery. This allows the buildPatronSearchQuery function to also perform the search on a related table, instead of the default patron table. This is relevant because the query will be performed on the ILL API endpoint, so the table being searched will have to be prefixed with 'patron.', instead of the default 'me.' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158397 [details] [review] Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters This will now use the centered logic from buildSearchPatronQuery, not only fixing the original issue of not being considering all the terms in the search input, but also reusing already established code. Test plan, k-t-d: 1) Install FreeForm, enable ILLModule sys pref, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) 2) Create a FreeForm ILL request and input 23529000035676 in the cardnumber, that's "Henry Acevedo". 3) Go back to the ILL list table, input "henry" in the bottom "Patron" filter input and press the "Search" button, notice it fetches the request correctly. 4) Now do the the same search but with "henry acevedo", notice the request is not retrieved. 5) Apply patch. Repeat. Do the same for the "patron" input field and the "keyword" input field. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158743 [details] [review] Bug 34058: Preparation: buildPatronSearchQuery Add table_prefix argument to buildPatronSearchQuery. This allows the buildPatronSearchQuery function to also perform the search on a related table, instead of the default patron table. This is relevant because the query will be performed on the ILL API endpoint, so the table being searched will have to be prefixed with 'patron.', instead of the default 'me.' Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158744 [details] [review] Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters This will now use the centered logic from buildSearchPatronQuery, not only fixing the original issue of not being considering all the terms in the search input, but also reusing already established code. Test plan, k-t-d: 1) Install FreeForm, enable ILLModule sys pref, run: bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) 2) Create a FreeForm ILL request and input 23529000035676 in the cardnumber, that's "Henry Acevedo". 3) Go back to the ILL list table, input "henry" in the bottom "Patron" filter input and press the "Search" button, notice it fetches the request correctly. 4) Now do the the same search but with "henry acevedo", notice the request is not retrieved. 5) Apply patch. Repeat. Do the same for the "patron" input field and the "keyword" input field. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Pushed to master for 23.11. Nice work everyone, thanks!
Missing tests, will be broken at some point...
(In reply to Jonathan Druart from comment #24) > Missing tests, will be broken at some point... True, but so is bug 35303 for example. A selenium test that grabs all these use cases is the best here, correct?
(In reply to Pedro Amorim from comment #25) > (In reply to Jonathan Druart from comment #24) > > Missing tests, will be broken at some point... > > True, but so is bug 35303 for example. A selenium test that grabs all these > use cases is the best here, correct? 35303 is not modifying datatables.js. I think all datatables.js changes should be covered by selenium tests.
(In reply to Jonathan Druart from comment #26) > (In reply to Pedro Amorim from comment #25) > > (In reply to Jonathan Druart from comment #24) > > > Missing tests, will be broken at some point... > > > > True, but so is bug 35303 for example. A selenium test that grabs all these > > use cases is the best here, correct? > > 35303 is not modifying datatables.js. I think all datatables.js changes > should be covered by selenium tests. I agree, but I thought we were talking about patron search, this bug and bug 35303 are about that, not datatables.
Enhancement not pushed to 23.05.x