Bug 34058 - ILL - Left filters not considering all terms in input
Summary: ILL - Left filters not considering all terms in input
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Pedro Amorim
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 22440 34092
Blocks:
  Show dependency treegraph
 
Reported: 2023-06-19 11:19 UTC by Pedro Amorim
Modified: 2023-11-29 10:56 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00


Attachments
Bug 34058: WIP (6.17 KB, patch)
2023-06-26 16:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34058: Add table_prefix argument to buildPatronSearchQuery (4.74 KB, patch)
2023-06-27 09:09 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters (3.30 KB, patch)
2023-06-27 09:09 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34058: Add table_prefix argument to buildPatronSearchQuery (4.76 KB, patch)
2023-06-29 13:02 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters (3.30 KB, patch)
2023-06-29 13:02 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34058: Add table_prefix argument to buildPatronSearchQuery (4.83 KB, patch)
2023-07-13 08:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters (3.37 KB, patch)
2023-07-13 08:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34058: Add table_prefix argument to buildPatronSearchQuery (4.33 KB, patch)
2023-09-29 10:46 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters (3.37 KB, patch)
2023-09-29 10:46 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34058: Preparation: buildPatronSearchQuery (4.67 KB, patch)
2023-10-09 11:22 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters (4.16 KB, patch)
2023-10-09 11:22 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34058: Preparation: buildPatronSearchQuery (4.73 KB, patch)
2023-11-04 18:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters (4.22 KB, patch)
2023-11-04 18:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34058: Preparation: buildPatronSearchQuery (4.37 KB, patch)
2023-11-09 15:12 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters (4.22 KB, patch)
2023-11-09 15:12 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-06-19 11:19:30 UTC
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.
Comment 1 Pedro Amorim 2023-06-26 16:49:58 UTC
Created attachment 152701 [details] [review]
Bug 34058: WIP

Patron search is now considering all inputs using new buildSearchPatronQuery
Still missing keyword input
Comment 2 Pedro Amorim 2023-06-27 09:09:56 UTC
Created attachment 152709 [details] [review]
Bug 34058: Add table_prefix argument to buildPatronSearchQuery
Comment 3 Pedro Amorim 2023-06-27 09:09:59 UTC
Created attachment 152710 [details] [review]
Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters
Comment 4 Pedro Amorim 2023-06-27 09:12:25 UTC
To test: Do the instructions on bug description, verify the issue occurs, apply patch, issue should not occur anymore.
Comment 5 Pedro Amorim 2023-06-29 13:02:00 UTC
Created attachment 152867 [details] [review]
Bug 34058: Add table_prefix argument to buildPatronSearchQuery
Comment 6 Pedro Amorim 2023-06-29 13:02:03 UTC
Created attachment 152868 [details] [review]
Bug 34058: Use buildSearchPatronQuery when searching for patrons on left side filters
Comment 7 Martin Renvoize 2023-07-13 08:34:05 UTC
Created attachment 153391 [details] [review]
Bug 34058: Add table_prefix argument to buildPatronSearchQuery

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2023-07-13 08:34:08 UTC
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>
Comment 9 Martin Renvoize 2023-07-13 08:34:51 UTC
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 :)
Comment 10 Marcel de Rooy 2023-09-29 09:11:11 UTC
Really sad that things dont go faster..
Comment 11 Pedro Amorim 2023-09-29 10:46:15 UTC
Created attachment 156375 [details] [review]
Bug 34058: Add table_prefix argument to buildPatronSearchQuery

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Pedro Amorim 2023-09-29 10:46:18 UTC
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>
Comment 13 Pedro Amorim 2023-09-29 10:46:36 UTC
Thanks for looking, Marcel.
Comment 14 Katrin Fischer 2023-09-29 14:46:07 UTC
Happy to QA, but please write nicer commit messages (description, test plan etc.)
Comment 15 Pedro Amorim 2023-10-09 11:22:49 UTC
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>
Comment 16 Pedro Amorim 2023-10-09 11:22:52 UTC
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>
Comment 17 Pedro Amorim 2023-10-09 11:23:27 UTC
Updated commit messages.
Comment 18 Katrin Fischer 2023-11-04 18:17:39 UTC
I think this is in a somewhat grey area for being a bug fix :)
Comment 19 Katrin Fischer 2023-11-04 18:20:44 UTC
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>
Comment 20 Katrin Fischer 2023-11-04 18:20:47 UTC
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>
Comment 21 Pedro Amorim 2023-11-09 15:12:27 UTC
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>
Comment 22 Pedro Amorim 2023-11-09 15:12:30 UTC
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>
Comment 23 Tomás Cohen Arazi 2023-11-09 17:43:57 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 24 Jonathan Druart 2023-11-09 17:49:40 UTC
Missing tests, will be broken at some point...
Comment 25 Pedro Amorim 2023-11-09 18:01:30 UTC
(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?
Comment 26 Jonathan Druart 2023-11-10 07:58:07 UTC
(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.
Comment 27 Pedro Amorim 2023-11-10 09:17:10 UTC
(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.
Comment 28 Fridolin Somers 2023-11-11 07:19:06 UTC
Enhancement not pushed to 23.05.x