user: allow customization of User.get_full_name() through templates (#72945) #2

Merged
fpeters merged 1 commits from wip/72945-custom-rendered-name-via-template into main 2023-02-10 09:14:08 +01:00
Owner

Via une app à installer manuellement sur les briques concernées qui s'occupe de monkey-patcher le modèle user. Ça permet d'avoir un rendu cohérent partout.

Pour tester, par exemple sur combo :

  1. Ajouter 'hobo.user_name.apps.UserNameConfig', dans combo/settings.py:INSTALLED_APPS
  2. Dans publik-base-theme, surcharcher le template d'un thème, par exemple echo '{{ user.first_name }} Bar' > templates/variants/univ-avignon/includes/user-info-user-name.html
  3. Activer le thème en question (ici, univ-avignon) sur https://hobo.dev.publik.love/theme/
  4. Visiter https://combo.dev.publik.love/, se connecter, vérifier que le cartouche en haut de page contient bien admin Bar ou approchant
  5. Visiter https://agent-combo.dev.publik.love/, même vérification

cc @fpeters parce qu'on a élaboré l'approche ensemble.

Via une app à installer manuellement sur les briques concernées qui s'occupe de monkey-patcher le modèle user. Ça permet d'avoir un rendu cohérent partout. Pour tester, par exemple sur combo : 1. Ajouter `'hobo.user_name.apps.UserNameConfig',` dans `combo/settings.py:INSTALLED_APPS` 2. Dans publik-base-theme, surcharcher le template d'un thème, par exemple `echo '{{ user.first_name }} Bar' > templates/variants/univ-avignon/includes/user-info-user-name.html` 3. Activer le thème en question (ici, univ-avignon) sur https://hobo.dev.publik.love/theme/ 4. Visiter https://combo.dev.publik.love/, se connecter, vérifier que le cartouche en haut de page contient bien `admin Bar` ou approchant 5. Visiter https://agent-combo.dev.publik.love/, même vérification cc @fpeters parce qu'on a élaboré l'approche ensemble.
aberriot added 1 commit 2023-01-04 12:18:41 +01:00
gitea-wip/hobo/pipeline/pr-main There was a failure building this commit Details
efd605875b
user: allow customization of User.get_full_name() through templates (#72945)
Owner

Via une app à installer manuellement

Pour tester. Une fois qu'on est ok on peut installer cette partie via debian/debian_config_common.py, dans la section qui installe déjà le provisionning :

if PROJECT_NAME != 'wcs' and 'authentic2' not in INSTALLED_APPS:
    MIDDLEWARE = MIDDLEWARE + (
        'mellon.middleware.PassiveAuthenticationMiddleware',
        'hobo.provisionning.middleware.ProvisionningMiddleware',
    )

Ou juste dans une branche PROJECT_NAME != 'wcs', en ajoutant au code la détection du fait qu'il est appelé depuis authentic, ou en modifiant le code pour que l'attribut soit nommé "attributes" plutôt que "cached_extra_attributes" ce qui de loin me semble suffisant pour assurer le comportement à l'identique. (et côté w.c.s. l'attribut "attributes" n'existe actuellement pas, il pourra être ajouté pour compatibilité).

> Via une app à installer manuellement Pour tester. Une fois qu'on est ok on peut installer cette partie via debian/debian_config_common.py, dans la section qui installe déjà le provisionning : ``` if PROJECT_NAME != 'wcs' and 'authentic2' not in INSTALLED_APPS: MIDDLEWARE = MIDDLEWARE + ( 'mellon.middleware.PassiveAuthenticationMiddleware', 'hobo.provisionning.middleware.ProvisionningMiddleware', ) ``` Ou juste dans une branche `PROJECT_NAME != 'wcs'`, en ajoutant au code la détection du fait qu'il est appelé depuis authentic, ou en modifiant le code pour que l'attribut soit nommé "attributes" plutôt que "cached_extra_attributes" ce qui de loin me semble suffisant pour assurer le comportement à l'identique. (et côté w.c.s. l'attribut "attributes" n'existe actuellement pas, il pourra être ajouté pour compatibilité).
aberriot added 1 commit 2023-01-04 15:29:27 +01:00
Author
Owner

Second commit pour réduire le souci de build qui crashe (une sombre histoire de settings preloadés et pas altérables dans les tests sans faire crasher le reste)

Second commit pour réduire le souci de build qui crashe (une sombre histoire de settings preloadés et pas altérables dans les tests sans faire crasher le reste)
aberriot added 1 commit 2023-01-05 09:40:46 +01:00
gitea-wip/hobo/pipeline/pr-main There was a failure building this commit Details
5c828a7524
fixup! fixup! user: allow customization of User.get_full_name() through templates (#72945)
aberriot reviewed 2023-01-05 09:41:41 +01:00
@ -289,2 +289,4 @@
'hobo.provisionning.middleware.ProvisionningMiddleware',
)
if PROJECT_NAME != 'wcs':
INSTALLED_APPS += ('hobo.user_name.apps.UserNameConfig',)
Author
Owner

@fpeters ça te parait okay ? J'ai modifié la logique dans le utils.py pour avoir un comportement distinct côté authentic.

@fpeters ça te parait okay ? J'ai modifié la logique dans le `utils.py` pour avoir un comportement distinct côté authentic.
aberriot reviewed 2023-01-05 09:42:42 +01:00
@ -0,0 +31,4 @@
def cached_extra_attributes(user):
if 'authentic2' in settings.INSTALLED_APPS:
# we're in authentic, attributes are looked up differently
return {k: v.to_python() for k, v in user.attributes.values.items()}
Author
Owner

Pas sûre de la meilleure façon de faire ça. Idéalement j'aimerai avoir la même payload que ce qui est envoyé lors du GET /__provision__ sur chaque service mais je n'ai pas trouvé où elle est générée.

Pas sûre de la meilleure façon de faire ça. Idéalement j'aimerai avoir la même payload que ce qui est envoyé lors du `GET /__provision__` sur chaque service mais je n'ai pas trouvé où elle est générée.
Owner

Oui en effet il me semble que ça ne va pas fonctionner pour les attributs multiples dans authentic. Je regarde.

Oui en effet il me semble que ça ne va pas fonctionner pour les attributs multiples dans authentic. Je regarde.
Owner

Il y a encore un souci de récursion, comme en témoignent les builds successifs dans les deux branches concernées.

À mon avis, il y a un endroit dans la gestion des erreurs où User.str ou User.repr est appelé, lequel appelle à son tour la nouvelle méthode monkeypatchée User.get_full_name, laquelle lève un TemplateNotFoundError, etc.

J’ai un peu creusé l’affaire mais sans trop d’inspiration, je pense que le monkeypatching se passe mal et que User.repr et User.str sont mal monkeypatchées et font appel à cette nouvelle méthode par gabarit. Je vais reprendre à tête reposée.

––––

En remarque plus générale, je note que j’aime moyen les substitutions automagiques sous le tapis telle que celle de ce patch, j’aurais préféré que le patch ajoute une méthode différente de get_full_name, par exemple get_templated_full_name, et qu’on adapte explicitement les gabarits partout où c’est nécessaire, par exemple en y ajoutant un truc du genre {% firstof user.get_templated_full_name user.get_full_name %}. Là déjà à l’écriture du patch on se tape un sale bug lorsque le gabarit du nom complet est absent dans les tests, c’est de mauvais augure pour la suite.

Il y a encore un souci de récursion, comme en témoignent les builds successifs dans les deux branches concernées. À mon avis, il y a un endroit dans la gestion des erreurs où User.__str__ ou User.__repr__ est appelé, lequel appelle à son tour la nouvelle méthode monkeypatchée User.get_full_name, laquelle lève un TemplateNotFoundError, etc. J’ai un peu creusé l’affaire mais sans trop d’inspiration, je pense que le monkeypatching se passe mal et que User.__repr__ et User.__str__ sont mal monkeypatchées et font appel à cette nouvelle méthode par gabarit. Je vais reprendre à tête reposée. –––– En remarque plus générale, je note que j’aime moyen les substitutions automagiques sous le tapis telle que celle de ce patch, j’aurais préféré que le patch ajoute une méthode différente de get_full_name, par exemple get_templated_full_name, et qu’on adapte explicitement les gabarits partout où c’est nécessaire, par exemple en y ajoutant un truc du genre {% firstof user.get_templated_full_name user.get_full_name %}. Là déjà à l’écriture du patch on se tape un sale bug lorsque le gabarit du nom complet est absent dans les tests, c’est de mauvais augure pour la suite.
Owner

Pour l’instant j’ai retravaillé le patch pour y inclure la gestion des attributs multiples. Une petite modification dans les tests aussi pour y inclure directement le dossier contenant le gabarit à tester (via le setting TEMPLATES), plutôt que de déclarer et installer une app qui ne contient que ce dossier (et de la déclaration via INSTALLED_APPS).

Il reste encore ce souci de récursion à régler à l’appel des tests authentic lorsque que le gabarit personnalisé est inexistant, je regarde plus tard aujourd’hui.

Pour l’instant j’ai retravaillé le patch pour y inclure la gestion des attributs multiples. Une petite modification dans les tests aussi pour y inclure directement le dossier contenant le gabarit à tester (via le setting TEMPLATES), plutôt que de déclarer et installer une app qui ne contient que ce dossier (et de la déclaration via INSTALLED_APPS). Il reste encore ce souci de récursion à régler à l’appel des tests authentic lorsque que le gabarit personnalisé est inexistant, je regarde plus tard aujourd’hui.
pmarillonnet force-pushed wip/72945-custom-rendered-name-via-template from 5c828a7524 to 0c14103e4b 2023-01-30 16:47:04 +01:00 Compare
Owner

Voilà, une nouvelle version qui corrige le souci de récursion dans les tests évoqué plus haut, et qui ajoute et teste le support des attributs multiples.

Voilà, une nouvelle version qui corrige le souci de récursion dans les tests évoqué plus haut, et qui ajoute et teste le support des attributs multiples.
Owner

Je remonte à un truc que j'écrivais :

ou en modifiant le code pour que l'attribut soit nommé "attributes" plutôt que "cached_extra_attributes" ce qui de loin me semble suffisant pour assurer le comportement à l'identique. (et côté w.c.s. l'attribut "attributes" n'existe actuellement pas, il pourra être ajouté pour compatibilité).

Il me semble que le code serait bien plus simple ainsi, zéro besoin de copier tout ce code authentic; pour authentic on garde le .attributes. natif qui fonctionne, pour les autres on ajoute un .attributes = functools.cached_property(cached_extra_attributes).

Je remonte à un truc que j'écrivais : > ou en modifiant le code pour que l'attribut soit nommé "attributes" plutôt que "cached_extra_attributes" ce qui de loin me semble suffisant pour assurer le comportement à l'identique. (et côté w.c.s. l'attribut "attributes" n'existe actuellement pas, il pourra être ajouté pour compatibilité). Il me semble que le code serait bien plus simple ainsi, zéro besoin de copier tout ce code authentic; pour authentic on garde le .attributes. natif qui fonctionne, pour les autres on ajoute un `.attributes = functools.cached_property(cached_extra_attributes)`.
pmarillonnet force-pushed wip/72945-custom-rendered-name-via-template from 0c14103e4b to 90f441d72d 2023-01-31 10:09:15 +01:00 Compare
pmarillonnet force-pushed wip/72945-custom-rendered-name-via-template from 90f441d72d to 729c174bf9 2023-01-31 10:10:41 +01:00 Compare
Owner

Il me semble que le code serait bien plus simple ainsi, zéro besoin de copier tout ce code authentic; pour authentic on garde le .attributes. natif qui fonctionne, pour les autres on ajoute un .attributes = functools.cached_property(cached_extra_attributes).

Oui ok j’avais loupé cette remarque en reprenant le ticket en cours de route. C’est corrigé dans la branche et c’est bien mieux ainsi, en effet.

> Il me semble que le code serait bien plus simple ainsi, zéro besoin de copier tout ce code authentic; pour authentic on garde le .attributes. natif qui fonctionne, pour les autres on ajoute un .attributes = functools.cached_property(cached_extra_attributes). Oui ok j’avais loupé cette remarque en reprenant le ticket en cours de route. C’est corrigé dans la branche et c’est bien mieux ainsi, en effet.
Owner

C'était dans le plan initial,

Dans publik-base-theme, surcharcher le template d'un thème, par exemple echo '{{ user.first_name }} Bar' > templates/variants/univ-avignon/includes/user-info-user-name.html

Mais je pense maintenant qu'il ne faut pas faire ainsi, que le gabarit doit plutôt être pris d'une variable hobo (qui se retrouvent dans tous les modules via settings.TEMPLATE_VARS).

L'idée c'est que ça n'est pas en soit lié à une intégration graphique et que ça permettra dans un second temps d'ajouter la configuration de cette partie via l'UI (via l'écran profil d'hobo qui permet déjà de définir les attributs).

Ici je ne demande pas cette UI, mais bien que le gabarit soit récupéré depuis settings.TEMPLATE_VARS['user_full_name_template'], elle y arriverait pour le moment via l'écran générique "variables".

C'était dans le plan initial, > Dans publik-base-theme, surcharcher le template d'un thème, par exemple echo '{{ user.first_name }} Bar' > templates/variants/univ-avignon/includes/user-info-user-name.html Mais je pense maintenant qu'il ne faut pas faire ainsi, que le gabarit doit plutôt être pris d'une variable hobo (qui se retrouvent dans tous les modules via settings.TEMPLATE_VARS). L'idée c'est que ça n'est pas en soit lié à une intégration graphique et que ça permettra dans un second temps d'ajouter la configuration de cette partie via l'UI (via l'écran profil d'hobo qui permet déjà de définir les attributs). Ici je ne demande pas cette UI, mais bien que le gabarit soit récupéré depuis `settings.TEMPLATE_VARS['user_full_name_template']`, elle y arriverait pour le moment via l'écran générique "variables".
pmarillonnet force-pushed wip/72945-custom-rendered-name-via-template from 729c174bf9 to ab28c31b2d 2023-02-07 10:09:04 +01:00 Compare
fpeters requested changes 2023-02-07 10:15:01 +01:00
@ -0,0 +23,4 @@
django.template.exceptions.TemplateDoesNotExist,
django.template.exceptions.TemplateSyntaxError,
) as e:
logger.exception('hobo.user_name: cannot render user "%s" name', user)
Owner

Ça ne me semble pas adéquat ça va spammer d'exceptions partout alors que ne pas avoir de gabarit personnalisé reste le cas normal.

C'est peut-être plus simple de ne pas découper autant, ramener le code de get_full_name_from_template dans cette fonction, et si if 'user_full_name_template' not in template_vars alors return user.original_get_full_name(), et laisser alors la gestion des exceptions pour les vrais cas exceptionnels.

(désolé de ne pas avoir pu tout noter d'un coup).

Ça ne me semble pas adéquat ça va spammer d'exceptions partout alors que ne pas avoir de gabarit personnalisé reste le cas normal. C'est peut-être plus simple de ne pas découper autant, ramener le code de get_full_name_from_template dans cette fonction, et si `if 'user_full_name_template' not in template_vars` alors `return user.original_get_full_name()`, et laisser alors la gestion des exceptions pour les vrais cas exceptionnels. (désolé de ne pas avoir pu tout noter d'un coup).
Owner

Ça ne me semble pas adéquat ça va spammer d'exceptions partout alors que ne pas avoir de gabarit personnalisé reste le cas normal.

Oui complètement, j’avais loupé ça, merci.

C'est peut-être plus simple de ne pas découper autant, ramener le code de get_full_name_from_template dans cette fonction, et si if 'user_full_name_template' not in template_vars alors return user.original_get_full_name(), et laisser alors la gestion des exceptions pour les vrais cas exceptionnels.

Ok, c’est modifié dans la branche.
J’ai laissé le ´except UserExtraAttributes.DoesNotExist´ dans le code bien qu’il ne me semble pas nécessaire, mais je me dis que peut-être j’ai loupé un cas limite qu’Agate aurait identifié pour certaines briques (?)

(désolé de ne pas avoir pu tout noter d'un coup).

(absolument aucun souci)

> Ça ne me semble pas adéquat ça va spammer d'exceptions partout alors que ne pas avoir de gabarit personnalisé reste le cas normal. Oui complètement, j’avais loupé ça, merci. > C'est peut-être plus simple de ne pas découper autant, ramener le code de get_full_name_from_template dans cette fonction, et si if 'user_full_name_template' not in template_vars alors return user.original_get_full_name(), et laisser alors la gestion des exceptions pour les vrais cas exceptionnels. Ok, c’est modifié dans la branche. J’ai laissé le ´except UserExtraAttributes.DoesNotExist´ dans le code bien qu’il ne me semble pas nécessaire, mais je me dis que peut-être j’ai loupé un cas limite qu’Agate aurait identifié pour certaines briques (?) > (désolé de ne pas avoir pu tout noter d'un coup). (absolument aucun souci)
Owner

Mais je pense maintenant qu'il ne faut pas faire ainsi, que le gabarit doit plutôt être pris d'une variable hobo (qui se retrouvent dans tous les modules via settings.TEMPLATE_VARS).

L'idée c'est que ça n'est pas en soit lié à une intégration graphique et que ça permettra dans un second temps d'ajouter la configuration de cette partie via l'UI (via l'écran profil d'hobo qui permet déjà de définir les attributs).

Ici je ne demande pas cette UI, mais bien que le gabarit soit récupéré depuis settings.TEMPLATE_VARS['user_full_name_template'], elle y arriverait pour le moment via l'écran générique "variables".

Oui ok, pourquoi pas, c’est sans doute mieux comme ça sans avoir à toucher au thème, en effet.
C’est à jour dans la branche, on cherche maintenant à retrouver le contenu d’un gabarit inline dans ´settings.TEMPLATE_VARS['user_full_name_template']´ au lieu du contenu d’un fichier.

> Mais je pense maintenant qu'il ne faut pas faire ainsi, que le gabarit doit plutôt être pris d'une variable hobo (qui se retrouvent dans tous les modules via settings.TEMPLATE_VARS). > > L'idée c'est que ça n'est pas en soit lié à une intégration graphique et que ça permettra dans un second temps d'ajouter la configuration de cette partie via l'UI (via l'écran profil d'hobo qui permet déjà de définir les attributs). > > Ici je ne demande pas cette UI, mais bien que le gabarit soit récupéré depuis settings.TEMPLATE_VARS['user_full_name_template'], elle y arriverait pour le moment via l'écran générique "variables". Oui ok, pourquoi pas, c’est sans doute mieux comme ça sans avoir à toucher au thème, en effet. C’est à jour dans la branche, on cherche maintenant à retrouver le contenu d’un gabarit inline dans ´settings.TEMPLATE_VARS['user_full_name_template']´ au lieu du contenu d’un fichier.
pmarillonnet force-pushed wip/72945-custom-rendered-name-via-template from ab28c31b2d to 1282779c1d 2023-02-07 10:44:13 +01:00 Compare
fpeters approved these changes 2023-02-07 11:05:57 +01:00
fpeters merged commit 6b76f3c3eb into main 2023-02-10 09:14:08 +01:00
fpeters deleted branch wip/72945-custom-rendered-name-via-template 2023-02-10 09:14:08 +01:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
3 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/hobo#2
No description provided.