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

Unification des adresses de création de contenu #6324

Merged
merged 3 commits into from
Jun 15, 2022

Conversation

Migwel
Copy link
Contributor

@Migwel Migwel commented May 31, 2022

Fix #5724.

Cette PR modifie les liens de créations de contenu de .../nouveau-tutoriel vers .../nouveau-contenu/TUTORIEL (et de même pour les articles et billets). Je ne suis pas nécessairement à 100% convaincu que ce type de contenu (i.e. TUTORIEL) tout en majuscule est nécessairement beau à voir mais c'est relativement facile à changer si on pense qu'une autre approche est meilleure.

Contrôle qualité

Scénario 1:

  • Se connecter
  • Cliquer sur "Commencer à rédiger" sur la page d'accueil
    Résultat attendu: Le lien est de la forme .../contenus/nouveau-contenu/TUTORIEL

Scénario 2:

  • Se connecter
  • Cliquer sur Bibliothèque > Accéder à tous les contenus de la bibliothèque
  • En bas de la page, cliquer sur "Écrire un tutoriel"
    Résultat attendu: Le lien est de la forme .../contenus/nouveau-contenu/TUTORIEL

Scénario 3:

  • Se connecter
  • Cliquer sur Bibliothèque > Accéder à tous les contenus de la bibliothèque
  • En bas de la page, cliquer sur "Écrire un article"
    Résultat attendu: Le lien est de la forme .../contenus/nouveau-contenu/ARTICLE

Scénario 4:

  • Se connecter
  • Cliquer sur Tribune > Tous les billets
  • En haut à gauche de la page, cliquer sur "Nouveau billet"
    Résultat attendu: Le lien est de la forme .../contenus/nouveau-contenu/OPINION

Scénario 5:

  • Se connecter
  • Se rendre manuellement sur la page .../contenus/nouveau-contenu/INCORRECT
    Résultat attendu: Le type de contenu choisi par défaut est tutoriel

@coveralls
Copy link

coveralls commented May 31, 2022

Coverage Status

Coverage increased (+0.003%) to 87.786% when pulling a1d7659 on Migwel:issue5724 into 8d0b952 on zestedesavoir:dev.

@Arnaud-D
Copy link
Contributor

Arnaud-D commented Jun 1, 2022

À mon avis, tu peux garder cette approche. On risque de changer à nouveau le fonctionnement de la création de contenu quand on s'attaquera à la nouvelle organisation des contenus, donc pas la peine de faire du zèle sur cette amélioration.

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.

Ça fonctionne, j'ai juste une petite remarque sur le code.

re_path(r"^nouveau-tutoriel/$", CreateContent.as_view(created_content_type="TUTORIAL"), name="create-tutorial"),
re_path(r"^nouvel-article/$", CreateContent.as_view(created_content_type="ARTICLE"), name="create-article"),
re_path(r"^nouveau-billet/$", CreateContent.as_view(created_content_type="OPINION"), name="create-opinion"),
re_path(r"^nouveau-contenu/(?P<created_content_type>[A-Z]+)", CreateContent.as_view(), name="create-content"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Tu devrais utiliser cette syntaxe, elle est plus lisible et va être généralisée partout (les PR sont en attente de QA) :

-    re_path(r"^nouveau-contenu/(?P<created_content_type>[A-Z]+)", CreateContent.as_view(), name="create-content"),
+    path("nouveau-contenu/<str:created_content_type>/", CreateContent.as_view(), name="create-content"),

Ça ne change a priori rien à la suite de ton code, voire même ça facilitera des évolutions futures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah oui, j'y avais pensé (car j'ai lu ton billet sur le sujet) mais j'avais utilisé re_path pour être cohérent avec le reste du fichier dans son format actuel. Mais je comprends aussi l'idée d'utiliser le "nouveau" format lorsqu'on ajoute des lignes dans ce fichier. C'est fait

@Migwel Migwel requested a review from Arnaud-D June 13, 2022 19:00
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.

QA OK ✔️

Le fait d'aller par défaut sur "Tutoriel" pourrait peut-être être remplacé par une 404, mais comme je disais l'autre jour, pas de zêle inutile. :D

@Arnaud-D Arnaud-D enabled auto-merge (squash) June 15, 2022 06:53
@Arnaud-D Arnaud-D merged commit f87657b into zestedesavoir:dev Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Unifier les adresses de création de contenu
3 participants