Save deleted events as ‘cancelled’ instead of deleting them.

Summary:
D1371 changed the way we stored deleted events — instead of deleting them we save them as « cancelled ». This diff updates the google event sync code to do the same.

What I changed:
- removed the event deletion code from remote_sync.py
- split the `SyncResponse` named tuple into `SyncResponse` and `CalendarResponse`  --- the reasoning being that we want to treat event deletes as regular updates, but we still need to distinguish calendar deletes from updates.

Test Plan: Tested manually.

Reviewers: jennie, emfree

Reviewed By: emfree

Projects: #calendars

Differential Revision: https://review.inboxapp.com/D1401

inbox/events/google.py

@@ -1,5 +1,4 @@
 """Provide Google Calendar events."""
-import collections
 import datetime
 import json
 import urllib
@@ -13,7 +12,7 @@
 from inbox.models.session import session_scope
 from inbox.models.backends.oauth import token_manager
 from inbox.events.util import (google_to_event_time, parse_google_time,
-                               parse_datetime)
+                               parse_datetime, CalendarSyncResponse)
 
 
 log = get_logger()
@@ -22,14 +21,6 @@
               'declined': 'no', 'tentative': 'maybe'}
 
 
-# Container for a parsed API response. API calls return adds/updates/deletes
-# all together, but we want to handle deletions separately in our persistence
-# logic. deleted_uids should be a list of uids, and updated_objects should be a
-# list of (un-added, uncommitted) model instances.
-SyncResponse = collections.namedtuple('SyncResponse',
-                                      ['deleted_uids', 'updated_objects'])
-
-
 class GoogleEventsProvider(object):
     """
     A utility class to fetch and parse Google calendar data for the
@@ -45,7 +36,7 @@ def sync_calendars(self):
         """ Fetches data for the user's calendars.
         Returns
         -------
-        SyncResponse
+        CalendarSyncResponse
         """
 
         deletes = []
@@ -57,7 +48,7 @@ def sync_calendars(self):
             else:
                 updates.append(parse_calendar_response(item))
 
-        return SyncResponse(deletes, updates)
+        return CalendarSyncResponse(deletes, updates)
 
     def sync_events(self, calendar_uid, sync_from_time=None):
         """ Fetches event data for an individual calendar.
@@ -75,19 +66,14 @@ def sync_events(self, calendar_uid, sync_from_time=None):
 
         Returns
         -------
-        SyncResponse
+        A list of uncommited Event instances.
         """
-        deletes = []
         updates = []
         items = self._get_raw_events(calendar_uid, sync_from_time)
         for item in items:
-            # We need to instantiate recurring event cancellations as overrides
-            if item.get('status') == 'cancelled' and not \
-                    item.get('recurringEventId'):
-                deletes.append(item['id'])
-            else:
-                updates.append(parse_event_response(item))
-        return SyncResponse(deletes, updates)
+            updates.append(parse_event_response(item))
+
+        return updates
 
     def _get_raw_calendars(self):
         """Gets raw data for the user's calendars."""

inbox/events/remote_sync.py

@@ -65,11 +65,10 @@ def sync(self):
             db_session.commit()
 
         for (uid, id_) in calendar_uids_and_ids:
-            deleted_uids, event_changes = self.provider.sync_events(
+            event_changes = self.provider.sync_events(
                 uid, sync_from_time=last_sync)
+
             with session_scope() as db_session:
-                handle_event_deletes(self.namespace_id, id_, deleted_uids,
-                                     self.log, db_session)
                 handle_event_updates(self.namespace_id, id_, event_changes,
                                      self.log, db_session)
                 db_session.commit()
@@ -126,27 +125,6 @@ def handle_calendar_updates(namespace_id, calendars, log, db_session):
     return ids_
 
 
-def handle_event_deletes(namespace_id, calendar_id, deleted_event_uids,
-                         log, db_session):
-    """Deletes any local Event rows with the given calendar_id and uid in
-    `deleted_event_uids`."""
-    deleted_count = 0
-    for uid in deleted_event_uids:
-        local_event = db_session.query(Event).filter(
-            Event.namespace_id == namespace_id,
-            Event.uid == uid,
-            Event.calendar_id == calendar_id).first()
-        if local_event is not None:
-            # Delete stored overrides for this event too, if it is recurring
-            if isinstance(local_event, RecurringEvent):
-                deleted_event_uids.extend(list(local_event.override_uids))
-            deleted_count += 1
-            db_session.delete(local_event)
-    log.info('synced deleted events',
-             calendar_id=calendar_id,
-             deleted=deleted_count)
-
-
 def handle_event_updates(namespace_id, calendar_id, events, log, db_session):
     """Persists new or updated Event objects to the database."""
     added_count = 0
@@ -163,6 +141,14 @@ def handle_event_updates(namespace_id, calendar_id, events, log, db_session):
             Event.uid == event.uid).first()
 
         if local_event is not None:
