Repository: omnivector-solutions/jobbergate (language: python)

Issue:
[2/3] Applications endpoint
This PR is a continuation of the https://github.com/omnivector-solutions/jobbergate-api-fastapi/pull/3
It covers for the DELETE and GET in the /applications endpoint.

The next will cover the PUT.


Discussion / hints:
👍 

Fix patch:
diff --git a/jobbergateapi2/apps/applications/models.py b/jobbergateapi2/apps/applications/models.py
index 55693b6b..49035b86 100644
--- a/jobbergateapi2/apps/applications/models.py
+++ b/jobbergateapi2/apps/applications/models.py
@@ -19,5 +19,5 @@
     Column("application_file", String, nullable=False),
     Column("application_config", String, nullable=False),
     Column("created_at", DateTime, nullable=False, default=func.now()),
-    Column("updated_at", DateTime, nullable=False, default=func.now(), onupdate=datetime.now),
+    Column("updated_at", DateTime, nullable=False, default=func.now(), onupdate=datetime.utcnow),
 )
diff --git a/jobbergateapi2/apps/applications/routers.py b/jobbergateapi2/apps/applications/routers.py
index f065a4bf..155229b3 100644
--- a/jobbergateapi2/apps/applications/routers.py
+++ b/jobbergateapi2/apps/applications/routers.py
@@ -1,6 +1,7 @@
 """
-Router for the Application resource
+Router for the Application resource.
 """
+from datetime import datetime
 from typing import List, Optional
 
 import boto3
@@ -18,7 +19,7 @@
 router = APIRouter()
 
 
