Allow for custom user registration approval email address

I second this need.

I'm currently creating a site with two administrators who need to receive this email. If nothing else, the documentation on this problem needs to be made more clear. I spent close to an hour googling around for a solution.

+1

I was just asked by a client "who gets the registration notification emails?" and said something like "i'm assuming the user with the user management permissions, i can't imagine it would be the SITE'S EMAIL..."

And... what do you know... ;)

I second this idea. Current settings make very little (if any) sense.

I agree. The site email might be useful as a default for some cases but in many cases the site email might be only a "from" email, having the ability to choose how it is used is a good idea.

This module allows customization of user registration notification...
http://drupal.org/project/user_register_notify

Having a list of places where emails should go might be a nice addition to core.

+1

I need to forward registration approval to someone else. Would be really great to be able to forward those to another email address than the one given to site's admin.

+ 1 for this need.

Subscribing.

Offers an option on the 'admin/config/people/accounts' page where you can input an e-mail address. This address, if available, overrides the site mail address in drupal_mail().

adding tags and updating issue summary with which tasks are novice

Updated the issue to current state, added markup.

The current issue summary describes a slightly different problem from the one originally reported by mcrittenden. The original report suggested that admins should be allowed to specify which email address(es) receive notifications when a user registers for an account and the "Require e-mail verification when a visitor creates an account." option is enabled on the admin/config/people/accounts page.

The patch in #9 adds an extra field on that same page and when it's filled out that value is used in the FROM address on emails automatically sent out by the system. This may be desirable behavior but it's not the same problem that mcrittenden was trying to solve.

Not able to apply the patch to user.admin.inc file. Please find the attached Screenshot.

Please find a new patch. The patch in #9 does not seem to work.

Steps to test

1. Download the latest version of Drupal 8.x
2. Before installing, apply the latest patch from Convert register_pending_approval_admin to config/state system to ensure admin emails can be sent
3. Install Drupal
4. Download and apply the patch in #14 above
5. Navigate to the account settings page (/admin/config/people/accounts)
6. Scroll down to the E-mail address field (Just before the E-mails section)
7. Enter an email address and save the page
8. Log out of Drupal
9. Navigate to the user registration page (/user/register)
10. Register with a different email address
11. Check an email is received at the email address entered on the accounts settings page

Before and After screenshots of account settings page (admin/config/people/accounts)

1. Before patch applied

   

2. After patch applied

   

tried patch #9 and worked for me as expected
tried patch #14 and git wont apply it

Did you try to apply the patch to the latest version of 8.x-dev?

yes, sir. i updated my git repo just before apply the patches
patch in #9 worked well
patch in #13 wasnt applied by git
i can send you the logs afterwards if you want them.
i can repeat all the process this afternoon, now i havent time

#14: 494518-custom-approval-email-12.patch queued for re-testing.

When testing, I found the additional user setting 'notify: register_pending_approval_admin' was required to trigger the new site email to be used. (Required by the change to mail.inc in the patch from #14). Rather than make this change to issue 1804926 - Convert register_pending_approval_admin to config/state system I've created a different patch that moves the functionality from mail.inc to user.module.

Additionally this patch causes the new user registration notification email to be sent to the admin email address entered on the account settings screen if present, as per the original requirement.

N.B. This change also sets the 'from' email address to the admin email address entered on the account settings screen for emails sent for other operations that require notification

Patch corrected (hopefully)

#23 patch worked for me on a fresh git checkout
[#9 patch works also]
i lack of the knowledge to decide technically between both

Patch in #23 has been updated to improve the help text associated with the new email address field, so it more accurately reflects what the new field does.

Adding tag

Patch in #26 updated to add validation to the new email address field.

I know these patches are small... but interdiffs are fun! :) (and helpful good habits)

http://drupal.org/node/1488712

Thanks for the great feedback from YesCT. I've re-uploaded the patch in #28 along with my first attempt at an interdiff. Every day's a school day :-)

Mery Xmas all

