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