diff --git a/src/mavedb/routers/collections.py b/src/mavedb/routers/collections.py index 32268208..f813ce5b 100644 --- a/src/mavedb/routers/collections.py +++ b/src/mavedb/routers/collections.py @@ -10,7 +10,7 @@ from mavedb import deps from mavedb.lib.authentication import UserData, get_current_user -from mavedb.lib.authorization import require_current_user_with_email +from mavedb.lib.authorization import require_current_user, require_current_user_with_email from mavedb.lib.logging import LoggedRoute from mavedb.lib.logging.context import ( format_raised_exception_info_as_dict, @@ -46,7 +46,7 @@ def list_my_collections( *, db: Session = Depends(deps.get_db), - user_data: Optional[UserData] = Depends(get_current_user), + user_data: UserData = Depends(require_current_user), ) -> Dict[str, Sequence[Collection]]: """ List my collections. @@ -685,7 +685,7 @@ async def add_user_to_collection_role( ) # A user can only be in one role per collection, so remove from any other roles elif collection_user_association: - item.users.remove(User) + item.users.remove(user) setattr(user, "role", role) item.users.append(user) @@ -762,7 +762,7 @@ async def remove_user_from_collection_role( assert_permission(user_data, item, Action.ADD_ROLE) # Since this is a post request, user should not already be in this role - if collection_user_association.contribution_role != role: + if collection_user_association is not None and collection_user_association.contribution_role != role: logger.info( msg="Failed to remove user from collection role; the requested user does not currently hold the requested role for this collection.", extra=logging_context(), diff --git a/src/mavedb/routers/permissions.py b/src/mavedb/routers/permissions.py index 4d2d5ae9..c10f49e2 100644 --- a/src/mavedb/routers/permissions.py +++ b/src/mavedb/routers/permissions.py @@ -50,7 +50,7 @@ async def check_permission( """ save_to_logging_context({"requested_resource": urn}) - item: Optional[Union[ExperimentSet, Experiment, ScoreSet]] = None + item: Optional[Union[Collection, ExperimentSet, Experiment, ScoreSet]] = None if model_name == ModelName.experiment_set: item = db.query(ExperimentSet).filter(ExperimentSet.urn == urn).one_or_none() diff --git a/src/mavedb/view_models/collection.py b/src/mavedb/view_models/collection.py index 7180213f..2bb9e328 100644 --- a/src/mavedb/view_models/collection.py +++ b/src/mavedb/view_models/collection.py @@ -33,12 +33,16 @@ class CollectionBase(BaseModel): ) -class CollectionModify(CollectionBase): +class CollectionModify(BaseModel): # all fields should be optional, because the client should specify only the fields they want to update private: Optional[bool] = Field( description="Whether the collection is visible to all MaveDB users. If set during collection update, input ignored unless requesting user is collection admin." ) name: Optional[str] + description: Optional[str] + badge_name: Optional[str] = Field( + description="Badge name. Input ignored unless requesting user has MaveDB admin privileges." + ) class CollectionCreate(CollectionBase): diff --git a/tests/helpers/constants.py b/tests/helpers/constants.py index 547f10f8..180ba9b2 100644 --- a/tests/helpers/constants.py +++ b/tests/helpers/constants.py @@ -193,6 +193,7 @@ # keys to be set after receiving response "urn": None, "experimentSetUrn": None, + "officialCollections": [], } TEST_EXPERIMENT_WITH_KEYWORD_RESPONSE = { @@ -227,6 +228,7 @@ # keys to be set after receiving response "urn": None, "experimentSetUrn": None, + "officialCollections": [], } TEST_EXPERIMENT_WITH_KEYWORD_HAS_DUPLICATE_OTHERS_RESPONSE = { @@ -270,6 +272,7 @@ # keys to be set after receiving response "urn": None, "experimentSetUrn": None, + "officialCollections": [], } TEST_TAXONOMY = { @@ -402,6 +405,7 @@ # keys to be set after receiving response "urn": None, "processingState": ProcessingState.incomplete.name, + "officialCollections": [], } TEST_MINIMAL_ACC_SCORESET = { @@ -473,6 +477,7 @@ # keys to be set after receiving response "urn": None, "processingState": ProcessingState.incomplete.name, + "officialCollections": [], } TEST_CDOT_TRANSCRIPT = { diff --git a/tests/routers/test_permissions.py b/tests/routers/test_permissions.py index 83cddc53..1473a125 100644 --- a/tests/routers/test_permissions.py +++ b/tests/routers/test_permissions.py @@ -65,9 +65,8 @@ def test_cannot_get_permission_with_wrong_action_in_experiment_set(client, setup assert response.status_code == 422 response_data = response.json() assert ( - response_data["detail"][0]["msg"] == "value is not a valid enumeration member; permitted: 'read', " - "'update', 'delete', 'add_experiment', 'add_score_set', 'set_scores'," - " 'add_role', 'publish'" + response_data["detail"][0]["msg"] == "value is not a valid enumeration member; permitted: 'lookup', 'read', " + "'update', 'delete', 'add_experiment', 'add_score_set', 'set_scores', 'add_role', 'publish', 'add_badge'" ) @@ -210,9 +209,8 @@ def test_cannot_get_permission_with_wrong_action_in_experiment(client, setup_rou assert response.status_code == 422 response_data = response.json() assert ( - response_data["detail"][0]["msg"] == "value is not a valid enumeration member; permitted: 'read', " - "'update', 'delete', 'add_experiment', 'add_score_set', 'set_scores'," - " 'add_role', 'publish'" + response_data["detail"][0]["msg"] == "value is not a valid enumeration member; permitted: 'lookup', 'read', " + "'update', 'delete', 'add_experiment', 'add_score_set', 'set_scores', 'add_role', 'publish', 'add_badge'" ) @@ -347,9 +345,8 @@ def test_cannot_get_permission_with_wrong_action_in_score_set(client, setup_rout assert response.status_code == 422 response_data = response.json() assert ( - response_data["detail"][0]["msg"] == "value is not a valid enumeration member; permitted: 'read', " - "'update', 'delete', 'add_experiment', 'add_score_set', 'set_scores'," - " 'add_role', 'publish'" + response_data["detail"][0]["msg"] == "value is not a valid enumeration member; permitted: 'lookup', 'read', " + "'update', 'delete', 'add_experiment', 'add_score_set', 'set_scores', 'add_role', 'publish', 'add_badge'" ) @@ -369,5 +366,5 @@ def test_cannot_get_permission_with_non_existing_item(client, setup_router_db): response_data = response.json() assert ( response_data["detail"][0]["msg"] == "value is not a valid enumeration member; permitted: " - "'experiment', 'experiment-set', 'score-set'" + "'collection', 'experiment', 'experiment-set', 'score-set'" )