Merge pull request #168 from openedx/robrap/add-time-and-fix-metadata

feat: updated event signal metadata time

Author: robrap

CHANGELOG.rst

@@ -13,6 +13,13 @@ Change Log
 
 Unreleased
 ----------
+Fixed
+~~~~~
+* Updated time metadata to include UTC timezone. The original implementation used utcnow(), which could give different results if the time were ever interpreted to be local time. See https://docs.python.org/3/library/datetime.html#datetime.datetime.utcnow
+
+Changed
+~~~~~~~
+* Updated send_event with an optional time argument to be used as metadata.
 
 [4.1.1] - 2023-01-23
 ---------------------

openedx_events/data.py

@@ -5,7 +5,7 @@
 pattern.
 """
 import socket
-from datetime import datetime
+from datetime import datetime, timezone
 from uuid import UUID, uuid1
 
 import attr
@@ -14,6 +14,17 @@
 import openedx_events
 
 
+def _ensure_utc_time(_, attribute, value):
+    """
+    Ensure the value is a UTC datetime.
+
+    Note: Meant to be used along-side an instance_of attr validator.
+    """
+    if value.tzinfo and value.tzinfo == timezone.utc:
+        return
+    raise ValueError(f"'{attribute.name}' must have timezone.utc")
+
+
 @attr.s(frozen=True)
 class EventsMetadata:
     """
@@ -28,16 +39,26 @@ class EventsMetadata:
         minorversion (int): version of the event type.
         source (str): logical source of an event.
         sourcehost (str): physical source of the event.
-        time (datetime): timestamp when the event was sent.
-        sourcelib (str): Open edX Events library version.
+        time (datetime): (optional) timestamp when the event was sent with
+            UTC timezone. Defaults to current time in UTC. See OEP-41 for
+            details.
+        sourcelib (tuple of ints): Open edX Events library version. A tuple was
+            selected so that version comparisons don't have to worry about
+            lexical ordering of strings (e.g. '0.9.0' vs. '0.10.0').
     """
 
     id = attr.ib(type=UUID, init=False)
     event_type = attr.ib(type=str)
     minorversion = attr.ib(type=int, converter=attr.converters.default_if_none(0))
     source = attr.ib(type=str, init=False)
     sourcehost = attr.ib(type=str, init=False)
-    time = attr.ib(type=datetime, init=False)
+    current_utc_time = datetime.now(timezone.utc)
+    time = attr.ib(
+        type=datetime,
+        default=None,
+        converter=attr.converters.default_if_none(attr.Factory(lambda: datetime.now(timezone.utc))),
+        validator=attr.validators.optional([attr.validators.instance_of(datetime), _ensure_utc_time]),
+    )
     sourcelib = attr.ib(type=tuple, init=False)
 
     def __attrs_post_init__(self):
@@ -56,7 +77,6 @@ def __attrs_post_init__(self):
             ),
         )
         object.__setattr__(self, "sourcehost", socket.gethostname())
-        object.__setattr__(self, "time", datetime.utcnow())
         object.__setattr__(
             self, "sourcelib", tuple(map(int, openedx_events.__version__.split(".")))
         )

openedx_events/event_bus/tests/test_loader.py

@@ -109,5 +109,6 @@ def test_default_does_nothing(self):
             # Nothing thrown, no warnings.
             assert producer.send(
                 signal=SESSION_LOGIN_COMPLETED, topic='user-logins',
-                event_key_field='user.id', event_data={}, event_metadata=EventsMetadata(event_type='eh', minorversion=0)
+                event_key_field='user.id', event_data={},
+                event_metadata=EventsMetadata(event_type='eh', minorversion=0)
             ) is None

openedx_events/tests/test_tooling.py

@@ -4,8 +4,10 @@
 Classes:
     EventsToolingTest: Test events tooling.
 """
+import datetime
 from contextlib import contextmanager
 from unittest.mock import Mock, patch
+from uuid import UUID
 
 import attr
 import ddt
@@ -85,7 +87,8 @@ def test_get_signal_by_type(self):
     @override_settings(SERVICE_VARIANT="lms")
     @patch("openedx_events.data.openedx_events")
     @patch("openedx_events.data.socket")
-    def test_get_signal_metadata(self, socket_mock, events_package_mock):
+    @patch("openedx_events.data.datetime")
+    def test_generate_signal_metadata(self, datetime_mock, socket_mock, events_package_mock):
         """
         This methods tests getting the generated metadata for an event.
 
@@ -94,23 +97,72 @@ def test_get_signal_metadata(self, socket_mock, events_package_mock):
         """
         events_package_mock.__version__ = "0.1.0"
         socket_mock.gethostname.return_value = "edx.devstack.lms"
+        expected_time = datetime.datetime.now(datetime.timezone.utc)
+        datetime_mock.now.return_value = expected_time
         expected_metadata = {
             "event_type": self.event_type,
             "minorversion": 0,
             "source": "openedx/lms/web",
             "sourcehost": "edx.devstack.lms",
             "sourcelib": [0, 1, 0],
+            "time": expected_time,
         }
 
         metadata = self.public_signal.generate_signal_metadata()
 
         self.assertDictContainsSubset(expected_metadata, attr.asdict(metadata))
