fix: create and implement attachment filename generator #725
Conversation
|
See the way I added an FETC-filename link in the detailed attachments review view (#752) for how I see the filename generator being used |
|
So this is working as it should now (I think?) Nothing too crazy. The way to get to the proper attachments and related objects is a bit annoying in the generator function ... But I've discussed this with Ty, and this is the best we can do. The generated filename gets displayed in the proper place. I also ran into one more bug, while testing, which I have now created a fix for here ... I have no idea how we did not run into this earlier, but Study.research_settings_contains_schools, after filling in the StudyDesignForm, causes a major breaking bug. I can also push the to the bugs branch if you'd prefer. It should make use of get_sessions/intervention/observation checks |
attachments/models.py
Outdated
| default="", | ||
| help_text=_( | ||
| "Geef je bestand een omschrijvende naam, het liefst " "maar enkele woorden." | ||
| ), |
There was a problem hiding this comment.
Is there a reason for removing this? Or is this just a merge effect. I think it should stay
attachments/models.py
Outdated
| @@ -21,6 +19,7 @@ class Attachment(models.Model, renderable): | |||
| upload = CDHFileField( | |||
| verbose_name=_("Bestand"), | |||
| help_text=_("Selecteer hier het bestand om toe te voegen."), | |||
| filename_generator=attachment_filename_generator, | |||
There was a problem hiding this comment.
Sorry, I didn't give you enough guidance on this. The filename generator should be plugged into the attachment download view. When the download view without original_fn is visited, it should provide the file as a download with the generated filename.
We don't want to change the filenames in the field or on disk. Those filenames being (pseudo-)random is a good thing because they avoid collisions and provide a minor amount of security!
Also, because attachments can belong to multiple proposals it doesn't make sense to put much of that info in the on-disk filename. This avoids the issue where documents from copied proposals have the wrong refnum in the filename. Generating the filename based on the proposal argument passed to the download view is better.
attachments/utils.py
Outdated
| def attachment_filename_generator(file): | ||
| #get the correct attachment | ||
| try: | ||
| attachment = ProposalAttachment.objects.get(upload=file.file_instance) |
There was a problem hiding this comment.
You can just fetch the base Attachment object and then call attachment.get_correct_submodel()
attachments/utils.py
Outdated
| lastname = proposal.created_by.last_name | ||
| refnum = proposal.reference_number | ||
| kind = attachment.kind | ||
| extension = mimetypes.guess_extension(file.file_instance.content_type) |
There was a problem hiding this comment.
I think this is a cool solution but it's safer to just strip the extension off of upload.original_filename. Docx files in particular can have wild mimetypes that are shared with many other formats.
Sometimes they're recognized as docx, sometimes as just binary, and sometimes as zipfiles. Because docx is basically just a zip file with XML in it.
attachments/utils.py
Outdated
| #get the correct attachment | ||
| try: | ||
| attachment = ProposalAttachment.objects.get(upload=file.file_instance) | ||
| proposal = attachment.attached_to.last() |
There was a problem hiding this comment.
This function can just take a proposal argument to avoid the inconsistency with this.
…_filename_generator
|
Closed to allow for reviewing by @EdoStorm96 the man himself |
So here is a little filename generator. It took me a bit long to figure out, but with some tips from Ty, I got it to work. It did not quite make sense to use the old FilenameFactory, as the filename generator of cdh FileField, expects something different.
So this function just receives the FileWrapper ... From there I search for the relevant ProposalAttachment or StudyAttachment and get the relevant info from there. This is a bit quick and dirty ... But it works for now.
Although there is some overlapping code between this and the FilenameFactory ... they are just different enough, that I thought let's just have them coexsist ... I guess that can be a helper function that removes the overlap, but this does not seem like a huge win to me, both for efficiency, or readability.
I have not though through all the possibilities for attachments resulting in the same filename ... I think I will first start implementing the slots and checkers a bit more, to get a grasp of all the possibilities for things like this to occur ... As a starting point, this is ok imo.