Opened 4 years ago

Closed 3 years ago

#16655 closed Bug (fixed)

Silent error in ModelAdmin.list_display callables...

Reported by: jedie Owned by: nobody
Component: contrib.admin Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Errors in ModelAdmin.list_display callables would be ignored.

I suspect not only a AttributeError.

Example:

class FooAdmin(admin.ModelAdmin):
    list_display = ('foobar',)

    def foobar(self, obj):
      raise AttributeError # ignored!

Attachments (1)

silent_error_in_list_display_callables1.diff (1.1 KB) - added by jedie 4 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 4 years ago by jedie

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Is probably only AttributeError, catch here: https://code.djangoproject.com/browser/django/trunk/django/contrib/admin/templatetags/admin_list.py#L168

Seems that one unittests is wrong, isn't it? I correct this in the patch.

comment:2 follow-up: Changed 4 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Design decision needed

This code was introduced as part of r11965 (read-only fields in the admin). It catches AttributeError explicitly, and it certainly does so for a reason (or at least it did at the time it was written). Unfortunately, after some investigation, I can't figure out that reason.

Note that if someone has a callable in list_display that sometimes raises AttributeError, currently this is silently ignored, but with your patch it will crash the admin. It is possible to break backwards compatibility to fix bugs, but we should just be extra careful and make sure your proposal is really the right thing to do. I don't feel we have enough information right now.

Finally, regarding the test that seems wrong to you, could you open a separate ticket?

comment:3 Changed 4 years ago by aaugustin

  • Needs tests set
  • Triage Stage changed from Design decision needed to Accepted

I reached Alex on IRC, he doesn't have a strong opinion on this ticket, besides the fact that it "needs more comprehensive tests for starters".

We've done a decent effort and found no reason to catch AttributeError. Let's consider it a bug.

comment:4 in reply to: ↑ 2 Changed 4 years ago by jedie

Replying to aaugustin:

Finally, regarding the test that seems wrong to you, could you open a separate ticket?

IMHO the two things are related. Both should be changed together, isn't it?

comment:5 Changed 4 years ago by frank.harper@…

I just started learning Django using version 1.3.1, and got bitten by this bug.

I think the fix really should be a high priority because the tutorial (very well done BTW), starts out with models and Admin mode. By definition beginners will be making a lot of mistakes that can cause AttributeError, and because of this bug they will have no feedback whatsoever about what is wrong. This makes for a very bad learning experience.

comment:6 Changed 3 years ago by pjdelport

I found this ticket after independently running into and submitting a fix for this bug: see #18593.

The patch is almost the same, but includes a test, and fixes callable_year() to be more in line with model_year() and modeladmin_year().

comment:7 Changed 3 years ago by aaugustin

  • Needs tests unset

Now there's a pull request with tests (from #18593): https://github.com/django/django/pull/193

comment:8 Changed 3 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in [617d077f].

Note: See TracTickets for help on using tickets.
Back to Top