From a0d4c621080fb24426dd76ff92bcdb192ba77683 Mon Sep 17 00:00:00 2001 From: "Gabriel de Paula Felix (gdpf)" Date: Thu, 5 Oct 2023 09:34:27 -0300 Subject: [PATCH] [FIX] google_calendar: do not spam while updating recurrent events Before this task, lots of mails were sent after updating or deleting recurrent event in 'All events' or 'This and future events' update type. This was happening because updating these recurrent events was triggering patch calls event by event, when they should be handled in batch. After this commit, updating or deleting recurrent events should trigger at most two mails for Google users. closes odoo/odoo#137607 Task-id: 3163695 X-original-commit: ad2106babda61446bce13283d570dc723418b630 Signed-off-by: Arnaud Joset (arj) Signed-off-by: Gabriel de Paula Felix (gdpf) --- addons/calendar/models/calendar_event.py | 143 ++++++++++++++---- addons/calendar/models/calendar_recurrence.py | 6 +- .../calendar/tests/test_event_recurrence.py | 45 +++++- addons/google_calendar/models/calendar.py | 38 +++++ .../models/calendar_recurrence_rule.py | 4 +- addons/google_calendar/models/google_sync.py | 5 +- .../tests/test_sync_odoo2google.py | 27 ++-- .../google_calendar/utils/google_calendar.py | 6 + 8 files changed, 222 insertions(+), 52 deletions(-) diff --git a/addons/calendar/models/calendar_event.py b/addons/calendar/models/calendar_event.py index 2242b7b9f10..3eaa4f25303 100644 --- a/addons/calendar/models/calendar_event.py +++ b/addons/calendar/models/calendar_event.py @@ -633,17 +633,17 @@ class Meeting(models.Model): # Update this event detached_events |= self._break_recurrence(future=recurrence_update_setting == 'future_events') else: - future_update_start = self.start if recurrence_update_setting == 'future_events' else None + future_edge_case = recurrence_update_setting == 'future_events' and self == self.recurrence_id.base_event_id time_values = {field: values.pop(field) for field in time_fields if field in values} if 'access_token' in values: values.pop('access_token') # prevents copying access_token to other events in recurrency - if recurrence_update_setting == 'all_events': + if recurrence_update_setting == 'all_events' or future_edge_case: # Update all events: we create a new reccurrence and dismiss the existing events self._rewrite_recurrence(values, time_values, recurrence_values) else: - # Update future events - detached_events |= self._split_recurrence(time_values) - self.recurrence_id._write_events(values, dtstart=future_update_start) + # Update future events: trim recurrence, delete remaining events except base event and recreate it + # All the recurrent events processing is done within the following method + self._update_future_events(values, time_values, recurrence_values) else: super().write(values) self._sync_activities(fields=values.keys()) @@ -900,10 +900,10 @@ class Meeting(models.Model): """ self.ensure_one() if recurrence_update_setting == 'all_events': - self.recurrence_id.calendar_event_ids.write({'active': False}) - elif recurrence_update_setting == 'future_events' and self.recurrence_id: + self.recurrence_id.calendar_event_ids.write(self._get_archive_values()) + elif recurrence_update_setting == 'future_events': detached_events = self.recurrence_id._stop_at(self) - detached_events.write({'active': False}) + detached_events.write(self._get_archive_values()) elif recurrence_update_setting == 'self_only': self.write({ 'active': False, @@ -1021,6 +1021,17 @@ class Meeting(models.Model): 'day': event_date.day, } + @api.model + def _get_recurrence_params_by_date(self, event_date): + """ Return the recurrence parameters from a date object. """ + weekday_field_name = weekday_to_field(event_date.weekday()) + return { + weekday_field_name: True, + 'weekday': weekday_field_name.upper(), + 'byday': str(get_weekday_occurence(event_date)), + 'day': event_date.day, + } + def _split_recurrence(self, time_values): """Apply time changes to events and update the recurrence accordingly. @@ -1060,16 +1071,11 @@ class Meeting(models.Model): recurrences_to_unlink.with_context(archive_on_error=True).unlink() return detached_events - self - def _rewrite_recurrence(self, values, time_values, recurrence_values): - """ Recreate the whole recurrence when all recurrent events must be moved - time_values corresponds to date times for one specific event. We need to update the base_event of the recurrence - and reapply the recurrence later. All exceptions are lost. - """ - self.ensure_one() - base_event = self.recurrence_id.base_event_id + def _get_time_update_dict(self, base_event, time_values): + """ Return the update dictionary for shifting the base_event's time to the new date. """ if not base_event: raise UserError(_("You can't update a recurrence without base event.")) - [base_time_values] = self.recurrence_id.base_event_id.read(['start', 'stop', 'allday']) + [base_time_values] = base_event.read(['start', 'stop', 'allday']) update_dict = {} start_update = fields.Datetime.to_datetime(time_values.get('start')) stop_update = fields.Datetime.to_datetime(time_values.get('stop')) @@ -1090,32 +1096,103 @@ class Meeting(models.Model): stop = base_time_values['stop'] + (stop_update - self.stop) stop_date = base_time_values['stop'].date() + (stop_update.date() - self.stop.date()) update_dict.update({'stop': stop, 'stop_date': stop_date}) + return update_dict + @api.model + def _get_archive_values(self): + """ Return parameters for archiving events in calendar module. """ + return {'active': False} + + @api.model + def _check_values_to_sync(self, values): + """ Method to be overriden: return candidate values to be synced within rewrite_recurrence function scope. """ + return False + + @api.model + def _get_update_future_events_values(self): + """ Return parameters for updating future events within _update_future_events function scope. """ + return {} + + @api.model + def _get_remove_sync_id_values(self): + """ Return parameters for removing event synchronization id within _update_future_events function scope. """ + return {} + + def _get_updated_recurrence_values(self, new_start_date): + """ Copy values from current recurrence and update the start date weekday. """ + [previous_recurrence_values] = self.recurrence_id.copy_data() + if self.start.weekday() != new_start_date.weekday(): + previous_recurrence_values.pop(weekday_to_field(self.start.weekday()), None) + return previous_recurrence_values + + def _update_future_events(self, values, time_values, recurrence_values): + """ + Trim the current recurrence detaching the occurrences after current event, + deactivate the detached events except for the updated event and apply recurrence values. + """ + self.ensure_one() + update_dict = self._get_time_update_dict(self, time_values) time_values.update(update_dict) - if time_values or recurrence_values: - rec_fields = list(self._get_recurrent_fields()) - [rec_vals] = base_event.read(rec_fields) - old_recurrence_values = {field: rec_vals.pop(field) for field in rec_fields if - field in rec_vals} - base_event.write({**values, **time_values}) - # Delete all events except the base event and the currently modified - expandable_events = self.recurrence_id.calendar_event_ids - (self.recurrence_id.base_event_id + self) - self.recurrence_id.with_context(archive_on_error=True).unlink() - expandable_events.with_context(archive_on_error=True).unlink() - # Make sure to recreate a new recurrence. Needed to prevent sync issues - base_event.recurrence_id = False - # Recreate all events and the recurrence: override updated values + # Get base values from the previous recurrence and update the start date weekday field. + start_date = time_values['start'].date() if 'start' in time_values else self.start.date() + previous_recurrence_values = self._get_updated_recurrence_values(start_date) + + # Trim previous recurrence at current event, deleting following events except for the updated event. + detached_events_split = self.recurrence_id._stop_at(self) + (detached_events_split - self).write({'active': False, **self._get_remove_sync_id_values()}) + + # Update the current event with the new recurrence information. + if values or time_values: + self.write({ + **time_values, **values, + **self._get_remove_sync_id_values(), + **self._get_update_future_events_values() + }) + + # Combine parameters from previous recurrence with the new recurrence parameters. + new_values = { + **previous_recurrence_values, + **self._get_recurrence_params_by_date(start_date), + **recurrence_values, + 'count': recurrence_values.get('count', 0) or len(detached_events_split) + } + new_values.pop('rrule', None) + + # Generate the new recurrence by patching the updated event and return an empty list. + self._apply_recurrence_values(new_values) + + def _rewrite_recurrence(self, values, time_values, recurrence_values): + """ Delete the current recurrence, reactivate base event and apply updated recurrence values. """ + self.ensure_one() + base_event = self.recurrence_id.base_event_id + update_dict = self._get_time_update_dict(base_event, time_values) + time_values.update(update_dict) + + if self._check_values_to_sync(values) or time_values or recurrence_values: + # Get base values from the previous recurrence and update the start date weekday field. + start_date = time_values['start'].date() if 'start' in time_values else self.start.date() + old_recurrence_values = self._get_updated_recurrence_values(start_date) + + # Archive all events and delete recurrence, reactivate base event and apply updated values. + base_event.action_mass_archive("all_events") + base_event.recurrence_id.unlink() + base_event.write({ + 'active': True, + 'recurrence_id': False, + **values, **time_values + }) + + # Combine parameters from previous recurrence with the new recurrence parameters. new_values = { **old_recurrence_values, **base_event._get_recurrence_params(), **recurrence_values, } - new_values.pop('rrule') + new_values.pop('rrule', None) + + # Patch base event with updated recurrence parameters: this will recreate the recurrence. detached_events = base_event._apply_recurrence_values(new_values) detached_events.write({'active': False}) - # archive the current event if all the events were recreated - if self != self.recurrence_id.base_event_id and time_values: - self.active = False else: # Write on all events. Carefull, it could trigger a lot of noise to Google/Microsoft... self.recurrence_id._write_events(values) diff --git a/addons/calendar/models/calendar_recurrence.py b/addons/calendar/models/calendar_recurrence.py index 550dcf51adb..06fb142e2f8 100644 --- a/addons/calendar/models/calendar_recurrence.py +++ b/addons/calendar/models/calendar_recurrence.py @@ -176,13 +176,15 @@ class RecurrenceRule(models.Model): 'mon', 'tue', 'wed', 'thu', 'fri', 'sat', 'sun', 'day', 'weekday') def _compute_rrule(self): for recurrence in self: - recurrence.rrule = recurrence._rrule_serialize() + current_rule = recurrence._rrule_serialize() + if recurrence.rrule != current_rule: + recurrence.write({'rrule': current_rule}) def _inverse_rrule(self): for recurrence in self: if recurrence.rrule: values = self._rrule_parse(recurrence.rrule, recurrence.dtstart) - recurrence.write(values) + recurrence.with_context(dont_notify=True).write(values) def _reconcile_events(self, ranges): """ diff --git a/addons/calendar/tests/test_event_recurrence.py b/addons/calendar/tests/test_event_recurrence.py index 5d13e03cc7d..689c46ae1a8 100644 --- a/addons/calendar/tests/test_event_recurrence.py +++ b/addons/calendar/tests/test_event_recurrence.py @@ -554,7 +554,7 @@ class TestUpdateRecurrentEvents(TestRecurrentEvents): (datetime(2019, 11, 2, 1, 0), datetime(2019, 11, 4, 18, 0)), (datetime(2019, 11, 9, 1, 0), datetime(2019, 11, 11, 18, 0)) ]) - self.assertFalse(outlier.exists(), 'The outlier should have been deleted') + self.assertTrue(outlier.exists(), 'The outlier should have its date and time updated according to the change.') def test_update_recurrence_future(self): event = self.events[1] @@ -576,12 +576,38 @@ class TestUpdateRecurrentEvents(TestRecurrentEvents): events = event.recurrence_id.calendar_event_ids.sorted('start') self.assertEqual(events[0], self.events[1], "Events on Tuesdays should not have changed") - self.assertEqual(events[2], self.events[2], "Events on Tuesdays should not have changed") + self.assertEqual(events[2].start, self.events[2].start, "Events on Tuesdays should not have changed") self.assertNotEqual(events.recurrence_id, self.recurrence, "Events should no longer be linked to the original recurrence") self.assertEqual(events.recurrence_id.count, 4, "The new recurrence should have 4") self.assertTrue(event.recurrence_id.tue) self.assertTrue(event.recurrence_id.fri) + def test_update_name_future(self): + # update regular event (not the base event) + old_events = self.events[1:] + old_events[0].write({ + 'name': 'New name', + 'recurrence_update': 'future_events', + 'rrule_type': 'daily', + 'count': 5, + }) + new_recurrence = self.env['calendar.recurrence'].search([('id', '>', self.events[0].recurrence_id.id)]) + self.assertTrue(self.events[0].recurrence_id.exists()) + self.assertEqual(new_recurrence.count, 5) + self.assertFalse(any(old_event.active for old_event in old_events - old_events[0])) + for event in new_recurrence.calendar_event_ids: + self.assertEqual(event.name, 'New name') + + # update the base event + new_events = new_recurrence.calendar_event_ids.sorted('start') + new_events[0].write({ + 'name': 'Old name', + 'recurrence_update': 'future_events' + }) + self.assertTrue(new_recurrence.exists()) + for event in new_recurrence.calendar_event_ids: + self.assertEqual(event.name, 'Old name') + def test_update_recurrence_all(self): self.events[1].write({ 'recurrence_update': 'all_events', @@ -672,6 +698,21 @@ class TestUpdateRecurrentEvents(TestRecurrentEvents): (datetime(2019, 11, 9, 8, 0), datetime(2019, 11, 12, 18, 0)), ]) + def test_update_name_all(self): + old_recurrence = self.events[0].recurrence_id + old_events = old_recurrence.calendar_event_ids - self.events[0] + self.events[0].write({ + 'name': 'New name', + 'recurrence_update': 'all_events', + 'count': '5' + }) + new_recurrence = self.env['calendar.recurrence'].search([('id', '>', old_recurrence.id)]) + self.assertFalse(old_recurrence.exists()) + self.assertEqual(new_recurrence.count, 5) + self.assertFalse(any(old_event.active for old_event in old_events)) + for event in new_recurrence.calendar_event_ids: + self.assertEqual(event.name, 'New name') + def test_archive_recurrence_all(self): self.events[1].action_mass_archive('all_events') self.assertEqual([False, False, False], self.events.mapped('active')) diff --git a/addons/google_calendar/models/calendar.py b/addons/google_calendar/models/calendar.py index ed591740852..66ec475c789 100644 --- a/addons/google_calendar/models/calendar.py +++ b/addons/google_calendar/models/calendar.py @@ -9,6 +9,7 @@ from uuid import uuid4 from odoo import api, fields, models, tools, _ from odoo.exceptions import ValidationError +from odoo.addons.google_calendar.utils.google_calendar import GoogleCalendarService class Meeting(models.Model): _name = 'calendar.event' @@ -60,6 +61,31 @@ class Meeting(models.Model): for vals in vals_list ]) + @api.model + def _check_values_to_sync(self, values): + """ Return True if values being updated intersects with Google synced values and False otherwise. """ + synced_fields = self._get_google_synced_fields() + values_to_sync = any(key in synced_fields for key in values) + return values_to_sync + + @api.model + def _get_update_future_events_values(self): + """ Add parameters for updating events within the _update_future_events function scope. """ + update_future_events_values = super()._get_update_future_events_values() + return {**update_future_events_values, 'need_sync': False} + + @api.model + def _get_remove_sync_id_values(self): + """ Add parameters for removing event synchronization while updating the events in super class. """ + remove_sync_id_values = super()._get_remove_sync_id_values() + return {**remove_sync_id_values, 'google_id': False} + + @api.model + def _get_archive_values(self): + """ Return the parameters for archiving events. Do not synchronize events after archiving. """ + archive_values = super()._get_archive_values() + return {**archive_values, 'need_sync': False} + def write(self, values): recurrence_update_setting = values.get('recurrence_update') if recurrence_update_setting in ('all_events', 'future_events') and len(self) == 1: @@ -233,6 +259,18 @@ class Meeting(models.Model): commands += [(0, 0, {'duration': duration, 'interval': interval, 'name': name, 'alarm_type': alarm_type})] return commands + def action_mass_archive(self, recurrence_update_setting): + """ Delete recurrence in Odoo if in 'all_events' or in 'future_events' edge case, triggering one mail. """ + self.ensure_one() + google_service = GoogleCalendarService(self.env['google.service']) + archive_future_events = recurrence_update_setting == 'future_events' and self == self.recurrence_id.base_event_id + if recurrence_update_setting == 'all_events' or archive_future_events: + self.recurrence_id.with_context(is_recurrence=True)._google_delete(google_service, self.recurrence_id.google_id) + # Increase performance handling 'future_events' edge case as it was an 'all_events' update. + if archive_future_events: + recurrence_update_setting = 'all_events' + super(Meeting, self).action_mass_archive(recurrence_update_setting) + def _google_values(self): if self.allday: start = {'date': self.start_date.isoformat()} diff --git a/addons/google_calendar/models/calendar_recurrence_rule.py b/addons/google_calendar/models/calendar_recurrence_rule.py index 69ccf6aa827..e8e49bf6c54 100644 --- a/addons/google_calendar/models/calendar_recurrence_rule.py +++ b/addons/google_calendar/models/calendar_recurrence_rule.py @@ -64,8 +64,8 @@ class RecurrenceRule(models.Model): def _write_events(self, values, dtstart=None): values.pop('google_id', False) - # If only some events are updated, sync those events. - values['need_sync'] = bool(dtstart) + # Events will be updated by patch requests, do not sync events for avoiding spam. + values['need_sync'] = False return super()._write_events(values, dtstart=dtstart) def _cancel(self): diff --git a/addons/google_calendar/models/google_sync.py b/addons/google_calendar/models/google_sync.py index 7978a17f4e0..c60c78f13e1 100644 --- a/addons/google_calendar/models/google_sync.py +++ b/addons/google_calendar/models/google_sync.py @@ -242,6 +242,8 @@ class GoogleSync(models.AbstractModel): def _google_delete(self, google_service: GoogleCalendarService, google_id, timeout=TIMEOUT): with google_calendar_token(self.env.user.sudo()) as token: if token: + is_recurrence = self._context.get('is_recurrence', False) + google_service.google_service = google_service.google_service.with_context(is_recurrence=is_recurrence) google_service.delete(google_id, token=token, timeout=timeout) # When the record has been deleted on our side, we need to delete it on google but we don't want # to raise an error because the record don't exists anymore. @@ -256,7 +258,8 @@ class GoogleSync(models.AbstractModel): except HTTPError as e: if e.response.status_code in (400, 403): self._google_error_handling(e) - self.exists().with_context(dont_notify=True).need_sync = False + if values: + self.exists().with_context(dont_notify=True).need_sync = False @after_commit def _google_insert(self, google_service: GoogleCalendarService, values, timeout=TIMEOUT): diff --git a/addons/google_calendar/tests/test_sync_odoo2google.py b/addons/google_calendar/tests/test_sync_odoo2google.py index 92d1e1e1316..461b0463a2b 100644 --- a/addons/google_calendar/tests/test_sync_odoo2google.py +++ b/addons/google_calendar/tests/test_sync_odoo2google.py @@ -106,7 +106,7 @@ class TestSyncOdoo2Google(TestSyncGoogle): }) partner_model = self.env.ref('base.model_res_partner') partner = self.env['res.partner'].search([], limit=1) - with self.assertQueryCount(__system__=84): + with self.assertQueryCount(__system__=86): event = self.env['calendar.event'].create({ 'name': "Event", 'start': datetime(2020, 1, 15, 8, 0), @@ -326,8 +326,8 @@ class TestSyncOdoo2Google(TestSyncGoogle): 'name': 'New name', 'recurrence_update': 'future_events', }) - self.assertGoogleEventPatched(event.google_id, { - 'id': event.google_id, + self.assertGoogleEventInserted({ + 'id': False, 'start': {'date': str(event.start_date)}, 'end': {'date': str(event.stop_date + relativedelta(days=1))}, 'summary': 'New name', @@ -336,9 +336,10 @@ class TestSyncOdoo2Google(TestSyncGoogle): 'guestsCanModify': True, 'organizer': {'email': 'odoobot@example.com', 'self': True}, 'attendees': [{'email': 'odoobot@example.com', 'responseStatus': 'accepted'}], - 'extendedProperties': {'shared': {'%s_odoo_id' % self.env.cr.dbname: event.id}}, + 'extendedProperties': {'shared': {'%s_odoo_id' % self.env.cr.dbname: event.recurrence_id.id}}, 'reminders': {'overrides': [], 'useDefault': False}, 'visibility': 'public', + 'recurrence': ['RRULE:FREQ=WEEKLY;WKST=SU;COUNT=1;BYDAY=WE'] }, timeout=3) @patch_api @@ -412,8 +413,9 @@ class TestSyncOdoo2Google(TestSyncGoogle): 'name': 'New name', 'recurrence_update': 'all_events', }) - self.assertGoogleEventPatched(recurrence.google_id, { - 'id': recurrence.google_id, + new_recurrence = self.env['calendar.recurrence'].search([('id', '>', recurrence.id)]) + self.assertGoogleEventInserted({ + 'id': False, 'start': {'date': str(event.start_date)}, 'end': {'date': str(event.stop_date + relativedelta(days=1))}, 'summary': 'New name', @@ -422,8 +424,8 @@ class TestSyncOdoo2Google(TestSyncGoogle): 'guestsCanModify': True, 'organizer': {'email': 'odoobot@example.com', 'self': True}, 'attendees': [{'email': 'odoobot@example.com', 'responseStatus': 'accepted'}], - 'recurrence': ['RRULE:FREQ=WEEKLY;COUNT=2;BYDAY=WE'], - 'extendedProperties': {'shared': {'%s_odoo_id' % self.env.cr.dbname: recurrence.id}}, + 'recurrence': ['RRULE:FREQ=WEEKLY;WKST=SU;COUNT=2;BYDAY=WE'], + 'extendedProperties': {'shared': {'%s_odoo_id' % self.env.cr.dbname: new_recurrence.id}}, 'reminders': {'overrides': [], 'useDefault': False}, 'visibility': 'public', }, timeout=3) @@ -578,8 +580,9 @@ class TestSyncOdoo2Google(TestSyncGoogle): 'name': 'New name', 'recurrence_update': 'all_events', }) - self.assertGoogleEventPatched(recurrence.google_id, { - 'id': recurrence.google_id, + new_recurrence = self.env['calendar.recurrence'].search([('id', '>', recurrence.id)]) + self.assertGoogleEventInserted({ + 'id': False, 'start': {'dateTime': "2020-01-15T08:00:00+00:00", 'timeZone': 'Europe/Brussels'}, 'end': {'dateTime': "2020-01-15T09:00:00+00:00", 'timeZone': 'Europe/Brussels'}, 'summary': 'New name', @@ -588,8 +591,8 @@ class TestSyncOdoo2Google(TestSyncGoogle): 'guestsCanModify': True, 'organizer': {'email': 'odoobot@example.com', 'self': True}, 'attendees': [{'email': 'odoobot@example.com', 'responseStatus': 'accepted'}], - 'recurrence': ['RRULE:FREQ=WEEKLY;COUNT=2;BYDAY=WE'], - 'extendedProperties': {'shared': {'%s_odoo_id' % self.env.cr.dbname: recurrence.id}}, + 'recurrence': ['RRULE:FREQ=WEEKLY;WKST=SU;COUNT=2;BYDAY=WE'], + 'extendedProperties': {'shared': {'%s_odoo_id' % self.env.cr.dbname: new_recurrence.id}}, 'reminders': {'overrides': [], 'useDefault': False}, 'visibility': 'public', }, timeout=3) diff --git a/addons/google_calendar/utils/google_calendar.py b/addons/google_calendar/utils/google_calendar.py index d063de6806a..566ad153256 100644 --- a/addons/google_calendar/utils/google_calendar.py +++ b/addons/google_calendar/utils/google_calendar.py @@ -92,6 +92,12 @@ class GoogleCalendarService(): url = "/calendar/v3/calendars/primary/events/%s?sendUpdates=all" % event_id headers = {'Content-type': 'application/json'} params = {'access_token': token} + # Delete all events from recurrence in a single request to Google and triggering a single mail. + # The 'singleEvents' parameter is a trick that tells Google API to delete all recurrent events individually, + # making the deletion be handled entirely on their side, and then we archive the events in Odoo. + is_recurrence = self.google_service._context.get('is_recurrence', True) + if is_recurrence: + params['singleEvents'] = 'true' try: self.google_service._do_request(url, params, headers=headers, method='DELETE', timeout=timeout) except requests.HTTPError as e: