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.)
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) {
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.
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'
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>
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>
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
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>
I tried to rebase, it applies but tests don't pass - can you look Andreas?
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
QA by RM. The selenium tests gave me some trouble at first, but they run smoothly on a clean database.
Pushed for 25.05! Well done everyone, thank you!
The Selenium tests are fine, but we broke some other tests: t_db_dependent_Koha_Template_Plugin_ExtendedAttributeTypes_t.codes
(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.
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.
Can you explain the selenium test please? What you are doing with this JS code there?
(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
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>
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>
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)
Created attachment 176823 [details] [review] Bug 36025: Add cypress tests
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"
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?
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 on attachment 176747 [details] [review] Bug 36025: Selenium test for the query parameters Marking obsolete in favor of the cypress tests
(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?