-
Notifications
You must be signed in to change notification settings - Fork 440
Django Signals for Automated Webhook Notifications #2317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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 |
WalkthroughThe changes introduce Django signal handlers to monitor status changes and creations for specific models, sending webhook notifications when relevant events occur. Supporting configuration and environment variable handling are added, along with updates to Docker and ignore files. The Docker Compose and Dockerfile setups are adjusted for local development and notification support. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DjangoApp
participant SignalsModule
participant Webhook
User->>DjangoApp: Create or update ComplianceAssessment/AppliedControl
DjangoApp->>SignalsModule: pre_save or post_save signal triggered
SignalsModule->>SignalsModule: Compare status/creation, build payload
alt Webhook URL configured
SignalsModule->>Webhook: Send POST request with payload
Webhook-->>SignalsModule: Responds with status
else No webhook URL
SignalsModule->>SignalsModule: Log missing configuration
end
Estimated code review effort4 (~90 minutes) Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (4)
✨ Finishing Touches
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. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.py
already does:
- Retrieves
WEBHOOK_URL
with a default ofNone
.- Checks
if not WEBHOOK_URL
and logs a warning before skipping.This covers both
None
and empty values, so the new setting is safe as-is.backend/.gitignore (1)
21-21
: No action needed: rootdocker-compose.yml
contains local overridesThe
docker-compose.yml
in 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_Store
matching is more comprehensive- Added cache directories (
__pycache__
,.pytest_cache
,.ruff_cache
) reduce context size- Including
db
,static/
, and test artifacts is appropriate- Excluding
.dockerignore
andDockerfile
from 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. |
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