Refacto : on sépare les API internes full calendar dans un autre namespace #4805
Conversation
AntoineGirard
force-pushed
the
feat/refacto-agenda-internal-api
branch
from
f733a59 to
2b772b9
Compare
| @@ -0,0 +1,39 @@ | |||
| class Api::Internal::BaseController < ApplicationController | |||
There was a problem hiding this comment.
je trouve l’idée de scoper à Api::Internal bonne pour montrer qu’il s’agit ici d’endpoints XHR et pas classiques.
En revanche j’ai l’impression que la majorité du code présent ici est en fait spécifique à l’agenda agent :
- l’authentification agent
- les helper methods de time range et date range
Je me demande donc s’il ne vaudrait pas mieux appeler cette classe Api::Internal::Admin::Agenda::BaseController
There was a problem hiding this comment.
Merci Antoine pour cette proposition qui lève des points de douleur, c'est cool d'en parler. 🙏
Pour ma part je ne suis pas convaincu que ce qui qualifie un endpoint HTTP d'API soit le format de la réponse. Dans le cas présent, nous avons deux façons d'afficher les plages d'un agent connecté : sur une page HTML, ou sur un fullcalendar JSON. Mais le contexte dans lequel nous voulons récupérer et afficher ces informations est le même : au sein de l'interface de notre application. C'est ce qui me pousse à vouloir garder ces endpoints proches en termes de namespace et de nommage.
Ceci étant dit, je trouve que dans notre application il est difficile de trouver quels sont les endpoints JSON et pas HTML. La preuve, pour trouver la liste, j'ai dû aller sur Skylight :
Je soutiens donc la démarche qui consiste à donner de la visibilité à ces endpoints JSON.
Et si j'ai bien compris cette PR répond aussi au besoin de pouvoir contrôler ce qu'il se passe en cas de HTTP 401pour une requête JSON, et je me demande si il n'y a pas d'autres solutions. Je vais faire des tests et revenir ici pour discuter en étant plus informé là-dessus.
There was a problem hiding this comment.
Hello,
@francois-ferrandis est-ce que tu as pu avancer tes réflexions ou investigations sur le sujet ?
@victormours de nos dernières discussions à l’oral jeudi, tu semblais plutôt partant pour l’éjection proposée, mais tu avais une autre proposition de rangement pour les routes. On était sur :
- Laisser les routes dans le fichier
routes.rbplutôt que dansapi.rb - Et tu étais plus partant d’un
admin/apiplutôt queapi/internal/admin
C’est bien ça ?
There was a problem hiding this comment.
toutafé ! La motivation était que que j'ai l'impression que le fichier config/routes/api.rb et le namespace api sont utilisés pour des endpoints qui permettent à des partenaires extérieurs de s'Interfacer pour Programmer leurs Applications, alors que les routes du fichier config/routes.rb et de /admin sont notre interface web.
Ici le format des requêtes est du json, mais il s'agit bien de notre interface web, et pas d'une api extérieure qui nous connecte au reste du monde.
Une autre formulation de la même idée, c'est que les api habituelles sont "externes", donc qu'on ne veut pas les mélanger avec cette api "interne"
There was a problem hiding this comment.
J'ai jeté un coup d'œil rapide, merci pour la relance ! Je vais te donner ci-dessous ma vision qui je l'espère pourra aider à prendre des décisions, et je te fais entièrement confiance pour la suite ! 🤝
Je pense qu'il y a plusieurs problèmes qui peuvent être traités séparément :
- Les controllers qui gèrent un format JSON sont difficiles à identifier
- Nous ne parvenons pas à rediriger vers la page de login lorsqu'un appel FullCalendar est fait non connecté
- L'usage des
alertne fait plaisir à personne, il y a d'autres moyens plus agréables d'indiquer l'état du refresh
Le fait de déplacer les controllers dans un namespace "API" répond au problème 1, et même si j'ai chipoté sur la sémantique, franchement ça me va bien (la solution admin/api proposée par Victor me plaît).
Pour répondre au problème 2, on peut soit :
- signaler le format de la requête au niveau route, par exemple en définissant
namespace :agenda, defaults: { format: :json }à la ligne 166 deroutes.rb - ajouter
format: :jsondans les URLs injectées dans la config FullCalendar (app/views/admin/agent_agendas/show.html.slim), pour ajouter.jsonà la fin (ex:/rdvs.json?param=...)
Devise va alors automatiquement renvoyer des 401 au format JSON, que l'on peut catcher dans notre callback JS pour rediriger vers la page de login.
Et pour le problème 3, c'est traitable complètement séparément, et y'a un petit boulot de recherche UI pour déterminer où et comment on affiche "Le dernier fetch des plages a échoué pour une raison inconnue, on va réessayer". En effet, normalement on ne doit afficher ça qu'en cas de 500 ou timeout, une fois qu'on a traité spécifiquement les autres cas (agent non connecté, ou connecté avec un compte non autorisé).
AntoineGirard
force-pushed
the
feat/refacto-agenda-internal-api
branch
2 times, most recently
from
69f30c7 to
ef3533f
Compare
AntoineGirard
changed the title
AntoineGirard
force-pushed
the
feat/refacto-agenda-internal-api
branch
from
ef3533f to
e964ebe
Compare
|
|
||
| let(:organisation) { create(:organisation) } | ||
|
|
||
| context "with a signed in agent" do |
There was a problem hiding this comment.
Ici j’ai juste rajouté ce contexte pour pouvoir rajouter le cas où l’agent n’est pas connecté
AntoineGirard
changed the title
AntoineGirard
force-pushed
the
feat/refacto-agenda-internal-api
branch
from
e964ebe to
8abe7af
Compare
|
Merci @francois-ferrandis pour ta réponse très complète. Effectivement j’étais passé un peu à côté du fait que devise pouvait gérer ça automatiquement en rajoutant l’info du format dans le fichier J’ai fait une proposition de code basé sur nos échanges et tes conclusions. Je te laisse me dire ce que tu en penses. Concernant la gestion de l’erreur côté front, on est assez ok avec Mehdi et Teo pour redirigé vers la page de connexion en cas de 401. Je suis moyen convaincu par le window.reload donc je l’ai sorti de la PR et je vais faire une proposition dans une autre PR. |
AntoineGirard
force-pushed
the
feat/refacto-agenda-internal-api
branch
from
8abe7af to
71bf013
Compare
There was a problem hiding this comment.
Magnifique ! 😍 🚀
Concernant la gestion de l’erreur côté front, on est assez ok avec Mehdi et Teo pour redirigé vers la page de connexion en cas de 401. Je suis moyen convaincu par le window.reload donc je l’ai sorti de la PR et je vais faire une proposition dans une autre PR. Il était juste là à titre d’exemple pour montrer que ça fonctionne.
C'est très raisonnable, j'ai hâte de voir la suite ! 👍
|
|
||
| context "when agent is not login" do | ||
| it "returns unauthorized" do | ||
| get :index, params: { agent_id: 1, organisation_id: 1, start: "2019-08-12", end: "2019-08-19", format: :json } |
There was a problem hiding this comment.
J'apprécie beaucoup que cette spec ne reprenne pas tous les let de l'enfer de celle d'au-dessus, bravo ! 👏
AntoineGirard
force-pushed
the
feat/refacto-agenda-internal-api
branch
2 times, most recently
from
4320302 to
50f9a4f
Compare
AntoineGirard
force-pushed
the
feat/refacto-agenda-internal-api
branch
from
50f9a4f to
2340c2a
Compare
|
Woooops on a pas pensé à la "migration", là on vient de déployer un changement de route et donc tout le monde se tape des |
|
@francois-ferrandis, je n’avais pas vu ton message 😬 |
Review app
Contexte
Nous rencontrons régulièrement un bug lorsque l’utilisateur est déconnecté, mais qu’il a laissé un onglet ouvert avec la page de calendrier. Voir ici : #4803
En cherchant des pistes de solutions, je me suis aperçu que les 3 points d’API que nous utilisons pour envoyer les données à fullCalendar ont la même base que les contrôleurs qui rendent du HTML.
Ceci me pose un problème, notamment dans la gestion des erreurs. Dans le cas du bug mentionné ci-avant, j’aurais voulu pouvoir gérer spécifiquement un code 401 pour faire une action dans ce cas (par exemple rediriger l’utilisateur), or, je reçois une 302 qui redirige vers la page de connexion.
Le
handleAjaxErrorest déclenché, car fullCalendar essaie de parser la réponse comme si c’était un JSON sauf qu’il s’agit ici du HTML de la page de connexion.Solution
Proposition initiale qui a permis les échanges
BaseController» dédié aux API internes.admindansroutes.rbest encapsulé dans unauthenticate :agentj’ai fait le choix de sortir complétement ces 3 contrôleurs et de les mettre dansapi/internal.authenticate_agent!pour ce contrôleur qui retourne du JSON plutôt que de rediriger vers la page d’accueil en cas de problème d’authentification.401proprement dansagenda.js. C’est un exemple d’utilisation, nous n’avons pas encore décidé en terme produit si nous voulons ce reload, un message d’erreur spécifique, ou ne rien faire… La suite de la discussion sur ce sujet dans l’issue dédiée.La proposition de solution est bien résmuée ici : #4805 (review)