Bug 30645 - Generated DBIC query incorrect for API searches across joined extended attributes when several terms are passed
Summary: Generated DBIC query incorrect for API searches across joined extended attrib...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Pedro Amorim
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 30639
Blocks: 32755 35287
  Show dependency treegraph
 
Reported: 2022-04-28 13:13 UTC by Jonathan Druart
Modified: 2024-05-22 12:29 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes patron searching where there are multiple patron attributes - all patron attributes are now searched. Before this, if your search included values from multiple patron attributes, no results would be found. Example: if you have two patron attributes with the value 'abc' in one and '123' in another, searching for either one worked as expected, but no results would be found searching for 'abc 123'.
Version(s) released in:
24.05.00


Attachments
Bug 30645: Add tests (3.90 KB, patch)
2022-04-28 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30645: Add tests (3.62 KB, patch)
2023-08-30 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30645: Consider multiple instances of extended_attributes in query params (5.26 KB, patch)
2023-11-30 17:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 30645: Add tests (4.70 KB, patch)
2023-12-01 13:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30645: Consider multiple instances of extended_attributes in query params (9.21 KB, patch)
2023-12-07 14:43 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 30645: Consider multiple instances of extended_attributes in query params (9.28 KB, patch)
2023-12-07 14:52 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 30645: Add tests (4.75 KB, patch)
2023-12-08 18:23 UTC, David Nind
Details | Diff | Splinter Review
Bug 30645: Consider multiple instances of extended_attributes in query params (9.32 KB, patch)
2023-12-08 18:23 UTC, David Nind
Details | Diff | Splinter Review
Bug 30645: Add REST/Plugin/Query tests (3.84 KB, patch)
2023-12-21 16:07 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 30645: Add REST/Plugin/Query tests (3.88 KB, patch)
2023-12-21 20:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 30645: (QA follow-up): Tidy (26.66 KB, patch)
2024-01-26 09:36 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 30645: (QA follow-up): Tidy (3.53 KB, patch)
2024-01-26 13:33 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 30645: Add tests (5.52 KB, patch)
2024-04-17 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30645: Consider multiple instances of extended_attributes in query params (9.39 KB, patch)
2024-04-17 13:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30645: Add REST/Plugin/Query tests (3.95 KB, patch)
2024-04-17 13:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30645: Add tests (5.58 KB, patch)
2024-04-17 13:15 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-04-28 13:13:33 UTC

    
Comment 1 Jonathan Druart 2022-04-28 13:14:05 UTC
Created attachment 134253 [details] [review]
Bug 30645: Add tests

to highlight the problem
Comment 2 Pedro Amorim 2023-08-25 08:30:22 UTC
Jonathan is this still the case? i.e. is the patron search still not building the query correctly for extended attributes if several terms passed?

If so, the tests patch doesn't apply anymore.
Comment 3 Jonathan Druart 2023-08-30 11:15:01 UTC
Created attachment 154949 [details] [review]
Bug 30645: Add tests

to highlight the problem
Comment 4 Jonathan Druart 2023-08-30 11:15:13 UTC
Yes, this is still valid. Tests rebased.
Comment 5 Pedro Amorim 2023-08-31 12:14:54 UTC
Same issue happens when searching ILL requests but with illrequestattributes instead.
Comment 6 Pedro Amorim 2023-11-30 17:25:59 UTC
Created attachment 159414 [details] [review]
Bug 30645: Consider multiple instances of extended_attributes in query params

This is a POC but its fully functional afaik.

Im not 100% happy with the code as Im addressing the 3 possible related metadata tables individually.
Im also not sure if this is the best place to put this code, please advise.

Testing in the UI, this is fixing the patron search and the licenses additional fields search (requires bug 35287 applied)

ILL is still not working, but I think its because the query builder in the javascript is not breaking apart multiple search words, in other words, this fix should also fix ILL illrequestattributes once the javascript is looked at.

Joubu can you please rebase your test patch?
Comment 7 Martin Renvoize 2023-12-01 12:49:32 UTC
After some consideration, I think we should put this into the API plugin side.. so REST/Plugin/Query.pm

We already have the Mixin which does this for additional_fields with a filter_by_ method... though I'm not sure how well advertised/documented that is.. and I think it may well confuse object consumers later having this level of magic in the base class search method.
Comment 8 Jonathan Druart 2023-12-01 13:15:34 UTC
Created attachment 159450 [details] [review]
Bug 30645: Add tests

