feat: add automatic IMAP email bounce parser#299
Conversation
- Add IMAP credential fields to ConnectedEmailAccount (host, port, username, password) - Add check_imap_bounces periodic Celery task - Connects via imaplib, searches INBOX for bounce notification subjects - Parses failed recipient from DSN headers with body fallback - Updates matching CampaignLead status to BOUNCED, scoped by organization - Marks processed messages as Seen to avoid reprocessing - Registered in CELERY_BEAT_SCHEDULE, runs every 15 minutes - Per-account error isolation so one failure doesn't abort the task Closes Kuldeeep18#123
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds symmetric field encryption infrastructure, extends ChangesIMAP Bounce Detection Feature with Encryption
Sequence DiagramsequenceDiagram
participant CeleryBeat
participant check_imap_bounces
participant IMAP4_SSL as IMAP4_SSL Server
participant CampaignLead as CampaignLead DB
CeleryBeat->>check_imap_bounces: trigger (every 900s)
check_imap_bounces->>check_imap_bounces: query ConnectedEmailAccount with imap_host
loop For each IMAP account
check_imap_bounces->>check_imap_bounces: decrypt imap_password using EncryptedTextField
check_imap_bounces->>IMAP4_SSL: login(imap_host, imap_port, imap_username, password)
check_imap_bounces->>IMAP4_SSL: SEARCH UNSEEN messages in INBOX
IMAP4_SSL-->>check_imap_bounces: message IDs
check_imap_bounces->>IMAP4_SSL: FETCH RFC822 for each message
IMAP4_SSL-->>check_imap_bounces: raw email bytes
check_imap_bounces->>check_imap_bounces: parse subject, extract bounced recipient email
check_imap_bounces->>CampaignLead: find by recipient email+connected_account (case-insensitive)
check_imap_bounces->>CampaignLead: update status=BOUNCED, trigger completion check
check_imap_bounces->>IMAP4_SSL: STORE message +FLAGS \Seen
end
check_imap_bounces-->>CeleryBeat: return "Processed N IMAP bounces."
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@Kuldeeep18 ready for your review! |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/campaigns/models.py`:
- Line 28: The imap_password field in the Campaign model is storing credentials
as plaintext, which is a critical security vulnerability. Replace the plain
CharField definition for imap_password with an encrypted field type from
django-encrypted-model-fields library or similar encryption solution. First
install the library, then change the imap_password field type from
models.CharField to the appropriate encrypted field type provided by the library
(typically EncryptedCharField), which will transparently encrypt the password
when storing it and decrypt it when retrieving it from the database.
In `@backend/campaigns/tasks.py`:
- Line 742: The imaplib.IMAP4_SSL connection in the account processing logic is
created without a timeout parameter, which can cause indefinite hangs if the
remote server is unresponsive or network issues occur. Add a timeout parameter
to the IMAP4_SSL constructor call to ensure that connection attempts fail
gracefully after a reasonable time period rather than blocking the Celery worker
indefinitely and preventing other accounts from being processed.
- Around line 796-804: The try-except blocks in the IMAP cleanup section (where
imap_conn.close() and imap_conn.logout() are called) silently suppress
exceptions with pass statements, making it impossible to debug connection or
resource issues. Replace each pass statement with a logging call that captures
the exception details at debug or warning level. For both the close() and
logout() calls, add a logger.debug or logger.warning statement (with the
exception included) so that cleanup failures are visible in logs while still
allowing the task to complete without failing on cleanup errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e1f538d9-b04b-4e07-bb53-f20a68b6f598
📒 Files selected for processing (5)
backend/backend/settings.pybackend/campaigns/migrations/0009_connectedemailaccount_imap_fields.pybackend/campaigns/models.pybackend/campaigns/tasks.pybackend/db.sqlite3
🚀 PR Guidelines — Read Before Raising a PRHey contributors 👋 I’m LeadOrbit's Bot, and I’ll review every PR before it gets merged. ✅ Your PR will only be merged if:
❌ PRs that may be rejected:
Before submitting:
And if you find the project useful, consider ⭐ starring the repository — it helps the project grow and motivates further development. Quality contributions > PR count. |
- Encrypt imap_password using project's existing encryption pattern - Add timeout to IMAP4_SSL connection to prevent indefinite hangs - Log exceptions during IMAP cleanup instead of silently swallowing them - Add unit tests for check_imap_bounces with mocked IMAP server (5 tests, all passing)
|
@CodeRabbit @Kuldeeep18 All CodeRabbit review comments addressed:
Ready for your review! |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and review comments
gh pr view 299 --repo Kuldeeep18/LeadOrbit --json title,state,reviews,comments
# Get review comments (inline)
gh api repos/Kuldeeep18/LeadOrbit/pulls/299/comments --jq '[.[] | {id, body, path, line, user: .user.login, resolved: .resolved}]' 2>/dev/null | head -200
# Get issue comments
gh api repos/Kuldeeep18/LeadOrbit/issues/299/comments --jq '[.[] | {id, body: .body[:300], user: .user.login}]' 2>/dev/null | head -200Length of output: 34851
A new review is currently being processed for the updated push (covering One thing to be aware of ahead of merge: the Docstring Coverage pre-merge check is currently failing at 22.22% against the required 80% threshold. You may want to add docstrings to the new functions in (◕‿◕✿) 🐇 ✅ Action performedReview finished.
|
- Add docstrings to all helper functions in check_imap_bounces flow - Add docstrings to encryption.py and fields.py - Add class docstring to ConnectedEmailAccount
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/campaigns/fields.py (1)
22-24: 💤 Low valuePrefix unused variable with underscore to satisfy linter.
Ruff flags
pathas unpacked but unused.♻️ Suggested fix
def deconstruct(self): - name, path, args, kwargs = super().deconstruct() + name, _path, args, kwargs = super().deconstruct() return name, 'campaigns.fields.EncryptedTextField', args, kwargs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/campaigns/fields.py` around lines 22 - 24, The deconstruct method in the EncryptedTextField class unpacks the path variable from the super().deconstruct() call but never uses it, which causes the Ruff linter to flag it as unused. Prefix the unused path variable with an underscore (change it to _path) in the unpacking statement to indicate that it is intentionally unused and satisfy the linter.Source: Linters/SAST tools
backend/campaigns/encryption.py (1)
9-18: 💤 Low valueConsider validating key format at module load time.
If
FIELD_ENCRYPTION_KEYis set but malformed (not a valid URL-safe base64-encoded 32-byte key), Fernet will raiseValueErroron first use rather than at startup. This could cause cryptic runtime failures during the first encrypt/decrypt call.♻️ Optional: Add early key validation
def _get_fernet_key_bytes(): configured = (getattr(settings, 'FIELD_ENCRYPTION_KEY', '') or '').strip() if configured: - return configured.encode('utf-8') + key_bytes = configured.encode('utf-8') + # Validate key format early to fail fast on misconfiguration + try: + Fernet(key_bytes) + except ValueError as e: + raise ImproperlyConfigured( + f'FIELD_ENCRYPTION_KEY is invalid: {e}. ' + 'Generate one with: python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())"' + ) + return key_bytes if settings.DEBUG:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/campaigns/encryption.py` around lines 9 - 18, The _get_fernet_key_bytes function returns encryption keys without validating their format, which causes Fernet to raise ValueError on first encrypt/decrypt call if the key is malformed. Add validation in the _get_fernet_key_bytes function to verify that the returned key is a valid Fernet-compatible key (a 32-byte URL-safe base64-encoded string) by attempting to instantiate a Fernet object with it and catching any ValueError to provide a clear ImproperlyConfigured error at module load time instead of allowing cryptic runtime failures later.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/campaigns/migrations/0010_encrypt_imap_password.py`:
- Around line 22-28: The operations list has the wrong order, causing double
encryption of the imap_password field. The AlterField operation converts the
field to EncryptedTextField before RunPython executes, so when account.save() is
called in encrypt_existing_imap_passwords, Django's get_prep_value()
automatically encrypts the already-encrypted value. Swap the order of the two
operations in the operations list so that
migrations.RunPython(encrypt_existing_imap_passwords, migrations.RunPython.noop)
runs first (while the field is still plain TextField), and migrations.AlterField
runs second (after manual encryption is complete).
In `@requirements.txt`:
- Line 16: The cryptography package is pinned to version 43.x which contains
multiple unpatched security vulnerabilities (GHSA-537c-gmf6-5ccf,
GHSA-r6ph-v2qm-q3c2, and others). Update the version constraint in the
cryptography dependency to use a current stable version that includes these
security patches, such as version 49.0.0 or later, to ensure the application is
protected against these known vulnerabilities.
---
Nitpick comments:
In `@backend/campaigns/encryption.py`:
- Around line 9-18: The _get_fernet_key_bytes function returns encryption keys
without validating their format, which causes Fernet to raise ValueError on
first encrypt/decrypt call if the key is malformed. Add validation in the
_get_fernet_key_bytes function to verify that the returned key is a valid
Fernet-compatible key (a 32-byte URL-safe base64-encoded string) by attempting
to instantiate a Fernet object with it and catching any ValueError to provide a
clear ImproperlyConfigured error at module load time instead of allowing cryptic
runtime failures later.
In `@backend/campaigns/fields.py`:
- Around line 22-24: The deconstruct method in the EncryptedTextField class
unpacks the path variable from the super().deconstruct() call but never uses it,
which causes the Ruff linter to flag it as unused. Prefix the unused path
variable with an underscore (change it to _path) in the unpacking statement to
indicate that it is intentionally unused and satisfy the linter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 87a684f2-33f9-49c2-be0d-d9f7c182620a
📒 Files selected for processing (9)
backend/backend/settings.pybackend/campaigns/encryption.pybackend/campaigns/fields.pybackend/campaigns/migrations/0010_encrypt_imap_password.pybackend/campaigns/models.pybackend/campaigns/tasks.pybackend/campaigns/tests.pybackend/db.sqlite3requirements.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/backend/settings.py
- backend/campaigns/models.py
|
@coderabbitai Added docstrings to all new functions to address the docstring coverage check. Ready for re-review. |
|
✅ Action performedReview finished.
|
- Swap migration order so passwords are encrypted before AlterField, fixing double-encryption bug - Use queryset.update() to bypass model save hooks during migration - Upgrade cryptography to >=49.0,<50.0 to address known vulnerabilities in 43.x
|
@coderabbitai
All 5 tests still passing. Ready for re-review 🙏 |
|
Triggering a fresh review now to verify the latest changes. 🐇✨ ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/campaigns/models.py (1)
33-33:⚠️ Potential issue | 🟡 MinorConstrain
imap_portto valid TCP port values.
IntegerFieldaccepts0, negatives, and values above65535, which will causeimaplib.IMAP4_SSL()to fail when the periodic IMAP bounce check (check_imap_bouncestask) attempts to connect. Add range validation viaPositiveIntegerFieldwith explicit validators and generate a new migration to update the existing field.Proposed fix
from django.db import models from django.conf import settings +from django.core.validators import MaxValueValidator, MinValueValidator from tenants.models import TenantModel from leads.models import Lead from .fields import EncryptedTextField import uuid @@ - imap_port = models.IntegerField(default=993) + imap_port = models.PositiveIntegerField( + default=993, + validators=[MinValueValidator(1), MaxValueValidator(65535)], + )Ensure the migration includes the field type and validator changes. Consider adding a DB
CheckConstraintfor additional safety if any code path bypasses ORM validation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/campaigns/models.py` at line 33, The imap_port field in the Campaign model uses IntegerField which accepts invalid port values (negatives, zero, and values above 65535) that will cause imaplib.IMAP4_SSL() to fail when the check_imap_bounces task attempts to connect. Replace the IntegerField with PositiveIntegerField for the imap_port field and add explicit validators such as MaxValueValidator to ensure the port does not exceed 65535. After making the field definition change, generate a new database migration that captures the field type change and validator additions. Additionally, consider adding a database-level CheckConstraint to the model's Meta class for extra safety against values that might bypass ORM validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/campaigns/fields.py`:
- Line 54: In the deconstruct method where super().deconstruct() is called, the
unpacked path variable is intentionally ignored because a hardcoded field path
is returned instead. Prefix this variable with an underscore by changing path to
_path in the tuple unpacking to indicate it is intentionally discarded and
satisfy the Ruff RUF059 lint rule.
---
Outside diff comments:
In `@backend/campaigns/models.py`:
- Line 33: The imap_port field in the Campaign model uses IntegerField which
accepts invalid port values (negatives, zero, and values above 65535) that will
cause imaplib.IMAP4_SSL() to fail when the check_imap_bounces task attempts to
connect. Replace the IntegerField with PositiveIntegerField for the imap_port
field and add explicit validators such as MaxValueValidator to ensure the port
does not exceed 65535. After making the field definition change, generate a new
database migration that captures the field type change and validator additions.
Additionally, consider adding a database-level CheckConstraint to the model's
Meta class for extra safety against values that might bypass ORM validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52ecf859-229c-4e82-8bcf-3804809e95fe
📒 Files selected for processing (4)
backend/campaigns/encryption.pybackend/campaigns/fields.pybackend/campaigns/models.pybackend/campaigns/tasks.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/campaigns/tasks.py
- backend/campaigns/encryption.py
|
@Kuldeeep18 kindly review the PR,all coderabbitai requirements have been met. |
2 participants
Description
Implements a periodic Celery task that connects to IMAP mailboxes and automatically detects/processes bounce notifications for accounts without webhook delivery.
Type of Change
✅ New feature
Related Issue
Closes #123
What's Changed
No new dependencies — uses Python's built-in imaplib and email stdlib only
Verification
✅ Migration applies cleanly (python manage.py migrate campaigns)
✅ python manage.py check passes with no issues
✅ Follows existing @shared_task, logger, and tenant-scoping patterns
✅ Existing tasks.py functionality untouched
Closes #123
Summary by CodeRabbit