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

Corrige une erreur si la durée d'une sanction temporaire est manquante #6564

Merged
merged 2 commits into from
Feb 11, 2024

Conversation

philippemilink
Copy link
Member

@philippemilink philippemilink commented Dec 25, 2023

Sentry nous rapporte que l'erreur suivante :

TypeError: exceptions must derive from BaseException
[...]
  File "zds/member/views/moderation.py", line 167, in modify_profile
    raise HttpResponseBadRequest

est levée lorsqu'on applique une sanction temporaire à un membre, mais que la durée de la sanction n'est pas précisée.

Cette PR corrige le problème en capturant les exceptions possibles s'il manque la durée. En plus, j'ajoute un test et je rends obligatoire le champ de la durée dans le formulaire pour ajouter une sanction temporaire.

Contrôle qualité

Se connecter en tant qu'admin et, depuis la page de profil d'un membre, appliquer des sanctions temporaires (banissement et lecture seule) et s'assurer que tout fonctionne toujours comme attendu. On ne peut pas valider le formulaire sans préciser une durée de sanction.

@coveralls
Copy link

coveralls commented Dec 25, 2023

Coverage Status

coverage: 88.827% (+0.01%) from 88.816%
when pulling e005998 on philippemilink:fix-temp-sanction-no-time
into 738f577 on zestedesavoir:dev.

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.

Quelques petits commentaires, mai sinon c'est OK.

Cette partie de la base de code n'est pas folichone de toute façon et la refondre n'est pas l'objectif de la PR. ^^

Comment on lines 181 to 186
except ValueError:
raise HttpResponseBadRequest
except (ValueError, TypeError):
messages.error(request, _("Une erreur est survenue lors de la récupération de la sanction."))
Copy link
Contributor

Choose a reason for hiding this comment

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

raise HttpResponseBadRequest était un bout de code erroné, vu le message que Sentry a remonté. C'est un objet à retourner et pas à raise. En conséquence, je pense que ce n'est pas nécessaire de mettre un TypeError dans le except du dessous, ce type d'erreur n'est pas censé arriver.

Je n'ai pas réussi à identifier d'où viendrait la ValueError. Probablement pas directement de notre code, ce qui est complique encore plus la recherche.

Aussi, faudrait supprimer l'import HttpBadRequest en haut du fichier aussi, il est désormais inutile.

@philippemilink philippemilink force-pushed the fix-temp-sanction-no-time branch from 8994aad to 4f2467a Compare February 10, 2024 21:07
@philippemilink
Copy link
Member Author

philippemilink commented Feb 10, 2024

En conséquence, je pense que ce n'est pas nécessaire de mettre un TypeError dans le except du dessous, ce type d'erreur n'est pas censé arriver.

En effet, sans doute un reste de mes essais pour corriger le bug. La CI vient de me prouver le contraire : avec le test que j'ai rajouté, si l'un des paramètres POST demandés est vide, ça lève une TypeError.

Je n'ai pas réussi à identifier d'où viendrait la ValueError. Probablement pas directement de notre code, ce qui est complique encore plus la recherche.

J'ai mis un commentaire pour l'expliquer. En gros, si les paramètres passés en POST sont invalides (ou absents), ça finit par planter lorsqu'on cherche à accéder à ces paramètres dans state.apply_sanction().

Aussi, faudrait supprimer l'import HttpBadRequest en haut du fichier aussi, il est désormais inutile.

Bien vu !

Et rend obligatoire la durée des sanctions temporaires pour valider le
formulaire.
@philippemilink philippemilink force-pushed the fix-temp-sanction-no-time branch from 4f2467a to 3eed6a6 Compare February 10, 2024 21:16
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 ✔️

@Arnaud-D Arnaud-D enabled auto-merge (squash) February 11, 2024 14:58
@Arnaud-D Arnaud-D merged commit 726ada0 into zestedesavoir:dev Feb 11, 2024
8 checks passed
@philippemilink philippemilink deleted the fix-temp-sanction-no-time branch February 11, 2024 15:07
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.

3 participants