Opened 9 years ago

Closed 7 years ago

#685 closed defect (fixed)

[patch] list_detail and archive_* generic views should default allow_empty to True

Reported by: django@… Owned by: nobody
Component: Generic views Version: master
Severity: major Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Ideally default behaviour should correlate with the assumptions of the majority. Although it is clearly documented otherwise, I suspect that most people (like me :) will assume that the list_detail generic view handles an empty list exactly as it handles a populated list. I was surprised at getting 404s after reseting my database. Current bahviour has the potential to facilitate unnoticed bugs. I suggest defaulting 'allow_empty' to True. Of course this must be weighed against the hassle of backwards incompatibility and I suspect it may lose out on these grounds, but I thought it was worth ticketing for consideration.

Attachments (2)

allow_empty=True.diff (4.8 KB) - added by Gary Wilson <gary.wilson@…> 8 years ago.
default value of allow_empty changed to True for the generic views (archive_* except archive_week which already defaulted to True and object_list) with the documentation changes as well.
685_allow_empty.diff (2.5 KB) - added by mcroydon 7 years ago.
Per jacob's comments, apply allow_empty to generic index views and update docs. Apply patch with -p1.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 9 years ago by L.Plant.98@…

I agree. I've already come across lots of scenarios where I have got it wrong and could easily get in wrong in the future and not notice. If you've already populated one item or more in the table you don't notice the behaviour with no items. When you later add searching facilities to the list, and someone enters a search with no results, they suddenly get a 404 error. The current behaviour is unintuitive, and is simply a shortcut for a common pattern that some apps might have, but as such it shouldn't be the default.

comment:2 Changed 9 years ago by django@…

  • Component changed from Admin interface to Generic views
  • Owner changed from adrian to jacob
  • priority changed from normal to high
  • Severity changed from normal to major

I'm glad this came back up. I can see this "feature" catching out newbies (and some oldbies) again and again. As the fix is backwards incompatible it should really be done before 1.0 -> Priority nudge.

comment:3 Changed 9 years ago by akaihola

It would help most newbies if at least the examples in the documentation used allow_empty=True for now.

comment:4 Changed 9 years ago by real.human@…

yep, i just spent many hours over two days trying to figure out why code that was previously working was now returning a "404" with no useful debug information, either. +1 for this change :)

comment:5 Changed 9 years ago by Here

  • Type defect deleted

comment:6 Changed 9 years ago by anonymous

  • Owner changed from jacob to anonymous
  • Status changed from new to assigned
  • Type set to defect

comment:7 Changed 8 years ago by anjocee # gmail.com

I agree, too.

It spent my two or three hours to find out why 404.
and lucky found the answer with google.

btw, I like djanjo very much.

comment:8 Changed 8 years ago by Jared Kuolt

I do NOT think this is a defect. It makes sense to me that when an object is not found, it throws a 404 (object not found).

comment:9 Changed 8 years ago by anonymous

  • milestone set to Version 1.0
  • Resolution set to duplicate
  • Status changed from assigned to closed
  • Version set to 0.95

comment:10 Changed 8 years ago by Gary Wilson <gary.wilson@…>

  • Resolution duplicate deleted
  • Status changed from closed to reopened
  • Version changed from 0.95 to SVN

Correcting probable spam.

And I also agree that the default should be allow_empty=True.

comment:11 Changed 8 years ago by Gary Wilson <gary.wilson@…>

  • Summary changed from For predictability list_detail generic view shoud default allow_empty to True to [patch] For predictability list_detail generic view should default allow_empty to True

patch coming...

Changed 8 years ago by Gary Wilson <gary.wilson@…>

default value of allow_empty changed to True for the generic views (archive_* except archive_week which already defaulted to True and object_list) with the documentation changes as well.

comment:12 Changed 8 years ago by Gary Wilson <gary.wilson@…>

  • Summary changed from [patch] For predictability list_detail generic view should default allow_empty to True to [patch] list_detail and archive_* generic views should default allow_empty to True

