Opened 9 years ago

Closed 8 years ago

Last modified 5 years ago

#7201 closed (fixed)

timeuntil filter misbehaves if my datetime has a timezone

Reported by: terry@… Owned by: Jeremy Carbaugh
Component: Template system Version: master
Severity: Keywords: timeuntil, timesince
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Looking in the code, I see that django.utils.timesince carefully tries to compute "now" using an appropriate timezone if you call timesince() with now=Null. However, if you call the timeuntil() routine in that module, or if you use django.template.defaultfilters.timeuntil(), then it computes now using no timezone at all. If the datetime you pass into timeuntil has a timezone set, then the result you get is several hours off.

Attachments (4)

7201.diff (8.0 KB) - added by Jeremy Carbaugh 8 years ago.
Fixes timezone errors in timeuntil and timesince. Includes tests and documentation updates.
7201.2.diff (7.3 KB) - added by Jeremy Carbaugh 8 years ago.
Modification of original patch to not assume timezone when none exists.
7201.3.diff (7.8 KB) - added by Jeremy Carbaugh 8 years ago.
This patch is the same as 7201.2.diff with an added change to AUTHORS per Malcolm's request.
7201.4.diff (7.9 KB) - added by Jeremy Carbaugh 8 years ago.
Updated to work with changes introduced in [8535]

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 years ago by Sung-jin Hong

Component: UncategorizedTemplate system
Keywords: timeuntil timesince added
milestone: 1.0
Triage Stage: UnreviewedAccepted

Seems to be a clear bug.

comment:2 Changed 8 years ago by Leah Culver

Owner: changed from nobody to Leah Culver
Status: newassigned

comment:3 Changed 8 years ago by Leah Culver

Owner: Leah Culver deleted
Status: assignednew

comment:4 Changed 8 years ago by Jeremy Carbaugh

Owner: set to Jeremy Carbaugh
Status: newassigned

comment:5 Changed 8 years ago by Jeremy Carbaugh

Has patch: set

Prior to this patch, the timesince(d, now=None) method would overwrite the timezone of the now argument if the d argument had tzinfo. This would cause the time difference to be calculated incorrectly. Additionally, if d did not have tzinfo, the tzinfo of now would be reset to None. In this case, the patched version will give the d argument the tzinfo of now to keep the behavior the same for both arguments.

This patch raises the issue of whether the timesince method should assume a timezone if one argument has tzinfo and the other does not. Default Python behavior is to raise a TypeError when operating on offset-naive and offset-aware datetimes. I am opening another ticket to address the issue of how this should be handled. Allowing an error to be raised when comparing timezone and non-timezone arguments will be a backwards incompatible change.

comment:6 Changed 8 years ago by Jeremy Carbaugh

Ticket #8080 has been created to address the behavior of timesince and timeuntil.

Changed 8 years ago by Jeremy Carbaugh

Attachment: 7201.diff added

Fixes timezone errors in timeuntil and timesince. Includes tests and documentation updates.

Changed 8 years ago by Jeremy Carbaugh

Attachment: 7201.2.diff added

Modification of original patch to not assume timezone when none exists.

comment:7 Changed 8 years ago by Jeremy Carbaugh

After thinking about the patch over the weekend and trying a few extra tests, I decided that the original patch did not adequately address the issue. The real bug in the code was not that a timezone was being overwritten, but that a timezone was assumed at all. Even with the bug fixed with the original patch, unexpected output still existed when a timezone was incorrectly assumed.

The current patch fixes the initial bug and any other instance where datetime arguments contain tzinfo. If two arguments are passed, one with tzinfo and one without, the filter will return an empty string as other filters do on TypeError or ValueError.

This patch did not break any previous tests, but may be backwards incompatible in cases where offset-naive and offset-aware datetimes are used as arguments.

I've closed #8080 that I opened earlier as this patch resolves that issue.

comment:8 Changed 8 years ago by Malcolm Tredinnick

I think you're logic is pretty sound in the revised patch. I think we can wear the backwards incompatibility, since I'd argue that if you're relying on the current behaviour, your results are already wrong.

Can you include a patch for the AUTHORS file as part of this? This isn't a trivial amount of effort to have worked it all out. We should immortalise your efforts.

Changed 8 years ago by Jeremy Carbaugh

Attachment: 7201.3.diff added

This patch is the same as 7201.2.diff with an added change to AUTHORS per Malcolm's request.

comment:9 Changed 8 years ago by Jeremy Carbaugh

Thanks for reviewing the patch, Malcolm.

Changed 8 years ago by Jeremy Carbaugh

Attachment: 7201.4.diff added

Updated to work with changes introduced in [8535]

comment:10 Changed 8 years ago by Malcolm Tredinnick

Resolution: fixed
Status: assignedclosed

(In [8579]) Fixed #7201 -- Fixed the timeuntil filter to work correctly with timezone-aware
times. Patch from Jeremy Carbaugh.

This is backwards incompatible in the sense that previously, if you tried to
compare timezone-aware and timezone-naive values, you got an incorrect result.
Now you get an empty string. So your previously incorrect code returns a
different incorrect result.

comment:11 Changed 5 years ago by Jacob

milestone: 1.0

Milestone 1.0 deleted

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