From 5e129c1f6a5f97abe329d7ea42cabeba6116b349 Mon Sep 17 00:00:00 2001
From: davelopez <46503462+davelopez@users.noreply.github.com>
Date: Wed, 17 Feb 2021 20:12:07 +0100
Subject: [PATCH 1/7] Add API tests for groups

---
 lib/galaxy_test/api/test_groups.py | 112 +++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)
 create mode 100644 lib/galaxy_test/api/test_groups.py

diff --git a/lib/galaxy_test/api/test_groups.py b/lib/galaxy_test/api/test_groups.py
new file mode 100644
index 00000000000..c2c508ac48f
--- /dev/null
+++ b/lib/galaxy_test/api/test_groups.py
@@ -0,0 +1,112 @@
+import json
+
+from galaxy_test.base.populators import DatasetPopulator
+from ._framework import ApiTestCase
+
+
+class GroupsApiTestCase(ApiTestCase):
+
+    def setUp(self):
+        super().setUp()
+        self.dataset_populator = DatasetPopulator(self.galaxy_interactor)
+
+    def test_create_valid(self, group_name: str = None):
+        payload = self._build_valid_group_payload(group_name)
+        response = self._post("groups", payload, admin=True, json=True)
+        self._assert_status_code_is(response, 200)
+        group = response.json()[0]  # POST /api/groups returns a list
+        self._assert_valid_group(group)
+        return group
+
+    def test_create_only_admin(self):
+        response = self._post("groups", json=True)
+        self._assert_status_code_is(response, 403)
+
+    def test_create_invalid_params_raises_400(self):
+        payload = self._build_valid_group_payload()
+        payload["name"] = None
+        response = self._post("groups", payload, admin=True, json=True)
+        self._assert_status_code_is(response, 400)
+
+    def test_create_duplicated_name_raises_400(self):
+        payload = self._build_valid_group_payload()
+        response = self._post("groups", payload, admin=True, json=True)
+        self._assert_status_code_is(response, 200)
+
+        response = self._post("groups", payload, admin=True, json=True)
+        self._assert_status_code_is(response, 400)
+
+    def test_index(self):
+        self.test_create_valid()
+        response = self._get("groups", admin=True)
+        self._assert_status_code_is(response, 200)
+        groups = response.json()
+        assert isinstance(groups, list)
+        assert len(groups) > 0
+        for group in groups:
+            self._assert_valid_group(group)
+
+    def test_index_only_admin(self):
+        response = self._get("groups")
+        self._assert_status_code_is(response, 403)
+
+    def test_show(self):
+        group = self.test_create_valid()
+        group_id = group["id"]
+        response = self._get(f"groups/{group_id}", admin=True)
+        self._assert_status_code_is(response, 200)
+        response_group = response.json()
+        self._assert_valid_group(response_group)
+        self._assert_has_keys(response_group, "users_url", "roles_url")
+
+    def test_show_only_admin(self):
+        group = self.test_create_valid()
+        group_id = group["id"]
+        response = self._get(f"groups/{group_id}")
+        self._assert_status_code_is(response, 403)
+
+    def test_update(self):
+        group = self.test_create_valid(group_name="group-test")
+
+        group_id = group["id"]
+        updated_name = "group-test-updated"
+        update_payload = json.dumps({
+            "name": updated_name,
+        })
+        update_response = self._put(f"groups/{group_id}", data=update_payload, admin=True)
+        self._assert_status_code_is_ok(update_response)
+
+    def test_update_only_admin(self):
+        group = self.test_create_valid()
+        group_id = group["id"]
+        response = self._put(f"groups/{group_id}")
+        self._assert_status_code_is(response, 403)
+
+    def test_update_duplicating_name_raises_400(self):
+        group_a = self.test_create_valid()
+        group_b = self.test_create_valid()
+
+        # Update group_b with the same name as group_a
+        group_b_id = group_b["id"]
+        updated_name = group_a["name"]
+        update_payload = json.dumps({
+            "name": updated_name,
+        })
+        update_response = self._put(f"groups/{group_b_id}", data=update_payload, admin=True)
+        self._assert_error_code_is(update_response, 400)
+
+    def _assert_valid_group(self, group, assert_id=None):
+        self._assert_has_keys(group, "id", "name", "model_class", "url")
+        if assert_id is not None:
+            assert group["id"] == assert_id
+
+    def _build_valid_group_payload(self, name: str = None):
+        name = name or self.dataset_populator.get_random_name()
+        user_id = self.dataset_populator.user_id()
+        role_id = self.dataset_populator.user_private_role_id()
+        payload = {
+            "name": name,
+            "user_ids": [user_id],
+            "role_ids": [role_id],
+        }
+        return payload

From 719d8139eba4042d68c40090eaee2615e9f37eb7 Mon Sep 17 00:00:00 2001
From: davelopez <46503462+davelopez@users.noreply.github.com>
Date: Wed, 17 Feb 2021 20:48:45 +0100
Subject: [PATCH 2/7] Replace legacy expose_api decorators xref #11252

---
 lib/galaxy/webapps/galaxy/api/groups.py | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/lib/galaxy/webapps/galaxy/api/groups.py b/lib/galaxy/webapps/galaxy/api/groups.py