to highlight the problem
Comment 9 Jonathan Druart 2023-12-01 13:15:50 UTC
(In reply to Pedro Amorim from comment #6)
> Joubu can you please rebase your test patch?

Done.
Comment 10 Pedro Amorim 2023-12-07 14:43:49 UTC Comment hidden (obsolete)
Comment 11 Pedro Amorim 2023-12-07 14:52:17 UTC
Created attachment 159652 [details] [review]
Bug 30645: Consider multiple instances of extended_attributes in query params

This will rewrite a query like:

{
    "-and":[
        [
            {
                "extended_attributes.value":{
                    "like":"abc%"
                },
                "extended_attributes.code":[
                    [
                        "arbitrary_attr_code",
                        "another_attr_code"
                    ]
                ]
            }
        ],
        [
            {
                "extended_attributes.value":{
                    "like":"123%"
                },
                "extended_attributes.code":[
                    [
                        "arbitrary_attr_code",
                        "another_attr_code"
                    ]
                ]
            }
        ]
    ]
}

To:

{
    "-and":[
        [
            {
                "extended_attributes.value":{
                    "like":"abc%"
                },
                "extended_attributes.code":[
                    [
                        "arbitrary_attr_code",
                        "another_attr_code"
                    ]
                ]
            }
        ],
        [
            {
                "extended_attributes_2.value":{
                    "like":"123%"
                },
                "extended_attributes_2.code":[
                    [
                        "arbitrary_attr_code",
                        "another_attr_code"
                    ]
                ]
            }
        ]
    ]
}

And it'll also add the number of required 'join' attributes to the query
attributes to match it.

Test plan (with Joubu's test patch applied), run:
prove t/db_dependent/selenium/patrons_search.t

Or

Test plan, k-t-d:
1) Create 2 patron attributes, visit:
/cgi-bin/koha/admin/patron-attr-types.pl
2) Name the first attribute 'test1' and the second attribute 'test2'
3) On both, tick the 'Searchable' and 'Search by default' checkboxes
4) Edit a koha borrower, example visit:
/cgi-bin/koha/members/memberentry.pl?op=modify&destination=circ&borrowernumber=51
5) Put '123' on the test1 attribute, and 'abc' on the test2 attribute
6) Visit the main patrons page:
/cgi-bin/koha/members/members-home.pl
7) On the first left side filter input called 'Search', type '123' and search. Notice you get the patron result
8) Repeat step 7, but type 'abc' instead of '123'. Notice you get the patron result
9) Now do the same search but type '123 abc'. Notice you do not get the patron result.
10) Apply patch. Repeat test plan. Notice you get the patron result in step 9).
Comment 12 David Nind 2023-12-08 18:23:18 UTC
Created attachment 159675 [details] [review]
Bug 30645: Add tests

to highlight the problem

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2023-12-08 18:23:21 UTC
Created attachment 159676 [details] [review]
Bug 30645: Consider multiple instances of extended_attributes in query params

This will rewrite a query like:

{
    "-and":[
        [
            {
                "extended_attributes.value":{
                    "like":"abc%"
                },
                "extended_attributes.code":[
                    [
                        "arbitrary_attr_code",
                        "another_attr_code"
                    ]
                ]
            }
        ],
        [
            {
                "extended_attributes.value":{
                    "like":"123%"
                },
                "extended_attributes.code":[
                    [
                        "arbitrary_attr_code",
                        "another_attr_code"
                    ]
                ]
            }
        ]
    ]
}

To:

{
    "-and":[
        [
            {
                "extended_attributes.value":{
                    "like":"abc%"
                },
                "extended_attributes.code":[
                    [
                        "arbitrary_attr_code",
                        "another_attr_code"
                    ]
                ]
            }
        ],
        [
            {
                "extended_attributes_2.value":{
                    "like":"123%"
                },
                "extended_attributes_2.code":[
                    [
                        "arbitrary_attr_code",
                        "another_attr_code"
                    ]
                ]
            }
        ]
    ]
}

And it'll also add the number of required 'join' attributes to the query
attributes to match it.

