Ajoute un lien vers l'Espace Producteur dans le menu quand pertinent #3684
Conversation
| @@ -353,11 +354,9 @@ defmodule TransportWeb.Router do | |||
| end | |||
|
|
|||
| # NOTE: method visibility set to public because we need to call the same logic from LiveView | |||
| def is_transport_data_gouv_member?(current_user) do | |||
There was a problem hiding this comment.
Ce changement impose aux membres de notre équipe de se déconnecter/reconnecter étant donné qu'on n'a pas l'attribut is_admin dans notre session actuellement. Si les membres de notre équipe ne font pas ça elles ne verront plus le lien vers le BO et si ils vont sur l'URL ils seront redirigés avec une erreur. Rien de dramatique donc.
Pas d'impact pour les autres.
There was a problem hiding this comment.
Amélioration sécurité à prévoir en lien avec:
En cas de vol de cookie, si je comprends bien (avant la PR ou après la PR d'ailleurs), le rôle restera encodé dans le cookie.
Il faudrait avoir un rafraîchissement systématique (ou plus frais en tout cas, ex: cache avec TTL si on veut éviter un appel d'API à chaque tour) sinon on se crée des problèmes (la suppression d'un compte sur data gouv ne protègera pas d'un vol de cookie admin).
There was a problem hiding this comment.
Les changements importants dans la session sont dans ce fichier.
| @@ -57,13 +57,4 @@ defmodule Helpers do | |||
| dates -> dates |> Enum.max(DateTime) |> DateTime.to_iso8601() | |||
| end | |||
| end | |||
|
|
|||
| @spec admin?(map | nil) :: boolean | |||
| def admin?(%{} = user) do | |||
There was a problem hiding this comment.
Supprime cette méthode elle faisait doublon avec ce qui était dans TransportWeb.Router
There was a problem hiding this comment.
Ok vu:
(doublon après l'ajout par la PR)
Merci pour l'explication !
| Are you a data producer? | ||
| You're a data producer if you're a member of an organization with an active dataset | ||
| on transport.data.gouv.fr. | ||
| This is set when you log in, we can refresh this field more often in the future. |
There was a problem hiding this comment.
La durée de notre session est de 15 jours
There was a problem hiding this comment.
Mis à jour dans l'Espace Producteur aussi b24229e
There was a problem hiding this comment.
Hello ! Deux commentaires pour le moment :
-
il faudrait à mon avis DRYer la logique métier autour de
is_producerpour éviter des soucis à plus long terme. Un module unique qui prend la décision autour de ça, voire fixe la clé, et les deux contrôleurs qui y font appel. Sûr, à court terme pas de souci, mais quelqu'un qui repasse derrière dans quelques temps va se trouver embrouillé. -
en terme d'UX, on se retrouve avec 2 opérations avec presque le même nombre de clics (1 hover + 1 clic, ou 1 clic + 1 clic) juste à côté dans le bandeau pour accéder à l'espace producteur :
Ca semble assez redondant à moins que j'ai raté quelque chose ?
Au final (et je le dis avec des pincettes, je sais que tu veux améliorer les conditions d'usage de cet espace et ça me paraît une bonne idée en général et je respecte ça) je ne suis pas sûr de comprendre l'apport UX de cette PR par rapport à ce qui avait avant ?
(par contre le début de refactoring en lien avec is_admin? et les tests etc me semblent utiles!)
À rediscuter, peut-être quelque chose que je n'ai pas vu !
| @@ -57,13 +57,4 @@ defmodule Helpers do | |||
| dates -> dates |> Enum.max(DateTime) |> DateTime.to_iso8601() | |||
| end | |||
| end | |||
|
|
|||
| @spec admin?(map | nil) :: boolean | |||
| def admin?(%{} = user) do | |||
There was a problem hiding this comment.
Ok vu:
(doublon après l'ajout par la PR)
Merci pour l'explication !
apps/transport/lib/transport_web/controllers/session_controller.ex
Outdated
Show resolved
Hide resolved
apps/transport/lib/transport_web/controllers/session_controller.ex
Outdated
Show resolved
Hide resolved
apps/transport/lib/transport_web/controllers/session_controller.ex
Outdated
Show resolved
Hide resolved
| @@ -353,11 +354,9 @@ defmodule TransportWeb.Router do | |||
| end | |||
|
|
|||
| # NOTE: method visibility set to public because we need to call the same logic from LiveView | |||
| def is_transport_data_gouv_member?(current_user) do | |||
There was a problem hiding this comment.
Amélioration sécurité à prévoir en lien avec:
En cas de vol de cookie, si je comprends bien (avant la PR ou après la PR d'ailleurs), le rôle restera encodé dans le cookie.
Il faudrait avoir un rafraîchissement systématique (ou plus frais en tout cas, ex: cache avec TTL si on veut éviter un appel d'API à chaque tour) sinon on se crée des problèmes (la suppression d'un compte sur data gouv ne protègera pas d'un vol de cookie admin).
|
Je finis mon refactor et je remets le tout à la review |
AntoineAugusti
force-pushed
the
menu_link_to_producer_space
branch
from
75bf77c
to
bfff9d9
Compare
UXLe changement était bien attendu de @etalab/transport-bizdev, il permet de mettre en avant l'Espace Producteur dans son "menu personnel". La page "Infos producteurs" mélange page d'informations utiles (pertinent en phase de découverte non connecté) et accéder à son espace. Le menu devrait être le meilleur endroit pour accéder à cet espace en phase de croisière. L'ajout du tracking permettra de voir l'usage qui en est fait et d'ajuster si besoin. RefactorMerci d'avoir mis ce sujet sur la table ! Je sentais qu'il y avait un peu de friction et de choses à améliorer. J'ai ajouté un module spécifique ( On vérifie désormais qu'on est admin ou producteur à l'aide de |
There was a problem hiding this comment.
Top !
- UX -> merci pour l'explication !
- Session -> nickel pour le refactor
Je ne vois rien de problématique, et j'approuve, toutefois je désactive par prudence l'auto-merge vu qu'il est tard.
On pourra merger demain matin tranquillement ça sera moins risqué.
GG !
Ajoute un lien vers l'Espace Producteur dans le menu de l'utilisateur quand l'utilisateur est membre d'une organisation ayant un jeu de données référencé sur le PAN.
J'ajoute des paramètres UTM aux liens vers l'Espace Producteur existant pour savoir d'où viennent les utilisateurs (menu, page "Infos producteur", notifications etc.)
Refactor des attributs de
:current_userdans la sessionCeci concerne
:current_userqui est stocké dans la session parSessionController."organizations"qui stockait en réalité uniquement l'organisation du PAN si l'utilisateur est membre de cette organisation (concerne uniquement notre équipe). Adaptation des méthodes qui permettent de déterminer si un utilisateur est admin en conséquence."is_admin""is_producer"