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

Déplacer la notion de volant du beneficiary vers le membership (re) #956

Closed
wants to merge 9 commits into from

Conversation

raphodn
Copy link
Member

@raphodn raphodn commented Aug 22, 2023

PR identique à #918, ré-ouverte pour la faire passer dans une release dédiée (et pouvoir tester master avant / après)

Quoi ?

Cette PR :

  • déplace la notion de volant du beneficiary vers le membership
  • met à jour les status (affichage, warning)

Note : la migration du statut de volant de beneficiary vers le membership récupére le status du main beneficiary et l'applique au membership.

Captures d'écran

Image
Compte fixe image
Compte volant image
Passer un compte en volant (ou fixe) image

@raphodn
Copy link
Member Author

raphodn commented Aug 22, 2023

Issues liées (?) à résoudre : #951, #952, #953

@raphodn raphodn linked an issue Sep 20, 2023 that may be closed by this pull request
8 tasks
@raphodn raphodn force-pushed the feature/move-flying-to-membership-2 branch from b4cbb8a to 657a0fc Compare September 20, 2023 08:30
<!-- Fermer le compte -->
{% if is_granted("ROLE_USER_MANAGER") and is_granted("close",member) %}
<li id="close">
<div class="collapsible-header {% if frontend_cookie and frontend_cookie.user_show is defined and frontend_cookie.user_show.close_open is defined and frontend_cookie.user_show.close_open %}active{% endif %}">
Copy link
Member Author

@raphodn raphodn Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pourquoi avoir déplacé la fermeture de compte ?? les collapsible permettent au contraire de faire des actions "importantes" (gel, suppression, roles), et de pouvoir mettre du texte explicatif avant le bouton

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deux choses :

  • il fallait trouver un endroit où mettre l'affichage fixe/volant et l'action de passer de volant à fixe (et vice versa)
  • la fermeture de compte n'était pas bien visible. Les comptes sont souvent gelés au lieu d'être fermés. L'idée était de mettre cette fonctionnalité plus en avant

Je trouve que tout mettre sous forme de collapsibles et donc mettre des infos hétérogères au même niveau n'est pas des plus efficaces en terme d'UI. Il faudrait essayer de hierachiser les choses. Typiquement, distinguer les actions, des informations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On est d'accord sur la hierarchisation, mon dernier commit fait justement ca
image

Copy link
Member Author

@raphodn raphodn Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Les comptes sont souvent gelés au lieu d'être fermés

les membres sont formés à ces actions, les comptes sont gelés si absence temporaire, et fermés si absence définitive, je veux bien voir les chiffres de ce que tu avances

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