+        self.assertIsInstance(metadata.id, UUID)
+
+    @override_settings(SERVICE_VARIANT="lms")
+    @patch("openedx_events.data.openedx_events")
+    @patch("openedx_events.data.socket")
+    def test_generate_signal_metadata_with_valid_time(self, socket_mock, events_package_mock):
+        """
+        Tests getting the generated metadata for an event, providing a valid time in UTC.
+
+        Expected behavior:
+            Returns the metadata containing information about the event.
+        """
+        events_package_mock.__version__ = "0.1.0"
+        socket_mock.gethostname.return_value = "edx.devstack.lms"
+        expected_time = datetime.datetime.now(datetime.timezone.utc)
+        expected_metadata = {
+            "event_type": self.event_type,
+            "minorversion": 0,
+            "source": "openedx/lms/web",
+            "sourcehost": "edx.devstack.lms",
+            "sourcelib": [0, 1, 0],
+            "time": expected_time,
+        }
+
+        metadata = self.public_signal.generate_signal_metadata(time=expected_time)
+
+        self.assertDictContainsSubset(expected_metadata, attr.asdict(metadata))
+        self.assertIsInstance(metadata.id, UUID)
+
+    @ddt.data(
+        (1, TypeError, "'time' must be <class 'datetime.datetime'",),
+        # WARNING: utcnow() has no timezone, and could be misinterpreted in local time
+        (datetime.datetime.utcnow(), ValueError, "'time' must have timezone.utc",),
+    )
+    @ddt.unpack
+    def test_generate_signal_metadata_fails_with_invalid_time(
+        self, invalid_time, error_class, error_message
+    ):
+        """
+        Tests getting generated metadata for an event fails with a non-UTC time.
+
+        Expected behavior:
+            Raises an exception
+        """
+        with self.assertRaisesMessage(error_class, error_message):
+            self.public_signal.generate_signal_metadata(time=invalid_time)
 
     @patch("openedx_events.tooling.OpenEdxPublicSignal.generate_signal_metadata")
     @patch("openedx_events.tooling.Signal.send")
     def test_send_event_successfully(self, send_mock, fake_metadata):
         """
-        This method tests the process of sending an event that's allow to fail.
+        This method tests the process of sending an event that's allowed to fail.
 
         Expected behavior:
             The event is sent as a django signal with send method.
@@ -155,6 +207,23 @@ def test_send_robust_event_successfully(self, format_responses_mock, log_mock, f
             "Responses of the Open edX Event <org.openedx.learning.session.login.completed.v1>: \nfake-output"
         )
 
+    @patch("openedx_events.tooling.OpenEdxPublicSignal.generate_signal_metadata")
+    def test_send_event_with_time(self, fake_metadata):
+        """
+        This method tests the process of sending an event with a time argument.
+
+        Expected behavior:
+            The generate_signal_metadata is called using the passed time.
+        """
+        expected_metadata = Mock(some_data="some_data")
+        expected_time = datetime.datetime.now(datetime.timezone.utc)
+        fake_metadata.return_value = expected_metadata
+
+        self.public_signal.send_event(user=self.user_mock, time=expected_time)
+
+        # generate_signal_metadata is fully tested elsewhere
+        fake_metadata.assert_called_once_with(time=expected_time)
+
     @ddt.data(
         (
             {"student": Mock()},
@@ -190,7 +259,7 @@ def test_send_event_with_django(self):
         method.
 
         Expected behavior:
-            A warning is showed advicing to use Open edX events custom
+            A warning is showed advising to use Open edX events custom
             send_signal method.
         """
         message = "Please, use 'send_event' when triggering an Open edX event."

openedx_events/tooling.py

@@ -66,13 +66,18 @@ def get_signal_by_type(cls, event_type):
         """
         return cls._mapping[event_type]
 
-    def generate_signal_metadata(self):
+    def generate_signal_metadata(self, time=None):
         """
         Generate signal metadata when an event is sent.
 
         These fields are generated on the fly and are a subset of the Event
         Message defined in the OEP-41.
 
+        Arguments:
+            time (datetime): (Optional) Timestamp when the event was sent with
+                UTC timezone. Defaults to current time in UTC. See OEP-41 for
+                details.
+
         Example usage:
             >>> metadata = \
                 STUDENT_REGISTRATION_COMPLETED.generate_signal_metadata()
@@ -90,12 +95,22 @@ def generate_signal_metadata(self):
         return EventsMetadata(
             event_type=self.event_type,
             minorversion=self.minor_version,
+            time=time,
         )
 
-    def send_event(self, send_robust=True, **kwargs):
+    def send_event(self, send_robust=True, time=None, **kwargs):
         """
         Send events to all connected receivers.
 
+        Arguments:
+            send_robust (bool): Defaults to True. See Django signal docs.
+            time (datetime): (Optional - see note) Timestamp when the event was
+                sent with UTC timezone. For events requiring a DB create or
+                update, use the timestamp from the DB record. Defaults to
+                current time in UTC. This argument is optional for backward
+                compatability, but ideally would be explicitly set. See OEP-41
+                for details.
+
         Used to send events just like Django signals are sent. In addition,
         some validations are executed on the arguments, and then generates relevant
         metadata that can be used for logging or debugging purposes. Besides this behavior,
@@ -158,7 +173,7 @@ def validate_sender():
 
         validate_sender()
 
-        kwargs["metadata"] = self.generate_signal_metadata()
+        kwargs["metadata"] = self.generate_signal_metadata(time=time)
 
         if self._allow_send_event_failure or settings.DEBUG or not send_robust:
             return super().send(sender=None, **kwargs)