Bug 30645

Created attachment 134253 [details] [review]
Bug 30645: Add tests

to highlight the problem

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.

Created attachment 154949 [details] [review]
Bug 30645: Add tests

to highlight the problem

Yes, this is still valid. Tests rebased.

Same issue happens when searching ILL requests but with illrequestattributes instead.

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?

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.

Created attachment 159450 [details] [review]
Bug 30645: Add tests

to highlight the problem

(In reply to Pedro Amorim from comment #6)
> Joubu can you please rebase your test patch?

Done.

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).

Created attachment 159675 [details] [review]
Bug 30645: Add tests

to highlight the problem

Signed-off-by: David Nind <david@davidnind.com>

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>

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.

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.

(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!

Created attachment 160208 [details] [review]
Bug 30645: Add REST/Plugin/Query tests

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

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>

 WARN   t/db_dependent/selenium/patrons_search.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 91, now: 100)

Created attachment 161495 [details] [review]
Bug 30645: (QA follow-up): Tidy

Please do not tidy the whole file, bug 35329 is on its way to master and it's going to conflict.

I guess performances (see bug 33554) will be even worst with this change :-/

Created attachment 161528 [details] [review]
Bug 30645: (QA follow-up): Tidy

(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.

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.

(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?

Created attachment 164982 [details] [review]
Bug 30645: Add tests

to highlight the problem

Signed-off-by: David Nind <david@davidnind.com>

(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!

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>

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>

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>

We've been using this fix in production for some time now.. it would be good to get a final QA round on it.

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.

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!

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.

Pushed for 24.05!

Well done everyone, thank you!

Backport on 23.11.x fails on t/db_dependent/selenium/patrons_search.t
Please create patches for 23.11.x if possible.

I'm not sure if this is a problem with the ILL requests DT only, but I have:

> SELECT COUNT(*) FROM illrequests;
54
> SELECT COUNT(*) FROM illrequestattributes;
754

That'd be -roughly- around 13/14 different attributes per request.

Using the DT filter box for searching "fra" is generating this query:

```sql
SELECT COUNT( * ) FROM (SELECT `me`.`illrequest_id` FROM `illrequests` `me` LEFT JOIN `illrequestattributes` `extended_attributes` ON `extended_attributes`.`illrequest_id` = `me`.`illrequest_id` LEFT JOIN `illrequestattributes` `extended_attributes_2` ON `extended_attributes_2`.`illrequest_id` =
`me`.`illrequest_id` LEFT JOIN `illrequestattributes` `extended_attributes_3` ON `extended_attributes_3`.`illrequest_id` = `me`.`illrequest_id` LEFT JOIN `illrequestattributes` `extended_attributes_4` ON `extended_attributes_4`.`illrequest_id` = `me`.`illrequest_id` LEFT JOIN `illrequestattributes` `extended_attributes_5` ON `extended_attributes_5`.`illrequest_id` = `me`.`illrequest_id` LEFT JOIN `illrequestattributes` `extended_attributes_6` ON `extended_attributes_6`.`illrequest_id` = `me`.`illrequest_id` LEFT JOIN `illrequestattributes` `extended_attributes_7` ON `extended_attributes_7`.`illrequest_id` = `me`.`illrequest_id` LEFT JOIN `illrequestattributes` `extended_attributes_8` ON `extended_attributes_8`.`illrequest_id` = `me`.`illrequest_id` LEFT JOIN `illbatches` `ill_batch` ON `ill_batch`.`ill_batch_id` = `me`.`batch_id`  JOIN `branches` `library` ON `library`.`branchcode` = `me`.`branchcode` LEFT JOIN `borrowers` `patron` ON `patron`.`borrowernumber` = `me`.`borrowernumber` WHERE ( ( ( `me`.`cost` LIKE '%fra%' OR `me`.`placed` LIKE '%fra%' OR `me`.`status` LIKE '%fra%' OR `me`.`backend` LIKE '%fra%' OR `me`.`orderid` LIKE '%fra%' OR `me`.`replied` LIKE '%fra%' OR `me`.`updated` LIKE '%fra%' OR `me`.`accessurl` LIKE '%fra%' OR `me`.`biblio_id` LIKE '%fra%' OR `me`.`completed` LIKE '%fra%' OR `me`.`notesopac` LIKE '%fra%' OR `me`.`notesstaff` LIKE '%fra%' OR `me`.`price_paid` LIKE '%fra%' OR `ill_batch`.`name` LIKE '%fra%' OR `me`.`illrequest_id` LIKE '%fra%' OR `library`.`branchname` LIKE '%fra%' OR ( `extended_attributes_8`.`type` = 'type' AND `extended_attributes_8`.`value` LIKE '%fra%' ) OR ( `extended_attributes_6`.`type` = 'year' AND `extended_attributes_6`.`value` LIKE '%fra%' ) OR ( `extended_attributes_4`.`type` = 'issue' AND `extended_attributes_4`.`value` LIKE '%fra%' ) OR ( `extended_attributes_7`.`type` = 'pages' AND `extended_attributes_7`.`value` LIKE '%fra%' ) OR ( `extended_attributes_2`.`type` = 'title' AND `extended_attributes_2`.`value` LIKE '%fra%' ) OR ( `extended_attributes`.`type` = 'author' AND `extended_attributes`.`value` LIKE '%fra%' ) OR ( `extended_attributes_5`.`type` = 'volume' AND `extended_attributes_5`.`value` LIKE '%fra%' ) OR ( `extended_attributes_3`.`type` = 'article_title' AND `extended_attributes_3`.`value` LIKE '%fra%' ) OR `patron`.`surname` LIKE '%fra%' OR `patron`.`firstname` LIKE '%fra%' OR `patron`.`cardnumber` LIKE '%fra%' ) AND `status` NOT IN ( 'COMP' ) AND ( `status_alias` NOT IN ( 'COMP' ) OR `status_alias` IS NULL ) ) ) GROUP BY `me`.`illrequest_id`) `me`;
```

which kills MariaDB \o/

It definitely hangs up the mariadb threads in a way that never seems to complete: 

|  46 | koha_reach2 | localhost | koha_reach2 | Query   | 82188 | Sending data             | SELECT COUNT( * ) FROM (SELECT `me`.`illrequest_id` FROM `illrequests` `me` LEFT JOIN `illrequestatt |    0.000 |

sorry I didn't get the full query for this.

(In reply to Liz Rea from comment #39)
> It definitely hangs up the mariadb threads

(In reply to Tomás Cohen Arazi from comment #38)
> Using the DT filter box for searching "fra" is generating this query
> which kills MariaDB \o/

Opened bug 37389.

(In reply to Martin Renvoize (ashimema) from comment #33)
> and preferably ported back too.
> 

Passage of time, it's on stable now, but...

(In reply to Fridolin Somers from comment #37)
> Backport on 23.11.x fails on t/db_dependent/selenium/patrons_search.t
> Please create patches for 23.11.x if possible.