feat(email): add project feedback check-in outreach flow#142
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant Scheduler as Scheduled Task
participant DB as Database
participant Queue as Task Queue
participant Email as Email Service
participant Tracking as Tracking Task
Scheduler->>DB: Query eligible profiles (2-day window, verified, has project)
DB-->>Scheduler: Return eligible profiles
Scheduler->>Scheduler: Filter out already-sent emails
loop For each eligible profile
Scheduler->>Queue: Enqueue send_project_feedback_checkin_email
end
Queue->>Email: Dequeue and send plain-text email
Email-->>Email: Compose email content
Email->>Email: Send via EmailMessage
Email->>Queue: Enqueue track_email_sent task
Queue->>Tracking: Dequeue and record EmailSent entry
Tracking->>DB: Store email sent record
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
Contributor
Greptile SummaryThis PR adds a new automated email outreach flow that sends personalized feedback check-ins to users who have registered within the past 2 days and created at least one project. The email is sent from Key changes:
The implementation follows the existing pattern used for Confidence Score: 5/5
Important Files Changed
Last reviewed commit: d343a13 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 1
🧹 Nitpick comments (3)
core/scheduled_tasks.py (1)
380-386: Extra N+1 query in log statement – use the annotatedproject_count
profile.projects.count()(line 385) issues an additional query per profile. The queryset already annotatesproject_count(line 354), soprofile.project_countcan be used directly.♻️ Proposed fix
logger.info( "[Schedule Project Feedback Check-in] Scheduling email", profile_id=profile.id, user_email=profile.user.email, days_since_registration=(now - profile.user.date_joined).days, - project_count=profile.projects.count(), + project_count=profile.project_count, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/scheduled_tasks.py` around lines 380 - 386, The log call using logger.info currently triggers an extra DB query by calling profile.projects.count(); change that to use the already-annotated attribute profile.project_count instead (i.e., replace profile.projects.count() with profile.project_count in the logger.info call) so no additional query is executed when logging.core/tests/test_project_feedback_checkin_emails.py (1)
17-81: Missing test for the "profile has no projects" skip path
send_project_feedback_checkin_emailreturns early whenprofile.projects.exists()isFalse(line 1604–1610 incore/tasks.py), but this branch has no test. As per coding guidelines, write tests for each function and each code path.✅ Suggested additional test
`@override_settings`(EMAIL_BACKEND="django.core.mail.backends.locmem.EmailBackend") `@patch`("core.tasks.async_task") def test_skips_when_profile_has_no_projects(self, mock_async_task): user = User.objects.create_user( username="no-project-user", email="no-project@example.com", password="password123", ) profile = Profile.objects.get(user=user) result = send_project_feedback_checkin_email(profile.id) assert result == f"Profile {profile.id} has no projects, skipping" assert len(mail.outbox) == 0 mock_async_task.assert_not_called()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/tests/test_project_feedback_checkin_emails.py` around lines 17 - 81, Add a new test to cover the "profile has no projects" early return in send_project_feedback_checkin_email: create a user and Profile (e.g., username "no-project-user"), do NOT create any Project for that profile, call send_project_feedback_checkin_email(profile.id), assert the returned message equals f"Profile {profile.id} has no projects, skipping", assert mail.outbox is empty, and assert the patched async_task (patch "core.tasks.async_task") was not called; place this test alongside the other tests in TestSendProjectFeedbackCheckinEmail in core/tests/test_project_feedback_checkin_emails.py.core/tasks.py (1)
1594-1647: Race condition: dedup check and tracking are not atomicThe
EmailSent.objects.filter(...).exists()check (line 1594) and the asynctrack_email_senttask (line 1641) have a TOCTOU window. Two concurrent workers can both pass theexists()check before either tracking record is written, resulting in duplicate emails.The established pattern in
check_and_send_project_setup_complete_email(lines 427–444) avoids this with an atomicget_or_create:♻️ Proposed fix – atomic idempotency guard
+ from django.db import transaction + - if EmailSent.objects.filter( - profile=profile, email_type=EmailType.PROJECT_FEEDBACK_CHECKIN - ).exists(): - logger.info( - "[Send Project Feedback Check-in Email] Email already sent to this profile, skipping", - profile_id=profile_id, - user_email=user.email, - ) - return f"Email already sent to {user.email}, skipping" + with transaction.atomic(): + email_sent_record, created = EmailSent.objects.get_or_create( + profile=profile, + email_type=EmailType.PROJECT_FEEDBACK_CHECKIN, + defaults={"email_address": user.email}, + ) + if not created: + logger.info( + "[Send Project Feedback Check-in Email] Email already sent to this profile, skipping", + profile_id=profile_id, + user_email=user.email, + ) + return f"Email already sent to {user.email}, skipping"Then remove the
async_task("core.tasks.track_email_sent", ...)call since the record is now created synchronously before sending.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/tasks.py` around lines 1594 - 1647, The exists() race can be fixed by creating the EmailSent record atomically before sending: replace the EmailSent.objects.filter(...).exists() check with an EmailSent.objects.get_or_create(profile=profile, email_type=EmailType.PROJECT_FEEDBACK_CHECKIN, defaults={...}) pattern (follow the same approach used in check_and_send_project_setup_complete_email) so only one worker proceeds to send; if get_or_create returns created=False, log and return as before. After switching to the synchronous get_or_create guard, remove the async_task("core.tasks.track_email_sent", ...) call because the EmailSent row is created synchronously and serves as the idempotency marker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/tasks.py`:
- Line 1636: Replace the hardcoded from_email="rasul@tuxseo.com" with a named
constant (UPPER_CASE) and use that constant in the function that sends the
email; add CHECKIN_EMAIL_FROM = "rasul@tuxseo.com" at the top of tasks.py (or
define it in settings.py and import it) and update the email send call to use
CHECKIN_EMAIL_FROM (or fallback to settings.DEFAULT_FROM_EMAIL if desired) so
the value is not embedded as a bare string.
---
Nitpick comments:
In `@core/scheduled_tasks.py`:
- Around line 380-386: The log call using logger.info currently triggers an
extra DB query by calling profile.projects.count(); change that to use the
already-annotated attribute profile.project_count instead (i.e., replace
profile.projects.count() with profile.project_count in the logger.info call) so
no additional query is executed when logging.
In `@core/tasks.py`:
- Around line 1594-1647: The exists() race can be fixed by creating the
EmailSent record atomically before sending: replace the
EmailSent.objects.filter(...).exists() check with an
EmailSent.objects.get_or_create(profile=profile,
email_type=EmailType.PROJECT_FEEDBACK_CHECKIN, defaults={...}) pattern (follow
the same approach used in check_and_send_project_setup_complete_email) so only
one worker proceeds to send; if get_or_create returns created=False, log and
return as before. After switching to the synchronous get_or_create guard, remove
the async_task("core.tasks.track_email_sent", ...) call because the EmailSent
row is created synchronously and serves as the idempotency marker.
In `@core/tests/test_project_feedback_checkin_emails.py`:
- Around line 17-81: Add a new test to cover the "profile has no projects" early
return in send_project_feedback_checkin_email: create a user and Profile (e.g.,
username "no-project-user"), do NOT create any Project for that profile, call
send_project_feedback_checkin_email(profile.id), assert the returned message
equals f"Profile {profile.id} has no projects, skipping", assert mail.outbox is
empty, and assert the patched async_task (patch "core.tasks.async_task") was not
called; place this test alongside the other tests in
TestSendProjectFeedbackCheckinEmail in
core/tests/test_project_feedback_checkin_emails.py.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/choices.pycore/migrations/0050_alter_emailsent_email_type.pycore/scheduled_tasks.pycore/tasks.pycore/tests/test_project_feedback_checkin_emails.py
| email = EmailMessage( | ||
| subject=subject, | ||
| body=plain_text, | ||
| from_email="rasul@tuxseo.com", |
Contributor
There was a problem hiding this comment.
Hardcoded from_email – extract to a named constant
from_email="rasul@tuxseo.com" is the only function in this file not using settings.DEFAULT_FROM_EMAIL. Even if the personal sender address is intentional, embedding it as a bare string makes it fragile and inconsistent with the rest of the module.
🛠️ Proposed fix
At the top of tasks.py (or in settings.py):
CHECKIN_EMAIL_FROM = "rasul@tuxseo.com"Then in the function:
email = EmailMessage(
subject=subject,
body=plain_text,
- from_email="rasul@tuxseo.com",
+ from_email=settings.CHECKIN_EMAIL_FROM,
to=[user.email],
)As per coding guidelines, extract unchanging values into constants using UPPER_CASE naming.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/tasks.py` at line 1636, Replace the hardcoded
from_email="rasul@tuxseo.com" with a named constant (UPPER_CASE) and use that
constant in the function that sends the email; add CHECKIN_EMAIL_FROM =
"rasul@tuxseo.com" at the top of tasks.py (or define it in settings.py and
import it) and update the email send call to use CHECKIN_EMAIL_FROM (or fallback
to settings.DEFAULT_FROM_EMAIL if desired) so the value is not embedded as a
bare 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.
Summary by CodeRabbit
New Features
Tests