Bug 36025

Summary: Extended attributes clause added to patron search query even when there are no searchable attributes
Product: Koha Reporter: Andreas Jonsson <andreas.jonsson>
Component: PatronsAssignee: Andreas Jonsson <andreas.jonsson>
Status: Pushed to main --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: bywater, david, gmcharlt, jonathan.druart, kyle.m.hall, kyle, laura, m.de.rooy, martin.renvoize, nick, pedro.amorim, sspohn, tomascohen
Version: MainKeywords: rel_24_11_candidate
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34519
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33554
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35361
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30645
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
REVERTED, REMOVE FROM RELASE NOTES
Version(s) released in:
25.05.00
Circulation function:
Attachments: Bug 36025 Selenium test for the query parameters
Bug 36025 Incorrect clause in patron search
Bug 36025: Selenium test for the query parameters
Bug 36025: Incorrect clause in patron search
Bug 36025: Selenium test for the query parameters
Bug 36025: Incorrect clause in patron search
Bug 36025: Selenium test for the query parameters
Bug 36025: Selenium test for the query parameters
Bug 36025: Incorrect clause in patron search
Bug 36025: Add cypress tests
Bug 36025: Fix failure when pref is off
Bug 36025: Incorrect clause in patron search
Bug 36025: Add cypress tests
Bug 36025: Fix failure when pref is off
Bug 36025: Incorrect clause in patron search
Bug 36025: Add cypress tests
Bug 36025: Fix failure when pref is off

Description Andreas Jonsson 2024-02-07 20:16:26 UTC
In bug 34519 there is a commit labelled "ensure we always have an array":

-            [% SET extended_attribute_types = ExtendedAttributeTypes.codes( staff_searchable => 1 ) %]
+            [% SET extended_attribute_types = [ ExtendedAttributeTypes.codes( staff_searchable => 1 ) ] %]

Somehow this expands into an array containing the empty string as an element if no attributes are searchable by default.

This is likely a bug in DBIx::Class.  It would be better to add the workaround for this in the ExtendedAtributeTypes template plugin.

The get_column method of ResultSet returns values of type ResultSetColumn according to the documentation.  But in reality it does not.  It returns an array.

So we must rely on a bug already by using this method.  Consistently using it in list context might be helpful as a workaround.

sub codes {
    my ( $self, $params ) = @_;

    my @codes = Koha::Patron::Attribute::Types->search($params)->get_column('code');

    return \@codes;
}

The consequence is that a clause for extended attributes is added to the patron search query.  This is significantly slower than not having such a clause, due to some other issues with DBIx::Class.  (Se bug 33554 and bug 35361.)
Comment 1 Andreas Jonsson 2024-02-08 01:33:20 UTC
There is also another issue in buildPatronSearchQuery that still adds the clause. Someone is trying to use an array as a boolean value in Javascript.  (A work related brain injury caused by excessive Perl programming?)

