makinacorpus / django-safedelete

Mask your objects instead of deleting them from your database.
https://django-safedelete.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
677 stars 122 forks source link

Setting highlight_deleted_field.short_description mistakenly affects all subclasses of SafeDeleteAdmin #218

Open sjdemartini opened 2 years ago

sjdemartini commented 2 years ago

Thanks for the great package! I noticed one issue using SafeDeleteAdmin and the highlight_deleted_field option, after following the example in the docs:

class ContactAdmin(SafeDeleteAdmin):
   list_display = (highlight_deleted, "highlight_deleted_field", "first_name", "last_name", "email") + SafeDeleteAdmin.list_display
   list_filter = ("last_name", SafeDeleteAdminFilter,) + SafeDeleteAdmin.list_filter

   field_to_highlight = "id"

ContactAdmin.highlight_deleted_field.short_description = ContactAdmin.field_to_highlight

This introduces a bug in that it will override the SafeDeleteAdmin's highlight_deleted_field.short_description, so all subclasses will end up with the same value, since it's an attribute on the superclass's highlight_deleted_field method. Even if multiple subclasses attempt to set the attribute, like ContactAdmin.highlight_deleted_field.short_description = ContactAdmin.field_to_highlight and OtherAdmin.highlight_deleted_field.short_description = "Foo", the one defined last will override all others.


One workaround right now is to redefine the highlight_deleted_field in every subclass, so that you can attach an attribute that doesn't clobber other usage of SafeDeleteAdmin, like:

class ContactAdmin(SafeDeleteAdmin):
    list_display = (highlight_deleted, "highlight_deleted_field", "first_name", "last_name", "email") + SafeDeleteAdmin.list_display
    list_filter = ("last_name", SafeDeleteAdminFilter,) + SafeDeleteAdmin.list_filter

    field_to_highlight = "id"

    def highlight_deleted_field(self, *args, **kwargs):
        return super().highlight_deleted_field(*args, **kwargs)

    highlight_deleted_field.short_description = "ID"

but this feels pretty messy. Is it possible to change SafeDeleteAdmin such that it references some other field on the class like:

class ContactAdmin(SafeDeleteAdmin):
    field_to_highlight = "id"
    field_to_highlight_short_description = "ID"  # <-- new field here

rather than needing to add an attribute to the method?

Gagaro commented 2 years ago

Indeed modifying the base class function is not a good idea.

I don't think we have a choice other than overwriting the function on the child to have a different function.

We could have a metaclass creating that function automatically if we have a field_to_highlight_short_description attribute, but this would mean creating our own metaclass, which is not really a good idea either.

In any case, the documentation should be updated.