Opened 10 years ago

Closed 10 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 Holovaty
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@…> 10 years ago.
Minimal test case
date_based.py.diff (3.8 KB) - added by Pete Crosier 10 years ago.
date_based.py.2.diff (3.3 KB) - added by Pete Crosier 10 years ago.
Works a bit more logically.

Download all attachments as: .zip

Change History (15)

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

Attachment: test_proj.zip added

Minimal test case

Changed 10 years ago by Pete Crosier

Attachment: date_based.py.diff added

comment:1 Changed 10 years ago by Pete Crosier

Cc: pete.crosier@… added
Resolution: fixed
Status: newclosed
Summary: previous_month and previous_day are non-empty even if there are no objects in previous months/days[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 10 years ago by Jacob

Resolution: fixed
Status: closedreopened

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 10 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 10 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 10 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 10 years ago by Pete Crosier

Attachment: date_based.py.2.diff added

Works a bit more logically.

comment:6 Changed 10 years ago by Adrian Holovaty

Owner: changed from Jacob to Adrian Holovaty
Status: reopenednew

comment:7 Changed 10 years ago by Adrian Holovaty

Status: newassigned

comment:8 Changed 10 years ago by Chris Beaven

Needs documentation: set
Triage Stage: UnreviewedReady 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 10 years ago by Chris Beaven

Triage Stage: Ready for checkinAccepted

Oops, I meant "accepted"

comment:10 Changed 10 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 10 years ago by Per Jonsson <poj@…>

Ticket #3585 has an alternate solution to this problem.

comment:12 Changed 10 years ago by Jacob

Resolution: duplicate
Status: assignedclosed

Marking duplicate (see Per's comments).

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