entrouvert/wcs
entrouvert
/
wcs
14
1
Fork 0
Code Issues Pull Requests 34 Releases Activity

misc: rely on phonenumbers library for is_french_mobile_phone_number (#72793) #1037

Open
fpeters wants to merge 1 commits from wip/72793-french-mobile-number-templatetag into main
pull from: wip/72793-french-mobile-number-templatetag
merge into: entrouvert:main
Conversation 6 Commits 1 Files Changed 3 +65 -51
fpeters commented 2024-01-16 19:21:22 +01:00
Owner
Copy Link
No description provided.
fpeters added 1 commit 2024-01-16 19:21:22 +01:00
gitea/wcs/pipeline/head There was a failure building this commit Details
cf8e6a5b5c misc: rely on phonenumbers library for is_french_mobile_phone_number (#72793)
fpeters changed title from WIP: misc: rely on phonenumbers library for is_french_mobile_phone_number (#72793) to misc: rely on phonenumbers library for is_french_mobile_phone_number (#72793) 2024-01-18 18:11:46 +01:00
tnoel requested changes 2024-01-19 12:02:58 +01:00
Dismissed
tnoel left a comment
Owner
Copy Link

A priori on pourrait donc faire évoluer test_is_french_mobile_phone_number pour tester l'usage de numéros mobiles avec numérotation internationale, genre +33 6 39 98 42 42 et +262 6 92 42 42 42 qui doivent être ok, alors qu'un +61 442 424242 (mobile australie) ne devrait toujours pas passer.

A priori on pourrait donc faire évoluer test_is_french_mobile_phone_number pour tester l'usage de numéros mobiles avec numérotation internationale, genre +33 6 39 98 42 42 et +262 6 92 42 42 42 qui doivent être ok, alors qu'un +61 442 424242 (mobile australie) ne devrait toujours pas passer.
fpeters commented 2024-01-19 13:14:23 +01:00
Author
Owner
Copy Link

A priori on pourrait donc faire évoluer test_is_french_mobile_phone_number

Peut-être mais je serais très frileux, en imaginant trop facilement que soudainement des numéros internationaux vont être envoyés à une application métier qui ne s'y attend pas; et je préfère donc rester prudent ici.

> A priori on pourrait donc faire évoluer test_is_french_mobile_phone_number Peut-être mais je serais très frileux, en imaginant trop facilement que soudainement des numéros internationaux vont être envoyés à une application métier qui ne s'y attend pas; et je préfère donc rester prudent ici.
fpeters requested review from tnoel 2024-01-19 13:14:26 +01:00
tnoel commented 2024-02-25 01:41:32 +01:00
Owner
Copy Link

A priori on pourrait donc faire évoluer test_is_french_mobile_phone_number

Peut-être mais je serais très frileux, en imaginant trop facilement que soudainement des numéros internationaux vont être envoyés à une application métier qui ne s'y attend pas; et je préfère donc rester prudent ici.

J'avais juste mal lu ce patch, qui n'autorise effectivement pas la numérotation internationale, et c'est ok.

> > A priori on pourrait donc faire évoluer test_is_french_mobile_phone_number > > Peut-être mais je serais très frileux, en imaginant trop facilement que soudainement des numéros internationaux vont être envoyés à une application métier qui ne s'y attend pas; et je préfère donc rester prudent ici. J'avais juste mal lu ce patch, qui n'autorise effectivement pas la numérotation internationale, et c'est ok.
tnoel closed this pull request 2024-02-25 01:41:33 +01:00
tnoel approved these changes 2024-02-25 01:41:46 +01:00
Dismissed
tnoel commented 2024-02-25 01:43:38 +01:00
Owner
Copy Link

Désolé j'ai cliqué sur "commenter et fermer" lors de mon commentaire, pensant clore juste la discussion et non pas la proposition dans son ensemble... et je ne sais plus comment on ré-ouvre...

En tout cas c'est validé.

Désolé j'ai cliqué sur "commenter et fermer" lors de mon commentaire, pensant clore juste la discussion et non pas la proposition dans son ensemble... et je ne sais plus comment on ré-ouvre... En tout cas c'est validé.
tnoel commented 2024-02-25 01:43:48 +01:00
Owner
Copy Link

Ah je viens de voir le bouton "Réouvrir".

Ah je viens de voir le bouton "Réouvrir".
tnoel reopened this pull request 2024-02-25 01:43:49 +01:00
fpeters force-pushed wip/72793-french-mobile-number-templatetag from cf8e6a5b5c to fef1563902 2024-02-25 18:52:00 +01:00 Compare
fpeters commented 2024-02-25 19:03:03 +01:00
Author
Owner
Copy Link

En tout cas c'est validé.

Il y avait un mini-rebase à faire mais avec #87044 on a récemment vu que l'utilisation de region_code "FR" pour parser les numéros était insuffisante, j'ai donc finalement tout repris pour toujours passer dans le même code :

  • get_valid_phone_number(string_value, region_codes, country_codes) : méthode qui retourne un objet phonenumber correspond à un numéro valide des codes régions et pays mentionnés, (et None s'il n'y a pas de numéro valide)
  • get_french_country_and_region_codes() qui fournit la liste des codes régions/pays pour la France
  • validate_phone_fr pour la validation de numéro français,
  • validate_mobile_phone_local pour la validation comme quoi le numéro est bien un mobile dans la région configurée (en site-settings) et/ou la région passée en paramètre
  • get_formatted_phone pour mettre en forme locale un numéro valide (ou retourner la chaine reçue si pas valide)
  • normalize_phone_number_for_fts pour mettre en forme internationale un numéro valide (ou retourner la chaine reçue si pas valide)
  • le template tag |is_french_mobile_phone_number qui appelle donc désormais validate_mobile_phone_local() en forçant le code région "FR".

(glossaire : code région, c'est BE pour la Belgique, RE pour la Réunion, etc. code pays c'est l'indicatif, 32 pour la Belgique, 262 pour la Réunion, etc. + on continue à traiter spécialement le code région "FR" pour dire "la France métropolitaine mais également la Réunion, la Martinique, etc.").

Avec ça je pense que le tour aura été fait et que #73948 pourra être fermé.

> En tout cas c'est validé. Il y avait un mini-rebase à faire mais avec #87044 on a récemment vu que l'utilisation de region_code "FR" pour parser les numéros était insuffisante, j'ai donc finalement tout repris pour toujours passer dans le même code : * get_valid_phone_number(string_value, region_codes, country_codes) : méthode qui retourne un objet phonenumber correspond à un numéro valide des codes régions et pays mentionnés, (et None s'il n'y a pas de numéro valide) * get_french_country_and_region_codes() qui fournit la liste des codes régions/pays pour la France * validate_phone_fr pour la validation de numéro français, * validate_mobile_phone_local pour la validation comme quoi le numéro est bien un mobile dans la région configurée (en site-settings) et/ou la région passée en paramètre * get_formatted_phone pour mettre en forme locale un numéro valide (ou retourner la chaine reçue si pas valide) * normalize_phone_number_for_fts pour mettre en forme internationale un numéro valide (ou retourner la chaine reçue si pas valide) + le template tag |is_french_mobile_phone_number qui appelle donc désormais validate_mobile_phone_local() en forçant le code région "FR". (glossaire : code région, c'est BE pour la Belgique, RE pour la Réunion, etc. code pays c'est l'indicatif, 32 pour la Belgique, 262 pour la Réunion, etc. + on continue à traiter spécialement le code région "FR" pour dire "la France métropolitaine mais également la Réunion, la Martinique, etc."). Avec ça je pense que le tour aura été fait et que #73948 pourra être fermé.
fpeters changed title from misc: rely on phonenumbers library for is_french_mobile_phone_number (#72793) to WIP: misc: rely on phonenumbers library for is_french_mobile_phone_number (#72793) 2024-02-25 19:03:10 +01:00
fpeters dismissed tnoel’s review 2024-02-25 19:03:17 +01:00
Reason:

va falloir tout relire

fpeters force-pushed wip/72793-french-mobile-number-templatetag from fef1563902 to 3cf3fda47f 2024-02-25 19:04:15 +01:00 Compare
fpeters force-pushed wip/72793-french-mobile-number-templatetag from 3cf3fda47f to b7fdff0146 2024-02-26 10:11:52 +01:00 Compare
fpeters force-pushed wip/72793-french-mobile-number-templatetag from b7fdff0146 to 53b7e0d3cc 2024-02-26 10:52:06 +01:00 Compare
fpeters changed title from WIP: misc: rely on phonenumbers library for is_french_mobile_phone_number (#72793) to misc: rely on phonenumbers library for is_french_mobile_phone_number (#72793) 2024-02-26 11:31:37 +01:00
tnoel requested changes 2024-04-30 11:47:15 +02:00
tnoel left a comment
Owner
Copy Link

On se retrouve ici avec un filtre "is_french_mobile_phone_number" qui va changer de comportement et accepter les numéros avec indicatif.

Ça part d'une bonne intention et c'est peut-être mieux ainsi, mais il faudra dire (dans les notes de mises à jour et dans la doc) et ajouter des tests, genre:

--- a/tests/test_templates.py
+++ b/tests/test_templates.py
@@ -864,6 +864,9 @@ def test_is_french_mobile_phone_number(pub):
     assert t.render({'number': '06.23.45.67.89'}) == 'True'
     assert t.render({'number': '0 6 2 3 45 67 89'}) == 'True'
 
+    assert t.render({'number': '+33 6 23 45 67 89'}) == 'True'
+    assert t.render({'number': '+262 6 92 11 22 33'}) == 'True'

Je clique sur "demander des changements" pour mettre l'accent sur ça...

On se retrouve ici avec un filtre "is_french_mobile_phone_number" qui va changer de comportement et accepter les numéros avec indicatif. Ça part d'une bonne intention et c'est peut-être mieux ainsi, mais il faudra dire (dans les notes de mises à jour et dans la doc) et ajouter des tests, genre: ``` --- a/tests/test_templates.py +++ b/tests/test_templates.py @@ -864,6 +864,9 @@ def test_is_french_mobile_phone_number(pub): assert t.render({'number': '06.23.45.67.89'}) == 'True' assert t.render({'number': '0 6 2 3 45 67 89'}) == 'True' + assert t.render({'number': '+33 6 23 45 67 89'}) == 'True' + assert t.render({'number': '+262 6 92 11 22 33'}) == 'True' ``` Je clique sur "demander des changements" pour mettre l'accent sur ça...
wcs/qommon/misc.py Outdated
@ -854,0 +852,4 @@
def get_valid_phone_number(string_value, region_codes=None, country_codes=None):
# get string_value as a valid phonenumber in default or specified region_codes,
# with additional check against country_codes if given.
if not re.match(r'^[0\+][\(\)\d\.\s]+$', string_value or ''):
tnoel commented 2024-04-30 10:34:36 +02:00
Owner
Copy Link

Dans les commentaires, quelque part, ici ou ailleurs, je verrais bien une copie de ce que tu as écrit à côté, parce que c'est utile de se le rappeler dans ce fratas de code:

glossaire : code région, c'est BE pour la Belgique, RE pour la Réunion, etc. code pays c'est l'indicatif, 32 pour la Belgique, 262 pour la Réunion, etc.

Dans les commentaires, quelque part, ici ou ailleurs, je verrais bien une copie de ce que tu as écrit à côté, parce que c'est utile de se le rappeler dans ce fratas de code: > glossaire : code région, c'est BE pour la Belgique, RE pour la Réunion, etc. code pays c'est l'indicatif, 32 pour la Belgique, 262 pour la Réunion, etc.
wcs/qommon/misc.py Outdated
@ -878,2 +874,2 @@
region_code = get_publisher().get_phone_local_region_code()
country_codes = []
def get_french_country_and_region_codes():
country_codes = [33, 262, 508, 590, 594, 596]
tnoel commented 2024-04-30 10:55:59 +02:00
Owner
Copy Link

Pour éviter de rechercher l'info, je proposerais ici de rappeler la signification de ces 6 indicatifs:

country_codes = [
   # France has 6 country codes in E.164 system :
    33,  # Metropolitan France (FR)
    262,  #  Réunion (RE)
    508,  #  Saint-Pierre-et-Miquelon (PM)
    590,  #  Guadeloupe, Saint-Barthélemy, Saint-Martin (GP)
    594,  #  Guyanne (GF)
    596,  #  Martinique (MQ)
]

éventuellement pour rappeler/sous-entendre que ça ne gère pas Nouvelle-Calédonie, Wallis-et-Futuna, Polynésie française et autres endroits qui ne sont pas partie du "plan de numérotation français".

Pour éviter de rechercher l'info, je proposerais ici de rappeler la signification de ces 6 indicatifs: ``` country_codes = [ # France has 6 country codes in E.164 system : 33, # Metropolitan France (FR) 262, # Réunion (RE) 508, # Saint-Pierre-et-Miquelon (PM) 590, # Guadeloupe, Saint-Barthélemy, Saint-Martin (GP) 594, # Guyanne (GF) 596, # Martinique (MQ) ] ``` éventuellement pour rappeler/sous-entendre que ça ne gère pas Nouvelle-Calédonie, Wallis-et-Futuna, Polynésie française et autres endroits qui ne sont pas partie du "plan de numérotation français".
wcs/qommon/templatetags/qommon.py Outdated
@ -1049,3 +1048,1 @@
return False
return value.startswith('06') or value.startswith('07')
return validate_mobile_phone_local(value, region_code='FR')
tnoel commented 2024-04-30 11:43:23 +02:00
Owner
Copy Link

Ici on se retrouve avec une fonction qui accepte également les numéros avec un indicatif, comme +33 6 23 45 67 89 ou +262 6 92 11 22 33, ce qui n'est pas le cas actuellement.

Soit on le précise (et donc aussi ajouter des tests qui le montre) soit on laisse le

  return value.startswith('06') or value.startswith('07')

final ?

Ici on se retrouve avec une fonction qui accepte également les numéros avec un indicatif, comme +33 6 23 45 67 89 ou +262 6 92 11 22 33, ce qui n'est pas le cas actuellement. Soit on le précise (et donc aussi ajouter des tests qui le montre) soit on laisse le ``` return value.startswith('06') or value.startswith('07') ``` final ?
fpeters added 1 commit 2024-04-30 15:21:27 +02:00
gitea/wcs/pipeline/head There was a failure building this commit Details
7621dd70e5 fixup
fpeters force-pushed wip/72793-french-mobile-number-templatetag from 7621dd70e5 to 30c1870533 2024-04-30 15:21:46 +02:00 Compare
fpeters commented 2024-04-30 15:22:07 +02:00
Author
Owner
Copy Link

Ok, le commit avec les modifications demandées : 7621dd70e5

Ok, le commit avec les modifications demandées : https://git.entrouvert.org/entrouvert/wcs/commit/7621dd70e5a1cbd684dafa5634dec10b2dc56cb0
fpeters requested review from tnoel 2024-04-30 15:32:09 +02:00
All checks were successful
gitea/wcs/pipeline/head This commit looks good
Details
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
Sign in to join this conversation.
Reviewers
No reviewers
tnoel
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/wcs#1037
No description provided.
Delete Branch "wip/72793-french-mobile-number-templatetag"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?