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 manager page #9564

Conversation

seb-by-ouidou
Copy link
Contributor

@seb-by-ouidou
Copy link
Contributor Author

seb-by-ouidou commented Oct 5, 2023

@krichtof ; @tchak ready for review

@seb-by-ouidou seb-by-ouidou force-pushed the feature-ouidou/admin_creation_delegation_manager_page branch from 6fceedb to cc67bf7 Compare October 9, 2023 15:00
<%= render("pagination", resources: attribute.resources(page_number), param_name: "#{attribute.name}[page]") %>
<% end %>

<% else %>
Copy link
Member

Choose a reason for hiding this comment

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

pourquoi y'a un else ici ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c'est un fichier auto generé par la gem administrate
le else dont tu parles es lié au <% if attribute.resources.any? %> au debut du fichier

@seb-by-ouidou seb-by-ouidou force-pushed the feature-ouidou/admin_creation_delegation_manager_page branch from cc67bf7 to 305aa46 Compare October 10, 2023 08:58
@tchak tchak marked this pull request as draft October 10, 2023 09:14
@seb-by-ouidou seb-by-ouidou force-pushed the feature-ouidou/admin_creation_delegation_manager_page branch from 305aa46 to 2301a8f Compare October 10, 2023 09:49
@seb-by-ouidou seb-by-ouidou marked this pull request as ready for review October 10, 2023 09:50
Copy link
Member

@LeSim LeSim left a comment

Choose a reason for hiding this comment

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

ça me paraît bien :)

@@ -4,10 +4,24 @@ class GroupeGestionnaire < ApplicationRecord
has_many :administrateurs
has_and_belongs_to_many :gestionnaires

def root_groupe_gestionnaire?
groupe_gestionnaire.nil?
Copy link
Member

Choose a reason for hiding this comment

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

J'ai mis du temps à comprendre que groupe_gestionnaire ici désignait le parent et pas l'enfant. Du coup, est ce qu'on pourrait l'appeler, dans une autre pr, parent_groupe_gestionnaire ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

le comportement change dans une des PR suivante
https://github.com/demarches-simplifiees/demarches-simplifiees.fr/pull/9566/files
l'attribut groupe_gestionnaire va disparaitre pour la gem ancestry

context 'when there are many gestionnaires' do
before { remove_gestionnaire(new_gestionnaire) }

it { expect(groupe_gestionnaire.gestionnaires).to include(gestionnaire) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Peut-être tester le fait que new_gestionnaire ne fait plus partie de groupe_gestionnaire ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c'est fait indirectement

    expect(groupe_gestionnaire.gestionnaires).to include(gestionnaire)
    expect(groupe_gestionnaire.reload.gestionnaires.count).to eq(1)

on teste que il n'y a "gestionnaire" dans groupe_gestionnaire.gestionnaires et qu;il y a que un seul gestionnaire
donc new_gestionnaire ne fait plus partie de groupe_gestionnaire

@seb-by-ouidou seb-by-ouidou force-pushed the feature-ouidou/admin_creation_delegation_manager_page branch from 03abbb2 to 92ffd6a Compare October 11, 2023 07:38
@tchak tchak added this pull request to the merge queue Oct 11, 2023
Merged via the queue into demarches-simplifiees:main with commit c1d6caf Oct 11, 2023
15 checks passed
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