Address PR Review Comments #38

object_storage_api/routers/image.py

@@ -83,7 +83,7 @@ def get_image(
 @router.delete(
     path="/{image_id}",
     summary="Delete an image by ID",
-    response_description="Image deleted Sucessfully",
+    response_description="Image deleted sucessfully",
     status_code=status.HTTP_204_NO_CONTENT,
 )
 def delete_image(

object_storage_api/services/image.py

@@ -104,5 +104,6 @@ def delete(self, image_id: str) -> None:
         :param image_id: The ID of the image to delete.
         """
         stored_image = self._image_repository.get(image_id)
+        # Deletes image from object store first to prevent unreferenced objects in storage
         self._image_store.delete(stored_image.object_key)
         self._image_repository.delete(image_id)

object_storage_api/stores/attachment.py

@@ -31,7 +31,10 @@ def create_presigned_post(
         """
         object_key = f"attachments/{attachment.entity_id}/{attachment_id}"
 
-        logger.info("Generating a presigned URL for uploading the attachment")
+        logger.info(
+            "Generating a presigned URL for uploading the attachment with object key: %s to the object store",
+            object_key,
+        )
         presigned_post_response = s3_client.generate_presigned_post(
             Bucket=object_storage_config.bucket_name.get_secret_value(),
             Key=object_key,

object_storage_api/stores/image.py

@@ -29,7 +29,7 @@ def upload(self, image_id: str, image_metadata: ImagePostMetadataSchema, upload_
         """
         object_key = f"images/{image_metadata.entity_id}/{image_id}"
 
-        logger.info("Uploading image file to the object storage")
+        logger.info("Uploading image file with object key: %s to the object store", object_key)
         s3_client.upload_fileobj(
             upload_file.file,
             Bucket=object_storage_config.bucket_name.get_secret_value(),
@@ -46,7 +46,7 @@ def create_presigned_get(self, image: ImageOut) -> str:
         :param image: `ImageOut` model of the image.
         :return: Presigned url to get the image.
         """
-        logger.info("Generating presigned url to get image from object storage")
+        logger.info("Generating presigned url to get image with object key: %s from the object store", image.object_key)
         response = s3_client.generate_presigned_url(
             "get_object",
             Params={
@@ -66,7 +66,7 @@ def delete(self, object_key: str) -> None:
         :param object_key: Key of the image to delete.
         """
 
-        logger.info("Deleting image file from the object storage")
+        logger.info("Deleting image file with object key: %s from the object store", object_key)
         s3_client.delete_object(
             Bucket=object_storage_config.bucket_name.get_secret_value(),
             Key=object_key,

test/unit/services/test_image.py

@@ -253,27 +253,39 @@ class DeleteDSL(ImageServiceDSL):
     """Base class for `delete` tests."""
 
     _delete_image_id: str
+    _expected_image_out: ImageOut
+    _delete_image_id: str
+    _delete_image_object_key: str
 
-    def call_delete(self, image_id: str) -> None:
+    def mock_delete(self, image_in_data: dict) -> None:
         """
-        Calls the `ImageService` `delete` method.
+        Mocks repo methods appropriately to test the `delete` service method.
 
-        :param image_id: ID of the image to be deleted.
+        :param image_in_data: Dictionary containing the image data as would be required for an `ImageIn`
+                                   database model (i.e. no created and modified times required).
         """
+        self._expected_image_out = ImageOut(**ImageIn(**image_in_data).model_dump())
+        self.mock_image_repository.get.return_value = self._expected_image_out
+        self._delete_image_id = self._expected_image_out.id
+        self._delete_image_object_key = self._expected_image_out.object_key
+
+    def call_delete(self) -> None:
+        """Calls the `ImageService` `delete` method."""
 
-        self._delete_image_id = image_id
-        self.image_service.delete(image_id)
+        self.image_service.delete(self._delete_image_id)
 
     def check_delete_success(self) -> None:
         """Checks that a prior call to `call_delete` worked as expected."""
 
         self.mock_image_repository.delete.assert_called_once_with(self._delete_image_id)
+        self.mock_image_store.delete.assert_called_once_with(self._delete_image_object_key)
 
 
 class TestDelete(DeleteDSL):
     """Tests for deleting an image."""
 
     def test_delete(self):
         """Test for deleting an image."""
-        self.call_delete("test_image_id")
+        self.mock_delete(IMAGE_IN_DATA_ALL_VALUES)
+        self.call_delete()
         self.check_delete_success()

test/unit/stores/test_image.py

@@ -88,27 +88,29 @@ def test_upload(self):
 class DeleteDSL(ImageStoreDSL):
     """Base class for `delete` tests."""
 
-    def call_delete(self) -> None:
-        """Calls the `ImageStore` `delete` method."""
+    _delete_object_key: str
 
-        self.image_store.delete("object_key")
+    def call_delete(self, object_key: str) -> None:
+        """Calls the `ImageStore` `delete` method."""
+        self._delete_object_key = object_key
+        self.image_store.delete(object_key)
 
     def check_delete_success(self) -> None:
         """Checks that a prior call to `call_delete` worked as expected."""
 
         self.mock_s3_client.delete_object.assert_called_once_with(
             Bucket=object_storage_config.bucket_name.get_secret_value(),
-            Key="object_key",
+            Key=self._delete_object_key,
         )
 
 
 class TestDelete(DeleteDSL):
     """Tests for deleting an image from object storage."""
 
-    def test_create_presigned_post(self):
+    def test_delete(self):
         """Test for deleting an image from object storage."""
 
-        self.call_delete()
+        self.call_delete("object-key")
         self.check_delete_success()