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

Ajout d'un système de labellisation #6462

Merged
merged 37 commits into from
Apr 27, 2023

Conversation

victorlohezic
Copy link
Contributor

Cette fonctionnalité répond au besoin suivant : Valoriser les publications de manière permanente sur le site (contrairement à la une).
Cette fonctionnalité est liée au groupe de travail et au projet GitHub associé. Pour en savoir plus, vous pouvez consulter ce sujet du forum.

Les modifications qui ont été faites sont :

  • ajout d'un modèle pour stocker les labels dans la base de données
  • ajout de la possibilité de création, gestion, suppression depuis l'interface d'administration Django
  • sur une publication, seulement pour un membre de l'équipe staff, il est possible d'ajouter, modifier, supprimer un label
  • modifications des templates pour qu'en tant qu'utilisateur pas nécessairement connecté, on peut visualiser les labels dans l'en-tête de la publicaton
  • modification des templates pour permettre l'affichage de toutes les publications portant le même label (et de filtrer en fonction des labels)
  • modification des templates et fichiers scss pour mettre en valeur les publications labellisées avec une étiquette rouge (par exemple sur la page d'accueil)
  • ajout des tests pour la navigation par labels et l'édition des labels

Pour l'organisation du code, nous nous sommes fortement inspirés de celle pour la classification des objectifs. Nous avons mis la vue et le formulaire dans un seul fichier labels.py. Les raisons sont expliquées ici.

Nous avons supposé que la majorité des publications labellisées posséderont un seul label. Par conséquent, si une publication est labellisée, nous affichons seulement son premier label.
Image pour illustrer le choix d'afficher un seul label
Note : le premier label est d'une couleur plus foncée car le curseur de la souris se trouve dessus.

Contrôle qualité

Par exemple :

  • Appliquer les migrations avec python manage.py migrate
  • Se connecter à l'interface d'administration : http://0.0.0.0:8000/admin/
  • Ajouter des labels
  • Supprimer des labels
  • Modifier des labels
  • Vérifier les étapes de modification, suppression, ajout en allant sur une publication, en se connectant en tant que staff, puis cliquer sur modifier les labels
  • Ajouter des labels à une publication
  • Vérifier qu'au dessus de la publication dans l'en-tête, le ou les labels sont présents
  • Enlever des labels, vérifier qu'ils sont bien retirés de la publication
  • Choisir une publication de la page d'accueil, lui ajouter un label
  • Vérifier que sur la page d'accueil, au dessus du tag, il y a une étiquette avec le label en rouge
  • Vérifier que si on supprime le label, cette étiquette rouge n'y est plus
  • Choisir une publication sur la page d'accueil, lui ajouter trois labels et vérifier que seulement deux tags sont affichés avec un label sur la page d'accueil
  • Ajouter un même label à plusieurs publications
  • Cliquer sur un label, soit depuis l'en-tête de la publication puis depuis une étiquette rouge
  • Vérifier que l'on accède bien à une page avec toutes les publications portant le même label
  • Modifier et supprimer le label sur certaines de ces publications
  • Vérifier que la page s'actualise correctement en ne les affichant plus

Copy link
Contributor

@Arnaud-D Arnaud-D left a comment

Choose a reason for hiding this comment

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

Il faudrait aussi retirer les mises à jour des fixtures/tuto/*, elles n'ont rien à faire là.

J'ai juste regardé rapidement le code, je n'ai pas testé.

.gitignore Outdated Show resolved Hide resolved
templates/tutorialv2/includes/labels.part.html Outdated Show resolved Hide resolved
zds/tutorialv2/migrations/0037_auto_20230213_2136.py Outdated Show resolved Hide resolved
zds/tutorialv2/models/labels.py Outdated Show resolved Hide resolved
zds/tutorialv2/models/labels.py Outdated Show resolved Hide resolved
zds/tutorialv2/models/labels.py Outdated Show resolved Hide resolved
zds/tutorialv2/tests/factories.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Feb 17, 2023

Coverage Status

Coverage: 88.376% (+0.07%) from 88.31% when pulling 0af3b88 on victorlohezic:labellisation into a1fccea on zestedesavoir:dev.

@Situphen Situphen added this to En développement in Suivi des PR via automation Feb 17, 2023
@philippemilink
Copy link
Member

Pas encore testé, mais ça a l'air pas mal !

Nous avons supposé que la majorité des publications labellisées posséderont un seul label. Par conséquent, si une publication est labellisée, nous affichons seulement son premier label.

Hmmm, est-ce que cette supposition est fondée ? On devrait pouvoir attribuer plusieurs labels à une publication. Maintenant, je comprends bien le problème pour afficher tous les labels dans les listes de contenus (comme sur la capture d'écran que vous avez mise). Qu'est-ce qu'on pourrait faire ? Quelques idées en vrac :

  • afficher tous les labels (j'ai peur que ça soit peu lisible, je pense que les labels vont être plus longs que les tags)
  • lorsqu'on attribue un label à une publication, on choisit un label "principal" qui sera celui affiché sur les listes de contenus (sans doute la solution la plus simple pour l'instant)
  • on attribue une icône à chaque label, et sur les listes de contenus on n'affiche que les icônes (il faut avoir les icônes)

Bonne idée de colorer ainsi les labels, pour les distinguer des tags (si on reste sur des labels uniquement textuels sans icône). Par contre, il serait peut-être intéressant de pouvoir spécifier la couleur pour chaque label, donc ajouter la couleur comme attribut des labels ?

(c'est des réflexions en vrac, à discuter)

@Arnaud-D
Copy link
Contributor

Pour le fait d'avoir plusieurs labels, il y a des chances que la plupart des publications n'aient effectivement qu'un seul label, même si le design devrait en accomoder plusieurs pour couvrir toutes les possibilités.

Le design actuel pose déjà des soucis de place sur les cartes. On affiche beaucoup d'information et il n'y a pas de place pour tout. Déjà actuellement, sans rajouter les labels, c'est assez dense et les titres sont régulièrement coupés par les tags. Il faudrait sûrement imaginer quelque chose de plus compact, voire sélectionner plus l'information.

C'est un boulot de design pas forcément aisé. À mon avis, ça peut très bien être un travail fait dans une seconde itération, tant que la première itération est présentable. C'est déjà ce que j'ai fait quand j'ai rajouté les objectifs, j'ai omis de faire l'intégration aux cartes.


Par contre, il serait peut-être intéressant de pouvoir spécifier la couleur pour chaque label, donc ajouter la couleur comme attribut des labels ?

Je verrais bien une solution à base de nom de classe conventionnel du type label-nom-du-label. Ensuite le CSS s'occupe du reste.

@philippemilink
Copy link
Member

Je propose d'enlever l'affichage des labels sur les cartes, et de merger une première version minimaliste comme ça. La façon dont on met en valeur les labels pourra faire l'objet d'une autre PR.

@victorlohezic
Copy link
Contributor Author

Je propose d'enlever l'affichage des labels sur les cartes, et de merger une première version minimaliste comme ça. La façon dont on met en valeur les labels pourra faire l'objet d'une autre PR.

J'ai enlevé l'affichage des labels sur les cartes. Il ne reste plus qu'à faire la QA de cette première version minimaliste.

@atman0eirb
Copy link
Contributor

Tout est OK !

@philippemilink philippemilink moved this from En développement to En attente de QA in Suivi des PR Apr 2, 2023
Copy link
Member

@philippemilink philippemilink left a comment

Choose a reason for hiding this comment

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

Beaucoup de petites choses à revoir dans le code.

Vous n'avez pas mis la possibilité d'utiliser le filtre Sans label sur la vue qui liste les contenus ayant tel label. Mais je n'arrive pas à me décider si c'est pertinent ou non d'avoir un tel filtre : il y aura forcément des contenus sans labels, et les lister n'est pas vraiment intéressant (pour le visiteur lambda, pour l'équipe de validation ça peut être intéressant pour remarquer les contenus qui mériteraient un label). Un avis @Arnaud-D ? Si on ajoute le filtre : il faut l'ajouter dans la vue ; si on ne l'ajoute pas, il y a pas mal de lignes qui sautent dans ContentByLabelMixin.

Il y a un champ Description pour les labels, mais il n'est affiché nul part, on pourrait peut-être l'ajouter au-dessus de la phrase Cette page est un aperçu de la labellisation. Elle vise à valoriser les publications de manière permanente sur le site. ?

Le fait qu'on puisse afficher la page /contenus/labels/ sans spécifier de label et que la page affiche alors toutes les publications, me paraît un peu bizarre... Je serai plutôt d'avis une URL de la forme /contenus/labels/foo/ et s'il n'y pas de label spécifié, c'est une 404.

Il faudrait aussi ajouter des fixtures, comme c'est fait pour les objectifs.

Sinon, c'est bien fonctionnel 👍

assets/scss/components/_content-item.scss Outdated Show resolved Hide resolved
fixtures/tuto/article_v1.zip Outdated Show resolved Hide resolved
fixtures/tuto/balise_audio.zip Outdated Show resolved Hide resolved
fixtures/tuto/big_tuto_v1.zip Outdated Show resolved Hide resolved
templates/tutorialv2/includes/content_item.part.html Outdated Show resolved Hide resolved
templates/tutorialv2/labels/view-labels.html Outdated Show resolved Hide resolved
templates/tutorialv2/labels/view-labels.html Outdated Show resolved Hide resolved
zds/tutorialv2/models/labels.py Outdated Show resolved Hide resolved
zds/tutorialv2/models/mixins.py Outdated Show resolved Hide resolved
Suivi des PR automation moved this from En attente de QA to Modification demandée Apr 2, 2023
@philippemilink
Copy link
Member

La CI ne passe pas parce qu'il manque des migrations :

python manage.py makemigrations

Et puisqu'on en est à la troisième migrations pour les labels dans cette PR, ce serait bien de les fusionner :

python manage.py squashmigrations --squashed-name labels tutorialv2 0037_labels 0039_auto_20230418_2206

Contrairement à ce que raconte la commande squashmigrations à la fin de son exécution, il faudra supprimer les 3 anciennes migrations, pour ne garder que celle résultant de leur fusion (personne n'a encore déployé ces migrations, donc on peut les supprimer).

@victorlohezic
Copy link
Contributor Author

La CI ne passe pas parce qu'il manque des migrations :

python manage.py makemigrations

Et puisqu'on en est à la troisième migrations pour les labels dans cette PR, ce serait bien de les fusionner :

python manage.py squashmigrations --squashed-name labels tutorialv2 0037_labels 0039_auto_20230418_2206

Contrairement à ce que raconte la commande squashmigrations à la fin de son exécution, il faudra supprimer les 3 anciennes migrations, pour ne garder que celle résultant de leur fusion (personne n'a encore déployé ces migrations, donc on peut les supprimer).

Je n'ai pas bien compris comment il fallait procéder, après avoir fait squashmigrations , il reste trois fichiers :

  • 0037_labels (qui contient la fusion des migrations)
  • 0038_labels_without_position
  • 0039_auto_20230420_2252

Je supprime les migrations dont les noms commencent par 0038_ et 0039_ .

Ensuite, j'applique les migrations avec la commande python3 manage.py migrate. Mais j'obtiens l'erreur suivante :

ValueError: The field tutorialv2.PublishableContent.helps was declared with a lazy reference to 'utils.helpwriting', but app 'utils' doesn't provide model 'helpwriting'.
The field tutorialv2.PublishableContent_helps.helpwriting was declared with a lazy reference to 'utils.helpwriting', but app 'utils' doesn't provide model 'helpwriting'.

@philippemilink philippemilink moved this from Modification demandée to En attente de QA in Suivi des PR Apr 21, 2023
Copy link
Member

@philippemilink philippemilink left a comment

Choose a reason for hiding this comment

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

C'est fonctionnel.

Quelques petits détails à corriger dans le code, et je merge.

zds/tutorialv2/tests/tests_views/tests_editlabels.py Outdated Show resolved Hide resolved
zds/tutorialv2/views/labels.py Outdated Show resolved Hide resolved
templates/tutorialv2/labels/view_labels.html Outdated Show resolved Hide resolved
templates/tutorialv2/labels/view_labels.html Outdated Show resolved Hide resolved
zds/tutorialv2/views/labels.py Outdated Show resolved Hide resolved
Suivi des PR automation moved this from En attente de QA to Modification demandée Apr 22, 2023
@abertin78
Copy link
Contributor

Nous avons fini les modifications demandées.

Copy link
Member

@philippemilink philippemilink left a comment

Choose a reason for hiding this comment

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

3 points restants et on est bon ! On va finir par converger vers le merge :)

@victorlohezic : ne marque pas toi-même les conversations comme résolues, laisse celui qui a entamé la discussion s'assurer que c'est vraiment résolu :) Sinon là c'est le bazar pour m'y retrouver dans les remarques que j'avais faite et voir si vous les avez correctement traitées.

@abertin78
Copy link
Contributor

J'ai fait les modifications demandées :)

Copy link
Member

@philippemilink philippemilink left a comment

Choose a reason for hiding this comment

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

QA OK ✔️

Suivi des PR automation moved this from Modification demandée to Fusionnable après rebase Apr 27, 2023
@philippemilink philippemilink enabled auto-merge (squash) April 27, 2023 20:18
@philippemilink philippemilink merged commit e3da299 into zestedesavoir:dev Apr 27, 2023
Suivi des PR automation moved this from Fusionnable après rebase to Fusionnée Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Suivi des PR
  
Fusionnée
Development

Successfully merging this pull request may close these issues.

None yet

6 participants