Bug 14874 introduced syspref 'DefaultPatronSearchFields' to define what fields should be used in default patron search. It would be nice see the fields in the search fields drop down as well. At the moment the are hard coded to: Surname,Email,Borrower number,Username,Phone number,Street Address,Date of birth,Sort field 1,Sort field 2
Created attachment 56921 [details] [review] Bug 17374 - Make use of fields from syspref 'DefaultPatronSearchFields' in patron search fields dropdown This patch preserves the current dropdown choices for patron search and adds fields from the DefaultPatronSearchFields system preference To test: 1 - View the regular patron search and note fields in dropdown 2 - Apply patch, ensure dropdown has not changed 3 - Add fields to DefaultPatronSearchFields, note they are available in dropdown 4 - Ensure existing and new fields search properly
Created attachment 56931 [details] [review] Bug 17374 - Make use of fields from syspref 'DefaultPatronSearchFields' in patron search fields dropdown This patch preserves the current dropdown choices for patron search and adds fields from the DefaultPatronSearchFields system preference To test: 1 - View the regular patron search and note fields in dropdown 2 - Apply patch, ensure dropdown has not changed 3 - Add fields to DefaultPatronSearchFields, note they are available in dropdown 4 - Ensure existing and new fields search properly
Hi Nick, can you add tags <span>...</span> for the options? From this [%- CASE 'standard' -%]Standard to this [%- CASE 'standard' -%]<span>Standard</span> It's better to translate each string than a big one full of '%s'.
Created attachment 56958 [details] [review] Bug 17374 - Make use of fields from syspref 'DefaultPatronSearchFields' in patron search fields dropdown This patch preserves the current dropdown choices for patron search and adds fields from the DefaultPatronSearchFields system preference To test: 1 - View the regular patron search and note fields in dropdown 2 - Apply patch, ensure dropdown has not changed 3 - Add fields to DefaultPatronSearchFields, note they are available in dropdown 4 - Ensure existing and new fields search properly
Created attachment 58239 [details] [review] Bug 17374 - Make use of fields from syspref 'DefaultPatronSearchFields' in patron search fields dropdown This patch preserves the current dropdown choices for patron search and adds fields from the DefaultPatronSearchFields system preference To test: 1 - View the regular patron search and note fields in dropdown 2 - Apply patch, ensure dropdown has not changed 3 - Add fields to DefaultPatronSearchFields, note they are available in dropdown 4 - Ensure existing and new fields search properly Signed-off-by: Marc Véron <veron@veron.ch>
Is there a reason not to use the same list of fields on the Filters block (left side of patron list)? members/member.tt If an entry of the pref is not valid, we will know get an empty entry. It may be better to skip it.
Created attachment 66400 [details] [review] Bug 17374 - QA Followup Skip invlid field names Add fields to sidebar
1) If you leave the preference empty, the default will show. The default doesn't include cardnumber, I think it might be good to include it on top. 2) When using the preference, the default will still show, so there is no way to remove fields. Wouldn't it make more sense to use the non-empty pref instead of the default? For example a lot of libraries don't use the sort fields and could remove them from the list this way. The current behaviour seems especially confusing since the preference is not empty by default but includes a list of values. Deleting one of them will have no effect as long as the value is in the list of default. Failing for 2).
Created attachment 70226 [details] [review] Bug 17374 - Add cardnumber to list of searchable patron fields To test: 1 - Apply patch 2 - Perform a 'cardnumber' search for patrons 3 - Verify results as expected
(In reply to Katrin Fischer from comment #8) > 1) If you leave the preference empty, the default will show. The default > doesn't include cardnumber, I think it might be good to include it on top. > > 2) When using the preference, the default will still show, so there is no > way to remove fields. Wouldn't it make more sense to use the non-empty pref > instead of the default? For example a lot of libraries don't use the sort > fields and could remove them from the list this way. > > The current behaviour seems especially confusing since the preference is not > empty by default but includes a list of values. Deleting one of them will > have no effect as long as the value is in the list of default. > > Failing for 2). 1 - Attached patch (this may be better as its own report) 2 - I think this is beyond scope here. The preference is intended to simply control the fields searched when using a 'standard' search. Adding these the dropdown is a convenience feature. I think removing fields is better done by javascript for customization. The list of fields here and the list in the pref didn't have parity before so introducing now would be a bigger change. Moving back to QA queue for second opinion
Hm, I think now I understand better how this works: DefaultPatronSearchFields currently defines what is used for the "standard" search option but doesn't change the pull down. I think the description of the pref needs to be improved to be more clear about its use: Comma separated list defining the default fields to be used during a patron search Should be currently mroe something like: Comma separated list defining the default fields to be used during a spatron search using the 'standard' search option. With the new behaviour then: Comma separated list defining the default fields to be used during a spatron search using the 'standard' search option. If left empty, Koha will use: ... If you add additional fields not in the default, they will also show as separate search options in the drop down. This about right? Nick, would it make sense to you to adapt the pref description a bit? I think my problem mostly was not understanding how this is supposed to work and others might have the same problem :)
Created attachment 71536 [details] [review] Bug 17374: (follow-up) update description of syspref DefaultPatronSearchFields
Would have accepted a change to just .pref :)
Created attachment 71597 [details] [review] Bug 17374: Make use of fields from syspref 'DefaultPatronSearchFields' in patron search fields dropdown This patch preserves the current dropdown choices for patron search and adds fields from the DefaultPatronSearchFields system preference To test: 1 - View the regular patron search and note fields in dropdown 2 - Apply patch, ensure dropdown has not changed 3 - Add fields to DefaultPatronSearchFields, note they are available in dropdown 4 - Ensure existing and new fields search properly Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 71598 [details] [review] Bug 17374: (QA follow-up) Skip invalid field names and add fields to sidebar Skip invlid field names Add fields to sidebar Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 71599 [details] [review] Bug 17374: (QA follow-up) Add cardnumber to list of searchable patron fields To test: 1 - Apply patch 2 - Perform a 'cardnumber' search for patrons 3 - Verify results as expected Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 71600 [details] [review] Bug 17374: (QA follow-up) Update description of syspref DefaultPatronSearchFields Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Fixed koha to Koha in the last commit and changed commit subject lines to meet standard.
+ [% NEXT IF display_name.length == 5 %] Hum?? How did you create the list for patronfields.inc? (if all of the borrowers table, then you are missing few).
*** Bug 20263 has been marked as a duplicate of this bug. ***
Created attachment 74828 [details] [review] Bug 17374: (follow-up) Remove spaces from include file
(In reply to Jonathan Druart from comment #19) > + [% NEXT IF display_name.length == 5 %] > > Hum?? Ah, the indents in the file were adding 5 spaces - I didn't know where they had come from but found that solution. Improved now. > > How did you create the list for patronfields.inc? (if all of the borrowers > table, then you are missing few). No, I believe I curated this to fields that were reasonable to search here. If we want more we need to add them I think ( I think some were added after this patchset ) Let me know what you want here.
2 majors problems: 1. compare columns.def and patronfields.inc => They are the "same" 2. It does not work, and cannot work without changes to C4/Utils/DataTables/Members.pm Test: Add altcontactsurname to the pref, it appears in the dropdown list, select it, search for something: everything is returned. Because of the following: my $searchfields = { # list of available fields then later: 130 for my $searchfield ( split /,/, $searchfields->{$searchfieldstype} ) { 131 push @where_strs_or, "borrowers." . $dbh->quote_identifier($searchfield) . " LIKE ?"; 132 push @where_args, $term; 133 }
Created attachment 93226 [details] [review] Bug 17374: Make use of fields from syspref 'DefaultPatronSearchFields' in patron search fields dropdown This patch preserves the current dropdown choices for patron search and adds fields from the DefaultPatronSearchFields system preference To test: 1 - View the regular patron search and note fields in dropdown 2 - Apply patch, ensure dropdown has not changed 3 - Add fields to DefaultPatronSearchFields, note they are available in dropdown 4 - Ensure existing and new fields search properly
(In reply to Jonathan Druart from comment #23) > 2 majors problems: > > 1. compare columns.def and patronfields.inc > => They are the "same" > Yes, but I think this is for another report. The new include removes duplicated code from two templates. I don't fully understand the use of the columns.def - it looks the same but doesn't work the same > 2. It does not work, and cannot work without changes to > C4/Utils/DataTables/Members.pm > Fixed
Created attachment 94831 [details] [review] Bug 17374: Make use of fields from syspref 'DefaultPatronSearchFields' in patron search fields dropdown This patch preserves the current dropdown choices for patron search and adds fields from the DefaultPatronSearchFields system preference To test: 1 - View the regular patron search and note fields in dropdown 2 - Apply patch, ensure dropdown has not changed 3 - Add fields to DefaultPatronSearchFields, note they are available in dropdown 4 - Ensure existing and new fields search properly Signed-off-by: Kelly <kelly@bywatersolutions.com>
This works as expected, see my video https://www.loom.com/share/3ab9469f887a4a2aa2793855bc99a7ad
There are debug statements left in the patch. Also, tests are missing.
Created attachment 94876 [details] [review] Bug 17374: (follow-up) Unit tests and remove warns
Created attachment 94890 [details] [review] Bug 17374: Make use of fields from syspref 'DefaultPatronSearchFields' in patron search fields dropdown This patch preserves the current dropdown choices for patron search and adds fields from the DefaultPatronSearchFields system preference To test: 1 - View the regular patron search and note fields in dropdown 2 - Apply patch, ensure dropdown has not changed 3 - Add fields to DefaultPatronSearchFields, note they are available in dropdown 4 - Ensure existing and new fields search properly Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Maxime Dufresne <maxime.dufresne@inlibro.com>
Created attachment 94891 [details] [review] Bug 17374: (follow-up) Unit tests and remove warns Signed-off-by: Maxime Dufresne <maxime.dufresne@inlibro.com>
Created attachment 94892 [details] [review] Bug 17374: Make use of fields from syspref 'DefaultPatronSearchFields' in patron search fields dropdown This patch preserves the current dropdown choices for patron search and adds fields from the DefaultPatronSearchFields system preference To test: 1 - View the regular patron search and note fields in dropdown 2 - Apply patch, ensure dropdown has not changed 3 - Add fields to DefaultPatronSearchFields, note they are available in dropdown 4 - Ensure existing and new fields search properly Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Maxime Dufresne <maxime.dufresne@inlibro.com>
Test plan was fine, but I realized that the dropdown options were different between before and after patch: I saw at least one new field. Everything else worked as expected.
(In reply to Nick Clemens from comment #22) > (In reply to Jonathan Druart from comment #19) > > + [% NEXT IF display_name.length == 5 %] > > > > Hum?? > Ah, the indents in the file were adding 5 spaces - I didn't know where they > had come from but found that solution. Improved now. + [% NEXT IF display_name.length == 5 %] Still there? Also there are debug statements in the last patch.
Created attachment 96341 [details] [review] Bug 17374: Make use of fields from syspref 'DefaultPatronSearchFields' in patron search fields dropdown This patch preserves the current dropdown choices for patron search and adds fields from the DefaultPatronSearchFields system preference To test: 1 - View the regular patron search and note fields in dropdown 2 - Apply patch, ensure dropdown has not changed 3 - Add fields to DefaultPatronSearchFields, note they are available in dropdown 4 - Ensure existing and new fields search properly Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Maxime Dufresne <maxime.dufresne@inlibro.com>
Created attachment 96342 [details] [review] Bug 17374: (follow-up) Remove spaces from include file
Created attachment 96343 [details] [review] Bug 17374: (follow-up) Unit tests and remove warns Signed-off-by: Maxime Dufresne <maxime.dufresne@inlibro.com>
(In reply to Jonathan Druart from comment #34) > (In reply to Nick Clemens from comment #22) > > (In reply to Jonathan Druart from comment #19) > > > + [% NEXT IF display_name.length == 5 %] > > > > > > Hum?? > > Ah, the indents in the file were adding 5 spaces - I didn't know where they > > had come from but found that solution. Improved now. > > > + [% NEXT IF display_name.length == 5 %] > > Still there? > > Also there are debug statements in the last patch. Looks like something happened in a rebase, patches restored and cleaned up
koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc: [% NEXT IF display_name.length == 5 %] So this one too I guess?
Created attachment 96679 [details] [review] Bug 17374: Fix another occurrence
Created attachment 98106 [details] [review] Bug 17374: (QA follow-up) Make search labels match with patron record Makes some small changes to the patronfields.inc to make sure the search labels match with the labels in the patron record.
The tests are failing: t/db_dependent/selenium/patrons_search.t .. # Failed test 'Invalid option not added when DefaultPatronSearchFields is populated with an invalid field' # at t/db_dependent/selenium/patrons_search.t line 97. # got: '13' # expected: '12' # Failed test 'Invalid filter option not added when DefaultPatronSearchFields is populated with an invalid field' # at t/db_dependent/selenium/patrons_search.t line 98. # got: '13' # expected: '12' # Looks like you failed 2 tests of 12. t/db_dependent/selenium/patrons_search.t .. 1/1 # Failed test 'Search patrons' # at t/db_dependent/selenium/patrons_search.t line 137. # Looks like you failed 1 test of 1.
Created attachment 98478 [details] [review] Bug 17374: (follow-up) Remove all spaces from include file
Created attachment 100436 [details] [review] Bug 17374: Make use of fields from syspref 'DefaultPatronSearchFields' in patron search fields dropdown This patch preserves the current dropdown choices for patron search and adds fields from the DefaultPatronSearchFields system preference To test: 1 - View the regular patron search and note fields in dropdown 2 - Apply patch, ensure dropdown has not changed 3 - Add fields to DefaultPatronSearchFields, note they are available in dropdown 4 - Ensure existing and new fields search properly Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Maxime Dufresne <maxime.dufresne@inlibro.com> Bug 17374: (follow-up) Remove all spaces from include file Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 100437 [details] [review] Bug 17374: (follow-up) Remove spaces from include file Bug 17374: Fix another occurrence Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 100438 [details] [review] Bug 17374: (follow-up) Unit tests Signed-off-by: Maxime Dufresne <maxime.dufresne@inlibro.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 100439 [details] [review] Bug 17374: (QA follow-up) Make search labels match with patron record Makes some small changes to the patronfields.inc to make sure the search labels match with the labels in the patron record. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I cleaned the patches a bit (squash and remove warn, whitespaces, etc.). After this patch, columns.def and koha-tmpl/intranet-tmpl/prog/en/includes/patronfields.inc list the borrowers fields. And bug 22844 is going to introduce a third one (koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/borrowers.json). We will need to centralize all of them in a single file.
Nice work everyone! Pushed to master for 20.05
enhancement not backported to 19.11.x