feat: add notification for D day for expired evidences #3089
Conversation
📝 WalkthroughWalkthroughAdds a daily 06:40 scheduled task Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant DB as Database
participant Checker as check_evidences_expired
participant Grouper as Grouping Logic
participant MailTask as send_notification_email_expired_evidence
participant Renderer as Template Renderer
participant Mailer
Scheduler->>Checker: trigger at 06:40
Checker->>DB: query Evidences where expiry_date < today
DB-->>Checker: expired evidences list
Checker->>Grouper: group by owner_email, compute max days-since-expiry
Grouper-->>Checker: per-owner payloads
Checker->>MailTask: enqueue per-owner send_notification_email_expired_evidence(owner_email, evidences, days)
MailTask->>Renderer: render "expired_evidences" with context
Renderer-->>MailTask: subject & body
MailTask->>Mailer: send email to owner_email
Mailer-->>MailTask: success / error
MailTask-->>Checker: log result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/tasks.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/tasks.py (1)
backend/core/models.py (1)
Evidence(3726-3813)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
backend/core/tasks.py (1)
545-547: Good improvement for clearer messaging.The special handling of
days==0as "today" improves notification clarity. The logic is correct, and the comment helpfully explains the intent.Note that this change creates a dependency on the email template correctly interpreting
days_text="today"in conjunction withdays_remaining=0. The template must avoid awkward constructions like "expiring in 0 today" and instead use contextual phrasing.
| # @db_periodic_task(crontab(minute="*/1")) # for testing | ||
| @db_periodic_task(crontab(hour="6", minute="33")) | ||
| def check_evidences_expiring_today(): | ||
| """Check for Evidences expiring today and notify owners""" | ||
| target_date = date.today() | ||
| evidences_expiring_today = ( | ||
| Evidence.objects.filter(expiry_date=target_date) | ||
| .exclude(status__in=["expired"]) | ||
| .prefetch_related("owner") | ||
| ) | ||
|
|
||
| # Group by individual owner | ||
| owner_evidences = {} | ||
| for evidence in evidences_expiring_today: | ||
| for owner in evidence.owner.all(): | ||
| if owner.email not in owner_evidences: | ||
| owner_evidences[owner.email] = [] | ||
| owner_evidences[owner.email].append(evidence) | ||
|
|
||
| # Send personalized email to each owner | ||
| for owner_email, evidences in owner_evidences.items(): | ||
| # days=0 indicates 'today' | ||
| send_evidence_expiring_soon_notification(owner_email, evidences, days=0) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the email template and related files
fd -type f -e html -e txt -e jinja -e jinja2 | xargs grep -l "evidence_expiring_soon\|days_remaining\|days_text" 2>/dev/null | head -20Repository: intuitem/ciso-assistant-community
Length of output: 247
🏁 Script executed:
# Search for the send_evidence_expiring_soon_notification function
rg -n "send_evidence_expiring_soon_notification" --type py -A 15Repository: intuitem/ciso-assistant-community
Length of output: 4237
🏁 Script executed:
# Look at lines 545-547 to understand the days_text modification
sed -n '540,550p' backend/core/tasks.pyRepository: intuitem/ciso-assistant-community
Length of output: 517
🏁 Script executed:
# Search for evidence_expiring_soon template file
fd evidence_expiring_soonRepository: intuitem/ciso-assistant-community
Length of output: 198
🏁 Script executed:
# Also search broader for email templates
fd -type f -name "*.html" -o -name "*.txt" | xargs grep -l "evidence_expiring_soon\|days_remaining" 2>/dev/nullRepository: intuitem/ciso-assistant-community
Length of output: 249
🏁 Script executed:
# Search in common email template locations
find . -path "*/templates/*" -type f | xargs grep -l "days_remaining\|days_text" 2>/dev/nullRepository: intuitem/ciso-assistant-community
Length of output: 479
🏁 Script executed:
cat -n backend/core/templates/emails/en/evidence_expiring_soon.yamlRepository: intuitem/ciso-assistant-community
Length of output: 488
🏁 Script executed:
cat -n backend/core/templates/emails/fr/evidence_expiring_soon.yamlRepository: intuitem/ciso-assistant-community
Length of output: 490
Template implementation does not properly handle days=0 case—revision required.
The concern in the original review is confirmed. The English template renders "Evidence is expiring in 0 today" (grammatically incorrect) and the French template renders "Preuve expirant dans 0 jour(s)" (also incorrect).
The template should use only days_text when days_remaining=0, not concatenate it with 0. Either:
- Modify templates to conditionally render: use only
${days_text}whendays_remaining=0, or - Adjust context logic to send a formatted string instead of separate
days_remaininganddays_textvariables.
Lines 545-547 correctly compute days_text, but the templates need updates to leverage it properly for the days=0 case.
🤖 Prompt for AI Agents
In backend/core/tasks.py around lines 197-220, the notification currently calls
send_evidence_expiring_soon_notification with days=0 which forces templates to
render awkward "0" phrases; instead, change the context logic so that when
days==0 you pass a single formatted string (e.g. days_display or reuse the
already-computed days_text from lines ~545-547) rather than separate
days_remaining and days_text, and update templates to use that single
days_display variable (or make them conditionally render only days_text when
days_remaining==0) so the message for expiring today renders correctly in both
English and French.
eric-intuitem
left a comment
eric-intuitem
left a comment
There was a problem hiding this comment.
We should align on applied controls. No need to do differently for "today" (which provokes an inconsistency as noted by coderabbitai). So in practice this simply means sending a mail everyday when the evidence is expired.
martinzerty
force-pushed
the
CA-1455-send-notification-for-evidence-expiration
branch
from
13ade2d to
c923886
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/core/templates/emails/en/expired_evidences.yaml (1)
5-5: Handle singular/plural forms properly.The notation "evidence(s)" is non-idiomatic. Consider using proper singular/plural handling based on the count.
🔎 Proposed improvement
Add a context variable in the task function for proper pluralization:
context = { "evidence_count": len(evidences), "evidence_word": "evidence" if len(evidences) == 1 else "evidences", ... }Then update the template:
- You have ${evidence_count} evidence(s) expired since ${expired_since} ${days_text}: + You have ${evidence_count} ${evidence_word} expired ${expired_message}:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/core/tasks.pybackend/core/templates/emails/en/expired_evidences.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/tasks.py (2)
backend/core/models.py (1)
Evidence(3726-3813)backend/core/email_utils.py (2)
render_email_template(70-101)format_evidence_list(151-177)
🪛 GitHub Actions: Backend Linters
backend/core/tasks.py
[error] 1-1: ruff format --check failed: 1 file would be reformatted (core/tasks.py). Run 'ruff format' to fix formatting.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
backend/core/tasks.py (1)
197-224: Implementation looks correct for recurring expired evidence reminders.The scheduled task correctly:
- Queries evidences with
expiry_date < todaywithout status filtering (intentional for daily reminders)- Groups by owner email
- Computes maximum days since expiry per owner
- Sends personalized notifications
The logic aligns with the existing pattern for expired control notifications (line 31).
| @task() | ||
| def send_notification_email_expired_evidence(owner_email, evidences, days=0): | ||
| if not check_email_configuration(owner_email, evidences): | ||
| return | ||
|
|
||
| from .email_utils import render_email_template, format_evidence_list | ||
|
|
||
| context = { | ||
| "evidence_count": len(evidences), | ||
| "evidence_list": format_evidence_list(evidences), | ||
| "expired_since": days, | ||
| "days_text": "day" if days == 1 else "days", | ||
| } | ||
|
|
||
| rendered = render_email_template("expired_evidences", context) | ||
| if rendered: | ||
| send_notification_email(rendered["subject"], rendered["body"], owner_email) | ||
| else: | ||
| logger.error( | ||
| f"Failed to render expired_evidences email template for {owner_email}" | ||
| ) |
There was a problem hiding this comment.
Context handling mirrors template grammar issue.
The context variables expired_since and days_text are passed separately, which causes the template to render awkward phrases like "expired since 0 day(s)" when days=0 or "expired since 7 days" for larger values.
This is similar to the issue flagged in past reviews for evidence_expiring_soon. Consider pre-computing a single formatted string in the context (e.g., expired_message = "today" when days=0, or expired_message = "7 days ago" otherwise) rather than separate numeric and text variables.
See the review comment on the template file for a detailed proposal.
| @@ -0,0 +1,12 @@ | |||
| subject: "CISO Assistant: Evidence is expired since ${expired_since} ${days_text}" | |||
There was a problem hiding this comment.
Improve subject grammar and handle the days=0 edge case.
The phrase "expired since ${expired_since} ${days_text}" is grammatically awkward. When expired_since=0, it renders as "expired since 0 day(s)", which is confusing for users.
Consider:
- Rephrasing to "expired ${expired_since} ${days_text} ago" for days > 0
- Using conditional logic to render "expired today" when days = 0
- Or pre-formatting the time phrase in the task context instead of separate variables
🔎 Proposed improvement
Option 1: Update the template to be more natural:
-subject: "CISO Assistant: Evidence is expired since ${expired_since} ${days_text}"
+subject: "CISO Assistant: Evidence expired ${expired_since} ${days_text} ago"Option 2 (better): Handle the days=0 case in send_notification_email_expired_evidence by computing a single expired_message context variable:
if days == 0:
expired_message = "today"
else:
days_text = "day" if days == 1 else "days"
expired_message = f"{days} {days_text} ago"
context = {
"evidence_count": len(evidences),
"evidence_list": format_evidence_list(evidences),
"expired_message": expired_message,
}Then update the template:
-subject: "CISO Assistant: Evidence is expired since ${expired_since} ${days_text}"
+subject: "CISO Assistant: Evidence expired ${expired_message}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| subject: "CISO Assistant: Evidence is expired since ${expired_since} ${days_text}" | |
| subject: "CISO Assistant: Evidence expired ${expired_since} ${days_text} ago" |
🤖 Prompt for AI Agents
backend/core/templates/emails/en/expired_evidences.yaml around line 1: the
subject "CISO Assistant: Evidence is expired since ${expired_since}
${days_text}" is grammatically awkward and renders poorly when expired_since = 0
(e.g., "0 day(s)"). Fix by computing a single pre-formatted time phrase in the
email-generation code (e.g., "today" or "<N> day(s) ago") and pass it as one
context variable like expired_message, then update the template subject to use
that variable (e.g., "CISO Assistant: Evidence expired ${expired_message}").
Ensure singular/plural handling for 1 vs >1 and the zero-days case.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.