Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Création d'un historique de sanctions pour le staff #4177

Closed
wants to merge 5 commits into from
Closed

Création d'un historique de sanctions pour le staff #4177

wants to merge 5 commits into from

Conversation

gllmc
Copy link
Member

@gllmc gllmc commented Feb 5, 2017

Q R
Type de modification nouvelle fonctionnalité

Cette pull request crée un historique de sanctions pour le staff qui permet de voir les actions qui ont été faites et à quelle date. Elle utilise le modèle Ban qui a toujours été utilisé pour archiver les sanctions (en effet, il n'est pas utilisé pour vérifier qu'un membre a le droit de se connecter/de poster), mais qui n'était jamais affiché.

QA

  • Avec un compte staff, essayer plusieurs sanctions sur un utilisateur (bannir, débannir, mettre en lecture seule, etc) ;
  • Cliquer sur le lien Historique dans le sous-menu Sanctions et vérifier que les actions effectuées ont bien été répertoriées ;
  • Vérifier que cette page n'est accessible qu'aux membres du staff (c'est couvert par les tests de toute façon) ;
  • Enfin, vérifier que les tests zds.member.tests.tests_views.MemberTests passent toujours.

@coveralls
Copy link

coveralls commented Feb 5, 2017

Coverage Status

Coverage increased (+0.005%) to 88.368% when pulling fbc31e4 on GCodeur:historique_sanctions into 54e11ef on zestedesavoir:dev.

@Eskimon
Copy link
Contributor

Eskimon commented Feb 7, 2017

Dans le profil d'un membre il existe(ait?) déjà une liste visible uniquement par le staff qui sert de "note de karma" pour le membre. Par exemple les staffeux peuvent y ajouter des notes sur le comportement du membre (s'il fait des multis, à tendance à insulter etc). Pourquoi ne pas fusionner ton idée avec cette liste qui éviterait de créer de nouvelles pages et ainsi rendrait plus rapide la lecture d'un profil ?

@gllmc
Copy link
Member Author

gllmc commented Feb 7, 2017

il existe(ait?)

Existe, je confirme. ;)

J'en avais parlé un peu avec @artragis sur IRC qui avait en effet évoqué l'existence du système de karma, mais qui avait trouvé toutefois cette liste utile (les informations ne sont en soi pas les mêmes que celles du karma).

Cependant, je trouve aussi que fusionner ces deux listes pourrait être pratique (on voit direct tout ce qui s'est passé sans devoir switcher entre les deux pages). À voir avec les besoins du staff.

Il reste à voir comment on fait ça techniquement. L'objet Ban existe il me semble depuis le début du site, donc il était facile de rajouter un simple historique. Pour fusionner les deux, c'est plus compliqué, je vois mal comment créer une liste avec deux types d'objets différents, le tout classé par date. On peut aussi créer une note de karma automatiquement lors d'une sanction, mais c'est dupliquer les informations. :/

Par contre, je peux déjà mettre cet historique en bas de la page de profil plutôt que dans une page séparée, ça simplifiera sans doute les choses.

@pierre-24 pierre-24 added C-Back Concerne le back-end Django QA svp labels Feb 8, 2017
@pierre-24
Copy link
Member

Rapport de QA: ok jusqu'ici. Par contre, je suis d'accord avec toi pour mettre le karma sur la même page (sous forme de tableau aussi). Et du coup, je remonterai le bouton pour le mettre en dessous de "promouvoir", qu'il soit facile d'accès :)

@gllmc
Copy link
Member Author

gllmc commented Feb 8, 2017

Du coup, on partirait sur une unique page nommée Modération ? Est-ce qu'on déplace aussi les sanctions en elle-même tant qu'à faire ?


user = get_object_or_404(User, pk=user_pk)

sanctions = Ban.objects.filter(user=user).order_by('-pubdate').select_related('moderator')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pour une raison qui m'échappe, c'est plus performant prefetch_related() (voir @vhf )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je suis un peu étonné pour le coup vu que prefetch_related fait une seconde requête, tandis que select_related fait une jointure SQL afin de tout faire en une seule et unique requête. :/

