Details
-
Type:
Bug
-
Status: Done/Fixed
-
Priority:
Minor
-
Resolution: Fixed/Completed
-
Affects Version/s: 4.3.0
-
Fix Version/s: Unscheduled
-
Component/s: None
-
Labels:None
-
Documentation Required?:None
-
Funding Source:Needs Funding
Description
Problem: I had a number in my database stored as 07817 802299. I sent this to clickatell. Clickatell adds a default international code (based on my clickatell settings). When it came back to CiviCRM, we couldn't match it back up with the contact because we weren't clever enough to work out that 07817 802299 is the same as 447817802299.
I added code to CRM_SMS_Provider::inbound() to perform an extra search for the contact before deciding to add a new contact. The extra search basically checks to see if the number that this is coming from is in the same country as the default country. If it is, it removes the country code so that it can try and match with any phone numbers in the DB that are in national format.
A couple of other improvements: it doesn't match for deleted contacts and it uses the new phone_numeric field rather than the old phone field.
Since we need to know phone number country codes for all countries to do this, these need to be added to the upgrade script.
Attachments
Issue Links
- supplements
-
CRM-11292 Format and validate phone numbers
-
- Done/Fixed
-
Activity
Michael - your sql patch looks good. Do you need any help adding it to the 4.3 upgrade script?
Have had an initial play around with https://github.com/davideme/libphonenumber-for-PHP and not sure that it is ready for prime time or whether it is well maintained enough. See for example
https://github.com/davideme/libphonenumber-for-PHP/issues/16 and
https://github.com/davideme/libphonenumber-for-PHP/pull/15
which we experienced after about 1 hour of playing around.
Someone did reply and suggested a solution for the bug that I reported which seems like it will work so it in general it looks like there are people suggesting solutions for those bugs but it may be a too high maintenance package to include (not sure of what bars we set for including packages). My gut feeling is that if we start using the PHP version of this package, we might cause more problems for people that don't use SMS that we solve for people that do use SMS (though obviously that depends on how invasively we implement it).
I added country codes UPDATE statements to the upgrade script and replaced country code INSERT statements in install SQL (xml/templates/civicrm_country.tpl)
That's a bit disappointing about the php version. My guess is the JS version is much more stable since it's maintained by Google.
Doing it all in JS would mostly work, but of course doing client-side data validation/normailzation is never 100% reliable. My thought was essentially to have the clientside code do most of the work, and have the php version as a backup to catch stuff from, say, front-end profiles where the end-user has JS disabled/broken. But maybe there's another approach here that I haven't thought of.
related PR: https://github.com/civicrm/civicrm-core/pull/684
Michael it's been a while but this issue is still open. What would you like to do with it?
Assigning to John since this was done under a contract to Future First and I'm not sure of the current status (has been a while), but John will know better.
Sent email to John today. If no response or they're not able to get to a PR, we'll move this to Future Version.
Literally all that we have is a string replace from "+44" to "". File attached. A better way would be a look up against a list of country codes - does that already exist in Civi?
Turns out I can't attach files!
John - I've updated your Jira user so you should be able to attach files.
As far as I can tell, there's been no progress on resolving this in a general way since April 13 (sounds like you've resolved it for your needs with a specific hack). Missing pieces seem to be:
- loading civicrm_country with accurate country codes - for new installs and upgrades (which has not happened yet at least in core distributions)
- modify processInbound() to conditionally remove default country code
I'm moving this to Future Version for now - but if you have resources to move this forward we can pull it back in.
Future First's Provider.php attached
Unassigning myself from the ticket as we don't have the resources to address this right now - sorry. We are contributing to CiviCRM's SMS system in other areas shortly.
I think we can close this now that there is a phone validator extension available.
Note there is also a client-side version of this library. Ideally we should use both the js and php versions.