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

Refactorise les templates des publications #6457

Merged
merged 16 commits into from
Oct 14, 2023

Conversation

Arnaud-D
Copy link
Contributor

@Arnaud-D Arnaud-D commented Jan 26, 2023

Fix #5906.

Cette PR refactorise les templates des publications.

Il y a une part de mise à jour de l'expérience utilisateur.

  • J'ai rationalisé un peu sans toucher au design. J'ai réorganisé les menus pour séparer les actions de bêta, celles concernant la version en ligne, la validation, etc. Je pense que c'est plus facile à naviguer qu'avant.
  • J'ai corrigé aussi les boutons précédent/suivant/sommaire, dont le comportement était bizarre parfois.

Sinon, il y a tout un pan de technique. L'objectif est de faciliter la création des liens de partage à terme, ce qui était impratiquable avec l'organisation actuelle. On a désormais :

  • seulement deux templates (content ou container) pour tout ce qui montre un contenu, quelque soit son état (draft, bêta, version en validation, version en ligne, version spécifque, et à l'avenir lien de partage).
  • ces templates sont configurables, pour adapter l'affichage à la vue. Par exemple, on ne montre pas les boutons d'édition en dehors de la vue draft ou on n'affiche pas le lien vers la version brouillon si on y est déjà. Ça marche bien en pratique, vu que j'ai pu tester la maintenabilité en corrigeant mes bugs au cours du développement.
  • la configuration se fait au niveau de la vue, avec beaucoup d'options possibles qui apporte de la flexibilité, sans être aussi complexe que ce qu'il y avait avant.
  • j'ai refait la hiérarchie des vues pour que ça soit plus facile à suivre et supprimer en partie la redondance entre templates voisins.
  • il y a nouvelle vue spécifique pour voir une version par son commit (auparavant, c'était à moitié mélangé avec la vue draft et générateur de bugs et complications dans le code).

Il y a des choses qu'on pourrait faire pour aller plus loin, mais que je ne ferais pas dans cette PR.

  • Par exemple, beaucoup d'affichages sont basés sur is_staff. Cette information est souvent pas très précise, et on devrait utiliser des véritables permissions sur les vues (quitte à réecrire has_permission), afin d'éviter d'avoir la logique d'affichage indépendante de celle des actions associées. Actuellement, on a des bugs à cause de ça.
  • Il y a de la duplication par exemple entre child et child_online ou au sein même des templates ou j'ai intégré des choses très similaires en parallèle. Ça peut être fait dans un deuxième temps, je pense.

Des performances

J'ai regardé le nombre de requêtes SQL, vu que le reste est très volatil et dur à évaluer.

La PR introduit des requêtes en plus sur certaines vues, mais c'est dur de trancher sur l'impact réel en termes de performance. Les requêtes ajoutées sont petites individuellement. Il y a aussi un compromis performance/simplicité du code (en l'état de la PR au moment où j'écris en tout cas). Le code est beaucoup plus simple qu'aurapavant, mais avec du code exécuté mais dont le résultat n'est pas forcément utilisé.

Si un besoin d'amélioration des performances est avéré, on peut envisager de faire le travail.

Pour référence, un template vide sur l'environnement de dév génère 11 requêtes d'après la barre d'outis de Django.

Page Pre-PR Post-PR Delta
Page publique, deconnecté 24 requêtes 32 requêtes +8
Page publique, connecté en tant qu'admin 54 requêtes 62 requêtes +8
Brouillon, connecté en tant qu'auteur 42 requêtes 47 requêtes +5

Contrôle qualité

Ce qui est en dessous n'est pas une description exhaustive, mais ça donne déjà des indications sur ce qu'il est utile de regarder.

  • les pages publiques (accueil d'un contenu et conteneurs)
    • en tant qu'utilisateur déconnecté
    • en tant que simple membre connecté
    • en tant qu'auteur de la publication
    • en tant que staff ou admin
  • les pages brouillons (accueil d'un contenu et conteneurs)
    • vérifier l'absence d'accès pour les déconnectés et simples membres
    • tester en tant qu'auteur
    • tester en tant qu'admin
  • les pages bêta (accueil d'un contenu et conteneurs)
    • vérifier l'absence d'accès pour les déconnectés
    • tester en tant que simple membre
    • tester en tant qu'auteur
    • tester en tant qu'admin
  • les pages de version spécifque (depuis l'historique des versions) (accueil d'un contenu et conteneurs)
    • vérifier l'absence d'accès pour les déconnectés et simples membres
    • tester en tant qu'auteur
    • tester en tant qu'admin

Choses à regarder sur les différentes pages :

  • la bonne navigation au sein du contenu (pas de liens qui pointent vers des mauvaises version, que ce soit dans le sommaire, dans le contenu ou dans le fil d'Ariane) ;
  • l'affichage des bonnes options dans la colonne de gauche, en particulier en fonction de la page affichée, du type de contenu et des droits (par exemple pas d'actions de modération pour les non-staff) ;
  • l'affichage des bonnes options en fonction du statut de validation/beta/publication du contenu ;
  • l'affichage des boutons d'édition sur les pages de brouillons (éditer le contenu, les parties, ajouter des sections, etc.) mais pas ailleurs ;
  • l'affichage des éléments spécifques sur la version en ligne (liens des réseaux sociaux, commentaires, contenus suggérés...)

C'est intéressant aussi de vérifier que les auteurs de la version publique et brouillon ne sont pas confondus quand ils sont différents.

@Arnaud-D Arnaud-D added C-Front Concerne l'interface du site C-Back Concerne le back-end Django labels Jan 26, 2023
@Arnaud-D Arnaud-D force-pushed the refacto-template-content branch 5 times, most recently from 9a464da to 11eb34a Compare January 26, 2023 14:29
@Arnaud-D Arnaud-D force-pushed the refacto-template-content branch 8 times, most recently from 44568b7 to 528864e Compare January 31, 2023 14:53
@coveralls
Copy link

coveralls commented Jan 31, 2023

Coverage Status

coverage: 88.801% (+0.2%) from 88.559% when pulling 710cf37 on Arnaud-D:refacto-template-content into bee600b on zestedesavoir:dev.

@Arnaud-D Arnaud-D force-pushed the refacto-template-content branch 3 times, most recently from 3d3b1e4 to a162ade Compare February 9, 2023 15:37
@Arnaud-D Arnaud-D marked this pull request as ready for review February 9, 2023 15:40
Copy link
Member

@artragis artragis left a comment

Choose a reason for hiding this comment

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

J'ai eu un peu de temps pour passer sur quelques requêtes comme tu les évoques dans ton message initial.

Sur la globalité, je nai rien vu de choquant mais bon 85 fichiers c'est complexe à review.

zds/tutorialv2/views/display/content.py Outdated Show resolved Hide resolved
zds/tutorialv2/models/database.py Outdated Show resolved Hide resolved
@Arnaud-D
Copy link
Contributor Author

Arnaud-D commented Feb 14, 2023

J'ai trouvé le plus gros coupable pour mon augmentation du nombre de requêtes. J'avais une méthode is_author qui tapait la base de donnée à chaque fois qu'elle était appelée (9 fois). J'ai trouvé une autre solution beaucoup plus avantageuse et qui utilise des données déjà prefetchées et n'introduit donc aucune nouvelle requête.

J'ai aussi au passage fait une partie des suggestions d'artragis et changé d'autres bricoles non introduites par ma PR, mais qui ne mangent pas de pain.

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.

J'ai parcouru le code modifié, pas les nouveaux fichiers. Pas encore testé, mais j'ai remarqué quelques petites choses à changer.

(faut vraiment éviter de faire des PRs aussi énormes (au moins en un seul commit), c'est un enfer à QA...)

templates/tutorialv2/includes/sidebar/summary.part.html Outdated Show resolved Hide resolved
href="{{ subchild.get_absolute_url }}{% if version %}?version={{ version }}{% endif %}"
{% endif %}
{% endif %}
href="{{ base_url }}{{ child.slug }}/{{ subchild.slug }}/"
Copy link
Member

Choose a reason for hiding this comment

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

Tu n'oublies pas beaucoup de cas là ? (les online, content.is_beta, et not subchild.text)

Copy link
Contributor Author

@Arnaud-D Arnaud-D Apr 17, 2023

Choose a reason for hiding this comment

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

Je ne crois pas. Les cas online et beta sont inclus dans base_url normalement, pour le dernier, c'est dur à dire de tête. C'est tout l'intérêt de la PR de déplacer cette logique plus en amont. Le juge de paix, c'est de vérifier le comportement de ces liens en faisant les tests.

templates/tutorialv2/validation/index.html Outdated Show resolved Hide resolved
templates/tutorialv2/view/container.html Show resolved Hide resolved
templates/tutorialv2/view/content.html Show resolved Hide resolved
zds/tutorialv2/models/mixins.py Outdated Show resolved Hide resolved
zds/tutorialv2/models/versioned.py Outdated Show resolved Hide resolved
zds/tutorialv2/views/validations_contents.py Outdated Show resolved Hide resolved
zds/tutorialv2/views/validations_contents.py Outdated Show resolved Hide resolved
@philippemilink
Copy link
Member

Tu peux corriger le conflit, rebaser sur dev et corriger le test qui plante, stp ?

@Arnaud-D Arnaud-D force-pushed the refacto-template-content branch 2 times, most recently from 187a5a8 to 7c2b864 Compare September 30, 2023 06:29
@Arnaud-D
Copy link
Contributor Author

Voilà, les tests sont corrigés !

* mise à jour de l'organisation des différentes actions possibles
* fusion des variantes en ligne et pas en ligne
* configuration définie dans la vue pour adapter l'affichage
* vues bien séparées pour bêta, brouillon, en ligne
* nouvelle vue spécifique pour voir une version par son commit
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.

Comme discuté lors de la dernière réunion des dev's, j'approuve et merge, mais sans avoir vraiment fait une QA complète... J'ai seulement testé en jouant avec un billet, je n'ai rien remarqué d'anormal.

Dans tous les cas, beau gros boulot ! 👍

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 C-Front Concerne l'interface du site
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Mélange de la bêta et de la version brouillon quand elles sont identiques
4 participants