feat: add deprecation task and common send notification mail #1482
Conversation
WalkthroughThe changes enhance the email notification system by introducing two new functions for handling notifications related to expired and deprecated controls. A new periodic task identifies and updates the status of deprecated controls, while a helper function checks email configurations before sending notifications. Additionally, logging has been added to the existing email sending function to improve traceability. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Task Scheduler
participant ExpiredTask as check_controls_with_expired_eta
participant EmailExpired as send_notification_email_expired_eta
participant EmailService as Email Service
Scheduler->>ExpiredTask: Trigger expired control check
ExpiredTask->>EmailExpired: Group controls by owner
EmailExpired->>EmailService: Send notification email
EmailService-->>EmailExpired: Acknowledge delivery
EmailExpired-->>ExpiredTask: Return status
sequenceDiagram
participant Scheduler as Task Scheduler
participant DeprecatedTask as check_deprecated_controls
participant EmailDeprecated as send_notification_email_deprecated_control
participant EmailService as Email Service
Scheduler->>DeprecatedTask: Trigger deprecated control check
DeprecatedTask->>DeprecatedTask: Identify & update control status
DeprecatedTask->>EmailDeprecated: Group controls by owner
EmailDeprecated->>EmailService: Dispatch notification email
EmailService-->>EmailDeprecated: Acknowledge delivery
EmailDeprecated-->>DeprecatedTask: Return status
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
backend/core/tasks.py (9)
38-40: Remove commented-out code if not needed.
The commented annotation can be removed to reduce clutter if it is no longer in use. Otherwise, consider adding a comment clarifying its purpose.-# @db_periodic_task(crontab(minute='*/1'))# for testing
49-55: Consider consolidating owner grouping logic.
Grouping controls by owner appears in multiple places (“expired” vs. “deprecated”). You could refactor into a helper function for DRY.
57-58: Clarify comment to match the updated status.
The code updates “status” to “deprecated” but the comment still refers to “expired control.” To avoid confusion, revise the comment.-# Update the status of each expired control +# Update the status of each active control that has passed its expiry date to deprecated
60-61: Reused “owner_controls” logic.
Same grouping approach as in the “expired” logic. You might gain from merging these flows or factoring out the shared logic.
63-64: Add docstring for new task function.
Expanding the function’s description can help explain how the email content is formed and used.
78-79: Add docstring for new deprecated control notifier.
Similar to the expired control email task, consider a short docstring explaining the function’s purpose.
82-91: Fix spelling and wording in email message.
“expiracy” is misspelled, and “This control(s)” is awkward. Consider a more natural phrasing, e.g., “These controls will be set to deprecated.”-subject = f"CISO Assistant: You have {len(controls)} deprecated control(s)" -message = "Hello,\n\nThe following controls have the expiracy date set to today:\n\n" -... -message += "\nThis control(s) will be set to deprecated.\n" +subject = f"CISO Assistant: You have {len(controls)} control(s) flagged as deprecated" +message = "Hello,\n\nThe following controls have the expiry date set to today:\n\n" +... +message += "\nThese controls will be set to deprecated.\n"
96-97: Evaluate appropriate log level for email details.
Since these logs don’t necessarily indicate errors, consider using INFO or DEBUG instead of WARNING.-logger.warning("subject = " + subject) -logger.warning("message = " + message) +logger.info("subject = " + subject) +logger.info("message = " + message)
109-110: Rename for clarity.
“check_email_configuration” could be “is_email_configured” or “can_send_email,” clarifying its boolean return.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/tasks.py(3 hunks)
🔇 Additional comments (13)
backend/core/tasks.py (13)
36-36: Well-structured notification call.
Splitting out the email-sending logic into a dedicated function for expired ETA controls helps keep concerns separated. Great approach.
41-44: Confirm business logic and time zones.
Filtering based on a strict “less than or equal to today” might need verification if you handle different time zones or daily boundaries in your system.
47-47: Prefetching owners improves performance.
Using prefetch_related for the owner relation is beneficial and prevents N+1 query issues.
65-65: Good safeguard for email configuration.
The check is correct for avoiding attempts to send email if the config is not set.
76-77: Centralized send function call.
Calling a shared function for the final send fosters consistency and maintainability.
80-81: Consistent email configuration check.
This matches the same pattern used elsewhere, ensuring consistent behavior.
93-94: Well-defined mail-sending function.
Delegating the final send operation to a dedicated function helps maintain clarity and reuse.
108-108: No issues.
Blank line or spacing change does not affect functionality.
111-120: Handle potential missing “general” settings.
If no GlobalSettings with name="general" exists, .get(...) may raise an exception. Consider a fallback or try/except block.
121-128: Thorough email settings check.
This is a straightforward way to ensure essential settings are present before sending.
129-133: Clear error logging for missing settings.
Returning early is a good fail-safe if crucial settings are missing.
134-137: Check for missing recipient address.
Refusing to proceed without a proper owner email prevents wasted sending logic.
138-139: Successful configuration check end.
Returning True clearly indicates everything is in order for email sending.
nas-tabchiche
changed the title
|
@Pioupuch can you please run a ruff formatter and commit this again? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
backend/core/tasks.py (5)
41-45: Optimize the database query by combining filters.The query can be simplified by combining the filters into a single Q object.
- deprecated_controls_list = ( - AppliedControl.objects - .filter(status="active") - .filter(expiry_date__lte=date.today(), expiry_date__isnull=False) - ) + deprecated_controls_list = AppliedControl.objects.filter( + Q(status="active") & + Q(expiry_date__lte=date.today()) & + Q(expiry_date__isnull=False) + )
49-61: Consider extracting the owner grouping logic into a helper function.The owner grouping logic is duplicated from
check_controls_with_expired_eta. Consider extracting it into a reusable helper function.+def group_controls_by_owner(controls): + owner_controls = {} + for control in controls: + for owner in control.owner.all(): + if owner.email not in owner_controls: + owner_controls[owner.email] = [] + owner_controls[owner.email].append(control) + return owner_controls def check_deprecated_controls(): deprecated_controls_list = ... deprecated_controls = deprecated_controls_list.prefetch_related("owner") - # Group by individual owner - owner_controls = {} - for control in deprecated_controls: - for owner in control.owner.all(): - if owner.email not in owner_controls: - owner_controls[owner.email] = [] - owner_controls[owner.email].append(control) + owner_controls = group_controls_by_owner(deprecated_controls) # Update the status of each expired control deprecated_controls_list.update(status="deprecated") for owner_email, controls in owner_controls.items(): send_notification_email_deprecated_control(owner_email, controls)
69-74: Consider using a template for email messages.The message construction could be moved to a template file for better maintainability and consistency.
Consider using Django's template system:
from django.template.loader import render_to_string message = render_to_string('email/expired_eta_notification.txt', { 'controls': controls, 'portal_url': settings.PORTAL_URL # Add this to settings })Example template (
templates/email/expired_eta_notification.txt):Hello, The following controls have expired: {% for control in controls %} - {{ control.name }} (ETA: {{ control.eta }}) {% endfor %} This reminder will stop once the control is marked as active or you update the ETA. Log in to your CISO Assistant portal and check 'my assignments' section to get to your controls directly: {{ portal_url }} Thank you.
110-110: Address the TODO comment about moving to a common section.The comment indicates this function should be moved. Consider creating a dedicated notifications module or utility class.
Would you like me to help create a dedicated notifications module and move this function there?
121-131: Consider moving email settings validation to Django settings configuration.The email settings validation could be performed during Django's settings configuration to fail fast on startup if required settings are missing.
Example implementation in your Django settings file:
def validate_email_settings(): required_settings = ["EMAIL_HOST", "EMAIL_PORT", "DEFAULT_FROM_EMAIL"] missing_settings = [ setting for setting in required_settings if not globals().get(setting) ] if missing_settings: raise ImproperlyConfigured( f"Missing required email settings: {', '.join(missing_settings)}" ) # Call this after all settings are loaded validate_email_settings()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/tasks.py(3 hunks)
🔇 Additional comments (2)
backend/core/tasks.py (2)
36-36: LGTM!The refactoring to use the new dedicated email function improves code organization.
96-96: LGTM!The change to use structured logging at debug level aligns with the requested improvements.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/core/tasks.py (2)
39-61: Consider wrapping status update and notifications in a transaction.The current implementation updates control status before sending notifications. If notification fails, controls will still be marked as deprecated without the owner being notified.
Consider wrapping the status update and notifications in a transaction:
- # Update the status of each expired control - deprecated_controls_list.update(status="deprecated") - - for owner_email, controls in owner_controls.items(): - send_notification_email_deprecated_control(owner_email, controls) + from django.db import transaction + + try: + with transaction.atomic(): + # Update the status of each expired control + deprecated_controls_list.update(status="deprecated") + + for owner_email, controls in owner_controls.items(): + send_notification_email_deprecated_control(owner_email, controls) + except Exception as e: + logger.error(f"Failed to process deprecated controls: {str(e)}") + raise
79-95: Fix typo in notification message.There's a typo in the message: "expiracy" should be "expiry".
- "Hello,\n\nThe following controls have the expiracy date set to today:\n\n" + "Hello,\n\nThe following controls have the expiry date set to today:\n\n"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/tasks.py(3 hunks)
🔇 Additional comments (3)
backend/core/tasks.py (3)
36-36: LGTM! Good separation of concerns.The change improves code organization by delegating email sending to a dedicated function.
63-77: LGTM! Clear and well-structured implementation.The function follows good practices with proper configuration checks and clear message construction.
100-100: LGTM! Good implementation of structured logging.The change correctly implements the requested structured logging at debug level, as discussed in previous reviews.
| @task() | ||
| def check_email_configuration(owner_email, controls): | ||
| notifications_enable_mailing = GlobalSettings.objects.get(name="general").value.get( | ||
| "notifications_enable_mailing", False | ||
| ) | ||
| if not notifications_enable_mailing: | ||
| logger.warning( | ||
| "Email notification is disabled. You can enable it under Extra/Settings. Skipping for now." | ||
| ) | ||
| return False | ||
|
|
||
| # Check required email settings | ||
| required_settings = ["EMAIL_HOST", "EMAIL_PORT", "DEFAULT_FROM_EMAIL"] | ||
| missing_settings = [ | ||
| setting | ||
| for setting in required_settings | ||
| if not hasattr(settings, setting) or not getattr(settings, setting) | ||
| ] | ||
|
|
||
| if missing_settings: | ||
| error_msg = f"Cannot send email notification: Missing email settings: {', '.join(missing_settings)}" | ||
| logger.error(error_msg) | ||
| return False | ||
|
|
||
| if not owner_email: | ||
| logger.error("Cannot send email notification: No recipient email provided") | ||
| return False | ||
|
|
||
| return True |
There was a problem hiding this comment.
Handle potential missing GlobalSettings.
The GlobalSettings query could raise an exception if the "general" settings don't exist.
- notifications_enable_mailing = GlobalSettings.objects.get(name="general").value.get(
- "notifications_enable_mailing", False
- )
+ try:
+ general_settings = GlobalSettings.objects.get(name="general")
+ notifications_enable_mailing = general_settings.value.get("notifications_enable_mailing", False)
+ except GlobalSettings.DoesNotExist:
+ logger.error("General settings not found. Email notifications disabled.")
+ return False📝 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.
| @task() | |
| def check_email_configuration(owner_email, controls): | |
| notifications_enable_mailing = GlobalSettings.objects.get(name="general").value.get( | |
| "notifications_enable_mailing", False | |
| ) | |
| if not notifications_enable_mailing: | |
| logger.warning( | |
| "Email notification is disabled. You can enable it under Extra/Settings. Skipping for now." | |
| ) | |
| return False | |
| # Check required email settings | |
| required_settings = ["EMAIL_HOST", "EMAIL_PORT", "DEFAULT_FROM_EMAIL"] | |
| missing_settings = [ | |
| setting | |
| for setting in required_settings | |
| if not hasattr(settings, setting) or not getattr(settings, setting) | |
| ] | |
| if missing_settings: | |
| error_msg = f"Cannot send email notification: Missing email settings: {', '.join(missing_settings)}" | |
| logger.error(error_msg) | |
| return False | |
| if not owner_email: | |
| logger.error("Cannot send email notification: No recipient email provided") | |
| return False | |
| return True | |
| @task() | |
| def check_email_configuration(owner_email, controls): | |
| - notifications_enable_mailing = GlobalSettings.objects.get(name="general").value.get( | |
| - "notifications_enable_mailing", False | |
| - ) | |
| + try: | |
| + general_settings = GlobalSettings.objects.get(name="general") | |
| + notifications_enable_mailing = general_settings.value.get("notifications_enable_mailing", False) | |
| + except GlobalSettings.DoesNotExist: | |
| + logger.error("General settings not found. Email notifications disabled.") | |
| + return False | |
| if not notifications_enable_mailing: | |
| logger.warning( | |
| "Email notification is disabled. You can enable it under Extra/Settings. Skipping for now." | |
| ) | |
| return False | |
| # Check required email settings | |
| required_settings = ["EMAIL_HOST", "EMAIL_PORT", "DEFAULT_FROM_EMAIL"] | |
| missing_settings = [ | |
| setting | |
| for setting in required_settings | |
| if not hasattr(settings, setting) or not getattr(settings, setting) | |
| ] | |
| if missing_settings: | |
| error_msg = f"Cannot send email notification: Missing email settings: {', '.join(missing_settings)}" | |
| logger.error(error_msg) | |
| return False | |
| if not owner_email: | |
| logger.error("Cannot send email notification: No recipient email provided") | |
| return False | |
| return True |
Summary by CodeRabbit