call core api delete from django delete #406
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rachaelcodes
commented
May 17, 2024
django_app/tests/test_views.py
Outdated
| @@ -195,7 +238,6 @@ def test_post_message_to_new_session(alice: User, client: Client, requests_mock: | |||
| assert response.status_code == HTTPStatus.FOUND | |||
| assert "Location" in response.headers | |||
| session_id = URL(response.url).parts[-2] | |||
| assert ChatMessage.objects.get(chat_history__id=session_id, role=ChatRoleEnum.user).text == "Are you there?" | |||
There was a problem hiding this comment.
hm not sure why this got deleted - will fix on Monday
gecBurton
reviewed
May 17, 2024
| if request.method == "POST": | ||
| logger.info("Removing document: %s", request.POST["doc_id"]) | ||
| file.delete() | ||
| api = CoreApiClient(host=settings.CORE_API_HOST, port=settings.CORE_API_PORT) |
There was a problem hiding this comment.
this could be declared just once, maybe as a constant?
There was a problem hiding this comment.
Ultimately I'd like to inject it.
rachaelcodes
force-pushed
the
bugfix/REDBOX-256-delete-file-and-chunks
branch
from
f5d9a17 to
d61ce14
Compare
252afh
approved these changes
May 20, 2024
gecBurton
reviewed
May 20, 2024
| @@ -64,6 +69,7 @@ class File(UUIDPrimaryKeyBase, TimeStampedModel): | |||
| user = models.ForeignKey(User, on_delete=models.CASCADE) | |||
| original_file_name = models.TextField(max_length=2048, blank=True, null=True) | |||
| core_file_uuid = models.UUIDField(null=True) | |||
| expiry_date = models.DateField(default=get_default_expiry_date) | |||
There was a problem hiding this comment.
either:
- this needs to be nullable or we will get an error on migration, or
- we could compute this field on the fly from the
created_at: datetime
?
There was a problem hiding this comment.
Hm something went wrong changing branch - that should be from a different ticket.
gecBurton
reviewed
May 20, 2024
| @@ -24,6 +24,7 @@ | |||
| from yarl import URL | |||
|
|
|||
| logger = logging.getLogger(__name__) | |||
| core_api = CoreApiClient(host=settings.CORE_API_HOST, port=settings.CORE_API_PORT) | |||
gecBurton
requested changes
May 20, 2024
rachaelcodes
force-pushed
the
bugfix/REDBOX-256-delete-file-and-chunks
branch
from
5c7a991 to
09b9eab
Compare
gecBurton
approved these changes
May 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Currently the frontend deletion of a file only deletes the file from the Django and S3 storage, but not the Elastic storage.
Changes proposed in this pull request
Deleting a file from the user interface propagates a call to core-api to delete the file and its chunks. If that API request fails, then the file will not delete from the Django database.
If the call to the API fails, the user will see this view:
Guidance to review
Try deleting files from django.
Relevant links
https://technologyprogramme.atlassian.net/browse/REDBOX-256
Things to check
[ ] I have added any new ENV vars in all deployed environments