Add test for update group endpoint and switch patch/put semantics

Author: ml-evs

pydatalab/src/pydatalab/routes/v0_1/admin.py

@@ -342,10 +342,22 @@ def create_group():
         "description": request_json.get("description"),
         "managers": request_json.get("managers"),
     }
+
+    if group_json["managers"]:
+        group_json["managers"] = [ObjectId(u) for u in request_json["managers"]]
+
     try:
         group = Group(**group_json)
     except Exception as e:
-        return jsonify({"status": "error", "message": f"Invalid group data: {str(e)}"}), 400
+        raise BadRequest(f"Invalid new group data: {str(e)}")
+
+    bad_managers = [
+        m for m in group_json["managers"] if not flask_mongo.db.users.find_one({"_id": m})
+    ]
+    if bad_managers:
+        raise BadRequest(
+            f"Manager(s) with ID(s) {bad_managers} not found; cannot create group {group_json['group_id']}"
+        )
 
     try:
         group_immutable_id = flask_mongo.db.groups.insert_one(group.dict()).inserted_id
@@ -360,22 +372,19 @@ def create_group():
     return jsonify({"status": "error", "message": "Unable to create group."}), 400
 
 
-@ADMIN.route("/groups", methods=["DELETE"])
-def delete_group():
-    request_json = request.get_json()
-
-    group_id = request_json.get("immutable_id")
-    if group_id is not None:
-        result = flask_mongo.db.groups.delete_one({"_id": ObjectId(group_id)})
+@ADMIN.route("/groups/<group_immutable_id>", methods=["DELETE"])
+def delete_group(group_immutable_id: str):
+    if group_immutable_id is not None:
+        result = flask_mongo.db.groups.delete_one({"_id": ObjectId(group_immutable_id)})
 
         if result.deleted_count == 1:
             return jsonify({"status": "success"}), 200
 
     return jsonify({"status": "error", "message": "Unable to delete group."}), 400
 
 
-@ADMIN.route("/groups/<group_immutable_id>", methods=["PUT"])
-def update_group(group_immutable_id):
+@ADMIN.route("/groups/<group_immutable_id>", methods=["PATCH"])
+def update_group(group_immutable_id: str):
     request_json = request.get_json()
 
     existing_group = flask_mongo.db.groups.find_one({"_id": ObjectId(group_immutable_id)})
@@ -391,7 +400,14 @@ def update_group(group_immutable_id):
         update_data["description"] = request_json["description"]
 
     if "managers" in request_json:
-        update_data["managers"] = request_json["managers"]
+        update_data["managers"] = [ObjectId(u) for u in request_json["managers"]]
+        bad_managers = [
+            m for m in update_data["managers"] if not flask_mongo.db.users.find_one({"_id": m})
+        ]
+        if bad_managers:
+            raise BadRequest(
+                f"Manager(s) with ID(s) {bad_managers} not found; cannot update group {group_immutable_id}"
+            )
 
     try:
         temp_group_data = {**existing_group, **update_data}
@@ -417,8 +433,8 @@ def update_group(group_immutable_id):
         return jsonify({"status": "error", "message": f"Failed to update group: {str(e)}"}), 500
 
 
-@ADMIN.route("/groups/<group_immutable_id>", methods=["PATCH"])
-def add_user_to_group(group_immutable_id):
+@ADMIN.route("/groups/<group_immutable_id>", methods=["PUT"])
+def add_user_to_group(group_immutable_id: str):
     request_json = request.get_json()
 
     user_id = request_json.get("user_id")

pydatalab/tests/server/test_users.py

@@ -5,7 +5,7 @@ def test_get_current_user_unauthenticated(unauthenticated_client):
     assert resp.status_code == 401
 
 
-def test_get_current_user(client, real_mongo_client):
+def test_get_current_user(client, database):
     """Test that the API key for the demo user has been set correctly."""
 
     resp = client.get("/get-current-user/")
@@ -26,21 +26,21 @@ def test_get_current_user_admin(admin_client):
     assert resp_json["role"] == "admin"
 
 
-def test_role(admin_client, real_mongo_client, user_id):
+def test_role(admin_client, database, user_id):
     endpoint = f"/roles/{str(user_id)}"
     admin_request = {"role": "manager"}
     resp = admin_client.patch(endpoint, json=admin_request)
     assert resp.status_code == 200
