Cleanup and response to PR feedback

Author: jstone-dev

src/mavedb/lib/permissions.py

@@ -98,10 +98,8 @@ def has_permission(user_data: Optional[UserData], item: Base, action: Action) ->
                 editor_user_ids.add(user_association.user_id)
             elif user_association.contribution_role == ContributionRole.viewer:
                 viewer_user_ids.add(user_association.user_id)
-        user_is_admin = user_is_owner or (user_data is not None and (user_data.user.id in admin_user_ids))
-        user_may_edit = user_is_admin or (
-            user_data is not None and (user_data.user.id in admin_user_ids or user_data.user.id in editor_user_ids)
-        )
+        user_is_admin = user_is_owner or (user_data is not None and user_data.user.id in admin_user_ids)
+        user_may_edit = user_is_admin or (user_data is not None and user_data.user.id in editor_user_ids)
         user_may_view_private = user_may_edit or (user_data is not None and (user_data.user.id in viewer_user_ids))
 
         save_to_logging_context({"resource_is_published": published})
@@ -293,7 +291,6 @@ def has_permission(user_data: Optional[UserData], item: Base, action: Action) ->
             if user_may_view_private or not private:
                 return PermissionResponse(True)
             # Roles which may perform this operation.
-            # TODO should the mapper be able to list all collections? Do we ever intend to pass a collection urn to the mapper?
             elif roles_permitted(active_roles, [UserRole.admin]):
                 return PermissionResponse(True)
             elif private:
@@ -317,8 +314,6 @@ def has_permission(user_data: Optional[UserData], item: Base, action: Action) ->
             else:
                 return PermissionResponse(False, 403, f"insufficient permissions for URN '{item.urn}'")
         elif action == Action.DELETE:
-            # TODO should collection admins be allowed to delete a collection?
-            # TODO who should be allowed to delete an official collection? mavedb admins only?
             # A collection may be deleted even if it has been published, as long as it is not an official collection.
             if user_is_owner:
                 return PermissionResponse(
@@ -334,7 +329,6 @@ def has_permission(user_data: Optional[UserData], item: Base, action: Action) ->
                 return PermissionResponse(False, 404, f"collection with URN '{item.urn}' not found")
             else:
                 return PermissionResponse(False)
-        # TODO should collection admins be allowed to publish a collection? (I think yes)
         elif action == Action.PUBLISH:
             if user_is_admin:
                 return PermissionResponse(True)
@@ -364,7 +358,6 @@ def has_permission(user_data: Optional[UserData], item: Base, action: Action) ->
                     else f"insufficient permissions for URN '{item.urn}'"
                 ),
             )
-        # TODO is add_role an ok name for this action, or should we create a new action called "add_user"?
         elif action == Action.ADD_ROLE:
             # Both collection admins and MaveDB admins can add a user to a collection role
             if user_is_admin or roles_permitted(active_roles, [UserRole.admin]):

src/mavedb/view_models/collection.py

@@ -91,6 +91,5 @@ class Collection(SavedCollection):
 
 # Properties to return to admin clients or non-admin clients who are admins of the returned collection
 # NOTE: Coupled to ContributionRole enum
-# TODO should MaveDB admins get AdminUsers instead of Users?
 class AdminCollection(Collection):
     pass

src/mavedb/view_models/experiment.py

@@ -30,7 +30,7 @@
 
 
 class OfficialCollection(BaseModel):
-    badge_name: Optional[str]
+    badge_name: str
     name: str
     urn: str
 

src/mavedb/view_models/score_set.py

@@ -43,7 +43,7 @@ class ExternalLink(BaseModel):
 
 
 class OfficialCollection(BaseModel):
-    badge_name: Optional[str]
+    badge_name: str
     name: str
     urn: str
 

tests/routers/test_collections.py

@@ -2,8 +2,10 @@
 from copy import deepcopy
 
 import jsonschema
