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: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: 23.11
Hardware: All All
: P5 - low minor (vote)
Assignee: Andreas Jonsson
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-02-07 20:16 UTC by Andreas Jonsson
Modified: 2024-03-22 11:23 UTC (History)
7 users (show)

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


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

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>