-    user = real_mongo_client.get_database().roles.find_one({"_id": user_id})
+    user = database.roles.find_one({"_id": user_id})
     assert user["role"] == "manager"
 
 
-def test_role_update_by_user(client, real_mongo_client, user_id):
+def test_role_update_by_user(client, database, user_id):
     endpoint = f"/roles/{str(user_id)}"
     user_request = {"role": "admin"}
     resp = client.patch(endpoint, json=user_request)
     assert resp.status_code == 403
-    user = real_mongo_client.get_database().roles.find_one({"_id": user_id})
+    user = database.roles.find_one({"_id": user_id})
     assert user["role"] == "manager"
 
 
@@ -58,20 +58,20 @@ def test_list_groups(admin_client, client):
     assert resp.status_code == 403
 
 
-def test_user_update(client, unauthenticated_client, real_mongo_client, user_id, admin_user_id):
+def test_user_update(client, unauthenticated_client, database, user_id, admin_user_id):
     endpoint = f"/users/{str(user_id)}"
     # Test display name update
     user_request = {"display_name": "Test Person II"}
     resp = client.patch(endpoint, json=user_request)
     assert resp.status_code == 200
-    user = real_mongo_client.get_database().users.find_one({"_id": user_id})
+    user = database.users.find_one({"_id": user_id})
     assert user["display_name"] == "Test Person II"
 
     # Test contact email update
     user_request = {"contact_email": "test2@example.org"}
     resp = client.patch(endpoint, json=user_request)
     assert resp.status_code == 200
-    user = real_mongo_client.get_database().users.find_one({"_id": user_id})
+    user = database.users.find_one({"_id": user_id})
     assert user["contact_email"] == "test2@example.org"
     assert user["identities"][-1]["identifier"] == "test2@example.org"
     assert not user["identities"][-1]["verified"]