Test plan (with Joubu's test patch applied), run:
prove t/db_dependent/selenium/patrons_search.t

Or

Test plan, k-t-d:
1) Create 2 patron attributes, visit:
/cgi-bin/koha/admin/patron-attr-types.pl
2) Name the first attribute 'test1' and the second attribute 'test2'
3) On both, tick the 'Searchable' and 'Search by default' checkboxes
4) Edit a koha borrower, example visit:
/cgi-bin/koha/members/memberentry.pl?op=modify&destination=circ&borrowernumber=51
5) Put '123' on the test1 attribute, and 'abc' on the test2 attribute
6) Visit the main patrons page:
/cgi-bin/koha/members/members-home.pl
7) On the first left side filter input called 'Search', type '123' and search. Notice you get the patron result
8) Repeat step 7, but type 'abc' instead of '123'. Notice you get the patron result
9) Now do the same search but type '123 abc'. Notice you do not get the patron result.
10) Apply patch. Repeat test plan. Notice you get the patron result in step 9).

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2023-12-08 18:33:05 UTC
Testing notes (using KTD):

1. To run the tests, start up KTD with selenium: ktd --selenium up

2. I ran the tests (these passed) and added patron attributes to test that patron search works as expected.
Comment 15 Tomás Cohen Arazi 2023-12-15 19:42:03 UTC
Hi, this is great but we need lower level tests, for the new mojo helper. There are several example tests. We can take a look together on monday if you feel like.
Comment 16 Pedro Amorim 2023-12-18 12:13:53 UTC
(In reply to Tomás Cohen Arazi from comment #15)
> Hi, this is great but we need lower level tests, for the new mojo helper.
> There are several example tests. We can take a look together on monday if
> you feel like.

Hi, yes please. Help is greatly appreciated, thank you!
Comment 17 Pedro Amorim 2023-12-21 16:07:33 UTC
Created attachment 160208 [details] [review]
Bug 30645: Add REST/Plugin/Query tests

prove t/Koha/REST/Plugin/Query.t
Comment 18 David Nind 2023-12-21 20:18:13 UTC
Created attachment 160216 [details] [review]
Bug 30645: Add REST/Plugin/Query tests

prove t/Koha/REST/Plugin/Query.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 Marcel de Rooy 2024-01-26 07:18:09 UTC
 WARN   t/db_dependent/selenium/patrons_search.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 91, now: 100)
Comment 20 Pedro Amorim 2024-01-26 09:36:04 UTC
Created attachment 161495 [details] [review]
Bug 30645: (QA follow-up): Tidy
Comment 21 Jonathan Druart 2024-01-26 12:48:42 UTC
Please do not tidy the whole file, bug 35329 is on its way to master and it's going to conflict.
Comment 22 Jonathan Druart 2024-01-26 12:57:59 UTC
I guess performances (see bug 33554) will be even worst with this change :-/
Comment 23 Pedro Amorim 2024-01-26 13:33:17 UTC
Created attachment 161528 [details] [review]
Bug 30645: (QA follow-up): Tidy
Comment 24 Pedro Amorim 2024-01-29 13:59:17 UTC
(In reply to Jonathan Druart from comment #22)
> I guess performances (see bug 33554) will be even worst with this change :-/

I've done some testing myself and cannot find evidence of this.

Clean k-t-d (before applying patches):
1) Visit patrons home:
http://localhost:8081/cgi-bin/koha/members/members-home.pl
2) Open network tools and click "Search"
3) Do this several times (like 10), take note of the average time it takes for the request to come back (for me, averaging around 185-195ms)
4) Run the following command (!!DEV KTD ONLY!!):
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/scripts/dev-data-pattributes.sh) 30000
5) If all goes well, the above command will add 30.000 patrons, 300 searchable borrower_attribute_types and 30.000 borrower_attributes. It may take a few minutes to finish, but required to test performance.
6) After 5) is done. Repeat 3) Notice it now takes a while longer (for me, its now averaging aroudn 225ms)
7) Apply patches from 30645, restart plack and flush_memcached
8) Notice if there's any difference, for me, I'm averaging about the same 225ms as before.
9) Do more testing, search for searchable fields specifically, having tried that myself I don't see any impact.
Comment 25 Martin Renvoize 2024-01-29 14:38:17 UTC
I don't believe this makes the query any slower, and this is a bugfix whilst the other is a performance enhancement.

