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

fix #5684: Adapte l'affichage du forum pour les membres en lecture seule #5837

Closed
wants to merge 1 commit into from
Closed

Conversation

jeanlapostolle
Copy link
Contributor

@jeanlapostolle jeanlapostolle commented Jun 23, 2020

Numéro du ticket concerné (optionnel) :
#5684

Contrôle qualité

Mettre un utilisateur en Lecture Seule. Vérifier qu'il ne peut pas poster de messages et qu'il n'a pas le bouton "Nouveau sujet"

@artragis artragis changed the title fix #5684 fix #5684: Adapte l'affichage du forum pour les membres en lecture seule Jun 23, 2020
Copy link
Member

@AmauryCarrade AmauryCarrade left a comment

Choose a reason for hiding this comment

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

Salut et merci pour cette contribution !
J'ai laissé un petit commentaire sur le code mais c'est vraiment rien du tout.

Par contre cette PR casse un test, comme tu peux le constater tout en bas. En l'occurrence le test_frontend_alert_existence_other_pages, qui vérifie la présence du message en bas de page signalant qu'il y a une autre page.

Et en effet : quand je teste, je constate la présence du message « Vous êtes en lecture seule. Vous ne pouvez pas poster de messages. » même quand je suis connecté via un compte sans restriction (j'ai testé en mettant user en LS, et en testant via admin). Plus généralement, il n'y a plus aucun bouton de création ou de formulaire d'envoi pour quiconque. Un peu trop efficace :D .

Le problème

En réalité le problème est simple : tu as confondu les deux objets qu'on a qui représentent un utilisateur. En effet on a :

  • d'un côté l'objet User de Django (celui exposé dans la variable user), qui ne contient que les éléments de base de l'utilisateur (nom, courriel, mot de passe, …), ce que Django gère lui-même pour nous ;
  • d'un autre côté l'objet Profile (exposé via user.profile), qui est une sorte d'extension de User avec ce qui concerne ZdS : biographie, site web, préférences, … et aussi l'état de lecture seule.

Là où c'est fourbe, c'est qu'en cas d'appel à une méthode ou une propriété inconnue, le moteur de gabari de Django ne râle pas : il ignore et remplace par une valeur fausse. user.can_write_now n'existe pas ; le moteur considère donc que c'est faux, et ainsi, {% if not user.can_write_now %} est toujours vrai.

Le correctif

Le correctif est simple : can_write_now est défini, donc, dans le Profile ; il faut remplacer tous ses appels par user.profile.can_write_now.

Et aussi

Je t'invite à ajouter un test (tu peux prendre comme modèle le test lié plus haut) vérifiant que pour un utilisateur en lecture seule, le bon message est bien affiché, et aussi que pour un utilisateur qui n'est pas en lecture seule, le message n'est pas affiché. Et de même pour les liens de création d'un sujet.

Il est tout aussi important de tester le fonctionnement de ce qu'on ajoute, que la non-régression du reste :) . (Et c'est aussi valable pour la QA.)

N'hésite pas à demander (ou, avant, à regarder dans d'autres tests) pour savoir comment mettre un membre en lecture seule en Python.

Enfin, il faudra rebase pour pouvoir fusionner la PR.

Et merci encore !

Comment on lines +26 to +29
{% elif not user.can_write_now %}
<div class="alert-box ico-after ico-alert light">
{% trans "Vous êtes en lecture seule. Vous ne pouvez pas poster de messages." %}
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Il serait mieux de respecter l'indentation ici

Suggested change
{% elif not user.can_write_now %}
<div class="alert-box ico-after ico-alert light">
{% trans "Vous êtes en lecture seule. Vous ne pouvez pas poster de messages." %}
</div>
{% elif not user.can_write_now %}
<div class="alert-box ico-after ico-alert light">
{% trans "Vous êtes en lecture seule. Vous ne pouvez pas poster de messages." %}
</div>

@philippemilink
Copy link
Member

Comment mentionné dans les commentaires de la PR #5845, il faudrait aussi empêcher les utilisateurs en LS de pouvoir masquer leurs messages.

@Situphen
Copy link
Member

Situphen commented Sep 2, 2020

Coucou @jeanlapostolle, penses-tu avoir le temps de continuer cette PR ? Si besoin d'aide n'hésites pas à nous demander ! :)

@jeanlapostolle
Copy link
Contributor Author

Oui, je la reprends après mes vacances ^^

@Arnaud-D
Copy link
Contributor

Arnaud-D commented Jun 6, 2021

@jeanlapostolle Est-ce que tes vacances sont finies ? :D

@jeanlapostolle
Copy link
Contributor Author

Oui euh j'ai peut-être oublié du travail par ici ^^. Je vais devoir le délaisser, je n'ai même plus d'environnement de dév' pour ZDS et je n'ai pas le temps d'en refaire avant un certain temps.

@Arnaud-D
Copy link
Contributor

Arnaud-D commented Jun 7, 2021

Je la déplace dans "En attente de reprise" alors. Tu pourras reprendre toi-même ta PR si le cœur t'en dit. Ou laisser quelqu'un la reprendre, mais c'est peu probable vu les habitudes de développement qu'on a. :-)

@SpaceFox
Copy link
Contributor

Pour info, le fork est inaccessible pour je ne sais quelle raison, donc je ne connais pas de moyen simple de reprendre cette PR.

@philippemilink
Copy link
Member

Pour info, le fork est inaccessible pour je ne sais quelle raison, donc je ne connais pas de moyen simple de reprendre cette PR.

Oui, la branche semble ne plus exister.

Tu peux récupérer le code qui a déjà été fait avec :

wget https://github.com/zestedesavoir/zds-site/pull/5837.patch
git apply 5837.patch

... et faire une nouvelle PR.

@Arnaud-D
Copy link
Contributor

Je ferme, ça a été repris par #6234.

@Arnaud-D Arnaud-D closed this Jan 23, 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.

Adapter l'affichage du forum pour les membres en lecture seule
6 participants