j'ai rajouté des captures d'écran ! on pourra demander aux membres concernés (BDM, poste d'accueil) si en effet il y a un besoin de rendre plus visible ces boutons

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Les comptes sont souvent gelés au lieu d'être fermés

les membres sont formés à ces actions, les comptes sont gelés si absence temporaire, et fermés si absence définitive, je veux bien voir les chiffres de ce que tu avances

Je ne suis plus ce qui se passe actuellement mais avant c'était le cas. Donc je pars du principe qu'il y avait un problème niveau UX. Il faudrait qu'intuitivement les membres fassent les bonnes actions sans qu'il n'y ait pas besoin de formation. Pas simple l'UX :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je pense qu'il est plus intuitif de mettre les boutons actions dans un même collapsible que dans plusieurs. Avec un même collapsible, l'utilisateur a une vue globale sur l'ensemble des actions possibles. Sinon c'est caché, et donc sans formation, pas intuitif.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pour des actions qui ont lieu seulement 1 ou 2 fois dans le cycle de vie du compte, et qui ont des incidences (à terme automatiser la libération de créneau par exemple), je préfère les cacher et mettre des warning ^^

@@ -143,6 +143,7 @@ public function detachBeneficiaryAction(Request $request, Beneficiary $beneficia
$new_member->setMainBeneficiary($beneficiary);
}
// init other fields
$new_member->setFlying(false);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

En fait même si le champ a une valeur par défaut au niveau de l'entité, il semblerait qu'il faille rajouter cette ligne un peu partout, sinon ca plante...

Copy link
Member Author

@raphodn raphodn Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Faudrait tester, mais ca me semble bon.

Truc à faire avant la MEP : pour les comptes avec 2 bénéficiaires, basculer en mainBeneficiary les bénéficiaires qui ont un créneau fixe

Et peut-être faire un backup aussi au cas où 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

En fait même si le champ a une valeur par défaut au niveau de l'entité, il semblerait qu'il faille rajouter cette ligne un peu partout, sinon ca plante...

C'est étrange ça ! La valeur par défaut est bien prise en compte ? Elle n'est pas surchargée à un endroit (i.e., le false - valeur par défaut - se transformerait en null - valeur manquante) ?

@raphodn raphodn force-pushed the feature/move-flying-to-membership-2 branch from 620fd03 to e63b5b2 Compare September 20, 2023 12:13
@raphodn raphodn force-pushed the feature/move-flying-to-membership-2 branch 2 times, most recently from e4557e9 to 558cca1 Compare October 17, 2023 17:20
@raphodn
Copy link
Member Author

raphodn commented Oct 18, 2023

Pour permettre la rétro-compatibilité avec d'autres coop, je vais modifier cette PR pour garder le champ aussi sur le Beneficiary + rajouter un paramètre use_fly_and_fixed_on_entity pour permettre de choisir (entre Membership & Beneficiary)
Avec pour objectif de merger tout ça d'ici dimanche 🙏

@petitalb
Copy link
Collaborator

Pour permettre la rétro-compatibilité avec d'autres coop, je vais modifier cette PR pour garder le champ aussi sur le Beneficiary + rajouter un paramètre use_fly_and_fixed_on_entity pour permettre de choisir (entre Membership & Beneficiary) Avec pour objectif de merger tout ça d'ici dimanche 🙏

Y’a d’autres coops qui l’utilise au niveau du beneficiary ? Faudrait regarder l’historique mais je me demande si ce n’est pas un truc que j’avais introduit assez récemment donc sûrement peu utilisé voir pas utilisé.

Mon point est de dire que si personne l’utilise, vaut mieux ne pas l’implémenter pour limiter les options (il y en a déjà beaucoup) et le code mort. Quite a le documenter dans une issue et le reintroduire plus tard si le besoin est la ?

@petitalb
Copy link
Collaborator

Pour permettre la rétro-compatibilité avec d'autres coop, je vais modifier cette PR pour garder le champ aussi sur le Beneficiary + rajouter un paramètre use_fly_and_fixed_on_entity pour permettre de choisir (entre Membership & Beneficiary) Avec pour objectif de merger tout ça d'ici dimanche 🙏

Une difficulté à avoir la notion au niveau du beneficiary et du membership c’est les migrations pour passer de l’un à l’autre. Tu vois ça comment ?

@raphodn
Copy link
Member Author

raphodn commented Oct 18, 2023

Y’a d’autres coops qui l’utilise au niveau du beneficiary ? Faudrait regarder l’historique mais je me demande si ce n’est pas un truc que j’avais introduit assez récemment donc sûrement peu utilisé voir pas utilisé.

En effet je ne sais pas. Mais en même temps chaque semaine je découvre une nouvelle coop qui utilise (ou souhaite utiliser) cet espace membre.
Je me suis mal exprimé, la rétro-compatibilité parait en effet peu nécessaire car c'est une fonctionnalité introduite depuis "seulemen" 2 ans (décembre 2021 : f39df49). Je pensais aussi de donner le choix aux coop. Voir même pour nous de revenir en arrière avec simplement un paramètre. Je pense que la difficulté additionnelle est gérable 😅

@raphodn
Copy link
Member Author

raphodn commented Oct 18, 2023

Une difficulté à avoir la notion au niveau du beneficiary et du membership c’est les migrations pour passer de l’un à l’autre. Tu vois ça comment ?

En gros l'idée que j'ai, c'est de garder le fonctionnement actuel, et rajouter un flag pour les coop qui souhaitent passer au flying sur le Membership. Dans l'interface ca cacherait l'un ou l'autre en fonction

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Basculer la notion de fixe / volant au niveau du Membership
2 participants