index a4ff3c90dd5..1f71fda1598 100644
--- a/lib/galaxy/webapps/galaxy/api/groups.py
+++ b/lib/galaxy/webapps/galaxy/api/groups.py
@@ -5,7 +5,10 @@ import logging
 
 from sqlalchemy import false
 
-from galaxy import web
+from galaxy.web import (
+    expose_api,
+    require_admin,
+)
 from galaxy.webapps.base.controller import BaseAPIController, url_for
 
 log = logging.getLogger(__name__)
@@ -13,8 +16,8 @@ log = logging.getLogger(__name__)
 
 class GroupAPIController(BaseAPIController):
 
-    @web.require_admin
-    @web.legacy_expose_api
+    @expose_api
+    @require_admin
     def index(self, trans, **kwd):
         """
         GET /api/groups
@@ -29,7 +32,8 @@ class GroupAPIController(BaseAPIController):
                 rval.append(item)
         return rval
 
-    @web.legacy_expose_api
+    @expose_api
+    @require_admin
     def create(self, trans, payload, **kwd):
         """
         POST /api/groups
@@ -71,8 +75,8 @@ class GroupAPIController(BaseAPIController):
         item['url'] = url_for('group', id=encoded_id)
         return [item]
 
-    @web.require_admin
-    @web.legacy_expose_api
+    @expose_api
+    @require_admin
     def show(self, trans, id, **kwd):
         """
         GET /api/groups/{encoded_group_id}
@@ -97,8 +101,8 @@ class GroupAPIController(BaseAPIController):
         item['roles_url'] = url_for('group_roles', group_id=group_id)
         return item
 
-    @web.require_admin
-    @web.legacy_expose_api
+    @expose_api
+    @require_admin
     def update(self, trans, id, payload, **kwd):
         """
         PUT /api/groups/{encoded_group_id}

From 4fdf4c50ef066fd8f4727064e715f67a626d215c Mon Sep 17 00:00:00 2001
From: davelopez <46503462+davelopez@users.noreply.github.com>
Date: Thu, 18 Feb 2021 12:18:24 +0100
Subject: [PATCH 3/7] Refactor move controller logic into GroupsManager

---
 lib/galaxy/managers/groups.py           | 107 ++++++++++++++++++++++++
 lib/galaxy/webapps/galaxy/api/groups.py |  97 +++------------------
 2 files changed, 117 insertions(+), 87 deletions(-)
 create mode 100644 lib/galaxy/managers/groups.py

