Produits Entr'ouvert » Authentic 2

• 0003 dur à relire malgré la docstring qui explique l’usager de la fixture créée. Je suis descendu dans les patches suivants pour comprendre comment ça s’utilisait, ok.
• dans les signatures de fonction, la délimitatian arguments positionnels, /, arguments mixtes, *, arguments par mot-clé -> on déprécie de fait buster (car pas compatible python < 3.8). J’ai l’impression que c’est trop tôt, non ?
• 0005 le nouveau fichier authentic2/utils/api.py, j’aurais plutôt vu un dossier authentic2/api/{utils, views, urls}.py (qui contiendrait les anciens api_views et api_urls), mais ok pourquoi pas comme ça.
• 0006 dans la nouvelle classe DjangoRBACPermission :
  

      def __call__(self):
          return self
  

  -> j’ai loupé l’endroit où ça nous était utile (?)
• 0006 toujours, pourquoi dans test_method_delete, dans la fonction lambda on a besoin de rajouter "and obj is mock_obj" contrairement aux autres tests. Je ne vois pas à la relecture du tests les cas où cette condition serait fausse.
• 0007 : peut-être un test pour chacune des deux méthodes ajoutées ? :)
  • Et du détail, mais peut-être inverse les deux if dans chacune des deux méthodes créées ? Le premier if va créer l’opération pour rien si le second if échoue (rbac_utils.get_operation fait un get_or_create).
• 0008 du détail encore : est-ce qu’on aurait pas intérêt à mettre direct is not None et annotate=True mutuellement exclusif ? On s’en fiche de l’annotation dans ce cas ci, c’est seulement lorsque direct is None que cela nous intéresse.
• 0009 :
  • url(r'^roles/(?P<role_uuid>[0-9a-z]{32})/[…]') mais juste en dessous url(r'^roles/(?P<role_uuid>[\w+]*)/[…]') -> pourquoi cette variation dans le format du paramètre d’uuid de rôle accepté ?
  • RolesParentsAPI.get : direct = None if 'all' in self.request.GET else True -> pas de moyen d’avoir les membres indirects seulement en un seul appel, est-ce grave ? (On peut décider que non, osef).
  • même remarque pour RolesParentsRelationshipsAPI.

Paul Marillonnet a écrit :

Ok, une première relecture :

• 0003 dur à relire malgré la docstring qui explique l’usager de la fixture créée. Je suis descendu dans les patches suivants pour comprendre comment ça s’utilisait, ok.
• dans les signatures de fonction, la délimitatian arguments positionnels, /, arguments mixtes, *, arguments par mot-clé -> on déprécie de fait buster (car pas compatible python < 3.8). J’ai l’impression que c’est trop tôt, non ?

Ça ne concerne pour l'instant qu'une fonction dans les tests, on a python3.9 sur jenkins.entrouvert.org, ça ne me semble pas gênant, après peut-être y-a-t-il encore des gens en buster sur leur bécane mais ça me parait une bonne occasion de monter en bullseye pour eux.

• 0005 le nouveau fichier authentic2/utils/api.py, j’aurais plutôt vu un dossier authentic2/api/{utils, views, urls}.py (qui contiendrait les anciens api_views et api_urls), mais ok pourquoi pas comme ça.

Je vais laisser ça là pour l'instant mais je n'ai rien contre du rangement autour des APIs.

• 0006 dans la nouvelle classe DjangoRBACPermission :
  [...] -> j’ai loupé l’endroit où ça nous était utile (?)

En fait django-rest-framework attend un constructeur dans permission_classes, et donc pour utiliser une instance, histoire d'avoir un truc configurable et pas statique comme les autres permissions genre IsAuthenticated il faut qu'elle soit appelable, c'est ça ou passer functools.partial(DjangoRBACPermission, <paramétrage>), ça me semble plus joli comme ça.

• 0006 toujours, pourquoi dans test_method_delete, dans la fonction lambda on a besoin de rajouter "and obj is mock_obj" contrairement aux autres tests. Je ne vois pas à la relecture du tests les cas où cette condition serait fausse.

Je teste les permissions sur les objets, alors je vérifie que DjangoRBACPermission appelle bien request.user.has_perms en passant l'objet.

• 0007 : peut-être un test pour chacune des deux méthodes ajoutées ? :)

Ok, fait sur la branche.

• Et du détail, mais peut-être inverse les deux if dans chacune des deux méthodes créées ? Le premier if va créer l’opération pour rien si le second if échoue (rbac_utils.get_operation fait un get_or_create).

Ok, fait sur la branche.

• 0008 du détail encore : est-ce qu’on aurait pas intérêt à mettre direct is not None et annotate=True mutuellement exclusif ? On s’en fiche de l’annotation dans ce cas ci, c’est seulement lorsque direct is None que cela nous intéresse.

Ok, fait sur la branche, avec des tests en plus dans tests/test_a2_rbac.py.

• 0009 :
  • url(r'^roles/(?P<role_uuid>[0-9a-z]{32})/[…]') mais juste en dessous url(r'^roles/(?P<role_uuid>[\w+]*)/[…]') -> pourquoi cette variation dans le format du paramètre d’uuid de rôle accepté ?

Aucune raison, corrigé sur la branche.

• RolesParentsAPI.get : direct = None if 'all' in self.request.GET else True -> pas de moyen d’avoir les membres indirects seulement en un seul appel, est-ce grave ? (On peut décider que non, osef).

Je ne vois pas de cas d'usage, en général on veut les enfants direct (pour la gestion) ou tout (pour l'audit).

• même remarque pour RolesParentsRelationshipsAPI.

Même réponse.

Branche à jour.

Automatic expiration