Opened 9 years ago

Closed 9 years ago

#2386 closed defect (duplicate)

[patch]previous_month and previous_day are non-empty even if there are no objects in previous months/days

Reported by: Tyson Tate <tyson@…> Owned by: adrian
Component: Generic views Version: master
Severity: normal Keywords: previous_month previous_day genericviews
Cc: tyson@…, pete.crosier@…, poj+django@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

Yes, I know this is documented behavior, but it is completely illogical.

In the django.views.generic.date_based generic views, previous_month and previous_day}} are not empty when there are no objects in previous months/days. {{{next_month and {{{next_day}} are empty when there are no next months and days, which is logical.

I'd expect previous_month and {{{previous_day}} to be empty when there are no objects in the previous months/days. How else are you supposed to implement date-based navigation?

Attachments (3)

test_proj.zip (8.5 KB) - added by Tyson Tate <tyson@…> 9 years ago.
Minimal test case
date_based.py.diff (3.8 KB) - added by Pete Crosier 9 years ago.
date_based.py.2.diff (3.3 KB) - added by Pete Crosier 9 years ago.
Works a bit more logically.

Download all attachments as: .zip

Change History (15)

Changed 9 years ago by Tyson Tate <tyson@…>

Minimal test case

Changed 9 years ago by Pete Crosier

comment:1 Changed 9 years ago by Pete Crosier

  • Cc pete.crosier@… added
  • Resolution set to fixed
  • Status changed from new to closed
  • Summary changed from previous_month and previous_day are non-empty even if there are no objects in previous months/days to [patch]previous_month and previous_day are non-empty even if there are no objects in previous months/days

I've put together a patch that I think does what you want.

archive_month now returns a next_month and previous_month only for months with objects, otherwise it returns None . This takes into account the specified year only, which seems logical (to me).

archive_day now returns a next_day and previous_day only for days with objects, otherwise it returns None . This takes into account the specified month only, which seems logical (again, to me).

Criticism fully appreciated, and please un-fix this if I shouldn't have marked it so :)

comment:2 Changed 9 years ago by jacob

  • Resolution fixed deleted
  • Status changed from closed to reopened

Good patch, Pete!

I'm not so hot on the idea of only returning next/prev in the current year/month; seems like that would bite me in the ass when dealing with Jan 1st :). Think you could rework the patch to always return the next/prev month/day (if it exists) reguardless of current year/month?

(Oh, and marking it fixed is a little premature; we still gotta apply the patch :)

comment:3 Changed 9 years ago by Pete Crosier

That was fast, and cheers for re-opening it :) I wasn't sure which way to go with the scope of returning things, but I see where you're coming from.. I'll see what I can conjure up.

comment:4 Changed 9 years ago by Tyson Tate <tyson@…>

Just a note, Pete: you rule. Thanks a ton for looking at this. I was going to take a crack at it tonight, but your patch does exactly what I'm looking for (except for what Jacob mentioned, which I agree with).

This is going to make date-based navigation with the generic views a dream.

comment:5 Changed 9 years ago by Pete Crosier

OK, version 2 is sorted.. it works more logically and the code is a bit cleaner too because of it.

Now it returns the closest previous/next months from the queryset from any year, if applicable. And it returns the closest previous/next days for any month/year, if applicable.

Thanks Tyson/Jacob :)

Changed 9 years ago by Pete Crosier

Works a bit more logically.

comment:6 Changed 9 years ago by adrian

  • Owner changed from jacob to adrian
  • Status changed from reopened to new

comment:7 Changed 9 years ago by adrian

  • Status changed from new to assigned

comment:8 Changed 9 years ago by SmileyChris

  • Needs documentation set
  • Triage Stage changed from Unreviewed to Ready for checkin

If the existing behaviour is "documented" as the ticket suggests, this documentation needs changing. Someone care to provide the patch for this too?

Extra credit if you could come up with some tests, but I'm not sure they are necessary.

comment:9 Changed 9 years ago by SmileyChris

  • Triage Stage changed from Ready for checkin to Accepted

Oops, I meant "accepted"

comment:10 Changed 9 years ago by Per Jonsson <poj@…>

  • Cc poj+django@… added
  • Patch needs improvement set

The patch no longer applies, as it's changes conflicts with changes introduced when fixing #2433.
The allow_future parameter introduced by #2433 is not honored. So patch needs improvement.

comment:11 Changed 9 years ago by Per Jonsson <poj@…>

Ticket #3585 has an alternate solution to this problem.

comment:12 Changed 9 years ago by jacob

  • Resolution set to duplicate
  • Status changed from assigned to closed

Marking duplicate (see Per's comments).

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