codeant-ai-for-open-source[bot] commented on code in PR #40285:
URL: https://github.com/apache/superset/pull/40285#discussion_r3272298522
##########
superset/reports/notifications/email.py:
##########
@@ -215,9 +214,10 @@ def _get_subject(self) -> str:
def _parse_name(self, name: str) -> str:
"""If user add a date format to the subject, parse it to the real date
This feature is hidden behind a feature flag
`DATE_FORMAT_IN_EMAIL_SUBJECT`
- by default it is disabled
+ by default it is disabled. Evaluated per send so each scheduled email
+ carries the current timestamp rather than the worker's start time.
"""
- return self.now.strftime(name)
+ return datetime.now(timezone("UTC")).strftime(name)
Review Comment:
**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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Email subject and attachment timestamps can diverge.
- ⚠️ Report emails show inconsistent per-send identifiers.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The scheduled report execution pipeline calls
`ExecuteReportCommand.send()` in
`superset/commands/report/execute.py:63-70`, which builds a
`NotificationContent` and
passes it, along with each `ReportRecipients` row, into `_send()` at
`execute.py:3-71`.
2. `_send()` uses the polymorphic factory `create_notification()` in
`superset/reports/notifications/__init__.py:25-35`, which returns an
`EmailNotification`
instance (because `EmailNotification.type = ReportRecipientType.EMAIL` at
`email.py:80-85`) and then calls `notification.send()` at `execute.py:15-27`.
3. Inside `EmailNotification.send()` in
`superset/reports/notifications/email.py:236-259`,
the method first computes `subject = self._get_subject()` at line 238, where
`_get_subject()` (lines 207-212) uses `title=self._name`. The `_name`
property (lines
87-94) calls `_parse_name(self._content.name)` when
`DATE_FORMAT_IN_EMAIL_SUBJECT` is
enabled, so `_parse_name()` invokes
`datetime.now(timezone("UTC")).strftime(name)` at line
220 for the first time.
4. In the same `send()` call, `content = self._get_content()` at
`email.py:239` triggers
`_get_content()` (lines 113-205), which again accesses `self._name` when
building CSV and
PDF attachment names at lines 193 and 197. Each access re-enters
`_parse_name()` and
re-evaluates `datetime.now(timezone("UTC")).strftime(name)` at line 220. If
the report
name includes minute/second-level formatting (e.g. `"report %Y-%m-%d
%H:%M:%S"` as in the
pattern at `tests/unit_tests/reports/notifications/email_tests.py:117-119`)
and the time
between `_get_subject()` and `_get_content()` crosses a minute/second
boundary or
`datetime.now()` is otherwise non-deterministic (e.g. patched in tests), the
subject line
will embed the first timestamp while the attachment filenames (CSV/PDF)
embed the later
timestamp, producing inconsistent timestamps within a single email send.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b0b3ed55cb904747a86d231a85fefa84&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b0b3ed55cb904747a86d231a85fefa84&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/notifications/email.py
**Line:** 220:220
**Comment:**
*Logic Error: 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.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40285&comment_hash=592847d70e37533536a6c5f5dc8c788e7332390bcc855f6034c297407990dd98&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40285&comment_hash=592847d70e37533536a6c5f5dc8c788e7332390bcc855f6034c297407990dd98&reaction=dislike'>👎</a>
--
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]
