Django Signals for Automated Webhook Notifications #2317
Conversation
- Add notification webhook URL to settings.py. - Import custom signals in core apps.py for better signal management. - Update Docker configuration and ignore files - Refactor .dockerignore to include additional files and directories. - Update docker-compose.yml to use a local backend image and add a new environment variable for notification webhook URL. - Modify backend .gitignore to streamline ignored files and directories. - Adjust Dockerfile to simplify poetry installation process.
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
WalkthroughThis update introduces webhook notification functionality for compliance and control status changes in the backend. It adds a new webhooks module, modifies model save methods to trigger notifications, updates environment and Docker settings for webhook configuration, and revises ignore files for both Docker and Git. Several utility and helper improvements are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DjangoModel
participant WebhooksModule
participant WebhookEndpoint
User->>DjangoModel: Create or update AppliedControl/ComplianceAssessment
DjangoModel->>DjangoModel: save()
DjangoModel->>WebhooksModule: get_payload(instance, old_status, event_type)
WebhooksModule->>WebhooksModule: send_webhook_notification(payload)
WebhooksModule->>WebhookEndpoint: POST payload
WebhookEndpoint-->>WebhooksModule: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (5)
backend/core/apps.py (1)
17-17: Use English for comment consistency.The signal import is correctly placed, but the comment should be in English to maintain consistency with the rest of the codebase.
- import core.signals # Active les signaux personnalisés + import core.signals # Activate custom signal handlersdocker-compose.yml (1)
4-4: Consider image consistency between backend and huey services.The backend service now uses a local 'backend' image while the huey service still uses the remote image. This could lead to version mismatches. Consider whether both services should use the same image source for consistency.
If using local development setup:
backend: container_name: backend image: backend huey: container_name: huey - image: ghcr.io/intuitem/ciso-assistant-community/backend:latest - pull_policy: always + image: backendbackend/core/signals.py (3)
1-9: Consolidate imports and remove verbose logging
- Combine the signal imports from lines 2 and 6 into a single import statement
- The module load logging on line 9 is unnecessary and will add noise to production logs
import logging -from django.db.models.signals import pre_save +from django.db.models.signals import pre_save, post_save from django.dispatch import receiver from django.conf import settings from core.models import ComplianceAssessment, AppliedControl -from django.db.models.signals import post_save logger = logging.getLogger(__name__) -logger.info('signals.py loaded')
55-88: Translate comment and good defensive programmingThe defensive programming for computed fields (lines 60-68) and attribute checks (lines 77, 84) is excellent. However, the French comment on line 87 should be in English.
'compliance_percentage': compliance_percentage, 'progress_percentage': progress_percentage, - # Ajoute d'autres champs si besoin + # Add other fields if needed }
14-141: Consider production-ready enhancementsFor a production environment, consider these architectural improvements:
- Bulk operations: Current signals fire for each instance in bulk updates, which could overwhelm the webhook endpoint
- Async processing: Webhook calls are synchronous and could slow down save operations
- Retry mechanism: Failed webhook calls are not retried
- Dead letter queue: Failed notifications are lost
Consider using a task queue (like Celery) for asynchronous webhook delivery with retry logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.dockerignore(1 hunks)backend/.gitignore(1 hunks)backend/Dockerfile(1 hunks)backend/ciso_assistant/settings.py(1 hunks)backend/core/apps.py(1 hunks)backend/core/signals.py(1 hunks)backend/manage.py(1 hunks)docker-compose.yml(1 hunks)
🧠 Learnings (2)
docker-compose.yml (1)
Learnt from: ab-smith
PR: intuitem/ciso-assistant-community#0
File: :0-0
Timestamp: 2025-01-21T19:16:31.223Z
Learning: The new Helm chart for CISO Assistant supports both SQLite and PostgreSQL modes, with backend and frontend pods separated, and includes configurable security contexts (disabled by default).
backend/Dockerfile (1)
Learnt from: nas-tabchiche
PR: #1641
File: dispatcher/startup.sh:1-6
Timestamp: 2025-04-17T17:28:15.732Z
Learning: In the CISO Assistant project, the startup.sh script is an entrypoint script that's intended to be executed only inside Docker containers where the uv package manager is installed at build time. The uv run commands are appropriate in this context.
🧰 Additional context used
🧠 Learnings (2)
docker-compose.yml (1)
Learnt from: ab-smith
PR: intuitem/ciso-assistant-community#0
File: :0-0
Timestamp: 2025-01-21T19:16:31.223Z
Learning: The new Helm chart for CISO Assistant supports both SQLite and PostgreSQL modes, with backend and frontend pods separated, and includes configurable security contexts (disabled by default).
backend/Dockerfile (1)
Learnt from: nas-tabchiche
PR: #1641
File: dispatcher/startup.sh:1-6
Timestamp: 2025-04-17T17:28:15.732Z
Learning: In the CISO Assistant project, the startup.sh script is an entrypoint script that's intended to be executed only inside Docker containers where the uv package manager is installed at build time. The uv run commands are appropriate in this context.
🔇 Additional comments (6)
backend/ciso_assistant/settings.py (1)
499-499: No action needed—None/empty URL is handled insend_webhook_notification.The signal handler in
backend/core/signals.pyalready does:
- Retrieves
WEBHOOK_URLwith a default ofNone.- Checks
if not WEBHOOK_URLand logs a warning before skipping.This covers both
Noneand empty values, so the new setting is safe as-is.backend/.gitignore (1)
21-21: No action needed: rootdocker-compose.ymlcontains local overridesThe
docker-compose.ymlin the project root holds environment-specific settings (debug flags, webhook URLs, etc.) and isn’t meant to be version-controlled. The canonical compose definitions live underconfig/(as templates and build scripts), so ignoring the root file prevents accidental commits of sensitive or local overrides.backend/manage.py (1)
6-6: LGTM! Proper signal registration pattern.Moving the signals import from manage.py to the app's ready() method is the correct Django approach. This ensures signal handlers are registered once during app initialization rather than on every management command execution.
backend/Dockerfile (1)
43-44: Good optimization: Consolidated RUN commandsCombining the poetry install and cache cleanup into a single RUN instruction reduces Docker layers, which improves build efficiency and reduces image size.
.dockerignore (1)
2-15: Improved Docker build context exclusionsThe updated patterns provide better coverage:
- Recursive
.DS_Storematching is more comprehensive- Added cache directories (
__pycache__,.pytest_cache,.ruff_cache) reduce context size- Including
db,static/, and test artifacts is appropriate- Excluding
.dockerignoreandDockerfilefrom the build context is a best practicebackend/core/signals.py (1)
91-141: Well-implemented signal handlersThe signal handlers are properly structured with:
- Clear separation between creation and update logic
- Comprehensive logging for debugging
- Proper handling of DoesNotExist exceptions
- Correct use of pre_save for status changes and post_save for creation notifications
|
Hello, Two pointers:
|
|
Hello @ab-smith , I appreciate your feedback and am pleased to hear that this feature is being developed on your end. Regarding your two points, I recognize that there's a performance issue, and I may address it in my next optimization. As for the .gitignore file, including it was an error.. There are URLs in the docker-compose that I prefer not to expose publicly. |
|
Sure thing, we appreciate the effort 🙏 |
…Notifications - Implemented webhook notifications on creation and status change for AppliedControl and ComplianceAssessment models. - Updated payload structure in get_compliance_assessment_payload and get_applied_control_payload to include asset and author IDs. - Removed pre_save signal handlers for both models, consolidating logic into the save method.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
backend/core/models.py (1)
715-744: Inconsistent error handling and potential performance impact.The webhook notification logic is well-structured, but there are a few concerns:
Performance Impact: Each save now requires an additional database query to fetch the old instance, plus a potential HTTP request for webhook notifications. Consider if this overhead is acceptable for all save operations.
Error Handling: The webhook notification calls aren't wrapped in try-catch blocks, which means webhook failures could prevent the model from saving successfully.
Consider wrapping webhook calls in exception handling:
# After save, check for status change or creation if is_new: payload = get_applied_control_payload(self, old_status=None) payload['type'] = 'applied_control_created' - send_webhook_notification(payload) + try: + send_webhook_notification(payload) + except Exception as e: + logger.warning(f"Failed to send webhook notification: {e}") elif old_status != self.status: payload = get_applied_control_payload(self, old_status) payload['type'] = 'applied_control_status_changed' - send_webhook_notification(payload) + try: + send_webhook_notification(payload) + except Exception as e: + logger.warning(f"Failed to send webhook notification: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/core/models.py(2 hunks)backend/core/signals.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/core/signals.py
…ly metrics - Set default min and max scores based on the framework if unset. - Assign scores definition from the framework when not provided. - Restore daily metrics tracking after saving the ComplianceAssessment. - Update payload structure in get_compliance_assessment_payload to include compliance and progress percentages, with improved logging for error handling.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend/core/signals.py (1)
47-50: Consider performance optimization for related object queriesThe function accesses multiple ManyToMany relationships (
owner,assets,evidences,security_exceptions) which could result in N+1 database queries. Since this is called from the model'ssave()method, ensure that these relationships are prefetched when the instance is loaded to avoid performance issues.Consider documenting that callers should use
select_related()andprefetch_related()when loading instances that will trigger webhooks, or add a note about potential performance impact with large related object sets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/core/models.py(2 hunks)backend/core/signals.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/core/models.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/core/signals.py (1)
backend/core/models.py (7)
ComplianceAssessment(3682-4480)AppliedControl(2560-2819)RequirementAssessment(4483-4668)assets(5116-5117)get_requirement_assessments(3971-4002)Result(4490-4495)get_progress(4436-4449)
🔇 Additional comments (1)
backend/core/signals.py (1)
14-24: LGTM! Well-implemented webhook notification functionThe function properly handles missing webhook URL, uses appropriate timeout, specific exception handling, and doesn't expose sensitive URLs in logs. All previous review feedback has been successfully incorporated.
backend/core/signals.py
Outdated
| # Calculate compliance percentage using the same logic as domain_compliance_percentage | ||
| requirement_assessments = instance.get_requirement_assessments(include_non_assessable=False) | ||
| total_count = len(requirement_assessments) | ||
| compliant_count = sum(1 for ra in requirement_assessments if ra.result == RequirementAssessment.Result.COMPLIANT) | ||
|
|
||
| compliance_percentage = int((compliant_count / total_count) * 100) if total_count > 0 else 0 | ||
| logger.info(f'ComplianceAssessment {instance.pk}: compliance_percentage={compliance_percentage}% ({compliant_count}/{total_count})') |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider using existing model methods for compliance calculation
The compliance percentage calculation duplicates logic that might already exist in the model. Consider checking if there's an existing method like get_global_score() or similar that could be reused to maintain consistency and avoid code duplication.
🏁 Script executed:
#!/bin/bash
# Search for existing compliance calculation methods in ComplianceAssessment
ast-grep --pattern 'class ComplianceAssessment {
$$$
def get_$_($$$) {
$$$
}
$$$
}'
# Also search for any method that calculates compliance or score
rg -A 10 'def get.*score|def get.*compliance' backend/core/models.pyLength of output: 1091
Extract compliance percentage into a reusable model method
The inline calculation in backend/core/signals.py (lines 60–66) duplicates business logic and there isn’t an existing model method that returns “compliance percentage” (the found get_global_score computes a different aggregate). To centralize this logic and avoid future drift:
• Define a new method on ComplianceAssessment (e.g. get_compliance_percentage(self)) that:
– Calls self.get_requirement_assessments(include_non_assessable=False)
– Computes int((compliant_count / total_count) * 100) (or 0 if no assessments)
• In backend/core/signals.py replace the block at lines 60–66 with:
- # …existing inline calculation…
- requirement_assessments = instance.get_requirement_assessments(include_non_assessable=False)
- total_count = len(requirement_assessments)
- compliant_count = sum(1 for ra in requirement_assessments if ra.result == RequirementAssessment.Result.COMPLIANT)
- compliance_percentage = int((compliant_count / total_count) * 100) if total_count > 0 else 0
+ compliance_percentage = instance.get_compliance_percentage()This ensures the percentage logic lives in one place on the model.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/core/signals.py around lines 60 to 66, the compliance percentage
calculation duplicates business logic and should be centralized. To fix this,
define a new method get_compliance_percentage(self) on the ComplianceAssessment
model that calls self.get_requirement_assessments(include_non_assessable=False),
calculates the compliance percentage as int((compliant_count / total_count) *
100) or returns 0 if no assessments exist. Then replace the inline calculation
in signals.py with a call to this new model method to ensure the logic is
maintained in one place.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
backend/core/signals.py (3)
1-4: Clean up unused importsThe code currently imports modules that are not used in this file. Based on past review comments, it appears Django signal imports were removed, but there may still be cleanup needed.
51-87: Address code duplication in compliance calculationThe inline compliance percentage calculation (lines 58-62) duplicates business logic. Based on past review comments, this should be extracted into a reusable model method to maintain consistency and avoid future drift.
80-81: Add defensive null checks for foreign key accessThe code accesses
framework.nameandperimeter.namewithout null checks, which could causeAttributeErrorif these foreign keys are None.
🧹 Nitpick comments (2)
backend/core/signals.py (2)
24-49: Consider adding type validation for event typesThe function hardcodes the event type as 'applied_control_status_changed', but this function might be used for creation events too. Consider making the event type a parameter to support different event types consistently.
-def get_applied_control_payload(instance, old_status): +def get_applied_control_payload(instance, old_status, event_type='applied_control_status_changed'): return { - 'type': 'applied_control_status_changed', + 'type': event_type,
1-87: Consider renaming file to better reflect its purposeThe filename
signals.pymight be misleading since this module doesn't actually use Django signals but rather provides utility functions for webhook notifications called from modelsave()methods. Consider renaming towebhooks.pyornotifications.pyto better reflect its actual purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/core/models.py(3 hunks)backend/core/signals.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/core/models.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/core/signals.py (1)
backend/core/models.py (7)
ComplianceAssessment(3682-4486)AppliedControl(2560-2819)RequirementAssessment(4489-4674)assets(5122-5123)get_requirement_assessments(3971-4002)Result(4496-4501)get_progress(4436-4446)
🔇 Additional comments (1)
backend/core/signals.py (1)
11-21: LGTM! Webhook notification implementation is secure and robustThe function correctly handles missing webhook URLs, uses appropriate timeout settings, implements proper exception handling with
requests.RequestException, and avoids logging sensitive URLs. The implementation follows Django best practices.
- Updated the CoreConfig to import webhooks instead of signals for registering notification handlers. - Refactored AppliedControl and ComplianceAssessment models to use webhooks for sending notifications on save. - Rename the signals.py file into webhooks.py.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/core/webhooks.py (3)
32-60: Potential non-JSON-serialisable fields in payload
priority,effort,control_impact,cost, andprogress_fieldare likelyDecimalor Enum instances.
json.dumps()insiderequestswill raiseTypeErrorfor non-primitives.Either cast to
float/strduring construction or provide a custom encoder, e.g.:- 'priority': instance.priority, + 'priority': float(instance.priority) if instance.priority is not None else None,Same applies to the compliance payload below.
55-58: Inefficient per-object queriesEven with the warning comment, the list-comprehensions invoke separate SQL queries if the caller forgot to prefetch.
A safer API is to require a fully-prefetched instance:
assert hasattr(instance, '_prefetched_objects_cache'), ( "Instance must be fetched with prefetch_related for owners/assets/…" )This fails fast in dev and avoids silent N+1 issues in prod.
71-82: Logging exceptions on every save will flood logsThe defensive try/except around
get_compliance_percentageandget_progresslogs at INFO/WARN on every save. For large imports this produces noise.Downgrade to
debugor log only on exception:- logger.info(f'ComplianceAssessment {instance.pk}: compliance_percentage={compliance_percentage}%') + logger.debug("ComplianceAssessment %s compliance=%s%%", instance.pk, compliance_percentage)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/core/apps.py(2 hunks)backend/core/models.py(3 hunks)backend/core/webhooks.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/core/apps.py
- backend/core/models.py
| from core.models import ComplianceAssessment, AppliedControl | ||
| import requests |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Risk of circular imports and unused dependency
ComplianceAssessment and AppliedControl are imported but never referenced in this module.
More importantly, models.py now imports core.webhooks (to call these helpers) while this file imports core.models, forming a hard circular dependency that will break app start-up once both modules are imported at the top level.
Remove the model imports (the payload helpers already receive the instances), or fetch the classes lazily via apps.get_model() inside the helper if you really need the class.
🤖 Prompt for AI Agents
In backend/core/webhooks.py at lines 3 to 4, the imports of ComplianceAssessment
and AppliedControl from core.models are unused and cause a circular import
because core.models imports core.webhooks. Remove these model imports entirely
since the helper functions already receive the instances they need, or if the
classes are absolutely required, import them lazily inside the functions using
apps.get_model() to avoid the circular dependency.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Hello @ab-smith, Should be better now by using an extension of the save method of the given objects. |
The signals.py file in backend/core/ defines Django signal handlers that automatically monitor changes to two key models: ComplianceAssessment and AppliedControl. Whenever the status of these objects changes or a new object is created, the signals trigger and send a structured JSON payload to an external webhook URL (configured via the NOTIFICATION_WEBHOOK_URL environment variable).
This mechanism enables seamless integration with external systems (such as SIEM, ticketing, or monitoring tools) by providing real-time notifications about important compliance and control events within the application. The file centralizes and automates the notification logic, ensuring that key changes are always communicated to interested third-party services without manual intervention.
Summary by CodeRabbit
New Features
Chores