From 75318d88640a570775adeb9cf349f47a38b84b63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Freitag?= Date: Thu, 6 Feb 2025 14:42:05 +0100 Subject: [PATCH] Improve admin organization membership handling An email is now always sent when an administrator joins a structure. --- itou/common_apps/organizations/admin.py | 75 ++++++++++++------------ itou/common_apps/organizations/models.py | 8 ++- tests/companies/test_admin.py | 20 +++++-- tests/institutions/tests.py | 18 +++++- tests/prescribers/tests.py | 18 +++++- 5 files changed, 90 insertions(+), 49 deletions(-) diff --git a/itou/common_apps/organizations/admin.py b/itou/common_apps/organizations/admin.py index e06ee4b98d..378965e7fc 100644 --- a/itou/common_apps/organizations/admin.py +++ b/itou/common_apps/organizations/admin.py @@ -1,46 +1,44 @@ -import logging - from django.contrib import admin, messages from django.db.models import Count -from django.forms import BaseInlineFormSet, ModelForm +from django.forms import BaseInlineFormSet from itou.utils.admin import ItouModelAdmin, ItouTabularInline -logger = logging.getLogger(__name__) - - -def get_membership_structure(membership): - if hasattr(membership, "institution"): - return membership.institution - elif hasattr(membership, "organization"): - return membership.organization - elif hasattr(membership, "company"): - return membership.company - else: - logger.error("Invalid membership kind : %s", membership) - - -class MembersInlineForm(ModelForm): - def save(self, commit=True): - instance = super().save(commit=commit) - if "is_admin" in self.changed_data: - structure = get_membership_structure(instance) - if structure is not None: - if instance.is_admin: - structure.add_admin_email(instance.user).send() - else: - structure.remove_admin_email(instance.user).send() - return instance - - class MembersInlineFormSet(BaseInlineFormSet): + def __init__(self, *args, acting_user, **kwargs): + self.acting_user = acting_user + return super().__init__(*args, **kwargs) + + def _handle_save(self, form): + if form.instance.pk: + membership = form.instance + if "is_admin" in form.changed_data: + self.instance.set_admin_role( + membership, + form.cleaned_data["is_admin"], + updated_by=self.acting_user, + ) + else: + membership = self.instance.add_or_activate_membership( + form.cleaned_data["user"], + force_admin=form.cleaned_data["is_admin"], + ) + return membership + + def save_new(self, form, commit=True): + if commit: + return self._handle_save(form) + return super().save_new(form, commit=commit) + + def save_existing(self, form, obj, commit=True): + if commit: + return self._handle_save(form) + return super().save_existing(form, obj, commit=commit) + def delete_existing(self, obj, commit=True): - if obj.is_admin is True: - structure = get_membership_structure(obj) - if structure is not None: - structure.remove_admin_email(obj.user).send() - super().delete_existing(obj, commit=commit) + if commit: + self.instance.deactivate_membership(obj, updated_by=self.acting_user) class MembersInline(ItouTabularInline): @@ -49,7 +47,6 @@ class MembersInline(ItouTabularInline): extra = 1 raw_id_fields = ("user",) readonly_fields = ("is_active", "created_at", "updated_at", "updated_by", "joined_at") - form = MembersInlineForm formset = MembersInlineFormSet @@ -77,6 +74,12 @@ def member_count(self, obj): def get_queryset(self, request): return super().get_queryset(request).annotate(_member_count=Count("members", distinct=True)) + def get_formset_kwargs(self, request, obj, inline, prefix): + kwargs = super().get_formset_kwargs(request, obj, inline, prefix) + if isinstance(inline, MembersInline): + kwargs["acting_user"] = request.user + return kwargs + def save_related(self, request, form, formsets, change): had_admin = change and form.instance.active_admin_members.exists() super().save_related(request, form, formsets, change) diff --git a/itou/common_apps/organizations/models.py b/itou/common_apps/organizations/models.py index e538044f5f..e68d8035d1 100644 --- a/itou/common_apps/organizations/models.py +++ b/itou/common_apps/organizations/models.py @@ -102,18 +102,19 @@ def expire_invitations(self, user, *, include_sent_invitations): }, ) - def add_or_activate_membership(self, user): + def add_or_activate_membership(self, user, *, force_admin=None): membership_model = self.members.through is_only_active_member = not self.memberships.active().exists() + should_be_admin = is_only_active_member if force_admin is None else force_admin try: membership = self.memberships.get(user=user) except membership_model.DoesNotExist: - membership = self.memberships.create(user=user, is_admin=is_only_active_member) + membership = self.memberships.create(user=user, is_admin=should_be_admin) action = "Creating" else: action = "Reactivating" membership.is_active = True - membership.is_admin = is_only_active_member + membership.is_admin = should_be_admin membership.save(update_fields=["is_active", "is_admin"]) self.expire_invitations(user, include_sent_invitations=False) logger.info( @@ -129,6 +130,7 @@ def add_or_activate_membership(self, user): ) if membership.is_admin: self.add_admin_email(membership.user).send() + return membership def deactivate_membership(self, membership, *, updated_by): """ diff --git a/tests/companies/test_admin.py b/tests/companies/test_admin.py index 4f9a30d808..bf80f4a7d8 100644 --- a/tests/companies/test_admin.py +++ b/tests/companies/test_admin.py @@ -77,7 +77,7 @@ def test_deactivate_last_admin(self, admin_client, mailoutbox): assert_set_admin_role__removal(membership.user, company, mailoutbox) - def test_delete_admin(self, admin_client, mailoutbox): + def test_delete_admin(self, admin_client, caplog, mailoutbox): company = CompanyFactory(with_membership=True) membership = company.memberships.first() assert membership.is_admin @@ -114,9 +114,17 @@ def test_delete_admin(self, admin_client, mailoutbox): assertRedirects(response, change_url, fetch_redirect_response=False) response = admin_client.get(change_url) - assert_set_admin_role__removal(membership.user, company, mailoutbox) - - def test_add_admin(self, admin_client, mailoutbox): + assert membership.user not in company.active_admin_members + [email] = mailoutbox + assert f"[DEV] [Désactivation] Vous n'êtes plus membre de {company.display_name}" == email.subject + assert "Un administrateur vous a retiré d'une structure" in email.body + assert email.to == [membership.user.email] + assert ( + f"User {admin_client.session['_auth_user_id']} deactivated companies.CompanyMembership " + f"of organization_id={company.pk} for user_id={membership.user_id} is_admin=False." + ) in caplog.messages + + def test_add_admin(self, admin_client, caplog, mailoutbox): company = CompanyFactory(with_membership=True) membership = company.memberships.first() employer = EmployerFactory() @@ -157,6 +165,10 @@ def test_add_admin(self, admin_client, mailoutbox): response = admin_client.get(change_url) assert_set_admin_role__creation(employer, company, mailoutbox) + assert ( + f"Creating companies.CompanyMembership of organization_id={company.pk} " + f"for user_id={employer.pk} is_admin=True." + ) in caplog.messages @freeze_time("2024-05-17T11:11:11+02:00") diff --git a/tests/institutions/tests.py b/tests/institutions/tests.py index 89bf408cf2..c19a47aa83 100644 --- a/tests/institutions/tests.py +++ b/tests/institutions/tests.py @@ -174,7 +174,7 @@ def test_deactivate_last_admin(admin_client, mailoutbox): assert_set_admin_role__removal(membership.user, institution, mailoutbox) -def test_delete_admin(admin_client, mailoutbox): +def test_delete_admin(admin_client, caplog, mailoutbox): institution = InstitutionWithMembershipFactory(department="") membership = institution.memberships.first() assert membership.is_admin @@ -209,10 +209,18 @@ def test_delete_admin(admin_client, mailoutbox): assertRedirects(response, change_url, fetch_redirect_response=False) response = admin_client.get(change_url) - assert_set_admin_role__removal(membership.user, institution, mailoutbox) + assert membership.user not in institution.active_admin_members + [email] = mailoutbox + assert f"[DEV] [Désactivation] Vous n'êtes plus membre de {institution.display_name}" == email.subject + assert "Un administrateur vous a retiré d'une structure" in email.body + assert email.to == [membership.user.email] + assert ( + f"User {admin_client.session['_auth_user_id']} deactivated institutions.InstitutionMembership " + f"of organization_id={institution.pk} for user_id={membership.user_id} is_admin=False." + ) in caplog.messages -def test_add_admin(admin_client, mailoutbox): +def test_add_admin(admin_client, caplog, mailoutbox): institution = InstitutionWithMembershipFactory(department="") membership = institution.memberships.first() labor_inspector = LaborInspectorFactory() @@ -251,6 +259,10 @@ def test_add_admin(admin_client, mailoutbox): response = admin_client.get(change_url) assert_set_admin_role__creation(labor_inspector, institution, mailoutbox) + assert ( + f"Creating institutions.InstitutionMembership of organization_id={institution.pk} " + f"for user_id={labor_inspector.pk} is_admin=True." + ) in caplog.messages @pytest.mark.parametrize( diff --git a/tests/prescribers/tests.py b/tests/prescribers/tests.py index ee2431df62..0bebae5007 100644 --- a/tests/prescribers/tests.py +++ b/tests/prescribers/tests.py @@ -935,7 +935,7 @@ def test_deactivate_last_admin(admin_client, mailoutbox): assert_set_admin_role__removal(membership.user, organization, mailoutbox) -def test_delete_admin(admin_client, mailoutbox): +def test_delete_admin(admin_client, caplog, mailoutbox): organization = PrescriberOrganizationWithMembershipFactory() membership = organization.memberships.first() assert membership.is_admin @@ -978,10 +978,18 @@ def test_delete_admin(admin_client, mailoutbox): assertRedirects(response, change_url, fetch_redirect_response=False) response = admin_client.get(change_url) - assert_set_admin_role__removal(membership.user, organization, mailoutbox) + assert membership.user not in organization.active_admin_members + [email] = mailoutbox + assert f"[DEV] [Désactivation] Vous n'êtes plus membre de {organization.display_name}" == email.subject + assert "Un administrateur vous a retiré d'une structure" in email.body + assert email.to == [membership.user.email] + assert ( + f"User {admin_client.session['_auth_user_id']} deactivated prescribers.PrescriberMembership " + f"of organization_id={organization.pk} for user_id={membership.user_id} is_admin=False." + ) in caplog.messages -def test_add_admin(admin_client, mailoutbox): +def test_add_admin(admin_client, caplog, mailoutbox): organization = PrescriberOrganizationWithMembershipFactory() membership = organization.memberships.first() prescriber = PrescriberFactory() @@ -1028,6 +1036,10 @@ def test_add_admin(admin_client, mailoutbox): response = admin_client.get(change_url) assert_set_admin_role__creation(prescriber, organization, mailoutbox) + assert ( + f"Creating prescribers.PrescriberMembership of organization_id={organization.pk} " + f"for user_id={prescriber.pk} is_admin=True." + ) in caplog.messages def test_prescriber_kinds_are_alphabetically_sorted():