Parcours de modification du numéro de téléphone identifiant (#72614) #80

Merged
pmarillonnet merged 2 commits from wip/72614-phone-identifier-modification-ui into main 2023-07-19 16:04:12 +02:00
Owner
No description provided.
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from ff09052ecf to 85b4fb82eb 2023-06-21 17:14:05 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 85b4fb82eb to cd51d3bf5a 2023-06-22 09:47:00 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from cd51d3bf5a to 21ba03b241 2023-06-22 10:42:41 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 21ba03b241 to 62700c9a72 2023-06-22 10:47:49 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 62700c9a72 to 898f7c2e2b 2023-06-22 11:50:51 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 898f7c2e2b to b02069a1e5 2023-06-26 16:50:39 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from b02069a1e5 to 0b8e9c0adf 2023-06-26 17:12:11 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 0b8e9c0adf to 67227f73a6 2023-06-27 15:53:36 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 67227f73a6 to 8521f2a670 2023-06-27 16:16:38 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 8521f2a670 to f73acb76cd 2023-06-27 16:51:55 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from f73acb76cd to 2e5fb0f4dc 2023-06-28 10:54:42 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 2e5fb0f4dc to 7c4b388e60 2023-06-28 11:25:39 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 7c4b388e60 to 2488ab182b 2023-06-28 11:57:35 +02:00 Compare
pmarillonnet changed target branch from wip/78046-password-authn-identifier-selection to main 2023-06-28 11:58:24 +02:00
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 2488ab182b to 011f10975e 2023-06-28 12:30:32 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 011f10975e to e48565ba9a 2023-06-29 11:04:24 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from e48565ba9a to d483fe0615 2023-07-10 10:14:31 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from d483fe0615 to fc81bc03ae 2023-07-10 12:04:06 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from fc81bc03ae to 3b2d08085e 2023-07-10 12:18:05 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 3b2d08085e to a9181beff2 2023-07-10 15:25:11 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from a9181beff2 to 8d83007668 2023-07-10 16:00:21 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 8d83007668 to 4cd4e8cd99 2023-07-11 11:13:17 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 4cd4e8cd99 to 592739e2b0 2023-07-11 11:36:22 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 592739e2b0 to c61a473183 2023-07-11 11:53:03 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from c61a473183 to 59326fab88 2023-07-11 14:44:35 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from e384b87ed0 to 957a7789eb 2023-07-11 15:41:55 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 957a7789eb to c736890a6e 2023-07-11 16:13:59 +02:00 Compare
pmarillonnet changed target branch from main to wip/79135-user-phone-model-field-deprecation 2023-07-11 16:14:13 +02:00
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from c736890a6e to 8b3405f1a6 2023-07-11 16:25:39 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 8b3405f1a6 to 1ee4cb64bf 2023-07-12 10:09:18 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 1ee4cb64bf to 21ade8096a 2023-07-12 10:45:55 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 5dde1308e6 to a01e08fd24 2023-07-12 12:51:53 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from a01e08fd24 to 4d4b9f0b69 2023-07-12 12:53:15 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 4d4b9f0b69 to 833bc0267a 2023-07-12 14:17:03 +02:00 Compare
pmarillonnet changed title from 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) 2023-07-12 14:17:15 +02:00
vdeniaud requested changes 2023-07-13 10:22:44 +02:00
vdeniaud left a comment
Owner

Pas encore testé pour de vrai, mais déjà quelques remarques de forme.

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(
Owner

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.

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.
Author
Owner

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 colonne content de la première entrée.

C’est modifié dans la nouvelle version de la PR.

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 colonne `content` 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
Owner

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).

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).
Author
Owner

Ok, fair enough, j’ai ajouté le get_form_kwargs correspondant dans la vue, et l’initialisateur qui va avec c

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.
Owner

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é)

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é)
Author
Owner