diff --git a/lib/galaxy/managers/groups.py b/lib/galaxy/managers/groups.py
new file mode 100644
index 00000000000..e32a6dc4378
--- /dev/null
+++ b/lib/galaxy/managers/groups.py
@@ -0,0 +1,107 @@
+from sqlalchemy import false
+
+from galaxy.web import url_for
+
+
+class GroupsManager:
+    """Interface/service object shared by controllers for interacting with groups."""
+
+    def index(self, trans, **kwd):
+        """
+        Displays a collection (list) of groups.
+        """
+        rval = []
+        for group in trans.sa_session.query(trans.app.model.Group).filter(trans.app.model.Group.table.c.deleted == false()):
+            if trans.user_is_admin:
+                item = group.to_dict(value_mapper={'id': trans.security.encode_id})
+                encoded_id = trans.security.encode_id(group.id)
+                item['url'] = url_for('group', id=encoded_id)
+                rval.append(item)
+        return rval
+
+    def create(self, trans, payload, **kwd):
+        """
+        Creates a new group.
+        """
+        if not trans.user_is_admin:
+            trans.response.status = 403
+            return "You are not authorized to create a new group."
+        name = payload.get('name', None)
+        if not name:
+            trans.response.status = 400
+            return "Enter a valid name"
+        if trans.sa_session.query(trans.app.model.Group).filter(trans.app.model.Group.table.c.name == name).first():
+            trans.response.status = 400
+            return "A group with that name already exists"
+
+        group = trans.app.model.Group(name=name)
+        trans.sa_session.add(group)
+        user_ids = payload.get('user_ids', [])
+        users = [trans.sa_session.query(trans.model.User).get(trans.security.decode_id(i)) for i in user_ids]
+        role_ids = payload.get('role_ids', [])
+        roles = [trans.sa_session.query(trans.model.Role).get(trans.security.decode_id(i)) for i in role_ids]
+        trans.app.security_agent.set_entity_group_associations(groups=[group], roles=roles, users=users)
+        """
+        # Create the UserGroupAssociations
+        for user in users:
+            trans.app.security_agent.associate_user_group( user, group )
+        # Create the GroupRoleAssociations
+        for role in roles:
+            trans.app.security_agent.associate_group_role( group, role )
+        """
+        trans.sa_session.flush()
+        encoded_id = trans.security.encode_id(group.id)
+        item = group.to_dict(view='element', value_mapper={'id': trans.security.encode_id})
+        item['url'] = url_for('group', id=encoded_id)
+        return [item]
+
+    def show(self, trans, id, **kwd):
+        """
+        Displays information about a group.
+        """
+        group_id = id
+        try:
+            decoded_group_id = trans.security.decode_id(group_id)
+        except TypeError:
+            trans.response.status = 400
+            return "Malformed group id ( %s ) specified, unable to decode." % str(group_id)
+        try:
+            group = trans.sa_session.query(trans.app.model.Group).get(decoded_group_id)
+        except Exception:
+            group = None
+        if not group:
+            trans.response.status = 400
+            return "Invalid group id ( %s ) specified." % str(group_id)
+        item = group.to_dict(view='element', value_mapper={'id': trans.security.encode_id})
+        item['url'] = url_for('group', id=group_id)
+        item['users_url'] = url_for('group_users', group_id=group_id)
+        item['roles_url'] = url_for('group_roles', group_id=group_id)
+        return item
+
+    def update(self, trans, id, payload, **kwd):
+        """
+        Modifies a group.
+        """
+        group_id = id
+        try:
+            decoded_group_id = trans.security.decode_id(group_id)
+        except TypeError:
+            trans.response.status = 400
+            return "Malformed group id ( %s ) specified, unable to decode." % str(group_id)
+        try:
+            group = trans.sa_session.query(trans.app.model.Group).get(decoded_group_id)
+        except Exception:
+            group = None
+        if not group:
+            trans.response.status = 400
+            return "Invalid group id ( %s ) specified." % str(group_id)
+        name = payload.get('name', None)
+        if name:
+            group.name = name
+            trans.sa_session.add(group)
+        user_ids = payload.get('user_ids', [])
+        users = [trans.sa_session.query(trans.model.User).get(trans.security.decode_id(i)) for i in user_ids]
+        role_ids = payload.get('role_ids', [])
+        roles = [trans.sa_session.query(trans.model.Role).get(trans.security.decode_id(i)) for i in role_ids]
+        trans.app.security_agent.set_entity_group_associations(groups=[group], roles=roles, users=users, delete_existing_assocs=False)
+        trans.sa_session.flush()
diff --git a/lib/galaxy/webapps/galaxy/api/groups.py b/lib/galaxy/webapps/galaxy/api/groups.py
index 1f71fda1598..5b55c26d482 100644
--- a/lib/galaxy/webapps/galaxy/api/groups.py
+++ b/lib/galaxy/webapps/galaxy/api/groups.py
@@ -3,19 +3,22 @@ API operations on Group objects.
 """
 import logging
 
-from sqlalchemy import false
-
+from galaxy.managers.groups import GroupsManager
 from galaxy.web import (
     expose_api,
     require_admin,
 )
-from galaxy.webapps.base.controller import BaseAPIController, url_for
+from galaxy.webapps.base.controller import BaseAPIController
 
 log = logging.getLogger(__name__)
 
 
 class GroupAPIController(BaseAPIController):
 
+    def __init__(self, app):
+        super().__init__(app)
+        self.manager = GroupsManager()
+
     @expose_api
     @require_admin
     def index(self, trans, **kwd):
@@ -23,14 +26,7 @@ class GroupAPIController(BaseAPIController):
         GET /api/groups
         Displays a collection (list) of groups.
         """
-        rval = []
-        for group in trans.sa_session.query(trans.app.model.Group).filter(trans.app.model.Group.table.c.deleted == false()):
-            if trans.user_is_admin:
-                item = group.to_dict(value_mapper={'id': trans.security.encode_id})
-                encoded_id = trans.security.encode_id(group.id)
-                item['url'] = url_for('group', id=encoded_id)
-                rval.append(item)
-        return rval
+        return self.manager.index(trans)
 
     @expose_api
     @require_admin
@@ -39,41 +35,7 @@ class GroupAPIController(BaseAPIController):
         POST /api/groups
         Creates a new group.
         """
-        log.info("groups payload%s\n" % (payload))
-        if not trans.user_is_admin:
-            trans.response.status = 403
-            return "You are not authorized to create a new group."
-        name = payload.get('name', None)
-        if not name:
-            trans.response.status = 400
-            return "Enter a valid name"
-        if trans.sa_session.query(trans.app.model.Group).filter(trans.app.model.Group.table.c.name == name).first():
-            trans.response.status = 400
-            return "A group with that name already exists"
-
-        group = trans.app.model.Group(name=name)
-        trans.sa_session.add(group)
-        user_ids = payload.get('user_ids', [])
-        for i in user_ids:
-            log.info("user_id: %s\n" % (i))
-            log.info("{} {}\n".format(i, trans.security.decode_id(i)))
-        users = [trans.sa_session.query(trans.model.User).get(trans.security.decode_id(i)) for i in user_ids]
-        role_ids = payload.get('role_ids', [])
-        roles = [trans.sa_session.query(trans.model.Role).get(trans.security.decode_id(i)) for i in role_ids]
-        trans.app.security_agent.set_entity_group_associations(groups=[group], roles=roles, users=users)
-        """
-        # Create the UserGroupAssociations
-        for user in users:
-            trans.app.security_agent.associate_user_group( user, group )
-        # Create the GroupRoleAssociations
-        for role in roles:
-            trans.app.security_agent.associate_group_role( group, role )
-        """
-        trans.sa_session.flush()
-        encoded_id = trans.security.encode_id(group.id)
-        item = group.to_dict(view='element', value_mapper={'id': trans.security.encode_id})
-        item['url'] = url_for('group', id=encoded_id)
-        return [item]
+        return self.manager.create(trans, payload)
 
     @expose_api
     @require_admin
@@ -82,24 +44,7 @@ class GroupAPIController(BaseAPIController):
         GET /api/groups/{encoded_group_id}
         Displays information about a group.
         """
