Fix: ISE 500 on member add/remove when is already added/removed (#332)

* Fix: ISE 500 on member add/remove when is already added/removed

* Added appropriate test

mwdb/model/group.py

@@ -1,5 +1,7 @@
 from sqlalchemy.dialects.postgresql.array import ARRAY
+from sqlalchemy.exc import IntegrityError
 from sqlalchemy.ext.associationproxy import association_proxy
+from sqlalchemy.orm.exc import FlushError
 
 from mwdb.core.capabilities import Capabilities
 
@@ -45,6 +47,32 @@ def user_logins(self):
     def group_admins(self):
         return [member.user.login for member in self.members if member.group_admin]
 
+    def add_member(self, user):
+        if user in self.users:
+            return False
+
+        db.session.begin_nested()
+        try:
+            self.users.append(user)
+            db.session.commit()
+        except (FlushError, IntegrityError):
+            db.session.rollback()
+            return False
+        return True
+
+    def remove_member(self, user):
+        if user not in self.users:
+            return False
+
+        db.session.begin_nested()
+        try:
+            self.users.remove(user)
+            db.session.commit()
+        except (FlushError, IntegrityError):
+            db.session.rollback()
+            return False
+        return True
+
     @staticmethod
     def public_group():
         return Group.get_by_name(Group.PUBLIC_GROUP_NAME)

mwdb/resources/group.py

@@ -298,7 +298,8 @@ def post(self, name, login):
         if member.pending:
             raise Forbidden("User is pending and need to be accepted first")
 
-        group.users.append(member)
+        if not group.add_member(member):
+            raise Conflict("Member is already added")
         db.session.commit()
 
         logger.info(
@@ -353,6 +354,8 @@ def put(self, name, login):
                     group is immutable or user is pending
             404:
                 description: When user or group doesn't exist
+            409:
+                description: When member is already added
         """
         group_name_obj = load_schema({"name": name}, GroupNameSchemaBase())
 
@@ -434,6 +437,8 @@ def delete(self, name, login):
                     group is immutable or user is pending
             404:
                 description: When user or group doesn't exist
+            409:
+                description: When member is already removed
         """
         group_name_obj = load_schema({"name": name}, GroupNameSchemaBase())
         user_login_obj = load_schema({"login": login}, UserLoginSchemaBase())
@@ -471,7 +476,8 @@ def delete(self, name, login):
         if member.pending:
             raise Forbidden("User is pending and need to be accepted first")
 
-        group.users.remove(member)
+        if not group.remove_member(member):
+            raise Conflict("Member is already removed")
         db.session.commit()
 
         logger.info(

tests/backend/test_multigroup_groups.py

@@ -17,6 +17,24 @@ def test_member_public_groups():
         session.remove_member("public", Alice.identity)
 
 
+def test_existing_member_groups():
+    testCase = RelationTestCase()
+
+    Alice = testCase.new_user("Alice")
+    Group = testCase.new_group("Group")
+
+    session = MwdbTest()
+    session.login()
+
+    session.add_member(Group.identity, Alice.identity)
+    with ShouldRaise(status_code=409):
+        session.add_member(Group.identity, Alice.identity)
+
+    session.remove_member(Group.identity, Alice.identity)
+    with ShouldRaise(status_code=409):
+        session.remove_member(Group.identity, Alice.identity)
+
+
 def test_member_private_groups():
     testCase = RelationTestCase()