tested that patch in #30 applies and works as advertised. leaving status as it is though, as i'm not sure what all it affects.

@bdone thanks for manually testing. you are right to not set it to rtbc until a code review and other concerns are addressed (like needing tests)

---

+++ b/core/modules/user/user.admin.incundefined
@@ -389,6 +389,15 @@ function user_admin_settings($form, &$form_state) {
+    '#description' => "The e-mail address to be used as the 'from' address for all notifications. If <em>'Visitors, but administrator approval is required'</em> is selected above, a notification email will also be sent to this address for any new registrations. Leave empty to use the default system e-mail address <em>(" . config('system.site')->get('mail') . ").</em>",

this needs to go through t() like other descriptions in this file.

http://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functio...

something like:

'#description' => t("The e-mail address to be used as the 'from' address for all notifications. If <em>'Visitors, but administrator approval is required'</em> is selected above, a notification email will also be sent to this address for any new registrations. Leave empty to use the default system e-mail address <em>(%site-email).</em>", array('%site-email' => config('system.site')->get('mail'))),

+++ b/core/modules/user/user.moduleundefined
@@ -2650,11 +2650,15 @@ function _user_mail_notify($op, $account, $langcode = NULL) {
+    // Get the custom site notification email to use as the from email address if it has been set.
...
+      // If the custom site notification email has not been set, we use the site default language for this.

Comments need to wrap at 80 chars.

http://drupal.org/coding-standards/docs#general

+++ b/core/modules/user/user.moduleundefined
@@ -2650,11 +2650,15 @@ function _user_mail_notify($op, $account, $langcode = NULL) {
+    $site_mail = config('system.site')->get('notification');
...
+        $site_mail = config('system.site')->get('mail');

since the site default mail address is 'mail' I think 'mail_notification' makes more sense.

+++ b/core/modules/user/user.moduleundefined
@@ -2650,11 +2650,15 @@ function _user_mail_notify($op, $account, $langcode = NULL) {
-      // We use the site default language for this.

This comment is incorrectly combined with the idea of a site default mail address.

The comment needs to stay, but perhaps in a better location.

It's strange that the notification email is used as the from in the email to the user without checking if it's empty,
and then later when it's used as the "to" address, then it checks if it's empty, uses the site default, and then if thats empty another... default.
it works though, because I think if the from is NULL then it defaults to the default.
but if the "to" is not set... there is no default "to" :)

I moved the code to check for if it's empty to before it's used as the from anyway, since that seemed to make more sense logically.
if there is a good reason to keep it only when sending "to" I'm totally fine with it being moved back in the other location.

patch attached.

---

for testing, make sure to delete config (sudo rm -r sites; git checkout sites;) before reinstalling to clear the config.

also, be sure to test before setting a notification mail and after a notification mail is set.

I saw that the admin message (for a account creation request) both before and after this patch is blank. so that is not a problem with this patch. that needs a follow-up issue (I could not find an existing issue.)

this needs tests for if a notification mail is specified (See Contributor Task: Write tests http://drupal.org/node/1468170)

updated screenshot showing the helptext that shows the current site default address.

maybe the @edrupal mentioned in #21 (#1804926: Convert register_pending_approval_admin email to config system and provide the ability to configure it in UI.) will address the concern of the empty admin email? that issue needs a reroll.

#32: drupal-custom_notification_sender_address-494518-32.patch queued for re-testing.

The steps listed in #15 are still mostly accurate. The only differences are which patches to apply. The non-patch related steps are still valid.

I also had to reroll the patch in #32

Updated steps to test

1. Download the latest version of Drupal 8.x
2. Before installing, apply the latest patch from Convert register_pending_approval_admin to config/state system to ensure admin emails can be sent
3. Install Drupal
4. Apply the attached patch (It is a reroll of #32)
5. Navigate to the account settings page (/admin/config/people/accounts)
6. Scroll down to the E-mail address field (before the E-mails section with vertical tabs)
7. Enter a different email address that what was used during install and save the page
8. Log out of Drupal
9. Navigate to the user registration page (/user/register)
10. Register with 3rd different email address
11. Ensure that the email that is recieved was sent from the address entered on the account settings page (/admin/config/people/accounts)

Oops... forgot to unassign this from myself.

I've created a new test for verifying this functionality.

@c4doug Thanks for writing these tests. Do you have any thoughts about if this needed, too much testing, or just right?

There are extra blank lines in there that need to be taken out (like where there are 3 in a row, and the one after the function start maybe... need to check the standards).

The one line comments are good, but need to be complete sentences that end in a period.

Check this for general recommendations on comment standards:
http://drupal.org/node/1354

This for coding standards:
http://drupal.org/coding-standards

And this one for specifics about tests (it needs updating though)
http://drupal.org/node/325974

I think that this is something that should be tested.

The test that I wrote validates two components:

1. The configuration field is present on the admin config people accounts page.
2. The configured Notification E-mail address is currectly used as expected

I think that the first validation is beneficial because it prevents accidental removal of the configuration option.

The second validation is beneficial because it ensures that new user account notifications are being recieved by the expected admin. Without testing, this could be inadvertently removed and a Drupal admin may stop receiving new account emails as a result.

I've corrected the whitespace issues, completed any incomplete sentences in the commets, and fixed what coding standard violations I observed.

I've also adjsuted the configuration field assertation to use assertRaw checking for the id of the input field as I think that 'E-mail address' is too generic a string to look for. Additionally, I think that 'E-mail address' is not a descriptive enough label and would suggest changing it to 'Notification e-mail address' or something similar.

This case also mentions having the ability to specify multiple admin e-mail addresses. I do not think that this functionality is present in the current patch nor do I think it is possible without a more significant and robust modification to the existing behavior.

Supporting multiple admin notification e-mail addresses would require determining which address to use as the sending address on the notification that is sent to the user.

This could be assumed to be the first listed address, but that seems like it would be confusing to the non-developer admin. Additionally, the benefit of having the notification e-mail address be used as the sender of the user message is that the user can reply to the message. If only one of many addresses is used as the sender, then the other admin's would not receive the user's reply. The only way that I see around this issue is to use cc: for the other admin addresses, but that doesn't prevent the user from clicking reply instead of reply all.

Interdiff for previous post

@c4doug The changes look good.

Note that your interdiff is ... backwards. It shows adding the lines you deleted. But I go the meaning from it anyway.
For next time, be in the most recent branch and diff to the older.
For example if I:
git checkout 8.x
git pull --rebase
git branch notify
git apply --index drupal-whatever_the_issue-NNNN-X.patch
git commit -m "previous patch"
make my changes

then I can:
git diff > interdiff-X-Y.txt
git diff 8.x > drupal-whatever_the_issue-NNNN-Y.patch

Or whatever workflow you like.
For more options:
http://drupal.org/node/1488712
http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...

I created an issue/494518 branch and had already committed the test and the updated version.

I ran:
git diff HEAD..HEAD~1
where I should have run
git diff HEAD~1..HEAD

I am attaching the corrected version of the interdiff for the benfit of others

@c4doug I agree this is just adding one address (an alternative one diff than the default admin).

Please update the issue summary to reflect that.
I dont know if that needs a separate issue or if we just wont worry about it.

I agree also that the label on the field could be more specific.

Next steps:
improve the label on the field (needs work for that)

post a combined patch with that change and the tests (can post an interdiff then to 36)
and a test only patch (I know you just did, but it's just the way.)

then a review.

Thanks for progress on this everyone!

Reverting the issue summary to indicate that only one notification email address can be specified.

Adjusted the admin config label to be more descriptive.

Attached is a new patch for the issue with the corresponding interdiff from #36 and the previously attached test patch, which remains unchanged.

One other thing - the comments should be wrapped at 80 characters per http://drupal.org/node/1354. Thanks for all your work on this @c4doug!

changing to needs review

Crosspost.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminTest.phpundefined
@@ -99,4 +99,49 @@ function testUserAdmin() {
+    // Test custom user registration approval email address(es).

There may be other places also, but the comments should just refer to one notification address, not address(es).

Also, I wanted to clarify, that the standards allow adding blank lines to separate code blocks. (just not multiple ones in a row)
Sometimes newlines are specifically allowed or not. Like at the beginning and ending of a class, or after ... well, sometimes I just search for "blank" in the standards.)

Oh, and the patch is what a core committer will commit, so that needs to have the fix, and also the tests.

So its common to have:
patch (the fix and the tests)
interdiff (to the last time the patch was posted)
tests only (so the testbot can show that the tests are testing for what the tests are testing for)

ack! crosspost central. :)

Wow, the crossposting is in full form!

Okay, I've attached the following:

• A new patch that a core committer can commit. It includes the fix and the tests.
• An interdiff of the fix patch from #45
• A new patch for the test with adjusted comments
• An interdiff of the test patch from #45

Thanks for all the patience!

rerolling

OH! I have slightly missled you in that the just tests should be attached first.
That way it will fail, d.o will mark it needs work,
then the second patch, with the fix and patch will pass and d.o will leave it needs review since that patch will pass.

@YesCT: Thanks for the heads-up on how to trick d.o.

I only rerolled the changes that were generated by this issue and did not apply the patch from Convert register_pending_approval_admin email to config system. As a result, no email is sent to the admin and the test fails.

I'm glad to see that the tests work as designed!

I see @c4doug has included all the changes from the latest patch in #1804926: Convert register_pending_approval_admin email to config system and provide the ability to configure it in UI. into the patch in #57.

Is the correct process to add a comment to #1804926: Convert register_pending_approval_admin email to config system and provide the ability to configure it in UI. as being included in this issue, and to mark it as closed? (Just trying to learn the correct process)

Probably the other issue, which is RTBC, will get committed, and then we will reroll this to take out the duplication. Then this can get committed.

Having the other patch in this issue makes it easy to use the testbot.

What is usually done? Well.. Sometimes, this would be marked postponed on the other... But it's actually useful in this case to use the time to keep working on this issue.

Next steps here: review to get this as ready as possible while we wait.

Oh @edrupal a side note/tip: you can use the pattern [#NNNNNNN] and use the node number is an issue, and d.o will make it a link to the issue, get the title automatically, and color it based on its status (like light green for RTBC). There is a hint in the help text below the comment text area that remind of the syntax. :)

Cool, better than a David Blaine slight of hand :-)

#1804926: Convert register_pending_approval_admin email to config system and provide the ability to configure it in UI. got in.

Rerolling patch in #56

#63: drupal-custom_notification_sender_address-494518-63.patch queued for re-testing.

drupalcon Sydney

Updated issue summary with remaining tasks to help new people.

Edit issue summary - Sydney 2013

Updated issue summary.

readyman and trpurcell reviewed the patch as part of the code sprint at SydneyDrupalcon 2013

tested the function prior to applying the patch from comment #63
applied the patch and retested, creating new user, blocking user and cancelling account with email notification checked.

worked as documented

The tests and code look great.

The help text is a little hard to parse. It requires scrolling back to look at the field to understand. Perhaps something like the below might be easier to understand. Bold below to highlight change, not suggested for the actual help text.

The e-mail address to be used as the 'from' address for all notifications. If 'Who can register accounts? Visitors, but administrator approval is required' is selected above, a notification email will also be sent to this address for any new registrations. Leave empty to use the default system e-mail address (drupal8@example.com).

Code works - which is good.

My only issues with the interface are that:

• The field isn't in a fieldset and this seems out of keeping with the rest of the page.
• Perhaps the wording could be altered a little bit as explained in #67

These issues can easily be fixed and are only cosmetic. As it is, I think the above patch is RTBC.

I was initially a little bit skeptical about this, but the arguments at the start of this issue are pretty compelling. Particularly for the use case of site visitors needing approval, which is the default setting. If that's going to null@null.null, that's obviously un-good. :)

I made two changes prior to commit:

1) + function testNotificationEmailAddress() { didn't have any PHPdoc, so I added a line.

2) I found "The e-mail address to be used as the 'from' address for all notifications. " a bit confusing. It's not all notifications, it's all account-related notifications, or more specifically, the ones that are listed below. Changed to "...for all account notifications listed below."

Committed and pushed to 8.x. Thanks!

awesome. @webchick, thanks for those additions

For those who are interested in a "quick fix" and don't want to go mucking about in Drupal 7 Core, I recommend the Rules module.

http://drupal.org/project/rules

Note, that if you want to use a token of a custom multiple-select field to set the Email, you'll have to adjust the custom display settings for Tokens. See this discussion for more info.

http://drupal.org/node/1781498

Updated issue summary.

I am not sure that this issue needs a change record. I think it does. I am pretty sure that "Needs backport" should be a status, not an issue tag.

I am also not sure if this should be ported to D7. If you think it should, then wait for the change record, then convert the Category to "feature request".

I am pretty sure that "Needs backport" should be a status, not an issue tag.

1) That status doesn't exist
2) That tag has been used at other times for other issues that I've worked on - and backported to D7.

I am also not sure if this should be ported to D7.

It should, as I needed this functionality on a D7 site the other day, which is why I changed the issue status.

I have no idea what a change report is, but there is no reason that we should put off backporting to D7 while waiting for it, that's just adding bureaucracy which slows things down. And as anyone who has worked on issues in Drupal knows, the bureaucracy is one of Drupal's biggest weaknesses.

I'm with Jaypan on this one. It sounds like useful functionality to me. I may not always use it, but I think it'd be a shame if I had to install a contrib module to get this functionality specifically.

I drafted a change record here https://drupal.org/node/2281541

Removed novice and needs change record tag, also set to needs review (per https://drupal.org/contributor-tasks/draft-change-record).

I'm note sure this is a Critical issue for D8, regardless of any discussion about backporting? Is this really a backport candidate, wouldn't that be a contrib as this is a new feature?

Marked needs review for the change notice

@Jaypan: Perhaps I should have suggested using the status "Patch (to be ported)" instead of the tag "Needs D7 backport". According to Needs D7 backport there are no other issues currently using this tag. After reading https://drupal.org/issue-tags/special I have added the tag "Needs backport to D7". Auto-complete is your friend.

Change records (not "change reports") are listed here: https://drupal.org/list-changes . Instructions for drafting them are at https://drupal.org/contributor-tasks/draft-change-record .

@bigjim: I believe that @xjm, at a previous sprint, said that all change records are considered critical. Thanks for taking on the task.

Published the change record.

Issues get tagged 'Needs backport to D7' when they're filed against 8.x - it's a reminder that it should be backported once the 8.x fix has gone in.

Once it's fixed against 8.x, then it gets moved to 'to be ported' - can leave the tag on since that helps to identify backported issues vs. issues that are newly opened only against 7.x.

Not sure this actually get backported since it's a new feature and alterable in contrib, but moving there for now.

Patch for D7.

Great work @aerozeppelin

after looking at the first test testbot errors, it appeared that they were unrelated to the patch.

So I kicked the testbot and the second test passes.

Congrats, your backport patch passes, and it looks good.

rtbc+1

I have applied patch(494518-84.patch) and tested. Its working fine.

Patch 84 looks great. This backport is nicely refactored from the original D8 patch and the simpletest is passing and the expected functionality is working.

Thank you @joseph.olstad for taking the time reviewing my patch. I am glad it worked out.

This looks like a very useful feature - thanks! But there are a few issues with the patch.

1.      '#description' => t("This text is displayed at the picture upload form in addition to the default guidelines. It's useful for helping or instructing your users."),
      );
   -
   +  // Default notifications address.
   

   (minor) I think this should preserve the blank line above and below the new code, rather than removing it. It's not really related to the other code around it so it's good to have it visually separate.

2. +    '#default_value' => variable_get('mail_notification_address'),
   

   Shouldn't this be namespaced to the User module (like most other variables on the form)? In other words, 'user_mail_notification_address'.

3. +    '#description' => t("The e-mail address to be used as the 'from' address for all notifications. If <em>'Visitors, but administrator approval is required'</em> is selected above, a notification email will also be sent to this address for any new registrations. Leave empty to use the default system e-mail address <em>(%site-email).</em>", array('%site-email' => variable_get('site_mail'))),
   

   I realize some of this is an issue with the Drupal 8 form too, but:

   • Should use double-quotes rather than single quotes in the text on the screen (e.g., "from").
   • I don't think "Visitors, but administrator approval is required" should have quotes at all since it's already enclosed in <em>.
   • Should consistently use "e-mail", not "email".
   • For <em>(%site-email).</em>, the <em> wrapper is unnecessary since the % placeholder already does that (so this is really just adding nested <em> tags).
   • I think it should be variable_get('site_mail', ini_get('sendmail_from')) since that's what is used elsewhere.

4. +  if (empty($site_mail)) {
   +    $site_mail = variable_get('site_mail');
   +  }
   +  if (empty($site_mail)) {
   +    $site_mail = ini_get('sendmail_from');
   +  }
   

   It's cleaner to just check if (!$site_mail).

   Also, why the two separate lines? Can't it just use a single $site_mail = variable_get('site_mail', ini_get('sendmail_from')) like the previous code did?

5. +  /**
   +   * Test user registration approval email is sent to an custom email
   +   * address, if it is set.
   +   */
   

   (minor) This should be on one line, also "a" not "an". It could just be something like "Tests the user registration notification e-mail address."

6. (minor) Throughout the test, "Notification E-mail" should not be capitalized like that - it should just use regular sentence case. Also all the assertion messages should be sentences (with periods at the end).

Update to patch based on comments in #90.

@webchick Since you committed the patch, shouldn't the issue be marked Fixed?

Unclear how this can be 'fixed' if the patch for D7 hasn't been committed - or has it transferred to a different issue?

Reviewed the interdiff based on #90, only minor discrepancy I saw was (!($site_mail)) should be (!$site_mail), I guess it can be fixed on commit if needed, but otherwise looks good.

Bumping to 7.70, this didn't make it into 7.60

The patch in #91 does not apply anymore, it needs to be rerolled.

In addition to the codestyle issue mentioned in #96 I found some others:

+    '#description' => t("The e-mail address to be used as the \"from\" address for all notifications. If <em>Visitors, but administrator approval is required</em> is selected above, a notification e-mail will also be sent to this address for any new registrations. Leave empty to use the default system e-mail address (%site-email).", array('%site-email' => variable_get('site_mail', ini_get('sendmail_from')))),

The text commited to the D8 was slightly changed, I think we should update this. Specifically add this part "for all account notifications listed below" instead of "for all notifications".

+  // Get the custom site notification email to use as the from email address
+  // If the custom site notification email has not been set, we use the site
+    // Test custom user registration approval email address(es).
+    // Set the site and notification email addresses.

There are still comments using "email" rather than "e-mail" (see the comment from @David_Rothstein - #90/3).

+    // Test that the Notification E-mail address field is on the config page.
+    // Notification E-mail address.
+    // Notification E-mail address.

There are still comments using "Notification E-mail" rather than uncapitalized version (see the comment from @David_Rothstein - #90/6).

I suggest that we open a new issue where the backport could be finished (according to the backport policy), because the patch still needs some work. Then we can close this for D8 as Fixed.

Also this was originally a Feature request (Task was set temporarily when the Change record was needed).

New patch on it's way

see
#3306690: D7 - Allow for custom user registration approval email address

Great, thanks! So we can close this.

No change record here :(

According to the #77, this seems to be the change record for this issue: https://www.drupal.org/node/2281541. I am not sure why it is not linked here anymore.