Implement endpoint for deleting attachments by entity ID #105
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #105 +/- ##
===========================================
- Coverage 99.57% 99.37% -0.20%
===========================================
Files 19 19
Lines 468 483 +15
===========================================
+ Hits 466 480 +14
- Misses 2 3 +1 ☔ View full report in Codecov by Sentry. |
|
|
||
| :param entity_id: The entity ID of the attachments to delete. | ||
| :param session: PyMongo ClientSession to use for database operations. | ||
| :return: |
|
|
||
|
|
||
| class DeleteByEntityIdDSL(ListDSL): | ||
| """Base class for delete tests.""" |
There was a problem hiding this comment.
| """Base class for delete tests.""" | |
| """Base class for delete_by_entity_id tests.""" |
| def test_delete_attachments_by_entity_id(self): | ||
| """Test deleting attachments.""" | ||
| attachments = self.post_test_attachments() | ||
| entity_id = attachments[0]["entity_id"] | ||
|
|
||
| self.delete_attachments_by_entity_id(entity_id) | ||
| self.check_delete_attachments_by_entity_id_success() | ||
|
|
||
| self.get_attachments(filters={"entity_id": entity_id}) | ||
| self.check_get_attachments_success([]) | ||
|
|
||
| def test_delete_attachments_by_entity_id_with_non_existent_id(self): | ||
| """Test deleting attachments with a non-existent entity ID.""" | ||
| self.delete_attachments_by_entity_id(str(ObjectId())) | ||
| self.check_delete_attachments_by_entity_id_success() | ||
|
|
||
| def test_delete_attachments_by_entity_id_with_invalid_id(self): |
There was a problem hiding this comment.
is attachments needed in each function name? could they just be e.g. test_delete_by_entity_id
| for the code that relies on the `deleted_count` value to work. | ||
|
|
||
| :param collection_mock: Mocked MongoDB database collection instance. | ||
| :param deleted_count: The value to be assigned to the `deleted_count` attribute of the `DeleteResult` object |
There was a problem hiding this comment.
| :param deleted_count: The value to be assigned to the `deleted_count` attribute of the `DeleteResult` object | |
| :param deleted_count: The value to be assigned to the `deleted_count` attribute of the `DeleteResult` object. |
| batch = object_keys[i : i + batch_size] | ||
| s3_client.delete_objects( | ||
| Bucket=object_storage_config.bucket_name.get_secret_value(), | ||
| Delete={"Objects": [{"Key": key for key in batch}]}, |
There was a problem hiding this comment.
| Delete={"Objects": [{"Key": key for key in batch}]}, | |
| Delete={"Objects": [{"Key": key } for key in batch]}, |
The current implementation works, but I'm not sure why it does?
The docs for the function requires the parameter to be structured as so:
If I modify the code to compare the 2 outputs of both options this is the result.
Again both implementation work, but I don't understand why yours works?
| try: | ||
| attachment_service.delete_by_entity_id(entity_id) | ||
| except InvalidObjectIdError: | ||
| # As this endpoint takes in a query parameter to delete multiple attachments, and to hide the database | ||
| # behaviour, we treat any invalid entity_id the same as a valid one that has no attachments associated to it. | ||
| pass |
There was a problem hiding this comment.
Whilst it is fine to catch the exception in the router layer, I think it would be better if it was in the repo layer to match what we are doing in the rest of the repo.
Description
This PR implements a
DELETEendpoint at/attachmentsthat accepts anentity_idquery parameter and deletes all attachments with thatentity_id.Testing instructions
entity_id204ifentity_idis invalid or not associated with any attachmentsAgile board tracking
closes #76