wip/63368-Clarifier-les-statistiques (#63368) #1

vdeniaud · 2023-02-22T09:50:35+01:00

vdeniaud commented

2023-02-22 09:50:35 +01:00

Trois patches préliminaires, puis un qui ajoute les nouveaux endpoints plus clairs et déprécie les autres, puis un dernier pour ajouter les totaux globaux.

Le code est assez moche mais je nettoierai ça dans quelques mois quand j'enlèverai pour de bon les endpoints dépréciés.

Trois patches préliminaires, puis un qui ajoute les nouveaux endpoints plus clairs et déprécie les autres, puis un dernier pour ajouter les totaux globaux. Le code est assez moche mais je nettoierai ça dans quelques mois quand j'enlèverai pour de bon les endpoints dépréciés.

vdeniaud added 3 commits 2023-02-22 09:50:35 +01:00

9e232c337e api: split statistics filters code (#63368 )

2b3cd08e42 tests: do not check whole statistics json (#63368 )

104b33fb88 api: drop legacy OU api filter (#63368 )

fpeters changed title from ~~wip/63368-Clarifier-les-statistiques~~ to wip/63368-Clarifier-les-statistiques (#63368)

2023-02-22 10:04:13 +01:00

vdeniaud force-pushed wip/63368-Clarifier-les-statistiques from 9dec5ac443 to 7f6785d3c2

2023-02-22 10:44:51 +01:00

Compare

pmarillonnet requested changes 2023-02-23 12:16:14 +01:00

src/authentic2/api_views.py Outdated

						
				@ -1687,3 +1728,3 @@

				        }

				        allowed_filters = getattr(self, self.action).filters

				        if not method:

pmarillonnet commented

2023-02-23 12:05:58 +01:00

Ici il y a du code posé dans 0004 et qui est modifié dans 0005 sans que ce soit lié à l’objet du commit, il me semble. Peut-être tout déplacer directement dans 0004 ?

vdeniaud marked this conversation as resolved

src/authentic2/api_views.py Outdated

						
				@ -1690,0 +1730,4 @@

				        if not method:

				            if data.get('group_by'):

				                method = {

				                    'method': 'get_method_statistics',

pmarillonnet commented

2023-02-23 12:04:51 +01:00

Ici question de lisibilité du code, j’ai pas capté tout de suite à la relecture qu’il y deux method, la méthode d’authentification qui est l’une des options possibles pour la méthode de regroupement. Peut-être mettre authn_method pour nommer la première.

Ici question de lisibilité du code, j’ai pas capté tout de suite à la relecture qu’il y deux `method`, la méthode d’authentification qui est l’une des options possibles pour la méthode de regroupement. Peut-être mettre `authn_method` pour nommer la première.

vdeniaud commented

2023-02-23 15:26:19 +01:00

Renommé en « authentication_type » ce qui correspond à la valeur verbeuse du choix en question.

src/authentic2/api_views.py Outdated

						
				@ -1690,0 +1735,4 @@

				                    'service_ou': 'get_service_ou_statistics',

				                }[data['group_by']]

				            else:

				                method = 'get_global_statistics'

pmarillonnet commented

2023-02-23 12:14:06 +01:00

Il me semble qu’il n’y a rien dans le ChoiceField DRF qui impose que la valeur par défaut fasse partie des choix possibles. Alors peut-être qu’on pourrait directement inclure ce 'get_global_statistics' comme valeur par défaut à la définition du champ StatisticsSerializer.group_by (et du coup le required=False est implicite, pas besoin de le préciser) ?

Il me semble qu’il n’y a rien dans le ChoiceField DRF qui impose que la valeur par défaut fasse partie des choix possibles. Alors peut-être qu’on pourrait directement inclure ce `'get_global_statistics'` comme valeur par défaut à la définition du champ `StatisticsSerializer.group_by` (et du coup le required=False est implicite, pas besoin de le préciser) ?

vdeniaud commented

2023-02-23 15:26:45 +01:00

Bonne idée, merci pour le niveau d'indentation en moins !