comment:13 Changed 8 years ago by Indy

+1 from me. The current behaviour is a gotcha.

comment:14 Changed 8 years ago by anonymous

  • milestone Version 1.0 deleted

Milestone Version 1.0 deleted

comment:15 Changed 8 years ago by Gary Wilson <gary.wilson@…>

  • Triage Stage changed from Unreviewed to Ready for checkin

From comments above, I would say this is accepted.

comment:16 Changed 8 years ago by mtredinnick

  • Owner changed from anonymous to mtredinnick
  • Status changed from reopened to new
  • Triage Stage changed from Ready for checkin to Design decision needed

I'm not convinced this is "ready for checkin", since it breaks backwards compatibility. Four or five commenters in a ticket does not make a vast majority. At a minimum, I think this should wait until post-0.96 and be part of the backwards compatible changes for 1.0 if we include it.

I'm probably +1 on including the patch, but I know there is code out there relying on the current behaviour (I've written more than a few lines of such code) and breaking existing sites is not cool. So I'm moving this back to "design decision needed" in order to at least hold it over until the forthcoming release prior to the 1.0 push. Then we can decide (along with a bunch of other small items) whether we want to break backwards-compat.

comment:17 Changed 8 years ago by bitprophet

I strongly agree that the default ought to be allow_empty=True, so make that five or six commenters ;)

I won't bother reiterating the arguments already made, but I will point out that if possible it'd be nice if this particular default setting was highlighted more strongly in the documentation or an FAQ. It's definitely a FAQ in the literal sense, as in I see it asked all the time.

comment:18 Changed 8 years ago by SmileyChris

I'm +1 too.

comment:19 Changed 8 years ago by cmgreen@…

I just got bit by this issue. If it helps, generic views allows_empty brought me to this page wishing that this change is made. I troubleshot almost everything else about my config before I tracked this down.

comment:20 Changed 8 years ago by mtredinnick

Will everybody please stop posting "me too" comments; it's adds no value to the ticket report, since a few extra comments in a userbase of many thousands does not change the weighting. As mentioned above, this is one of the backwards incompatibility changes we will have after 0.96 is released in preparation for 1.0. Not before 0.96.

comment:21 Changed 8 years ago by mtredinnick

Err ... replace "backwards compatibility changes" with "backwards compatbility conversations" in the above. :(

comment:22 Changed 8 years ago by vinay_sajip@…

Backward compatibility can easily be maintained as follows: change the default value of allow_empty in the view definitions to a setting e.g. DEFAULT_ALLOW_EMPTY. This can be set to False by default in settings.py, and people who want to can change the setting to True on a per-instance basis. Doesn't this let us have our cake and eat it too?

comment:23 Changed 8 years ago by Gary Wilson <gary.wilson@…>

0.96 has been released now, do we want to go forward with this patch?

comment:24 Changed 8 years ago by mtredinnick

It hasn't been forgotten, Gary, don't worry.

comment:25 Changed 8 years ago by jacob

  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

Adrian and I feel that only the index views (archive_index, object_list) ought to default to allow_empty=True; I should get a 404 visiting (e.g.) a date that doesn't exist.

Changed 7 years ago by mcroydon

Per jacob's comments, apply allow_empty to generic index views and update docs. Apply patch with -p1.

comment:26 Changed 7 years ago by mcroydon

  • Patch needs improvement unset

Removing patch needs improvement flag. Should be ready for checkin pending review. Should this be noted in BackwardsIncompatibleChanges?

comment:27 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Accepted to Ready for checkin

Patch looks fine. Does need a note on the backwards-incompat page when it's committed.

comment:28 Changed 7 years ago by mtredinnick

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

(In [6833]) Fixed #685 -- archive_index() and object_list() generic views allow empty
result sets by default now. Thanks, Gary Wilson and Matt Croydon.

This is a backwards incompatible change.

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