-
Notifications
You must be signed in to change notification settings - Fork 160
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
Refactorise les templates des publications #6457
Conversation
9a464da
to
11eb34a
Compare
44568b7
to
528864e
Compare
3d3b1e4
to
a162ade
Compare
a162ade
to
c6d74fb
Compare
There was a problem hiding this 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.
c6d74fb
to
c489abf
Compare
J'ai trouvé le plus gros coupable pour mon augmentation du nombre de requêtes. J'avais une méthode 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. |
c489abf
to
5e44438
Compare
There was a problem hiding this 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...)
href="{{ subchild.get_absolute_url }}{% if version %}?version={{ version }}{% endif %}" | ||
{% endif %} | ||
{% endif %} | ||
href="{{ base_url }}{{ child.slug }}/{{ subchild.slug }}/" |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
Tu peux corriger le conflit, rebaser sur dev et corriger le test qui plante, stp ? |
187a5a8
to
7c2b864
Compare
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
bdb9734
to
410f091
Compare
There was a problem hiding this 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 ! 👍
Fix #5906.
Cette PR refactorise les templates des publications.
Il y a une part de mise à jour de l'expérience utilisateur.
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 :
Il y a des choses qu'on pourrait faire pour aller plus loin, mais que je ne ferais pas dans cette PR.
is_staff
. Cette information est souvent pas très précise, et on devrait utiliser des véritables permissions sur les vues (quitte à réecrirehas_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.child
etchild_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.
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.
Choses à regarder sur les différentes pages :
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.