Fix: Add send_weekly_stats management command #5360
Conversation
|
👋 Hi @ShreyashBRN! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you! |
WalkthroughAdds a new Django management command Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Cmd as Management Command
participant DB as Database
participant Reporter as Report Builder
participant Email as SMTP/Email Service
participant Logger as Logger
Note over Cmd,DB: Start - query eligible domains
Cmd->>DB: Query domains with organization & admin email
DB-->>Cmd: Filtered domain list
loop for each domain
Cmd->>DB: Fetch issues from last 7 days (open/closed + metadata)
DB-->>Cmd: Issue records
Cmd->>Reporter: Build formatted weekly report
Reporter-->>Cmd: Formatted report (subject, body, recipients)
Cmd->>Email: send_mail(subject, body, from, [recipient])
alt email sent
Email-->>Cmd: Success
Cmd->>Logger: Log delivery success
else send failed
Email-->>Cmd: Error/Exception
Cmd->>Logger: Log delivery failure with exception
end
end
Cmd->>Logger: Print final summary (processed / successes / failures)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
📊 Monthly LeaderboardHi @ShreyashBRN! Here's how you rank for December 2025:
Leaderboard based on contributions in December 2025. Keep up the great work! 🚀 |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| open_issues = domain.open_issues | ||
| closed_issues = domain.closed_issues | ||
| total_issues = open_issues.count() + closed_issues.count() | ||
| issues = Issue.objects.filter(domain=domain) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
website/management/commands/send_weekly_stats.py (2)
40-42: Consider optimizing issue count queries.The current approach calls
.count()multiple times on querysets. Once the date filtering is added (per the critical issue comment), consider computing counts more efficiently.🔎 Suggested optimization
After adding date filtering, you could use aggregation:
from django.db.models import Count, Q week_ago = timezone.now() - timedelta(days=7) issue_stats = Issue.objects.filter( domain=domain, created__gte=week_ago ).aggregate( total=Count('id'), open=Count('id', filter=Q(status__ne='closed')), closed=Count('id', filter=Q(status='closed')) ) open_count = issue_stats['open'] closed_count = issue_stats['closed'] total_count = issue_stats['total']
46-71: Consider using email templates for better maintainability.The current approach uses string concatenation to build the email body. For better maintainability and formatting flexibility, consider using Django's template system or a dedicated email template.
This is a nice-to-have improvement that would make the email content easier to maintain and style, especially if HTML emails are desired in the future.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/management/commands/send_weekly_stats.py
🧰 Additional context used
🧬 Code graph analysis (1)
website/management/commands/send_weekly_stats.py (1)
website/models.py (4)
Domain(386-484)Issue(590-739)open_issues(419-420)closed_issues(423-424)
⏰ 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). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (5)
website/management/commands/send_weekly_stats.py (5)
1-9: LGTM! Clean imports and logger setup.The imports are appropriate for a Django management command that sends emails, and the logger is properly initialized.
12-13: LGTM! Proper command structure.The command class correctly inherits from
BaseCommandand includes descriptive help text.
24-28: Domain filtering logic is sound.The queryset correctly filters for domains that have an organization with an admin and a non-empty email address. The filtering approach is appropriate for the stated requirements.
92-102: LGTM! Clear and informative summary output.The summary provides good visibility into the command's execution with success/failure counts and appropriate logging.
74-80: No action needed. The Django email backend is already properly configured inblt/settings.pywith all required parameters (EMAIL_HOST, EMAIL_PORT, EMAIL_HOST_USER, EMAIL_HOST_PASSWORD, and EMAIL_USE_TLS), including environment variable handling for sensitive credentials in production.Likely an incorrect or invalid review comment.
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
github-actions
bot
added
the
changes-requested
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
website/management/commands/send_weekly_stats.py (1)
73-73: Handle None value for views field.According to the Issue model, the
viewsfield is nullable. WhenviewsisNone, the email will display "Views: None" which is not user-friendly.🔎 Proposed fix
description = issue.description - views = issue.views + views = issue.views if issue.views is not None else 0 label = issue.get_label_display()
🧹 Nitpick comments (1)
website/management/commands/send_weekly_stats.py (1)
25-29: Suggest adding select_related to avoid N+1 queries.The query filters domains correctly, but accessing
domain.organization.namemultiple times in the loop (lines 61, 93, 94, 98, 100) will trigger a separate database query for each domain's organization relationship.🔎 Proposed optimization
domains = Domain.objects.filter( organization__isnull=False, organization__admin__isnull=False, email__isnull=False, - ).exclude(email="") + ).exclude(email="").select_related('organization')
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/management/commands/send_weekly_stats.py
🧰 Additional context used
🧬 Code graph analysis (1)
website/management/commands/send_weekly_stats.py (1)
website/models.py (4)
Domain(386-484)Issue(590-739)open_issues(419-420)closed_issues(423-424)
⏰ 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). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
github-actions
bot
removed
the
changes-requested
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
github-actions
bot
added
pre-commit: failed
| domains = Domain.objects.filter( | ||
| organization__isnull=False, | ||
| organization__admin__isnull=False, | ||
| email__isnull=False, | ||
| ).exclude(email="") |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
website/management/commands/send_weekly_stats.py (2)
75-80: Fix f-string indentation for consistency.The implicit string concatenation is valid Python, but the f-strings are not properly indented relative to the
append(call, making the code harder to read.🔎 Proposed fix for consistent indentation
report_data.append( - f"\nDescription: {description}\n" - f"Views: {views}\n" - f"Labels: {label}\n" - f"{'-' * 50}\n" + f"\nDescription: {description}\n" + f"Views: {views}\n" + f"Labels: {label}\n" + f"{'-' * 50}\n" )Or use a single f-string:
report_data.append( - f"\nDescription: {description}\n" - f"Views: {views}\n" - f"Labels: {label}\n" - f"{'-' * 50}\n" + f"\nDescription: {description}\n" + f"Views: {views}\n" + f"Labels: {label}\n" + f"{'-' * 50}\n" )
17-21: Consider adding optional command arguments for flexibility.The command works well, but could benefit from optional enhancements:
- --dry-run flag: Preview emails without sending
- --days argument: Customize the reporting period (currently hardcoded to 7 days)
- --limit argument: Process a subset of domains for testing
🔎 Example implementation
def add_arguments(self, parser): parser.add_argument( '--dry-run', action='store_true', help='Preview reports without sending emails', ) parser.add_argument( '--days', type=int, default=7, help='Number of days to include in report (default: 7)', ) parser.add_argument( '--limit', type=int, help='Limit number of domains to process (for testing)', ) def handle(self, *args, **options): dry_run = options.get('dry_run', False) days = options.get('days', 7) limit = options.get('limit') # ... then use these values in the logic week_ago = timezone.now() - timedelta(days=days) domains = Domain.objects.filter(...) if limit: domains = domains[:limit] # ... in email sending: if not dry_run: send_mail(...) else: self.stdout.write(self.style.WARNING(f"[DRY RUN] Would send to {domain.email}"))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/management/commands/send_weekly_stats.py
🧰 Additional context used
🧬 Code graph analysis (1)
website/management/commands/send_weekly_stats.py (1)
website/models.py (4)
Domain(386-484)Issue(590-739)open_issues(419-420)closed_issues(423-424)
⏰ 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). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (9)
website/management/commands/send_weekly_stats.py (9)
1-11: LGTM! Clean imports.All necessary imports are present and properly organized (stdlib, Django, local models).
14-15: LGTM! Proper command structure.The command properly inherits from
BaseCommandwith clear help text.
37-52: Excellent fixes! Date filtering and query optimization implemented correctly.The weekly date filtering has been properly implemented, addressing previous feedback:
- ✓ Uses
created__gte=week_agofor all issue queries- ✓ Stores counts in variables (
open_count,closed_count) to eliminate redundant database queries- ✓ Consistently applies date range across open/closed/all issue queries
59-65: Great fix! Now correctly shows organization name.The report now properly displays
domain.organization.nameinstead ofdomain.name, addressing previous feedback.
71-74: Perfect fix for None handling!The None-check for
issue.views(line 73) correctly defaults to 0, preventing "None" from appearing in emails.
87-93: Verify EMAIL_HOST_USER is configured.The
send_mailcall relies onsettings.EMAIL_HOST_USER. If this setting is not properly configured, all emails will fail.Consider adding a configuration check at the start of
handle()to fail fast with a clear error message.🔎 Suggested configuration check
Add this check after line 22:
# Validate email configuration if not getattr(settings, 'EMAIL_HOST_USER', None): self.stdout.write(self.style.ERROR("ERROR: EMAIL_HOST_USER is not configured in settings")) logger.error("EMAIL_HOST_USER setting is not configured") return
95-104: LGTM! Solid error handling with logging.The error handling correctly:
- Wraps each domain's processing in try-except
- Continues processing other domains on failure
- Logs both successes and failures
- Provides clear console output
106-116: LGTM! Clear summary output.The final summary provides excellent visibility into the command's execution with clear counts of successful/failed deliveries.
25-29: Clarify email recipient: should reports go todomain.emailororganization.admin.email?The filter requires an organization admin to exist (
organization__admin__isnull=False), and the report content references the organization by name, but the email recipient isdomain.emailrather than the organization admin's email. The help text mentions sending to "organization owners," which typically refers to admins. Confirm whether this is intentional—isdomain.emailmeant to be the organization admin's contact, or should the recipient beorganization.admin.email?
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/management/commands/send_weekly_stats.py (1)
43-52: Consider combining issue queries to reduce database calls.Lines 44-45 query open and closed issues separately, then line 52 queries all issues again. This results in three database queries per domain when you could combine the already-filtered querysets.
🔎 Proposed optimization
# Generate report data for this domain - FILTER BY WEEK open_issues = domain.open_issues.filter(created__gte=week_ago) closed_issues = domain.closed_issues.filter(created__gte=week_ago) # Store counts to avoid redundant queries open_count = open_issues.count() closed_count = closed_issues.count() total_issues = open_count + closed_count - issues = Issue.objects.filter(domain=domain, created__gte=week_ago) + # Reuse the already-filtered querysets instead of a third query + issues = open_issues.union(closed_issues).order_by('created')Note: If you need to preserve specific ordering or access related fields later, you might need to adjust the union approach. An alternative is to iterate over both querysets separately in the report building section.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/management/commands/send_weekly_stats.py
🧰 Additional context used
🧬 Code graph analysis (1)
website/management/commands/send_weekly_stats.py (1)
website/models.py (3)
Issue(590-739)open_issues(419-420)closed_issues(423-424)
⏰ 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). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (8)
website/management/commands/send_weekly_stats.py (8)
1-11: LGTM! Clean import structure.All necessary imports are present and well-organized (standard library, Django, local models).
14-22: LGTM! Well-documented command structure.The Command class follows Django conventions with clear help text and docstring.
25-29: LGTM! N+1 query issue resolved.The use of
select_related('organization')correctly addresses the previous N+1 query concern. The filtering logic properly ensures only domains with valid organizations, admins, and emails are processed.
31-41: LGTM! Proper setup for weekly reporting.Counter initialization and date range calculation using timezone-aware datetime is correct.
54-81: LGTM! Report building handles previous concerns correctly.The report correctly uses
domain.organization.name(not domain name) and handlesNonevalues forissue.views. The email content is clear and well-formatted.
83-95: LGTM! Email sending with proper error handling.The
send_mailconfiguration is correct withfail_silently=Falseallowing exceptions to be caught by the outer try/except block. Success logging provides good observability.
96-101: LGTM! Appropriate error handling for batch processing.The broad exception catch ensures one failed email doesn't halt processing for all domains. Error logging provides good debugging information.
103-113: LGTM! Clear and informative summary output.The final summary provides comprehensive statistics with good visual formatting. The combination of stdout and logger output ensures both interactive and programmatic observability.
github-actions
bot
added
pre-commit: passed
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
website/management/commands/send_weekly_stats.py (5)
24-33: Excellent query optimization with select_related!The domain filtering is well-designed:
- Correctly filters for organizations with admins and valid emails
- Uses
select_related("organization")to avoid N+1 query problems- Properly handles both null and empty email values
Optional: Consider filtering for active domains
If inactive domains shouldn't receive weekly reports, you could add:
domains = ( Domain.objects.filter( organization__isnull=False, organization__admin__isnull=False, email__isnull=False, + is_active=True, ) .exclude(email="") .select_related("organization") )
48-56: Excellent! Date filtering and count caching implemented correctly.The code properly:
- Filters issues by the weekly date range (past review issue addressed ✓)
- Caches
.count()results to avoid redundant queries (past review issue addressed ✓)- Maintains separate querysets for open, closed, and all issues
Optional: Consider using .only() for memory optimization
If there are many issues per domain, you could reduce memory usage by fetching only needed fields:
-issues = Issue.objects.filter(domain=domain, created__gte=week_ago) +issues = Issue.objects.filter(domain=domain, created__gte=week_ago).only( + 'description', 'views', 'label' +)This is a micro-optimization and only beneficial if issues have many unused fields.
59-84: LGTM! Email content properly constructed.The email building logic correctly:
- Uses
domain.organization.nameinstead ofdomain.name(past review issue addressed ✓)- Uses cached counts to avoid redundant queries ✓
- Handles
Nonevalues forviewsfield (past review issue addressed ✓)- Uses
get_label_display()for human-readable labelsOptional: Include issue URLs for better user experience
Consider adding clickable links to issues:
for issue in issues: description = issue.description views = issue.views if issue.views is not None else 0 label = issue.get_label_display() + issue_url = f"{settings.DOMAIN_NAME}{issue.get_absolute_url()}" report_data.append( - f"\nDescription: {description}\n" f"Views: {views}\n" f"Labels: {label}\n" f"{'-' * 50}\n" + f"\nDescription: {description}\n" + f"Views: {views}\n" + f"Labels: {label}\n" + f"URL: {issue_url}\n" + f"{'-' * 50}\n" )
100-105: Consider more specific exception handling.While catching broad
Exceptionallows the command to continue processing other domains (which is good), it's better to catch specific exceptions and log full tracebacks for debugging.Proposed refinement for exception handling
- except Exception as e: + except Exception as e: failed_sends += 1 - logger.error(f"Failed to send weekly stats to {domain.organization.name} ({domain.email}): {str(e)}") + logger.error( + f"Failed to send weekly stats to {domain.organization.name} ({domain.email}): {str(e)}", + exc_info=True # This logs the full traceback + ) self.stdout.write( self.style.ERROR(f"✗ Failed to send to {domain.organization.name} ({domain.email}): {str(e)}") )Adding
exc_info=Truewill log the full stack trace, making it much easier to debug issues without changing the control flow.
14-117: Optional: Consider adding a dry-run option.For testing and safety, consider adding a
--dry-runflag that simulates the command without sending actual emails.Implementation example
class Command(BaseCommand): help = "Send weekly statistics emails to all domains with organization owners" + def add_arguments(self, parser): + parser.add_argument( + '--dry-run', + action='store_true', + help='Simulate the command without sending emails', + ) + def handle(self, *args, **options): """ Main function that runs when command is executed. Sends weekly reports to domains that have organizations with owners. """ + dry_run = options.get('dry_run', False) + if dry_run: + self.stdout.write(self.style.WARNING("DRY RUN MODE - No emails will be sent")) + self.stdout.write(self.style.WARNING("Starting weekly statistics email delivery...")) # ... rest of the code ... report_string = "".join(report_data) - # Send the email - send_mail( - subject="Weekly Report from OWASP BLT", - message=report_string, - from_email=settings.EMAIL_HOST_USER, - recipient_list=[domain.email], - fail_silently=False, - ) + # Send the email (unless dry-run) + if not dry_run: + send_mail( + subject="Weekly Report from OWASP BLT", + message=report_string, + from_email=settings.EMAIL_HOST_USER, + recipient_list=[domain.email], + fail_silently=False, + ) + else: + self.stdout.write(f"[DRY RUN] Would send email to {domain.email}")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
website/management/commands/send_weekly_stats.py
🧰 Additional context used
🧬 Code graph analysis (1)
website/management/commands/send_weekly_stats.py (1)
website/models.py (4)
Domain(386-484)Issue(590-739)open_issues(419-420)closed_issues(423-424)
⏰ 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). (3)
- GitHub Check: Run pre-commit
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (5)
website/management/commands/send_weekly_stats.py (5)
1-12: LGTM! Clean imports and setup.The imports are well-organized and include everything needed for the command. Logger initialization follows Django best practices.
14-22: LGTM! Well-structured command class.The command follows Django management command conventions with clear help text and documentation.
35-42: LGTM! Correct date handling.The date range calculation properly uses timezone-aware
timezone.now()for Django compatibility.
86-99: LGTM! Email sending properly implemented.The email sending logic:
- Uses correct
send_mailparameters- Sets
fail_silently=Falseto properly catch and handle email errors- Provides good logging and user feedback
107-117: LGTM! Clear and informative summary.The final summary provides excellent feedback with:
- Clear success/failure counts
- Good visual formatting
- Appropriate logging of final results
github-actions
bot
added
last-active: 0d
github-actions
bot
added
last-active: 1d
1 participant
Fixes #2182.
Adds a
send_weekly_statsmanagement command and moves the weekly stats logic there.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.