--- a/koha-tmpl/intranet-tmpl/prog/js/staff-global.js
+++ b/koha-tmpl/intranet-tmpl/prog/js/staff-global.js
@@ -650,7 +650,7 @@ function buildPatronSearchQuery(term, options) {
     q.push({ "-or": term_subquery_or });
 
     // Add each pattern for each extended patron attributes
-    if ( typeof options !== 'undefined' && ( (options.search_fields == 'standard' && options.extended_attribute_types) || ( searched_attribute_fields.length > 0 ) ) && extendedPatronAttributes) {
+    if ( typeof options !== 'undefined' && ( (options.search_fields == 'standard' && options.extended_attribute_types.length > 0) || ( searched_attribute_fields.length > 0 ) ) && extendedPatronAttributes) {
         extended_attribute_codes_to_search = (searched_attribute_fields.length > 0) ? searched_attribute_fields : options.extended_attribute_types;
         extended_attribute_subquery_and = [];
         patterns.forEach(function (pattern, i) {
Comment 2 Andreas Jonsson 2024-02-08 01:35:08 UTC Comment hidden (obsolete)
Comment 3 Andreas Jonsson 2024-02-08 01:35:10 UTC Comment hidden (obsolete)
Comment 4 Nick Clemens (kidclamp) 2024-02-08 14:16:41 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens (kidclamp) 2024-02-08 14:16:43 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens (kidclamp) 2024-12-23 19:51:32 UTC Comment hidden (obsolete)
Comment 7 Nick Clemens (kidclamp) 2024-12-23 19:51:34 UTC Comment hidden (obsolete)
Comment 8 Nick Clemens (kidclamp) 2024-12-23 19:51:59 UTC
I tried to rebase, it applies but tests don't pass - can you look Andreas?
Comment 9 Andreas Jonsson 2024-12-27 10:08:09 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2024-12-30 13:40:57 UTC
QA by RM.

The selenium tests gave me some trouble at first, but they run smoothly on a clean database.
Comment 11 Katrin Fischer 2024-12-30 14:14:56 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 12 Katrin Fischer 2024-12-30 15:38:35 UTC
The Selenium tests are fine, but we broke some other tests:

t_db_dependent_Koha_Template_Plugin_ExtendedAttributeTypes_t.codes
Comment 13 Katrin Fischer 2025-01-03 14:05:32 UTC
(In reply to Katrin Fischer from comment #12)
> The Selenium tests are fine, but we broke some other tests:
> 
> t_db_dependent_Koha_Template_Plugin_ExtendedAttributeTypes_t.codes

This hinders pushing new features. Please help to fix.
Comment 14 Katrin Fischer 2025-01-07 14:50:24 UTC
I have reverted these patches from main. 

There are some other concerns that were raised on Mattermost: 

* A change to .pm should have unit test, a selenium test doesn't cover this requirement.
* Context could be forced in the .tt avoiding the change of the return signature.
Comment 15 Jonathan Druart 2025-01-07 14:58:21 UTC
Can you explain the selenium test please? What you are doing with this JS code there?
Comment 16 Nick Clemens (kidclamp) 2025-01-17 16:29:20 UTC
(In reply to Jonathan Druart from comment #15)
> Can you explain the selenium test please? What you are doing with this JS
> code there?

The selenium test is trying to prove that there are no extended attributes included in the search, I had looked but wasn't able to find a better way of doing that
Comment 17 Nick Clemens (kidclamp) 2025-01-17 17:31:25 UTC Comment hidden (obsolete)
Comment 18 Nick Clemens (kidclamp) 2025-01-17 17:31:28 UTC Comment hidden (obsolete)
Comment 19 Nick Clemens (kidclamp) 2025-01-17 17:32:54 UTC
Removed module changes, updated template, please test, this is an issue for libraries with many attributes that are not searchable, performance can be highly impacted (see bug 33554)
Comment 20 Jonathan Druart 2025-01-20 15:08:05 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2025-01-20 15:08:08 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2025-01-20 15:11:30 UTC
I think those cypress tests are better: more readable, maintainable, and extendable.

There is only one downside: we don't correctly test if there is at least one searchable attribute in DB (we don't want to remove them all).

I think it's good as it as the default data do not have any.

What do you think?
Comment 23 David Nind 2025-01-25 20:30:50 UTC
I've attempted to test - everything works except the Selenium tests. They don't pass for me.[1]

[1] Results from running the Selenium tests:

prove t/db_dependent/selenium/patrons_search.t
t/db_dependent/selenium/patrons_search.t .. 1/2 
        #   Failed test 'there were no extended attributes on the search clause'
        #   at t/db_dependent/selenium/patrons_search.t line 878.
        #          got: '1'
        #     expected: '0'
        # Looks like you failed 1 test of 2.

    #   Failed test 'no clause for extended_attributes when none are default searchable'
    #   at t/db_dependent/selenium/patrons_search.t line 880.
    # Looks like you failed 1 test of 3.
t/db_dependent/selenium/patrons_search.t .. 2/2 
#   Failed test 'Search patrons in modal'
#   at t/db_dependent/selenium/patrons_search.t line 883.
# Looks like you failed 1 test of 2.
t/db_dependent/selenium/patrons_search.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/2 subtests 

Test Summary Report
-------------------
t/db_dependent/selenium/patrons_search.t (Wstat: 256 (exited 1) Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=1, Tests=2, 132 wallclock secs ( 0.03 usr  0.00 sys +  5.44 cusr  2.11 csys =  7.58 CPU)
Result: FAIL

[2] Testing notes (using KTD):

1. Start up KTD with Selenium: ktd --selenium up
2. Create two new patron attributes, without "Searchable" and "Search by default" set:
   . Administration > Patrons and circulation > Patron attribute types
3. Add some values for two new attributes to a couple of patrons, for example:
   . Attribute 1: ABC
   . Attribute 2: XYZ
4. Open the network testing tools using your browser's web developer tools. In Firefox, Menu > More tools > Web Developer Tools > Network.
5. Go to Patrons and do a search that returns multiple patrons, for example search for 'a'.
6. In the network tab, look for the <staffinterfaceurl>:8081/api/v1/patrons?_page=1.... request and note that there is a "extended_attributes.code" as part of the URL.
7. Apply the patch and restart everything (restart_all).
8. Update one of the attributes from step 3 so that "Searchable" and "Search by default" are set.
9. Repeat steps 5 to 6 and note that request now includes the "extended_attributes.code".
10. Search for "ABC" and the results should include the two patrons you set this value for.
11. Update the other attribute from step 3 so that "Searchable" and "Search by default" are set.
12. Search for "XYZ" and the results should include the two patrons you set this value for.
13. Check that the tests pass:
    . New Cypress tests: yarn cypress run --spec t/cypress/integration/PatronSearch_spec.ts
    . Selenium tests: prove t/db_dependent/selenium/patrons_search.t
Comment 24 Nick Clemens (kidclamp) 2025-02-11 12:17:59 UTC
Comment on attachment 176747 [details] [review]
Bug 36025: Selenium test for the query parameters

Marking obsolete in favor of the cypress tests
Comment 25 Nick Clemens (kidclamp) 2025-02-11 12:18:56 UTC
(In reply to David Nind from comment #23)
> I've attempted to test - everything works except the Selenium tests. They
> don't pass for me.[1]
> 
> [1] Results from running the Selenium tests:
> 
> prove t/db_dependent/selenium/patrons_search.t
> t/db_dependent/selenium/patrons_search.t .. 1/2 
>         #   Failed test 'there were no extended attributes on the search
> clause'
>         #   at t/db_dependent/selenium/patrons_search.t line 878.
>         #          got: '1'
>         #     expected: '0'
>         # Looks like you failed 1 test of 2.
> 
>     #   Failed test 'no clause for extended_attributes when none are default
> searchable'
>     #   at t/db_dependent/selenium/patrons_search.t line 880.
>     # Looks like you failed 1 test of 3.
> t/db_dependent/selenium/patrons_search.t .. 2/2 
> #   Failed test 'Search patrons in modal'
> #   at t/db_dependent/selenium/patrons_search.t line 883.
> # Looks like you failed 1 test of 2.
> t/db_dependent/selenium/patrons_search.t .. Dubious, test returned 1 (wstat
> 256, 0x100)
> Failed 1/2 subtests 
> 

The selenium tests would fail when there are unexpected attributes defined in the DB - I obsoleted that patch in favor of Joubu's Cypress tests - if they worked for you could you add your SO?
Comment 26 Magnus Enger 2025-02-13 16:27:38 UTC
Created attachment 178037 [details] [review]
Bug 36025: Incorrect clause in patron search

This patch removes the search clause for
extended_attributes when there is no patron
attribute types which are searchable by default.

The clause was incorrectly added because the array
extended_attribute_types was rendered as [""]

This seems to be an issue with get_columns from DBIX Class and how that is cast when forcing an array
in the template. It returns an empty string for nothing, a scalar for a single value, and an array for multiple
values.

This patch now checks if the variable is set and has content, and wraps as an array if so. In the case of two attributes,
we do double the array, but this was true before as we always cast the return as an array.

Test plan:

* Make sure that no borrower attribute type is set to "searched by default"
  in Koha administration -> Patron attribute types.
* Enable the network monitring in the developer tools in your web browser
  (Ctrl + Shift + E in firefox or Ctrl + Shift + I and select the "Network"
  tab).
* Go to the patrons view members-home.pl (click patrons in top menu bar).
* Search for a string that will match multiple borrowers (to not be automatically
  redirected)
* Examine the ajax-request and make sure that "extended_attributes.code" is NOT
  part of the query.
* Also, in koha-testing-docker with selenium enabled,
  run 'perl t/db_dependent/selenium/patrons_search.t'
* Now set one patro attribute to searchable and searched by default and add a value for a patron, confirm search works
* Set a second patron attribute to searchable and searched by default and confirm searching works

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised. Did not run the selenium tests, as they have
been obsoleted.
Comment 27 Magnus Enger 2025-02-13 16:27:44 UTC
Created attachment 178038 [details] [review]
Bug 36025: Add cypress tests

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Tests pass, with the followup patch.
Comment 28 Magnus Enger 2025-02-13 16:27:47 UTC
Created attachment 178039 [details] [review]
Bug 36025: Fix failure when pref is off

Prevent JS error if ExtendedPatronAttributes is off
 "Uncaught TypeError: options.extended_attribute_types is undefined"

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Makes the Cypress tests pass.
Comment 29 Marcel de Rooy 2025-02-14 08:33:46 UTC
Created attachment 178081 [details] [review]
Bug 36025: Incorrect clause in patron search

This patch removes the search clause for
extended_attributes when there is no patron
attribute types which are searchable by default.

The clause was incorrectly added because the array
extended_attribute_types was rendered as [""]

This seems to be an issue with get_columns from DBIX Class and how that is cast when forcing an array
in the template. It returns an empty string for nothing, a scalar for a single value, and an array for multiple
values.

This patch now checks if the variable is set and has content, and wraps as an array if so. In the case of two attributes,
we do double the array, but this was true before as we always cast the return as an array.

Test plan:

* Make sure that no borrower attribute type is set to "searched by default"
  in Koha administration -> Patron attribute types.
* Enable the network monitring in the developer tools in your web browser
  (Ctrl + Shift + E in firefox or Ctrl + Shift + I and select the "Network"
  tab).
* Go to the patrons view members-home.pl (click patrons in top menu bar).
* Search for a string that will match multiple borrowers (to not be automatically
  redirected)
* Examine the ajax-request and make sure that "extended_attributes.code" is NOT
  part of the query.
* Also, in koha-testing-docker with selenium enabled,
  run 'perl t/db_dependent/selenium/patrons_search.t'
* Now set one patro attribute to searchable and searched by default and add a value for a patron, confirm search works
* Set a second patron attribute to searchable and searched by default and confirm searching works

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised. Did not run the selenium tests, as they have
been obsoleted.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2025-02-14 08:33:49 UTC
Created attachment 178082 [details] [review]
Bug 36025: Add cypress tests

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Tests pass, with the followup patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2025-02-14 08:33:52 UTC
Created attachment 178083 [details] [review]
Bug 36025: Fix failure when pref is off

Prevent JS error if ExtendedPatronAttributes is off
 "Uncaught TypeError: options.extended_attribute_types is undefined"

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Makes the Cypress tests pass.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Katrin Fischer 2025-02-17 13:52:01 UTC
 FAIL	t/cypress/support/e2e.js
   FAIL	  tidiness
		File is not tidy, please run `perl misc/devel/tidy.pl t/cypress/support/e2e.js`

I ran the tidy and merged the changes into the second patch.
Comment 33 Katrin Fischer 2025-02-17 14:07:07 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 34 Laura O'Neil 2025-02-19 20:31:38 UTC
Will it be possible to backport this to 24.11?