-        group_id = id
-        try:
-            decoded_group_id = trans.security.decode_id(group_id)
-        except TypeError:
-            trans.response.status = 400
-            return "Malformed group id ( %s ) specified, unable to decode." % str(group_id)
-        try:
-            group = trans.sa_session.query(trans.app.model.Group).get(decoded_group_id)
-        except Exception:
-            group = None
-        if not group:
-            trans.response.status = 400
-            return "Invalid group id ( %s ) specified." % str(group_id)
-        item = group.to_dict(view='element', value_mapper={'id': trans.security.encode_id})
-        item['url'] = url_for('group', id=group_id)
-        item['users_url'] = url_for('group_users', group_id=group_id)
-        item['roles_url'] = url_for('group_roles', group_id=group_id)
-        return item
+        return self.manager.show(trans, id)
 
     @expose_api
     @require_admin
@@ -108,26 +53,4 @@ class GroupAPIController(BaseAPIController):
         PUT /api/groups/{encoded_group_id}
         Modifies a group.
         """
-        group_id = id
-        try:
-            decoded_group_id = trans.security.decode_id(group_id)
-        except TypeError:
-            trans.response.status = 400
-            return "Malformed group id ( %s ) specified, unable to decode." % str(group_id)
-        try:
-            group = trans.sa_session.query(trans.app.model.Group).get(decoded_group_id)
-        except Exception:
-            group = None
-        if not group:
-            trans.response.status = 400
-            return "Invalid group id ( %s ) specified." % str(group_id)
-        name = payload.get('name', None)
-        if name:
-            group.name = name
-            trans.sa_session.add(group)
-        user_ids = payload.get('user_ids', [])
-        users = [trans.sa_session.query(trans.model.User).get(trans.security.decode_id(i)) for i in user_ids]
-        role_ids = payload.get('role_ids', [])
-        roles = [trans.sa_session.query(trans.model.Role).get(trans.security.decode_id(i)) for i in role_ids]
-        trans.app.security_agent.set_entity_group_associations(groups=[group], roles=roles, users=users, delete_existing_assocs=False)
-        trans.sa_session.flush()
+        self.manager.update(trans, id, payload)

From 71214ae5675fd635f37252cc33573929db7ef761 Mon Sep 17 00:00:00 2001
From: davelopez <46503462+davelopez@users.noreply.github.com>
Date: Thu, 18 Feb 2021 13:01:54 +0100
Subject: [PATCH 4/7] Add basic type annotations

---
 lib/galaxy/managers/groups.py           | 34 +++++++++++++++----------
 lib/galaxy/webapps/galaxy/api/groups.py | 14 +++++++---
 2 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/lib/galaxy/managers/groups.py b/lib/galaxy/managers/groups.py
index e32a6dc4378..6150b539a3f 100644
--- a/lib/galaxy/managers/groups.py
+++ b/lib/galaxy/managers/groups.py
@@ -1,17 +1,25 @@
+from typing import (
+    Any,
+    Dict,
+)
+
 from sqlalchemy import false
 
+from galaxy import model
+from galaxy.managers.context import ProvidesAppContext
+from galaxy.schema.fields import EncodedDatabaseIdField
 from galaxy.web import url_for
 
 
 class GroupsManager:
     """Interface/service object shared by controllers for interacting with groups."""
 
-    def index(self, trans, **kwd):
+    def index(self, trans: ProvidesAppContext):
         """
         Displays a collection (list) of groups.
         """
         rval = []
-        for group in trans.sa_session.query(trans.app.model.Group).filter(trans.app.model.Group.table.c.deleted == false()):
+        for group in trans.sa_session.query(model.Group).filter(model.Group.table.c.deleted == false()):
             if trans.user_is_admin:
                 item = group.to_dict(value_mapper={'id': trans.security.encode_id})
                 encoded_id = trans.security.encode_id(group.id)
@@ -19,7 +27,7 @@ class GroupsManager:
                 rval.append(item)
         return rval
 
-    def create(self, trans, payload, **kwd):
+    def create(self, trans: ProvidesAppContext, payload: Dict[str, Any]):
         """
         Creates a new group.
         """
@@ -30,16 +38,16 @@ class GroupsManager:
         if not name:
             trans.response.status = 400
             return "Enter a valid name"
-        if trans.sa_session.query(trans.app.model.Group).filter(trans.app.model.Group.table.c.name == name).first():
+        if trans.sa_session.query(model.Group).filter(model.Group.table.c.name == name).first():
             trans.response.status = 400
             return "A group with that name already exists"
 
-        group = trans.app.model.Group(name=name)
+        group = model.Group(name=name)
         trans.sa_session.add(group)
         user_ids = payload.get('user_ids', [])
-        users = [trans.sa_session.query(trans.model.User).get(trans.security.decode_id(i)) for i in user_ids]
+        users = [trans.sa_session.query(model.User).get(trans.security.decode_id(i)) for i in user_ids]
         role_ids = payload.get('role_ids', [])
-        roles = [trans.sa_session.query(trans.model.Role).get(trans.security.decode_id(i)) for i in role_ids]
+        roles = [trans.sa_session.query(model.Role).get(trans.security.decode_id(i)) for i in role_ids]
         trans.app.security_agent.set_entity_group_associations(groups=[group], roles=roles, users=users)
         """
         # Create the UserGroupAssociations