+            # We also need to mark all overrides as cancelled if we're
+            # cancelling a recurring event.
+            if isinstance(event, RecurringEvent) and \
+                    event.status == 'cancelled' and \
+                    local_event.status != 'cancelled':
+                for override in local_event.overrides:
+                    override.status = 'cancelled'
+
             local_event.update(event)
             updated_count += 1
         else:

inbox/events/util.py

@@ -63,3 +63,12 @@ def google_to_event_time(start_raw, end_raw):
     event_time = when_to_event_time(d)
 
     return event_time
+
+
+# Container for a parsed API response. API calls return adds/updates/deletes
+# all together, but we want to handle deletions separately in our persistence
+# logic. deleted_uids should be a list of uids, and updated_objects should be a
+# list of (un-added, uncommitted) model instances.
+CalendarSyncResponse = namedtuple('CalendarSyncResponse',
+                                  ['deleted_uids',
+                                   'updated_objects'])

tests/events/test_google_events.py

@@ -217,8 +217,18 @@ def test_event_parsing():
     provider = GoogleEventsProvider(1, 1)
     provider._get_raw_events = mock.MagicMock(
         return_value=raw_response)
-    deletes, updates = provider.sync_events('uid', 1)
-    assert deletes == expected_deletes
+    updates = provider.sync_events('uid', 1)
+
+    # deleted events are actually only marked as
+    # cancelled. Look for them in the updates stream.
+    found_cancelled_event = False
+    for event in updates:
+        if event.uid in expected_deletes and event.status == 'cancelled':
+            found_cancelled_event = True
+            break
+
+    assert found_cancelled_event
+
     for obtained, expected in zip(updates, expected_updates):
         print obtained, expected
         assert cmp_event_attrs(obtained, expected)
@@ -533,6 +543,5 @@ def test_cancelled_override_creation():
     provider = GoogleEventsProvider(1, 1)
     provider._get_raw_events = mock.MagicMock(
         return_value=raw_response)
-    deletes, updates = provider.sync_events('uid', 1)
-    assert len(deletes) == 0
+    updates = provider.sync_events('uid', 1)
     assert updates[0].cancelled is True

tests/events/test_recurrence.py

@@ -5,7 +5,7 @@
 from datetime import timedelta
 from inbox.models.event import Event, RecurringEvent, RecurringEventOverride
 from inbox.models.when import Date, Time, DateSpan, TimeSpan
-from inbox.events.remote_sync import handle_event_updates, handle_event_deletes
+from inbox.events.remote_sync import handle_event_updates
 from inbox.events.recurring import (link_events, get_start_times,
                                     parse_exdate, rrule_to_json)
 
