feat: add lead tracking and engagement analytics (issue #134)#468
Open
Garcia-786 wants to merge 1 commit into
Open
feat: add lead tracking and engagement analytics (issue #134)#468Garcia-786 wants to merge 1 commit into
Garcia-786 wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a ChangesLead Engagement Event Tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
backend/leads/migrations/0006_leadengagementevent.py (1)
30-33: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd an organization-leading index for the timeline query.
The new API scopes every read by
organizationbefore applying optional filters, but this migration only addslead/campaign/event_typecomposite indexes. The default org timeline (WHERE organization_id = ? ORDER BY occurred_at DESC) will still require a sort for larger tenants. Please add at least(organization, occurred_at)here, and mirror it inLeadEngagementEvent.Meta.indexes.🤖 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/migrations/0006_leadengagementevent.py` around lines 30 - 33, Add an organization-leading composite index for the timeline query: the migration for LeadEngagementEvent currently only defines lead/campaign/event_type indexes, so update the indexes list to include an index on organization and occurred_at for the default org-scoped ordering path. Also mirror the same index in LeadEngagementEvent.Meta.indexes so the model definition and migration stay aligned.
🤖 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 616-623: The engagement-event write in the send path can trigger a
retry of the email send if LeadEngagementEvent.objects.create() fails, because
the surrounding exception handler restores next_execution_time; update the send
flow in the task handling the EMAIL_SENT path so analytics recording is
best-effort or moved outside the retryable block. Use the existing send logic
around LeadEngagementEvent.objects.create(), _advance_to_next_step(), and the
outer except in the campaign task to ensure a provider send is not retried just
because event logging fails, including the mock-path handling noted in the
related block.
In `@backend/campaigns/views.py`:
- Around line 484-492: Authenticate the public email webhook in WebhookView
before creating LeadEngagementEvent records, since the current event handling
path accepts request-supplied event/email/message_id and can be spoofed. Update
the webhook entry point and the event creation flow around
LeadEngagementEvent.objects.create to validate the request with a shared
secret/signature or equivalent trust check before writing any engagement data.
In `@backend/leads/models.py`:
- Around line 109-117: `LeadEngagementEvent` currently allows `organization`,
`lead`, and `campaign` to be saved without checking they belong to the same
tenant, which can create cross-tenant records. Add a model-level validation path
on `LeadEngagementEvent` (for example in `clean()`, `save()`, or a dedicated
creation helper) that verifies `self.organization` matches `lead.organization`
and, when present, `campaign.organization` before persistence. Make sure the
check rejects mismatched tenants early so any callers creating events through
`LeadEngagementEvent` cannot bypass the invariant.
In `@backend/leads/serializers.py`:
- Around line 16-21: The LeadEngagementEvent serialization is exposing campaign
as the default primary key, but the frontend expects campaign details like name.
Update the LeadEngagementEvent serializer in the Meta fields handling so
campaign is serialized as a nested object or otherwise includes the campaign
name instead of only the UUID, using the LeadEngagementEvent serializer and its
campaign relation as the target for the fix.
In `@frontend/leads.html`:
- Around line 635-645: openLeadEngagement should ignore stale fetch responses so
a slower request for one lead cannot overwrite the modal for a newer lead. Add a
request token or AbortController inside openLeadEngagement, and before updating
lead-engagement-body or lead-engagement-meta, verify the response still belongs
to the latest leadId/open invocation. Apply the same guard to the later modal
update path in the engagement rendering logic so mismatched metadata and events
cannot be shown.
- Around line 663-666: The campaign cell rendering in the events modal is using
`ev.campaign.name`, but `LeadEngagementEventSerializer` only exposes `campaign`
as the foreign key value, not an object. Update the frontend mapping in the
modal generation logic to match the API contract by either rendering the
campaign ID/value directly or switching to a serializer field that returns a
display name. Keep the logic in the `events.map(...)` block aligned with the
data shape returned by `LeadEngagementEventSerializer` so `escapeHtml` only
receives a valid string.
---
Nitpick comments:
In `@backend/leads/migrations/0006_leadengagementevent.py`:
- Around line 30-33: Add an organization-leading composite index for the
timeline query: the migration for LeadEngagementEvent currently only defines
lead/campaign/event_type indexes, so update the indexes list to include an index
on organization and occurred_at for the default org-scoped ordering path. Also
mirror the same index in LeadEngagementEvent.Meta.indexes so the model
definition and migration stay aligned.
🪄 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: 2c26e658-5cab-462c-ad7f-3c6eedaaedb2
📒 Files selected for processing (8)
backend/backend/urls.pybackend/campaigns/tasks.pybackend/campaigns/views.pybackend/leads/migrations/0006_leadengagementevent.pybackend/leads/models.pybackend/leads/serializers.pybackend/leads/views.pyfrontend/leads.html
Comment on lines
+616
to
+623
| LeadEngagementEvent.objects.create( | ||
| organization=clead.organization, | ||
| lead=clead.lead, | ||
| campaign=clead.campaign, | ||
| event_type='EMAIL_SENT', | ||
| occurred_at=timezone.now(), | ||
| metadata={'message_id': message_id, 'provider': account.provider}, | ||
| ) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Keep engagement-event writes out of the send retry path.
If LeadEngagementEvent.objects.create() fails after a real provider send, the except restores next_execution_time, so the same email can be sent again. In the mock path, the outer except skips _advance_to_next_step() after the send was claimed. Treat analytics recording as best-effort or move it to a separate non-retrying path.
Proposed fix
- LeadEngagementEvent.objects.create(
- organization=clead.organization,
- lead=clead.lead,
- campaign=clead.campaign,
- event_type='EMAIL_SENT',
- occurred_at=timezone.now(),
- metadata={'message_id': message_id, 'provider': account.provider},
- )
+ try:
+ LeadEngagementEvent.objects.create(
+ organization=clead.organization,
+ lead=clead.lead,
+ campaign=clead.campaign,
+ event_type='EMAIL_SENT',
+ occurred_at=timezone.now(),
+ metadata={'message_id': message_id, 'provider': account.provider},
+ )
+ except Exception:
+ logger.exception(
+ "Failed to record EMAIL_SENT engagement event for campaign_lead=%s",
+ clead.id,
+ )
@@
- LeadEngagementEvent.objects.create(
- organization=clead.organization,
- lead=clead.lead,
- campaign=clead.campaign,
- event_type='EMAIL_SENT',
- occurred_at=timezone.now(),
- metadata={'mock': True, 'subject': subject},
- )
+ try:
+ LeadEngagementEvent.objects.create(
+ organization=clead.organization,
+ lead=clead.lead,
+ campaign=clead.campaign,
+ event_type='EMAIL_SENT',
+ occurred_at=timezone.now(),
+ metadata={'mock': True, 'subject': subject},
+ )
+ except Exception:
+ logger.exception(
+ "Failed to record mock EMAIL_SENT engagement event for campaign_lead=%s",
+ clead.id,
+ )Also applies to: 633-640
🤖 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 616 - 623, The engagement-event
write in the send path can trigger a retry of the email send if
LeadEngagementEvent.objects.create() fails, because the surrounding exception
handler restores next_execution_time; update the send flow in the task handling
the EMAIL_SENT path so analytics recording is best-effort or moved outside the
retryable block. Use the existing send logic around
LeadEngagementEvent.objects.create(), _advance_to_next_step(), and the outer
except in the campaign task to ensure a provider send is not retried just
because event logging fails, including the mock-path handling noted in the
related block.
Comment on lines
+484
to
+492
| if event_type_mapped: | ||
| LeadEngagementEvent.objects.create( | ||
| organization=cl.organization, | ||
| lead=cl.lead, | ||
| campaign=cl.campaign, | ||
| event_type=event_type_mapped, | ||
| occurred_at=now, | ||
| metadata=metadata, | ||
| ) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether WebhookView has any signature/shared-secret validation before mutating campaign leads
# or creating LeadEngagementEvent rows.
ast-grep outline backend/campaigns/views.py --match WebhookView --view expanded
rg -n -C4 '(signature|secret|token|webhook|verify|HMAC|hmac|HTTP_|headers)' backend/campaigns/views.py backend/backend/urls.pyRepository: Kuldeeep18/LeadOrbit
Length of output: 17517
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the webhook view body and nearby helpers.
sed -n '367,507p' backend/campaigns/views.py
printf '\n---\n'
# Look for any webhook auth/verification wiring elsewhere in the repo.
rg -n -C3 '(WEBHOOK|webhook|signature|secret|shared[-_ ]secret|HMAC|hmac|verify|signer|BadSignature|X-Hub|Stripe-Signature|SendGrid|Mailgun|Postmark|Mailjet)' backendRepository: Kuldeeep18/LeadOrbit
Length of output: 21061
Authenticate the email webhook before writing engagement events. WebhookView is public (AllowAny) and writes LeadEngagementEvent rows from request-supplied event, email, and message_id, so anyone who knows a lead email can spoof opens/clicks/replies/bounces into engagement history.
🤖 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/views.py` around lines 484 - 492, Authenticate the public
email webhook in WebhookView before creating LeadEngagementEvent records, since
the current event handling path accepts request-supplied event/email/message_id
and can be spoofed. Update the webhook entry point and the event creation flow
around LeadEngagementEvent.objects.create to validate the request with a shared
secret/signature or equivalent trust check before writing any engagement data.
Comment on lines
+109
to
+117
| id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False) | ||
| lead = models.ForeignKey(Lead, on_delete=models.CASCADE, related_name='engagement_events') | ||
| campaign = models.ForeignKey( | ||
| 'campaigns.Campaign', | ||
| on_delete=models.SET_NULL, | ||
| null=True, | ||
| blank=True, | ||
| related_name='engagement_events', | ||
| ) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Enforce tenant consistency between organization, lead, and campaign.
LeadEngagementEvent duplicates tenant identity but never verifies that lead.organization, campaign.organization, and the inherited organization field match. That becomes a cross-tenant leak because LeadEngagementEventViewSet reads by event.organization only, while writers create rows with those references independently. Please enforce this invariant at the model boundary (for example in clean()/save flow or a dedicated factory) and reject mismatched campaign/lead tenants before persistence.
🧰 Tools
🪛 ast-grep (0.44.0)
[info] 117-117: use help_text to document model columns
Context: models.CharField(max_length=20, choices=EVENT_TYPE_CHOICES)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
🤖 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 109 - 117, `LeadEngagementEvent`
currently allows `organization`, `lead`, and `campaign` to be saved without
checking they belong to the same tenant, which can create cross-tenant records.
Add a model-level validation path on `LeadEngagementEvent` (for example in
`clean()`, `save()`, or a dedicated creation helper) that verifies
`self.organization` matches `lead.organization` and, when present,
`campaign.organization` before persistence. Make sure the check rejects
mismatched tenants early so any callers creating events through
`LeadEngagementEvent` cannot bypass the invariant.
Comment on lines
+16
to
+21
| class Meta: | ||
| model = LeadEngagementEvent | ||
| fields = [ | ||
| 'id', 'lead', 'campaign', 'event_type', 'event_type_display', | ||
| 'occurred_at', 'metadata', 'created_at', | ||
| ] |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return campaign details, not just the campaign UUID.
frontend/leads.html:655-664 renders ev.campaign.name, but this serializer exposes campaign as the default PK field. Any event linked to a campaign will therefore serialize campaign as a UUID string, so the modal cannot render the campaign name correctly.
Proposed fix
class LeadEngagementEventSerializer(serializers.ModelSerializer):
event_type_display = serializers.CharField(source='get_event_type_display', read_only=True)
+ campaign = serializers.SerializerMethodField()
class Meta:
model = LeadEngagementEvent
fields = [
'id', 'lead', 'campaign', 'event_type', 'event_type_display',
'occurred_at', 'metadata', 'created_at',
]
read_only_fields = ['organization']
+
+ def get_campaign(self, obj):
+ if not obj.campaign_id:
+ return None
+ return {
+ 'id': str(obj.campaign_id),
+ 'name': obj.campaign.name,
+ }📝 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
| class Meta: | |
| model = LeadEngagementEvent | |
| fields = [ | |
| 'id', 'lead', 'campaign', 'event_type', 'event_type_display', | |
| 'occurred_at', 'metadata', 'created_at', | |
| ] | |
| class LeadEngagementEventSerializer(serializers.ModelSerializer): | |
| event_type_display = serializers.CharField(source='get_event_type_display', read_only=True) | |
| campaign = serializers.SerializerMethodField() | |
| class Meta: | |
| model = LeadEngagementEvent | |
| fields = [ | |
| 'id', 'lead', 'campaign', 'event_type', 'event_type_display', | |
| 'occurred_at', 'metadata', 'created_at', | |
| ] | |
| read_only_fields = ['organization'] | |
| def get_campaign(self, obj): | |
| if not obj.campaign_id: | |
| return None | |
| return { | |
| 'id': str(obj.campaign_id), | |
| 'name': obj.campaign.name, | |
| } |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 18-21: Mutable default value for class attribute
(RUF012)
🤖 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 16 - 21, The LeadEngagementEvent
serialization is exposing campaign as the default primary key, but the frontend
expects campaign details like name. Update the LeadEngagementEvent serializer in
the Meta fields handling so campaign is serialized as a nested object or
otherwise includes the campaign name instead of only the UUID, using the
LeadEngagementEvent serializer and its campaign relation as the target for the
fix.
Comment on lines
+635
to
+645
| async function openLeadEngagement(leadId, leadEmail) { | ||
| const modalBody = document.getElementById('lead-engagement-body'); | ||
| const modalMeta = document.getElementById('lead-engagement-meta'); | ||
| modalBody.innerHTML = '<tr><td colspan="4" class="text-center py-4 text-muted"><div class="spinner-border spinner-border-sm text-primary me-2" role="status" aria-label="Loading engagement"></div>Loading...</td></tr>'; | ||
| modalMeta.textContent = `${leadEmail}`; | ||
| bootstrap.Modal.getOrCreateInstance(document.getElementById('leadEngagementModal')).show(); | ||
|
|
||
| try { | ||
| const res = await fetchWithAuth(`/lead-engagement-events/?lead_id=${leadId}`); | ||
| if (!res.ok) throw new Error('Failed to load engagement events'); | ||
| const events = await res.json(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Ignore stale engagement responses.
If a user opens lead A and then lead B before A finishes, the slower response can overwrite the modal with the wrong timeline. That leaves the modal showing mismatched lead metadata vs. events. Guard this with a request token or cancel the previous fetch.
Suggested guard
+ let leadEngagementRequestId = 0;
+
async function openLeadEngagement(leadId, leadEmail) {
+ const requestId = ++leadEngagementRequestId;
const modalBody = document.getElementById('lead-engagement-body');
const modalMeta = document.getElementById('lead-engagement-meta');
modalBody.innerHTML = '<tr><td colspan="4" class="text-center py-4 text-muted"><div class="spinner-border spinner-border-sm text-primary me-2" role="status" aria-label="Loading engagement"></div>Loading...</td></tr>';
modalMeta.textContent = `${leadEmail}`;
bootstrap.Modal.getOrCreateInstance(document.getElementById('leadEngagementModal')).show();
try {
const res = await fetchWithAuth(`/lead-engagement-events/?lead_id=${leadId}`);
if (!res.ok) throw new Error('Failed to load engagement events');
const events = await res.json();
+ if (requestId !== leadEngagementRequestId) return;
if (!events.length) {
modalBody.innerHTML = '<tr><td colspan="4" class="text-center py-4 text-muted">No engagement events found.</td></tr>';
return;
}
@@
} catch (e) {
+ if (requestId !== leadEngagementRequestId) return;
console.error(e);
modalBody.innerHTML = '<tr><td colspan="4" class="text-center py-4 text-danger">Failed to load engagement events.</td></tr>';
}
}Also applies to: 663-681
🤖 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 `@frontend/leads.html` around lines 635 - 645, openLeadEngagement should ignore
stale fetch responses so a slower request for one lead cannot overwrite the
modal for a newer lead. Add a request token or AbortController inside
openLeadEngagement, and before updating lead-engagement-body or
lead-engagement-meta, verify the response still belongs to the latest
leadId/open invocation. Apply the same guard to the later modal update path in
the engagement rendering logic so mismatched metadata and events cannot be
shown.
Comment on lines
+663
to
+666
| modalBody.innerHTML = events.map(ev => { | ||
| const icon = eventIcons[ev.event_type] || 'bi-circle'; | ||
| const time = ev.occurred_at ? new Date(ev.occurred_at).toLocaleString() : '-'; | ||
| const campaign = ev.campaign ? `<a href="/campaigns.html">${escapeHtml(ev.campaign.name)}</a>` : '<span class="text-muted">-</span>'; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the campaign cell with the API contract.
LeadEngagementEventSerializer currently returns campaign as the FK value, not an object with name (backend/leads/serializers.py, Lines 13-23). ev.campaign.name therefore never exists here, so the Campaign column will render incorrectly and may throw if escapeHtml requires a string. Either add a display field on the API or render the ID only.
Suggested contract fix
# backend/leads/serializers.py
class LeadEngagementEventSerializer(serializers.ModelSerializer):
event_type_display = serializers.CharField(source='get_event_type_display', read_only=True)
+ campaign_name = serializers.CharField(source='campaign.name', read_only=True)
class Meta:
model = LeadEngagementEvent
fields = [
'id', 'lead', 'campaign', 'event_type', 'event_type_display',
- 'occurred_at', 'metadata', 'created_at',
+ 'occurred_at', 'metadata', 'created_at', 'campaign_name',
]- const campaign = ev.campaign ? `<a href="/campaigns.html">${escapeHtml(ev.campaign.name)}</a>` : '<span class="text-muted">-</span>';
+ const campaign = ev.campaign_name ? `<a href="/campaigns.html">${escapeHtml(ev.campaign_name)}</a>` : '<span class="text-muted">-</span>';📝 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
| modalBody.innerHTML = events.map(ev => { | |
| const icon = eventIcons[ev.event_type] || 'bi-circle'; | |
| const time = ev.occurred_at ? new Date(ev.occurred_at).toLocaleString() : '-'; | |
| const campaign = ev.campaign ? `<a href="/campaigns.html">${escapeHtml(ev.campaign.name)}</a>` : '<span class="text-muted">-</span>'; | |
| modalBody.innerHTML = events.map(ev => { | |
| const icon = eventIcons[ev.event_type] || 'bi-circle'; | |
| const time = ev.occurred_at ? new Date(ev.occurred_at).toLocaleString() : '-'; | |
| const campaign = ev.campaign_name ? `<a href="/campaigns.html">${escapeHtml(ev.campaign_name)}</a>` : '<span class="text-muted">-</span>'; |
🤖 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 `@frontend/leads.html` around lines 663 - 666, The campaign cell rendering in
the events modal is using `ev.campaign.name`, but
`LeadEngagementEventSerializer` only exposes `campaign` as the foreign key
value, not an object. Update the frontend mapping in the modal generation logic
to match the API contract by either rendering the campaign ID/value directly or
switching to a serializer field that returns a display name. Keep the logic in
the `events.map(...)` block aligned with the data shape returned by
`LeadEngagementEventSerializer` so `escapeHtml` only receives a valid string.
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.
Related Issue
Closes #134
Summary of Changes
This pull request introduces comprehensive lead tracking and engagement analytics by implementing a central event-logging system and connecting it to campaign tasks and webhooks.
Backend Changes
LeadEngagementEventmodel to log communication events (such as emails sent, opened, clicked, replied, and bounced) with relational tracking for leads, campaigns, and organizations.LeadEngagementEventViewSetto allow filtered queries of engagement history by lead, campaign, or event type.Frontend Changes
Type of Change
Testing
python manage.py migrate).Summary by CodeRabbit