Oui tu as raison, c’est complètement dans `form_valid

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:
Owner

L'authentificateur existe forcément, je trouve ça étrange de le mettre dans une condition.

L'authentificateur existe forcément, je trouve ça étrange de le mettre dans une condition.
Author
Owner

Ok, je l’ai sorti de la condition.

Ok, je l’ai sorti de la condition.
Owner

La correction a atterri dans le second commit au lieu du premier

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.'),)
Owner

Vraiment besoin d'un tuple ?

Vraiment besoin d'un tuple ?
Author
Owner

Non :D
C’est modifié.

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
Owner

Je suis plutôt pour ouvrir dors et déjà un ticket à traiter ensuite, plutôt qu'on TODO qui restera à jamais :)

Je suis plutôt pour ouvrir dors et déjà un ticket à traiter ensuite, plutôt qu'on TODO qui restera à jamais :)
Author
Owner

Ok, #79686.

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(
Owner

Même remarque que plus haut sur la double évaluation.

Même remarque que plus haut sur la double évaluation.
Author
Owner

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 !)

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:
Owner

Cosmétique, je verrais plutôt un if not token: return, ça duplique le return mais ça économise beaucoup d'indentations.

Cosmétique, je verrais plutôt un if not token: return, ça duplique le return mais ça économise beaucoup d'indentations.
Author
Owner

Ok, c’est plus lisible ainsi en effet.

Ok, c’est plus lisible ainsi en effet.
@ -1379,0 +1584,4 @@
duration=120,
)
except Lock.Error:
request = StoreRequestMiddleware.get_request()
Owner

On est dans une vue, on a accès à self.request non ?

On est dans une vue, on a accès à `self.request` non ?
Author
Owner

Oui complètement, c’est modifié, merci.

Oui complètement, c’est modifié, merci.
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 647c748fa2 to ef3ab62445 2023-07-13 15:08:37 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 4eb0ffad3a to 4212900b5f 2023-07-13 15:41:25 +02:00 Compare
Author
Owner

Pas encore testé pour de vrai, mais déjà quelques remarques de forme.

Voilà c’est tout pris en compte :)

> Pas encore testé pour de vrai, mais déjà quelques remarques de forme. Voilà c’est tout pris en compte :)
pmarillonnet requested review from vdeniaud 2023-07-13 16:09:52 +02:00
Owner

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.

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.
Author
Owner

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.

> 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.
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from aee93614ed to fbf6f15952 2023-07-17 10:58:30 +02:00 Compare
Author
Owner

Ok, merci pour ta vigilance, sans doute des modifications nécessaires côté hobo agent authentic. Je regarde.

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.

> Ok, merci pour ta vigilance, sans doute des modifications nécessaires côté hobo agent authentic. Je regarde. 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.
pmarillonnet added 7 commits 2023-07-17 16:05:04 +02:00
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 4182f0a7ef to 7dff41ebc1 2023-07-18 15:29:54 +02:00 Compare
pmarillonnet changed title from 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) 2023-07-18 15:30:09 +02:00
pmarillonnet changed target branch from wip/79135-user-phone-model-field-deprecation to main 2023-07-18 15:30:10 +02:00
vdeniaud requested changes 2023-07-18 17:37:47 +02:00
vdeniaud left a comment
Owner

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

539        authenticator = utils_misc.get_password_authenticator() 

qui serait à déplacer dans le commit d'après.

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 ``` 539 authenticator = utils_misc.get_password_authenticator() ``` 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]:
Owner

Peut-être plus simple et j'espère équivalent,

if (
            models.AttributeValue.objects.with_owner(self.user)
            .filter(attribute=self.authenticator.phone_identifier_field, content=phone).exists()
):
Peut-être plus simple et j'espère équivalent, ```python if ( models.AttributeValue.objects.with_owner(self.user) .filter(attribute=self.authenticator.phone_identifier_field, content=phone).exists() ): ```
Author
Owner

Oui, c’est mieux comme ça en effet. C’est corrigé, merci.

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,
Owner

Le message change mais l'action reste nommée email-change, peut-être juste ne pas chercher à mutualiser ce dispatch ?

Le message change mais l'action reste nommée email-change, peut-être juste ne pas chercher à mutualiser ce dispatch ?
Author
Owner

J’ai conservé le dispatch unique mais avec une action qui varie.

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']
Owner

Ç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.

Ç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.
Author
Owner

Oui, ok, on va faire sans. C’est corrigé.

Oui, ok, on va faire sans. C’est corrigé.
@ -278,0 +309,4 @@
content__isnull=False,
)
if atvs:
ctx['phone'] = atvs.first().to_python()
Owner

Ici même remarque que #80 (comment), il me semble que ça provoque deux requêtes

Ici même remarque que https://git.entrouvert.org/entrouvert/authentic/pulls/80#issuecomment-13982, il me semble que ça provoque deux requêtes
Author
Owner

C’est corrigé dans la branche, merci.

C’est corrigé dans la branche, merci.
Owner

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)

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)
Author
Owner

Arf j’avais loupé cela, c’est corrigé, merci.

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)
Owner

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).

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).
Author
Owner

Ok, ça me va de taper une 404, c’est modifié dans la branche.

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
Owner

Je ne comprends pas le commentaire, quel est le parcours réel qui correspond à ça ?

Je ne comprends pas le commentaire, quel est le parcours réel qui correspond à ça ?
Author
Owner

Oui c’est du code mort, un copier-coller sans doute, j’ai viré le truc.

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)
Owner

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 ?)

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 ?)
Author
Owner

Oui tu as raison, le atomic() est mal placé, il doit venir avec le Lock.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 ?

Oui tu as raison, le `atomic()` est mal placé, il doit venir avec le `Lock.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 ?
Owner