@@ -55,7 +63,7 @@ class GroupsManager:
         item['url'] = url_for('group', id=encoded_id)
         return [item]
 
-    def show(self, trans, id, **kwd):
+    def show(self, trans: ProvidesAppContext, id: EncodedDatabaseIdField):
         """
         Displays information about a group.
         """
@@ -66,7 +74,7 @@ class GroupsManager:
             trans.response.status = 400
             return "Malformed group id ( %s ) specified, unable to decode." % str(group_id)
         try:
-            group = trans.sa_session.query(trans.app.model.Group).get(decoded_group_id)
+            group = trans.sa_session.query(model.Group).get(decoded_group_id)
         except Exception:
             group = None
         if not group:
@@ -78,7 +86,7 @@ class GroupsManager:
         item['roles_url'] = url_for('group_roles', group_id=group_id)
         return item
 
-    def update(self, trans, id, payload, **kwd):
+    def update(self, trans: ProvidesAppContext, id: EncodedDatabaseIdField, payload: Dict[str, Any]):
         """
         Modifies a group.
         """
@@ -89,7 +97,7 @@ class GroupsManager:
             trans.response.status = 400
             return "Malformed group id ( %s ) specified, unable to decode." % str(group_id)
         try:
-            group = trans.sa_session.query(trans.app.model.Group).get(decoded_group_id)
+            group = trans.sa_session.query(model.Group).get(decoded_group_id)
         except Exception:
             group = None
         if not group:
@@ -100,8 +108,8 @@ class GroupsManager:
             group.name = name
             trans.sa_session.add(group)
         user_ids = payload.get('user_ids', [])
-        users = [trans.sa_session.query(trans.model.User).get(trans.security.decode_id(i)) for i in user_ids]
+        users = [trans.sa_session.query(model.User).get(trans.security.decode_id(i)) for i in user_ids]
         role_ids = payload.get('role_ids', [])
-        roles = [trans.sa_session.query(trans.model.Role).get(trans.security.decode_id(i)) for i in role_ids]
+        roles = [trans.sa_session.query(model.Role).get(trans.security.decode_id(i)) for i in role_ids]
         trans.app.security_agent.set_entity_group_associations(groups=[group], roles=roles, users=users, delete_existing_assocs=False)
         trans.sa_session.flush()
diff --git a/lib/galaxy/webapps/galaxy/api/groups.py b/lib/galaxy/webapps/galaxy/api/groups.py
index 5b55c26d482..43386c22b93 100644
--- a/lib/galaxy/webapps/galaxy/api/groups.py
+++ b/lib/galaxy/webapps/galaxy/api/groups.py
@@ -2,8 +2,14 @@
 API operations on Group objects.
 """
 import logging
+from typing import (
+    Any,
+    Dict,
+)
 
+from galaxy.managers.context import ProvidesAppContext
 from galaxy.managers.groups import GroupsManager
+from galaxy.schema.fields import EncodedDatabaseIdField
 from galaxy.web import (
     expose_api,
     require_admin,
@@ -21,7 +27,7 @@ class GroupAPIController(BaseAPIController):
 
     @expose_api
     @require_admin
-    def index(self, trans, **kwd):
+    def index(self, trans: ProvidesAppContext, **kwd):
         """
         GET /api/groups
         Displays a collection (list) of groups.
@@ -30,7 +36,7 @@ class GroupAPIController(BaseAPIController):
 
     @expose_api
     @require_admin
-    def create(self, trans, payload, **kwd):
+    def create(self, trans: ProvidesAppContext, payload: Dict[str, Any], **kwd):
         """
         POST /api/groups
         Creates a new group.
@@ -39,7 +45,7 @@ class GroupAPIController(BaseAPIController):
 
     @expose_api
     @require_admin
-    def show(self, trans, id, **kwd):
+    def show(self, trans: ProvidesAppContext, id: EncodedDatabaseIdField, **kwd):
         """
         GET /api/groups/{encoded_group_id}
         Displays information about a group.
@@ -48,7 +54,7 @@ class GroupAPIController(BaseAPIController):
 
     @expose_api
     @require_admin
