Fixed #339 -- Added mass mailing for User model in admin panel#370
Fixed #339 -- Added mass mailing for User model in admin panel#370sathwikshetty33 wants to merge 1 commit intodjangoindia:mainfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to Django India! 🎉🇮🇳
We're so happy to see you contribute. Your efforts mean a lot to us and we're excited to have you on this journey.
Before we proceed, please take a moment to review our contribution guide. It's packed with all the information you need to make your contribution seamless.
If you're fixing an issue from the Django India issue tracker, remember to get it assigned to you before you acutally start working on it.
If you ever need help or just want to chat, the Django India discord server is always open. We're here for you!
And while you're here, don't forget to ⭐ star our repo to show your support! It helps the community grow.
Thank you for being a part of Django India. Together, let's make an amazing community! 🚀
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds a Celery task to send mass emails to arbitrary user email lists and integrates an admin action, custom view and route in UserAdmin to compose and queue emails to selected users. Also exposes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin User
participant AdminSite as Django Admin (UserAdmin)
participant SendView as send_email_view
participant Celery as Celery Queue
participant Task as send_mass_mail_task_users
participant Mailer as Email Backend
Admin->>AdminSite: select users + action "Send email"
AdminSite->>SendView: show compose form (/send_email/)
Admin->>SendView: POST subject/body
SendView->>Celery: queue Task.delay(emails, kwargs)
Celery->>Task: execute task
Task->>Mailer: send_mass_mail(batched emails)
Mailer-->>Task: results (success/errors)
Task-->>Celery: log outcome / raise on error
SendView-->>Admin: display success or error message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| actions = [send_email_to_selected_users] | ||
|
|
There was a problem hiding this comment.
Restore default admin actions.
Setting actions to just the custom handler removes Django’s built-in delete_selected action, so staff can no longer bulk-delete users from the changelist. Please append the new action instead of replacing the defaults.
- actions = [send_email_to_selected_users]
+ actions = ["delete_selected", send_email_to_selected_users]📝 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.
| actions = [send_email_to_selected_users] | |
| ++ b/backend/djangoindia/db/admin.py | |
| @@ -360,7 +360,7 @@ class UserAdmin(admin.ModelAdmin): | |
| """ | |
| Custom actions for UserAdmin. | |
| """ | |
| actions = ["delete_selected", send_email_to_selected_users] | |
| def send_email_to_selected_users(self, request, queryset): | |
| # ... implementation ... |
🤖 Prompt for AI Agents
In backend/djangoindia/db/admin.py around lines 362-363, the current assignment
replaces Django’s default admin actions (removing delete_selected); change it so
the custom send_email_to_selected_users is appended to the ModelAdmin defaults
instead of replacing them. Retrieve the default actions from
django.contrib.admin.ModelAdmin (handling the case they may be None), convert to
a mutable list if necessary, append send_email_to_selected_users, and assign
that combined list to actions.
| "form": form, | ||
| "opts": self.model._meta, | ||
| "queryset": json.loads(request.session.get("selected_email_ids", [])), | ||
| } |
There was a problem hiding this comment.
Avoid crashing when no selection is stored.
json.loads cannot handle the list default ([]), so visiting /send_email/ without a session value raises a TypeError. Use a string fallback (or conditional parse) to keep the view resilient.
- "queryset": json.loads(request.session.get("selected_email_ids", [])),
+ "queryset": json.loads(
+ request.session.get("selected_email_ids", "[]")
+ ),📝 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.
| "form": form, | |
| "opts": self.model._meta, | |
| "queryset": json.loads(request.session.get("selected_email_ids", [])), | |
| } | |
| "form": form, | |
| "opts": self.model._meta, | |
| "queryset": json.loads( | |
| request.session.get("selected_email_ids", "[]") | |
| ), | |
| } |
🤖 Prompt for AI Agents
In backend/djangoindia/db/admin.py around lines 460 to 463, the code calls
json.loads(request.session.get("selected_email_ids", [])) which crashes when the
session key is missing because json.loads expects a string, not a list; change
to either provide a JSON string default (e.g.
request.session.get("selected_email_ids", "[]")) or guard the value (val =
request.session.get("selected_email_ids"); parsed = json.loads(val) if
isinstance(val, str) else val or []) so the view safely handles missing or
already-parsed selections.
|
@CodeRabbit fullreview |
|
@Yadavanurag13 I'll perform a full review of PR #370 which adds mass mailing functionality for the User model in the Django admin panel. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/djangoindia/db/admin.py (2)
189-189: Incorrect Celery task signature usage (missingcomm_id).
send_mass_mail_taskrequires(emails, comm_id, **kwargs), but this call only passesemails. It will raiseTypeErrorat runtime.Fix one of:
- If you don’t track an
EventCommunicationhere, call the users task:- send_mass_mail_task.delay(emails, fail_silently=False) + send_mass_mail_task_users.delay(emails, fail_silently=False)
- Or create an
EventCommunicationrecord (as inEventUserRegistrationAdmin) and pass its id.
726-728: Don’t mark communication SENT before the async task completes.You set
status=SENTandsent_atimmediately after queuing. The Celery task also updates status and may fail, causing incorrect state.Apply this diff:
- communication.status = EventCommunication.Status.SENT - communication.sent_at = timezone.now() - communication.save() + # Keep as PENDING; task will set SENT/FAILED and sent_at.Optional: show a “Queued N emails” message instead of “sent”.
🧹 Nitpick comments (3)
backend/djangoindia/bg_tasks/event_tasks.py (1)
126-127: Consider adding Celery auto‑retry/backoff.Transient SMTP/network errors are common. Add
autoretry_for+ limited retries and a delay.Example:
-@shared_task +@shared_task(bind=True, autoretry_for=(Exception,), retry_backoff=True, retry_jitter=True, max_retries=3) def send_mass_mail_task_users(emails, **kwargs):If project Celery version doesn’t support
retry_backoff/retry_jitter, usedefault_retry_delay=60withautoretry_for.backend/djangoindia/db/admin.py (2)
419-456: Harden session parsing and clear selection after send.
- Session value may be missing or non‑string; parsing defensively avoids surprises.
- Clear
selected_email_idsafter success to prevent accidental re‑use on the next visit.Apply this diff:
- user_ids = request.session.get("selected_email_ids", []) + raw = request.session.get("selected_email_ids") + user_ids = json.loads(raw) if isinstance(raw, str) else (raw or []) if not user_ids: messages.error(request, "No user IDs provided.") return redirect("../") - - if user_ids: - user_ids = json.loads(user_ids) + # user_ids is now a list of IDs @@ - if emails: - send_mass_mail_task_users.delay(emails, fail_silently=False) + if emails: + send_mass_mail_task_users.delay(emails, fail_silently=False) messages.success( request, f"{len(emails)} emails sent successfully." ) + request.session.pop("selected_email_ids", None) else:Optionally validate addresses with
EmailValidatorto catch malformed inputs early.
549-549: Use Celery asynchronously for confirmation emails.These are Celery tasks but are called synchronously; use
.delay(...)to avoid blocking admin requests.- rsvp_confirmation_email_task(registration.user.email, event_id) + rsvp_confirmation_email_task.delay(registration.user.email, event_id)and
- rsvp_confirmation_email_task(registration.user.email, event.id) + rsvp_confirmation_email_task.delay(registration.user.email, event.id)Also applies to: 667-667
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/djangoindia/bg_tasks/event_tasks.py(1 hunks)backend/djangoindia/db/admin.py(4 hunks)
🔇 Additional comments (4)
backend/djangoindia/db/admin.py (4)
21-22: Import looks correct.
362-363: Don’t replace default admin actions. Append instead.Overriding
actionsremoves Django’s built‑indelete_selected. Prefer extending viaget_actions:- actions = [send_email_to_selected_users] + # Keep default actions (incl. delete_selected) and add our custom action + def get_actions(self, request): + actions = super().get_actions(request) + actions["send_email_to_selected_users"] = ( + send_email_to_selected_users, + "send_email_to_selected_users", + send_email_to_selected_users.short_description, + ) + return actions
459-464: Avoidjson.loadswith a list default.
json.loadsexpects a string; using[]as the default raisesTypeErrorwhen the session key is missing. Use a JSON string fallback or guard the type.Apply this diff:
- "queryset": json.loads(request.session.get("selected_email_ids", [])), + "queryset": json.loads(request.session.get("selected_email_ids", "[]")),Do the same adjustment in other admin views using this pattern.
352-355: No issues with list_filter fields
Bothis_password_expiredandis_onboardedare defined on theUsermodel, so including them inlist_filterwill not trigger aFieldError.
| if not isinstance(emails, (list, tuple)): | ||
| logger.exception( | ||
| "Invalid input: Emails must be a list or tuple of email data tuples." | ||
| ) | ||
|
|
There was a problem hiding this comment.
Don’t just log invalid input — fail fast.
You log an invalid emails type but continue execution, which can cascade into harder-to-debug failures. Raise a TypeError (or return 0) immediately.
Apply this diff:
- if not isinstance(emails, (list, tuple)):
- logger.exception(
- "Invalid input: Emails must be a list or tuple of email data tuples."
- )
+ if not isinstance(emails, (list, tuple)):
+ raise TypeError(
+ "emails must be a list/tuple of (subject, message, from_email, recipient_list) tuples"
+ )📝 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.
| if not isinstance(emails, (list, tuple)): | |
| logger.exception( | |
| "Invalid input: Emails must be a list or tuple of email data tuples." | |
| ) | |
| if not isinstance(emails, (list, tuple)): | |
| raise TypeError( | |
| "emails must be a list/tuple of (subject, message, from_email, recipient_list) tuples" | |
| ) |
🤖 Prompt for AI Agents
In backend/djangoindia/bg_tasks/event_tasks.py around lines 138 to 142, the code
only logs when `emails` is not a list/tuple and then continues; change this to
fail fast by raising a TypeError (or returning 0 if calling convention expects
that) immediately after detecting the invalid type. Replace the logger.exception
call with a raised TypeError containing a clear message like "emails must be a
list or tuple of email data tuples" (or return 0 instead), so execution stops
and callers can handle the error.
| if len(emails) > 100: | ||
| loops = len(emails) // 100 | ||
| else: | ||
| loops = 1 | ||
| for i in range(loops): | ||
| send_mass_mail(emails[i * 100 : (i + 1) * 100], **kwargs) | ||
|
|
||
| if len(emails) % 100 != 0: | ||
| send_mass_mail(emails[loops * 100 :], **kwargs) | ||
| except Exception as e: | ||
| logger.exception("Failed to send mass emails.") | ||
| raise e |
There was a problem hiding this comment.
Fix return value and simplify batching; currently always returns None.
Docstring promises “same as send_mass_mail” but the function returns None. Also the loop/remainder logic is overcomplicated and can make a redundant call for <100 items.
Apply this diff:
- try:
- if len(emails) > 100:
- loops = len(emails) // 100
- else:
- loops = 1
- for i in range(loops):
- send_mass_mail(emails[i * 100 : (i + 1) * 100], **kwargs)
-
- if len(emails) % 100 != 0:
- send_mass_mail(emails[loops * 100 :], **kwargs)
- except Exception as e:
- logger.exception("Failed to send mass emails.")
- raise e
+ try:
+ if not emails:
+ return 0
+ total_sent = 0
+ for start in range(0, len(emails), 100):
+ batch = emails[start : start + 100]
+ total_sent += send_mass_mail(batch, **kwargs)
+ return total_sent
+ except Exception as e:
+ logger.exception("Failed to send mass emails.")
+ raise📝 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.
| if len(emails) > 100: | |
| loops = len(emails) // 100 | |
| else: | |
| loops = 1 | |
| for i in range(loops): | |
| send_mass_mail(emails[i * 100 : (i + 1) * 100], **kwargs) | |
| if len(emails) % 100 != 0: | |
| send_mass_mail(emails[loops * 100 :], **kwargs) | |
| except Exception as e: | |
| logger.exception("Failed to send mass emails.") | |
| raise e | |
| try: | |
| if not emails: | |
| return 0 | |
| total_sent = 0 | |
| for start in range(0, len(emails), 100): | |
| batch = emails[start : start + 100] | |
| total_sent += send_mass_mail(batch, **kwargs) | |
| return total_sent | |
| except Exception as e: | |
| logger.exception("Failed to send mass emails.") | |
| raise |
🤖 Prompt for AI Agents
In backend/djangoindia/bg_tasks/event_tasks.py around lines 144-155, the
batching logic overcomplicates slicing and can call send_mass_mail redundantly
and the function always returns None; change the loop to iterate in fixed-size
chunks (for start in range(0, len(emails), 100)) and call send_mass_mail once
per chunk, accumulate the returned counts into a total variable, and at the end
return that total so the function behaves "same as send_mass_mail"; keep the
existing exception handling (log and re-raise) unchanged.
| def get_urls(self): | ||
| urls = super().get_urls() | ||
| custom_urls = [ | ||
| path( | ||
| "send_email/", | ||
| self.admin_site.admin_view(self.send_email_view), | ||
| name="send_email", | ||
| ), | ||
| ] | ||
| return custom_urls + urls |
There was a problem hiding this comment.
URL name collision risk across multiple admins. Use unique names per model.
Several admins define name="send_email". Named URLs must be unique within the admin namespace; collisions break reversing and make logs ambiguous.
Apply this diff to make names unique and model-scoped:
- path(
- "send_email/",
- self.admin_site.admin_view(self.send_email_view),
- name="send_email",
- ),
+ path(
+ "send_email/",
+ self.admin_site.admin_view(self.send_email_view),
+ name=f"{self.model._meta.app_label}_{self.model._meta.model_name}_send_email",
+ ),Consider updating any reverse(...) calls accordingly (you mostly use relative redirects, which are unaffected).
📝 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.
| def get_urls(self): | |
| urls = super().get_urls() | |
| custom_urls = [ | |
| path( | |
| "send_email/", | |
| self.admin_site.admin_view(self.send_email_view), | |
| name="send_email", | |
| ), | |
| ] | |
| return custom_urls + urls | |
| def get_urls(self): | |
| urls = super().get_urls() | |
| custom_urls = [ | |
| path( | |
| "send_email/", | |
| self.admin_site.admin_view(self.send_email_view), | |
| name=f"{self.model._meta.app_label}_{self.model._meta.model_name}_send_email", | |
| ), | |
| ] | |
| return custom_urls + urls |
🤖 Prompt for AI Agents
In backend/djangoindia/db/admin.py around lines 408 to 417, the custom URL name
"send_email" collides across multiple admin classes; change the URL name to a
model-scoped unique name (e.g., f"{self.model._meta.model_name}_send_email" or
"{app_label}_{model_name}_send_email") when calling path(..., name=...), and
update any reverse(...) or admin_site.reverse calls that reference "send_email"
to use the new unique name (or construct it dynamically from the model/app meta)
so names are globally unique within the admin namespace.
3 participants
Closes #339
Add functionality to send bulk emails to users from Django admin panel with background task processing
Changes
UserAdminclass with enhanced functionalitysend_email_to_selected_usersaction to User admin for bulk email operationslist_filterwithis_password_expiredandis_onboardedfields for better user managementsend_email_viewmethod with custom URL routing for email form handlingsend_mass_mail_task_usersshared task for background email processing with batch supportType of change
Flags
DEFAULT_FROM_EMAILsetting from Django configurationDemo
/admin/db/user/send_email/with subject and message fieldsHow has this been tested?
is_password_expired,is_onboarded) work correctlyAuthor Checklist
mainfromfork:issue-339Additional context
EventUserRegistrationAdminemail functionality for consistencysend_mass_mail_task_usersmirrors the existingsend_mass_mail_taskbut specifically handles User model email sendingSummary by CodeRabbit
New Features
Improvements