Bug 85239 - ActiveSync does not handle Outlook all day events correctly
Summary: ActiveSync does not handle Outlook all day events correctly
Status: RESOLVED FIXED
Alias: None
Product: SyncEvolution
Classification: Unclassified
Component: ActiveSync (show other bugs)
Version: 1.4
Hardware: Other Linux (All)
: medium normal
Assignee: SyncEvolution Community
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-20 12:40 UTC by Graham Cobb
Modified: 2015-02-09 13:52 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Patch to fix London timezone handling bug (1.17 KB, patch)
2014-10-28 23:03 UTC, Graham Cobb
Details | Splinter Review
Patch to improve logging problems with all day event times (1.58 KB, patch)
2014-10-28 23:06 UTC, Graham Cobb
Details | Splinter Review
Patch to fix theoretical problem with timezone calculation (1.50 KB, patch)
2014-10-28 23:08 UTC, Graham Cobb
Details | Splinter Review
Patch to turn all day event into not all day event if Exchange supplies wrong timezone info (2.09 KB, patch)
2014-10-29 18:43 UTC, Graham Cobb
Details | Splinter Review
Correct logging to stop memory leak (1.23 KB, patch)
2014-11-11 20:54 UTC, Graham Cobb
Details | Splinter Review

Description Graham Cobb 2014-10-20 12:40:35 UTC
Discussion from mailing list...

On Mon, 2014-10-20 at 10:17 +0100, Graham Cobb wrote:
> I know I have had trouble with all day events before, but it might have
> been in my OpenSync days (or even my own OWASync!).
>
> I have just noticed, in my testing of 1.4.99.4, that all day events are
> not working properly.  I don't suppose this is new in this version -- I
> probably didn't notice it before.

Right. That code hasn't changed in quite a while. The relevant code to
look at are the _eas2ical_convert_datetime_property() and
_eas2ical_convert_component() methods
activesyncd/eas-daemon/libeas/eas-cal-info-translator.c.

> I have an all day event in Outlook, scheduled for this Thursday
> (20141023). I did a one-way sync from Exchange (activesync) to files,
> and the file ended up with the following:
>
> DTSTART;VALUE=DATE:20141022
> DTEND;VALUE=DATE:20141023
>
> To make matters worse, I then synced those files with Owncloud and
> Owncloud now believes this is an all day event on the 22nd! (And so does
> Thunderbird when I synced that against Owncloud).
>
> I have not yet attempted to debug this (looking at what activesync
> sends, etc) but wondered if anyone either has seen this before, or has a
> view on what the correct VCALENDAR file should look like for this event.

Correct for an all-day event on (and just on) the 23rd would be:

DTSTART;VALUE=DATE:20141023
DTEND;VALUE=DATE:20141024

The end date(-time) is exclusive.

The key problem with the conversion is that Microsoft requires putting a
time into the start and end fields. But which time does an all-day event
start and end at? Local time? UTC? Does it depend on where someone is?

This is all not defined in the Microsoft documentation. It only
documents a flag that marks an event as all-day.

If I remember correctly, in practice it had to be the local time of the
user. So if I am in GMT+2, the 00:00 time as to be shifted by two hours,
thus causing the date to also change. Obviously this shift goes wrong
when the time zones are not set correctly (or at least consistently). I
have no idea how that is supposed to be handled correctly in all cases.
Comment 1 Graham Cobb 2014-10-28 23:03:22 UTC
Created attachment 108597 [details] [review]
Patch to fix London timezone handling bug

Patch to fix the actual problem reported in this bug.

The problem was because the timezone handling code incorrectly optimized away the timezone for the UK daylight savings time, which meant the all day event start and end times were not being correctly converted to local time.
Comment 2 Graham Cobb 2014-10-28 23:06:42 UTC
Created attachment 108598 [details] [review]
Patch to improve logging problems with all day event times

This second patch just improves the logging when all day event times fail the "sanity test" (ie. are not at midnight).
Comment 3 Graham Cobb 2014-10-28 23:08:59 UTC
Created attachment 108599 [details] [review]
Patch to fix theoretical problem with timezone calculation

This third patch fixes a bug unlikely to be seen in real life.  It would only occur if the "StandardBias" feild of a timezone was set to a non-zero value.  This is not the normal way timezones are used but could happen in theory.
Comment 4 Graham Cobb 2014-10-29 18:43:29 UTC
Created attachment 108643 [details] [review]
Patch to turn all day event into not all day event if Exchange supplies wrong timezone info

This patch adds code to effectively turn an all day event into a non-all-day event if the time is not midnight.  It actually just changes the format to be a full time, instead of just a day, but that has the effect of it not being an all day event in most applications.
Comment 5 Patrick Ohly 2014-10-30 20:21:29 UTC
Comment on attachment 108598 [details] [review]
Patch to improve logging problems with all day event times

Review of attachment 108598 [details] [review]:
-----------------------------------------------------------------

A better formatting of the commit message would be a short summary line followed by an empty line, then full description.

g_warning("All day event with no timezone does not start at UTC midnight: %s", icaltime_as_ical_string_r(tt));

This leaks the string, because the _r variant returns the newly allocated string and expects the caller to free it. The other locations with the normal version are okay because libical then retains ownership in a ring buffer.

Can you fix that in a future patch? For now I'll take the patch as-is because I cannot test it myself and don't want to break something that you already tested successfully.
Comment 6 Graham Cobb 2014-11-11 20:54:13 UTC
Created attachment 109306 [details] [review]
Correct logging to stop memory leak

Sorry about the commit message style -- I am used to a different style.  Hope this one is better.

This fixes the logging memory leak.  I originally thought the _r variants used the ring buffers but after reading the code I realised that, in fact, the variants without the _r used the ring.  But I missed one of the calls to correct.
Comment 7 Patrick Ohly 2015-02-09 13:52:06 UTC
Memory leak patch merged, thanks.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.