templatetags: allow UUIDs as |has_role filter argument (#79857) #137

pmarillonnet · 2023-07-20T15:28:46+02:00

pmarillonnet commented

2023-07-20 15:28:46 +02:00

No description provided.

pmarillonnet force-pushed wip/79857-has-role-uuid from 9c5baa0157 to 875db57854

2023-07-20 15:52:59 +02:00

Compare

pmarillonnet changed title from ~~WIP: templatetags: allow UUIDs as |has_role filter argument (#79857)~~ to templatetags: allow UUIDs as |has_role filter argument (#79857)

2023-07-20 15:53:06 +02:00

pmarillonnet commented

2023-07-20 15:55:19 +02:00

Aucune idée de comment tester cela : il n’y a que via l’agent hobo que l’on connaît l’uuid du rôle, qui n’est pas utilisé dans combo sinon.
Pour tester il faudrait moquer tout un ensemble de données, i.e. prendre comme postulat de test une structure de données qui est justement celle que l’on doit prouver.
Testé en local dans mon devinst, cela fonctionne.

Aucune idée de comment tester cela : il n’y a que via l’agent hobo que l’on connaît l’uuid du rôle, qui n’est pas utilisé dans combo sinon. Pour tester il faudrait moquer tout un ensemble de données, i.e. prendre comme postulat de test une structure de données qui est justement celle que l’on doit prouver. Testé en local dans mon devinst, cela fonctionne.

pmarillonnet commented

2023-07-20 16:05:04 +02:00

Peut-être dans un monde idéal la manière la plus correcte de procéder :
· faire que combo définisse son propre modèle de rôle, lequel contient un champ uuid,
· migrer tous les django.contrib.auth.models.Group connus de combo dans ce nouveau modèle, et abandonner l’usage de ces Group,
· modifier dans hobo le provisionning côté combo pour que la réception des rôles mettent à jour l’uuid dans ce modèle local à combo, et non plus dans le générique hobo.agent.common.models.Role.

Mais c’est sans doute surdimensionné pour ce qu’on souhaite faire ici (et au delà de la demi-journée de dév facturée)

Peut-être dans un monde idéal la manière la plus correcte de procéder : · faire que combo définisse son propre modèle de rôle, lequel contient un champ `uuid`, · migrer tous les `django.contrib.auth.models.Group` connus de combo dans ce nouveau modèle, et abandonner l’usage de ces `Group`, · modifier dans hobo le provisionning côté combo pour que la réception des rôles mettent à jour l’uuid dans ce modèle local à combo, et non plus dans le générique `hobo.agent.common.models.Role`. Mais c’est sans doute surdimensionné pour ce qu’on souhaite faire ici (et au delà de la demi-journée de dév facturée)

pmarillonnet closed this pull request

2023-07-20 16:06:29 +02:00

pmarillonnet reopened this pull request

2023-07-20 16:06:48 +02:00

fpeters commented

2023-07-20 16:22:31 +02:00

Perso je pense que ça a été accepté trop vite (y compris par moi qui répond qu'on peut réfléchir) et je pense qu'on ne devrait pas permettre ça. Qu'on devrait plutôt permettre l'utilisation de slug.

pmarillonnet commented

2023-07-20 17:05:01 +02:00

Perso je pense que ça a été accepté trop vite (y compris par moi qui répond qu'on peut réfléchir) et je pense qu'on ne devrait pas permettre ça. Qu'on devrait plutôt permettre l'utilisation de slug.

C’est demandé dans https://dev.entrouvert.org/issues/76975#note-12

> Perso je pense que ça a été accepté trop vite (y compris par moi qui répond qu'on peut réfléchir) et je pense qu'on ne devrait pas permettre ça. Qu'on devrait plutôt permettre l'utilisation de slug. C’est demandé dans https://dev.entrouvert.org/issues/76975#note-12

bdauvergne requested changes 2023-12-20 17:49:22 +01:00

combo/public/templatetags/combo.py

						
				@ -331,7 +338,15 @@ def has_role(user, groupname):

				        return False

				    if not hasattr(user, 'groups'):

				        return False

				    return user.groups.filter(name=groupname).exists()

bdauvergne commented

2023-12-20 17:49:15 +01:00

On doit pouvoir simplifier tout ça en

qs = Role.objects.filter(name=groupname)
if is_uuid(groupname):
    qs |= Role.objects.filter(uuid=grouname)
return qs.exists()

On doit pouvoir simplifier tout ça en ``` qs = Role.objects.filter(name=groupname) if is_uuid(groupname): qs |= Role.objects.filter(uuid=grouname) return qs.exists() ```

bdauvergne commented

2023-12-20 18:31:19 +01:00

À noter que ce souci de code pas trop testable restera si on remplace uuid par slug. Je conseillerai de mocker Role pour les tests (et dans le code du module faire:

try:
     from hobo.agent.common.models import Role
except ImportError:
    Role = None

def has_role(user, groupname):
      if Role:
            ....
      else:
            return user.groups.filter(name=groupname).exists()

Ensuite dans le test on peut faire un:

   with mock.patch('combo.public.templatetags.combo.Role') as Role:
       ....
      assert Role.calls == [mock.call(...), ...]

pour vérifier que les bons appels ont été fait.

À noter que ce souci de code pas trop testable restera si on remplace uuid par slug. Je conseillerai de mocker Role pour les tests (et dans le code du module faire: ``` try: from hobo.agent.common.models import Role except ImportError: Role = None def has_role(user, groupname): if Role: .... else: return user.groups.filter(name=groupname).exists() ``` Ensuite dans le test on peut faire un: ``` with mock.patch('combo.public.templatetags.combo.Role') as Role: .... assert Role.calls == [mock.call(...), ...] ```` pour vérifier que les bons appels ont été fait.

gitea/combo/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.

No reviewers

No Label

No Milestone

No Assignees

4 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/combo#137