Je trouve que prefetch_related est plus adapté pour récupérer une liste d'objets rattachés à une objet. Exemple : récupérer la liste des alertes pour chaque commentaire (doc).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ce que je croyais aussi, jusqu'à ce que @vhf démontre le contraire : #4096 (comment)

follow=False
)

self.assertContains(result, 'Test de LS')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tu peux même aller plus loin et utiliser result.context['sanctions'] pour vérifier ce que la liste de sanctions contient effectivement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bonne idée :)

@pierre-24
Copy link
Member

pierre-24 commented Feb 8, 2017

Du coup, on partirait sur une unique page nommée Modération ?

Comme tu veux, moi j'aimais bien "historique" aussi :)

Est-ce qu'on déplace aussi les sanctions en elle-même tant qu'à faire ?

Pourquoi pas ? (mais alors, le nom "modération" a effectivement plus de sens)

@coveralls
Copy link

coveralls commented Feb 8, 2017

Coverage Status

Coverage increased (+0.004%) to 88.367% when pulling e39fe7b on GCodeur:historique_sanctions into 5582cd3 on zestedesavoir:dev.

@vhf
Copy link
Contributor

vhf commented Feb 10, 2017

Je vais pas insister ou être lourd avec ça, mais je préférerais qu'on affiche cet historique directement dans la liste des "notes" de karma.

Ça me semble facile mais je me trompe peut-être. En gros sur la page profile, on a une liste des notes de karma. On récupère ces notes. On récupère l'historique dont il est question ici. On merge les deux listes d'objets (pas grave que ce soient pas les mêmes objets), on sort d'après la date, on passe au template qui l'affiche.

Faisable @GCodeur ? Qu'en penses-tu ?

@gllmc
Copy link
Member Author

gllmc commented Feb 10, 2017

Je pense que oui. J'avais pensé à ça, mais je m'étais dit que ce serait assez moche niveau code, mais c'est pas forcément le cas. Juste quelques questions :

On merge les deux listes d'objets

Donc on passe à un objet list ?

on sort d'après la date

Pour être franc, je n'avais aucune idée de comment faire ça, mais je viens de chercher et ai trouvé ça. Les deux objets ayant un champ date appelé pubdate, je devrais pouvoir me débrouiller avec.

on passe au template qui l'affiche.

Tu verrais ça sous la forme d'une liste ou d'un tableau (les deux me paraissent possibles) ? Et pour identifier si l'objet est un Ban ou une KarmaNote, on testerait un attribut (genre karma) ou on ajouterait un attribut type lors du traitement en Python ? Je ne sais pas ce qui est le plus propre.

@vhf
Copy link
Contributor

vhf commented Feb 10, 2017

Donc on passe à un objet list ?

Yep. En gros par défaut un QuerySet est lazy. C'est super parce que ça permet d'éviter que la requête soit effectivement faite jusqu'au dernier moment, quand Django est certain qu'on a besoin de faire la requête.

Un moyen de forcer l'évaluation d'un QuerySet, c'est de faire list() dessus. Genre list(Foo.objects.filter(…)).

Ici, ça n'a pas d'impact négatif, on sait qu'on a besoin de ces données, forcer l'évaluation du QuerySet ne pose pas de problème. (Faut juste faire gaffe à ne faire ça que si la personne regardant la page a bien le droit de voir ces données, comme ça y'a pas d'impact sur les autres visiteurs.)

je viens de chercher et ai trouvé ça

👍

Tu verrais ça sous la forme d'une liste ou d'un tableau

Plutôt tableau à mon avis.

Et pour identifier si l'objet est un Ban ou une KarmaNote, on testerait un attribut (genre karma) ou on ajouterait un attribut type lors du traitement en Python

J'ai pas d'avis, fais ce que tu trouves le mieux. Si ça me semble pas bien et que je vois un moyen simple de rendre ça plus propre, j'hésiterai pas à te le dire. Mais là, sans implémentation, difficile de te conseiller. :)

@Eskimon
Copy link
Contributor

Eskimon commented Feb 10, 2017

Merci @vhf d'avoir exprimé ce que je ne savais pas comment dire !

@gllmc gllmc closed this Feb 11, 2017
@gllmc gllmc removed the QA svp label Apr 3, 2017
@gllmc gllmc deleted the historique_sanctions branch July 17, 2017 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Back Concerne le back-end Django
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants