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

Feature ouidou/admin creation delegation gestionnaire page group administrateur management #9571

Conversation

seb-by-ouidou
Copy link
Contributor

@seb-by-ouidou seb-by-ouidou commented Oct 6, 2023

@cmayran
Copy link

cmayran commented Oct 9, 2023

@tchak et @krichtof ready for review

@seb-by-ouidou seb-by-ouidou force-pushed the feature-ouidou/admin_creation_delegation_gestionnaire_page_group_administrateur_management branch from 74e917c to 2e02a06 Compare October 9, 2023 17:37
@tchak tchak marked this pull request as draft October 10, 2023 09:17
@seb-by-ouidou seb-by-ouidou force-pushed the feature-ouidou/admin_creation_delegation_gestionnaire_page_group_administrateur_management branch from 2e02a06 to c4b6db8 Compare November 8, 2023 16:15
@seb-by-ouidou seb-by-ouidou marked this pull request as ready for review November 8, 2023 16:17
@krichtof
Copy link
Contributor

Merci pour ta PR :)

Petite question : je vois bien dans les controllers le fait de pouvoir supprimer un admin d'un groupe, mais je ne le vois pas dans la vue. Ai-je mal regardé ? Ou peut-être dans une autre PR ?

@seb-by-ouidou
Copy link
Contributor Author

@krichtof c'est dans la deuxieme image ci dessus, avec la modal et le message de confirmation
voici le lien direct
https://user-images.githubusercontent.com/137039013/273252927-f95043e1-7fb1-4d85-bc38-2ae6441c26b8.png

@krichtof
Copy link
Contributor

Oui, je l'ai vu dans la screenshot mais pas dans le code.

@seb-by-ouidou
Copy link
Contributor Author

tu as le remove_button ici app/components/groupe_gestionnaire/groupe_gestionnaire_administrateurs/administrateur_component/administrateur_component.html.haml
|https://github.com/demarches-simplifiees/demarches-simplifiees.fr/pull/9571/files#diff-a8e814a8db3ea2a441fc3e00792f36449dae5947642ce6de4fb03e1d55df6650R5

@krichtof
Copy link
Contributor

Au temps pour moi, sorry pour le bruit. Je n'avais sur mon poste qu'un admin actif dans le groupe gestionnaire, c'est pour cette raison que le bouton n'était pas affiché

Copy link
Member

@colinux colinux left a comment

Choose a reason for hiding this comment

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

Review faite avec @krichtof, ça nous semble globalement très bien, merci !

Quelques retours généraux :

@seb-by-ouidou
Copy link
Contributor Author

merci pour les commentaires.

concernant ce point

dans le modèle GroupeGestionnaire, cela nous ennuie un peu qu'il y ait de grosses méthodes qui génèrent les alertes/notices alors que c'est de la logique de controller. Nous préfèrerions que ce code reste dans le controller (peut-être avec des méthodes privées), un peu à l'image de ce qui est fait dans https://github.com/demarches-simplifiees/demarches-simplifiees.fr/blob/main/app/controllers/instructeurs/groupe_instructeurs_controller.rb#L19
cela m obligera donc a avoir 2 fois le meme code dans app/controllers/manager/groupe_gestionnaires_controller.rb et dans app/controllers/gestionnaires/groupe_gestionnaire_gestionnaires_controller.rb
est ce bien cela que vous souhaitez ?

@seb-by-ouidou seb-by-ouidou force-pushed the feature-ouidou/admin_creation_delegation_gestionnaire_page_group_administrateur_management branch from c4b6db8 to ca54642 Compare November 16, 2023 12:55
@colinux
Copy link
Member

colinux commented Nov 16, 2023

Merci pour ton retour. Effectivement on n'avait pas percuté que depuis le manager on pouvait créer des groupes.

Du coup on s'interroge si le manager n'est utile que pour créer le groupe racine ? Car si c'est le cas, est-ce qu'on pourrait envisager de le créer autrement, par exemple :

  • avec une maintenance task qui créé le groupe racinet et met (par exemple) les super admins actifs gestionnaires.
    (Ex de maintenance task: app/tasks/maintenance/backfill_departement_services_task.rb Elles sont déclenchées à la main depuis leur interface, lien en bas du manager)
  • ou depuis le vue du manager super-admin, un simple bouton "ajouter dans le groupe racine" qui s'affiche s'il n'y est pas encore, et créé le groupe racine s'il n'existe pas encore
  • autre solution… ?

Ensuite le gestionnaire du groupe racine utilise l'UI que tu as créé pour ajouter d'autres gestionnaires/admins etc…

Comme c'est un bootstrap initité par l'opérateur / un super-admin, on peut ne pas avoir à gérer les duplicats, erreurs d'email etc… (qui font la complexité des méthodes du modèle), et on se fiche du message flash.

Ceci permettait de déplacer tout le code des grosses méthodes du modèle vers le controller, et le manager ne conserverait plus que les opérations de lecture pour les groupes et gestionnaires.

Qu'en penses-tu ?

@cmayran
Copy link

cmayran commented Nov 17, 2023

@colinux effectivement, dans ce contexte le manager permet :

  • l'activation de la fonctionnalité
  • l'ajout des gestionnaires du groupe racine et la création du groupe racine
  • la visualisation des gestionnaires et des groupes

Pour le moment il n'y a pas de restriction sur un seul groupe racine ou plusieurs ce qui explique peut être un duplicat (c'est une question qu'on s'est posée d'ailleurs).
Si ta proposition permet de garder de la clarté dans le code nous ça nous convient complètement :)

je laisse répondre @seb-by-ouidou également pour les suggestions :)

@seb-by-ouidou seb-by-ouidou force-pushed the feature-ouidou/admin_creation_delegation_gestionnaire_page_group_administrateur_management branch from ca54642 to 57b3dd3 Compare December 1, 2023 09:52
@seb-by-ouidou seb-by-ouidou force-pushed the feature-ouidou/admin_creation_delegation_gestionnaire_page_group_administrateur_management branch from 9ff87af to e34307b Compare December 6, 2023 13:47
Co-Authored-By: krichtof <christophe.robillard@beta.gouv.fr>
@colinux colinux force-pushed the feature-ouidou/admin_creation_delegation_gestionnaire_page_group_administrateur_management branch from 6e1ed79 to acbddb5 Compare December 12, 2023 10:31
Copy link
Member

@colinux colinux left a comment

Choose a reason for hiding this comment

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

OK pour nous, on a juste rajouté de toutes petites corrections de style/typo.

Merci !

@colinux colinux enabled auto-merge December 12, 2023 10:34
@colinux colinux added this pull request to the merge queue Dec 12, 2023
Merged via the queue into demarches-simplifiees:main with commit f9c4846 Dec 12, 2023
15 checks passed
@colinux colinux deleted the feature-ouidou/admin_creation_delegation_gestionnaire_page_group_administrateur_management branch December 12, 2023 10:51
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.

4 participants