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

[TECH] Seuls les pilotes V3 pourront être pilotes séparation Pix/Pix+ (PIX-13171). #9395

Conversation

Steph0
Copy link
Contributor

@Steph0 Steph0 commented Jun 27, 2024

🦄 Problème

Modifier la règle métier sur Pix Admin, on peut etre pilote séparation pix / pix+ si on est V3. Inversement, on ne peut plus l'être si on est V2 (puisqu’on implémente pas le pilote en V2).

🤖 Proposition

  • Revoir la règle métier
    • vérifier que le centre est v3 pour être séparation pix/pix+
    • empêcher de retirer le centre des pilotes V3 si il a été aussi inscrit comme pilot séparation pix/pix+
    • empêcher un centre v2 d'être séparation pix+ (cas métier non souhaité)

🌈 Remarques

  • J'ai rendu l'erreur autour des "features" + générique. Comme ça c'est ré-utilisable et je me dis que si l'erreur venait à remonter, que l'on saurait identifier facilement pourquoi (sans avoir besoin d'un détail ultra précis niveau front)
  • J'ai mis des transactions avec isolation

Repeatable Read isolation level allows changes on what you have scanned, except for rows returned after all filtering conditions. This means that it will not be allowed if another transaction tries to update or delete the rows that you fetched.

source : https://dev.to/franckpachot/isolation-levels-part-vii-repeatable-read-l11 (j'aurais pu mettre d'autres sources + solides, mais j'ai bien aimé la formulation de celle-ci)

💯 Pour tester

Version rapide : https://admin-pr9395.review.pix.fr/certification-centers/7304 et tenter de décocher pilote V3.

Version complète de tout ce qui est mise en branle par cette PR

  • Pix Admin, créer un centre pilote V3
  • Script api/scripts/certification/import-pilot-certification-centers-from-csv.js ; ajouter le centre créé en tant que pilot séparation Pix/Pix+
  • Sur Pix Admin (rafraichir page) vérifier que le centre est bien à la fois V3 et Pix+ pilote
  • Modifier le centre, décocher la case "pilote V3"
  • Sauvegarder, une erreur apparait informant d'une incompatibilité entre les fonctionalités pilotes
  • Rafraichir la page, constater que le centre est toujours V3 et pilote

@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@Steph0 Steph0 changed the title [TECH] Seuls les pilotes V3 pourront être pilotes spération Pix/Pix+ (PIX-13171). [TECH] Seuls les pilotes V3 pourront être pilotes séparation Pix/Pix+ (PIX-13171). Jun 27, 2024
@mcampourcy mcampourcy marked this pull request as draft July 2, 2024 08:21
@Steph0 Steph0 force-pushed the PIX-13171-allow-V3-centers-to-be-pix-pix+-separation-pilot branch 5 times, most recently from 09393cc to 7f606c9 Compare July 8, 2024 12:11
@Steph0 Steph0 marked this pull request as ready for review July 8, 2024 12:14
@Steph0 Steph0 requested a review from a team as a code owner July 8, 2024 12:14
@AndreiaPena
Copy link
Member

AndreiaPena commented Jul 9, 2024

Plusieurs choses que je remarque :

  • On a encore ce soucis coté front qui donne l'impression que le centre n'est plus pilote v3 alors que a modification n'a pas été pris en compte

  • Je savais que de décocher la V3 allait générer une erreur (c'est le but de la PR) mais à la lecture du message d'erreur j'ai un peu buggé sur la formulation.
    Peut être parce que j'étais dans le cas de vouloir passer mon centre en V2, qui n'est pas considéré comme fonctionnalité pilote (la V3 en revanche est pilote comme la séparation).
    Mais l'important est que ce message soit compris par le métier qui va faire la manip;

  • Première fois que je lis qu'un centre est "abonné" à, intéressant. Est-ce le terme métier ?

Capture d’écran 2024-07-09 à 11 54 11

@P-Jeremy
Copy link
Contributor

Test func

Avant:
image

Après:
image
image

Question: On pourrait mettre à jour le modèle front et éviter d'avoir à rafraichir la page ? Pour ne pas se retrouver dans cet état qui prète à confusion

image

@Steph0 Steph0 force-pushed the PIX-13171-allow-V3-centers-to-be-pix-pix+-separation-pilot branch from 7f606c9 to 45a311e Compare July 12, 2024 12:29
@Steph0
Copy link
Contributor Author

Steph0 commented Jul 12, 2024

@AndreiaPena @P-Jeremy J'ai modifié le wording (vu avec andreia pour le wording). Par contre le rechargement je ne l'ai pas géré, j'ai essayé mais je n'ai pas réussi (le reload ne fonctionne pas car ce n'est pas le model, c'est un sous-model, le refresh controller ne se déclenche pas, peut-être à cause de la mécanique RSVP).

Comme "c'était comme ça avant" y compris dans la proécèdente version de cette erreur, je vais laisser ainsi. Mais si quelqu'un veut m'accompagner pour regarder ensemble je prends :)

@Steph0 Steph0 force-pushed the PIX-13171-allow-V3-centers-to-be-pix-pix+-separation-pilot branch 2 times, most recently from a8de561 to f3aba01 Compare July 12, 2024 12:48
@Steph0 Steph0 force-pushed the PIX-13171-allow-V3-centers-to-be-pix-pix+-separation-pilot branch from e5b7ac7 to d7408d0 Compare July 12, 2024 12:51
@pix-service-auto-merge pix-service-auto-merge merged commit 5f14b7f into dev Jul 12, 2024
4 of 7 checks passed
@pix-service-auto-merge pix-service-auto-merge deleted the PIX-13171-allow-V3-centers-to-be-pix-pix+-separation-pilot branch July 12, 2024 12:56
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.

None yet

6 participants