Communication class and admin for email texts#1658
Conversation
uses template and context to generate in branch if found in database
|
SUMMARY Some help text for editing of the email subject and message fields in the database has been added. I added a kind of template render preview to the admin edit page for communication. This is by way of an extra non-editable field that shows the rendering of the text the user has entered
|
Render template preview in Admin for communication editing, initial draft
This comment was marked as outdated.
This comment was marked as outdated.
Woseseltops
left a comment
There was a problem hiding this comment.
In addition to my one comment in the code, a place other than attachments for this whole thing is probably better, as discussed in another issue.
signbank/dictionary/adminviews.py
Outdated
| access_email = Communication.objects.filter(label='dataset_to_owner_existing_user_given_access').first() | ||
|
|
||
| if access_email: | ||
| subject_template = template.Template(access_email.subject) | ||
| subject = subject_template.render(mail_template_context) | ||
| else: | ||
| subject = render_to_string('registration/dataset_to_owner_existing_user_given_access_subject.txt', | ||
| context={'dataset': dataset_object.name, | ||
| 'site': current_site}) |
There was a problem hiding this comment.
I see this pattern repeated several times... perhaps a function?
There was a problem hiding this comment.
This was originally much cooler in one expression statement. Unfortunately the branches handle different types.
I'll try to recreate the original one and hide the branches. DONE
|
|
I finished the requested revisions.
|
|
Here is what the "instructions help text" in the admin looks like: This includes all of the possible context variables that are used in all the template files for the various places where an email is sent. Each of the places in the code creates a context for the email. Since we don't know whether the fixtures initial content will be imported, there is no "starting block" for the various emails if the user wants to construct these from scratch. Since the content of the context depends on the file and the place in the code, it would be best to make the data structure shown above have an additional set of keys above that is the label, then show the context variables dynamically based on what the user has chosen. Here is a modification to display the instructions in a separate panel. |
extra json per type of email label
|
For the help text, I suggest this:
Why not use class MyModel(models.Model):
description = models.TextField(
help_text=(
"Explain how to use this field. "
"You can write multiple sentences here. "
"Formatting like paragraphs is fine."
)
) |
Yes, it was originally a help text. Except the stupid context variables are "context sensitive" based on where in the code the emails are generated. (That's what I was trying to do with the "new" json expression, to make it context sensitive what the variables may be.) I guess I could add something to "check" what is entered and see whether all the context variables are available for that kind of email. (Kip of Ei) Another question about the context variables, Unify them? (Like the ones that use |
|
@Woseseltops I figured out how to make use of the context per label dictionary! This can be used in the "render" preview. Although some additional comments are needed to point out that the variable used isn't available for the type label. I normalised the context variables. See #1672 (comment) One of the context variables is wrong in registration |
|
@Woseseltops check it out! I got it working, to validate the context variables the user has in their text |
variables per email type; form validation of context variables depending on type of communication chosed, to match code where the emails are sent
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Communication class to centralize email text management and removes the unused Attachment model. It replaces inline template rendering with a database-backed system for managing email subjects and bodies.
Changes:
- Added a Communication model and admin interface for managing email templates
- Refactored email sending code to use the new
generate_communicationfunction - Removed the Attachment model, views, URLs, and related functionality
Reviewed changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| signbank/communication/models.py | New Communication model with validation for email templates and a generate_communication function to render emails |
| signbank/communication/admin.py | Admin interface for Communication model with context variable instructions and preview rendering |
| signbank/communication/fixtures/communication.json | Initial fixture data with default email templates |
| signbank/communication/migrations/0001_initial.py | Database migration creating the Communication table |
| signbank/registration/views.py | Updated to use generate_communication instead of inline template rendering |
| signbank/registration/models.py | Updated to use generate_communication for activation emails |
| signbank/dictionary/adminviews.py | Updated to use generate_communication for dataset access emails |
| signbank/registration/templates/registration/*.txt | Fixed template variable references (e.g., {{site}} to {{site.domain}}, {{new_user_*}} to {{user.*}}) |
| signbank/settings/base.py | Added 'signbank.communication' to INSTALLED_APPS and removed ATTACHMENT_LOCATION setting |
| signbank/urls.py | Removed attachments URL configuration |
| signbank/attachments/* | Removed Attachment model, views, URLs, admin, and management commands |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,47 @@ | |||
| [ | |||
| { | |||
| "model": "attachments.communication", | |||
There was a problem hiding this comment.
The model reference is incorrect. It should be 'communication.communication' instead of 'attachments.communication' since the Communication model is in the communication app, not the attachments app.
| } | ||
| }, | ||
| { | ||
| "model": "attachments.communication", |
There was a problem hiding this comment.
The model reference is incorrect. It should be 'communication.communication' instead of 'attachments.communication' since the Communication model is in the communication app, not the attachments app.
| } | ||
| }, | ||
| { | ||
| "model": "attachments.communication", |
There was a problem hiding this comment.
The model reference is incorrect. It should be 'communication.communication' instead of 'attachments.communication' since the Communication model is in the communication app, not the attachments app.
| } | ||
| }, | ||
| { | ||
| "model": "attachments.communication", |
There was a problem hiding this comment.
The model reference is incorrect. It should be 'communication.communication' instead of 'attachments.communication' since the Communication model is in the communication app, not the attachments app.
| } | ||
| }, | ||
| { | ||
| "model": "attachments.communication", |
There was a problem hiding this comment.
The model reference is incorrect. It should be 'communication.communication' instead of 'attachments.communication' since the Communication model is in the communication app, not the attachments app.
| A new user has signed up at {{site.name}}. | ||
|
|
||
| {{new_user_firstname}} {{new_user_lastname}} (user {{new_user_username}}, email address {{new_user_email}}) | ||
| {{user.firstname}} {{user.lastname}} (user {{user.username}}, email address {{user.email}}) |
There was a problem hiding this comment.
The template variable {{user.firstname}} should be {{user.first_name}} to match Django's User model field name. Similarly, {{user.lastname}} should be {{user.last_name}}.
| {{user.firstname}} {{user.lastname}} (user {{user.username}}, email address {{user.email}}) | |
| {{user.first_name}} {{user.last_name}} (user {{user.username}}, email address {{user.email}}) |
signbank/communication/models.py
Outdated
| 'dataset_to_owner_existing_user_given_access': {'user': {'firstname': 'FIRST NAME', | ||
| 'lastname': 'LAST NAME', |
There was a problem hiding this comment.
The context keys 'firstname' and 'lastname' should be 'first_name' and 'last_name' to match Django's User model field names.
signbank/communication/models.py
Outdated
| 'user': {'firstname': 'FIRST NAME', | ||
| 'lastname': 'LAST NAME', |
There was a problem hiding this comment.
The context keys 'firstname' and 'lastname' should be 'first_name' and 'last_name' to match Django's User model field names.
signbank/communication/models.py
Outdated
| 'user': {'firstname': 'FIRST NAME', | ||
| 'lastname': 'LAST NAME', |
There was a problem hiding this comment.
The context keys 'firstname' and 'lastname' should be 'first_name' and 'last_name' to match Django's User model field names.
signbank/communication/admin.py
Outdated
| 'user': {'firstname': 'FIRST NAME', | ||
| 'lastname': 'LAST NAME', |
There was a problem hiding this comment.
The context keys 'firstname' and 'lastname' should be 'first_name' and 'last_name' to match Django's User model field names.
|
CoPilot changes (i.e., bugs) fixed! |
Woseseltops
left a comment
There was a problem hiding this comment.
I love the screenshot @susanodd :)



Of the existing kinds of email subject and body text files, the various places that they are sent by the code, it looks like the case where a user has request change permission for a dataset does not send any email if this has been granted.
Mail to dataset owner
Mail to user
Context variables:
signbank_name
url
user.firstname
user.lastname
user.email
user.username
new_user_firstname
new_user_lastname
new_user_email
dataset
motivation
site.name
https://{{site}}/datasets/manager/