0013526: Calendar ICS import can't cope with floating time #6691

Closed
Gloirin opened this Issue Jun 9, 2018 · 21 comments

Comments

Projects
None yet
3 participants
@Gloirin

Gloirin commented Jun 9, 2018

Reported by ingoratsdorf on 6 Oct 2017 06:03

Version: git master

Imported events from a booking calendar do not contain organizers (as there are none) and are in local UTC+12 (NZDT).
The resulting imported events automatically add the current user as organizer (which is wrong, I am not organizing this and I am not booked for this event) plus is gets imported for 12 hours later.

Test data:

BEGIN:VEVENT
SUMMARY:160:Susan
DESCRIPTION: Blabla
DTSTART:20171006T150001
DTEND:20171009T100002
UID:2017-10-06 15:00:01_160@demo.icalendar.org
DTSTAMP:20171006T053151
CREATED:20170804T112346
LAST-MODIFIED:20170804T112346
STATUS:CONFIRMED
END:VEVENT

Gets imported for the 7. October at 4am.

Steps to reproduce: Import calendar.
Import adds current user as organizer and is 12 hours too late.
Since no timezone information is in the event, it gets imported as UTC, which is wrong as server and user timezone is Pacific/Auckland (NZDT / UTC+1200)

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by ingoratsdorf on 7 Oct 2017 09:21

Timezone issue:
FORM #1: DATE WITH LOCAL TIME

  The date with local time form is simply a DATE-TIME value that
  does not contain the UTC designator nor does it reference a time
  zone.  For example, the following represents January 18, 1998, at
  11 PM:

   19980118T230000

  DATE-TIME values of this type are said to be "floating" and are
  not bound to any time zone in particular.  They are used to
  represent the same hour, minute, and second value regardless of
  which time zone is currently being observed. 

However the Abstract VCalendar converter uses Sabre\VObject\Property\ICalendar\DateTime->getDateTime(); which returns all floating dates with a UTC timezone attached. This should be the local time.
Sabre has always returned floating datetimes as UTC and still does.

Fixed Tinebase->Convert->VCalendar->Abstract.php code snippet:

/**
* get datetime from sabredav datetime property (user TZ is fallback)
*
* @param Sabre\VObject\Property $dateTimeProperty
* @param boolean $_useUserTZ
* @return Tinebase_DateTime
*
* @todo try to guess some common timezones
*/
protected function _convertToTinebaseDateTime(\Sabre\VObject\Property $dateTimeProperty, $_useUserTZ = FALSE)
{
$defaultTimezone = date_default_timezone_get();
date_default_timezone_set((string) Tinebase_Core::getUserTimezone());

    if ($dateTimeProperty instanceof Sabre\VObject\Property\ICalendar\DateTime) {
        $dateTime = $dateTimeProperty->getDateTime();
        $originalValue = $dateTimeProperty->getValue();
        if (substr(strtoupper($originalValue) , strlen($originalValue)-1, 1 ) != 'Z')
            // floating datetime
            $_useUserTZ = true;
        $tz = ($_useUserTZ || (isset($dateTimeProperty['VALUE']) && strtoupper($dateTimeProperty['VALUE']) == 'DATE')) ? 
            (string) Tinebase_Core::getUserTimezone() : 
            $dateTime->getTimezone();

        Tinebase_Core::getLogger()->warn(__METHOD__ . '::' . __LINE__ . ' Sabre DateTime Value: ' . print_r($originalValue, true));
        $result = new Tinebase_DateTime($dateTime->format(Tinebase_Record_Abstract::ISO8601LONG), $tz);
    } else {
        $result = new Tinebase_DateTime($dateTimeProperty->getValue());
    }
    
    date_default_timezone_set($defaultTimezone);
    
    return $result

}

Gloirin commented Jun 11, 2018

Comment posted by ingoratsdorf on 7 Oct 2017 09:21

