Ticket #10141 (closed Bug: fixed)

Opened 4 years ago

Last modified 3 years ago

pubdate keep updated (moving) to future time by time zone TD

Reported by: papago Owned by:
Priority: blocker Milestone: 4.0
Component: General Version:
Keywords: TuneUp28 Cc:

Description

[operation] The first make a content and set a future pubdate for a future publish. Then, modify the content and save it.

[symptom] Pubdate(publishing time) of the content move forward by time zone TD you set to site. For example, 9 hours will be added to pubdate the content every time modify and save in Japan.

Change History

comment:1 Changed 4 years ago by kleist

  • Priority changed from critical to major
  • Component changed from Unknown to Infrastructure

seems related to #10140

comment:2 follow-up: ↓ 5 Changed 4 years ago by witsch

  • Owner set to witsch
  • Keywords TuneUp26 added

marking for this week's tuneup — see  discussion on "plone-devel"

comment:3 Changed 4 years ago by witsch

also see  the thread on plone-dev for more info...

comment:4 Changed 4 years ago by kleist

  • Priority changed from major to blocker

see also #10171

comment:5 in reply to: ↑ 2 Changed 4 years ago by witsch

  • Owner witsch deleted

Replying to witsch:

marking for this week's tuneup [...]

unfortunately, i didn't get around to look at this during the tuneup, so i'm freeing it up again for now...

comment:6 Changed 4 years ago by jluvsu2

  • Keywords TuneUp27 added; TuneUp26 removed

comment:7 Changed 4 years ago by jessilfp

  • Keywords TuneUp28 added; TuneUp27 removed

comment:8 follow-up: ↓ 9 Changed 4 years ago by kleist

Maybe we should consolidate these three tickets, which all seem to describe the same problem: #10140 #10141 #10171 ?

comment:9 in reply to: ↑ 8 Changed 4 years ago by witsch

Replying to kleist:

Maybe we should consolidate these three tickets, which all seem to describe the same problem: #10140 #10141 #10171 ?

quoting from #10171:

this indeed seems to be a duplicate of #10171, but slightly different from #10140 (EffectiveDate vs ModificationDate). imho, the underlying problem with DateTime & parsing of strings is the same, though. let's keep the tickets open nevertheless so we make sure to catch both of these...

comment:10 Changed 4 years ago by witsch

