fix(reports): evaluate email subject datetime per send instead of at import

[Abdulrehman-PIAIC80387]

SUMMARY

Fixes #35908. The DATE_FORMAT_IN_EMAIL_SUBJECT feature was substituting the datetime token (e.g. %c) with a value frozen to the time the worker started, not the time each email was sent. For an hourly scheduled report this means every email arrives with the same subject — defeating the original purpose of the feature (per the original author in #31413: "enabling the email client to correctly thread the emails").

Root cause

EmailNotification.now was declared as a class attribute:

class EmailNotification(BaseNotification):
    type = ReportRecipientType.EMAIL
    now = datetime.now(timezone("UTC"))   # evaluated ONCE at module import time

Python evaluates class-body expressions when the class is first imported, so now was effectively datetime(worker_start_time) forever. _parse_name then used self.now.strftime(name) for every email.

Fix

• Remove the now class attribute.
• Compute datetime.now(timezone("UTC")) inside _parse_name on every call, so each scheduled email picks up the current timestamp.

TESTING INSTRUCTIONS

pytest tests/unit_tests/reports/notifications/email_tests.py -v

Result locally: 3 passed in 0.73s (existing 2 tests + 1 new regression test).

Manual repro (before the fix):

1. Set FEATURE_FLAGS = {"DATE_FORMAT_IN_EMAIL_SUBJECT": True} in superset_config.py.
2. Create an hourly scheduled report with an email subject containing a strftime token, e.g. Hourly Report %c.
3. Receive two consecutive hourly emails — both subjects show the worker's start time, not the send time.

After the fix: each email's subject shows the actual send time.

Regression test

tests/unit_tests/reports/notifications/email_tests.py::test_email_subject_datetime_evaluated_per_send patches datetime.now to return two different times between two _get_subject() calls on the same EmailNotification instance and asserts the resulting subjects differ.

ADDITIONAL INFORMATION

• Has associated issue: DATE_FORMAT_IN_EMAIL_SUBJECT only evaluated at schedule save time #35908
• Required feature flags: DATE_FORMAT_IN_EMAIL_SUBJECT (already exists, unchanged)
• Changes UI
• Includes DB Migration
• Introduces new feature or API
• Removes existing feature or API

[bito-code-review]

Code Review Agent Run #19e9db

Actionable Suggestions - 0

Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

• superset/reports/notifications/email.py - 2

  • CWE-374: Mutable Set Shared Reference · Line 62-62
  • Correctness Fix Verified · Line 214-220

Review Details

• Files reviewed - 2 · Commit Range: 4fa23bc..4fa23bc

  • superset/reports/notifications/email.py
  • tests/unit_tests/reports/notifications/email_tests.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.16%. Comparing base (e617903) to head (5f92915).

Files with missing lines	Patch %	Lines
superset/reports/notifications/email.py	66.66%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #40285   +/-   ##
=======================================
  Coverage   64.16%   64.16%           
=======================================
  Files        2591     2591           
  Lines      138293   138296    +3     
  Branches    32084    32084           
=======================================
+ Hits        88736    88738    +2     
- Misses      48027    48028    +1     
  Partials     1530     1530           

Flag	Coverage Δ
hive	39.44% <66.66%> (+<0.01%)	⬆️
mysql	59.13% <66.66%> (+<0.01%)	⬆️
postgres	59.21% <66.66%> (+<0.01%)	⬆️
presto	41.13% <66.66%> (+<0.01%)	⬆️
python	60.64% <66.66%> (+<0.01%)	⬆️
sqlite	58.85% <66.66%> (+<0.01%)	⬆️
unit	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bito-code-review]

The PR comment file contains 1 comment row (excluding the header). Here's the full comment content:

"Suggestion: The timestamp is recomputed on every _name access, not once per send. During a single email send flow, _name is read multiple times (subject plus attachment names), so if formatting includes seconds/minutes and execution crosses a boundary, subject and attachment filenames can disagree within the same message. Compute the current timestamp once per send (or once per notification rendering) and reuse it for all name/subject/attachment generation in that send. [logic error]"

[Abdulrehman-PIAIC80387]

@codeant-ai-for-open-source Good catch — addressed in 5f92915:

• 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.

[codeant-ai-for-open-source]

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.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.