Timezone issue:
FORM #1: DATE WITH LOCAL TIME

  The date with local time form is simply a DATE-TIME value that
  does not contain the UTC designator nor does it reference a time
  zone.  For example, the following represents January 18, 1998, at
  11 PM:

   19980118T230000

  DATE-TIME values of this type are said to be "floating" and are
  not bound to any time zone in particular.  They are used to
  represent the same hour, minute, and second value regardless of
  which time zone is currently being observed. 

However the Abstract VCalendar converter uses Sabre\VObject\Property\ICalendar\DateTime->getDateTime(); which returns all floating dates with a UTC timezone attached. This should be the local time.
Sabre has always returned floating datetimes as UTC and still does.

Fixed Tinebase->Convert->VCalendar->Abstract.php code snippet:

/**
* get datetime from sabredav datetime property (user TZ is fallback)
*
* @param Sabre\VObject\Property $dateTimeProperty
* @param boolean $_useUserTZ
* @return Tinebase_DateTime
*
* @todo try to guess some common timezones
*/
protected function _convertToTinebaseDateTime(\Sabre\VObject\Property $dateTimeProperty, $_useUserTZ = FALSE)
{
$defaultTimezone = date_default_timezone_get();
date_default_timezone_set((string) Tinebase_Core::getUserTimezone());

    if ($dateTimeProperty instanceof Sabre\VObject\Property\ICalendar\DateTime) {
        $dateTime = $dateTimeProperty->getDateTime();
        $originalValue = $dateTimeProperty->getValue();
        if (substr(strtoupper($originalValue) , strlen($originalValue)-1, 1 ) != 'Z')
            // floating datetime
            $_useUserTZ = true;
        $tz = ($_useUserTZ || (isset($dateTimeProperty['VALUE']) && strtoupper($dateTimeProperty['VALUE']) == 'DATE')) ? 
            (string) Tinebase_Core::getUserTimezone() : 
            $dateTime->getTimezone();

        Tinebase_Core::getLogger()->warn(__METHOD__ . '::' . __LINE__ . ' Sabre DateTime Value: ' . print_r($originalValue, true));
        $result = new Tinebase_DateTime($dateTime->format(Tinebase_Record_Abstract::ISO8601LONG), $tz);
    } else {
        $result = new Tinebase_DateTime($dateTimeProperty->getValue());
    }
    
    date_default_timezone_set($defaultTimezone);
    
    return $result

}

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by pschuele on 9 Oct 2017 06:26

thanks for the fix, we'll review that soon. i'll push it to gerrit for verification.

Gloirin commented Jun 11, 2018

Comment posted by pschuele on 9 Oct 2017 06:26

thanks for the fix, we'll review that soon. i'll push it to gerrit for verification.

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by pschuele on 9 Oct 2017 06:27

https://gerrit.tine20.com/customers/5943

Gloirin commented Jun 11, 2018

Comment posted by pschuele on 9 Oct 2017 06:27

https://gerrit.tine20.com/customers/5943

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by pschuele on 9 Oct 2017 07:10

hm, we have a failing test. it looks like the TZ isn't detected any more ...

Calendar_Convert_Event_VCalendar_GenericTest::testConvertToTine20ModelWithTehranTZ
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Asia/Tehran'
+'Europe/Berlin'

/usr/local/share/tine20.git/tests/tine20/Calendar/Convert/Event/VCalendar/GenericTest.php:169

Gloirin commented Jun 11, 2018

Comment posted by pschuele on 9 Oct 2017 07:10

hm, we have a failing test. it looks like the TZ isn't detected any more ...

Calendar_Convert_Event_VCalendar_GenericTest::testConvertToTine20ModelWithTehranTZ
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Asia/Tehran'
+'Europe/Berlin'

/usr/local/share/tine20.git/tests/tine20/Calendar/Convert/Event/VCalendar/GenericTest.php:169

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by ingoratsdorf on 9 Oct 2017 09:15

Yeah, I need to do some cross testing with this, plus get the organizer/attendee issue resolved.
Thanks for testing.

Gloirin commented Jun 11, 2018

Comment posted by ingoratsdorf on 9 Oct 2017 09:15

