From d73a82052b35de3c7d8d295986bacf3616962899 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 11 Jun 2024 17:35:13 +0200 Subject: [PATCH 01/15] Increase API test coverage for group update The second update does not provide a group_name as we are not changing it. This will fail with the current code as the update payload model is the same as the create one. --- lib/galaxy_test/api/test_groups.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/galaxy_test/api/test_groups.py b/lib/galaxy_test/api/test_groups.py index e644392b6151..e6fe63772231 100644 --- a/lib/galaxy_test/api/test_groups.py +++ b/lib/galaxy_test/api/test_groups.py @@ -76,10 +76,15 @@ def test_update(self): group_id = group["id"] updated_name = f"group-test-updated-{self.dataset_populator.get_random_name()}" - update_payload = { - "name": updated_name, - } - update_response = self._put(f"groups/{group_id}", data=update_payload, admin=True, json=True) + update_response = self._put(f"groups/{group_id}", data={"name": updated_name}, admin=True, json=True) + self._assert_status_code_is_ok(update_response) + + # Update with another user + another_user_id = None + with self._different_user(): + another_user_id = self.dataset_populator.user_id() + assert another_user_id is not None + update_response = self._put(f"groups/{group_id}", data={"user_ids": [another_user_id]}, admin=True, json=True) self._assert_status_code_is_ok(update_response) def test_update_only_admin(self): From 0b504cf543a6e8b4c6c553a28671a5e0905a5d49 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 11 Jun 2024 17:36:46 +0200 Subject: [PATCH 02/15] Fix update group payload model All fields in the payload should be optional when updating. --- lib/galaxy/managers/groups.py | 11 +++++++---- lib/galaxy/schema/groups.py | 6 ++++++ lib/galaxy/webapps/galaxy/api/groups.py | 3 ++- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/galaxy/managers/groups.py b/lib/galaxy/managers/groups.py index 33de69670cf5..da58a2e18807 100644 --- a/lib/galaxy/managers/groups.py +++ b/lib/galaxy/managers/groups.py @@ -18,7 +18,10 @@ from galaxy.model.base import transaction from galaxy.model.scoped_session import galaxy_scoped_session from galaxy.schema.fields import Security -from galaxy.schema.groups import GroupCreatePayload +from galaxy.schema.groups import ( + GroupCreatePayload, + GroupUpdatePayload, +) from galaxy.structured_app import MinimalManagerApp @@ -77,7 +80,7 @@ def show(self, trans: ProvidesAppContext, group_id: int): item["roles_url"] = self._url_for(trans, "group_roles", group_id=encoded_id) return item - def update(self, trans: ProvidesAppContext, group_id: int, payload: GroupCreatePayload): + def update(self, trans: ProvidesAppContext, group_id: int, payload: GroupUpdatePayload): """ Modifies a group. """ @@ -87,9 +90,9 @@ def update(self, trans: ProvidesAppContext, group_id: int, payload: GroupCreateP self._check_duplicated_group_name(sa_session, name) group.name = name sa_session.add(group) - user_ids = payload.user_ids + user_ids = payload.user_ids or [] users = get_users_by_ids(sa_session, user_ids) - role_ids = payload.role_ids + role_ids = payload.role_ids or [] roles = get_roles_by_ids(sa_session, role_ids) self._app.security_agent.set_entity_group_associations( groups=[group], roles=roles, users=users, delete_existing_assocs=False diff --git a/lib/galaxy/schema/groups.py b/lib/galaxy/schema/groups.py index aebe0796f8cd..1a4bde58f764 100644 --- a/lib/galaxy/schema/groups.py +++ b/lib/galaxy/schema/groups.py @@ -9,6 +9,7 @@ ) from typing_extensions import Literal +from galaxy.schema import partial_model from galaxy.schema.fields import ( DecodedDatabaseIdField, EncodedDatabaseIdField, @@ -69,3 +70,8 @@ class GroupCreatePayload(Model): [], title="role IDs", ) + + +@partial_model() +class GroupUpdatePayload(GroupCreatePayload): + pass diff --git a/lib/galaxy/webapps/galaxy/api/groups.py b/lib/galaxy/webapps/galaxy/api/groups.py index 7525e55fd1c0..f23e2a79f976 100644 --- a/lib/galaxy/webapps/galaxy/api/groups.py +++ b/lib/galaxy/webapps/galaxy/api/groups.py @@ -17,6 +17,7 @@ GroupCreatePayload, GroupListResponse, GroupResponse, + GroupUpdatePayload, ) from galaxy.webapps.galaxy.api import ( depends, @@ -80,8 +81,8 @@ def show( def update( self, group_id: Annotated[DecodedDatabaseIdField, Path(...)], + payload: Annotated[GroupUpdatePayload, Body(...)], trans: ProvidesAppContext = DependsOnTrans, - payload: GroupCreatePayload = Body(...), ) -> GroupResponse: return self.manager.update(trans, group_id, payload) From c69e54a12a44ee47f9bbf3b9538b9ab8b4cf5105 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 11 Jun 2024 17:50:52 +0200 Subject: [PATCH 03/15] Update client API schema --- client/src/api/schema/schema.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/client/src/api/schema/schema.ts b/client/src/api/schema/schema.ts index a68de4e76d45..f592e3f59745 100644 --- a/client/src/api/schema/schema.ts +++ b/client/src/api/schema/schema.ts @@ -5483,6 +5483,15 @@ export interface components { */ url: string; }; + /** GroupUpdatePayload */ + GroupUpdatePayload: { + /** name of the group */ + name?: string | null; + /** role IDs */ + role_ids?: string[] | null; + /** user IDs */ + user_ids?: string[] | null; + }; /** GroupUserListResponse */ GroupUserListResponse: components["schemas"]["GroupUserResponse"][]; /** GroupUserResponse */ @@ -15135,7 +15144,7 @@ export interface operations { }; requestBody: { content: { - "application/json": components["schemas"]["GroupCreatePayload"]; + "application/json": components["schemas"]["GroupUpdatePayload"]; }; }; responses: { From a8cb547519931361e5c9765c27b0b4377970f896 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 13 Jun 2024 16:53:41 +0200 Subject: [PATCH 04/15] Add tests for replacing and removing group users and roles --- lib/galaxy_test/api/test_groups.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/lib/galaxy_test/api/test_groups.py b/lib/galaxy_test/api/test_groups.py index e6fe63772231..7441c3740663 100644 --- a/lib/galaxy_test/api/test_groups.py +++ b/lib/galaxy_test/api/test_groups.py @@ -79,7 +79,7 @@ def test_update(self): update_response = self._put(f"groups/{group_id}", data={"name": updated_name}, admin=True, json=True) self._assert_status_code_is_ok(update_response) - # Update with another user + # Update replace with another user another_user_id = None with self._different_user(): another_user_id = self.dataset_populator.user_id() @@ -87,6 +87,23 @@ def test_update(self): update_response = self._put(f"groups/{group_id}", data={"user_ids": [another_user_id]}, admin=True, json=True) self._assert_status_code_is_ok(update_response) + # get group and check if it was updated + response = self._get(f"groups/{group_id}", admin=True) + self._assert_status_code_is_ok(response) + users = self._get(f"groups/{group_id}/users", admin=True).json() + assert len(users) == 1 + assert users[0]["id"] == another_user_id + + # Remove private role from group + update_response = self._put(f"groups/{group_id}", data={"role_ids": []}, admin=True, json=True) + self._assert_status_code_is_ok(update_response) + + # get group and check if it was updated + response = self._get(f"groups/{group_id}", admin=True) + self._assert_status_code_is_ok(response) + roles = self._get(f"groups/{group_id}/roles", admin=True).json() + assert len(roles) == 0 + def test_update_only_admin(self): group = self.test_create_valid() group_id = group["id"] From 480358c90e00b94b0f521687a5d168b64a6b878a Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 13 Jun 2024 16:54:35 +0200 Subject: [PATCH 05/15] Fix update group API endpoint Allow to replace or remove users and roles from the group --- lib/galaxy/managers/groups.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/managers/groups.py b/lib/galaxy/managers/groups.py index da58a2e18807..69fcabc5612c 100644 --- a/lib/galaxy/managers/groups.py +++ b/lib/galaxy/managers/groups.py @@ -90,13 +90,18 @@ def update(self, trans: ProvidesAppContext, group_id: int, payload: GroupUpdateP self._check_duplicated_group_name(sa_session, name) group.name = name sa_session.add(group) - user_ids = payload.user_ids or [] - users = get_users_by_ids(sa_session, user_ids) - role_ids = payload.role_ids or [] - roles = get_roles_by_ids(sa_session, role_ids) - self._app.security_agent.set_entity_group_associations( - groups=[group], roles=roles, users=users, delete_existing_assocs=False - ) + + users = None + if payload.user_ids is not None: + users = get_users_by_ids(sa_session, payload.user_ids) + + roles = None + if payload.role_ids is not None: + roles = get_roles_by_ids(sa_session, payload.role_ids) + + if payload.user_ids is not None or payload.role_ids is not None: + self._app.security_agent.set_entity_group_associations(groups=[group], roles=roles, users=users) + with transaction(sa_session): sa_session.commit() From 1e2348e5553dbd267cf40a9280c94aa5a52e0ac3 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 13 Jun 2024 17:44:09 +0200 Subject: [PATCH 06/15] Fix typing --- lib/galaxy/managers/roles.py | 2 +- lib/galaxy/managers/users.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/managers/roles.py b/lib/galaxy/managers/roles.py index 89bf69815e2c..b20244fc7fb4 100644 --- a/lib/galaxy/managers/roles.py +++ b/lib/galaxy/managers/roles.py @@ -162,6 +162,6 @@ def undelete(self, trans: ProvidesUserContext, role: model.Role) -> model.Role: return role -def get_roles_by_ids(session: Session, role_ids): +def get_roles_by_ids(session: Session, role_ids: List[int]) -> List[Role]: stmt = select(Role).where(Role.id.in_(role_ids)) return session.scalars(stmt).all() diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py index 210566025061..519bc6dbfab4 100644 --- a/lib/galaxy/managers/users.py +++ b/lib/galaxy/managers/users.py @@ -874,7 +874,7 @@ def _add_parsers(self): self.fn_filter_parsers.update({}) -def get_users_by_ids(session: Session, user_ids): +def get_users_by_ids(session: Session, user_ids: List[int]) -> List[User]: stmt = select(User).where(User.id.in_(user_ids)) return session.scalars(stmt).all() From a04942fa7aafd02ea0f3b7acb0a219037ceea36f Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 13 Jun 2024 18:05:16 +0200 Subject: [PATCH 07/15] Keep old API behavior for now Do not allow to remove or replace, only add users and roles. --- lib/galaxy/managers/groups.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/managers/groups.py b/lib/galaxy/managers/groups.py index 69fcabc5612c..df30b2b39853 100644 --- a/lib/galaxy/managers/groups.py +++ b/lib/galaxy/managers/groups.py @@ -99,8 +99,9 @@ def update(self, trans: ProvidesAppContext, group_id: int, payload: GroupUpdateP if payload.role_ids is not None: roles = get_roles_by_ids(sa_session, payload.role_ids) - if payload.user_ids is not None or payload.role_ids is not None: - self._app.security_agent.set_entity_group_associations(groups=[group], roles=roles, users=users) + self._app.security_agent.set_entity_group_associations( + groups=[group], roles=roles, users=users, delete_existing_assocs=False + ) with transaction(sa_session): sa_session.commit() From fbf903d1e710261a72ea16fb922ee0e44ce9f200 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 13 Jun 2024 18:24:18 +0200 Subject: [PATCH 08/15] Refactor API test for updating a group --- lib/galaxy_test/api/test_groups.py | 72 +++++++++++++++++++++++------- 1 file changed, 56 insertions(+), 16 deletions(-) diff --git a/lib/galaxy_test/api/test_groups.py b/lib/galaxy_test/api/test_groups.py index 7441c3740663..7e0d40943868 100644 --- a/lib/galaxy_test/api/test_groups.py +++ b/lib/galaxy_test/api/test_groups.py @@ -1,4 +1,7 @@ -from typing import Optional +from typing import ( + List, + Optional, +) from galaxy_test.base.populators import DatasetPopulator from ._framework import ApiTestCase @@ -72,37 +75,62 @@ def test_show_unknown_raises_400(self): self._assert_status_code_is(response, 400) def test_update(self): - group = self.test_create_valid(group_name=f"group-test-{self.dataset_populator.get_random_name()}") + user_id = self.dataset_populator.user_id() + user_private_role_id = self.dataset_populator.user_private_role_id() + original_name = f"group-test-{self.dataset_populator.get_random_name()}" + group = self.test_create_valid(group_name=original_name) + + self._assert_group_has_expected_values( + group["id"], + name=original_name, + user_ids=[user_id], + role_ids=[user_private_role_id], + ) group_id = group["id"] updated_name = f"group-test-updated-{self.dataset_populator.get_random_name()}" update_response = self._put(f"groups/{group_id}", data={"name": updated_name}, admin=True, json=True) self._assert_status_code_is_ok(update_response) - # Update replace with another user + # Only the name should be updated + self._assert_group_has_expected_values( + group_id, + name=updated_name, + user_ids=[user_id], + role_ids=[user_private_role_id], + ) + + # Add another user to the group another_user_id = None with self._different_user(): another_user_id = self.dataset_populator.user_id() + another_role_id = self.dataset_populator.user_private_role_id() assert another_user_id is not None update_response = self._put(f"groups/{group_id}", data={"user_ids": [another_user_id]}, admin=True, json=True) self._assert_status_code_is_ok(update_response) - # get group and check if it was updated - response = self._get(f"groups/{group_id}", admin=True) - self._assert_status_code_is_ok(response) - users = self._get(f"groups/{group_id}/users", admin=True).json() - assert len(users) == 1 - assert users[0]["id"] == another_user_id + # Check if the user was added + self._assert_group_has_expected_values( + group_id, + name=updated_name, + user_ids=[user_id, another_user_id], + role_ids=[user_private_role_id], + ) - # Remove private role from group - update_response = self._put(f"groups/{group_id}", data={"role_ids": []}, admin=True, json=True) + # Add another role to the group + update_response = self._put(f"groups/{group_id}", data={"role_ids": [another_role_id]}, admin=True, json=True) self._assert_status_code_is_ok(update_response) - # get group and check if it was updated - response = self._get(f"groups/{group_id}", admin=True) - self._assert_status_code_is_ok(response) - roles = self._get(f"groups/{group_id}/roles", admin=True).json() - assert len(roles) == 0 + # Check if the role was added + self._assert_group_has_expected_values( + group_id, + name=updated_name, + user_ids=[user_id, another_user_id], + role_ids=[user_private_role_id, another_role_id], + ) + + # TODO: Test removing users and roles + # Currently not possible because the API can only add users and roles def test_update_only_admin(self): group = self.test_create_valid() @@ -177,6 +205,18 @@ def _assert_valid_group(self, group, assert_id=None): if assert_id is not None: assert group["id"] == assert_id + def _assert_group_has_expected_values(self, group_id: str, name: str, user_ids: List[str], role_ids: List[str]): + group = self._get(f"groups/{group_id}", admin=True).json() + assert group["name"] == name + users = self._get(f"groups/{group_id}/users", admin=True).json() + assert len(users) == len(user_ids) + for user in users: + assert user["id"] in user_ids + roles = self._get(f"groups/{group_id}/roles", admin=True).json() + assert len(roles) == len(role_ids) + for role in roles: + assert role["id"] in role_ids + def _build_valid_group_payload(self, name: Optional[str] = None): name = name or self.dataset_populator.get_random_name() user_id = self.dataset_populator.user_id() From 84d6971d5e9704631e0592799f679a705e775887 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 13 Jun 2024 19:15:40 +0200 Subject: [PATCH 09/15] Use random user in test Since we cannot remove the user from the group to restore its previous state we need to use a user that will not be part of other tests to avoid unwanted interactions. --- lib/galaxy_test/api/test_groups.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/galaxy_test/api/test_groups.py b/lib/galaxy_test/api/test_groups.py index 7e0d40943868..8e4c5510fe98 100644 --- a/lib/galaxy_test/api/test_groups.py +++ b/lib/galaxy_test/api/test_groups.py @@ -101,8 +101,9 @@ def test_update(self): ) # Add another user to the group + another_user_email = f"user-{self.dataset_populator.get_random_name()}@example.com" another_user_id = None - with self._different_user(): + with self._different_user(another_user_email): another_user_id = self.dataset_populator.user_id() another_role_id = self.dataset_populator.user_private_role_id() assert another_user_id is not None From 03001d4d20ce7c60cc6143b6b58387fe4d283e6d Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 13 Jun 2024 20:13:46 +0200 Subject: [PATCH 10/15] Reset current page when changing folders --- client/src/components/FilesDialog/FilesDialog.vue | 1 + .../components/SelectionDialog/SelectionDialog.vue | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/client/src/components/FilesDialog/FilesDialog.vue b/client/src/components/FilesDialog/FilesDialog.vue index 1fb89ae29130..5e6d2bde46e3 100644 --- a/client/src/components/FilesDialog/FilesDialog.vue +++ b/client/src/components/FilesDialog/FilesDialog.vue @@ -409,6 +409,7 @@ onMounted(() => { :is-busy="isBusy" :items="items" :items-provider="itemsProvider" + :provider-url="currentDirectory?.url" :total-items="totalItems" :modal-show="modalShow" :modal-static="modalStatic" diff --git a/client/src/components/SelectionDialog/SelectionDialog.vue b/client/src/components/SelectionDialog/SelectionDialog.vue index c3958b198003..ea501a759411 100644 --- a/client/src/components/SelectionDialog/SelectionDialog.vue +++ b/client/src/components/SelectionDialog/SelectionDialog.vue @@ -27,6 +27,7 @@ interface Props { isEncoded?: boolean; items?: SelectionItem[]; itemsProvider?: ItemsProvider; + providerUrl?: string; totalItems?: number; leafIcon?: string; modalShow?: boolean; @@ -48,6 +49,7 @@ const props = withDefaults(defineProps(), { isEncoded: false, items: () => [], itemsProvider: undefined, + providerUrl: undefined, totalItems: 0, leafIcon: "fa fa-file-o", modalShow: true, @@ -127,6 +129,16 @@ watch( filtered(props.items); } ); + +watch( + () => props.providerUrl, + () => { + // We need to reset the current page when drilling down sub-folders + if (props.itemsProvider !== undefined) { + currentPage.value = 1; + } + } +);