-    def update(self, trans, id, payload, **kwd):
+    def update(self, trans: ProvidesAppContext, id: EncodedDatabaseIdField, payload: Dict[str, Any], **kwd):
         """
         PUT /api/groups/{encoded_group_id}
         Modifies a group.

From 559ba46302a57b8a6621bd14ea2656228bb4e886 Mon Sep 17 00:00:00 2001
From: davelopez <46503462+davelopez@users.noreply.github.com>
Date: Thu, 18 Feb 2021 14:33:09 +0100
Subject: [PATCH 5/7] Refactor error handling logic

---
 lib/galaxy/managers/groups.py           | 100 ++++++++++--------------
 lib/galaxy/webapps/galaxy/api/groups.py |   5 +-
 2 files changed, 46 insertions(+), 59 deletions(-)

diff --git a/lib/galaxy/managers/groups.py b/lib/galaxy/managers/groups.py
index 6150b539a3f..1c85418b2a0 100644
--- a/lib/galaxy/managers/groups.py
+++ b/lib/galaxy/managers/groups.py
@@ -6,6 +6,13 @@ from typing import (
 from sqlalchemy import false
 
 from galaxy import model
+from galaxy.app import StructuredApp
+from galaxy.exceptions import (
+    Conflict,
+    ObjectAttributeMissingException,
+    ObjectNotFound,
+)
+from galaxy.managers.base import decode_id
 from galaxy.managers.context import ProvidesAppContext
 from galaxy.schema.fields import EncodedDatabaseIdField
 from galaxy.web import url_for
@@ -14,102 +21,81 @@ from galaxy.web import url_for
 class GroupsManager:
     """Interface/service object shared by controllers for interacting with groups."""
 
+    def __init__(self, app: StructuredApp) -> None:
+        self._app = app
+
     def index(self, trans: ProvidesAppContext):
         """
         Displays a collection (list) of groups.
         """
         rval = []
         for group in trans.sa_session.query(model.Group).filter(model.Group.table.c.deleted == false()):
-            if trans.user_is_admin:
-                item = group.to_dict(value_mapper={'id': trans.security.encode_id})
-                encoded_id = trans.security.encode_id(group.id)
-                item['url'] = url_for('group', id=encoded_id)
-                rval.append(item)
+            item = group.to_dict(value_mapper={'id': trans.security.encode_id})
+            encoded_id = trans.security.encode_id(group.id)
+            item['url'] = url_for('group', id=encoded_id)
+            rval.append(item)
         return rval
 
     def create(self, trans: ProvidesAppContext, payload: Dict[str, Any]):
         """
         Creates a new group.
         """
-        if not trans.user_is_admin:
-            trans.response.status = 403
-            return "You are not authorized to create a new group."
         name = payload.get('name', None)
-        if not name:
-            trans.response.status = 400
-            return "Enter a valid name"
-        if trans.sa_session.query(model.Group).filter(model.Group.table.c.name == name).first():
-            trans.response.status = 400
-            return "A group with that name already exists"
+        if name is None:
+            raise ObjectAttributeMissingException("Missing required name")
+        self._check_duplicated_group_name(trans, name)
 
         group = model.Group(name=name)
         trans.sa_session.add(group)
         user_ids = payload.get('user_ids', [])
-        users = [trans.sa_session.query(model.User).get(trans.security.decode_id(i)) for i in user_ids]
+        users = [trans.sa_session.query(model.User).get(self._decode_id(i)) for i in user_ids]
         role_ids = payload.get('role_ids', [])
-        roles = [trans.sa_session.query(model.Role).get(trans.security.decode_id(i)) for i in role_ids]
+        roles = [trans.sa_session.query(model.Role).get(self._decode_id(i)) for i in role_ids]
         trans.app.security_agent.set_entity_group_associations(groups=[group], roles=roles, users=users)
-        """
-        # Create the UserGroupAssociations
-        for user in users:
-            trans.app.security_agent.associate_user_group( user, group )
-        # Create the GroupRoleAssociations
-        for role in roles:
-            trans.app.security_agent.associate_group_role( group, role )
-        """
         trans.sa_session.flush()
+
         encoded_id = trans.security.encode_id(group.id)
         item = group.to_dict(view='element', value_mapper={'id': trans.security.encode_id})
         item['url'] = url_for('group', id=encoded_id)
         return [item]
 
-    def show(self, trans: ProvidesAppContext, id: EncodedDatabaseIdField):
+    def show(self, trans: ProvidesAppContext, encoded_id: EncodedDatabaseIdField):
         """
         Displays information about a group.
         """
-        group_id = id
-        try:
-            decoded_group_id = trans.security.decode_id(group_id)
-        except TypeError:
-            trans.response.status = 400
-            return "Malformed group id ( %s ) specified, unable to decode." % str(group_id)
-        try:
-            group = trans.sa_session.query(model.Group).get(decoded_group_id)
-        except Exception:
-            group = None
-        if not group:
-            trans.response.status = 400
-            return "Invalid group id ( %s ) specified." % str(group_id)
+        group = self._get_group(trans, encoded_id)
         item = group.to_dict(view='element', value_mapper={'id': trans.security.encode_id})
-        item['url'] = url_for('group', id=group_id)
-        item['users_url'] = url_for('group_users', group_id=group_id)
-        item['roles_url'] = url_for('group_roles', group_id=group_id)
+        item['url'] = url_for('group', id=encoded_id)
+        item['users_url'] = url_for('group_users', group_id=encoded_id)
+        item['roles_url'] = url_for('group_roles', group_id=encoded_id)
         return item
 
-    def update(self, trans: ProvidesAppContext, id: EncodedDatabaseIdField, payload: Dict[str, Any]):
+    def update(self, trans: ProvidesAppContext, encoded_id: EncodedDatabaseIdField, payload: Dict[str, Any]):
         """
         Modifies a group.
         """
-        group_id = id
-        try:
-            decoded_group_id = trans.security.decode_id(group_id)
-        except TypeError:
-            trans.response.status = 400
-            return "Malformed group id ( %s ) specified, unable to decode." % str(group_id)
-        try:
-            group = trans.sa_session.query(model.Group).get(decoded_group_id)
-        except Exception:
-            group = None
-        if not group:
-            trans.response.status = 400
-            return "Invalid group id ( %s ) specified." % str(group_id)
+        group = self._get_group(trans, encoded_id)
         name = payload.get('name', None)
         if name:
             group.name = name
             trans.sa_session.add(group)
         user_ids = payload.get('user_ids', [])
-        users = [trans.sa_session.query(model.User).get(trans.security.decode_id(i)) for i in user_ids]
+        users = [trans.sa_session.query(model.User).get(self._decode_id(i)) for i in user_ids]
         role_ids = payload.get('role_ids', [])
-        roles = [trans.sa_session.query(model.Role).get(trans.security.decode_id(i)) for i in role_ids]
+        roles = [trans.sa_session.query(model.Role).get(self._decode_id(i)) for i in role_ids]
         trans.app.security_agent.set_entity_group_associations(groups=[group], roles=roles, users=users, delete_existing_assocs=False)
         trans.sa_session.flush()
+
+    def _decode_id(self, encoded_id: EncodedDatabaseIdField) -> int:
+        return decode_id(self._app, encoded_id)
+
+    def _check_duplicated_group_name(self, trans: ProvidesAppContext, group_name: str) -> None:
+        if trans.sa_session.query(model.Group).filter(model.Group.table.c.name == group_name).first():
+            raise Conflict(f"A group with name '{group_name}' already exists")
+
+    def _get_group(self, trans: ProvidesAppContext, encoded_id: EncodedDatabaseIdField) -> model.Group:
+        decoded_group_id = self._decode_id(encoded_id)
+        group = trans.sa_session.query(model.Group).get(decoded_group_id)
+        if group is None:
+            raise ObjectNotFound(f"Group with id {encoded_id} was not found.")
+        return group
diff --git a/lib/galaxy/webapps/galaxy/api/groups.py b/lib/galaxy/webapps/galaxy/api/groups.py
index 43386c22b93..9433dab6ff6 100644
--- a/lib/galaxy/webapps/galaxy/api/groups.py
+++ b/lib/galaxy/webapps/galaxy/api/groups.py
@@ -7,6 +7,7 @@ from typing import (
     Dict,
 )
 
+from galaxy.app import StructuredApp
 from galaxy.managers.context import ProvidesAppContext
 from galaxy.managers.groups import GroupsManager
 from galaxy.schema.fields import EncodedDatabaseIdField
@@ -21,9 +22,9 @@ log = logging.getLogger(__name__)
 
 class GroupAPIController(BaseAPIController):
 
-    def __init__(self, app):
+    def __init__(self, app: StructuredApp):
         super().__init__(app)
-        self.manager = GroupsManager()
+        self.manager = GroupsManager(app)
 
     @expose_api
     @require_admin

From d38ba53b212f8d24d76e1d9523e5eb9dc3e90a62 Mon Sep 17 00:00:00 2001
From: davelopez <46503462+davelopez@users.noreply.github.com>
Date: Thu, 18 Feb 2021 14:45:31 +0100
Subject: [PATCH 6/7] Fix groups update endpoint

Prevent possible name duplication and add error handling.
Fix tests to assert 409 when the operation may result in a name conflict.
---
 lib/galaxy/managers/groups.py      |  1 +
 lib/galaxy_test/api/test_groups.py | 13 +++++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/lib/galaxy/managers/groups.py b/lib/galaxy/managers/groups.py
index 1c85418b2a0..0e43b91336b 100644
--- a/lib/galaxy/managers/groups.py
+++ b/lib/galaxy/managers/groups.py
@@ -77,6 +77,7 @@ class GroupsManager:
         group = self._get_group(trans, encoded_id)
         name = payload.get('name', None)
         if name:
+            self._check_duplicated_group_name(trans, name)
             group.name = name
             trans.sa_session.add(group)
         user_ids = payload.get('user_ids', [])
diff --git a/lib/galaxy_test/api/test_groups.py b/lib/galaxy_test/api/test_groups.py
index c2c508ac48f..66606be3dc6 100644
--- a/lib/galaxy_test/api/test_groups.py
+++ b/lib/galaxy_test/api/test_groups.py
@@ -28,13 +28,13 @@ class GroupsApiTestCase(ApiTestCase):
         response = self._post("groups", payload, admin=True, json=True)
         self._assert_status_code_is(response, 400)
 
-    def test_create_duplicated_name_raises_400(self):
+    def test_create_duplicated_name_raises_409(self):
         payload = self._build_valid_group_payload()
         response = self._post("groups", payload, admin=True, json=True)
         self._assert_status_code_is(response, 200)
 
         response = self._post("groups", payload, admin=True, json=True)
-        self._assert_status_code_is(response, 400)
+        self._assert_status_code_is(response, 409)
 
     def test_index(self):
         self.test_create_valid()
@@ -65,6 +65,11 @@ class GroupsApiTestCase(ApiTestCase):
         response = self._get(f"groups/{group_id}")
         self._assert_status_code_is(response, 403)
 
+    def test_show_unknown_raises_400(self):
+        group_id = "invalid-group-id"
+        response = self._get(f"groups/{group_id}", admin=True)
+        self._assert_status_code_is(response, 400)
+
     def test_update(self):
         group = self.test_create_valid(group_name="group-test")
 
@@ -82,7 +87,7 @@ class GroupsApiTestCase(ApiTestCase):
         response = self._put(f"groups/{group_id}")
         self._assert_status_code_is(response, 403)
 
-    def test_update_duplicating_name_raises_400(self):
+    def test_update_duplicating_name_raises_409(self):
         group_a = self.test_create_valid()
         group_b = self.test_create_valid()
 
@@ -93,7 +98,7 @@ class GroupsApiTestCase(ApiTestCase):
             "name": updated_name,
         })
         update_response = self._put(f"groups/{group_b_id}", data=update_payload, admin=True)
-        self._assert_error_code_is(update_response, 400)
+        self._assert_status_code_is(update_response, 409)
 
     def _assert_valid_group(self, group, assert_id=None):
         self._assert_has_keys(group, "id", "name", "model_class", "url")

From b0fcf86a0fd3578f1b6da2c245453ffaed508db3 Mon Sep 17 00:00:00 2001
From: davelopez <46503462+davelopez@users.noreply.github.com>
Date: Thu, 18 Feb 2021 16:02:27 +0100
Subject: [PATCH 7/7] Decode all ids and fetch them all in one query

As @mvdbeek suggested
---
 lib/galaxy/managers/groups.py | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/lib/galaxy/managers/groups.py b/lib/galaxy/managers/groups.py
index 0e43b91336b..8162bf3ebfe 100644
--- a/lib/galaxy/managers/groups.py
+++ b/lib/galaxy/managers/groups.py
@@ -1,6 +1,7 @@
 from typing import (
     Any,
     Dict,
+    List,
 )
 
 from sqlalchemy import false
@@ -47,10 +48,10 @@ class GroupsManager:
 
         group = model.Group(name=name)
         trans.sa_session.add(group)
-        user_ids = payload.get('user_ids', [])
-        users = [trans.sa_session.query(model.User).get(self._decode_id(i)) for i in user_ids]
-        role_ids = payload.get('role_ids', [])
-        roles = [trans.sa_session.query(model.Role).get(self._decode_id(i)) for i in role_ids]
+        encoded_user_ids = payload.get('user_ids', [])
+        users = self._get_users_by_encoded_ids(trans, encoded_user_ids)
+        encoded_role_ids = payload.get('role_ids', [])
+        roles = self._get_roles_by_encoded_ids(trans, encoded_role_ids)
         trans.app.security_agent.set_entity_group_associations(groups=[group], roles=roles, users=users)
         trans.sa_session.flush()
 
@@ -80,16 +81,19 @@ class GroupsManager:
             self._check_duplicated_group_name(trans, name)
             group.name = name
             trans.sa_session.add(group)
-        user_ids = payload.get('user_ids', [])
-        users = [trans.sa_session.query(model.User).get(self._decode_id(i)) for i in user_ids]
-        role_ids = payload.get('role_ids', [])
-        roles = [trans.sa_session.query(model.Role).get(self._decode_id(i)) for i in role_ids]
+        encoded_user_ids = payload.get('user_ids', [])
+        users = self._get_users_by_encoded_ids(trans, encoded_user_ids)
+        encoded_role_ids = payload.get('role_ids', [])
+        roles = self._get_roles_by_encoded_ids(trans, encoded_role_ids)
         trans.app.security_agent.set_entity_group_associations(groups=[group], roles=roles, users=users, delete_existing_assocs=False)
         trans.sa_session.flush()
 
     def _decode_id(self, encoded_id: EncodedDatabaseIdField) -> int:
         return decode_id(self._app, encoded_id)
 
+    def _decode_ids(self, encoded_ids: List[EncodedDatabaseIdField]) -> List[int]:
+        return [self._decode_id(encoded_id) for encoded_id in encoded_ids]
+
     def _check_duplicated_group_name(self, trans: ProvidesAppContext, group_name: str) -> None:
         if trans.sa_session.query(model.Group).filter(model.Group.table.c.name == group_name).first():
             raise Conflict(f"A group with name '{group_name}' already exists")
@@ -100,3 +104,13 @@ class GroupsManager:
         if group is None:
             raise ObjectNotFound(f"Group with id {encoded_id} was not found.")
         return group
+
+    def _get_users_by_encoded_ids(self, trans: ProvidesAppContext, encoded_user_ids: List[EncodedDatabaseIdField]) -> List[model.User]:
+        decoded_user_ids = self._decode_ids(encoded_user_ids)
+        users = trans.sa_session.query(model.User).filter(model.User.table.c.id.in_(decoded_user_ids)).all()
+        return users
+
+    def _get_roles_by_encoded_ids(self, trans: ProvidesAppContext, encoded_role_ids: List[EncodedDatabaseIdField]) -> List[model.Role]:
+        decoded_role_ids = self._decode_ids(encoded_role_ids)
+        roles = trans.sa_session.query(model.Role).filter(model.Role.table.c.id.in_(decoded_role_ids)).all()
+        return roles