LO-030: Global Unsubscribe Domain List – BlockedDomain model, API, and campaign guard#306
Open
Anushreer22 wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughFour previously separate Django migrations are removed and replaced by a single consolidated migration. ChangesBlockedDomain Refactor and Campaign Send Guard
Sequence Diagram(s)sequenceDiagram
participant Celery as Celery Worker
participant Task as send_email_step
participant DB as BlockedDomain (DB)
participant Lead as Lead Model
Celery->>Task: execute(lead_id, campaign_id)
Task->>Task: extract domain from lead.email
Task->>DB: filter(domain=domain, organization=campaign.org)
alt Domain is blocked
DB-->>Task: record found
Task->>Lead: status = SKIPPED, save()
Task-->>Celery: return (skipped)
else Domain not blocked
DB-->>Task: no record
Task->>Task: proceed with global_unsubscribe check
Task->>Task: send / advance step logic
Task-->>Celery: return (sent or advanced)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
backend/leads/serializers.py (1)
79-112:⚠️ Potential issue | 🔴 CriticalRemove the duplicate serializer class definition and merge field/read_only changes into the existing one.
Lines 108–112 redefine
BlockedDomainSerializer, causing Python to discard the first definition (lines 79–106) entirely. This dropsvalidate_domain()and the duplicate-domain validation check. Withoutvalidate_domain(), the API persists raw domains likeHTTPS://Competitor.COM/path, while the domain lookup (and any campaign guards) normalize them first, creating a mismatch that can bypass filters.Suggested fix
class BlockedDomainSerializer(serializers.ModelSerializer): class Meta: model = BlockedDomain - fields = ['id', 'domain', 'created_at', 'updated_at'] - read_only_fields = ['id', 'created_at', 'updated_at'] + fields = ['id', 'domain', 'organization', 'created_at'] + read_only_fields = ['id', 'organization', 'created_at'] def validate_domain(self, value): try: return validate_domain(value) except DjangoValidationError as exc: raise serializers.ValidationError(exc.messages) def validate(self, attrs): request = self.context.get('request') organization = getattr(getattr(request, 'user', None), 'organization', None) domain = attrs.get('domain', getattr(self.instance, 'domain', None)) if organization and domain: existing = BlockedDomain.objects.filter( organization=organization, domain=domain, ) if self.instance: existing = existing.exclude(id=self.instance.id) if existing.exists(): raise serializers.ValidationError({'domain': 'This domain is already blocked.'}) return attrs - - -class BlockedDomainSerializer(serializers.ModelSerializer): - class Meta: - model = BlockedDomain - fields = ['id', 'domain', 'organization', 'created_at'] - read_only_fields = ['id', 'organization', 'created_at']🤖 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/leads/serializers.py` around lines 79 - 112, The BlockedDomainSerializer class is defined twice, with the second definition overwriting the first and discarding all validation logic. Merge the two class definitions by keeping the first BlockedDomainSerializer (which contains validate_domain and validate methods) and update its Meta class fields to include 'organization' and adjust the read_only_fields list to match the second definition. Delete the duplicate BlockedDomainSerializer class definition that appears after the first one, ensuring all validation methods (validate_domain and validate for duplicate checks) are preserved in the final single class definition.backend/leads/models.py (1)
80-110:⚠️ Potential issue | 🟠 MajorRemove the duplicate
BlockedDomainclass definition and consolidate to a single model.The first
BlockedDomain(TenantModel)definition (lines 80–95) is shadowed by the secondclass BlockedDomain(models.Model)definition (lines 96–110). Django registers both classes during module import, leaving stale model-registration state. The second definition also loses the domain validation logic (cleanandsavemethods) from the first. Keep the second definition with explicitorganizationFK but restore the validation methods.Suggested consolidation
-class BlockedDomain(TenantModel): - domain = models.CharField(max_length=255) - - class Meta: - unique_together = ('organization', 'domain') - ordering = ['domain'] - - def clean(self): - self.domain = validate_domain(self.domain) - - def save(self, *args, **kwargs): - self.domain = validate_domain(self.domain) - super().save(*args, **kwargs) - - def __str__(self): - return self.domain class BlockedDomain(models.Model): domain = models.CharField(max_length=255) organization = models.ForeignKey( - 'tenants.Organization', # adjust if path differs + 'tenants.Organization', on_delete=models.CASCADE, related_name='blocked_domains' ) created_at = models.DateTimeField(auto_now_add=True) class Meta: unique_together = ('domain', 'organization') ordering = ['domain'] + + def clean(self): + self.domain = validate_domain(self.domain) + + def save(self, *args, **kwargs): + self.domain = validate_domain(self.domain) + super().save(*args, **kwargs) def __str__(self): return f"{self.domain} (Org: {self.organization_id})"🤖 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/leads/models.py` around lines 80 - 110, Remove the first BlockedDomain class definition that inherits from TenantModel (the one without the explicit organization FK) to eliminate the duplicate class definition that shadows the second one. Keep the second BlockedDomain class that inherits from models.Model and explicitly defines the organization ForeignKey and created_at fields, but restore the domain validation logic by adding back the clean() method and save() method from the first definition that call validate_domain() on the domain field to ensure the validation is preserved in the consolidated model.
🤖 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/tasks.py`:
- Around line 434-444: Remove the dedented global unsubscribe domain check block
that appears after the try body at lines 434-444. This block is causing a
SyntaxError because it's at function level immediately after try without an
except or finally clause. Instead of this duplicate inline logic, use the
existing _skip_blocked_domain_lead() helper function which properly handles all
necessary cleanup including clearing current_step, next_execution_time, and
marking campaign completion.
---
Outside diff comments:
In `@backend/leads/models.py`:
- Around line 80-110: Remove the first BlockedDomain class definition that
inherits from TenantModel (the one without the explicit organization FK) to
eliminate the duplicate class definition that shadows the second one. Keep the
second BlockedDomain class that inherits from models.Model and explicitly
defines the organization ForeignKey and created_at fields, but restore the
domain validation logic by adding back the clean() method and save() method from
the first definition that call validate_domain() on the domain field to ensure
the validation is preserved in the consolidated model.
In `@backend/leads/serializers.py`:
- Around line 79-112: The BlockedDomainSerializer class is defined twice, with
the second definition overwriting the first and discarding all validation logic.
Merge the two class definitions by keeping the first BlockedDomainSerializer
(which contains validate_domain and validate methods) and update its Meta class
fields to include 'organization' and adjust the read_only_fields list to match
the second definition. Delete the duplicate BlockedDomainSerializer class
definition that appears after the first one, ensuring all validation methods
(validate_domain and validate for duplicate checks) are preserved in the final
single class definition.
🪄 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: 3339eee0-e02f-4404-afaa-a6290e087253
⛔ Files ignored due to path filters (2)
backend/leads/migrations/__pycache__/0001_initial.cpython-314.pycis excluded by!**/*.pycbackend/leads/migrations/__pycache__/__init__.cpython-314.pycis excluded by!**/*.pyc
📒 Files selected for processing (10)
backend/campaigns/tasks.pybackend/leads/migrations/0002_blockeddomain.pybackend/leads/migrations/0002_lead_custom_variables.pybackend/leads/migrations/0002_lead_custom_variables_tag_color_leadimportjob_and_more.pybackend/leads/migrations/0003_merge_0002_blockeddomain_0002_lead_custom_variables.pybackend/leads/migrations/0004_leadimportjob.pybackend/leads/migrations/0004_tag_color.pybackend/leads/migrations/0005_merge_0004_leadimportjob_0004_tag_color.pybackend/leads/models.pybackend/leads/serializers.py
💤 Files with no reviewable changes (6)
- backend/leads/migrations/0003_merge_0002_blockeddomain_0002_lead_custom_variables.py
- backend/leads/migrations/0005_merge_0004_leadimportjob_0004_tag_color.py
- backend/leads/migrations/0002_lead_custom_variables.py
- backend/leads/migrations/0004_leadimportjob.py
- backend/leads/migrations/0004_tag_color.py
- backend/leads/migrations/0002_blockeddomain.py
Comment on lines
+434
to
+444
| # --- Global unsubscribe domain check --- | ||
| domain = clead.lead.email.split('@')[-1].lower() | ||
| if BlockedDomain.objects.filter( | ||
| domain=domain, | ||
| organization=clead.campaign.organization | ||
| ).exists(): | ||
| clead.status = 'SKIPPED' | ||
| clead.save(update_fields=['status']) | ||
| logger.info(f"Skipping lead {clead.lead.email}: domain {domain} is blocked") | ||
| return | ||
| # --- end domain check --- |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import Path
import ast
path = Path("backend/campaigns/tasks.py")
ast.parse(path.read_text(), filename=str(path))
print("syntax ok")
PYRepository: Kuldeeep18/LeadOrbit
Length of output: 452
🏁 Script executed:
cd backend/campaigns && head -n 450 tasks.py | tail -n 40 | cat -nRepository: Kuldeeep18/LeadOrbit
Length of output: 1794
🏁 Script executed:
rg "_skip_blocked_domain_lead" backend/campaigns/tasks.py -A 10Repository: Kuldeeep18/LeadOrbit
Length of output: 945
Remove the dedented blocked-domain block at lines 434-444.
The module fails to parse with SyntaxError: expected 'except' or 'finally' block because this block is dedented to function level immediately after the try body (line 419). This duplicate code also bypasses the existing _skip_blocked_domain_lead() helper, which properly clears current_step, next_execution_time, and marks campaign completion—none of which happen in this inline version.
Suggested fix
- # --- Global unsubscribe domain check ---
- domain = clead.lead.email.split('@')[-1].lower()
- if BlockedDomain.objects.filter(
- domain=domain,
- organization=clead.campaign.organization
- ).exists():
- clead.status = 'SKIPPED'
- clead.save(update_fields=['status'])
- logger.info(f"Skipping lead {clead.lead.email}: domain {domain} is blocked")
- return
- # --- end domain check ---
-📝 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.
Suggested change
| # --- Global unsubscribe domain check --- | |
| domain = clead.lead.email.split('@')[-1].lower() | |
| if BlockedDomain.objects.filter( | |
| domain=domain, | |
| organization=clead.campaign.organization | |
| ).exists(): | |
| clead.status = 'SKIPPED' | |
| clead.save(update_fields=['status']) | |
| logger.info(f"Skipping lead {clead.lead.email}: domain {domain} is blocked") | |
| return | |
| # --- end domain check --- |
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 435-435: Expected except or finally after try block
(invalid-syntax)
🤖 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/tasks.py` around lines 434 - 444, Remove the dedented
global unsubscribe domain check block that appears after the try body at lines
434-444. This block is causing a SyntaxError because it's at function level
immediately after try without an except or finally clause. Instead of this
duplicate inline logic, use the existing _skip_blocked_domain_lead() helper
function which properly handles all necessary cleanup including clearing
current_step, next_execution_time, and marking campaign completion.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #209
This PR adds a global unsubscribe domain list, so that if an entire company domain is blocked, no email will be sent to any lead belonging to that domain within the user’s organisation.
Changes
1. Model
BlockedDomainmodel (leads/models.py)domain,organization(FK totenants.Organization),created_at(domain, organization)2. API Endpoints
BlockedDomainSerializer(leads/serializers.py)BlockedDomainViewSet(leads/views.py)/leads/api/blocked-domains/3. Campaign Sending Guard
send_email_steptask incampaigns/tasks.pySKIPPEDand the task returns earlyTesting
Files Modified
backend/leads/models.pybackend/leads/serializers.pybackend/leads/views.pybackend/leads/urls.py(created)backend/LeadOrbit/urls.pybackend/campaigns/tasks.pybackend/leads/migrations/(new migration)Summary by CodeRabbit
New Features
Improvements