Improve admin organization membership handling

An email is now always sent when an administrator joins a structure.

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)

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):
         """

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")

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(

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():