@@ -22,10 +22,13 @@
 def recurring_event(db, account, calendar, rrule,
                     start=arrow.get(2014, 8, 7, 20, 30, 00),
                     end=arrow.get(2014, 8, 7, 21, 30, 00),
-                    all_day=False):
-    ev = db.session.query(Event).filter_by(uid='myuid').first()
-    if ev:
-        db.session.delete(ev)
+                    all_day=False, commit=True):
+
+    # commit: are we returning a commited instance object?
+    if commit:
+        ev = db.session.query(Event).filter_by(uid='myuid').first()
+        if ev:
+            db.session.delete(ev)
     ev = Event(namespace_id=account.namespace.id,
                calendar=calendar,
                title='recurring',
@@ -47,8 +50,11 @@ def recurring_event(db, account, calendar, rrule,
                original_start_time=None,
                master_event_uid=None,
                source='local')
-    db.session.add(ev)
-    db.session.commit()
+
+    if commit:
+        db.session.add(ev)
+        db.session.commit()
+
     return ev
 
 
@@ -434,25 +440,6 @@ def test_new_instance_cancelled(db, default_account, calendar):
     assert find_override.cancelled is True
 
 
-def test_master_cancelled(db, default_account, calendar):
-    # Test that when the master recurring event is cancelled, we delete it
-    # in its entirety, including all overrides.
-    event = recurring_event(db, default_account, calendar, TEST_EXDATE_RULE)
-    override = recurring_override(db, event,
-                                  arrow.get(2014, 9, 4, 20, 30, 00),
-                                  arrow.get(2014, 9, 4, 21, 30, 00),
-                                  arrow.get(2014, 9, 4, 22, 30, 00))
-    deletes = [event.uid]
-    handle_event_deletes(default_account.namespace.id,
-                         calendar.id,
-                         deletes, log, db.session)
-    db.session.commit()
-    find_master = db.session.query(Event).filter_by(uid=event.uid).first()
-    assert find_master is None
-    find_override = db.session.query(Event).filter_by(uid=override.uid).first()
-    assert find_override is None
-
-
 def test_when_delta():
     # Test that the event length is calculated correctly
     ev = Event(namespace_id=0)
@@ -501,3 +488,28 @@ def test_rrule_to_json():
     j = rrule_to_json(r)
     assert j.get('until') is None
     assert j.get('byminute') is 42
+
+
+def test_master_cancelled(db, default_account, calendar):
+    # Test that when the master recurring event is cancelled, we cancel every
+    # override too.
+    event = recurring_event(db, default_account, calendar, TEST_EXDATE_RULE)
+    override = recurring_override(db, event,
+                                  arrow.get(2014, 9, 4, 20, 30, 00),
+                                  arrow.get(2014, 9, 4, 21, 30, 00),
+                                  arrow.get(2014, 9, 4, 22, 30, 00))
+
+    update = recurring_event(db, default_account, calendar, TEST_EXDATE_RULE,
+                             commit=False)
+    update.status = 'cancelled'
+    updates = [update]
+
+    handle_event_updates(default_account.namespace.id,
+                         calendar.id,
+                         updates, log, db.session)
+    db.session.commit()
+    find_master = db.session.query(Event).filter_by(uid=event.uid).first()
+    assert find_master.status == 'cancelled'
+
+    find_override = db.session.query(Event).filter_by(uid=override.uid).first()
+    assert find_override.status == 'cancelled'

tests/events/test_sync.py

@@ -1,5 +1,6 @@
 from datetime import datetime
 from inbox.events.remote_sync import EventSync
+from inbox.events.util import CalendarSyncResponse
 from inbox.models import Calendar, Event, Transaction
 from tests.util.base import new_account
 
@@ -19,7 +20,7 @@
 
 
 def calendar_response():
-    return ([], [
+    return CalendarSyncResponse([], [
         Calendar(name='Important Meetings',
                  uid='first_calendar_uid',
                  read_only=False),
@@ -30,9 +31,10 @@ def calendar_response():
 
 
 def calendar_response_with_update():
-    return ([], [Calendar(name='Super Important Meetings',
-                          uid='first_calendar_uid',
-                          read_only=False)])
+    return CalendarSyncResponse(
+                [], [Calendar(name='Super Important Meetings',
+                     uid='first_calendar_uid',
+                     read_only=False)])
 
 
 def calendar_response_with_delete():
@@ -41,7 +43,7 @@ def calendar_response_with_delete():
 
 def event_response(calendar_uid, sync_from_time):
     if calendar_uid == 'first_calendar_uid':
-        return ([], [
+        return [
             Event(uid='first_event_uid',
                   title='Plotting Meeting',
                   **default_params),
@@ -51,28 +53,29 @@ def event_response(calendar_uid, sync_from_time):
             Event(uid='third_event_uid',
                   title='Innocent Meeting',
                   **default_params)
-        ])
+        ]
     else:
-        return ([], [
+        return [
             Event(uid='second_event_uid',
                   title='Plotting Meeting',
                   **default_params),
             Event(uid='third_event_uid',
                   title='Scheming meeting',
                   **default_params)
-        ])
+        ]
 
 
 def event_response_with_update(calendar_uid, sync_from_time):
     if calendar_uid == 'first_calendar_uid':
-        return ([], [Event(uid='first_event_uid',
-                           title='Top Secret Plotting Meeting',
-                           **default_params)])
+        return [Event(uid='first_event_uid',
+                      title='Top Secret Plotting Meeting',
+                      **default_params)]
 
 
 def event_response_with_delete(calendar_uid, sync_from_time):
     if calendar_uid == 'first_calendar_uid':
-        return (['first_event_uid'], [])
+        return [Event(uid='first_event_uid', status='cancelled',
+                      **default_params)]
 
 
 def test_handle_changes(db, new_account):
@@ -139,7 +142,9 @@ def test_handle_changes(db, new_account):
         Event.namespace_id == namespace_id,
         Event.calendar_id == first_calendar.id,
         Event.uid == 'first_event_uid').first()
-    assert first_event is None
+
+    db.session.refresh(first_event)
+    assert first_event.status == 'cancelled'
 
     # Sync a calendar delete
     event_public_ids = [id_ for id_, in db.session.query(Event.public_id).
@@ -158,7 +163,7 @@ def test_handle_changes(db, new_account):
         Transaction.command == 'delete',
         Transaction.namespace_id == namespace_id,
         Transaction.object_public_id.in_(event_public_ids)).all()
-    assert len(deleted_event_transactions) == 2
+    assert len(deleted_event_transactions) == 3
 
     # Check that events with the same uid but associated to a different
     # calendar still survive.