-
Notifications
You must be signed in to change notification settings - Fork 1
SS-1478 Add email sending functionality and templates for account notifications #363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Nikita Churikov <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new manual email-sending feature and accompanying templates for account notifications.
- Introduces base and specific email templates for account status updates.
- Configures
EMAIL_TEMPLATES, email backend, and default sender addresses in settings. - Implements
EmailSendingTablemodel, Celery task, signal handlers, and admin integration for manual email workflows.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/admin/email/base.html | Adds base HTML layout and styling for all notification emails. |
| templates/admin/email/user-in-not-from-a-swedish-uni.html | New template for non-Swedish-affiliated account notification. |
| templates/admin/email/account-enabled-email.html | New template for account-enabled notification. |
| studio/settings.py | Loads all email templates, sets up email backend and default senders. |
| common/tasks.py | Updates send_email_task signature to accept from_email. |
| common/signals.py | Adds pre-save signal to trigger manual email sending on save. |
| common/models.py | Defines EmailSendingTable with email rendering and send logic. |
| common/migrations/0002_emailsendingtable.py | Migration to create EmailSendingTable. |
| common/admin.py | Registers EmailSendingTable in Django admin with custom admin class. |
Comments suppressed due to low confidence (2)
common/tasks.py:156
- The docstring above does not mention the new
from_emailparameter; please update it to reflect this addition.
from_email: str = settings.EMAIL_FROM,
common/signals.py:47
- Using a
pre_savesignal to send emails and updatestatuscan trigger another save and lead to recursion. Consider switching to apost_savehandler and guarding oncreatedor currentstatusto prevent repeated sends.
instance.send_email()
| <p>Dear {{ user.first_name }},</p> | ||
|
|
||
| <p> | ||
| You have request an account on our service SciLifeLab Serve (<a href="https://serve.scilifelab.se">https://serve.scilfielab.se</a>). Our service is only available to researchers affiliated with a Swedish university or their collaborators who need to work with them on the service. Your email is not related to a Swedish university, however. |
Copilot
AI
Jun 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the grammatical error: change 'request an account' to 'requested an account'.
| You have request an account on our service SciLifeLab Serve (<a href="https://serve.scilifelab.se">https://serve.scilfielab.se</a>). Our service is only available to researchers affiliated with a Swedish university or their collaborators who need to work with them on the service. Your email is not related to a Swedish university, however. | |
| You have requested an account on our service SciLifeLab Serve (<a href="https://serve.scilifelab.se">https://serve.scilfielab.se</a>). Our service is only available to researchers affiliated with a Swedish university or their collaborators who need to work with them on the service. Your email is not related to a Swedish university, however. |
common/models.py
Outdated
| if self.template: | ||
| # If a template is selected, render the message using the template | ||
|
|
||
| html_message = render_to_string(self.template, {"message": self.message}) |
Copilot
AI
Jun 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rendering templates with only message in context may break templates that reference other variables (e.g., user). Ensure you pass all required context or update templates to only use the provided data.
| html_message = render_to_string(self.template, {"message": self.message}) | |
| html_message = render_to_string( | |
| self.template, | |
| { | |
| "message": self.message, | |
| "subject": self.subject, | |
| "to_user": self.to_user, | |
| "to_email": self.to_email, | |
| } | |
| ) |
Co-authored-by: Copilot <[email protected]>
…ording in account request notification Signed-off-by: Nikita Churikov <[email protected]>
anondo1969
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Description
Now there is an email sending table

That you can use to send emails

Emails are sent on object's save, that means, that if email was not sent you can edit it and send it again.
As this PR modifies
settings.pyfile, here is the pr in serve-charts: ScilifelabDataCentre/serve-charts#99Checklist
If you're unsure about any of the items below, don't hesitate to ask. We're here to help!
This is simply a reminder of what we are going to look for before merging your code.
Further comments
Anything else you think we should know before merging your code!