From 36076aefcc35b57b7475e888b2a4afd2223e0a71 Mon Sep 17 00:00:00 2001
From: imperosol <thgirod@hotmail.com>
Date: Fri, 7 Feb 2025 13:28:47 +0100
Subject: [PATCH] fix user groups update view
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Le formulaire remplaçait la totalité des groupes de l'utilisateur, c'est-à-dire également les groupes pas affichés dans le formulaire. Ça fait que la soumission du formulaire retirait l'utilisateur de tous ses groupes de groupes et des autres groupes non-gérables manuellement (comme Publique et Anciens Cotisants).

Jusqu'ici, les groupes non-manuels étaient gérés bizarrement, en regardant dynamiquement à chaque fois si l'utilisateur est dans le groupe, donc le bug ne se voyait pas. Maintenant que tous les groupes sont gérés presque de la même manière, ça se voit.
---
 core/tests/test_user.py | 23 ++++++++++++++++++++++-
 core/views/forms.py     | 13 +++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/core/tests/test_user.py b/core/tests/test_user.py
index 0e14bad8..9b2209b3 100644
--- a/core/tests/test_user.py
+++ b/core/tests/test_user.py
@@ -8,6 +8,7 @@ from django.urls import reverse
 from django.utils.timezone import now
 from model_bakery import baker, seq
 from model_bakery.recipe import Recipe, foreign_key
+from pytest_django.asserts import assertRedirects
 
 from com.models import News
 from core.baker_recipes import (
@@ -15,7 +16,7 @@ from core.baker_recipes import (
     subscriber_user,
     very_old_subscriber_user,
 )
-from core.models import User
+from core.models import Group, User
 from counter.models import Counter, Refilling, Selling
 from eboutic.models import Invoice, InvoiceItem
 
@@ -198,3 +199,23 @@ def test_user_added_to_public_group():
     user = baker.make(User)
     assert user.groups.filter(pk=settings.SITH_GROUP_PUBLIC_ID).exists()
     assert user.is_in_group(pk=settings.SITH_GROUP_PUBLIC_ID)
+
+
+@pytest.mark.django_db
+def test_user_update_groups(client: Client):
+    client.force_login(baker.make(User, is_superuser=True))
+    manageable_groups = baker.make(Group, is_manually_manageable=True, _quantity=3)
+    hidden_groups = baker.make(Group, is_manually_manageable=False, _quantity=4)
+    user = baker.make(User, groups=[*manageable_groups[1:], *hidden_groups[:3]])
+    response = client.post(
+        reverse("core:user_groups", kwargs={"user_id": user.id}),
+        data={"groups": [manageable_groups[0].id, manageable_groups[1].id]},
+    )
+    assertRedirects(response, user.get_absolute_url())
+    # only the manually manageable groups should have changed
+    assert set(user.groups.all()) == {
+        Group.objects.get(pk=settings.SITH_GROUP_PUBLIC_ID),
+        manageable_groups[0],
+        manageable_groups[1],
+        *hidden_groups[:3],
+    }
diff --git a/core/views/forms.py b/core/views/forms.py
index a0cdfb6b..0c0ec0e2 100644
--- a/core/views/forms.py
+++ b/core/views/forms.py
@@ -323,6 +323,19 @@ class UserGroupsForm(forms.ModelForm):
         model = User
         fields = ["groups"]
 
+    def save(self, *args, **kwargs) -> User:
+        # make the super method manage error without persisting in db
+        super().save(commit=False)
+        # Don't forget to add the non-manageable groups when setting groups,
+        # or the user would lose all of those when the form is submitted
+        self.instance.groups.set(
+            [
+                *self.cleaned_data["groups"],
+                *self.instance.groups.filter(is_manually_manageable=False),
+            ]
+        )
+        return self.instance
+
 
 class UserGodfathersForm(forms.Form):
     type = forms.ChoiceField(