Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#7201 closed (fixed)

timeuntil filter misbehaves if my datetime has a timezone

Reported by: terry@… Owned by: jcarbaugh
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 jcarbaugh 6 years ago.
Fixes timezone errors in timeuntil and timesince. Includes tests and documentation updates.
7201.2.diff (7.3 KB) - added by jcarbaugh 6 years ago.
Modification of original patch to not assume timezone when none exists.
7201.3.diff (7.8 KB) - added by jcarbaugh 6 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 jcarbaugh 6 years ago.
Updated to work with changes introduced in [8535]

Download all attachments as: .zip

Change History (15)

comment:1 Changed 6 years ago by serialx

  • Component changed from Uncategorized to Template system
  • Keywords timeuntil, timesince added
  • milestone set to 1.0
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Seems to be a clear bug.

comment:2 Changed 6 years ago by leahculver

  • Owner changed from nobody to leahculver
  • Status changed from new to assigned

comment:3 Changed 6 years ago by leahculver

  • Owner leahculver deleted
  • Status changed from assigned to new

comment:4 Changed 6 years ago by jcarbaugh

  • Owner set to jcarbaugh
  • Status changed from new to assigned

comment:5 Changed 6 years ago by jcarbaugh

  • 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 6 years ago by jcarbaugh

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

Changed 6 years ago by jcarbaugh

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

Changed 6 years ago by jcarbaugh

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

comment:7 Changed 6 years ago by jcarbaugh

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 6 years ago by mtredinnick

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 6 years ago by jcarbaugh

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

comment:9 Changed 6 years ago by jcarbaugh

Thanks for reviewing the patch, Malcolm.

Changed 6 years ago by jcarbaugh

Updated to work with changes introduced in [8535]

comment:10 Changed 6 years ago by mtredinnick

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

(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 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.