Yeah, I need to do some cross testing with this, plus get the organizer/attendee issue resolved.
Thanks for testing.

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by weberho on 12 Oct 2017 14:19

For me this problem has never been solved. I had a patch prepared in #6296 which fixed the tinezone problem for me. The second patch there is still not integrated. You could give it a try.

Gloirin commented Jun 11, 2018

Comment posted by weberho on 12 Oct 2017 14:19

For me this problem has never been solved. I had a patch prepared in #6296 which fixed the tinezone problem for me. The second patch there is still not integrated. You could give it a try.

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by pschuele on 13 Oct 2017 07:12

ah, this patch might have been overlooked... I'll try to integrate it.

Gloirin commented Jun 11, 2018

Comment posted by pschuele on 13 Oct 2017 07:12

ah, this patch might have been overlooked... I'll try to integrate it.

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by pschuele on 13 Oct 2017 07:21

@ingo: could you try weberhos patch? maybe it fixes your TZ issue, too.

Gloirin commented Jun 11, 2018

Comment posted by pschuele on 13 Oct 2017 07:21

@ingo: could you try weberhos patch? maybe it fixes your TZ issue, too.

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by pschuele on 13 Oct 2017 07:22

weberho patch: https://gerrit.tine20.com/customers/6025

Gloirin commented Jun 11, 2018

Comment posted by pschuele on 13 Oct 2017 07:22

weberho patch: https://gerrit.tine20.com/customers/6025

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by ingoratsdorf on 13 Oct 2017 09:51

Aus irgendwelchen unerfindlichen Gründen habe ich keinen Zugang zu Gerrit mehr. Und da ich mich nicht einloggen kann, kann ich auch nicht mir einen Passwort reset link senden... Oder so.

Gloirin commented Jun 11, 2018

Comment posted by ingoratsdorf on 13 Oct 2017 09:51

Aus irgendwelchen unerfindlichen Gründen habe ich keinen Zugang zu Gerrit mehr. Und da ich mich nicht einloggen kann, kann ich auch nicht mir einen Passwort reset link senden... Oder so.

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by ingoratsdorf on 13 Oct 2017 09:55

I fixed the timezone issue meanwhile, however struggle big time with the organizer issue.
How do you import a, say, holiday calendar without being added as attendee and organizer and your time shown blocked. That's just plain silly.
I have dug deep in tine calendar for this and tried to trace the auto addition but was not too successful.. Somewhere ended up in Event.php.

Gloirin commented Jun 11, 2018

Comment posted by ingoratsdorf on 13 Oct 2017 09:55

I fixed the timezone issue meanwhile, however struggle big time with the organizer issue.
How do you import a, say, holiday calendar without being added as attendee and organizer and your time shown blocked. That's just plain silly.
I have dug deep in tine calendar for this and tried to trace the auto addition but was not too successful.. Somewhere ended up in Event.php.

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by ingoratsdorf on 13 Oct 2017 19:41

The patch for the timezone conversion issue, when

  • no timezone is specified in the event
  • no TZID is noted in the DATETIME values
  • times are floating and not in UTC, ie local times

Gloirin commented Jun 11, 2018

Comment posted by ingoratsdorf on 13 Oct 2017 19:41

The patch for the timezone conversion issue, when

  • no timezone is specified in the event
  • no TZID is noted in the DATETIME values
  • times are floating and not in UTC, ie local times
@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by pschuele on 16 Oct 2017 07:23

i also uploaded the patches to the community gerrit:

weberhos patch: https://gerrit.tine20.org/tine20/3483
Ingos patch: https://gerrit.tine20.org/tine20/3485

I'll post the results of jenkins when i have them.

Gloirin commented Jun 11, 2018

Comment posted by pschuele on 16 Oct 2017 07:23

i also uploaded the patches to the community gerrit:

weberhos patch: https://gerrit.tine20.org/tine20/3483
Ingos patch: https://gerrit.tine20.org/tine20/3485

I'll post the results of jenkins when i have them.

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by pschuele on 16 Oct 2017 07:26

> How do you import a, say, holiday calendar without being added as attendee and organizer and your time shown blocked. That's just plain silly.

