If a library chooses to enter their patron phone numbers in as xxx-xxx-xxxx when creating the patron record; then when the phone number is searched with the filters, the library must always use hyphens when searching or the search will not work. It would be nice that the number could be searched in the filters with either hyphens or no hyphens and the library would get the same results.
I've had a few libraries talk to me about this, and I imagine that there are many many libraries around the world who find this frustrating. I have a solution that normalizes the search term as well as the database field, so it will ignore everything aside from the numbers. So searching "1-(306)-123-4567" will match "1 306 123 4567", because they'll both be normalized to be "13061234567".
I wasn't sure how to handle searching with or without prefixes, but I think I'll add a wildcard on the front of the number so that "123-4567" will match "1-306-123-4567", or "306-123-4567" or "123-4567". I think that makes sense...
Normalizing to remove spaces, - and other non numeric characters would probably work for how we enter phone numbers here too, but I wonder if some libraries might enter multiple phone numbers? Then we could create a non-working search string. I can certainly imagine this happening, also thinking of migrations. Maybe we need to do a search both for the value entered and a normalized version?
(In reply to Katrin Fischer from comment #3) > Normalizing to remove spaces, - and other non numeric characters would > probably work for how we enter phone numbers here too, but I wonder if some > libraries might enter multiple phone numbers? Then we could create a > non-working search string. I can certainly imagine this happening, also > thinking of migrations. Maybe we need to do a search both for the value > entered and a normalized version? In my opinion, if they're putting multiple phone numbers into one field, they're using Koha wrong, and I don't think that we should modify Koha to support that behaviour.
If we are changing how search works, I think it's valid to think about edge cases. The field is not used for anything by Koha currently in most cases (phone notifications are not a thing at all around here). So there is nothing to stop people from entering different formats and often notes too.
(In reply to Katrin Fischer from comment #5) > If we are changing how search works, I think it's valid to think about edge > cases. The field is not used for anything by Koha currently in most cases > (phone notifications are not a thing at all around here). So there is > nothing to stop people from entering different formats and often notes too. Indeed it looks like we've only added validation to the "smsalertnumber" and not the other phone fields. But I'd say that's a bug rather than a feature. Something to be removed rather than accommodated. The current phone search is fundamentally broken. If you've stored the number as "0412 345 678", you'll never retrieve the record if you input "0412345678" or "(04)12 345 678" or any other format. I'll upload my patch, and then folk can decide what they want to do about it. But in my opinion, every telephone database field should only contain 1 value. If people need multiple phone numbers or notes, then they need a new feature with a denormalized contact table.
I never asked not to improve the search, just to think about ways we could make it flexible enough and maybe work for other cases too.
It looks like there's a new wrinkle in this one... I was patching an older Koha that still used C4::Utils::DataTables::Members, but in master it looks like we're using the REST API, which I suspect will be much harder to change. But let's see...
(In reply to David Cook from comment #8) > It looks like there's a new wrinkle in this one... > > I was patching an older Koha that still used C4::Utils::DataTables::Members, > but in master it looks like we're using the REST API, which I suspect will > be much harder to change. But let's see... I don't think that it's going to be possible to easily fix this with code in master. Using literal SQL, it's possible to craft a search using DBIx::Class to do the search[1], but we can't pass in that literal SQL via the API. We could potentially inject the literal SQL within the API backend but it would need to be in the Koha/REST/Plugin/Objects.pm helper... and it would involve recursively navigating the data (more than it already is...), and that would really just be a hack at that stage anyway. [1] my $test = $rs->search(\[ "regexp_replace(phone,?,?) LIKE ?",'[^0-9]','','%1234%']);
I think the only way forward is to normalize the data in the database (ie only store [\+0-9], and normalize the search query in "koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc" to [\+0-9] as well. It would still be necessary to update all the patron data in the database. We'd need something like "touch_all_patron_records.pl", or a once off SQL update to strip out everything that isn't [\+0-9]. regexp_replace(phone,'[^0-9]','') would be useful for that. This patch doesn't take into account Katrin's edge cases of multiple phone numbers or text comments in the "phone" field. There's no way to do that unless we completely change how DataTables work with the REST API. I've taken a look at a large patron database and over 99% of phone number data is just 1 phone number. Out of 100,000+ records, I see maybe 5 that have text comments in them. I see 2 records with 2 phone numbers in them. If they need to note the phone number belongs to a particular person or is text only, I think that needs to go in the "Contact note".
I do have one more idea that wouldn't involve changing the data...
(In reply to David Cook from comment #11) > I do have one more idea that wouldn't involve changing the data... There are some ways to provide additional columns to the DBIC query, but it auto-escapes single quotes, which makes it impossible to use regexp_replace as a derived column. Plus the DBIC search only works on real columns it seems (which is a bit silly...) -- I suppose another solution would be a pure Javascript solution that strips anything but [\+0-9] out of the phone number field when saving and searching... -- Another option would be a database trigger, but Koha has avoided those to date. -- I suppose another idea would be to add a new phone number column called "phone_normalized". That seems unnecessary though. -- Now I've spent quite a bit of time on this one, and I suspect it's going to go nowhere, unfortunately. There just isn't a good way to deal with this one using DBIC and the REST API I reckon...
Created attachment 140610 [details] [review] Bug 23817: Normalize phone number when storing and when searching in Patrons module This patch normalizes the phone number when stored via Koha::Patron, and normalizes the phone number when searching "Primary phone" in the Patrons module. Test plan: 0. Apply patch and koha-plack --restart kohadev 1. Go to http://localhost:8081/cgi-bin/koha/members/memberentry.pl?op=modify&destination=circ&borrowernumber=51 2. Input '1-(234)-567-8901' into the 'Primary phone' field 3. Note on the next screen the phone number is shown as 12345678901 4. Go to http://localhost:8081/cgi-bin/koha/members/members-home.pl 5. Choose "Search field" of "Primary phone" 6. Search for '1-(234)-567-8901' 7. Note you are taken to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51 8. Try out different permutations like '234 567 8901' or '5678901' 9. Note that every permutation works to find the borrower
I've tested this, and it works as per the test plan. While this works, and I probably don't appreciate the effort required to get searching the phone number to work -- I have some feedback. I'm not a fan of displaying the phone number as a number without any formatting, such as +, dashes, spaces, and brackets: - Usability: They are hard to read or verify by a human - They are displayed on transit slips (see note on the patron edit form), I think it makes it harder to read phone numbers if they are displayed as a string of numbers without formatting - It doesn't follow best practices for web forms Some ideas (but not how to code): 1. Validate the input of phone numbers, so they are entered without inputting +, dashes, spaces, brackets, etc - see this article: https://uxplanet.org/phone-number-field-design-best-practices-23957cbd86d5 2. Number stored as just a number without any formatting 3. Displayed for patron according to a format display setting (as an aside, this would work going forward, but not with existing numbers if they have been entered inconsistently) 4. As long as the number is stored without formatting, then any characters such as brackets, spaces, dashes, +, etc could be removed by JavaScript when inputting in the search box. I couldn't find an "authoritative" article about phone number and web form input, validation and display - but I didn't look very hard: - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/tel - Maybe there is a well supported JavaScript library for resolving this, such as https://github.com/google/libphonenumber I'm sure there is a trade-off between being able to search (how the number is stored), vs how the number is displayed. Maybe sorting out the input of phone numbers as a separate bug would then make solving the search problem easier....
Thanks for testing and the feedback, David :) One thing to note about the phone number input is that the web form isn't the only way that phone numbers are added. Patrons can be added via the "Import patrons" tool as well, so I think the ultimate phone number validation needs to be done on the backend. I think adding Javascript validation would be helpful from the UX point of view though. I was thinking a bit about formatting and we have had some discussion on the listserv (hopefully there will be more discussion there). Internationalization does make the phone number formatting tough. For instance, in Australia I think we have 4+ different phone number formats (although in practical terms only 2 would probably be used by the library) whereas back in Canada I think we only had 1 phone number format. -- Ideally, I would rather just normalize the data only when searching (like I'm doing in Koha 21.11), but the DataTables/REST API mechanism makes that seemingly impossible.
Created attachment 140648 [details] [review] Bug 23817: [Alternative patch] Normalize phone number only when searching This patch rewrites the phone number DBIC query when sent from DataTables like in the Patron module. Test plan: 0. Apply patch and koha-plack --restart kohadev 1. Go to http://localhost:8081/cgi-bin/koha/members/memberentry.pl?op=modify&destination=circ&borrowernumber=51 2. Input '1-(234)-567-8901' into the 'Primary phone' field 3. Note on the next screen the phone number is shown as '1-(234)-567-8901' 4. Go to http://localhost:8081/cgi-bin/koha/members/members-home.pl 5. Choose "Search field" of "Primary phone" 6. Search for '12345678901' 7. Note you are taken to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51 8. Try out different permutations like '234 567 8901' or '5678901' 9. Note that every permutation works to find the borrower
(In reply to David Cook from comment #15) > Ideally, I would rather just normalize the data only when searching (like > I'm doing in Koha 21.11), but the DataTables/REST API mechanism makes that > seemingly impossible. Maybe not impossible after all... just challenging haha.
Still looking for testers for this one.
Bug 23817 - Normalize phone number when searching patrons 140610 - Bug 23817: Normalize phone number when storing and when searching in Patrons module 140648 - Bug 23817: [Alternative patch] Normalize phone number only when searching Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 23817: Normalize phone number when storing and when searching in Patrons module Applying: Bug 23817: [Alternative patch] Normalize phone number only when searching Using index info to reconstruct a base tree... M Koha/REST/Plugin/Objects.pm M koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc Auto-merging Koha/REST/Plugin/Objects.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 23817: [Alternative patch] Normalize phone number only when searching hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-23817-Alternative-patch-Normalize-phone-number-wqS2eF.patch
Hi Solene, You can't apply both patches. The second one it marked as "alternative patch", so it's either/or. That said, perhaps I should just obsolete the first patch, since the first patch is destructive while the second is not...
The patch works well. But I cannot put it on the master right now. I get an error like this "Failed to attach patch to bug 23817, status=200" when I run the "attach" command. I tried with "git bz attach 23817 HEAD" and "git bz attach -e 23817 HEAD" but neither of them worked.
Created attachment 145219 [details] [review] Bug 23817: [Alternative patch] Normalize phone number only when searching This patch rewrites the phone number DBIC query when sent from DataTables like in the Patron module. Test plan: 0. Apply patch and koha-plack --restart kohadev 1. Go to http://localhost:8081/cgi-bin/koha/members/memberentry.pl?op=modify&destination=circ&borrowernumber=51 2. Input '1-(234)-567-8901' into the 'Primary phone' field 3. Note on the next screen the phone number is shown as '1-(234)-567-8901' 4. Go to http://localhost:8081/cgi-bin/koha/members/members-home.pl 5. Choose "Search field" of "Primary phone" 6. Search for '12345678901' 7. Note you are taken to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51 8. Try out different permutations like '234 567 8901' or '5678901' 9. Note that every permutation works to find the borrower Signed-off-by: Marius Mandrescu <marius.mandrescu@inLibro.com>
Created attachment 145221 [details] [review] Bug 23817: [Alternative patch] Normalize phone number only when searching This patch rewrites the phone number DBIC query when sent from DataTables like in the Patron module. Test plan: 0. Apply patch and koha-plack --restart kohadev 1. Go to http://localhost:8081/cgi-bin/koha/members/memberentry.pl?op=modify&destination=circ&borrowernumber=51 2. Input '1-(234)-567-8901' into the 'Primary phone' field 3. Note on the next screen the phone number is shown as '1-(234)-567-8901' 4. Go to http://localhost:8081/cgi-bin/koha/members/members-home.pl 5. Choose "Search field" of "Primary phone" 6. Search for '12345678901' 7. Note you are taken to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51 8. Try out different permutations like '234 567 8901' or '5678901' 9. Note that every permutation works to find the borrower Signed-off-by: Marius Mandrescu <marius.mandrescu@inLibro.com> Signed-off-by: Solene Ngamga <solene.ngamga@inLibro.com>
FAIL Koha/REST/V1/Patrons.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD is missing for 'preprocess_dbic_for_datatables'
Created attachment 151961 [details] [review] Bug 23817: Normalize phone number when searching This patch rewrites the phone number DBIC query when sent from DataTables like in the Patron module. Test plan: 0. Apply patch and koha-plack --restart kohadev 1. Go to http://localhost:8081/cgi-bin/koha/members/memberentry.pl?op=modify&destination=circ&borrowernumber=51 2. Input '1-(234)-567-8901' into the 'Primary phone' field 3. Note on the next screen the phone number is shown as '1-(234)-567-8901' 4. Go to http://localhost:8081/cgi-bin/koha/members/members-home.pl 5. Choose "Search field" of "All phones" 6. Search for '12345678901' 7. Note you are taken to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51 8. Try out different permutations like '234 567 8901' or '5678901' 9. Note that every permutation works to find the borrower
Created attachment 151962 [details] [review] Bug 23817: Include all phone numbers
Created attachment 152450 [details] [review] Bug 23817: Normalize phone number when searching This patch rewrites the phone number DBIC query when sent from DataTables like in the Patron module. Test plan: 0. Apply patch and koha-plack --restart kohadev 1. Go to http://localhost:8081/cgi-bin/koha/members/memberentry.pl?op=modify&destination=circ&borrowernumber=51 2. Input '1-(234)-567-8901' into the 'Primary phone' field 3. Note on the next screen the phone number is shown as '1-(234)-567-8901' 4. Go to http://localhost:8081/cgi-bin/koha/members/members-home.pl 5. Choose "Search field" of "All phones" 6. Search for '12345678901' 7. Note you are taken to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51 8. Try out different permutations like '234 567 8901' or '5678901' 9. Note that every permutation works to find the borrower Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Created attachment 152451 [details] [review] Bug 23817: Include all phone numbers Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Going to Fail QA my own patches... I really should include a unit test for "preprocess_dbic_for_datatables"
Failed QA (missing unit test) plus patch doesn't apply anymore, although it's not hard to fix the merge conflict