un1t / django-cleanup

Automatically deletes old file for FileField and ImageField. It also deletes files on models instance deletion.
MIT License
1.11k stars 79 forks source link

Breaks django-admin-sortable2 #34

Closed kennell closed 7 years ago

kennell commented 7 years ago

Hey. Django-cleanup seems to break django-admin-sortable2 (or vice versa?)

When reordering some models in the Admin, the AJAX request sent will fail with HTTP 500. I am not quite sure what is happening, but fallback_pre_save (part of django-cleanup) is called at some point, resulting in the following error:

TypeError at /admin/main/trailers/adminsortable2_update/
fallback_pre_save() missing 2 required positional arguments: 'raw' and 'using'

Not sure what app actually needs its behaviour fixed, maybe @jrief can jump on the issue too.

vinnyrose commented 7 years ago

Not our problem. They are manually sending a django pre_save signal without providing all the expected arguments as documented here: https://docs.djangoproject.com/en/dev/ref/signals/#pre-save

kennell commented 7 years ago

I see, will reopen the issue on their side. Thanks anyway for looking into this.

ppython commented 7 years ago

link of @kennell comment at django-admin-sortable2 issues for reference :link:

jrief commented 7 years ago

@vinnyrose , so according to your statement:

They are manually sending a django pre_save signal without providing all the expected arguments

what's wrong with this invocation?

pre_save.send(self.model, instance=instance, update_fields=[self.default_order_field])
vinnyrose commented 7 years ago

Compare it to django, you are missing raw and using: https://github.com/django/django/blob/d2a26c1a90e837777dabdf3d67ceec4d2a70fb86/django/db/models/base.py#L827

Both raw and using are trivial to add. raw would be False. using would be router.db_for_write(self.model, instance=instance).

This is a django signal, only meant to be sent from django. If you are going to hijack it that is fine, but you have to follow django's implementation.

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.