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

Gère plus d'erreurs possibles venant de Matomo #6151

Merged
merged 1 commit into from
Oct 23, 2021

Conversation

philippemilink
Copy link
Member

Cette PR gère plus d'erreurs possibles retournées par Matomo.

Actuellement, sur l'environnement de dev, consulter les statistiques d'un contenu renvoie une erreur 500 AttributeError 'str' object has no attribute 'items'. Cela provient du fait que Matomo renvoie des erreurs You can't access this resource as it requires 'view' access for the website id = 4. (ce point devra être corrigé dans une autre PR, mais autant en profiter pour gérer correctement ce cas).

J'en ai profité pour corriger quelques détails dans le code auquel j'ai touché:

  • faire data = {} juste avant de tenter data.get("message", ...) n'est pas une très bonne idée...
  • pas besoin de f-strings lorsqu'il n'y a rien à formater dans les chaînes de caractères

Si quelqu'un a une idée comment factoriser les deux lignes self.logger / message.error qui se répètent (presque identiquement) trois fois, je suis preneur.

Contrôle qualité

Sur l'environnement de dev, aller à la page des statistiques d'un contenu: la page s'affiche, sans erreur 500.

Comme toujours pour les statistiques, il faudra s'assurer sur la bêta que ça fonctionne bien, même lorsque des statistiques sont disponibles.

@coveralls
Copy link

coveralls commented Aug 21, 2021

Coverage Status

Coverage increased (+0.04%) to 86.661% when pulling 74940c7 on philippemilink:handle-matomo-errors into b547584 on zestedesavoir:dev.

zds/tutorialv2/views/statistics.py Outdated Show resolved Hide resolved
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 NOK ❌

Globalement ça marche, mais il y a des trucs qui me gênent, notamment les messages d'erreur en anglais et cryptique affichés à l'utilisateur. C'est pas forcément dans le périmètre de ta modif, mais comme c'est connexe...

J'ai fait des suggestions pour cacher les détails sur les messages du site. Si on veut garder l'aspect technique visible, il faudrait tout de même l'introduire avec un message d'erreur compréhensible.


Pour ce qui est d'éviter la duplication, stocker les messages d'erreur comme attribut de classe est déjà un bon début.

zds/tutorialv2/views/statistics.py Outdated Show resolved Hide resolved
zds/tutorialv2/views/statistics.py Outdated Show resolved Hide resolved
zds/tutorialv2/views/statistics.py Outdated Show resolved Hide resolved
@philippemilink
Copy link
Member Author

J'ai pris en compte les remarques, ajouté un test pour s'assurer que les erreurs Matomo ne provoquent pas d'erreur 500, et dans le cas où il y a une erreur, ne pas afficher les graphiques (sinon, ça provoquait une erreur JS).

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.

J'aime bien comment la fonction gère ses erreurs par exception et qu'on fasse la gestion de ça ailleurs que dans la fonction elle-même, c'est plus propre.

J'ai juste une gêne sur un point précis, je ne suis pas sûr d'avoir compris ce que le code devrait faire, ça me semble étrange.

zds/tutorialv2/views/statistics.py Outdated Show resolved Hide resolved
zds/tutorialv2/views/statistics.py Outdated Show resolved Hide resolved
@Arnaud-D
Copy link
Contributor

QA OK ✔️

@Arnaud-D Arnaud-D merged commit 6c78b21 into zestedesavoir:dev Oct 23, 2021
@philippemilink philippemilink deleted the handle-matomo-errors branch November 11, 2021 18:08
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.

4 participants