|
Description
Kelly McElligott
2022-01-07 17:45:23 UTC
*** Bug 30608 has been marked as a duplicate of this bug. *** Created attachment 144814 [details] [review] Bug 29822: Update 'DefaultPatronSearchFields' to a select list This patch updates the `DefaultPatronSearchFields` preference from freetext expecting a comma delimited list of borrower fields to a multi-select with a sensible list of fields available. I opted to not use the Modal approach with all borrower fields listed as I felt it made more sense to only expose a more limited subset of the fields available to us. Created attachment 144831 [details] [review] Bug 29822: Update 'DefaultPatronSearchFields' to a select list This patch updates the `DefaultPatronSearchFields` preference from freetext expecting a comma delimited list of borrower fields to a multi-select with a sensible list of fields available. I opted to not use the Modal approach with all borrower fields listed as I felt it made more sense to only expose a more limited subset of the fields available to us. Signed-off-by: David Nind <david@davidnind.com> The test plan I used (using KTD): 1. Check system preference DefaultPatronSearchFields and note that it is defined by entering a comma separated list. 2. Go to the patrons module and view the HTML source for the page: . search the source for 'standard' - this should have two results . note that the option value for the Standard input field are "firstname,middle_name,surname,othernames,cardnumber,userid" 3. Apply the patch. 4. Reload or search again for the DefaultPatronSearchFields system preference. 5. Note that this is now a dropdown list with selectable values, rather than needing to enter a comma separated list. 6. Add a field to the list, such as 'Primary email'. 7. Repeat step 2 - the standard input field should now include the field you added. 8. Make sure some patrons have a primary email address, such as firstname@gmail.com 9. Do a standard search for gmail.com ==> the results should include all the patrons you added a gmail address to 10. Do a specific search on the 'Primary email' field ==> the search results should be the same as for step 9 BONUS: Perform the same search using the header search bar and the search options drop down (note: you need to change the 'Search type' from 'Starts with' to 'Contains'). (entered in the 'Other phone' field Ignore the last bit of random text! I was going to comment on the fact that you enter the mobile number in the 'Other number' field for this to work for patron search. This is different from entering the SMS number when the SMSSendDriver system preference is enabled. It took me a while to figure this out. Should I create a bug to sort out this confusion? I found a couple of bugs related to this, but Bug 14872 (specifically comment no. 6) would seem to be a better approach. Thanks for testing David, always great to get your feedback. I should have said somewhere I. The bug, the list of fields I picked is entirely arbitrarily picked at the moment.. I picked what made most sense to me.. so do feel free to help refine that list. Regards SMS alert number, I found that one rather confusing too, though I wasn't aware of the bug you linked. I like the idea of dropping the filed from borrowers and instead having a preference to define which field should contain this notification number. I'll go comment there. Many thanks for testing again. 1) I believe the list is now too limited. This is also likely to cause issues for libraries that have added fields not in your list. I think we should offer the full list, like BorrowerUnwantedField does - it would also simplify your patch :) 2) I believe we had discussed not changing this one, because of something with the extended patron attributes, but I cannot remember the exact issue now :( (In reply to Katrin Fischer from comment #7) > > 1) I believe the list is now too limited. This is also likely to cause > issues for libraries that have added fields not in your list. I think we > should offer the full list, like BorrowerUnwantedField does - it would also > simplify your patch :) Honestly, I'm a bit surprised you want every field listed and I think it increases the code burden personally. > 2) I believe we had discussed not changing this one, because of something > with the extended patron attributes, but I cannot remember the exact issue > now :( I can definitively say we do not support patron attributes as options here: * The code limits the options to an allow list that can be seen in koha-tmpl/intranet-tmpl/prog/en/includes/patronfields.inc for the field selection in various patron search forms. * The autocomplete javascript does NOT limit the fields in the same way, but is hard coded to use `me.` as a table identifier.. adding `extended_attributes.field_name` is very unlikely to work, but I have not tested.. what I have found though is that if you add a random "field" name, then autocomplete just completely breaks. Created attachment 146430 [details]
Modal highlighting why I think it's a bad idea
I've attached a mock of the modal.. all the red dots are the fields I feel sohuld be actively disabled for this preference.. many of the remaining one's I also feel don't make much sense / would lead to unusable results - searching for 'mr' in the title field for example.. how is that remotely helpful?
Also, to convert this to a Modal we would need to do a database update to convert the existing preference from csv to pipe delimited string, update the preference descriptions and catch all the cases where the preference is currently in use and convert them from expecting a CSV to expecting a Pipe delimited string.
I'm happy to expand the select list.. but I really don't think it's appropriate to display a modal filled with options greyed out.
You can still use the modal and exclude those fields easily in the yaml file. That way we still have a consistent display/one place of code to change things and it's reduced to the more helfpul fields. The excludes just get greyed out rather than removed as far as I can tell. I'd include the note fields OPAC and circulation. Wondering: how do we handle additional patron attributes here? Are they always searchable automatically or not searchable at all? (In reply to Martin Renvoize from comment #11) > The excludes just get greyed out rather than removed as far as I can tell. Maybe we could rethink that? I still think having more ways to do a similar thing is not good. Can we deal with patron attributes as it's own bug please.. this is rapidly going from a small improvement to a large and difficult development. (In reply to Martin Renvoize from comment #14) > Can we deal with patron attributes as it's own bug please.. this is rapidly > going from a small improvement to a large and difficult development. I don't want to change behaviour, was just wondering about existing behaviour and that is about this bug. For BorrowerMandatoryFields you could add the patron attribute names to the system preference and it worked. If a similar feature existed for this, it could block switching it to a modal or at least we should verify first. This preference does not affect the attributes in any way.. I've varified that in the code. Attributes are handled as their own case in the main patron search page.. always searched. Created attachment 146445 [details] [review] Bug 29822: Use the modal This looks silly to me... and it still requires changes to koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc koha-tmpl/intranet-tmpl/prog/en/includes/member-search-box.inc koha-tmpl/intranet-tmpl/prog/en/includes/js_includes.inc and a database update. (In reply to Katrin Fischer from comment #7) > it would also simplify your patch :) Not in the slightest.. it makes it significantly more involved and has a much higher risk of regression. Created attachment 146488 [details] [review] Bug 29822: Update 'DefaultPatronSearchFields' to a select list This patch updates the `DefaultPatronSearchFields` preference from freetext expecting a comma delimited list of borrower fields to a multi-select with a sensible list of fields available. I opted to not use the Modal approach with all borrower fields listed as I felt it made more sense to only expose a more limited subset of the fields available to us. Created attachment 146489 [details] [review] Bug 29822: Use the modal As requested by Katrin.. switch to the modal and expose all fields from the borrowers database table Created attachment 146490 [details] [review] Bug 29822: Update code to reference by pipe Moving to modalselect also has the effect of moving from comma delimited to pipe delimitation for the preference contents Created attachment 146491 [details] [review] Bug 29822: Database and Preference description updates Created attachment 146492 [details] [review] Bug 29822: Update unit tests 1 patch turns into 5 to convert it to the Modal approach.. and the unit tests are failing.. I can't see why the split doesn't appear to work with pipe instead of comma as the separation character. Created attachment 147433 [details] [review] Bug 29822: Update 'DefaultPatronSearchFields' to a select list This patch updates the `DefaultPatronSearchFields` preference from freetext expecting a comma delimited list of borrower fields to a multi-select with a sensible list of fields available. I opted to not use the Modal approach with all borrower fields listed as I felt it made more sense to only expose a more limited subset of the fields available to us. Created attachment 147434 [details] [review] Bug 29822: Use the modal As requested by Katrin.. switch to the modal and expose all fields from the borrowers database table Created attachment 147435 [details] [review] Bug 29822: Update code to reference by pipe Moving to modalselect also has the effect of moving from comma delimited to pipe delimitation for the preference contents Created attachment 147436 [details] [review] Bug 29822: Database and Preference description updates Created attachment 147437 [details] [review] Bug 29822: Update unit tests Rebased Sorry, I get a template error:
Template process failed: file error - parse error - patronfields.inc line 90-91: unexpected token ([)
[% standard.replace('|', ',')
[% SET full_address = 'streetnumber,streettype,address,address2,city,state,zipcode,country' %] at /kohadevbox/koha/C4/Templates.pm line 127
(I also tried on PTFS' and Bywater's sandboxes, one with the patch applied, and one with the patch applied after initialisation - just to make sure it wasn't a sandbox fault.)
Created attachment 149890 [details] [review] Bug 29822: Update 'DefaultPatronSearchFields' to a select list This patch updates the `DefaultPatronSearchFields` preference from freetext expecting a comma delimited list of borrower fields to a multi-select with a sensible list of fields available. I opted to not use the Modal approach with all borrower fields listed as I felt it made more sense to only expose a more limited subset of the fields available to us. Created attachment 149891 [details] [review] Bug 29822: Use the modal As requested by Katrin.. switch to the modal and expose all fields from the borrowers database table Created attachment 149892 [details] [review] Bug 29822: Update code to reference by pipe Moving to modalselect also has the effect of moving from comma delimited to pipe delimitation for the preference contents Created attachment 149893 [details] [review] Bug 29822: Database and Preference description updates Created attachment 149894 [details] [review] Bug 29822: Update unit tests We should be back on again now.. master is moving so fast at the moment :) FAIL installer/data/mysql/atomicupdate/bug_29822.pl
FAIL file permissions
File must have the exec flag
OK installer/data/mysql/mandatory/sysprefs.sql
OK koha-tmpl/intranet-tmpl/prog/en/includes/js_includes.inc
OK koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc
FAIL koha-tmpl/intranet-tmpl/prog/en/includes/patronfields.inc
FAIL forbidden patterns
forbidden pattern: Do not use line breaks inside template tags (bug 18675) (line 90)
FAIL valid_template
parse error - /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/includes/patronfields.inc line 90-91: unexpected token ([)
[% standard.replace('|', ',')
[% SET full_address = 'streetnumber,streettype,address,address2,city,state,zipcode,country' %]
OK koha-tmpl/intranet-tmpl/prog/js/patron-autocomplete.js
OK t/db_dependent/selenium/patrons_search.t
Processing additional checks OK!
Running tests (1)
* Proving /kohadevbox/koha/t/db_dependent/selenium/patrons_search.t KO!
# Failed test 'All standard fields are searchable if DefaultPatronSearchFields not set. middle_name is there.' # at /kohadevbox/koha/t/db_dependent/selenium/patrons_search.t line 163.
# got: '0' # expected: '14'
# Looks like you planned 25 tests but ran 1. # Looks like you failed 1 test of 1 run.
# Failed test 'Search patrons'
# at /kohadevbox/koha/t/db_dependent/selenium/patrons_search.t line 341.
Can't call method "get_value" on an undefined value at /kohadevbox/koha/t/db_dependent/selenium/patrons_search.t line 164.
# Looks like your test exited with 255 just after 1.
/kohadevbox/koha/t/db_dependent/selenium/patrons_search.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/1 subtests
Test Summary Report-------------------
/kohadevbox/koha/t/db_dependent/selenium/patrons_search.t (Wstat: 65280 Tests: 1 Failed: 1)
Created attachment 151058 [details] [review] Bug 29822: Update 'DefaultPatronSearchFields' to a select list This patch updates the `DefaultPatronSearchFields` preference from freetext expecting a comma delimited list of borrower fields to a multi-select with a sensible list of fields available. I opted to not use the Modal approach with all borrower fields listed as I felt it made more sense to only expose a more limited subset of the fields available to us. Created attachment 151059 [details] [review] Bug 29822: Use the modal As requested by Katrin.. switch to the modal and expose all fields from the borrowers database table Created attachment 151060 [details] [review] Bug 29822: Update code to reference by pipe Moving to modalselect also has the effect of moving from comma delimited to pipe delimitation for the preference contents tmp Created attachment 151061 [details] [review] Bug 29822: Database and Preference description updates Created attachment 151062 [details] [review] Bug 29822: Update unit tests Rebased.. seems we lost a closing brace.. should be working now The tests are still failing for me. Created attachment 151064 [details] [review] Bug 29822: Escape backslash on split + effectively replace 'standard', don't display it. + avoid mixing | and , Created attachment 151233 [details] [review] Bug 29822: Update 'DefaultPatronSearchFields' to a select list This patch updates the `DefaultPatronSearchFields` preference from freetext expecting a comma delimited list of borrower fields to a multi-select with a sensible list of fields available. I opted to not use the Modal approach with all borrower fields listed as I felt it made more sense to only expose a more limited subset of the fields available to us. Signed-off-by: David Nind <david@davidnind.com> Created attachment 151234 [details] [review] Bug 29822: Use the modal As requested by Katrin.. switch to the modal and expose all fields from the borrowers database table Signed-off-by: David Nind <david@davidnind.com> Created attachment 151235 [details] [review] Bug 29822: Update code to reference by pipe Moving to modalselect also has the effect of moving from comma delimited to pipe delimitation for the preference contents tmp Signed-off-by: David Nind <david@davidnind.com> Created attachment 151236 [details] [review] Bug 29822: Database and Preference description updates Signed-off-by: David Nind <david@davidnind.com> Created attachment 151237 [details] [review] Bug 29822: Update unit tests Signed-off-by: David Nind <david@davidnind.com> Created attachment 151238 [details] [review] Bug 29822: Escape backslash on split + effectively replace 'standard', don't display it. + avoid mixing | and , Signed-off-by: David Nind <david@davidnind.com> Testing notes using KTD:
1. Check system preference DefaultPatronSearchFields and note that it is defined by entering a comma separated list.
2. Go to the patrons module and view the HTML source for the page:
. search the source for 'standard' - this should have two results
. note that the option value for the Standard input field are "firstname,middle_name,surname,othernames,cardnumber,userid"
3. Break the patron search:
. for the DefaultPatronSearchFields system preference, add a field that doesn't exist, for example: someunknownfield
. for patron search, enter any value in the search field and make sure standard is selected for the search field drop down list
. you get a pop-up error message:
"Something went wrong when loading the table.
500: Internal Server Error."
4. Apply the patch.
5. Update the database: databaseupdate
6. Restart everything: flushmemcached, restart_all
7. Reload or search again for the DefaultPatronSearchFields system preference.
8. Note that this is now a modal window with all the patron fields that are selectable shown (greyed out items are not selectable), rather than needing to enter a comma separated list.
9. Select some fields, such as 'First name', 'Surname', 'Primary email'.
10. Repeat step 2 - the standard input field now includes the fields you selected.
11. Make sure some patrons have a primary email address, such as firstname@gmail.com
12. Do a standard search for gmail.com
==> the results should include all the patrons you added a gmail address to
13. Do a specific search on the 'Primary email' field
==> the search results should be the same as for step 12
14. Run the tests, these should pass (note - KTD needs to be started with selenium: ktd --selenium up):
. prove t/db_dependent/selenium/patrons_search.t
BONUS: Perform the same search using the header search bar and the search options drop down (note: you need to change the 'Search type' from 'Starts with' to 'Contains').
Sorry Martin, this needs another rebase (bug 34092). Does not seem hard but not trivial. Bugger.. I thought this was part of that dependency tree already :(. Looking at the rebase now. Created attachment 153373 [details] [review] Bug 29822: Update 'DefaultPatronSearchFields' to a select list This patch updates the `DefaultPatronSearchFields` preference from freetext expecting a comma delimited list of borrower fields to a multi-select with a sensible list of fields available. I opted to not use the Modal approach with all borrower fields listed as I felt it made more sense to only expose a more limited subset of the fields available to us. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153374 [details] [review] Bug 29822: Use the modal As requested by Katrin.. switch to the modal and expose all fields from the borrowers database table Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153375 [details] [review] Bug 29822: Update code to reference by pipe Moving to modalselect also has the effect of moving from comma delimited to pipe delimitation for the preference contents tmp Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153376 [details] [review] Bug 29822: Database and Preference description updates Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153377 [details] [review] Bug 29822: Update unit tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153378 [details] [review] Bug 29822: Escape backslash on split + effectively replace 'standard', don't display it. + avoid mixing | and , Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> OK.. rebased.. but waiting for my ktd to startup so I can re-run the tests. Patches uploaded just in case someone is already available ;P Unit tests failing with rebase.. resetting to Failed whilst I work on that Created attachment 153379 [details] [review] Bug 29822: Update 'DefaultPatronSearchFields' to a select list This patch updates the `DefaultPatronSearchFields` preference from freetext expecting a comma delimited list of borrower fields to a multi-select with a sensible list of fields available. I opted to not use the Modal approach with all borrower fields listed as I felt it made more sense to only expose a more limited subset of the fields available to us. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153380 [details] [review] Bug 29822: Use the modal As requested by Katrin.. switch to the modal and expose all fields from the borrowers database table Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153381 [details] [review] Bug 29822: Update code to reference by pipe Moving to modalselect also has the effect of moving from comma delimited to pipe delimitation for the preference contents Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153382 [details] [review] Bug 29822: Database and Preference description updates Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153383 [details] [review] Bug 29822: Update unit tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153384 [details] [review] Bug 29822: Escape backslash on split + effectively replace 'standard', don't display it. + avoid mixing | and , Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153442 [details] [review] Bug 29822: Update 'DefaultPatronSearchFields' to a select list This patch updates the `DefaultPatronSearchFields` preference from freetext expecting a comma delimited list of borrower fields to a multi-select with a sensible list of fields available. I opted to not use the Modal approach with all borrower fields listed as I felt it made more sense to only expose a more limited subset of the fields available to us. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153443 [details] [review] Bug 29822: Use the modal As requested by Katrin.. switch to the modal and expose all fields from the borrowers database table Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153444 [details] [review] Bug 29822: Update code to reference by pipe Moving to modalselect also has the effect of moving from comma delimited to pipe delimitation for the preference contents Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153445 [details] [review] Bug 29822: Database and Preference description updates Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153446 [details] [review] Bug 29822: Update unit tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 153447 [details] [review] Bug 29822: Escape backslash on split + effectively replace 'standard', don't display it. + avoid mixing | and , Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> This and 33428 are going to clash, happy to rebase mine on top of this Would it be better to leave the fields from the pref as pipe, and just move the other fields 'full_address', 'all_emails' etc to use the pipe too? Then we don't have to replace in the one instance for splitting. I don't understand the comment: + effectively replace 'standard', don't display it. Created attachment 154361 [details] [review] Bug 29822: Update 'DefaultPatronSearchFields' to a select list This patch updates the `DefaultPatronSearchFields` preference from freetext expecting a comma delimited list of borrower fields to a multi-select with a sensible list of fields available. I opted to not use the Modal approach with all borrower fields listed as I felt it made more sense to only expose a more limited subset of the fields available to us. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 154362 [details] [review] Bug 29822: Use the modal As requested by Katrin.. switch to the modal and expose all fields from the borrowers database table Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 154363 [details] [review] Bug 29822: Update code to reference by pipe Moving to modalselect also has the effect of moving from comma delimited to pipe delimitation for the preference contents Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 154364 [details] [review] Bug 29822: Database and Preference description updates Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 154365 [details] [review] Bug 29822: Update unit tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 154366 [details] [review] Bug 29822: (QA follow-up) Use pipe for all field separators - Tidy atomic update Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Pushed to master for 23.11. Nice work everyone, thanks! Enhancement not pushed to 23.05.x Manual updated |