@@ -80,48 +80,48 @@ def test_user_update(client, unauthenticated_client, real_mongo_client, user_id,
     user_request = {"display_name": None}
     resp = client.patch(endpoint, json=user_request)
     assert resp.status_code == 200
-    user = real_mongo_client.get_database().users.find_one({"_id": user_id})
+    user = database.users.find_one({"_id": user_id})
     assert user["display_name"] == "Test Person II"
 
     # Test that contact_email -> None or empty DOES remove email
     user_request = {"contact_email": None}
     resp = client.patch(endpoint, json=user_request)
     assert resp.status_code == 200
-    user = real_mongo_client.get_database().users.find_one({"_id": user_id})
+    user = database.users.find_one({"_id": user_id})
     assert user["contact_email"] is None
 
     # Check empty string does the same, but reset email first
     user_request = {"contact_email": "test2@example.org"}
     resp = client.patch(endpoint, json=user_request)
     assert resp.status_code == 200
-    user = real_mongo_client.get_database().users.find_one({"_id": user_id})
+    user = database.users.find_one({"_id": user_id})
     assert user["contact_email"] == "test2@example.org"
     user_request = {"contact_email": ""}
     resp = client.patch(endpoint, json=user_request)
     assert resp.status_code == 200
-    user = real_mongo_client.get_database().users.find_one({"_id": user_id})
+    user = database.users.find_one({"_id": user_id})
     assert user["contact_email"] is None
 
     # Test bad display name does not update
     user_request = {"display_name": " "}
     resp = client.patch(endpoint, json=user_request)
     assert resp.status_code == 400
-    user = real_mongo_client.get_database().users.find_one({"_id": user_id})
+    user = database.users.find_one({"_id": user_id})
     assert user["display_name"] == "Test Person II"
 
     # Test bad contact email does not update
     user_request = {"contact_email": "not_an_email"}
     resp = client.patch(endpoint, json=user_request)
     assert resp.status_code == 400
-    user = real_mongo_client.get_database().users.find_one({"_id": user_id})
+    user = database.users.find_one({"_id": user_id})
     assert user["contact_email"] is None
 
     # Test that user cannot update admin account
     endpoint = f"/users/{str(admin_user_id)}"
     user_request = {"display_name": "Test Person"}
     resp = client.patch(endpoint, json=user_request)
     assert resp.status_code == 403
-    user = real_mongo_client.get_database().users.find_one({"_id": admin_user_id})
+    user = database.users.find_one({"_id": admin_user_id})
     assert user["display_name"] == "Test Admin"
 
     # Test that differing user auth can/cannot search for users
@@ -140,18 +140,18 @@ def test_user_update(client, unauthenticated_client, real_mongo_client, user_id,
     assert resp.status_code == 401
 
 
-def test_user_update_admin(admin_client, real_mongo_client, user_id):
+def test_user_update_admin(admin_client, database, user_id):
     endpoint = f"/users/{str(user_id)}"
     # Test admin override of display name
     user_request = {"display_name": "Test Person"}
     resp = admin_client.patch(endpoint, json=user_request)
     assert resp.status_code == 200
-    user = real_mongo_client.get_database().users.find_one({"_id": user_id})
+    user = database.users.find_one({"_id": user_id})
     assert user["display_name"] == "Test Person"
 
 
-def test_create_group(
-    admin_client, client, unauthenticated_client, real_mongo_client, another_user_id
+def test_groups(
+    admin_client, client, unauthenticated_client, database, admin_id, user_id, another_user_id
 ):
     from bson import ObjectId
 
@@ -172,7 +172,7 @@ def test_create_group(
     resp = admin_client.put("/groups", json=good_group)
     assert resp.status_code == 200
     group_immutable_id = ObjectId(resp.json["group_immutable_id"])
-    assert real_mongo_client.get_database().groups.find_one({"_id": group_immutable_id})
+    assert database.groups.find_one({"_id": group_immutable_id})
 
     # Group ID must be unique
     resp = admin_client.put("/groups", json=good_group)
@@ -181,41 +181,43 @@ def test_create_group(
     # Request must come from admin
     # Make ID unique so that this would otherwise pass
     good_group["group_id"] = "my-new-group-2"
-    resp = unauthenticated_client.put("/groups", json=good_group)
+    resp = unauthenticated_client.patch("/groups", json=good_group)
     assert resp.status_code == 401
-    assert (
-        real_mongo_client.get_database().groups.find_one({"group_id": good_group["group_id"]})
-        is None
-    )
+    assert database.groups.find_one({"group_id": good_group["group_id"]}) is None
 
     # Request must come from admin
     resp = client.put("/groups", json=good_group)
     assert resp.status_code == 403
-    assert (
-        real_mongo_client.get_database().groups.find_one({"group_id": good_group["group_id"]})
-        is None
-    )
+    assert database.groups.find_one({"group_id": good_group["group_id"]}) is None
 
     # Check a user can search groups
     resp = client.get("/search/groups?query=New")
     assert resp.status_code == 200
     assert len(resp.json["data"]) == 1
 
     # Check that a user can be added to the group by an admin
-    resp = admin_client.patch(f"/groups/{group_immutable_id}", json={"user_id": another_user_id})
+    resp = admin_client.put(f"/groups/{group_immutable_id}", json={"user_id": another_user_id})
     assert resp.status_code == 200
 
-    user_groups = real_mongo_client.get_database().users.find_one(
-        {"_id": ObjectId(another_user_id)}
-    )["groups"]
+    user_groups = database.users.find_one({"_id": ObjectId(another_user_id)})["groups"]
     assert user_groups[1]["immutable_id"] == group_immutable_id
     assert len(user_groups) == 2
 
     # Check that repeated addition is idempotent
-    resp = admin_client.patch(f"/groups/{group_immutable_id}", json={"user_id": another_user_id})
+    resp = admin_client.put(f"/groups/{group_immutable_id}", json={"user_id": another_user_id})
     assert resp.status_code == 304
 
-    user_groups = real_mongo_client.get_database().users.find_one(
-        {"_id": ObjectId(another_user_id)}
-    )["groups"]
+    user_groups = database.users.find_one({"_id": ObjectId(another_user_id)})["groups"]
     assert len(user_groups) == 2
+
+    # Test that an admin can update a group's details/managers
+    new_details = {
+        "display_name": "My Newly Named Group",
+        "group_id": "my-new-group-renamed",
+        "description": "A group for testing the group update mechanism",
+        "managers": [admin_id, user_id, another_user_id],
+    }
+
+    resp = admin_client.patch(f"/groups/{group_immutable_id}", json=new_details)
+    assert resp.status_code == 200
+    assert database.groups.find_one({"group_id": new_details["group_id"]}) is not None