Skip to content

Send email notifications#17

Merged
jchate6 merged 8 commits into7-revisit-and-revamp-tom-registrationfrom
16-fix-tom_registration-email-notifications
Mar 13, 2025
Merged

Send email notifications#17
jchate6 merged 8 commits into7-revisit-and-revamp-tom-registrationfrom
16-fix-tom_registration-email-notifications

Conversation

@jchate6
Copy link
Copy Markdown
Contributor

@jchate6 jchate6 commented Mar 6, 2025

Emails are sent to "managers" upon a user registering with the TOM.
Another email is sent to the user upon acceptance.

I don't have a good way to check the default links back to the TOM in the emails work without deploying these changes to tom_demo.

@jchate6 jchate6 changed the title update readme Send email notifications Mar 6, 2025
@jchate6 jchate6 requested review from Fingel and phycodurus March 6, 2025 23:33
try:
current_domain = Site.objects.get_current().domain
link_to_user_list = f'https://{current_domain}{reverse("user-list")}'
mail_managers(
Copy link
Copy Markdown
Contributor Author

@jchate6 jchate6 Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a mail_admin function as well.
I don't have a strong preference. The main difference is one pulls emails from the MANAGERS setting, while the other pulls from the ADMINS setting.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since TOMToolkit already has the concept of ADMINS, I'd suggest using that and not introducing a new concept (MANAGERS) unless someone asks for it (even if this idea of ADMINS isn't exactly the same as our current idea of what admins are (i.e. the list of Users with admin priviledge). What actually is the "ADMINS setting" here?).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know if we care about the difference.

The ADMINS setting is a list of tuples containing a name and email.
If this is set, those people would receive emails containing the details of all exceptions raised in the request/response cycle.

The MANAGERS Setting is a lit of tuples containing a name and email, same as admin.
If set, those people would receive emails if we were using BrokenLinkEmailsMiddleware.

Does one of those make more sense for us?
I used Managers partially because it doesn't do anything else by default.

@jchate6 jchate6 marked this pull request as ready for review March 6, 2025 23:37
@jchate6 jchate6 moved this to Needs Review in TOM Toolkit Mar 6, 2025
try:
current_domain = Site.objects.get_current().domain
link_to_user_list = f'https://{current_domain}{reverse("user-list")}'
mail_managers(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since TOMToolkit already has the concept of ADMINS, I'd suggest using that and not introducing a new concept (MANAGERS) unless someone asks for it (even if this idea of ADMINS isn't exactly the same as our current idea of what admins are (i.e. the list of Users with admin priviledge). What actually is the "ADMINS setting" here?).

Copy link
Copy Markdown

@Fingel Fingel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@jchate6 jchate6 merged commit ba962fc into 7-revisit-and-revamp-tom-registration Mar 13, 2025
18 checks passed
@jchate6 jchate6 deleted the 16-fix-tom_registration-email-notifications branch March 13, 2025 00:21
@github-project-automation github-project-automation bot moved this from Needs Review to Merged (to dev) in TOM Toolkit Mar 13, 2025
@jchate6 jchate6 moved this from Merged (to dev) to Released in TOM Toolkit Mar 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants