[RFC] When importing events preserve VTIMEZONES

CHANGELOG.rst

@@ -45,6 +45,18 @@ ikhal
   to another date (while the event column is in focus), that date should be
   highlighted in the calendar (Christian Geier)
 
+0.8.4
+=====
+released 2016-10-06
+
+* **IMPORTANT BUGFIX** fixed a bug that lead to imported events being
+  erroneously shifted if they had a timezone identifier that wasn't an Olson
+  database identifier. All users are advised to upgrade as soon as possible. To
+  see if you are affected by this and how to resolve any issues, please see the
+  release announcement (khal/doc/source/news/khal084.rst or
+  http://lostpackets.de/khal/news/khal084.html). Thanks to Wayne Werner for
+  finding and reporting this bug.
+
 0.8.3
 =====
 released 2016-08-28

doc/source/news.rst

@@ -12,6 +12,7 @@ available as an `rss feed <https://lostpackets.de/khal/index.rss>`_ |rss|.
     :title: khal news
     :link: http://lostpackets.de/khal/
 
+    news/khal084
     news/khal083
     news/khal082
     news/khal081

doc/source/news/khal084.rst

@@ -0,0 +1,45 @@
+khal v0.8.4 released
+====================
+
+.. feed-entry::
+        :date: 2016-10-06
+
+`khal v0.8.4`_ (pypi_) is a bugfix release that fixes a **critical bug** in `khal
+import`. **All users are advised to upgrade as soon as possible**.
+
+Details
+~~~~~~~
+If importing events from `.ics` files, any VTIMEZONEs (specifications of the
+timezone) would *not* be imported with those events.
+As khal understands Olson DB timezone specifiers (such as "Europe/Berlin" or
+"America/New_York", events using those timezones are displayed in the correct
+timezone, but all other events are displayed as if they were in the configured
+*default timezone*.
+**This can lead to imported events being shown at wrong times!**
+
+
+Solution
+~~~~~~~~
+First, please upgrade khal to either v0.8.4 or, if you are using a version of khal directly
+from the git repository, upgrade to the latest version from github_.
+
+To see if you are affected by this bug, delete your local khal caching db,
+(usually `~/.local/share/khal/khal.db`), re-run khal and watch out for lines
+looking like this:
+``warning: $PROPERTY has invalid or incomprehensible timezone information in
+$long_uid.ics in $my_collection``.
+You will then need to edit these files by hand and either replace the timezone
+identifiers with the corresponding one from the Olson DB (e.g., change
+`Europe_Berlin` to `Europe/Berlin`) or copy original VTIMZONE definition in.
+
+If you have any problems with this, please either open an `issue at github`_ or come into
+our `irc channel`_ (`#pimutils` on Freenode).
+
+We are sorry for any inconveniences this is causing you!
+
+
+.. _khal v0.8.4: https://lostpackets.de/khal/downloads/khal-0.8.4.tar.gz
+.. _github: https://github.com/pimutils/khal/
+.. _issue at github: https://github.com/pimutils/khal/issues
+.. _pypi: https://pypi.python.org/pypi/khal/
+.. _irc channel: irc://#pimutils@Freenode

khal/controllers.py

@@ -27,7 +27,7 @@
 
 import pytz
 
-from collections import defaultdict, OrderedDict
+from collections import OrderedDict
 from shutil import get_terminal_size
 
 from datetime import timedelta, datetime
@@ -39,7 +39,6 @@
 from khal.khalendar.exceptions import ReadOnlyCalendarError, DuplicateUid
 from khal.exceptions import InvalidDate, FatalError
 from khal.khalendar.event import Event
-from khal.khalendar.backend import sort_key
 from khal import __version__, __productname__
 from khal.log import logger
 from .terminal import merge_columns
@@ -527,32 +526,32 @@ def import_ics(collection, conf, ics, batch=False, random_uid=False, format=None
     :param batch: setting this to True will insert without asking for approval,
                   even when an event with the same uid already exists
     :type batch: bool
+    :param random_uid: whether to assign a random UID to imported events or not
+    :type random_uid: bool
+    :param format: the format string to print events with
+    :type format: str
     """
-    cal = icalendar.Calendar.from_ical(ics)
-    events = [item for item in cal.walk() if item.name == 'VEVENT']
-    events_grouped = defaultdict(list)
-    for event in events:
-        events_grouped[event['UID']].append(event)
-
     if format is None:
         format = conf['view']['event_format']
-
-    vevents = list()
-    for uid in events_grouped:
-        vevents.append(sorted(events_grouped[uid], key=sort_key))
+    vevents = utils.split_ics(ics, random_uid)
     for vevent in vevents:
-        import_event(vevent, collection, conf['locale'], batch, random_uid, format, env)
+        import_event(vevent, collection, conf['locale'], batch, format, env)
 
 
-def import_event(vevent, collection, locale, batch, random_uid, format=None, env=None):
-    """import one event into collection, let user choose the collection"""
+def import_event(vevent, collection, locale, batch, format=None, env=None):
+    """import one event into collection, let user choose the collection
 
+    :type vevent: list of vevents, which can be more than one VEVENT, i.e., the
+        same UID, i.e., one "master" event and (optionally) 1+ RECURRENCE-ID events
+    :type vevent: list(str)
+    """
     # print all sub-events
-    for sub_event in vevent:
-        if not batch:
-            event = Event.fromVEvents(
-                [sub_event], calendar=collection.default_calendar_name, locale=locale)
-            echo(event.format(format, datetime.now(), env=env))
+    if not batch:
+        for item in icalendar.Calendar.from_ical(vevent).walk():
+            if item.name == 'VEVENT':
+                event = Event.fromVEvents(
+                    [item], calendar=collection.default_calendar_name, locale=locale)
+                echo(event.format(format, datetime.now(), env=env))
 
     # get the calendar to insert into
     if batch or len(collection.writable_names) == 1:
@@ -563,7 +562,8 @@ def import_event(vevent, collection, locale, batch, random_uid, format=None, env
             ['{}({})'.format(name, num) for num, name in enumerate(calendar_names)])
         while True:
             value = prompt(
-                'Which calendar do you want to import to? \n{}'.format(choices),
+                "Which calendar do you want to import to? (unique prefixes are fine)\n"
+                "{}".format(choices),
                 default=collection.default_calendar_name,
             )
             try:
@@ -577,13 +577,11 @@ def import_event(vevent, collection, locale, batch, random_uid, format=None, env
             echo('invalid choice')
 
     if batch or confirm("Do you want to import this event into `{}`?".format(calendar_name)):
-        ics = utils.ics_from_list(vevent, random_uid)
         try:
-            collection.new(Item(ics.to_ical().decode('utf-8')), collection=calendar_name)
+            collection.new(Item(vevent), collection=calendar_name)
         except DuplicateUid:
-            if batch or confirm(u"An event with the same UID already exists. "
-                                u"Do you want to update it?"):
-                collection.force_update(
-                    Item(ics.to_ical().decode('utf-8')), collection=calendar_name)
+            if batch or confirm(
+                    "An event with the same UID already exists. Do you want to update it?"):
+                collection.force_update(Item(vevent), collection=calendar_name)
             else:
-                logger.warn(u"Not importing event with UID `{}`".format(event.uid))
+                logger.warn("Not importing event with UID `{}`".format(event.uid))

khal/khalendar/utils.py

@@ -1,4 +1,25 @@
-
+# Copyright (c) 2013-2016 Christian Geier et al.
+#
+# Permission is hereby granted, free of charge, to any person obtaining
+# a copy of this software and associated documentation files (the
+# "Software"), to deal in the Software without restriction, including
+# without limitation the rights to use, copy, modify, merge, publish,
+# distribute, sublicense, and/or sell copies of the Software, and to
+# permit persons to whom the Software is furnished to do so, subject to
+# the following conditions:
+#
+# The above copyright notice and this permission notice shall be
+# included in all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+# EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+# MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+# NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+# LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+# OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+# WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+"""collection of utility functions"""
 from datetime import datetime, timedelta
 import calendar
 
@@ -121,8 +142,8 @@ def sanitize(vevent, default_timezone, href='', calendar=''):
     clean up vevents we do not understand
 
     :param vevent: the vevent that needs to be cleaned
-    :type vevent: icalendar.cal.event
-    :param default_timezone: timezone to apply to stard and/or end dates which
+    :type vevent: icalendar.cal.Event
+    :param default_timezone: timezone to apply to start and/or end dates which
          were supposed to be localized but which timezone was not understood
          by icalendar
     :type timezone: pytz.timezone
@@ -133,18 +154,21 @@ def sanitize(vevent, default_timezone, href='', calendar=''):
         problematic
     :type calendar: str
     :returns: clean vevent
-    :rtype: icalendar.cal.event
+    :rtype: icalendar.cal.Event
     """
     # convert localized datetimes with timezone information we don't
     # understand to the default timezone
-    # TODO do this for everything where a TZID can appear (RDATE, EXDATE,
-    # RRULE:UNTIL)
+    # TODO do this for everything where a TZID can appear (RDATE, EXDATE)
     for prop in ['DTSTART', 'DTEND', 'DUE', 'RECURRENCE-ID']:
         if prop in vevent and invalid_timezone(vevent[prop]):
+            timezone = vevent[prop].params.get('TZID')
             value = default_timezone.localize(vevent.pop(prop).dt)
             vevent.add(prop, value)
-            logger.warn('{} has invalid or incomprehensible timezone '
-                        'information in {} in {}'.format(prop, href, calendar))
+            logger.warn(
+                "{} localized in invalid or incomprehensible timezone `{}` in {}/{}. "
+                "This could lead to this event being wrongly displayed."
+                "".format(prop, timezone, calendar, href)
+            )
 
     vdtstart = vevent.pop('DTSTART', None)
     vdtend = vevent.pop('DTEND', None)
@@ -236,7 +260,7 @@ def to_naive_utc(dtime):
 
 
 def invalid_timezone(prop):
-    """check if a icalendar property has a timezone attached we don't understand"""
+    """check if an icalendar property has a timezone attached we don't understand"""
     if hasattr(prop.dt, 'tzinfo') and prop.dt.tzinfo is None and 'TZID' in prop.params:
         return True
     else:

khal/utils.py

@@ -23,6 +23,7 @@
 strings to date(time) or event objects"""
 
 from calendar import isleap
+from collections import defaultdict
 from datetime import date, datetime, timedelta, time
 import random
 import string
@@ -37,12 +38,14 @@
 
 
 def timefstr(dtime_list, timeformat):
-    """converts a time (as a string) to a datetimeobject
+    """converts the first item of a list (a time as a string) to a datetimeobject
 
-    the date is today
+    where the date is today and the time is given by the a string
     removes "used" elements of list
 
-    :returns: datetimeobject
+    :type dtime_list: list(str)
+    :type timeformat: str
+    :rtype: datetime.datetime
     """
     if len(dtime_list) == 0:
         raise ValueError()
@@ -590,19 +593,85 @@ def new_event(locale, dtstart=None, dtend=None, summary=None, timezone=None,
     return event
 
 
-def ics_from_list(vevent, random_uid=False):
-    """convert an iterable of icalendar.Event to an icalendar.Calendar
+def split_ics(ics, random_uid=False):
+    """split an ics string into several according to VEVENT's UIDs
+
+    and sort the right VTIMEZONEs accordingly
+    ignores all other ics components
+    :type ics: str
+    :param random_uid: assign random uids to all events
+    :type random_uid: bool
+    :rtype list:
+    """
+    cal = icalendar.Calendar.from_ical(ics)
+    tzs = {item['TZID']: item for item in cal.walk() if item.name == 'VTIMEZONE'}
+
+    events_grouped = defaultdict(list)
+    for item in cal.walk():
+        if item.name == 'VEVENT':
+            events_grouped[item['UID']].append(item)
+        else:
+            continue
+    return [ics_from_list(events, tzs, random_uid) for uid, events in
+            sorted(events_grouped.items())]
+
+
+def ics_from_list(events, tzs, random_uid=False):
+    """convert an iterable of icalendar.Events to an icalendar.Calendar
 
-    :param random_uid: asign the same random UID to all events
+    :params events: list of events all with the same uid
+    :type events: list(icalendar.cal.Event)
+    :param random_uid: assign random uids to all events
     :type random_uid: bool
+    :param tzs: collection of timezones
+    :type tzs: dict(icalendar.cal.Vtimzone
     """
     calendar = icalendar.Calendar()
     calendar.add('version', '2.0')
     calendar.add('prodid', '-//CALENDARSERVER.ORG//NONSGML Version 1//EN')
+
     if random_uid:
-        new_uid = icalendar.vText(generate_random_uid())
-    for sub_event in vevent:
+        new_uid = generate_random_uid()
+
+    needed_tz, missing_tz = set(), set()
+    for sub_event in events:
         if random_uid:
-            sub_event['uid'] = new_uid
+            sub_event['UID'] = new_uid
+        # icalendar round-trip converts `TZID=a b` to `TZID="a b"` investigate, file bug XXX
+        for prop in ['DTSTART', 'DTEND', 'DUE', 'EXDATE', 'RDATE', 'RECURRENCE-ID', 'DUE']:
+            if isinstance(sub_event.get(prop), list):
+                items = sub_event.get(prop)
+            else:
+                items = [sub_event.get(prop)]
+
+            for item in items:
+                if not (hasattr(item, 'dt') or hasattr(item, 'dts')):
+                    continue
+                # if prop is a list, all items have the same parameters
+                datetime_ = item.dts[0].dt if hasattr(item, 'dts') else item.dt
+
+                if not hasattr(datetime_, 'tzinfo'):
+                    continue
+
+                # check for datetimes' timezones which are not understood by
+                # icalendar
+                if datetime_.tzinfo is None and 'TZID' in item.params and \
+                        item.params['TZID'] not in missing_tz:
+                    logger.warn(
+                        'Cannot find timezone `{}` in .ics file, using default timezone. '
+                        'This can lead to erroneous time shifts'.format(item.params['TZID'])
+                    )
+                    missing_tz.add(item.params['TZID'])
+                elif datetime_.tzinfo != pytz.UTC:
+                    needed_tz.add(datetime_.tzinfo)
+
+    for tzid in needed_tz:
+        if str(tzid) in tzs:
+            calendar.add_component(tzs[str(tzid)])
+        else:
+            logger.warn(
+                'Cannot find timezone `{}` in .ics file, this could be a bug, '
+                'please report this issue at http://github.com/pimutils/khal/.'.format(tzid))
+    for sub_event in events:
         calendar.add_component(sub_event)
-    return calendar
+    return calendar.to_ical().decode('utf-8')