Edgewall Software
Modify

Opened 10 years ago

Closed 10 years ago

#8240 closed defect (fixed)

Can not set completion date on milestone due to timezone daylight saving

Reported by: Felix Schwarz <felix.schwarz@…> Owned by: Remy Blank
Priority: normal Milestone: 0.11.6
Component: ticket system Version: 0.11.4
Severity: normal Keywords:
Cc: info@…, michaelc@…, jappie@… Branch:
Release Notes:
API Changes:

Description

If I want to set a completion date for a milestone very close to the actual time, I get the warning message: "Warning: Completion date may not be in the future". For example at '04/27/09 15:30:00' I can not put a completion date of a milestone which is '04/27/09 15:00:00'.

I think this is something closely related to trac's/Python's datetime handling with timezones (the culprit is probably 'trac.util.datefmt.parse_date').

This is my setup:

  • Trac 0.11.4 (seeing this with 0.11stable as well)
  • Python 2.3 (also checked with Python 2.5)
  • pytz is installed
  • server timezone (default_timezone in trac.ini) is set to utc
  • in the user preferences a different timezone was stored, in that timezone daylight saving is currently active (e.g. Europe/Berlin)

What happens is that parse_date returns a date with the non-dst timezone. Therefore the computed time is off by one hour and compared to UTC it looks like it is a date in the future (because trac only deducts 1h instead of 2h in Europe/Berlin (CEST → GMT+2).

This does not happen if pytz is not installed because trac's timezones don't imply any daylight saving settings.

You can add a workaround for the specific issue if you just remove the check for future days. However this still affects plugin writers.

I think the correct way to deal with this is to use the normalize()/localize() method of a pytz tzinfo object on the generated datetime. I think something like that should be added (branches/0.11-stable/trac/util/datefmt.py#L233):

if not hasattr(tzinfo, 'localize'):
    # This is a tzinfo define by trac which don't have to deal with dst
    dt = datetime(*(tm[0:6] + (0, tzinfo)))
else:
    # We need to detect daylight saving correctly - see #...
    dt = tzinfo.localize(datetime(*tm[0:6]))

Attachments (1)

8240-tzinfo-localize-r8816.patch (2.8 KB ) - added by Remy Blank 10 years ago.
Use tzinfo.localize() when constructing datetime objects.

Download all attachments as: .zip

Change History (12)

comment:1 by Christian Boos, 10 years ago

Milestone: 0.11.5
Owner: set to Christian Boos

Yes, supporting the DST correctly is definitely needed (see also #5895).

As the pytz documentation states:

using the tzinfo argument of the standard datetime constructors ‘’does not work’’ with pytz for many timezones.

So we should rather find a way to consistently use localize/normalize in to_datetime/to_timestamp and related places.

comment:2 by Felix Schwarz <felix.schwarz@…>, 10 years ago

I think this should be a fairly simply change if we assume that all datetime instances - once built - contain the correct timezone. So I think the only functions to change are to_datetime and parse_date in datefmt.py.

comment:3 by Thijs Triemstra <info@…>, 10 years ago

Cc: info@… added

comment:4 by Michael Ching <michaelc@…>, 10 years ago

Cc: michaelc@… added

comment:5 by Jappie <jappie@…>, 10 years ago

Cc: jappie@… added

I think this should be a fairly simply change if we assume that all datetime instances - once built - contain the correct timezone. So I think the only functions to change are to_datetime and parse_date in datefmt.py.

Is there an indication to if and when such fix will be made?

in reply to:  5 comment:6 by Jappie <jappie@…>, 10 years ago

Replying to Jappie <jappie@…>:

Is there an indication to if and when such fix will be made?

Sorry for not giving additional information!

trac: 0.11.5
python: 2.6.2-r1
pytz: 2009j
timezone: Europe/Amsterdam

I have the same problem as reported by Felix Schwarz. The patch Felix Schwarz sugests works perfectly for me. I haven't encountered a related issue in Trac yet, so I cannot verify if the second solution will work.

comment:7 by Remy Blank, 10 years ago

The pytz docstring for tzinfo.localize() is quite explicit about this:

Convert naive time to local time. This method should be used to construct localtimes, rather than passing a tzinfo argument to a datetime constructor.

So I think Felix' suggestion is correct: we should always construct "naive" datetime objects (i.e. without a tzinfo), then pass them to tzinfo.localize(). I have a slightly more elegant solution than the proposed patch. I also have a test case that fails with the current implementation, and passes when tzinfo.localize() is used. Patch follows.

by Remy Blank, 10 years ago

Use tzinfo.localize() when constructing datetime objects.

comment:8 by Remy Blank, 10 years ago

Resolution: fixed
Status: newclosed

Patch applied in [8831].

comment:9 by Remy Blank, 10 years ago

Owner: changed from Christian Boos to Remy Blank

comment:10 by Remy Blank, 10 years ago

Resolution: fixed
Status: closedreopened

Following the consistent failures on Tim's buildbot, I have investigated this some more, and I can reproduce the issue by switching my timezone to PST. I'm working on a fix.

comment:11 by Remy Blank, 10 years ago

Resolution: fixed
Status: reopenedclosed

I was a bit too enthusiastic with my changes from [8831]. Only the calls to the datetime constructor must be wrapped in tzinfo.localize(), not the calls to now() or fromtimestamp().

I didn't notice the failure, because the one test case that tested it was using exactly my timezone. The issue is fixed in [8857], with an additional test to avoid this special case.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.