en incluant dans un gestionnaire de contexte (with atomic():) la partie du code qui doit nécessairement s’exécuter de façon atomique.

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 !

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 ?

Oui tout à fait, loin de moi l'idée de demander un truc compliqué au détour de ce commentaire.

> en incluant dans un gestionnaire de contexte (with atomic():) la partie du code qui doit nécessairement s’exécuter de façon atomique. 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 ! > 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 ? Oui tout à fait, loin de moi l'idée de demander un truc compliqué au détour de ce commentaire.
Author
Owner

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.

Oui, encore mieux comme cela, c’est modifié.

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 !

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.

Oui tout à fait, loin de moi l'idée de demander un truc compliqué au détour de ce commentaire.

D’ac.

> 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. Oui, encore mieux comme cela, c’est modifié. > 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 ! 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. > Oui tout à fait, loin de moi l'idée de demander un truc compliqué au détour de ce commentaire. D’ac.
Author
Owner

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.

Oui, encore mieux comme cela, c’est modifié.

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.

> > 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. > > Oui, encore mieux comme cela, c’est modifié. 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)
Owner

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 un if self.authenticator.phone_identifier_field: ....

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 un `if self.authenticator.phone_identifier_field: ...`.
Author
Owner

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é.

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é.
Author
Owner

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.

Aucun souci, j’intègre déjà cette série de remarque.

> 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. Aucun souci, j’intègre déjà cette série de remarque.
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 7dff41ebc1 to c3022fe93f 2023-07-19 09:53:47 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 2e731818a0 to c88098d200 2023-07-19 10:09:43 +02:00 Compare
vdeniaud reviewed 2023-07-19 10:11:27 +02:00
@ -55,2 +55,4 @@
class PhoneChangeFormNoPassword(forms.Form):
phone = PhoneField(label=_('New email'))
Owner

Mauvais libellé ici :)

Mauvais libellé ici :)
Author
Owner

Oups, flagrant délit de copier-coller. C’est corrigé, merci !

Oups, flagrant délit de copier-coller. C’est corrigé, merci !
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from b6dfbd4f18 to dc18bc882d 2023-07-19 11:02:55 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from ad8e80f4e0 to bb4c7c9dd1 2023-07-19 11:20:21 +02:00 Compare
Author
Owner

remarque pas faisable dans l'interface, dans le premier commit il y a

539        authenticator = utils_misc.get_password_authenticator() 

qui serait à déplacer dans le commit d'après.

C’est fait, merci.

> remarque pas faisable dans l'interface, dans le premier commit il y a > ``` > 539 authenticator = utils_misc.get_password_authenticator() > ``` > > qui serait à déplacer dans le commit d'après. C’est fait, merci.
pmarillonnet requested review from vdeniaud 2023-07-19 11:34:40 +02:00
vdeniaud reviewed 2023-07-19 12:30:04 +02:00
@ -1378,1 +1593,3 @@
)
try:
Lock.lock_identifier(self.code.phone)
token = models.Token.create(
Owner

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.

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.
Author
Owner

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:.

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:`.
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from bb4c7c9dd1 to 187c9e60f0 2023-07-19 14:42:07 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from 187c9e60f0 to b3ea4a4a3a 2023-07-19 14:45:58 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from b3ea4a4a3a to c5f1b0cbdc 2023-07-19 14:59:52 +02:00 Compare
pmarillonnet force-pushed wip/72614-phone-identifier-modification-ui from c5f1b0cbdc to 6c27ec47dd 2023-07-19 15:50:09 +02:00 Compare
Author
Owner

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()).

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()`).
vdeniaud approved these changes 2023-07-19 15:57:43 +02:00
vdeniaud left a comment
Owner

Let's go !

Let's go !
Author
Owner

Let's go !

Merci ❤️

> Let's go ! Merci ❤️
pmarillonnet merged commit 6c27ec47dd into main 2023-07-19 16:04:12 +02:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
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/authentic#80
No description provided.