-@router.post("/applications/", description="Endpoint for application creation")
+@router.post("/applications/", status_code=201, description="Endpoint for application creation")
 async def applications_create(
     application_name: str = Form(...),
     application_description: str = Form(""),
@@ -43,13 +44,7 @@ async def applications_create(
     async with database.transaction():
         try:
             query = applications_table.insert()
-            values = {
-                "application_name": application_name,
-                "application_description": application_description,
-                "application_owner_id": current_user.id,
-                "application_file": application_file,
-                "application_config": application_config,
-            }
+            values = application.dict()
             application_created_id = await database.execute(query=query, values=values)
             application.id = application_created_id
 
@@ -148,5 +143,71 @@ async def applications_get_by_id(
     return application
 
 
+@router.put(
+    "/applications/{application_id}",
+    status_code=201,
+    description="Endpoint to update an application given the id",
+    response_model=Application,
+)
+async def application_update(
+    application_id: int = Query(...),
+    application_name: Optional[str] = Form(None),
+    application_description: Optional[str] = Form(None),
+    application_config: Optional[str] = Form(None),
+    application_file: Optional[str] = Form(None),
+    current_user: User = Depends(get_current_user),
+    upload_file: Optional[UploadFile] = File(None),
+):
+    """
+    Update an application given it's id.
+    """
+    s3_client = boto3.client("s3")
+
+    query = applications_table.select().where(
+        (applications_table.c.application_owner_id == current_user.id)
+        & (applications_table.c.id == application_id)
+    )
+    raw_application = await database.fetch_one(query)
+    if not raw_application:
+        raise HTTPException(
+            status_code=status.HTTP_404_NOT_FOUND,
+            detail=f"Application {application_id=} not found for the user with id={current_user.id}",
+        )
+
+    update_dict = {}
+    if application_name:
+        update_dict["application_name"] = application_name
+    if application_description:
+        update_dict["application_description"] = application_description
+    if application_file:
+        update_dict["application_file"] = application_file
+    if application_config:
+        update_dict["application_config"] = application_config
+    update_dict["updated_at"] = datetime.utcnow()
+
+    q_update = (
+        applications_table.update().where(applications_table.c.id == application_id).values(update_dict)
+    )
+    async with database.transaction():
+        try:
+            await database.execute(q_update)
+
+        except INTEGRITY_CHECK_EXCEPTIONS as e:
+            raise HTTPException(status_code=422, detail=str(e))
+    application_location = (
+        f"{settings.S3_BASE_PATH}/TEST/applications/{application_id}/jobbergate.tar.gz"
+        # f"{S3_BASE_PATH}/{application.owner_id}/applications/{application.id}/jobbergate.tar.gz"
+    )
+    if upload_file:
+        s3_client.put_object(
+            Body=upload_file.file,
+            Bucket=S3_BUCKET,
+            Key=application_location,
+        )
+    query = applications_table.select().where(applications_table.c.id == application_id)
+    application = Application.parse_obj(await database.fetch_one(query))
+    return application
+
+
 def include_router(app):
     app.include_router(router)


Test patch:
diff --git a/jobbergateapi2/tests/apps/applications/test_routers.py b/jobbergateapi2/tests/apps/applications/test_routers.py
index d73911e3..37a962ab 100644
--- a/jobbergateapi2/tests/apps/applications/test_routers.py
+++ b/jobbergateapi2/tests/apps/applications/test_routers.py
@@ -1,5 +1,5 @@
 """
-Tests for the /applications endpoint
+Tests for the /applications endpoint.
 """
 from io import StringIO
 from unittest import mock
@@ -26,7 +26,11 @@
 @database.transaction(force_rollback=True)
 async def test_create(boto3_client_mock, application_data, client, user_data):
     """
-    Test creating a application
+    Test POST /applications/ correctly creates an application.
+
+    This test proves that an application is successfully created via a POST request to the /applciations/
+    endpoint. We show this by asserting that the application is created in the database after the post
+    request is made, the correct status code (201) is returned and the correct boto3 method was called.
     """
     s3_client_mock = mock.Mock()
     boto3_client_mock.client.return_value = s3_client_mock
@@ -36,7 +40,7 @@ async def test_create(boto3_client_mock, application_data, client, user_data):
     await insert_objects(user, users_table)
 
     response = client.post("/applications/", data=application_data, files={"upload_file": file_mock})
-    assert response.status_code == status.HTTP_200_OK
+    assert response.status_code == status.HTTP_201_CREATED
     s3_client_mock.put_object.assert_called_once()
 
     count = await database.fetch_all("SELECT COUNT(*) FROM applications")
@@ -48,7 +52,12 @@ async def test_create(boto3_client_mock, application_data, client, user_data):
 @database.transaction(force_rollback=True)
 async def test_create_without_application_name(boto3_client_mock, application_data, client, user_data):
     """
-    Don't create application when required value is missing
+    Test that is not possible to create an application without the required parameters.
+
+    This test proves that is not possible to create an application without the name. We show this by
+    trying to create an application without the application_name in the request then assert that the
+    application still does not exists in the database, the correct status code (422) is returned and that the
+    boto3 method is never called.
     """
     s3_client_mock = mock.Mock()
     boto3_client_mock.client.return_value = s3_client_mock
@@ -71,7 +80,12 @@ async def test_create_without_application_name(boto3_client_mock, application_da
 @database.transaction(force_rollback=True)
 async def test_create_without_file(boto3_client_mock, application_data, client, user_data):
     """
-    Don't create application without file
+    Test that is not possible to create an application without a file.
+
+    This test proves that is not possible to create an application without a file. We show this by
+    trying to create an application without a file in the request then assert that the application still
+    does not exists in the database, the correct status code (422) is returned and that the boto3 method
+    is never called.
     """
     s3_client_mock = mock.Mock()
     boto3_client_mock.client.return_value = s3_client_mock
@@ -93,7 +107,12 @@ async def test_create_without_file(boto3_client_mock, application_data, client,
 @database.transaction(force_rollback=True)
 async def test_delete_application(boto3_client_mock, client, user_data, application_data):
     """
-    Test delete an application
+    Test DELETE /applications/<id> correctly deletes an application.
+
+    This test proves that an application is successfully deleted via a DELETE request to the
+    /applciations/<id> endpoint. We show this by asserting that the application
+    no longer exists in the database after the delete request is made, the correct status code
+    is returned and the correct boto3 method was called.
     """
     s3_client_mock = mock.Mock()
     boto3_client_mock.client.return_value = s3_client_mock
@@ -117,7 +136,11 @@ async def test_delete_application(boto3_client_mock, client, user_data, applicat
 @database.transaction(force_rollback=True)
 async def test_delete_application_not_from_user(boto3_client_mock, client, user_data, application_data):
     """
-    Do nothing if current user id is not the owner of the application
+    Test that a userX cannot delete an application owned by userY.
+
+    This test proves that userX cannot delete an application owned by userY. We show this by
+    asserting that the application owned by userY still exists after userX tries to delete it, that
+    the correct response code (404) is returned and that the boto3 method is never called.
     """
     s3_client_mock = mock.Mock()
     boto3_client_mock.client.return_value = s3_client_mock
@@ -133,7 +156,7 @@ async def test_delete_application_not_from_user(boto3_client_mock, client, user_
     assert response.status_code == status.HTTP_404_NOT_FOUND
     count = await database.fetch_all("SELECT COUNT(*) FROM applications")
     assert count[0][0] == 1
-    s3_client_mock.put_object.assert_not_called()
+    s3_client_mock.delete_object.assert_not_called()
 
 
 @pytest.mark.asyncio
@@ -141,7 +164,11 @@ async def test_delete_application_not_from_user(boto3_client_mock, client, user_
 @database.transaction(force_rollback=True)
 async def test_delete_application_not_found(boto3_client_mock, client, user_data, application_data):
     """
-    Do nothing if application id does not exists
+    Test DELETE /applications/<id> the correct response code when the application doesn't exist.
+
+    This test proves that DELETE /applications/<id> returns the correct response code (404)
+    when the application id does not exist in the database. We show this by asserting that a 404 response
+    code is returned for a request made with an application id that doesn't exist.
     """
     s3_client_mock = mock.Mock()
     boto3_client_mock.client.return_value = s3_client_mock
@@ -157,7 +184,7 @@ async def test_delete_application_not_found(boto3_client_mock, client, user_data
     assert response.status_code == status.HTTP_404_NOT_FOUND
     count = await database.fetch_all("SELECT COUNT(*) FROM applications")
     assert count[0][0] == 1
-    s3_client_mock.put_object.assert_not_called()
+    s3_client_mock.delete_object.assert_not_called()
 
 
 @pytest.mark.asyncio
@@ -165,7 +192,11 @@ async def test_delete_application_not_found(boto3_client_mock, client, user_data
 @database.transaction(force_rollback=True)
 async def test_delete_application_without_id(boto3_client_mock, client, user_data, application_data):
     """
-    Don't accept DELETE in /applications/ without id
+    Test DELETE /applications/ without <id> returns the correct response.
+
+    This test proves that DELETE /applications returns the correct response code (405)
+    when an application id is not specified. We show this by asserting that a 405 response
+    code is returned for a request made without an application id.
     """
     s3_client_mock = mock.Mock()
     boto3_client_mock.client.return_value = s3_client_mock
@@ -174,14 +205,19 @@ async def test_delete_application_without_id(boto3_client_mock, client, user_dat
 
     response = client.delete("/applications/")
     assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED
-    s3_client_mock.put_object.assert_not_called()
+    s3_client_mock.delete_object.assert_not_called()
 
 
 @pytest.mark.asyncio
 @database.transaction(force_rollback=True)
 async def test_get_application_by_id(client, user_data, application_data):
     """
-    Return the application with the given id
+    Test GET /applications/<id>.
+
+    This test proves that GET /applications/<id> returns the correct application, owned by
+    the user making the request. We show this by asserting that the application data
+    returned in the response is equal to the application data that exists in the database
+    for the given application id.
     """
     user = [UserCreate(id=1, **user_data)]
     await insert_objects(user, users_table)
@@ -205,7 +241,11 @@ async def test_get_application_by_id(client, user_data, application_data):
 @database.transaction(force_rollback=True)
 async def test_get_application_by_id_invalid(client, user_data, application_data):
     """
-    Return 404 when the application id does not exists
+    Test the correct response code is returned when an application does not exist.
+
+    This test proves that GET /application/<id> returns the correct response code when the
+    requested application does not exist. We show this by asserting that the status code
+    returned is what we would expect given the application requested doesn't exist (404).
     """
     user = [UserCreate(id=1, **user_data)]
     await insert_objects(user, users_table)
@@ -218,7 +258,11 @@ async def test_get_application_by_id_invalid(client, user_data, application_data
 @database.transaction(force_rollback=True)
 async def test_get_application_all_from_user(client, user_data, application_data):
     """
-    Return all applications from the user
+    Test GET /applications returns only applications owned by the user making the request.
+
+    This test proves that GET /applications returns the correct applications for the user making
+    the request. We show this by asserting that the applications returned in the response are
+    only applications owned by the user making the request.
     """
     user = [UserCreate(id=1, **user_data)]
     await insert_objects(user, users_table)
@@ -245,7 +289,12 @@ async def test_get_application_all_from_user(client, user_data, application_data
 @database.transaction(force_rollback=True)
 async def test_get_application_all_from_user_empty(client, user_data, application_data):
     """
-    Return all applications from the user, even when the user don't have any
+    Test applications list doesn't include applications owned by other users.
+
+    This test proves that the user making the request cannot see applications owned by other users.
+    We show this by creating applications that are owned by another user id and assert that
+    the user making the request to list applications doesn't see any of the other user's
+    applications in the response, len(resp.json()) == 0.
     """
     user = [UserCreate(id=1, **user_data)]
     await insert_objects(user, users_table)
@@ -268,7 +317,12 @@ async def test_get_application_all_from_user_empty(client, user_data, applicatio
 @database.transaction(force_rollback=True)
 async def test_get_all_applications(client, user_data, application_data):
     """
-    If all=True returns all applications
+    Test that listing applications, when all=True, contains applications owned by other users.
+
+    This test proves that the user making the request can see applications owned by other users.
+    We show this by creating three applications, two that are owned by the user making the request, and one
+    anther owned by another user. Assert that the response to GET /applications?all=True includes all three
+    applications.
     """
     user = [UserCreate(id=1, **user_data)]
     await insert_objects(user, users_table)
@@ -290,3 +344,71 @@ async def test_get_all_applications(client, user_data, application_data):
     assert data[0]["id"] == 1
     assert data[1]["id"] == 2
     assert data[2]["id"] == 3
+
+
+@pytest.mark.asyncio
+@mock.patch("jobbergateapi2.apps.applications.routers.boto3")
+@database.transaction(force_rollback=True)
+async def test_update_application(boto3_client_mock, client, user_data, application_data):
+    """
+    Test that an application is updated via PUT.
+
+    This test proves that an application's values are correctly updated following a PUT request to the
+    /application/<id> endpoint. We show this by asserting that the values provided to update the
+    application are returned in the response made to the PUT /applciation/<id> endpoint.
+    """
+    s3_client_mock = mock.Mock()
+    boto3_client_mock.client.return_value = s3_client_mock
+    file_mock = mock.MagicMock(wraps=StringIO("test"))
+    user = [UserCreate(id=1, **user_data)]
+    await insert_objects(user, users_table)
+
+    applications = [
+        Application(
+            id=1, application_owner_id=1, application_description="old description", **application_data
+        ),
+    ]
+    await insert_objects(applications, applications_table)
+    count = await database.fetch_all("SELECT COUNT(*) FROM applications")
+    assert count[0][0] == 1
+
+    application_data["application_name"] = "new_name"
+    application_data["application_description"] = "new_description"
+    response = client.put("/applications/1", data=application_data, files={"upload_file": file_mock})
+    assert response.status_code == status.HTTP_201_CREATED
+
+    data = response.json()
+    assert data["application_name"] == application_data["application_name"]
+    assert data["application_description"] == application_data["application_description"]
+
+    s3_client_mock.put_object.assert_called_once()
+
+
+@pytest.mark.asyncio
+@mock.patch("jobbergateapi2.apps.applications.routers.boto3")
+@database.transaction(force_rollback=True)
+async def test_update_application_wrong_user(boto3_client_mock, client, user_data, application_data):
+    """
+    Test that a userX cannot update an application owned by userY.
+
+    This test proves that userX cannot update an application owned by userY. We show this by
+    asserting that the application id doesn't exist for the user making the request and the
+    expected response is returned.
+    """
+    s3_client_mock = mock.Mock()
+    boto3_client_mock.client.return_value = s3_client_mock
+    file_mock = mock.MagicMock(wraps=StringIO("test"))
+    user = [UserCreate(id=1, **user_data)]
+    await insert_objects(user, users_table)
+
+    applications = [
+        Application(id=1, application_owner_id=2, **application_data),
+    ]
+    await insert_objects(applications, applications_table)
+    count = await database.fetch_all("SELECT COUNT(*) FROM applications")
+    assert count[0][0] == 1
+
+    response = client.put("/applications/1", data=application_data, files={"upload_file": file_mock})
+    assert response.status_code == status.HTTP_404_NOT_FOUND
+
+    s3_client_mock.put_object.assert_not_called()