Skip to content
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

Add [DO NOT REPLY] to the from name of accounts email for mailers #4803

Merged

Conversation

Sukhpreet-s
Copy link
Contributor

@Sukhpreet-s Sukhpreet-s commented Nov 26, 2024

Resolves #4521

Description

This PR updates the from name for all the emails sent using the [email protected] email address to include [DO NOT REPLY]. These changes are implemented in the CustomDeviseMailer.

It seems only some of the emails, those set up by Devise, use the [email protected] email address, as other mailers override the from address to [email protected]. These non-Devise mailers, with the from name format "Please do not reply to this email as this mailbox is not monitored — Human Essentials", remain unchanged as they are outside this issue's scope.

Observation notes:

While working on this issue, I noticed that some emails are not sent to users at all.

List of Devise configured emails that are set-up to send (changed in this PR):

  • Inviting/re-inviting users
  • Reset password instructions - This email is sent when 'Forgot Password' button is used from the main Login page.

List of Devise configured emails that are not set-up to send (unchanged):

  • Email changed - requires confirmable module
  • Password changed - the mailer function is not executed by default. Use callback to execute it.
  • Confirmation instruction - requires confirmable module
  • Unlock instruction - requires localable module

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

This fix is tested manually.

I haven't added any tests for this since the tests for other mailers are only checking for the email address in the from name and not for the text part. Please let me know if we should still add tests to check if [DO NOT REPLY] is included.

Steps to reproduce the Invitation Instructions email:

  1. Log in as Organization Admin with [email protected] email.
  2. Click on My Organization in the left navigation bar.
  3. Scroll all the way to bottom and click Invite User to this Organization button.
  4. Enter any valid email in the email input box and click Invite User.
  5. You should receive the email with [DO NOT REPLY] in the from name.

Steps to reproduce the Reset Password email:

  1. Log out if already logged in.
  2. Navigate to the sign in page by clicking Login button.
  3. Click on Forgot your password? link.
  4. Enter [email protected] in the email input box and click the Send me reset password instructions.
  5. You should receive the email with [DO NOT REPLY] in the from name.

Screenshots

When inviting new user to an Organization:

Before

After

When resetting the password:

Before

After

@Sukhpreet-s Sukhpreet-s changed the title [WIP] Add DO NOT REPLY to the 'from' name of accounts email for mailers [WIP] Add DO NOT REPLY to the from name of accounts email for mailers Nov 26, 2024
@Sukhpreet-s Sukhpreet-s changed the title [WIP] Add DO NOT REPLY to the from name of accounts email for mailers [WIP] Add [DO NOT REPLY] to the from name of accounts email for mailers Nov 26, 2024
@Sukhpreet-s Sukhpreet-s force-pushed the 4521-update-name-for-accounts-email branch 3 times, most recently from 4d0b002 to 14199aa Compare November 29, 2024 02:53
@Sukhpreet-s Sukhpreet-s changed the title [WIP] Add [DO NOT REPLY] to the from name of accounts email for mailers Add [DO NOT REPLY] to the from name of accounts email for mailers Nov 29, 2024
@Sukhpreet-s Sukhpreet-s marked this pull request as ready for review November 29, 2024 04:08
@cielf cielf self-requested a review November 29, 2024 23:19
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hey -- Thanks for this!
I think your solution would produce the desired result - but would be "DRYer" to just change it in the place we've already defined the from, yes?.

app/mailers/custom_devise_mailer.rb Outdated Show resolved Hide resolved
@Sukhpreet-s Sukhpreet-s force-pushed the 4521-update-name-for-accounts-email branch from 14199aa to 211763e Compare December 2, 2024 02:26
@Sukhpreet-s
Copy link
Contributor Author

Sukhpreet-s commented Dec 2, 2024

Thank you for the review, @cielf! I’ve made the requested changes and would appreciate it if you could take another look when you have time.

I apologize for force-pushing the update; I wanted to squash the commits to keep the history cleaner by excluding "revert" commit.

@Sukhpreet-s Sukhpreet-s requested a review from cielf December 2, 2024 02:56
@cielf cielf merged commit 8180529 into rubyforgood:main Dec 2, 2024
11 checks passed
@cielf
Copy link
Collaborator

cielf commented Dec 2, 2024

Looks good to me! Thank you!

Copy link
Contributor

github-actions bot commented Dec 8, 2024

@Sukhpreet-s: Your PR Add [DO NOT REPLY]to thefrom name of accounts email for mailers is part of today's Human Essentials production release: 2024.12.08.
Thank you very much for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add [DO NOT REPLY] to the name for the accounts email.
3 participants