Séparer un bénéficiaire d'un membre #591

raphodn · 2022-11-03T14:56:30Z

Quoi

Tentative rapide pour permettre de séparer 2 bénéficiaires

rajouter un bouton pour le bénéficiaire secondaire
popup et url pour séparer ce bénéficiaire, et le rattacher à un nouveau membership (ou un membership existant si il est mainBeneficiary d'un vieux membership)

Questions en suspens :

Membership.registration ?
Nécessité de ré-adhérer
compteur de temps, historique de créneaux

Captures d'écran

Bouton sur le bénéficiaire non-principal (réservé aux ADMIN pour l'instant)	modale de confirmation

petitalb · 2022-11-03T15:09:29Z

Membership.registration ?

Aucune pour moi, le nouveau membre doit réadhérer.

Ca pourrait être aussi une bonne alternative pour garder les bénéficiary comme actuellement.
=> supprimer un bénéficiary reviendrait à séparer le compte et à fermer le nouveau compte.

compteur de temps, historique de créneaux

Le compteur reprend à 0 (comme un nouveau compte) donc pas de log de temps.
Tu perds l'historique des créneaux. Les créneaux c'est lié à un compte, qui dit nouveau compte, il n'y a donc pas d'historique.

src/AppBundle/Controller/BeneficiaryController.php

petitalb · 2022-11-03T15:13:58Z

src/AppBundle/Controller/BeneficiaryController.php

+                $m = $em->getRepository('AppBundle:Membership')->findOneBy(array(), array('member_number' => 'DESC'));
+                $mm = 1;
+                if ($m)
+                    $mm = $m->getMemberNumber() + 1;


C'est très dangereux de faire ça. Les id c'est en autoincrement dans la DB, on ne devrait pas les gérer directement dans le code. Si jamais il y a deux créations en même temps, on arriverait dans une situation complètement incohérente.

En fait, je ne suis pas sur du autoincrement (pas regardé) mais ça devrait être le cas pour être sur de ne pas faire de bétise.

j'ai repris le bout de code qui existe déjà dans MembershipController > member_new
donc il n'y a pas besoin ? new Membership() va initialiser automatiquement le member_number ?

Je ne sais pas comment symfony gère des plusieurs requêtes simultannées, mais s'il y a des threads ce code n'est pas thread safe.

Pour moi, faudrait meettre le champs en autoincrement et laisser la DB gérer.

Pour l'unicité il y a cette PR qu'on a mergé, mais ça n'avait pas provoqué de migration... #606

Je viens de rajouter la migration

Pour l'id / member_number il faut que je reflechisse aux cas particuliers 🤔 Il doit y avoir une raison d'avoir séparé les 2 non ?

Les 2 numéros ne se suivent pas tout à fait. Si on remonte dans l'historique on voit qu'il y a des trous dans les id par endroit mais pas dans les member_number. J'imagine que c'est quand il y a un problème quelconque lors de la création d'un membership. Les événements semblent assez rare (ça s'est produit 52 fois en tout, 8 fois en 2021 et rien en 2022). Etait-ce du à l'utilisation du bouton "Nouvelle adhésion" ?

Je pense que le champ member_number est important à conserver de manière indépendante de l'ID car je crois que dans notre coop à La Rochelle par exemple, la personne qui gère les adhésions utilise ce numéro sur d'autres outils pour identifier les coopérateurs.

@quot17 et c'est embêtant pour vous du coup la séparation telle que proposée ? Vu qu'on va créer un nouveau membre et qu'on va lui associer un nouveau member_number ?

@petitalb non non c'est très bien, je ne crois pas qu'on renseigne les bénéficiaires sur Elefan chez nous actuellement.

src/AppBundle/Controller/MembershipController.php

raphodn · 2022-11-03T15:19:31Z

supprimer un bénéficiary reviendrait à séparer le compte et à fermer le nouveau compte

yes exactement ! séparation + new Membership + membership.isWithdrawn

même si je me dis qu'il y a peut-être moyen de résoudre ca "à la source" (en créant un membership à la création du compte, et le garder somehow même si le beneficiaire est rattaché ? ca permettrait de garder le member_number, et ne pas avoir à recréer un membership quand le bénéficiaire décide de se séparer 🤔 )

aussi pour les stats : ca ne les fausserait pas sur les dates de création

raphodn · 2022-11-15T19:23:29Z

@petitalb tu vois des choses qui manquent ? ou ca peut déjà être une première version (réservé aux ADMIN pour l'instant)

idées supplémentaires :

créer automatiquement une note sur le membership existant indiquant qu'un des bénéficiaire est parti
créer automatiquement une note sur le nouveau membership indiquant qu'il provient d'une séparation

petitalb · 2022-11-16T10:01:29Z

@petitalb tu vois des choses qui manquent ? ou ca peut déjà être une première version (réservé aux ADMIN pour l'instant)

idées supplémentaires :
* créer automatiquement une note sur le membership existant indiquant qu'un des bénéficiaire est parti

* créer automatiquement une note sur le nouveau membership indiquant qu'il provient d'une séparation

Ca me parait peut-être bien de rajouter les notes avant de merger. Je pense que c'est relativement simple à faire, et ca me parait important de tracer l'action pour comprendre à posteriori ce qui a pu se passer.

raphodn

looks good ! (je peux pas "approve" car je suis l'auteur de la PR ^^)

raphodn · 2022-11-16T09:55:26Z

src/AppBundle/Controller/BeneficiaryController.php

+        $session = new Session();
+        $member = $beneficiary->getMembership();
+
+        $this->denyAccessUnlessGranted('edit', $member);


remplacer par @Security("has_role('ROLE_ADMIN')") ?

raphodn · 2022-11-16T10:02:30Z

app/Resources/views/beneficiary/_partial/beneficiary_card.html.twig

 {{ form_start(delete_form) }}
 <div id="remove-beneficiary-{{ beneficiary.id }}" class="modal">
    <div class="modal-content">
        <h5><i class="material-icons left small">remove_circle_outline</i>Supprimer le bénéficiaire</h5>
-        <p>Attention, le bénéficiaire n'aura plus accès à son compte et ses informations seront perdus.</p>
+        <p>
+            Attention, le bénéficiaire n'aura plus accès à son compte et ses informations seront perdus.


peut-être indiquer les autres impacts ?

compteur temps reset ?

créneaux fixes restent, ...

app/Resources/views/beneficiary/_partial/beneficiary_card.html.twig

raphodn · 2022-11-16T10:05:08Z

Ca me parait peut-être bien de rajouter les notes avant de merger. Je pense que c'est relativement simple à faire, et ca me parait important de tracer l'action pour comprendre à posteriori ce qui a pu se passer.

@petitalb pour les notes on peut le faire dans une PR séparé (je vais essayer de la faire cette aprem)

vu que l'action sera réservé aux Admin pour l'instant il ne devrait pas y en avoir beaucoup d'ici là
on peut merger cette PR sans nécessairement la mettre en prod tout de suite (quoique je vais en avoir besoin pour mon créneau BdM de ce soir ^^)

edit : j'ai créé une issue à ce sujet : #619

* Super simple detach mechanism * [membership] Add uniq constraint on member_number attribute * Give access to beneficiary delete button to ROLE_ADMIN only Co-authored-by: Albin PETIT <albin.petit@gmail.com>

raphodn linked an issue Nov 3, 2022 that may be closed by this pull request

Pouvoir séparer un bénéficiaire d'un membre (et lui créer un nouveau membre autonome) #510

Closed

raphodn requested a review from petitalb November 3, 2022 15:00

raphodn self-assigned this Nov 3, 2022

petitalb reviewed Nov 3, 2022

View reviewed changes

src/AppBundle/Controller/BeneficiaryController.php Show resolved Hide resolved

petitalb reviewed Nov 3, 2022

View reviewed changes

src/AppBundle/Controller/MembershipController.php Show resolved Hide resolved

raphodn force-pushed the raphodn/member-beneficiary-detach-poc branch from 73a6a15 to 3fe391f Compare November 15, 2022 19:42

petitalb force-pushed the raphodn/member-beneficiary-detach-poc branch from 3fe391f to 9fb56a1 Compare November 16, 2022 05:31

petitalb changed the title ~~[POC] Séparer un bénéficiaire d'un membre~~ Séparer un bénéficiaire d'un membre Nov 16, 2022

petitalb marked this pull request as ready for review November 16, 2022 06:18

raphodn commented Nov 16, 2022

View reviewed changes

raphodn and others added 3 commits November 20, 2022 16:51

Super simple detach mechanism

80611c2

[membership] Add uniq constraint on member_number attribute

6116291

Give access to beneficiary delete button to ROLE_ADMIN only

8ac9e0f

raphodn force-pushed the raphodn/member-beneficiary-detach-poc branch from 9fb56a1 to 8ac9e0f Compare November 20, 2022 16:03

raphodn merged commit 42827b5 into master Nov 20, 2022

raphodn deleted the raphodn/member-beneficiary-detach-poc branch November 20, 2022 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Séparer un bénéficiaire d'un membre #591

Séparer un bénéficiaire d'un membre #591

raphodn commented Nov 3, 2022 •

edited

Loading

petitalb commented Nov 3, 2022

petitalb Nov 3, 2022

petitalb Nov 3, 2022

raphodn Nov 3, 2022

petitalb Nov 3, 2022

petitalb Nov 3, 2022

petitalb Nov 16, 2022

petitalb Nov 16, 2022

quot17 Nov 16, 2022

petitalb Nov 16, 2022

quot17 Nov 16, 2022

raphodn commented Nov 3, 2022 •

edited

Loading

raphodn commented Nov 15, 2022 •

edited

Loading

petitalb commented Nov 16, 2022

raphodn left a comment

raphodn Nov 16, 2022

raphodn Nov 16, 2022

raphodn commented Nov 16, 2022 •

edited

Loading

Séparer un bénéficiaire d'un membre #591

Séparer un bénéficiaire d'un membre #591

Conversation

raphodn commented Nov 3, 2022 • edited Loading

Quoi

Captures d'écran

petitalb commented Nov 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphodn commented Nov 3, 2022 • edited Loading

raphodn commented Nov 15, 2022 • edited Loading

petitalb commented Nov 16, 2022

raphodn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphodn commented Nov 16, 2022 • edited Loading

raphodn commented Nov 3, 2022 •

edited

Loading

raphodn commented Nov 3, 2022 •

edited

Loading

raphodn commented Nov 15, 2022 •

edited

Loading

raphodn commented Nov 16, 2022 •

edited

Loading