I think this is important as is and if we want to introduce an entirely new route for 'lookup', then that should be judged and used on its own merit.
Comment 26 Pedro Amorim 2024-04-08 14:46:20 UTC
(In reply to Jonathan Druart from comment #9)
> (In reply to Pedro Amorim from comment #6)
> > Joubu can you please rebase your test patch?
> 
> Done.

Sorry Joubu, can you please rebase your test patch again?
Comment 27 Jonathan Druart 2024-04-17 09:18:32 UTC
Created attachment 164982 [details] [review]
Bug 30645: Add tests

to highlight the problem

Signed-off-by: David Nind <david@davidnind.com>
Comment 28 Jonathan Druart 2024-04-17 09:18:50 UTC
(In reply to Pedro Amorim from comment #26)
> (In reply to Jonathan Druart from comment #9)
> > (In reply to Pedro Amorim from comment #6)
> > > Joubu can you please rebase your test patch?
> > 
> > Done.
> 
> Sorry Joubu, can you please rebase your test patch again?

Done, it was painful and boring!
Comment 29 Martin Renvoize 2024-04-17 13:15:50 UTC
Created attachment 165009 [details] [review]
Bug 30645: Consider multiple instances of extended_attributes in query params

This will rewrite a query like:

{
    "-and":[
        [
            {
                "extended_attributes.value":{
                    "like":"abc%"
                },
                "extended_attributes.code":[
                    [
                        "arbitrary_attr_code",
                        "another_attr_code"
                    ]
                ]
            }
        ],
        [
            {
                "extended_attributes.value":{
                    "like":"123%"
                },
                "extended_attributes.code":[
                    [
                        "arbitrary_attr_code",
                        "another_attr_code"
                    ]
                ]
            }
        ]
    ]
}

To:

{
    "-and":[
        [
            {
                "extended_attributes.value":{
                    "like":"abc%"
                },
                "extended_attributes.code":[
                    [
                        "arbitrary_attr_code",
                        "another_attr_code"
                    ]
                ]
            }
        ],
        [
            {
                "extended_attributes_2.value":{
                    "like":"123%"
                },
                "extended_attributes_2.code":[
                    [
                        "arbitrary_attr_code",
                        "another_attr_code"
                    ]
                ]
            }
        ]
    ]
}

And it'll also add the number of required 'join' attributes to the query
attributes to match it.

Test plan (with Joubu's test patch applied), run:
prove t/db_dependent/selenium/patrons_search.t

Or

Test plan, k-t-d:
1) Create 2 patron attributes, visit:
/cgi-bin/koha/admin/patron-attr-types.pl
2) Name the first attribute 'test1' and the second attribute 'test2'
3) On both, tick the 'Searchable' and 'Search by default' checkboxes
4) Edit a koha borrower, example visit:
/cgi-bin/koha/members/memberentry.pl?op=modify&destination=circ&borrowernumber=51
5) Put '123' on the test1 attribute, and 'abc' on the test2 attribute
6) Visit the main patrons page:
/cgi-bin/koha/members/members-home.pl
7) On the first left side filter input called 'Search', type '123' and search. Notice you get the patron result
8) Repeat step 7, but type 'abc' instead of '123'. Notice you get the patron result
9) Now do the same search but type '123 abc'. Notice you do not get the patron result.
10) Apply patch. Repeat test plan. Notice you get the patron result in step 9).

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2024-04-17 13:15:53 UTC
Created attachment 165010 [details] [review]
Bug 30645: Add REST/Plugin/Query tests

prove t/Koha/REST/Plugin/Query.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2024-04-17 13:15:56 UTC
Created attachment 165011 [details] [review]
Bug 30645: Add tests

to highlight the problem

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2024-04-17 13:16:33 UTC
We've been using this fix in production for some time now.. it would be good to get a final QA round on it.
Comment 33 Martin Renvoize 2024-04-24 10:33:50 UTC
This has been around for too long and really must be part of the next release and preferably ported back too.

Setting to PQA.. it's had testing and feedback.
Comment 34 Katrin Fischer 2024-04-25 16:05:27 UTC
I had a little trouble here with 	

* Proving /kohadevbox/koha/t/db_dependent/selenium/patrons_search.t KO!

But this test is currently failing for me on master as well, right after a reset_all. 

Help appreciated!
Comment 35 Katrin Fischer 2024-04-26 06:05:55 UTC
I'll push this one as it doesn't seem to be the cause of the breaking test, but we should definitely check on the test.
Comment 36 Katrin Fischer 2024-04-26 06:53:31 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 37 Fridolin Somers 2024-05-22 12:29:03 UTC
Backport on 23.11.x fails on t/db_dependent/selenium/patrons_search.t
Please create patches for 23.11.x if possible.