Haie / ajout de deux booléens dans les analytics #933
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
envergo/moulinette/views.py
Outdated
|
|
||
| def log_moulinette_event(self, moulinette, context, **kwargs): | ||
| if not moulinette.is_triage_valid(): | ||
| super().log_moulinette_event(moulinette, context) |
There was a problem hiding this comment.
Est ce que ce comportement est acceptable dans un Mixin ?
There was a problem hiding this comment.
Ah ça me paraît aussi bizarre d'appeler une méthode d'une classe parente sur une mixin. Pourquoi ne pas l'avoir laissé dans la classe enfant ?
Il y a un test sur triage ici, qui vérifie avant que moulinette est sur triage et pas form :
https://github.com/MTES-MCT/envergo/pull/933/changes#diff-2eda7161f91966c43de31eb00616f3b0f93d417249644d5000ff61e14d41e2afR178
There was a problem hiding this comment.
La méthode de la classe enfant n'est pas appelée, elle est cachée par le mixin.
J'ai finalement mis l'ensemble des cas dans le Mixin, c'est de loin l'option la plus cohérente à mes yeux/
envergo/moulinette/views.py
Outdated
|
|
||
| def log_moulinette_event(self, moulinette, context, **kwargs): | ||
| if not moulinette.is_triage_valid(): | ||
| super().log_moulinette_event(moulinette, context) |
There was a problem hiding this comment.
Ah ça me paraît aussi bizarre d'appeler une méthode d'une classe parente sur une mixin. Pourquoi ne pas l'avoir laissé dans la classe enfant ?
Il y a un test sur triage ici, qui vérifie avant que moulinette est sur triage et pas form :
https://github.com/MTES-MCT/envergo/pull/933/changes#diff-2eda7161f91966c43de31eb00616f3b0f93d417249644d5000ff61e14d41e2afR178
envergo/moulinette/views.py
Outdated
| return url_with_params | ||
|
|
||
| def log_moulinette_event(self, moulinette, context, **kwargs): | ||
| if not moulinette.is_triage_valid(): |
There was a problem hiding this comment.
Je ne comprends pas pourquoi il faut logguer un événement différent en fonction de l'état de la validation de la Moulinette. Un petit commentaire ne serait pas de refus ici.
There was a problem hiding this comment.
J'ai modifié un peu la structure, et ajouté un commentaire. C'est sans doute mieux.
envergo/users/models.py
Outdated
| return f"{self.name}" | ||
|
|
||
| @classmethod | ||
| def get_type(cls, user): |
There was a problem hiding this comment.
Ça me semble peu clair et mal défini ce que signifie un « user type ». Par exemple le terme « administrator » veut-il dire avoir accès au backend (si on se réfère à la version technique du terme) ou avoir accès aux dossiers (version métier) ?
Par ailleurs, ça n'a pas vraiment de sens si on est côté Aménagement.
Cette fonction n'est utilisée que dans un cadre d'analytics, et n'est pas vraiment lié à la gestion fonctionnelle des utilisateurs. Pour cette raison, je l'aurais bien sortie de la classe User pour la déporter quelque part vers analytics.utils.get_user_type par exemple.
There was a problem hiding this comment.
Effectivement, ces labels n'ont pas beaucoup de sens nu d'un point de vue technique, ni d'un point de vue métier. Mais, à ce jour, on n'a pas vraiment d'équivalent métier ou technique qui m'aurait permis de répondre au besoin de tracking.
J'ai déplacé le tout (tel quel) dans l'app analytics pour que l'on ne soit pas tenter de réutiliser ce type en lui donnant plus de valeur que ce qu'il a vraiement.
https://trello.com/c/KhfsomuI/2014-haie-ajout-de-deux-bool%C3%A9ens-dans-les-analytics#