Allow modifying pep images with same name #1556
Conversation
Currently, 'peps.converters.add_pep_image' does not allow to add an image that already exists in the database for the same name. As a consequence images cannot be updated. This commit modifies 'peps.converters.add_pep_image' to also update the image db record if the corresponding file has changed as per 'filecmp.cmp'. This requires removing the file prior to the db update, because the django 'FileField' does not override existing files. Alternatives to the ad hoc removal of the file in 'add_pep_image' include passing a custom 'OverwriteStorage' to the used 'ImageField', see e.g.: https://code.djangoproject.com/ticket/4339 https://stackoverflow.com/questions/9522759/imagefield-overwrite-image-file-with-same-name Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
|
I just skimmed through the peps git history to see how other image updates were propagated in the past. It generally doesn't seem to have happened often. One case that I found is |
|
@lukpueh I'm not aware of any special thing done for that PEP. |
|
There was at least one point where we accidentally broke PEP image publication in general, so I was wondering if it could be that the earlier versions of the image didn't make it into the website DB. However, I don't think the image commit history backs that up. |
Test a recent change in `add_pep_image` that allows modifying images. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
3c870e9 to
bc91a39
Compare
There was a problem hiding this comment.
I just pushed a test for my patch. While writing it I found another little issue (see comment inline), which suggests to use my alternative solution (override on upload). What are others' thoughts?
It would be nice to fix this. Pep 458 still references an outdated file, although the repo has the correct one.
Thanks
|
|
||
| # We need to remove a prior uploaded file to fix an inconsistency | ||
| # between db and filesystem. | ||
| # TODO: This should be dealt with in page.model or converters module |
There was a problem hiding this comment.
Here's some additional info for this line;
A prior test that calls add_pep_image might have already created the 'pep-3001-1.png' file, without persisting the db object. Now a new call to add_pep_image will check for objects, whose name ends with 'pep-3001-1.png', and, since it does not find any, create it anew along with the file. But since Django does not allow to override files it duplicates it using a random infix, e.g 'pep-3001-1_0u7tlLY.png', which is also used as a name on the object.
Now due to the endswith-check and with the random infix, each subsequent attempt to add 'pep-3001-1.png' with add_pep_image, will also duplicate it the object and file.
Maybe we should just override images on upload as suggested as alternative fix in the PR description?
|
This can be closed: after implementing PEP 676, PEP pages are now deployed directly from https://github.com/python/peps to https://peps.python.org/. It does however highlight there's some old PEP-handling code in the repo which is no longer needed and can be removed. |
|
@lukpueh I don't have permission to close things in this repo. Please could you close this PR? |
Fixes #824
Currently,
peps.converters.add_pep_imagedoes not allow to add an image that already exists in the database for the same name. As a consequence images cannot be updated.This PR modifies
peps.converters.add_pep_imageto also update the image db record if the corresponding file has changed as perfilecmp.cmp'This requires removing the file prior to the db update, because the djangoFileFielddoes not override existing files.Alternatives to the ad hoc removal of the file in
add_pep_imageinclude passing a customOverwriteStorageto the usedImageField, see e.g.:https://code.djangoproject.com/ticket/4339
https://stackoverflow.com/questions/9522759/imagefield-overwrite-image-file-with-same-name