Feature/rationalise file name #1127
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
gecBurton
force-pushed
the
feature/rationalise-file-name
branch
2 times, most recently
from
aa666ee
to
efae1ac
Compare
gecBurton
commented
Oct 28, 2024
| ) | ||
|
|
||
| def update_status_from_core(self, status_label): |
gecBurton
commented
Oct 28, 2024
| except ValueError as e: | ||
| logger.exception("attempt to access non-existent file %s", self.pk, exc_info=e) | ||
| def file_name(self) -> str: | ||
| if self.original_file_name: # delete me? |
There was a problem hiding this comment.
for backwards compatability
gecBurton
commented
Oct 28, 2024
| original_file = models.FileField(storage=settings.STORAGES["default"]["BACKEND"]) | ||
| original_file = models.FileField( | ||
| storage=settings.STORAGES["default"]["BACKEND"], | ||
| upload_to=build_s3_key, |
There was a problem hiding this comment.
this upload_to is the key change
gecBurton
commented
Oct 28, 2024
| @@ -64,29 +64,42 @@ def test_document_upload_status(client, alice, file_pdf_path: Path, s3_client): | |||
|
|
|||
| @pytest.mark.django_db() | |||
| def test_upload_view_duplicate_files(alice, bob, client, file_pdf_path: Path, s3_client): | |||
| # delete all alice's files | |||
There was a problem hiding this comment.
this test has been changed significantly to capture the new intended behaviour
| @@ -307,10 +307,10 @@ async def handle_citations(self, citations: list[AICitation]): | |||
| for s in c.sources: | |||
| try: | |||
| file = await File.objects.aget(original_file=s.source) | |||
| payload = {"url": str(file.url), "original_file_name": file.original_file_name} | |||
There was a problem hiding this comment.
Don't we still want to use original_file_name here so we see 'stuff.pdf' in the frontend rather than 'me@cabinetoffice/stuff.pdf'?
There was a problem hiding this comment.
file_name will strip out me@cabinetoffice/ and just return stuff.pdf
TBH i was hoping that we could just delete original_file_name altogether in 30 days (this PR means that its no longer getting populated)
jamesrichards4
approved these changes
Oct 29, 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
The file-name is especially significant and problematic as it has to be both human readable, and globally unique.
Currently we handle this via:
original_file_namefield to store the name of the file as the user uploaded itunique_name,nameI propose that we dont actually need a file name to be globally unique, we want it to be unique per user, so that:
to achieve this we should change the file name to user.email/file_name, this will have to convenient side effect of presenting the files in directories named after the user within s3.
Changes proposed in this pull request
orginal_file_name-> now unused but kept for backwards compatibilityname->file_name, the original file name - for the benefit of the userGuidance to review
do we actually want this behaviour?
Relevant links
Things to check