codeant-ai-for-open-source[bot] commented on PR #40285: URL: https://github.com/apache/superset/pull/40285#issuecomment-4496629031
Question: - added a `@cached_property _send_time` on `emailnotification` so the timestamp is captured **once per instance** and reused for subject + csv/pdf attachment filenames. - `_parse_name` now uses `self._send_time` instead of calling `datetime.now()` per access. - added a second regression test (`test_email_subject_datetime_consistent_within_single_send`) that patches `datetime.now` between two `_name` reads on the same instance and asserts both reads return the construct-time value — guarding against the boundary-crossing inconsistency you flagged. existing test `test_email_subject_datetime_evaluated_per_send` was updated to construct two separate `emailnotification` instances (matching the new per-send semantics) instead of two calls on one instance. `pytest tests/unit_tests/reports/notifications/email_tests.py -v`: 4 passed. Answer: Yes — the new implementation matches the intended fix, and the test update is aligned with it. A few notes: - `@cached_property _send_time` is a good fit here because it keeps the timestamp consistent across all reads on a single `EmailNotification` instance. That prevents the subject and any attachment filenames from drifting if `_name` is accessed more than once during one send. - Switching `_parse_name` to `self._send_time.strftime(name)` fixes the original bug: the timestamp is now evaluated lazily per notification instance instead of once at import time. - The new regression test is valuable because it proves the boundary-crossing issue you called out: two `_name` reads on the same instance stay stable even if `datetime.now` changes in between. - Updating `test_email_subject_datetime_evaluated_per_send` to use two separate `EmailNotification` instances is the correct expectation for the new semantics. One small caveat to keep in mind: with `cached_property`, the time is captured on first access, not strictly at object construction. That’s usually fine for “per send” behavior, but if construction and first use can be far apart, the captured time will reflect first use rather than instantiation. If that distinction matters, you’d need to set `_send_time` explicitly during initialization instead. -- 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]
