From 2ad7e4582fbbd6f4ef35dfb8bd7857dfae861825 Mon Sep 17 00:00:00 2001
From: Jose Buitron <jose.buitron@gmail.com>
Date: Wed, 21 Feb 2024 16:04:42 -0500
Subject: [PATCH] fix: Validate managers count on update (#1169)

---
 terraso_backend/apps/core/permission_rules.py | 15 +++++++++++
 .../test_group_membership_mutations.py        | 25 +++++++++++++------
 .../test_landscape_membership_mutations.py    |  2 ++
 3 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/terraso_backend/apps/core/permission_rules.py b/terraso_backend/apps/core/permission_rules.py
index 118283e0c..3fa79311d 100644
--- a/terraso_backend/apps/core/permission_rules.py
+++ b/terraso_backend/apps/core/permission_rules.py
@@ -84,8 +84,16 @@ def validate_managers_count(user, membership, entity):
     Returns:
     bool: True if the count of managers is valid after the membership change, False otherwise.
     """
+    is_new = not membership
+    if is_new:
+        return True
+
     manager_role = get_manager_role(entity)
     is_manager = entity.membership_list.has_role(user, manager_role)
+
+    if not is_manager:
+        return True
+
     managers_count = entity.membership_list.memberships.by_role(manager_role).count()
     is_own_membership = user.collaboration_memberships.filter(pk=membership.id).exists()
 
@@ -137,6 +145,7 @@ def validate_change_membership(user, entity, obj):
                  - "user_role": The role the user will have after the change.
                  - "user_exists": A boolean indicating if the user is already registered.
                  - "user_email": The email of the user whose membership is being changed.
+                 - "current_membership": The current user membership
 
     Returns:
     bool: True if the user is allowed to change the membership, False otherwise.
@@ -144,6 +153,12 @@ def validate_change_membership(user, entity, obj):
     user_role = obj.get("user_role")
     user_exists = obj.get("user_exists")
     user_email = obj.get("user_email")
+    current_membership = obj.get("current_membership")
+
+    valid_managers_count = validate_managers_count(user, current_membership, entity)
+
+    if not valid_managers_count:
+        return False
 
     manager_role = get_manager_role(entity)
     is_manager = entity.membership_list.has_role(user, manager_role)
diff --git a/terraso_backend/tests/graphql/mutations/test_group_membership_mutations.py b/terraso_backend/tests/graphql/mutations/test_group_membership_mutations.py
index 8d85cdc90..271129ea8 100644
--- a/terraso_backend/tests/graphql/mutations/test_group_membership_mutations.py
+++ b/terraso_backend/tests/graphql/mutations/test_group_membership_mutations.py
@@ -30,10 +30,13 @@ def test_group_membership_add(client_query, groups, users):
     group = groups[0]
     user = users[0]
 
+    group.membership_list.memberships.all().delete()
+
     response = client_query(
         """
         mutation addMembership($input: GroupMembershipSaveMutationInput!){
           saveGroupMembership(input: $input) {
+            errors
             memberships {
               id
               userRole
@@ -432,15 +435,23 @@ def test_group_membership_approve_by_member_fails(client_query, users, groups_cl
     )
 
 
-def test_group_membership_update_role_by_last_manager_fails(
-    client_query, users, group_manager_memberships, groups
-):
-    manager_membership = group_manager_memberships[0]
+def test_group_membership_update_role_by_last_manager_fails(client_query, users, groups):
+    user = users[0]
     group = groups[0]
 
-    manager_membership.delete()
+    group.membership_list.memberships.all().delete()
 
-    old_membership = group.membership_list.memberships.filter(user=users[1]).first()
+    group.membership_list.save_membership(
+        user_email=user.email,
+        user_role=group_collaboration_roles.ROLE_MANAGER,
+        membership_status=CollaborationMembership.APPROVED,
+    )
+
+    group.membership_list.save_membership(
+        user_email=users[1].email,
+        user_role=group_collaboration_roles.ROLE_MEMBER,
+        membership_status=CollaborationMembership.APPROVED,
+    )
 
     response = client_query(
         """
@@ -459,7 +470,7 @@ def test_group_membership_update_role_by_last_manager_fails(
         """,
         variables={
             "input": {
-                "userEmails": [old_membership.user.email],
+                "userEmails": [user.email],
                 "userRole": group_collaboration_roles.ROLE_MEMBER,
                 "groupSlug": group.slug,
             }
diff --git a/terraso_backend/tests/graphql/mutations/test_landscape_membership_mutations.py b/terraso_backend/tests/graphql/mutations/test_landscape_membership_mutations.py
index d9dd4f077..e15466bfe 100644
--- a/terraso_backend/tests/graphql/mutations/test_landscape_membership_mutations.py
+++ b/terraso_backend/tests/graphql/mutations/test_landscape_membership_mutations.py
@@ -31,6 +31,8 @@ def test_landscape_membership_add(client_query, managed_landscapes, users):
     landscape = managed_landscapes[0]
     user = users[0]
 
+    landscape.membership_list.memberships.all().delete()
+
     response = client_query(
         """
         mutation addLandscapeMembership($input: LandscapeMembershipSaveMutationInput!){