Fix #229 -- Delete old file after validation has passed #293
Conversation
|
Hi @jeromelebleu, thanks for the contribution. I ran the test without your patch, and it passe 8000 s. However, it should fail :( Maybe you can give it another go? |
|
Hi @codingjoe, so here is what I have done:
As expected, the test I add fails for me: Did you do the same? |
|
@jeromelebleu sorry, I don't get the same error. Would you mind rebasing your commit to the latest main branch? I did start from there and then checked our your test, not the other way around. |
Codecov Report
@@ Coverage Diff @@
## master #293 +/- ##
==========================================
+ Coverage 97.61% 97.67% +0.06%
==========================================
Files 5 5
Lines 293 301 +8
==========================================
+ Hits 286 294 +8
Misses 7 7
Continue to review full report at Codecov.
|
|
I have rebased this branch on master, tell me if it is better… |
|
Would love to see this merged! Will there be a new release on pypi as well, with this fix included? Latest pypi is 5.3.0 whereas on github latest is |
|
@kciredor yes, v6 will receive bug fixes, such as this one :) @jeromelebleu, I'll give it a go. |
Previously files were deleted even if the from validation failed. Co-Authored-By: codingjoe <johannes@maron.family>
|
@jeromelebleu found the problem, I was missing a change in the models, that's why I didn't work for me. Now the tail fails without your patch also the patch does fix it. Furthermore, I did test the fix manually in a project. Works great! |
|
@jeromelebleu maybe you'd like to contribute the file deletion feature to django-pictures as well? It hasn't been implemented it. It's the only feature missing for the beta released. |
|
Thanks @jeromelebleu for the fix and @codingjoe for the pypi release with this fix! When a future Django 5.x comes out, assuming this package would not be compatible anymore, is there a probability that the package will receive a fix release for that as well? Or should it really be considered end of life? I'm asking because out of a dozen thumbnail packages, this is the only one that seems to work properly with external storage and cleanup etc. and still being 'maintained'. For a new project I'd like to use this, but it feels a bit awkward to start a new project with a deprecated package. |
|
@kciredor it's unlikely that we will add Django 5 support. However, django-pictures is developed by the with the same care I put into this package. It just adapted to the (new) reality of responsive web. I am sure you understand that I can only maintain so many packages at once. All that being said, the migration to django-pictures is actually pretty straightforward. I am currently migrating a number of projects, mainly to root out some bugs. Thus far the process has been fairly easy. |
@codingjoe I didn't give a try to django-pictures and investigate how to migrate from django-stdimage yet, and I don't have time for that at the moment unfortunately. It is in my todo list, but feel free to implement the file deletion feature if you want as it will be much faster. Thanks by the way for the release - and the work! |
Note that calling
file.delete(save=False)inpre_save()is not working since - if I understood correctly - it also resets the field on the model object which prevents the new file to be properly saved. Thus, we just delete the file and its variations from the storage.