Bug 23817 - Normalize phone number when searching patrons
Summary: Normalize phone number when searching patrons
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-10-15 19:22 UTC by Kelly McElligott
Modified: 2024-02-12 00:21 UTC (History)
10 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 23817: Normalize phone number when storing and when searching in Patrons module (2.81 KB, patch)
2022-09-14 05:18 UTC, David Cook
Details | Diff | Splinter Review
Bug 23817: [Alternative patch] Normalize phone number only when searching (5.14 KB, patch)
2022-09-15 04:39 UTC, David Cook
Details | Diff | Splinter Review
Bug 23817: [Alternative patch] Normalize phone number only when searching (5.20 KB, patch)
2023-01-11 20:15 UTC, Marius
Details | Diff | Splinter Review
Bug 23817: [Alternative patch] Normalize phone number only when searching (5.26 KB, patch)
2023-01-11 20:50 UTC, solene.ngamga
Details | Diff | Splinter Review
Bug 23817: Normalize phone number when searching (5.18 KB, patch)
2023-06-02 03:03 UTC, David Cook
Details | Diff | Splinter Review
Bug 23817: Include all phone numbers (3.54 KB, patch)
2023-06-02 03:03 UTC, David Cook
Details | Diff | Splinter Review
Bug 23817: Normalize phone number when searching (5.26 KB, patch)
2023-06-16 18:43 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 23817: Include all phone numbers (3.61 KB, patch)
2023-06-16 18:43 UTC, Emily Lamancusa
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kelly McElligott 2019-10-15 19:22:23 UTC
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.
Comment 1 David Cook 2022-09-05 06:27:47 UTC
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".
Comment 2 David Cook 2022-09-05 06:30:41 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2022-09-07 09:43:25 UTC
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?
Comment 4 David Cook 2022-09-07 23:21:14 UTC
(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.
Comment 5 Katrin Fischer 2022-09-08 08:38:05 UTC
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.
Comment 6 David Cook 2022-09-08 23:38:26 UTC
(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.
Comment 7 Katrin Fischer 2022-09-10 13:53:59 UTC
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.
Comment 8 David Cook 2022-09-14 00:55:29 UTC
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...
Comment 9 David Cook 2022-09-14 03:16:27 UTC
(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%']);
Comment 10 David Cook 2022-09-14 04:15:45 UTC
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".
Comment 11 David Cook 2022-09-14 04:19:17 UTC
I do have one more idea that wouldn't involve changing the data...
Comment 12 David Cook 2022-09-14 05:18:36 UTC
(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...
Comment 13 David Cook 2022-09-14 05:18:57 UTC
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
Comment 14 David Nind 2022-09-14 19:21:12 UTC
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....
Comment 15 David Cook 2022-09-15 01:02:16 UTC
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.
Comment 16 David Cook 2022-09-15 04:39:30 UTC
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
Comment 17 David Cook 2022-09-15 04:40:02 UTC
(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.
Comment 18 David Cook 2022-11-15 05:07:14 UTC
Still looking for testers for this one.
Comment 19 solene.ngamga 2023-01-09 20:10:44 UTC
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
Comment 20 David Cook 2023-01-09 22:11:10 UTC
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...
Comment 21 solene.ngamga 2023-01-10 14:59:32 UTC
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.
Comment 22 Marius 2023-01-11 20:15:31 UTC
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>
Comment 23 solene.ngamga 2023-01-11 20:50:08 UTC
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>
Comment 24 Marcel de Rooy 2023-02-24 10:55:40 UTC
 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'
Comment 25 David Cook 2023-06-02 03:03:46 UTC
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
Comment 26 David Cook 2023-06-02 03:03:50 UTC
Created attachment 151962 [details] [review]
Bug 23817: Include all phone numbers
Comment 27 Emily Lamancusa 2023-06-16 18:43:49 UTC
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>
Comment 28 Emily Lamancusa 2023-06-16 18:43:52 UTC
Created attachment 152451 [details] [review]
Bug 23817: Include all phone numbers

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 29 David Cook 2023-06-19 01:54:58 UTC
Going to Fail QA my own patches...

I really should include a unit test for "preprocess_dbic_for_datatables"
Comment 30 David Cook 2024-02-12 00:21:58 UTC
Failed QA (missing unit test) plus patch doesn't apply anymore, although it's not hard to fix the merge conflict