Skip to content

fix(reports): stamp email subject date at send time, not import time#40693

Open
rusackas wants to merge 1 commit into
masterfrom
fix/report-email-subject-send-time
Open

fix(reports): stamp email subject date at send time, not import time#40693
rusackas wants to merge 1 commit into
masterfrom
fix/report-email-subject-send-time

Conversation

@rusackas
Copy link
Copy Markdown
Member

@rusackas rusackas commented Jun 3, 2026

SUMMARY

EmailNotification.now was a class attribute evaluated once, when the module is first imported:

class EmailNotification(BaseNotification):
    now = datetime.now(timezone("UTC"))   # frozen at import time

Two problems flow from this:

  1. Latent product bug: in a long-running worker, every report email rendered with DATE_FORMAT_IN_EMAIL_SUBJECT enabled stamps the subject with the date the process started, not the actual send date. A worker up for several days would put a stale date in every alert/report subject.

  2. Flaky CI: test_email_subject_with_datetime reads two different clocks — the class attribute (frozen at import) and a fresh datetime.now() it samples itself — and asserts they render the same %Y-%m-%d. Those two samples are taken minutes apart, so a unit-test job straddling UTC midnight breaks the assumption:

    time (UTC) what happens date used
    23:54:xx the suite first imports email.py, so EmailNotification.now is evaluated → frozen 2026-06-02
    00:07:xx ~13 min later the test runs, samples its own now, builds the subject (which uses the frozen class attribute), and asserts 2026-06-03

    The subject carries the import-time date while the test compares against the run-time date, so once the run crosses midnight they disagree:

    AssertionError: assert '2026-06-03' in '[Report]  test alert 2026-06-02'
    #                       run-time date              import-time date
    

    Any unit-test job whose run happens to span 00:00 UTC trips this — which is why it was failing on several unrelated open PRs (chore(mcp): return a generic error from the webdriver pool-stats endpoint #40559, fix(embedded): enforce configured allowed domains for postMessage origin #40629, fix(export): sanitize user-supplied CSV export filename (charts + SQL Lab) #40632, fix(jinja): apply consistent escaping to url_param values from request args #40633) at the same time. Same single flake, not separate bugs. (It also self-heals on a re-run that doesn't cross midnight, which is what made it look intermittent.)

Changes

  • superset/reports/notifications/email.py: stamp self.now once per instance in __init__ (at send time) instead of once per process at import. Each notification now reflects its real creation/send time.
  • tests/unit_tests/reports/notifications/email_tests.py: assert against the notification's own now rather than a separately-sampled clock, so the test can't flake at the day boundary. Drops the now-unused datetime/timezone imports.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

python -m pytest tests/unit_tests/reports/notifications/email_tests.py

3/3 pass. The fixed test no longer depends on wall-clock alignment across the midnight boundary.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

🤖 Generated with Claude Code

`EmailNotification.now` was a class attribute evaluated once when the module
is first imported. In a long-running worker that means every report email with
DATE_FORMAT_IN_EMAIL_SUBJECT enabled renders the date the process started rather
than the actual send date. It also made test_email_subject_with_datetime flaky:
the test sampled its own clock separately from the frozen class attribute, so a
run crossing the UTC midnight boundary compared two different dates and failed
(e.g. `assert '2026-06-03' in '... 2026-06-02'`).

Move the timestamp to a per-instance value set in __init__ so each notification
reflects its real creation/send time, and assert against the notification's own
`now` in the test so it can't flake at the day boundary.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review Bot commented Jun 3, 2026

Code Review Agent Run #f66398

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: f97a68e..f97a68e
    • 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

AI Code Review powered by Bito Logo

@dosubot dosubot Bot added the alert-reports Namespace | Anything related to the Alert & Reports feature label Jun 3, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.05%. Comparing base (3bbb35e) to head (f97a68e).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #40693   +/-   ##
=======================================
  Coverage   64.05%   64.05%           
=======================================
  Files        2662     2662           
  Lines      143254   143256    +2     
  Branches    32941    32941           
=======================================
+ Hits        91764    91766    +2     
  Misses      49903    49903           
  Partials     1587     1587           
Flag Coverage Δ
hive 39.80% <60.00%> (-0.01%) ⬇️
mysql 58.48% <100.00%> (+<0.01%) ⬆️
postgres 58.55% <100.00%> (+<0.01%) ⬆️
presto 41.40% <60.00%> (-0.01%) ⬇️
python 60.03% <100.00%> (+<0.01%) ⬆️
sqlite 58.17% <100.00%> (+<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.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes report email subjects being stamped with a stale date by moving the “now” timestamp from an import-time class attribute to an instance attribute set at notification creation time, and updates the corresponding unit test to avoid midnight-boundary flakiness.

Changes:

  • Stamp EmailNotification.now per instance in __init__ (instead of once at module import) so subject date formatting reflects the actual notification creation/send time.
  • Update test_email_subject_with_datetime to assert against notification.now (the same timestamp used to render the subject), eliminating UTC-midnight flakes.
  • Remove now-unused datetime/timezone imports from the test module.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
superset/reports/notifications/email.py Move now stamping to EmailNotification.__init__ so date formatting is per-notification rather than frozen at worker import time.
tests/unit_tests/reports/notifications/email_tests.py Make the datetime subject assertion use the notification’s own stamped timestamp to prevent midnight-boundary test flakiness.

# 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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both sides of this assertion read notification.now, so if someone reverted this fix, this test would still pass. We should update the test or add one such that any revert to your fix would fail the test.

@bito-code-review
Copy link
Copy Markdown
Contributor

The current test assertion assert notification.now.strftime(datetime_pattern) in subject is correct because it compares the subject against the specific timestamp instance (notification.now) that the EmailNotification object used to generate that subject. This ensures the test is deterministic and avoids flakiness if the test execution crosses a midnight boundary, as the subject and the assertion are both derived from the same notification.now value. To make the test more robust against a revert, you could mock datetime.now to return a fixed, known timestamp, ensuring the subject contains that specific date regardless of when the test runs.

tests/unit_tests/reports/notifications/email_tests.py

# Mocking datetime to ensure a fixed, predictable timestamp
    with patch("superset.reports.notifications.email.datetime") as mock_datetime:
        mock_datetime.now.return_value = datetime(2026, 1, 1, tzinfo=timezone("UTC"))
        notification = EmailNotification(
            recipient=ReportRecipients(type=ReportRecipientType.EMAIL), content=content
        )
        subject = notification._get_subject()
        assert "2026-01-01" in subject

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

alert-reports Namespace | Anything related to the Alert & Reports feature preset-io size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants