Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sjdemartini opened this issue Sep 2, 2022 · 1 comment

Comments

@sjdemartini
Copy link

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
Copy link
Member

Gagaro commented Sep 13, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants