From 65fbcb9b3352874cead863efd28db7dc2ca61a03 Mon Sep 17 00:00:00 2001 From: Marc Durepos Date: Fri, 7 Feb 2025 10:54:15 -0500 Subject: [PATCH] caldav_sync bug fixes: * Fixed an issue where accepting events from an external organizer would send emails to the all event attendees upon synchronization. * Corrected the data model for events where multiple Odoo users are attendees for the same event. Now, only one event is created in Odoo, with all the attendees on the same event. This conforms to Odoo's existing calendar event data model. * Updated the setting of the organizer field (user_id) on the calendar events upon synchronization. Previously, the organizer field was always set to the current user whose events were being synchronized. Now, it is set based on the ORGANIZER parameter of the iCalendar event, if present. If not present, it defaults to the current synchronizing user. In the case that the organizer is external, the user_id field is left blank. --- caldav_sync/__manifest__.py | 2 +- caldav_sync/models/calendar_event.py | 82 +++++++++++++++---- caldav_sync/models/res_users.py | 24 ++++-- caldav_sync/tests/__init__.py | 1 + .../tests/data/test_external_organizer.ics | 14 ++++ caldav_sync/tests/test_calendar.py | 37 ++++++--- caldav_sync/tests/test_external_organizer.py | 55 +++++++++++++ caldav_sync/tests/test_res_users.py | 73 ++++++++++++++--- 8 files changed, 242 insertions(+), 46 deletions(-) create mode 100644 caldav_sync/tests/data/test_external_organizer.ics create mode 100644 caldav_sync/tests/test_external_organizer.py diff --git a/caldav_sync/__manifest__.py b/caldav_sync/__manifest__.py index 3fea042..e8d7afb 100644 --- a/caldav_sync/__manifest__.py +++ b/caldav_sync/__manifest__.py @@ -8,7 +8,7 @@ { "name": "CalDAV Synchronization", - "version": "17.0.0.6.2", + "version": "17.0.0.6.3", "license": "LGPL-3", "category": "Productivity", "summary": "Synchronize Odoo Calendar Events with CalDAV Servers", diff --git a/caldav_sync/models/calendar_event.py b/caldav_sync/models/calendar_event.py index d00b2a8..f1c9c5a 100644 --- a/caldav_sync/models/calendar_event.py +++ b/caldav_sync/models/calendar_event.py @@ -2,7 +2,8 @@ import uuid import icalendar.cal -from odoo import models, api, fields +from odoo import models, api, fields, _ +from odoo.tools.misc import _logger from odoo.addons.calendar.models.calendar_recurrence import MAX_RECURRENT_EVENT import caldav import logging @@ -869,7 +870,23 @@ class CalendarEvent(models.Model): "caldav_uid": str(component.get("uid")), "partner_ids": [(6, 0, attendee_ids.ids)], "partner_id": organizer.id if organizer else user.partner_id.id, - "user_id": user.id, + # For user_id: + # - If there's an organizer with an Odoo user account, use that + # - If there's an organizer but no Odoo account, set to False (external) + # - If no organizer, the current user owns it + "user_id": ( + # For debugging + ( + _logger.info("Looking up user for organizer: %s", organizer) + or _logger.info("Organizer ID: %s", organizer.id) + or self.env["res.users"] + .search([("partner_id", "=", organizer.id)], limit=1) + .id + or False + ) + if organizer + else user.id + ), } return values @@ -886,6 +903,13 @@ class CalendarEvent(models.Model): matching Odoo event belongs to. :return: The res.partner records who are attendees for the event.""" attendee_emails = self._get_ical_attendee_emails(component) + # Add organizer to attendees if present + organizer = component.get("organizer") + if organizer: + organizer_email = _extract_vcal_email(organizer) + if organizer_email not in attendee_emails: + attendee_emails.append(organizer_email) + # Add current user if not already in attendees if current_user_email not in attendee_emails: attendee_emails.append(current_user_email) existing_partners = self.env["res.partner"].search( @@ -896,14 +920,24 @@ class CalendarEvent(models.Model): for email in attendee_emails if email not in [partner.email for partner in existing_partners] ] - added_partners = self.env["res.partner"].create( - [ - { - "name": email, - "email": email, - } - for email in missing_emails - ] + # Create new partners without triggering notifications + added_partners = ( + self.env["res.partner"] + .with_context( + mail_notify_author=False, # Don't notify the author + mail_notify_force_send=False, # Don't force send notifications + tracking_disable=True, # Disable tracking which can trigger notifications + no_reset_password=True, # Don't trigger password reset emails + ) + .create( + [ + { + "name": email, + "email": email, + } + for email in missing_emails + ] + ) ) final_attendees = {} all_partners = existing_partners | added_partners @@ -933,11 +967,29 @@ class CalendarEvent(models.Model): organizer = component.get("organizer") if organizer: - partner = self.env["res.partner"].search( - [("email", "=", _extract_vcal_email(organizer))] - ) - # TODO: prioritize partner with a user if there is one - return partner[0] if partner else partner # partner[0] in case many matches + email = _extract_vcal_email(organizer) + _logger.info("Organizer email: %s", email) + partner = self.env["res.partner"].search([("email", "=", email)], limit=1) + _logger.info("Found partner: %s", partner) + if not partner: + # Create new partner without triggering notifications + partner = ( + self.env["res.partner"] + .with_context( + mail_notify_author=False, + mail_notify_force_send=False, + tracking_disable=True, + no_reset_password=True, + ) + .create( + { + "name": email, + "email": email, + } + ) + ) + _logger.info("Created partner: %s", partner) + return partner else: return self.env["res.partner"] diff --git a/caldav_sync/models/res_users.py b/caldav_sync/models/res_users.py index defefcb..0088848 100644 --- a/caldav_sync/models/res_users.py +++ b/caldav_sync/models/res_users.py @@ -25,14 +25,24 @@ class ResUsers(models.Model): @api.depends("caldav_username", "caldav_password", "caldav_calendar_url") def _compute_is_caldav_enabled(self): - """This is a bit of an odd way of computing the field, but it works since any - failed attempt to get the events from the server should mark the user as - CalDAV being disabled. We just make sure to mute the logger in the case that - an exception is raised because we are just computing the field, not actually - attempting to synchronize anything.""" + """Compute whether CalDAV is enabled for each user by validating their credentials. + We only check if we can connect to the server and access the principal, without + fetching any events to avoid timeouts with large calendars.""" for rec in self: - with mute_logger("odoo.addons.caldav_sync.models.res_users"): - rec._get_caldav_events() + # If any required field is empty, CalDAV is disabled + if not ( + rec.caldav_username and rec.caldav_password and rec.caldav_calendar_url + ): + rec.is_caldav_enabled = False + continue + try: + client = rec._get_caldav_client() + # Just try to access the principal, which is a lightweight operation + client.principal() + rec.is_caldav_enabled = True + except Exception as e: + rec.is_caldav_enabled = False + _logger.error("Failed to validate CalDAV credentials: %s", e) def _get_caldav_client(self): self.ensure_one() diff --git a/caldav_sync/tests/__init__.py b/caldav_sync/tests/__init__.py index 3908f85..4137ec6 100644 --- a/caldav_sync/tests/__init__.py +++ b/caldav_sync/tests/__init__.py @@ -1,2 +1,3 @@ from . import test_res_users from . import test_calendar +from . import test_external_organizer diff --git a/caldav_sync/tests/data/test_external_organizer.ics b/caldav_sync/tests/data/test_external_organizer.ics new file mode 100644 index 0000000..70866ce --- /dev/null +++ b/caldav_sync/tests/data/test_external_organizer.ics @@ -0,0 +1,14 @@ +BEGIN:VCALENDAR +VERSION:2.0 +PRODID:-//Odoo//CalDAV Client//EN +BEGIN:VEVENT +UID:external-organizer-test-123 +DTSTART:20250207T143000Z +DTEND:20250207T153000Z +DTSTAMP:20250207T143000Z +ORGANIZER;CN=External Person:mailto:external.person@otherdomain.com +ATTENDEE;PARTSTAT=ACCEPTED;CN=Test User:mailto:test@example.com +SUMMARY:Meeting with External Organizer +DESCRIPTION:This is a test event with an external organizer +END:VEVENT +END:VCALENDAR diff --git a/caldav_sync/tests/test_calendar.py b/caldav_sync/tests/test_calendar.py index fde0324..5eec38a 100644 --- a/caldav_sync/tests/test_calendar.py +++ b/caldav_sync/tests/test_calendar.py @@ -31,18 +31,20 @@ def _patch_caldav_with_events_from_ics(ics_paths, user, last_modified=None): patch("caldav.Calendar") as MockCalendar, ): mock_client = MockDAVClient.return_value - mock_calendar = MockCalendar.return_value - mock_client.calendar = mock_calendar mock_calendars = {} def calendar_side_effect(url): if url not in mock_calendars: - mock_calendars[url] = MockCalendar() - if url == user.caldav_calendar_url: - return mock_calendars[url] - raise Exception("Calendar does not exist.") + mock_cal = MagicMock() + mock_cal.events = MagicMock(return_value=[]) + mock_cal.event_by_uid = MagicMock() + mock_calendars[url] = mock_cal + return mock_calendars[url] - mock_calendar.side_effect = calendar_side_effect + mock_client.calendar = calendar_side_effect + + # Get or create the mock calendar for this user + mock_calendar = calendar_side_effect(user.caldav_calendar_url) def event_by_uid_side_effect(self, uid): for event in self.events(): @@ -89,11 +91,26 @@ class TestCalendarEvent(TransactionCase, CaldavTestCommon): super().setUpClass() cls.env["res.users"].search([])._compute_is_caldav_enabled() cls.user_1_url = "https://mycaldav.test.com/test1calendar" - cls.user_1 = cls._generate_user("test1", "test1", cls.user_1_url) + cls.user_1 = cls._generate_user( + "test1", + caldav_username="user1", + caldav_password="pass1", + caldav_url=cls.user_1_url, + ) cls.user_2_url = "https://mycaldav.test.com/test2calendar" - cls.user_2 = cls._generate_user("test2", "test2", cls.user_2_url) + cls.user_2 = cls._generate_user( + "test2", + caldav_username="user2", + caldav_password="pass2", + caldav_url=cls.user_2_url, + ) cls.user_3_url = "https://mycaldav.test.com/test3calendar" - cls.user_3 = cls._generate_user("test3", "test3", cls.user_3_url) + cls.user_3 = cls._generate_user( + "test3", + caldav_username="user3", + caldav_password="pass3", + caldav_url=cls.user_3_url, + ) def test_basic_event_from_server_create(self): user = self.user_1 diff --git a/caldav_sync/tests/test_external_organizer.py b/caldav_sync/tests/test_external_organizer.py new file mode 100644 index 0000000..d50236c --- /dev/null +++ b/caldav_sync/tests/test_external_organizer.py @@ -0,0 +1,55 @@ +from odoo.tests import TransactionCase +from unittest.mock import patch, MagicMock +from .common import CaldavTestCommon +from .test_calendar import _get_ics_path, _patch_caldav_with_events_from_ics + + +class TestExternalOrganizer(TransactionCase, CaldavTestCommon): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.user = cls._generate_user( + "test", + caldav_username="test", + caldav_password="test", + caldav_url="https://example.com/calendar", + ) + + def test_external_organizer_event_sync(self): + """Test that events with external organizers are handled correctly.""" + # Setup mock for CalDAV client with our test ICS file + with _patch_caldav_with_events_from_ics( + [_get_ics_path("test_external_organizer.ics")], self.user + ): + # Ensure caldav is enabled + self.user._compute_is_caldav_enabled() + # Sync events from the mock server + self.env["calendar.event"].poll_caldav_server() + + # Find the synced event + event = self.env["calendar.event"].search( + [("caldav_uid", "=", "external-organizer-test-123")] + ) + + # Verify event was created + self.assertTrue(event, "Event should be created") + + # Verify user_id is False for external organizer + self.assertFalse( + event.user_id, + "Event with external organizer should have user_id set to False", + ) + + # Verify the organizer's email is preserved in attendees + external_attendee = event.attendee_ids.filtered( + lambda a: a.email == "external.person@otherdomain.com" + ) + self.assertTrue( + external_attendee, + "External organizer should be present in attendees", + ) + # The external organizer should be in the attendees list + self.assertTrue( + external_attendee, + "External organizer should be present in attendees list", + ) diff --git a/caldav_sync/tests/test_res_users.py b/caldav_sync/tests/test_res_users.py index f00c58d..3c42316 100644 --- a/caldav_sync/tests/test_res_users.py +++ b/caldav_sync/tests/test_res_users.py @@ -1,6 +1,10 @@ from odoo.tests import TransactionCase from unittest.mock import patch, MagicMock from .common import CaldavTestCommon +import caldav +import logging + +_logger = logging.getLogger(__name__) class TestUsers(TransactionCase, CaldavTestCommon): @@ -9,24 +13,67 @@ class TestUsers(TransactionCase, CaldavTestCommon): super().setUpClass() def test_caldav_enabled_false_without_url(self): - user = self._generate_user("test", "test") + # Create user with no CalDAV credentials + user = self._generate_user("test") self.assertFalse(user.is_caldav_enabled) + def test_caldav_enabled_false_without_credentials(self): + """Test that is_caldav_enabled is False when any required field is missing.""" + # Test with missing URL - has username and password only + user1 = self._generate_user( + "test1", caldav_username="user1", caldav_password="pass1" + ) + self.assertFalse(user1.is_caldav_enabled) + + # Test with missing username - has password and URL only + user2 = self._generate_user( + "test2", caldav_password="pass2", caldav_url="https://example.com" + ) + self.assertFalse(user2.is_caldav_enabled) + + # Test with missing password - has username and URL only + user3 = self._generate_user( + "test3", caldav_username="user3", caldav_url="https://example.com" + ) + self.assertFalse(user3.is_caldav_enabled) + @patch("caldav.DAVClient") - def test_caldav_connection_succeeds_but_not_calendar(self, MockDAVClient): - user = self._generate_user("test", "test", "https://example.com/abc123") - # Create a mock client and mock calendar + def test_caldav_enabled_success(self, MockDAVClient): + """Test that is_caldav_enabled is True when connection succeeds.""" + # Create user with name 'test' and set CalDAV credentials + user = self._generate_user( + "test", + caldav_username="user", + caldav_password="pass", + caldav_url="https://example.com/abc123", + ) + + # Mock successful connection mock_client = MockDAVClient.return_value - mock_calendar = MagicMock() - mock_client.calendar.return_value = mock_calendar + mock_principal = MagicMock() + mock_client.principal.return_value = mock_principal - # Mock the events method to raise an exception - mock_calendar.events.side_effect = Exception("Failed to get events") + # Compute should succeed and set is_caldav_enabled to True + user._compute_is_caldav_enabled() + self.assertTrue(user.is_caldav_enabled) - # An exception should not be raised because we need to continue to sync other - # users' calendars. Instead the exception message is simply logged to error. + @patch("caldav.DAVClient") + def test_caldav_enabled_connection_fails(self, MockDAVClient): + """Test that is_caldav_enabled is False when connection fails.""" + user = self._generate_user( + "test", + caldav_username="user", + caldav_password="pass", + caldav_url="https://example.com/abc123", + ) + + # Mock failed connection + mock_client = MockDAVClient.return_value + mock_client.principal.side_effect = caldav.error.AuthorizationError( + "Invalid credentials" + ) + + # Should handle the error gracefully and set is_caldav_enabled to False with self.assertLogs("odoo.addons.caldav_sync.models.res_users", "ERROR"): - user._get_caldav_events() - - # Ensure the is_caldav_enabled is set to False + user._compute_is_caldav_enabled() self.assertFalse(user.is_caldav_enabled)