Bug 36025 - Extended attributes clause added to patron search query even when there are no searchable attributes
Summary: Extended attributes clause added to patron search query even when there are n...
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Andreas Jonsson
QA Contact: Nick Clemens (kidclamp)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-02-07 20:16 UTC by Andreas Jonsson
Modified: 2025-02-11 12:18 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
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 (2.80 KB, patch)
2024-02-08 01:35 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 36025 Incorrect clause in patron search (3.78 KB, patch)
2024-02-08 01:35 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 36025: Selenium test for the query parameters (2.90 KB, patch)
2024-02-08 14:16 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36025: Incorrect clause in patron search (3.84 KB, patch)
2024-02-08 14:16 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36025: Selenium test for the query parameters (2.63 KB, patch)
2024-12-23 19:51 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36025: Incorrect clause in patron search (3.40 KB, patch)
2024-12-23 19:51 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36025: Selenium test for the query parameters (2.15 KB, patch)
2024-12-27 10:08 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 36025: Selenium test for the query parameters (2.82 KB, patch)
2025-01-17 17:31 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36025: Incorrect clause in patron search (3.98 KB, patch)
2025-01-17 17:31 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36025: Add cypress tests (5.54 KB, patch)
2025-01-20 15:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36025: Fix failure when pref is off (1.06 KB, patch)
2025-01-20 15:08 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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
Created attachment 161873 [details] [review]
Bug 36025 Selenium test for the query parameters

The test verifies that there is no clause for extended
attributes in the patron search query when no patron
attribute is searchable by default.
Comment 3 Andreas Jonsson 2024-02-08 01:35:10 UTC
Created attachment 161874 [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 [""] in
the template patron-search.inc most likely due to
a bug in DBIx::Class.

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'
Comment 4 Nick Clemens (kidclamp) 2024-02-08 14:16:41 UTC
Created attachment 161914 [details] [review]
Bug 36025: Selenium test for the query parameters

The test verifies that there is no clause for extended
attributes in the patron search query when no patron
attribute is searchable by default.

NC amended patch - tidied

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Nick Clemens (kidclamp) 2024-02-08 14:16:43 UTC
Created attachment 161915 [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 [""] in
the template patron-search.inc most likely due to
a bug in DBIx::Class.

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'

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Nick Clemens (kidclamp) 2024-12-23 19:51:32 UTC
Created attachment 175917 [details] [review]
Bug 36025: Selenium test for the query parameters

The test verifies that there is no clause for extended
attributes in the patron search query when no patron
attribute is searchable by default.

NC amended patch - tidied

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>


Current status: Patch doesn't apply
Comment 7 Nick Clemens (kidclamp) 2024-12-23 19:51:34 UTC
Created attachment 175918 [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 [""] in
the template patron-search.inc most likely due to
a bug in DBIx::Class.

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'

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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
Created attachment 175952 [details] [review]
Bug 36025: Selenium test for the query parameters

Fix selenium test:

1. Use classname of form instead of no longer existing id
2. Fix test count
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
Created attachment 176747 [details] [review]
Bug 36025: Selenium test for the query parameters

The test verifies that there is no clause for extended
attributes in the patron search query when no patron
attribute is searchable by default.

NC amended patch - tidied

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Nick Clemens (kidclamp) 2025-01-17 17:31:28 UTC
Created attachment 176748 [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>
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
Created attachment 176823 [details] [review]
Bug 36025: Add cypress tests
Comment 21 Jonathan Druart 2025-01-20 15:08:08 UTC
Created attachment 176824 [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"
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?