I'll ask Cornelius about that, maybe he has a good suggestion how to solve this.

Gloirin commented Jun 11, 2018

Comment posted by pschuele on 16 Oct 2017 07:26

> How do you import a, say, holiday calendar without being added as attendee and organizer and your time shown blocked. That's just plain silly.

I'll ask Cornelius about that, maybe he has a good suggestion how to solve this.

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by ingoratsdorf on 20 Mar 2018 09:27

Houston, we have a regression.

My patch has not been included and as I have replaced my installation of tine20 with a snapshot of GIT, the issue is back again. My patch fixes the issue for me, but not sure why the TZID=Asia/Tehran fails (Import/files/lightning_TehranTZ.ics) when the test with Lightings bad TZ (Import/files/lightning_badTZ.ics) apparently goes through correctly.
My modifications only change the timezone to the local timezone when the recognised timezone is "UTC" with a missing "Z" at the end of the datetime string.

Gloirin commented Jun 11, 2018

Comment posted by ingoratsdorf on 20 Mar 2018 09:27

Houston, we have a regression.

My patch has not been included and as I have replaced my installation of tine20 with a snapshot of GIT, the issue is back again. My patch fixes the issue for me, but not sure why the TZID=Asia/Tehran fails (Import/files/lightning_TehranTZ.ics) when the test with Lightings bad TZ (Import/files/lightning_badTZ.ics) apparently goes through correctly.
My modifications only change the timezone to the local timezone when the recognised timezone is "UTC" with a missing "Z" at the end of the datetime string.

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by pschuele on 19 Apr 2018 14:14

yep, the patch has not been merged yet as there are those failing tests. @cornelius: please have a look at it. I just rebased the change.

Gloirin commented Jun 11, 2018

Comment posted by pschuele on 19 Apr 2018 14:14

yep, the patch has not been merged yet as there are those failing tests. @cornelius: please have a look at it. I just rebased the change.

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by pschuele on 19 Apr 2018 14:14

https://gerrit.tine20.com/customers/#/c/5943/

Gloirin commented Jun 11, 2018

Comment posted by pschuele on 19 Apr 2018 14:14

https://gerrit.tine20.com/customers/#/c/5943/

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by cweiss on 19 Apr 2018 14:49

no test case? are you serious guys?

Gloirin commented Jun 11, 2018

Comment posted by cweiss on 19 Apr 2018 14:49

no test case? are you serious guys?

@Gloirin

This comment has been minimized.

Show comment
Hide comment
@Gloirin

Gloirin Jun 11, 2018

Comment posted by cweiss on 19 Apr 2018 15:22

here is the fix for the floating thing (also attached)
https://gerrit.tine20.com/customers/c/8959/

For the organiser/attendee there should already be an issue somewhere here. if not feel free to open one.
I think we should have import options for this.

Gloirin commented Jun 11, 2018

Comment posted by cweiss on 19 Apr 2018 15:22

here is the fix for the floating thing (also attached)
https://gerrit.tine20.com/customers/c/8959/

For the organiser/attendee there should already be an issue somewhere here. if not feel free to open one.
I think we should have import options for this.

@ingo

This comment has been minimized.

Show comment
Hide comment
@ingo

ingo Jun 11, 2018

@Gloirin @corneliusweiss @ingoratsdorf I am not the "ingo" you keep messaging, and I have 100 messages in my inbox directed to the wrong person. Please use mentions correctly.

ingo commented Jun 11, 2018

@Gloirin @corneliusweiss @ingoratsdorf I am not the "ingo" you keep messaging, and I have 100 messages in my inbox directed to the wrong person. Please use mentions correctly.

@pschuele

This comment has been minimized.

Show comment
Hide comment
@pschuele

pschuele Jun 11, 2018

Member

@ingo sorry for that - just migrated our issues from mantis to github (which creates notifications for all mentions) ...

Member

pschuele commented Jun 11, 2018

@ingo sorry for that - just migrated our issues from mantis to github (which creates notifications for all mentions) ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment