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

[BUGFIX] Ré-afficher les messages d'erreurs renvoyés par l'API (PO-414). #1265

Merged
merged 1 commit into from
Apr 8, 2020

Conversation

MathieuGilet
Copy link
Contributor

@MathieuGilet MathieuGilet commented Apr 6, 2020

🦄 Problème

Dans la mire de connexion/inscription de Pix Orga (accessible depuis un lien d'invitation), lorsque l'on tente de créer un compte avec une adresse email déjà existante, le message d'erreur "Cette adresse e-mail est déjà enregistrée, connectez-vous." est censé s'afficher en dessous du label "Adresse e-mail". Malheureusement, ce message d'erreur n'est plus affiché ce qui empêche l'utilisateur de comprendre ce qu'il se passe.

🤖 Solution

Après analyse, il a été constaté que cette disparition est survenue suite au passage d'Ember en version 3.15 sur Pix Orga et plus spécifiquement à la montée de version d'ember-data (une explications plus complète du problème est disponible dans la section "Remarques"). Ce problème a été corrigé par l'équipe d'ember-data à partir de la version 3.16.2. Par ailleurs, les application Pix Admin et Pix App possèdent elles aussi une dépendance à ember-data en version 3.15. Il a donc été décidé, pour ses trois applications, de passer en version 3.17 d'ember-data (la dernière).

🌈 Remarques

Pour comprendre le problème actuel, il faut déjà comprendre comment Ember gère les erreurs renvoyées par une API qui suit la spec JSON:API (c'est notre cas). Lorsque l'on tente de créer une ressource (.save()) et que cette création échoue à cause d'un des attributs de l'objet, Ember s'attend à ce que l'API retourne une erreur 422 (Unprocessable Entity) avec un "lien" vers l'attribut fautif. Une fois cette erreur reçue par l'adapter d'ember-data, celui alimente une propriété "errors" sur le model que l'on a voulu créer (plus d'informations sur ce blog: https://davidtang.io/2016/01/09/handling-errors-with-ember-data.html). Cependant, à partir de la 3.15, il s'avère que cette propriété "errors" n'était plus alimentée.

L'issue coté ember-data: emberjs/data#6936
La PR fixant le problème: emberjs/data#6941
Le CHANGELOG: https://github.com/emberjs/data/blob/master/CHANGELOG.md (merge du BUGFIX en version 3.16.2)

💯 Pour tester

@pix-service
Copy link
Contributor

@lisequesnel
Copy link
Contributor

Du coup le titre serait pas plutôt "[TECH] Mise à jour de Ember sur Pix Orga et Pix App en 3.17" ?

@octo-topi octo-topi force-pushed the po-414-bugfix-api-422-errors branch 2 times, most recently from 2c8255e to 3ec819c Compare April 7, 2020 09:33
@octo-topi
Copy link
Contributor

Belle description de PR, chapeau !

@@ -61,7 +61,7 @@
"ember-cli-uglify": "^3.0.0",
"ember-composable-helpers": "^3.1.1",
"ember-concurrency": "^1.1.6",
"ember-data": "^3.15.1",
"ember-data": "~3.17.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Est-ce un choix délibéré de ne pas utiliser le chapeau ? Si oui, pourquoi ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comme expliqué sur le meet, je me suis inspiré de ce qui était fait dans le package.json de Pix Orga pour ember-data (à savoir ~) et l'ai appliqué sur les autres. Personnellement, je pense qu'accepter toutes les version 3.17.x et seulement celles-ci est une bonne chose.

@thloesch
Copy link
Contributor

thloesch commented Apr 7, 2020

Belle description de PR, chapeau !

Mention spéciale à la partie "Pour tester"

@thloesch
Copy link
Contributor

thloesch commented Apr 7, 2020

Du coup le titre serait pas plutôt "[TECH] Mise à jour de Ember sur Pix Orga et Pix App en 3.17" ?

*ember-data plutôt qu'Ember mais oui, on préfère mettre le changement en titre plutôt que le problème constaté

@thloesch thloesch changed the title [BUGFIX] Disparition des messages d'erreurs renvoyés par l'API (PO-414). [BUGFIX] Ré-afficher les messages d'erreurs renvoyés par l'API (PO-414). Apr 7, 2020
@jonathanperret
Copy link
Contributor

Du coup le titre serait pas plutôt "[TECH] Mise à jour de Ember sur Pix Orga et Pix App en 3.17" ?

*ember-data plutôt qu'Ember mais oui, on préfère mettre le changement en titre plutôt que le problème constaté

Hmm, question intéressante. J'avoue que je n'étais pas au courant de cette préférence, est-ce qu'elle est partagée ? 😉

J'aurais plutôt un biais pour parler des problèmes visibles, quand il y en a qui sont corrigés par un changement. Typiquement, dans le CHANGELOG voir Mise à jour Ember ne dira rien à un utilisateur (ou à quelqu'un qui s'apprête à mettre une release en production), alors que On affiche de nouveau les messages d'erreur parle clairement de l'impact.

D'un autre côté, un changement de version de framework est significatif et je comprends qu'on préfère que ça ne passe pas au second plan.

Une partie du problème vient probablement du fait qu'on essaie d'associer "un bug (ticket JIRA)" à "un changement dans le code (PR)". En réalité, un changement dans le code peut très bien corriger plusieurs bugs, ou avoir une intention autre (mise à jour de dépendance) mais corriger par effet de bord un ou plusieurs bugs.

En attendant, je reste sur ma préférence pour les titres qui parlent de l'expérience utilisateur plutôt que de la machinerie interne. Mais peut-être le débat a-t-il déjà été tranché ? Je veux bien en savoir plus si c'est le cas.

@MathieuGilet MathieuGilet merged commit 4b0141e into dev Apr 8, 2020
@MathieuGilet MathieuGilet deleted the po-414-bugfix-api-422-errors branch April 8, 2020 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants