[FIX] phone_validation: brazilian phone numbers #153282
Conversation
There was a problem hiding this comment.
looks good to me if the number whatsapp send us is lke +55 11 6123-4567 👍
intl_number_format exists in the region_BR.py file of phonenumbers since june 2015 (from this commit) which corresponds to version 7.0.6 and ubuntu of 2016 or end of 2015, so this seems safe.
I tried installing 7.0.5 to test what error we would get if we had an old version, but with python 3.6 and 3.11 I can't install it because of an issue in a regex:
File "/tmp/pip-install-bwcr44br/phonenumbers_2576ab7f89c349cd98849c55abbcc91a/phonenumbers/phonenumbermatcher.py", line 120, in <module>
_PATTERN = re.compile(u("(?:") + _LEAD_CLASS + _PUNCTUATION + u(")") + _LEAD_LIMIT +
...
File "/usr/lib/python3.11/re/_parser.py", line 843, in _parse
raise source.error('global flags not at the start '
re.error: global flags not at the start of the expression at position 57So this seems safe like this and good to keep like thi (if the version with the problem is not possible to install), but otherwise we could check the version or have a try:; except AttributeError: pass
|
As we merged whatsapp in 16.3, shouldn't we fix in 16.3 ? It is a request coming from whatsapp users mainly. |
ande-odoo
force-pushed
the
17.0-OPW_3694150-phone_validation-brazil_phone_numbers-ande
branch
from
d8db73e
to
91ef058
Compare
There was a problem hiding this comment.
Comment + I wonder if we really need to patch the whole library for that.
If it's super safe and we're confident in this then good, but otherwise it might be worth considering solving this on the side of whatsapp where the format is guaranteed and we don't need to play with regex. As this format is not valid today anyway, and the reason we're getting it is because whatsapp is stupid and gives us the "number id" where we have to infer the real number.
That regex is a bit scary is all ^^
|
Hi @odoo/rd-sm-phishing Could I get a review ? Thanks ! |
There was a problem hiding this comment.
As patterns are matched in order of the array anyway it should be fairly safe so seems ok to go with this indeed.
I think it could be good for the commit message to link towards the file of the lib and wikipedia article. And specify that the pattern will only apply if none of the other formats in the lib match, which was a point of confusion for me.
ok for me code-wise, adding [6-9] or [68] could be good for accuracy/clarity
Thanks for the thorough explanation :)
ande-odoo
force-pushed
the
17.0-OPW_3694150-phone_validation-brazil_phone_numbers-ande
branch
from
91ef058
to
22c30b4
Compare
|
Hi @reth-odoo I have added the 6 8 9 filter for mobile numbers (7 is already filtered in the previous regex) |
There was a problem hiding this comment.
Seems legit to force fix invalid numbers then 👌
(pinging @tde-banana-odoo again as he started the review + is much better versed in phone stuff)
|
@reth-odoo But I hate regexes. |
Current behavior: Brazilian phone numbers are not managed correctly following the 2016 changes in Brazil. (Adding a 9 to mobile phone numbers) Fix: Patched the phonenumbers library, adding a 9 at the right place for mobile phone numbers. opw-3694150
ande-odoo
force-pushed
the
17.0-OPW_3694150-phone_validation-brazil_phone_numbers-ande
branch
from
22c30b4
to
dce570f
Compare
|
@robodoo r+ |
Current behavior: Brazilian phone numbers are not managed correctly following the 2016 changes in Brazil. (Adding a 9 to mobile phone numbers) Fix: Patched the phonenumbers library, adding a 9 at the right place for mobile phone numbers. opw-3694150 closes #153282 Signed-off-by: Thibault Delavallee (tde) <tde@openerp.com>
fw-bot
deleted the
17.0-OPW_3694150-phone_validation-brazil_phone_numbers-ande
branch
Current behavior:
Brazilian phone numbers are not managed correctly
following the 2016 changes in Brazil.
(Adding a 9 to mobile phone numbers)
Fix:
Patched the phonenumbers library, adding a 9
at the right place for mobile phone numbers.
opw-3694150
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr