Feat/dat 260/etq i je peux revoquer une habilitation validee #137

Isalafont · 2024-03-26T10:53:33Z

Affichage de la Pop Up Clé à lier
Fix i18n Unused key : instruction.revoke_authorization_requests.create.success.title | La demande d'habilitation %{name} a été révoquée
Modification du status Révoquée (s'affiche actuellement un status révoquée et un status en cours de mis à jour en même temps
Sur l'habilitation liée à la demande (snapshot), le status est encore à validée
Confirmation que la couleur du status Révoquée est bien la bonne @eva.spaeter
Rspec
Cucumber

linear · 2024-03-26T10:53:36Z

DAT-260 ETQ I, je peux révoquer une habilitation validée

Isalafont · 2024-03-27T14:16:24Z

@SchweisguthN

Dans le cas d'une révocation :

Enregistrement.de.l.ecran.2024-03-27.a.15.12.08.mov

Dans le cas du refus, l'utilisation de la même popup (review le wording)

Enregistrement.de.l.ecran.2024-03-27.a.15.14.39.mov

Un3x

Quelques petites remarques mais surtout cette histoire de denial. J'ai un peu survolé la review parce que je connais pas tout votre système.

Concrètement là tel quel ca ne me convient pas.

J'explique :

Avant cette PR il y a 2 classes identique IntructorModificationRequest et DenialOfAuthorization qui correspondent chacune à un cas d'usage différent. C'est un choix technique qui a été fait (et potentiellement ca se challenge).

Du coup si on se base sur l'existant la logique voudrait de créer une nouvelle classe dédié au revoke (on est sur un autre event, sur un autre objet, qui a des conséquences métier autre).

Si on estime que c'est pas DRY et pas clair, alors on crée une notion abstraite genre EventWithReason (à challenger comme naming) et on l'utilise partout.

MAIS, on ne fait pas un entre les deux à base de "ici on unifie" et "ici on unifie pas". Le fais que le revoke et le refuse évoque plus ou moins la même chose (alors que c'est différent) est suffisament confusant pour qu'on isole pas uniquement ces 2 events dans le code parce que les termes se ressemblent.

Il faut être en mesure au premier coup d'oeil de savoir quelles sont les caractéristiques discriminentes pour savoir si on utilise telle classe ou telle classe ou une notion abstraite. Là franchement c'est pas clair.

Perso, je m'en fiche du choix technique final qui est fait, je souhaite juste qu'il soit cohérent de facon transverse sur la code base.

app/notifiers/base_notifier.rb

db/migrate/20240326144413_add_revoke_constraints_to_events.rb

mehdi-farsi · 2024-03-27T15:13:09Z

@Un3x En effet, on avait en tête de faire un Event::Reason.

Je pense que ça peut attendre le lancement de reborn. Ca nous permettra d'avoir un peu + de recule sur les différents types de authorization_request_event.entity et de faire en sorte d'avoir une abstraction qui couvre la majorité des cas (avec de bons namings 😊).

Un3x · 2024-03-27T15:35:09Z

Moi cette décision @mehdi-farsi elle me convient, mais dans ce cas on utilise pas le DenialOfAuthorization, on en fait un autre custom et autant de custom dont on a besoin jusqu'à ce qu'on arrive au point où on sait le nomner correctement.

Comme ca on reste cohérent.

mehdi-farsi · 2024-03-27T15:37:43Z

Moi cette décision @mehdi-farsi elle me convient, mais dans ce cas on utilise pas le DenialOfAuthorization, on en fait un autre custom et autant de custom dont on a besoin jusqu'à ce qu'on arrive au point où on sait le nomner correctement.

Comme ca on reste cohérent.

Pas de problème. C'est un peu hors de cette PR qui est déjà à 27 fichiers. Je te propose de faire cela dans une autre PR dans la foulée.

skelz0r · 2024-03-27T15:38:39Z

Pour info j’avais décidé de faire N modèles pour les raisons suivantes: 1. Pour sortir des statistiques c’est plus simple 2. C’est plus simple d’augmenter les infos sur des modèles distincts qu’un modèle générique. Typiquement sur le request change ajouter des champs à modifier, le refus des templates spécifiques/type de raison, etc.. 3. C’est plus simple niveau wording/i18n de manipuler des modèles différents 4. C’est plus clair de manipuler un denial/instructor request change, qu’une classe générique 5. C’est assez simple de créer des nouveaux modèles en rails (1 ligne de commande), l’argument de la complexité technique Pour moi le plus important reste la clarté, ce qui a clairement fait pencher la balance vers le N modèles. Et je vous cache pas que le spectre du god pattern de la v1 a aussi eu son effet.

…

On Wed 27 Mar 2024 at 15:13, Mehdi FARSI ***@***.***> wrote: @Un3x <https://github.com/Un3x> En effet, on avait en tête de faire un Event::Reason. Je pense que ça peut attendre le lancement de reborn. Ca nous permettra d'avoir un peu + de recule sur les différents types de authorization_request_event.entity et de faire en sorte d'avoir une abstraction qui couvre la majorité des cas (avec de bons namings 😊). — Reply to this email directly, view it on GitHub <#137 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJYPYCGH7WMWPMBUYU2PNDY2LHYXAVCNFSM6AAAAABFIVX4ZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRTGAZDKNRWHA> . You are receiving this because your review was requested.Message ID: ***@***.***>

Un3x · 2024-03-27T19:53:24Z

Pas de problème. C'est un peu hors de cette PR qui est déjà à 27 fichiers. Je te propose de faire cela dans une autre PR dans la foulée.

Désolé mais je ne suis pas d'accord. Cette PR introduit le problème en utilisant le modèle à tord. J'ai pas d'avis sur la solution technique et je ne me rangerai pas d'un côté ou de l'autre alors que je ne travaille pas activement sur le projet,

Encore une fois j'essaye d'apporter un point de vue extérieur. Je m'en fiche que le mur soit bleu ou vert, mais pas à moitié bleu et à moitié vert.

Concernant la taille des PRs, c'est un autre sujet. Aucun problème à ce que vous affiniez le découpage des chantiers et l'explosiez en plusieurs PRs de facon plus itérative. Encore une fois, sous réserve que l'ensemble de ce qui est livré soit cohérent.

mehdi-farsi · 2024-03-27T19:59:50Z

@Un3x Il y a clairement un sujet de refacto global sur les naming d'authorization_request_event.entity. Du coup, je ne traiterai pas cela dans cette PR.

Je vais à la place, créer une nouvelle table à associer à un event de révocation en mode RevocationOfAuthorization histoire de garder une certaine "homogénéïté" et ne pas être dans l'entre-deux.

evaspae · 2024-04-02T15:47:27Z

@Isalafont @mehdi-farsi @SchweisguthN voici mes retours dans le cas d'une révocation :

Pour la page d'accueil de l'instructeur :

Le titre de la page devrait être au dessus des filtres
Il manque des filtres
Les colonnes ne sont pas les bonnes
les badges de statut passent sur deux lignes alors qu'ils devraient être sur une seule ligne

Screenshot

Maquette

Sur la page de l'habilitation :

La dropdown d'actions n'est pas à la bonne taille + elle est décalée par rapport au bouton
Il manque le numéro de l'habilitation / de la demande
Dans l'historique, le message de révocation ne devrait pas être déployé de base mais seulement si l'utilisateur clique sur "voir plus" (cf maquette)

Screenshot

Maquette :

Modale pour le motif de révocation :

Pour le wording, peut-on remplacer par "Vous êtes sur le point de révoquer cette habilitation, merci de renseigner les motifs qui seront communiqués au demandeur."
L'intitulé du champ devrait être "Indiquez les motifs de révocation"

Screenshot

Page d'accueil, une fois que la révocation est validée :

Le bandeau d'alerte est trop proche du header
La taille et le placement du bandeau ne sont pas les bons (cf maquette)

Screenshot

Maquette :

Isalafont · 2024-04-03T12:36:19Z

@evaspae
Merci pour tous ces retours !

En premier retour concernant directement le wording de la modal, j'ai effectué les changements suggérés.

Je me suis permis de changer le titre car nous sommes sur une habilitation et non une demande à ce moment là.
Peux tu me confirmer que cela te convient ?

Isalafont · 2024-04-03T13:16:13Z

Pour Information @evaspae, J'ai rajouté du margin top sur tous les flash message.
Après création du ticket concernant la disposition du flash message pour cette page, nous nous demandions si la disposition ne changeait que sur la page instructeurs ? cf le ticket DAT-276

evaspae · 2024-04-04T06:31:55Z

@evaspae Merci pour tous ces retours !

En premier retour concernant directement le wording de la modal, j'ai effectué les changements suggérés.

Je me suis permis de changer le titre car nous sommes sur une habilitation et non une demande à ce moment là. Peux tu me confirmer que cela te convient ?

Top, ça me va très bien ! Merci

evaspae · 2024-04-04T06:35:33Z

Pour Information @evaspae, J'ai rajouté du margin top sur tous les flash message. Après création du ticket concernant la disposition du flash message pour cette page, nous nous demandions si la disposition ne changeait que sur la page instructeurs ? cf le ticket DAT-276

Sur les maquettes, l'emplacement est le même partout donc pas uniquement sur la page instructeur.
A dispo pour en discuter

mehdi-farsi · 2024-04-08T08:00:21Z

Pour Information @evaspae, J'ai rajouté du margin top sur tous les flash message. Après création du ticket concernant la disposition du flash message pour cette page, nous nous demandions si la disposition ne changeait que sur la page instructeurs ? cf le ticket DAT-276

Sur les maquettes, l'emplacement est le même partout donc pas uniquement sur la page instructeur. A dispo pour en discuter

Du coup, on va devoir faire une passe globale sur l'ensemble des pages pour vérifier que le design des flash messages soit bon partout.

On laisse comme ça dans cette PR et on crée un ticket dans la foulée histoire de ne pas polluer cette PR!

mehdi-farsi · 2024-04-08T12:42:44Z

@skelz0r une review stp 🙏

skelz0r · 2024-04-08T14:17:10Z

Vous attendez une review ici btw ?

mehdi-farsi · 2024-04-08T14:20:05Z

@skelz0r une review stp 🙏

En effet :-)

evaspae · 2024-04-09T09:26:31Z

Comme discuté ce matin, il faudrait ajouter un message dans la modale sur les statuts "refusée" et "révoquée" : "attention, cette action est irréversible"

Je valide le wording globale de la modale + l'emplacement du flash message

…iler.

- Add factories for revoke event - Check in models line 36 if condition if correct ? `return if %w[approve refuse request_changes revoke].exclude?(name) && entity_type == 'AuthorizationRequest'` - Decorators

features/instruction/révocation_habilitation.feature

skelz0r · 2024-04-10T07:29:06Z

#137 (comment)

Étrange je n'ai pas eu d'emails 🙄

- Api Entreprise is in charge to send emails

skelz0r · 2024-04-10T08:09:02Z

app/queries/authorization_request_events_query.rb

+        authorization_request.denial_ids,
+        authorization_request.modification_request_ids,
+        authorization_request.changelog_ids,
+        authorization_request.message_ids,
+        authorization_request.authorization_ids,
+        authorization_request.revocation_ids


Ah clairement mieux

skelz0r

Let's ship it!

Isalafont force-pushed the feat/DAT-260/etq-i-je-peux-revoquer-une-habilitation-validee branch from eaebd84 to f973054 Compare March 27, 2024 14:09

Isalafont changed the title ~~Draft : Feat/dat 260/etq i je peux revoquer une habilitation validee~~ Feat/dat 260/etq i je peux revoquer une habilitation validee Mar 27, 2024

Isalafont requested review from skelz0r, mehdi-farsi and Un3x March 27, 2024 14:17

Un3x reviewed Mar 27, 2024

View reviewed changes

app/notifiers/base_notifier.rb Outdated Show resolved Hide resolved

db/migrate/20240326144413_add_revoke_constraints_to_events.rb Outdated Show resolved Hide resolved

Isalafont requested a review from Un3x March 27, 2024 15:10

Isalafont force-pushed the feat/DAT-260/etq-i-je-peux-revoquer-une-habilitation-validee branch 2 times, most recently from 65059a3 to dec6643 Compare April 3, 2024 12:34

Isalafont added 3 commits April 9, 2024 13:51

Add revoke state and event

3b31130

Add revocation controller / organizer and base notifier method for ma…

1e755e6

…iler.

Add policies

7886992

Isalafont and others added 18 commits April 9, 2024 13:51

Add policies and views for moderation button

621b28d

Add routes + revocation views + locales

770cb8c

Scout commit

399a169

Add migration revoke constraints

58add55

Add key to yam for the revoke status

0c390fe

- Add factories for revoke event - Check in models line 36 if condition if correct ? `return if %w[approve refuse request_changes revoke].exclude?(name) && entity_type == 'AuthorizationRequest'` - Decorators

Exclude revoked state from reopening label.

13c1e13

Cucumber Test for Revocation

0456d03

Add ingore_unused i18n revoke key

2e06b68

Correct migration file

b790564

add RevocationOfAuthorization model

9db8161

add CreateRevocationOfAuthorizationModel interactor

e69f5cf

add RevocationOfAuthorization to AuthorizationRequestEventsQuery

4af8674

add revocation to revoke modal

a831ad0

change revocation wording to revoke mailer action

de3a4f1

add :revoke trait to authorization_request_event factory

30a0d0b

rename refusal and revocation reason button label in features

5241316

Change wording for revocation modal

2323c63

Add margin top to flash message

ac29af7

Isalafont force-pushed the feat/DAT-260/etq-i-je-peux-revoquer-une-habilitation-validee branch from a311c7a to 86c7f1d Compare April 9, 2024 11:52

Isalafont commented Apr 9, 2024

View reviewed changes

features/instruction/révocation_habilitation.feature Show resolved Hide resolved

Isalafont added 2 commits April 10, 2024 09:41

Add irréversible message for refuse and revoke pop up action.

d5b3fcf

Fix cucumber by changing Api Entreprise to Api Particulier

b8e58d4

- Api Entreprise is in charge to send emails

Isalafont force-pushed the feat/DAT-260/etq-i-je-peux-revoquer-une-habilitation-validee branch from 86c7f1d to b8e58d4 Compare April 10, 2024 07:44

skelz0r reviewed Apr 10, 2024

View reviewed changes

skelz0r approved these changes Apr 10, 2024

View reviewed changes

Isalafont merged commit a811806 into develop Apr 10, 2024
7 checks passed

Isalafont deleted the feat/DAT-260/etq-i-je-peux-revoquer-une-habilitation-validee branch April 10, 2024 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/dat 260/etq i je peux revoquer une habilitation validee #137

Feat/dat 260/etq i je peux revoquer une habilitation validee #137

Isalafont commented Mar 26, 2024 •

edited

Loading

linear bot commented Mar 26, 2024

Isalafont commented Mar 27, 2024 •

edited

Loading

Un3x left a comment •

edited

Loading

mehdi-farsi commented Mar 27, 2024

Un3x commented Mar 27, 2024

mehdi-farsi commented Mar 27, 2024

skelz0r commented Mar 27, 2024 via email •

edited

Loading

Un3x commented Mar 27, 2024

mehdi-farsi commented Mar 27, 2024 •

edited

Loading

evaspae commented Apr 2, 2024 •

edited

Loading

Isalafont commented Apr 3, 2024

Isalafont commented Apr 3, 2024

evaspae commented Apr 4, 2024

evaspae commented Apr 4, 2024

mehdi-farsi commented Apr 8, 2024

mehdi-farsi commented Apr 8, 2024

skelz0r commented Apr 8, 2024

mehdi-farsi commented Apr 8, 2024

evaspae commented Apr 9, 2024

skelz0r commented Apr 10, 2024

skelz0r Apr 10, 2024

skelz0r left a comment

Feat/dat 260/etq i je peux revoquer une habilitation validee #137

Feat/dat 260/etq i je peux revoquer une habilitation validee #137

Conversation

Isalafont commented Mar 26, 2024 • edited Loading

linear bot commented Mar 26, 2024

Isalafont commented Mar 27, 2024 • edited Loading

Dans le cas d'une révocation :

Dans le cas du refus, l'utilisation de la même popup (review le wording)

Un3x left a comment • edited Loading

Choose a reason for hiding this comment

mehdi-farsi commented Mar 27, 2024

Un3x commented Mar 27, 2024

mehdi-farsi commented Mar 27, 2024

skelz0r commented Mar 27, 2024 via email • edited Loading

Un3x commented Mar 27, 2024

mehdi-farsi commented Mar 27, 2024 • edited Loading

evaspae commented Apr 2, 2024 • edited Loading

Pour la page d'accueil de l'instructeur :

Sur la page de l'habilitation :

Modale pour le motif de révocation :

Page d'accueil, une fois que la révocation est validée :

Isalafont commented Apr 3, 2024

Isalafont commented Apr 3, 2024

evaspae commented Apr 4, 2024

evaspae commented Apr 4, 2024

mehdi-farsi commented Apr 8, 2024

mehdi-farsi commented Apr 8, 2024

skelz0r commented Apr 8, 2024

mehdi-farsi commented Apr 8, 2024

evaspae commented Apr 9, 2024

skelz0r commented Apr 10, 2024

skelz0r Apr 10, 2024

Choose a reason for hiding this comment

skelz0r left a comment

Choose a reason for hiding this comment

Isalafont commented Mar 26, 2024 •

edited

Loading

Isalafont commented Mar 27, 2024 •

edited

Loading

Un3x left a comment •

edited

Loading

skelz0r commented Mar 27, 2024 via email •

edited

Loading

mehdi-farsi commented Mar 27, 2024 •

edited

Loading

evaspae commented Apr 2, 2024 •

edited

Loading