+import pytest
 
 from mavedb.lib.validation.urn_re import MAVEDB_COLLECTION_URN_RE
+from mavedb.models.enums.contribution_role import ContributionRole 
 from mavedb.view_models.collection import Collection
 from tests.helpers.constants import (
     EXTRA_USER,
@@ -48,7 +50,11 @@ def test_create_public_collection(client, setup_router_db):
         assert (key, expected_response[key]) == (key, response_data[key])
 
 
-def test_add_collection_admin(client, setup_router_db):
+@pytest.mark.parametrize(
+     "role",
+     ContributionRole._member_names_
+)
+def test_add_collection_user_to_collection_role(role, client, setup_router_db):
     collection = create_collection(client, {"private": True})
 
     response = client.post(f"/api/v1/collections/{collection['urn']}/admins", json={"orcid_id": EXTRA_USER["username"]})
@@ -79,62 +85,6 @@ def test_add_collection_admin(client, setup_router_db):
         assert (key, expected_response[key]) == (key, response_data[key])
 
 
-def test_add_collection_editor(client, setup_router_db):
-    collection = create_collection(client)
-
-    response = client.post(
-        f"/api/v1/collections/{collection['urn']}/editors", json={"orcid_id": EXTRA_USER["username"]}
-    )
-    assert response.status_code == 200
-    response_data = response.json()
-    expected_response = deepcopy(TEST_COLLECTION_RESPONSE)
-    expected_response.update(
-        {
-            "urn": collection["urn"],
-            "badgeName": None,
-            "description": None,
-            "editors": [
-                {
-                    "firstName": EXTRA_USER["first_name"],
-                    "lastName": EXTRA_USER["last_name"],
-                    "orcidId": EXTRA_USER["username"],
-                }
-            ],
-        }
-    )
-    assert sorted(expected_response.keys()) == sorted(response_data.keys())
-    for key in expected_response:
-        assert (key, expected_response[key]) == (key, response_data[key])
-
-
-def test_add_collection_viewer(client, setup_router_db):
-    collection = create_collection(client)
-
-    response = client.post(
-        f"/api/v1/collections/{collection['urn']}/viewers", json={"orcid_id": EXTRA_USER["username"]}
-    )
-    assert response.status_code == 200
-    response_data = response.json()
-    expected_response = deepcopy(TEST_COLLECTION_RESPONSE)
-    expected_response.update(
-        {
-            "urn": response_data["urn"],
-            "badgeName": None,
-            "description": None,
-            "viewers": [
-                {
-                    "firstName": EXTRA_USER["first_name"],
-                    "lastName": EXTRA_USER["last_name"],
-                    "orcidId": EXTRA_USER["username"],
-                }
-            ],
-        }
-    )
-    assert sorted(expected_response.keys()) == sorted(response_data.keys())
-    for key in expected_response:
-        assert (key, expected_response[key]) == (key, response_data[key])
-
-
 def test_creator_can_read_private_collection(session, client, setup_router_db, anonymous_app_overrides):
     collection = create_collection(client)
 
@@ -239,7 +189,6 @@ def test_unauthorized_user_cannot_read_private_collection(session, client, setup
 
     with DependencyOverrider(extra_user_app_overrides):
         response = client.get(f"/api/v1/collections/{collection['urn']}")
-        # response = client.get(f"/api/v1/users/me")
 
     assert response.status_code == 404
     assert f"collection with URN '{collection['urn']}' not found" in response.json()["detail"]
@@ -250,7 +199,6 @@ def test_anonymous_cannot_read_private_collection(session, client, setup_router_
 
     with DependencyOverrider(anonymous_app_overrides):
         response = client.get(f"/api/v1/collections/{collection['urn']}")
-        # response = client.get(f"/api/v1/users/me")
 
     assert response.status_code == 404
     assert f"collection with URN '{collection['urn']}' not found" in response.json()["detail"]