(In [34520]) we probably need some changes & a new  DateTime release to fix the current time zone issues (refs #10140, #10141, #10171)

comment:11 Changed 4 years ago by witsch

(In [34521]) revert r34520 — it turns out DateTime doesn't need changes, but we need to use ISO8601() instead of ISO() everywhere (refs #10140, #10141, #10171)

comment:12 Changed 4 years ago by witsch

(In [34522]) fix issues with sliding modification/publishing dates by not using DateTime.ISO() anymore and replacing all occurrences with DateTime.ISO8601(), which contains the time zone. to make finding & fixing all of these easier, ISO gets monkey-patched to issue a deprecation warning and return the result of ISO8601 instead (fixes #10140 & refs #10141, #10171 pending more tests)

comment:13 Changed 4 years ago by witsch

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

(In [34523]) add more tests to make sure r34522 also fixes #10141 & #10171

comment:14 follow-up: ↓ 18 Changed 4 years ago by thet

these changes seem not to solve the problem for me. EffectiveDate, ExpirationDate as well as ATEvent's startDate and endDate keep on moving with the timezone-offset.

This is because they are displayed in the widget with the time converted to the local TZ (in my case CET) but saved with timezone set to GMT but the timevalues from the widget. So every time i reopen the object in the browser and save it, the time is moving for GMT+localtimezone.

The test in https://dev.plone.org/plone/changeset/34523 doesn't cover the behavior for EffectiveDate and ExpirationDate because the object isn't reopend and saved in testbrowser.

comment:15 Changed 4 years ago by thet

  • Status changed from closed to reopened
  • Resolution fixed deleted

comment:16 follow-ups: ↓ 17 ↓ 19 Changed 4 years ago by thet

Okey, in Products.Archetypes.Field.DateTimeField.set:

        if not value:
            value = None
        elif not isinstance(value, DateTime):
            try:
                value = DateTime(value)
            except DateTime.DateTimeError:
                value = None

DateTime() returns the current date as DateTime object in local timezone, but DateTime(value) returns a DateTime object in GMT.

We need value to be in local timezone here.

comment:17 in reply to: ↑ 16 ; follow-up: ↓ 20 Changed 4 years ago by thet

Replying to thet: this behavior of DateTime is due to:  http://zope3.pov.lt/trac/changeset/81213 and discussed in:  http://dev.plone.org/plone/ticket/10140

comment:18 in reply to: ↑ 14 Changed 4 years ago by witsch

Replying to thet:

This is because they are displayed in the widget with the time converted to the local TZ (in my case CET) but saved with timezone set to GMT but the timevalues from the widget.

aha, so the widget also returns the desired date & time as a string without a time zone (which is then interpreted to be GMT, by the DateTime constructor that is)...

The test in https://dev.plone.org/plone/changeset/34523 doesn't cover the behavior for EffectiveDate and ExpirationDate because the object isn't reopend and saved in testbrowser.

right, we should probably have a browsertest to make sure...

comment:19 in reply to: ↑ 16 Changed 4 years ago by witsch

Replying to thet:

DateTime() returns the current date as DateTime object in local timezone, but DateTime(value) returns a DateTime object in GMT.

exactly. one possible fix in the set() code, would be to get the local time zone (e.g. from another DateTime instance) and add its string representation to the value, kinda like so:

  • Products.Archetypes/Products/Archetypes/Field.py

     
    14461446        elif not isinstance(value, DateTime): 
    14471447            try: 
     1448                # strings returned from the widget need a time zone, 
     1449                # which is assumed to be the local one 
     1450                # see http://dev.plone.org/plone/ticket/10141 
     1451                value +=  ' ' + DateTime().timezone() 
    14481452                value = DateTime(value) 
    14491453            except DateTime.DateTimeError: 

this is untested and needs to have a failing test first, though ;)

comment:20 in reply to: ↑ 17 Changed 4 years ago by witsch

Replying to thet:

Replying to thet: this behavior of DateTime is due to:  http://zope3.pov.lt/trac/changeset/81213

unfortunately, i think this should/can not be reverted as discussed in #10140...

comment:21 Changed 4 years ago by thet

i currently wonder about this behavior of DateTime:

At midnight, TZ info is stripped:

>>> DateTime('2010-01-01 00:00:00 GMT+1')
DateTime('2010/01/01')

After midnight, not:

>>> DateTime('2010/01/01 00:00:01 GMT+1')
DateTime('2010/01/01 00:00:01 GMT+1')

is that intentional? if not, is there a bugtracker system for DateTime?  http://svn.zope.org/,  http://zope3.pov.lt/ or email?

comment:22 follow-up: ↓ 25 Changed 4 years ago by thet

okey, but

>>> DateTime('2010/01/01 00:00:00 GMT+1').ISO8601()
'2010-01-01T00:00:00+01:00'

returns a correct string...

comment:23 follow-up: ↓ 26 Changed 4 years ago by thet

Another behavior which seems to be inconsistent:

As expected, with dashes and a given timezone, the timzone info is preserved:
>>> DateTime('2010-03-08 11:00:00 GMT+3')
DateTime('2010/03/08 11:00:00 GMT+3')

The CalendarWidget returns DateTime values in the form '2010-03-08 11:00:00'.
DateTime uses naive timezone mode here and assumes to be in GMT:
>>> DateTime('2010-03-08 11:00:00')
DateTime('2010/03/08 11:00:00 GMT+0')

If we use slashes instead of dashes for the date ('2010/03/08 11:00:00'),
DateTime uses the local timezone:
>>> DateTime('2010/03/08 11:00:00')
DateTime('2010/03/08 11:00:00 GMT+1')

comment:24 Changed 4 years ago by witsch

(In [34639]) add browsertest to demonstrate the previous fix wasn't enough to keep the publishing date from moving when editing content. please note that the test currently still fails, but can be fixed using the suggested fix (refs #10141)

comment:25 in reply to: ↑ 22 Changed 4 years ago by witsch

Replying to thet:

okey, but

>>> DateTime('2010/01/01 00:00:00 GMT+1').ISO8601()
'2010-01-01T00:00:00+01:00'

returns a correct string...

exactly. i noticed the same, but __repr__ (or __str__) just tries to keep things short...

comment:26 in reply to: ↑ 23 Changed 4 years ago by witsch

Replying to thet:

If we use slashes instead of dashes for the date ('2010/03/08 11:00:00'), DateTime uses the local timezone:

i think this is because it internally uses two parsers, the old one and a new one for iso8601. i suspect that in this case the old one is used, resulting in the time zone being preserved. but that's not to say it's right or anything, i would totally agree this is a bug...

comment:27 Changed 4 years ago by witsch

(In [34640]) new releases of AT and ATCT will be required for test-adjustments regarding the timezone issues (refs #10141)

comment:28 follow-up: ↓ 30 Changed 4 years ago by witsch

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

the issue has been  fixed as suggested above

comment:29 Changed 4 years ago by witsch

(In [34642]) adjust the tests now that DateTime objects are  stored with a time zone: since having the given dates interpreted as from your local time is what you would expect, all time zone info as output by ISO8601 is simply ignored by them (refs #10141)

comment:30 in reply to: ↑ 28 Changed 4 years ago by witsch

Replying to witsch:

the issue has been  fixed as suggested above

the fix was updated in  r12303, now also handling values that do include a time zone...

comment:31 Changed 4 years ago by thet

  • Status changed from closed to reopened
  • Resolution fixed deleted

The fix r12303 didn't respect Daylight Saving Times and always converted dates to the server's current timezone. For CET, the date "2010-06-01" was converted to "2010-06-01 GMT+1" instead of "2010-06-01 GMT+2". This also caused moving dates in edit screen.

DateTime's timezone naive conversion only affects ISO strings like this examples show:

>>> from DateTime import DateTime

ISO with timezone preserves timezone:
>>> DateTime('2010-01-01 10:00:00 GMT+3')
DateTime('2010/01/01 10:00:00 GMT+3')

ISO without timezone assumes timezone naive dates:
>>> DateTime('2010-01-01 10:00:00')
DateTime('2010/01/01 10:00:00 GMT+0')

Non-ISO strings without timezone get the server's local timezone:
>>> DateTime('2010/01/01 10:00:00')
DateTime('2010/01/01 10:00:00 GMT+1')

For those, also Daylight Saving Times are respected:
>>> DateTime('2010/06/01 10:00:00')
DateTime('2010/06/01 10:00:00 GMT+2')

Perfect, let's convert ISO strings to NON-ISO strings in Products.Archetypes.Field.DateTimeField.set...

comment:32 Changed 4 years ago by thet

(In [35747]) Add test for date's timezone and Daylight Saving Time. refs #10141, r12413

comment:33 Changed 4 years ago by thet

(In [35748]) Adjust tests not to use EffectiveDate() and ExpirationDate() which convert dates to the servers current local zone (including Daylight Saving Time (DST)) if no zone is given. Standard Time dates (like 2001-01-01) become DST dates when server is in DST, which makes no sense. refs #10141, r12413

comment:34 Changed 4 years ago by thet

(In [35749]) moving dates bug. refs #10141, r12413

comment:35 follow-up: ↓ 36 Changed 4 years ago by thet

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

Closed by r12413

comment:36 in reply to: ↑ 35 ; follow-up: ↓ 37 Changed 4 years ago by witsch

Replying to thet:

Closed by r12413

looking at your iso/non-iso comparison above, wouldn't it be easier to just do

value = DateTime(value.replace('-', '/', 2))

instead of using the rather expensive regex parsing?

comment:37 in reply to: ↑ 36 Changed 4 years ago by witsch

Replying to witsch:

instead of using the rather expensive regex parsing?

 r12506 implements this, and the tests seem to be fine. please revert if you disagree on the change...

comment:38 Changed 3 years ago by ldr

Changing characters in a date string breaks parsing of many valid dates (e.g. '2000-01-01T00:00Z'). Instead let DateTime parse the string and then move timezone naive dates into the local timezone.  https://github.com/plone/Products.Archetypes/commit/873e9bcdfa36c5bc138e69345635a9767aa02cad

comment:39 Changed 22 months ago by davisagli

  • Component changed from Infrastructure to General
Note: See TracTickets for help on using tickets.