Parcours de modification du numéro de téléphone identifiant (#72614) #80
Loading…
Reference in New Issue
No description provided.
Delete Branch "wip/72614-phone-identifier-modification-ui"
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?
ff09052ecf
to85b4fb82eb
85b4fb82eb
tocd51d3bf5a
cd51d3bf5a
to21ba03b241
21ba03b241
to62700c9a72
62700c9a72
to898f7c2e2b
898f7c2e2b
tob02069a1e5
b02069a1e5
to0b8e9c0adf
0b8e9c0adf
to67227f73a6
67227f73a6
to8521f2a670
8521f2a670
tof73acb76cd
f73acb76cd
to2e5fb0f4dc
2e5fb0f4dc
to7c4b388e60
7c4b388e60
to2488ab182b
2488ab182b
to011f10975e
011f10975e
toe48565ba9a
e48565ba9a
tod483fe0615
d483fe0615
tofc81bc03ae
fc81bc03ae
to3b2d08085e
3b2d08085e
toa9181beff2
a9181beff2
to8d83007668
8d83007668
to4cd4e8cd99
4cd4e8cd99
to592739e2b0
592739e2b0
toc61a473183
c61a473183
to59326fab88
e384b87ed0
to957a7789eb
957a7789eb
toc736890a6e
c736890a6e
to8b3405f1a6
8b3405f1a6
to1ee4cb64bf
1ee4cb64bf
to21ade8096a
5dde1308e6
toa01e08fd24
a01e08fd24
to4d4b9f0b69
4d4b9f0b69
to833bc0267a
WIP: Parcours de modification du numéro de téléphone identifiant (#72614)to Parcours de modification du numéro de téléphone identifiant (#72614)Pas encore testé pour de vrai, mais déjà quelques remarques de forme.
@ -57,0 +71,4 @@
def clean_phone(self):
phone = self.cleaned_data['phone']
if (
qs := models.AttributeValue.objects.with_owner(self.user).filter(
Le fait que le queryset soit dans une condition fait qu'il va être évalué entièrement une première fois, puis une deuxième fois partiellement au moment du first. Ça pourrait être réécrit pour éviter la première requête.
Oui ma faute, j’avais la certitude que
.first()
tapait une erreur si la queryset était vide, j’aurais dû relire la doc :)Plus simple encore, puisque la méthode
.to_python()
est en fait la fonction identité pour les valeurs d’attribut de type numéro de téléphone, on peut se contenter de remonter la colonnecontent
de la première entrée.C’est modifié dans la nouvelle version de la PR.
@ -57,0 +72,4 @@
phone = self.cleaned_data['phone']
if (
qs := models.AttributeValue.objects.with_owner(self.user).filter(
attribute=utils_misc.get_password_authenticator().phone_identifier_field
Idéalement comme l'authentificateur est récupéré dans la vue, il faudrait le passer au formulaire et ainsi économiser une requête (il y a déjà plusieurs couples vues/formulaires qui font ça).
Ok, fair enough, j’ai ajouté le
get_form_kwargs
correspondant dans la vue, et l’initialisateur qui va avec c@ -57,0 +89,4 @@
def save(self):
"""
Generates eithera code sent by SMS which the user needs to input in order to confirm phone change.
Typo eithera et en fait même pas besoin de either puisque une seule option.
Mais ce commentaire est nécessaire parce qu'on détourne complètement la méthode save(), pour moi ce code devrait aller dans le form_valid de la vue (je dis ça sans réfléchir aux obstacles éventuels).
(d'ailleurs on est même pas sur un ModelForm, ce qui ajoute à l'étrangeté)
Oui tu as raison, c’est complètement dans `form_valid
@ -119,6 +120,8 @@ class EditProfile(HomeURLMixin, cbv.HookMixin, cbv.TemplateNamesMixin, UpdateVie
if attribute.user_editable:
editable_profile_fields.append(field)
attributes = models.Attribute.objects.filter(user_editable=True)
if (authn := utils_misc.get_password_authenticator()) and authn.phone_identifier_field:
L'authentificateur existe forcément, je trouve ça étrange de le mettre dans une condition.
Ok, je l’ai sorti de la condition.
La correction a atterri dans le second commit au lieu du premier
@ -255,0 +253,4 @@
template_names = ['profiles/email_change.html', 'authentic2/change_email.html']
title = _('Email Change')
success_url = '..'
reauthn_message = (_('You must re-authenticate to change your email address.'),)
Vraiment besoin d'un tuple ?
Non :D
C’est modifié.
@ -278,0 +320,4 @@
def form_valid(self, form):
phone = form.cleaned_data.get('phone')
# TODO handle multiple SMS warning message
Je suis plutôt pour ouvrir dors et déjà un ticket à traiter ensuite, plutôt qu'on TODO qui restera à jamais :)
Ok, #79686.
@ -278,0 +368,4 @@
return utils_misc.redirect(self.request, reverse('auth_homepage'))
old_phone = ''
if (
qs := models.AttributeValue.objects.with_owner(self.request.user).filter(
Même remarque que plus haut sur la double évaluation.
Ok, pour ne pas perdre la face j’essayais de trouver le bout de doc django qui indiquait dans ce cas précis le queryset ne serait évalué qu’une seule fois, en vain :)
(Et en mettant un
log_statement = 'all'
dans sa conf locale postgres et en inspectant les logs, on a l’impression qu’effectivement la requête n’apparaît qu’une fois dans la transaction, mais bref je n’y mettrais pas ma main à couper et retiens la remarque. D’ailleurs je préfère la nouvelle version qui ne remonte qu’une colonne !)@ -278,0 +392,4 @@
@atomic(savepoint=False)
def get(self, request, *args, **kwargs):
token = kwargs['token'].replace(' ', '')
if token:
Cosmétique, je verrais plutôt un if not token: return, ça duplique le return mais ça économise beaucoup d'indentations.
Ok, c’est plus lisible ainsi en effet.
@ -1379,0 +1584,4 @@
duration=120,
)
except Lock.Error:
request = StoreRequestMiddleware.get_request()
On est dans une vue, on a accès à
self.request
non ?Oui complètement, c’est modifié, merci.
647c748fa2
toef3ab62445
4eb0ffad3a
to4212900b5f
Voilà c’est tout pris en compte :)
Pour tester en local j'ai créé un nouvel attribut dans hobo, ajouté dans l'authentificateur mot de passe, je me crée un compte par numéro de tel, et cet attribut apparaît dans le formulaire de modification du profil alors qu'il ne devrait pas.
Techniquement c'est parce qu'il s'est retrouvé automatiquement dans app_settings.A2_PROFILE_FIELDS, je ne sais pas par quelle magie et je n'ai pas le temps de creuser plus maintenant.
Ok, merci pour ta vigilance, sans doute des modifications nécessaires côté hobo agent authentic. Je regarde.
aee93614ed
tofbf6f15952
Voilà, c’est adapté ici dans 0001, où on fait finalement la même chose que pour le courriel, à savoir :
· on affiche le numéro de téléphone sur la page principale ;
· mais dans la vue d’édition on retire au champ son formulaire associé, puisque ce champ dispose de son propre parcours de modification.
4182f0a7ef
to7dff41ebc1
Parcours de modification du numéro de téléphone identifiant (#72614)to Parcours de modification du numéro de téléphone identifiant (#72614)Désolé encore un train de suggestions de simplification et questionnements divers. Là encore je ne peux pas garantir que ce sera le dernier car 1/ plus j'ajoute de commentaires plus la page lagge et devient inutilisable 2/ mon firefox s'amuse à tuer certains onglets selon des critères flous et j'ai peur de perdre mes remarques, bref j'envoie en l'état.
Et remarque pas faisable dans l'interface, dans le premier commit il y a
qui serait à déplacer dans le commit d'après.
@ -57,0 +76,4 @@
.filter(attribute=self.authenticator.phone_identifier_field)
.values_list('content', flat=True)
)
if atvs and phone == atvs[0]:
Peut-être plus simple et j'espère équivalent,
Oui, c’est mieux comme ça en effet. C’est corrigé, merci.
@ -232,0 +232,4 @@
if not self.can_validate_with_password() and not self.has_recent_authentication():
return self.reauthenticate(
action='email-change',
message=self.reauthn_message,
Le message change mais l'action reste nommée email-change, peut-être juste ne pas chercher à mutualiser ce dispatch ?
J’ai conservé le dispatch unique mais avec une action qui varie.
@ -276,2 +286,4 @@
class PhoneChangeView(HomeURLMixin, IdentifierChangeMixin, cbv.TemplateNamesMixin, FormView):
template_names = ['profiles/phone_change.html', 'authentic2/change_phone.html']
Ça me semble être une très vieille histoire de définir plusieurs templates dont un qui n'existe pas, à mon avis tu peux faire sans.
Oui, ok, on va faire sans. C’est corrigé.
@ -278,0 +309,4 @@
content__isnull=False,
)
if atvs:
ctx['phone'] = atvs.first().to_python()
Ici même remarque que #80 (comment), il me semble que ça provoque deux requêtes
C’est corrigé dans la branche, merci.
Il me semble que cette ligne est devenue
context.update({'phone': atvs.first().to_python()})
donc le problème demeure (il faudrait faire atvs[0] comme ailleurs dans le patch)Arf j’avais loupé cela, c’est corrigé, merci.
@ -278,0 +312,4 @@
ctx['phone'] = atvs.first().to_python()
ctx['allow_phone_change'] = getattr(
self.authenticator.phone_identifier_field, 'user_editable', False
) and not getattr(self.authenticator.phone_identifier_field, 'disabled', True)
Même remarque que https://git.entrouvert.org/entrouvert/authentic/pulls/80/files#issuecomment-14527 (mais ici je dirais même faire 403 ou 404 en amont si pas de phone_identifier_field, puisque le lien vers cette vue n'est pas affiché dans ce cas).
Ok, ça me va de taper une 404, c’est modifié dans la branche.
@ -278,0 +316,4 @@
return ctx
def get_success_url(self):
if not self.authenticator.phone_identifier_field or not self.code: # user input is email
Je ne comprends pas le commentaire, quel est le parcours réel qui correspond à ça ?
Oui c’est du code mort, un copier-coller sans doute, j’ai viré le truc.
@ -278,0 +400,4 @@
class PhoneChangeVerifyView(TemplateView):
@atomic(savepoint=False)
Pas évident de comprendre le comportement attendu avec cette utilisation de atomic qui englobe 100 lignes, de quoi se protège-t-on ? (ou y a-t-il un test qui correspond ?)
Oui tu as raison, le
atomic()
est mal placé, il doit venir avec leLock.lock_identifier(phone)
. J’ai ré-écrit en incluant dans un gestionnaire de contexte (with atomic():
) la partie du code qui doit nécessairement s’exécuter de façon atomique.Quant à la nécessité de désactiver les SAVEPOINTs postgresql, je ne suis pas très familier avec cela mais ne vois rien dans l’usage des verrous custom authentic qui nécessite cela, j’ai viré le truc.
Pour ce qui est des tests, s’il y a des usages de ces verrous dans des transactions pas testés actuellement, peut-être qu’on pourrait faire un ticket à par et étendre
test_models.TestLock
directement ?Yep c'est nettement plus clair, tu pourrais également sortir du bloc le check non_unique (atomic concerne les écritures et est sans effet sur les lectures) et les lignes à partir de messages.info pourraient aller dans le
else:
du try/except.Remarque générale à propos de ce code, je suis vraiment pas fan de la gestion des exceptions où on ne sait pas quelle ligne est susceptible de lever quoi, même si j'entends que ça permet d'avoir un seul return. Notamment le rattrapage de ValueError sur tout le bloc rendra le debuggage complexe. Mais je ne bloque pas là dessus, comme tu le sens !
Oui tout à fait, loin de moi l'idée de demander un truc compliqué au détour de ce commentaire.
Oui, encore mieux comme cela, c’est modifié.
Je préfère avoir le bloc qui doit s’exécuter de façon atomique contenu et bref, et les exceptions autour. Je pense laisser comme cela.
D’ac.
Et en fait, à y réfléchir, dès la phase de vérification
non_unique
il faut le verrou sur le numéro de téléphone, donc le code doit être contenu dans la transaction, j’ai rétabli cette partie du code à sa version antérieure.@ -599,0 +810,4 @@
allow_phone_change = getattr(
authenticator.phone_identifier_field, 'user_editable', False
) and not getattr(authenticator.phone_identifier_field, 'disabled', True)
Je serais pour éviter ces getattr, on peut réécrire en
ctx['allow_phone_change'] = (attribute := self.authenticator.phone_identifier_field) and attribute.user_editable and not attribute.disabled
, ou mettre tout le bloc de code sous unif self.authenticator.phone_identifier_field: ...
.C’est ré-écrit comme tu le suggères, juste sans opérateur morse, et en englobant le tout dans un
bool()
par pur souci de lisibilité.Aucun souci, j’intègre déjà cette série de remarque.
7dff41ebc1
toc3022fe93f
2e731818a0
toc88098d200
@ -55,2 +55,4 @@
class PhoneChangeFormNoPassword(forms.Form):
phone = PhoneField(label=_('New email'))
Mauvais libellé ici :)
Oups, flagrant délit de copier-coller. C’est corrigé, merci !
b6dfbd4f18
todc18bc882d
ad8e80f4e0
tobb4c7c9dd1
C’est fait, merci.
@ -1378,1 +1593,3 @@
)
try:
Lock.lock_identifier(self.code.phone)
token = models.Token.create(
Puisque je suis lancé sur le sujet gestion des exceptions, je pense qu'il n'y a pas d'utilité à avoir cette ligne dans le try/except.
Et en fait je réalise qu’il n’y a pas d’intérêt à garantir l’atomicité de cette phase de création du jeton ni un verrou sur le numéro rentré. C’est à la consommation du jeton qu’il faut être vigilant. J’ai viré le décorateur, le verrou et le
try: … except:
.bb4c7c9dd1
to187c9e60f0
187c9e60f0
tob3ea4a4a3a
b3ea4a4a3a
toc5f1b0cbdc
c5f1b0cbdc
to6c27ec47dd
Vu de vive voix avec Valentin, quelques corrections sur la gestion des exceptions à la consommation du jeton de changement de numéro de téléphone (dans
PhoneChangeVerifyView.get()
).Let's go !
Merci ❤️