rusackas commented on code in PR #40693:
URL: https://github.com/apache/superset/pull/40693#discussion_r3358053083
##########
tests/unit_tests/reports/notifications/email_tests.py:
##########
@@ -130,8 +125,12 @@ def test_email_subject_with_datetime() -> None:
"execution_id": "test-execution-id",
},
)
- subject = EmailNotification(
+ notification = EmailNotification(
recipient=ReportRecipients(type=ReportRecipientType.EMAIL),
content=content
- )._get_subject()
+ )
+ subject = notification._get_subject()
assert datetime_pattern not in subject
- assert now.strftime(datetime_pattern) in subject
+ # Compare against the notification's own stamped timestamp rather than a
+ # separately-sampled clock, so the assertion can't flake when the test runs
+ # across the UTC midnight boundary.
+ assert notification.now.strftime(datetime_pattern) in subject
Review Comment:
Good catch. Reworked the test so it now constructs the notification under a
frozen clock and asserts the subject reflects that frozen instant. A
class-/module-level timestamp (the regression) carries the import-time date
instead, so I verified the test fails when the fix is reverted and passes with
it. Pushed in 797a06c.
##########
superset/reports/notifications/email.py:
##########
@@ -83,7 +83,16 @@ class EmailNotification(BaseNotification): # pylint:
disable=too-few-public-met
"""
type = ReportRecipientType.EMAIL
- now = datetime.now(timezone("UTC"))
+
+ def __init__(
+ self, recipient: ReportRecipients, content: NotificationContent
+ ) -> None:
+ super().__init__(recipient, content)
+ # Stamp the notification's creation time once, at send time, so the
date
Review Comment:
Thanks. In practice `create_notification` builds each EmailNotification per
recipient immediately before `notification.send()` (see `_send` in
commands/report/execute.py), so construction is effectively dispatch time and
the gap is negligible. I have rewritten the comment to describe that accurately
and dropped the reference to the old behavior.
##########
superset/reports/notifications/email.py:
##########
@@ -83,7 +83,16 @@ class EmailNotification(BaseNotification): # pylint:
disable=too-few-public-met
"""
type = ReportRecipientType.EMAIL
- now = datetime.now(timezone("UTC"))
+
+ def __init__(
+ self, recipient: ReportRecipients, content: NotificationContent
+ ) -> None:
+ super().__init__(recipient, content)
+ # Stamp the notification's creation time once, at send time, so the
date
Review Comment:
Addressed alongside the duplicate thread above: the comment now describes
the per-instance dispatch-time stamp accurately, and construction happens per
recipient immediately before send.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
