Project

General

Profile

Bug #68390

statistiques : plus possible de filtrer par service

Added by Valentin Deniaud over 2 years ago. Updated about 2 years ago.

Status:
Fermé
Priority:
Normal
Assignee:
Benjamin Dauvergne
Category:
-
Target version:
-
Start date:
24 August 2022
Due date:
% Done:

0%

Estimated time:
Patch proposed:
Yes
Planning:
No

Description

Du moins depuis février.

#64853 a corrigé seulement un bout du problème (et ce ticket est pas sympa parce que pas de lien vers le ticket qui introduit le bug et patch qui mélange mille choses).

Files

0001-statistics-lookup-service-also-with-their-subclass-r.patch (5.63 KB) 0001-statistics-lookup-service-also-with-their-subclass-r.patch Benjamin Dauvergne, 12 September 2022 10:39 PM
0001-statistics-lookup-service-also-with-their-subclass-r.patch (5.74 KB) 0001-statistics-lookup-service-also-with-their-subclass-r.patch Benjamin Dauvergne, 13 September 2022 10:22 AM

Associated revisions

Revision 21249f84 (diff)
Added by Benjamin Dauvergne about 2 years ago

statistics: lookup service also with their subclass reference_id (#68390)

Revision 1016ca06 (diff)
Added by Benjamin Dauvergne about 2 years ago

journal: remove unused service parameter (#68390)

Revision 467bb3c7 (diff)
Added by Benjamin Dauvergne about 2 years ago

journal: log references to the base Service instance (#68390)

History

#4

Updated by Benjamin Dauvergne over 2 years ago

Valentin Deniaud a écrit :

Du moins depuis février.

#64853 a corrigé seulement un bout du problème (et ce ticket est pas sympa parce que pas de lien vers le ticket qui introduit le bug et patch qui mélange mille choses).

Je ne sais pas quel ticket introduit le bug, tu peux le rajouter si tu sais. Par ailleurs si tu sais quel bout du problème il reste à corriger tu peux l'écrire aussi ici. Ce ticket est encore plus cryptique pour l'instant.

#6

Updated by Valentin Deniaud over 2 years ago

Benjamin Dauvergne a écrit :

Ce ticket est encore plus cryptique pour l'instant.

Effectivement la description du bug ici est succinte mais le ticket lié est limpide.

Par ailleurs si tu sais quel bout du problème il reste à corriger tu peux l'écrire aussi ici.

Le problème c'est que « content_type_id peut parfois être l'id du modèle ContentType pour OIDCClient ou LibertyProvider », le ticket que je pointe a corrigé l'endroit où on groupe par service, il reste à corriger l'endroit où on filtre par service.

#9

Updated by Benjamin Dauvergne over 2 years ago

Valentin Deniaud a écrit :

Par ailleurs si tu sais quel bout du problème il reste à corriger tu peux l'écrire aussi ici.

Le problème c'est que « content_type_id peut parfois être l'id du modèle ContentType pour OIDCClient ou LibertyProvider », le ticket que je pointe a corrigé l'endroit où on groupe par service, il reste à corriger l'endroit où on filtre par service.

C'est normal il est ouvert suite à un problème concernant l'agrégation par service pas le filtrage, il n'a effectivement résolu que ça et je ne vois pas en quoi il a cassé le filtrage ou alors le filtrage était déjà cassé ou bien je ne comprends pas.

#10

Updated by Benjamin Dauvergne over 2 years ago

  • Assignee set to Benjamin Dauvergne
#11

Updated by Benjamin Dauvergne over 2 years ago

J'ai trouvé la raison du changement en février, la façon dont l'objet service est journalisé a changé à ce moment, de la classe authentic2.models.Service on est passé au sous-classes des différents modules (OIDCClient, LibertyProvider) ce qui a provoqué cette cassure. Comme on a désormais un mélange de deux types de référence j'ai préféré ajouté la recherche des deux ids; j'aurai pu faire une migration mais ça fait beaucoup de lignes à réécrire pour un web-service peu utilisé.

#13

Updated by Valentin Deniaud over 2 years ago

(c'est rouge)

Benjamin Dauvergne a écrit :

j'aurai pu faire une migration mais ça fait beaucoup de lignes à réécrire pour un web-service peu utilisé

Dommage, là ça complexifie encore un peu plus un code qui est déjà illisible (je peux parler c'est moi qui l'ait écrit).

Comme on a désormais un mélange de deux types de référence j'ai préféré ajouté la recherche des deux ids

Du coup ça mérite peut-être l'ajout d'un commentaire à l'endroit où on récupère ces deux ids ?

            return functools.reduce(
                operator.or_,
                (cls._which_references_query(ref) for ref in instance_or_model_class_or_queryset),
            )

C'est pas équivalent à return any(cls._which_references_query(ref) for ref in instance_or_model_class_or_queryset) ?

Je découvre aussi la dépendance à django-model-utils, si j'avais su je me serais pas embêté à réinventer tout ça dans apps/authenticators/query.py.

#14

Updated by Benjamin Dauvergne over 2 years ago

  • Status changed from Solution proposée to En cours

Valentin Deniaud a écrit :

(c'est rouge)

Oui, j'étais perdu sur lasso.

Benjamin Dauvergne a écrit :

j'aurai pu faire une migration mais ça fait beaucoup de lignes à réécrire pour un web-service peu utilisé

Dommage, là ça complexifie encore un peu plus un code qui est déjà illisible (je peux parler c'est moi qui l'ait écrit).

Ouais, je vais mettre des comentaires mais dans l'immédiat je préfère ne pas toucher aux données.

Comme on a désormais un mélange de deux types de référence j'ai préféré ajouté la recherche des deux ids

Du coup ça mérite peut-être l'ajout d'un commentaire à l'endroit où on récupère ces deux ids ?

Ouaip.

[...]
C'est pas équivalent à return any(cls._which_references_query(ref) for ref in instance_or_model_class_or_queryset) ?

C'est pas le même opérateur (https://docs.python.org/3/library/operator.html#operator.__or__, "or" != "|").

Je découvre aussi la dépendance à django-model-utils, si j'avais su je me serais pas embêté à réinventer tout ça dans apps/authenticators/query.py.

Je découvre ce query.py :)

#15

Updated by Benjamin Dauvergne over 2 years ago

  • Status changed from En cours to Solution validée

Voilà, j'ai quand même restauré le comportement précédent (j'enregistre la référence vers le ContentType de authentic2.models.Service); on pourra voir pour ne pas aggréger via service_name dans un autre ticket.

#16

Updated by Benjamin Dauvergne over 2 years ago

  • Status changed from Solution validée to Solution proposée
#17

Updated by Valentin Deniaud about 2 years ago

  • Status changed from Solution proposée to Solution validée

Looks good

Benjamin Dauvergne a écrit :

Je découvre aussi la dépendance à django-model-utils, si j'avais su je me serais pas embêté à réinventer tout ça dans apps/authenticators/query.py.

Je découvre ce query.py :)

Je pense que tu avais aussi oublié cette dépendance quand tu as écrit #53902#note-11.

#18

Updated by Benjamin Dauvergne about 2 years ago

  • Status changed from Solution validée to Résolu (à déployer)
commit 467bb3c7dfac7f109c63ee243598faf9481f1f84
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Tue Sep 13 11:20:48 2022 +0200

    journal: log references to the base Service instance (#68390)

commit 1016ca0647ae0aec796cd575ae0288f0e0aff0bd
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Tue Sep 13 11:19:00 2022 +0200

    journal: remove unused service parameter (#68390)

commit 21249f84bb8b5e6190060570d690a12194f6cd26
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Mon Sep 12 22:36:54 2022 +0200

    statistics: lookup service also with their subclass reference_id (#68390)
#19

Updated by Benjamin Dauvergne about 2 years ago

  • Tracker changed from Développement to Bug
#20

Updated by Transition automatique about 2 years ago

  • Status changed from Résolu (à déployer) to Solution déployée
#21

Updated by Transition automatique about 2 years ago

Automatic expiration

Also available in: Atom PDF