Problem/Motivation

New users registering accounts on a Drupal site can lose the welcome email to their spam filtering software or 3rd party email provider error.

New users can be created/imported into a Drupal system in bulk, and then it would be useful to be able to send a welcome email manually afterwards - #8.

Proposed resolution

Add the ability for users with the requisite permissions to (re)send the welcome email to a user by visiting user/edit/XXX or using batch action in /admin/people.

Only users who can register accounts can (re)send the welcome email - #71

To test: Apply patch and run update.php

Before
User edit page has no email send button

No Send email action at /admin/people

After
User edit page has new email send button

New send mail action at /admin/people

Message displayed after sending email to two users

Remaining tasks

Usability review
Respond to the usability review
Review

User interface changes

A new button "Send welcome message" appears on the user edit page for users with the correct permissions to use it.

API changes

None.

Release notes snippet

https://www.drupal.org/node/3109478

Original report by @Chris Johnson

It would be nice if the admin could force a resend of the welcome message from the user edit page.

One case where this would be useful:
Many people are using spam-filtering software these days which make use of white lists, and the Drupal website sending address is not yet known to the user requesting an account.

CommentFileSizeAuthor
#231 interdiff_230-231.txt1.03 KBpradhumanjain2311
#231 5688-231.patch14.43 KBpradhumanjain2311
#230 5688-230.patch19.59 KBJurgenR
#227 5688-227.patch19.01 KBweseze
#226 5688-226.patch19.06 KBweseze
#223 5688--resend--browserstack.png82.29 KBsokru
#223 5688--resend--pantheon.png64.01 KBsokru
#223 5688-223.patch19.06 KBsokru
#222 drupal-resend_email-5688-222.patch19.02 KBVladimirAus
#219 SendWelcomeMessageButton.png18.68 KBquietone
#219 SendEmailToTwoUsers.png10.67 KBquietone
#219 Before-NoSendEmailButton.png13.2 KBquietone
#219 After-WithSendAction.png31.33 KBquietone
#219 Before-NoSendAction.png26.34 KBquietone
#218 5688-218.patch19.02 KBquietone
#218 interdiff-216-218.txt2.88 KBquietone
#216 5688-216.patch18.86 KBSuresh Prabhu Parkala
#212 Patch 5688 Error1.png700.61 KBchetanbharambe
#212 Patch 5688 Error.png1.17 MBchetanbharambe
#211 interdiff-209-211.txt1.12 KBsokru
#211 5688-211.patch18.85 KBsokru
#210 resend_welcome_after_apply_patch.png52.8 KBvikashsoni
#210 Before_apply_patch.png44.02 KBvikashsoni
#209 interdiff_207-209.txt714 bytesvsujeetkumar
#209 5688-209.patch18.85 KBvsujeetkumar
#207 5688-207.patch18.82 KBsokru
#207 interdiff-202-207.txt20.05 KBsokru
#202 interdiff_199-202.txt1.02 KBMeenakshiG
#202 5688-202.patch18.89 KBMeenakshiG
#199 interdiff_196_199.txt1.09 KBabhisekmazumdar
#199 5688-199.patch18.89 KBabhisekmazumdar
#196 interdiff.5688.194-196.txt1.61 KBabhisekmazumdar
#196 5688-196.patch18.71 KBabhisekmazumdar
#194 interdiff.5688.192-194.txt973 bytesabhisekmazumdar
#194 5688-194.patch18.41 KBabhisekmazumdar
#192 interdiff_185-192.txt7.09 KBadityasingh
#192 5688-192.patch17.69 KBadityasingh
#185 5688-185-resend_welcome_email.patch17.65 KBayushmishra206
#185 interdiff_183-185.txt522 bytesayushmishra206
#183 interdiff-181-182.txt1.17 KBErik Frèrejean
#183 5688-182-resend_welcome_email.patch17.65 KBErik Frèrejean
#181 5688-181-action--after.png382.75 KBsokru
#181 interdiff-179-181.txt5.6 KBsokru
#181 5688-resend-welcome-email-181.patch18.82 KBsokru
#181 5688-181-bulk-message--after.png43.68 KBsokru
#181 5688-181-message--after.png44.98 KBsokru
#181 5688-181-user-profile-form--after.png24.7 KBsokru
#181 5688-181-action--after.png382.75 KBsokru
#179 interdiff_177_179.txt1.66 KBanmolgoyal74
#179 5688-resend-welcome-email-179.patch19.46 KBanmolgoyal74
#178 diff-171-175.txt1.51 KBquietone
#178 diff-168-171.txt1.42 KBquietone
#177 interdiff-175-177.txt1.07 KBsokru
#177 5688-resend-welcome-email-177.patch19.47 KBsokru
#175 5688-resend-welcome-email-175.patch19.46 KBsokru
#173 5688-resend-welcome-message-173.patch21.21 KBkamkejj
#171 diff-168-171.txt5.7 KBkishor_kolekar
#171 5688-resend-welcome-email-171.patch19.46 KBkishor_kolekar
#168 interdiff_166-168.txt465 bytessokru
#168 5688-resend-welcome-email-168.patch19.41 KBsokru
#166 interdiff-163-166.txt2.08 KBkishor_kolekar
#166 5688-resend-welcome-email-166.patch19.48 KBkishor_kolekar
#163 interdiff-160-163.txt6.58 KBkishor_kolekar
#163 5688-resend-welcome-email-163.patch19.49 KBkishor_kolekar
#160 interdiff_158_160.txt3.86 KBErik Frèrejean
#160 5688-resend-welcome-email-160.patch19.57 KBErik Frèrejean
#158 5688-resend-welcome-email-158.patch21.23 KBErik Frèrejean
#153 interdiff_150-152.txt1.87 KBsokru
#152 5688-resend-welcome-email-152.patch21.22 KBsokru
#151 5688-resend-welcome-email-151-1.patch21.21 KBJurgenR
#150 interdiff_149-150.txt2.6 KBsokru
#150 5688-resend-welcome-email-150.patch21.23 KBsokru
#148 interdiff_145-149.txt656 bytessokru
#148 5688-resend-welcome-email-149.patch19.16 KBsokru
#145 5688-user-profile-form--after.png30.14 KBsokru
#145 5688-action--after.png168.34 KBsokru
#145 5688-resend-welcome-email-145.patch19.23 KBsokru
#143 5688-resend-welcome-email-143.patch17.66 KBsokru
#136 5688-resend-welcome-email-136-D8.patch18.13 KBsokru
#135 5688-resend-welcome-email-135-D8.patch30.94 KBsokru
#134 5688-resend-welcome-email-134-D8.patch34.52 KBsokru
#133 5688-resend-welcome-email-132-D8.patch34.52 KBsokru
#131 5688-resend-welcome-email-131-D8.patch18.09 KBsokru
#128 5688-127.patch18.09 KBErik Frèrejean
#125 5688-125.patch18.1 KByogeshmpawar
#122 5688-122.patch18.1 KBweseze
#120 resend-welcome-message-5688-120.patch9.34 KBweseze
#116 interdiff-5688-113-116.txt2.13 KBwengerk
#116 5688-116.patch18.11 KBwengerk
#5 5688.001.patch1.28 KBkarschsp
#25 resend-welcome-message.5668.025.patch2.37 KBkarschsp
#27 resend-welcome-message.5668.027.patch2.32 KBbass28
#33 resend-welcome-message-5688-33.patch2.65 KBmarji
#36 resend-welcome-message-5688-36.patch2.67 KBjlscott
#38 interdiff-5688-36.txt946 bytesjlscott
#44 resend_after.png35.38 KBadammalone
#44 resend_before.png33.27 KBadammalone
#47 interdiff-5688-47.txt1.14 KBmsypolt
#47 resend-welcome-message-5688-47.patch2.77 KBmsypolt
#51 resend-welcome-message-5688-51.patch2.81 KBbecw
#54 resend-welcome-message-5688-54.patch3.43 KBbecw
#55 resend-welcome-message-5688-55.patch3.62 KBbecw
#56 resend-welcome-message-5688-56.patch4.05 KBbecw
#58 resend-welcome-message-5688-58.patch5.31 KBkbentham
#60 resend-welcome-message-5688-60-interdiff.txt3.64 KBbecw
#60 resend-welcome-message-5688-60.patch4.66 KBbecw
#63 resend-welcome-message-5688-63.patch4.74 KBmahaprasad
#65 interdiff-5688-65.txt740 bytesmahaprasad
#65 resend-welcome-message-5688-65.patch4.74 KBmahaprasad
#68 interdiff-5688-68.txt5.06 KBjoestewart
#68 resend-welcome-message-5688-68.patch4.26 KBjoestewart
#73 resend_after.png51.97 KBrakeshfalke
#83 resend-welcome-message-5688-83.patch3.58 KBandreasderijcke
#84 resend-welcome-message-5688-84.patch3.58 KBandreasderijcke
#86 resend-welcome-message-5688-86.patch3.67 KBErik Frèrejean
#88 resend-welcome-message-5688-88.patch9.43 KBErik Frèrejean
#90 resend-welcome-message-5688-90.patch10.69 KBErik Frèrejean
#96 5688-96.patch12.58 KBwengerk
#96 interdiff-5688-90-96.txt5.26 KBwengerk
#100 5688-100.patch12.59 KBErik Frèrejean
#100 interdiff-5688-98-100.txt593 bytesErik Frèrejean
#101 5688-101.patch12.59 KBErik Frèrejean
#101 interdiff-5688-100-101.txt462 bytesErik Frèrejean
#107 interdiff-5688-101-107.txt7.12 KBwengerk
#107 5688-107.patch16.96 KBwengerk
#109 interdiff-5688-107-109.txt2.01 KBwengerk
#109 5688-109.patch18.47 KBwengerk
#110 interdiff-5688-107-110.txt2.34 KBwengerk
#110 5688-110.patch18.32 KBwengerk
#113 interdiff-5688-110-113.txt6.06 KBwengerk
#113 5688-113.patch18.37 KBwengerk

Issue fork drupal-5688

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

coreb’s picture

Version: x.y.z » 6.x-dev

Moving from x.y.z queue to 6.x-dev.

Pasqualle’s picture

Version: 6.x-dev » 7.x-dev
Anonymous’s picture

+1

I have this problem now with Drupal 6.
I have first created a list of users without sending them notification.
And know it's too late : I can't send them notification !

jjma’s picture

I would like this functionality as well.

Jon

karschsp’s picture

Status: Active » Needs review
FileSize
1.28 KB

Here's a patch that adds a "Re-send welcome e-mail" checkbox to the user/edit form.

Status: Needs review » Needs work

The last submitted patch failed testing.

babbage’s picture

Subscribing—I need this and may try and review this patch in the next couple of weeks if I can find time, and if no-one has fixed it by then. :)

jeffschuler’s picture

The Account Reminder module is built somewhat for this purpose, though without a means for manually sending these reminders, but, rather, automated sending based on new users who haven't yet logged-in. See issue #355648: add functionallity to user edit page? for getting manual sending into Account Reminder.

I do think such functionality could be a worthwhile addition to core, though. Often, one creates/imports a number of user accounts then wants to send out the notification...

I think part of the issue is that their password can't be sent to them anymore, so the message would need to say "Login if you remember your password, or click to re-set it..."

milerosu’s picture

Hi, thanks for the patch, it worked for me with a small fix: $array['resend'] == 1 instead of $edit['resend'] == 1

babbage’s picture

Version: 7.x-dev » 8.x-dev

I guess this is moving to 8.x now... I think getting it into Account Reminder in Contrib for 7.x would thus be a great idea.

hornetnz’s picture

subscribing

izmeez’s picture

subscribing

jfmoore’s picture

Status: Needs work » Needs review

#5: 5688.001.patch queued for re-testing.

SocialNicheGuru’s picture

The change in #9 to the patch worked for me.

I am surprised this was not already in there.

fjen’s picture

+1

lubnax’s picture

subscribe

servantleader’s picture

+1

Status: Needs review » Needs work

The last submitted patch, 5688.001.patch, failed testing.

axaubet’s picture

Status: Needs work » Needs review

#5: 5688.001.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 5688.001.patch, failed testing.

seanmcginley’s picture

Patch adds the checkbox to resend welcome email.... but does not send the email.

John Bickar’s picture

As a workaround, you can deactivate and re-activate the user(s) account(s). Reactivating it will re-send the activation welcome message.

izmeez’s picture

It seems like there's a lot of discussion on d.o. about what to take out of core to make it leaner and more maintainable and what to put in.

It seems that if we had a core with links to some commonly useful contrib modules possibly even with the ability to download and enable them it might be possible to achieve both, a leaner core and a more friendly install for users. With that in mind I would advocate for the Account reminder module as a contrib module with new features to be added there.

Maybe there is already an issue open to add a feature to core to link to and enable commonly useful contrib modules. I guess the advanced help does some of this, but now I'm rambling and should save it for specific issue queues.

GeminiAgaloos’s picture

+1 to this. Drupal should have an admin functionality that allows manual resend of welcome message. I vote for this to be added to core because i believe user management should be integral to core.

karschsp’s picture

Status: Needs work » Needs review
FileSize
2.37 KB

Two and a half years later, here's a patch that adds a "Re-send welcome message" button to the user edit form.

meba’s picture

In your patch "$account->uid > 1 &&" why can't a user 1 send a welcome message?

bass28’s picture

Version: 8.x-dev » 7.x-dev
FileSize
2.32 KB

Here a version of this patch for the Drupal 7.x branch.

BrockBoland’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work
Issue tags: +Needs backport to D7

Appreciate the D7 patch, but this needs to be addressed in 8.x first.

Set back to Needs Work, since the patch in #27 needs to be forward-ported for the new file structure in 8.x. Once that is accepted, it can be set to 7.x for potential inclusion in that branch.

ratinakage’s picture

Assigned: Unassigned » ratinakage

I'm going to port this patch for Drupal 8.x

Anybody’s picture

Is this work in progress for D7? It seems that development is currently sleeping?
What's the current plan for this?

It would be nice if you could provide some information.

Rob C’s picture

I created this not to long ago, but it still needs some work.
https://github.com/ClusterFCK/user_mail_actions/blob/master/user_mail_ac...
Github: https://github.com/ClusterFCK/user_mail_actions
D.o.: http://drupal.org/sandbox/ClusterFCK/1819072
That's for D7.

Patch #28 looks interesting, i might wrap that into my custom module, that would fix up D7.

D8 patch in the works for the bulk-re-send functionality, i hope to get it finished in time, will cross link it here when i post it.

YesCT’s picture

any update?

marji’s picture

Status: Needs work » Needs review
FileSize
2.65 KB

forward ported for D8, based on #27 (as part of DrupalCon Sydney 2013 code sprint :)
Had to modify two filles - ProfileFormController.php and user.pages.inc

adammalone’s picture

Status: Needs review » Needs work
  $element['resend']['#access'] = $account->uid > 1 && (($account->uid == $GLOBALS['user']->uid && user_access('cancel 

In Drupal 8 users may be created without an email address. It seems logical that if the user has no email address then the button doesn't appear.

YesCT’s picture

From irc convo,
http://drupal.org/project/user_registrationpassword has some code in the dev release that deals with this for D7. D8 is another story.

jlscott’s picture

Status: Needs work » Needs review
FileSize
2.67 KB

Rerolled the patch from #33 to suppress the "resend" button if the account does not have an email address defined.

Status: Needs review » Needs work

The last submitted patch, resend-welcome-message-5688-36.patch, failed testing.

jlscott’s picture

FileSize
946 bytes

Interdiff for the patch in #36 attached.

Not sure why if failed. Looks like some additional tests have been introduced since #33 passed. Will need to investigate further.

Edit: Yes, there are a number of "customblock" tests and a date-time test that have been introduced. The failing test is "CustomBlockTransalationUITest" which does not make a lot of sense to me. I think the error message is "Invalid values generate a list of form errors." from EntityTranslationUITest.php.

Any suggestions anyone?

jrsinclair’s picture

Assigned: ratinakage » jrsinclair

Assigning to myself for Drupal Sprint weekend D8 work.

jrsinclair’s picture

Patch from #38 appears to be generating an error message like the following:

Warning: call_user_func_array() expects parameter 1 to be a valid callback, function 'user_edit_resend_submit' not found or invalid function name in form_execute_handlers() (line 1544 of core/includes/form.inc).

Will do some investigation.

...

And it appears that I applied the patch wrong. Mia culpa.

jrsinclair’s picture

Status: Needs work » Needs review
jrsinclair’s picture

Assigned: jrsinclair » Unassigned
Status: Needs review » Reviewed & tested by the community

Test passed after re-submitting. In my review, it worked as advertised. Marking as RTBC.

YesCT’s picture

Issue tags: +Needs screenshots

a before and after screenshot would be nice.

adammalone’s picture

Issue tags: -Needs screenshots
FileSize
35.38 KB
33.27 KB

Before
resend_before.png
and After
resend_after.png

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This looks like a really useful feature! Unfortunately right now we are ridiculously above thresholds, so this won't be able to be committed anytime soon, but looking forward to when it can be. :)

In the meantime...

+++ b/core/modules/user/lib/Drupal/user/ProfileFormController.phpundefined
@@ -26,6 +26,11 @@ protected function actions(array $form, array &$form_state) {
+    $element['resend']['#access'] = $account->uid > 1 && $account->mail && (($account->uid == $GLOBALS['user']->uid && user_access('cancel account')) || user_access('administer users'));

My goodness, that's a long condition. :) Probably worth a comment, or even better to break it up somehow.

+++ b/core/modules/user/user.pages.incundefined
@@ -409,3 +409,39 @@ function user_page() {
+/*
+ * Submit function for the 'Re-send welcome message' button on the user edit form.
+ */

(nitpick) That first line needs to be /** rather than /*

+++ b/core/modules/user/user.pages.incundefined
@@ -409,3 +409,39 @@ function user_page() {
+function user_edit_resend_submit($form, &$form_state) {

In general, this function could use a few in-line comments to sort of explain the gist of what's happening at each major chunk.

+++ b/core/modules/user/user.pages.incundefined
@@ -409,3 +409,39 @@ function user_page() {
+  global $language;
+  $destination = array();
+  if (isset($_GET['destination'])) {
+    $destination = drupal_get_destination();
+    unset($_GET['destination']);
+  }

Hm. That sort of futzing with global variables is a bit strange, and not something I've seen done before in similar submit functions. Is there a reason why that's strictly necessary? We're really trying to remove globals in general from D8.

+++ b/core/modules/user/user.pages.incundefined
@@ -409,3 +409,39 @@ function user_page() {
+  switch ($user_register) {
+    case 0:
+      $op = 'register_admin_created';
+      break;
+    case 1:
+      $op = 'register_no_approval_required';
+      break;
+    case 2:
+      $op = 'register_pending_approval';
+  }

These should be using named constants (e.g. USER_REGISTER_VISITORS), rather than integers.

And finally, we should get some automated tests for this feature to ensure it continues to work.

msypolt’s picture

I am working on this as part of Portland2013 Code sprint.

msypolt’s picture

The attached patch improves commenting for the code. Still needs and automated test written for it, which someone else should do.

heddn’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, resend-welcome-message-5688-47.patch, failed testing.

Andre-B’s picture

Assigned: Unassigned » Andre-B

going to reroll this in the next days

becw’s picture

Status: Needs work » Needs review
FileSize
2.81 KB

Here's a rerolled patch for the testbot.

becw’s picture

Assigned: Andre-B » becw

There are a couple more issues with this patch. I'm going to steal this issue and work on it now.

Status: Needs review » Needs work

The last submitted patch, resend-welcome-message-5688-51.patch, failed testing.

becw’s picture

Status: Needs work » Needs review
FileSize
3.43 KB

I didn't test the patch when I re-rolled it, and there were issues with the form redirect, the button #access logic, and changes to the UserInterface (use the getEmail() and getUsername() methods).

Here's a working version of the patch. The major issue with this code is that the user object should be passed to the profile form using dependency injection instead of using the global user object, and $user->hasPermission() should be used instead of user_access(). Also, variable_get() might be the wrong way to retrieve module configuration variables now.

becw’s picture

Here's a patch without variable_get(). Almost there.

becw’s picture

Assigned: becw » Unassigned
FileSize
4.05 KB

In order to eliminate references to the global user object, we need access to the request object somewhere in the form. Per #1987692: Convert all references to entity_get_form() as page callbacks to _entity_form w/ corresponding routing.yml entries, the user routes have not yet been converted to the new routing system, which means that we can't yet access the request object properly.

Here's the patch that should work after the new routing is wired up. This will fail tests until then.

Status: Needs review » Needs work

The last submitted patch, resend-welcome-message-5688-56.patch, failed testing.

kbentham’s picture

Status: Needs work » Needs review
FileSize
5.31 KB

Here is a working version of the patch with tests. The tests fail because of a bug with assertMail. See issue #2067259: AssertMailTrait::assertMail can't test multiple mails for more details.

becw’s picture

Status: Needs review » Needs work

That test was supposed to fail! Something is wrong here.

becw’s picture

Ok, the test passed because user.settings.register defaults to USER_REGISTOR_VISITORS, which sends one email, rather than USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL, which sends one email to the user and one to the administrator.

So that's good. I did a little cleanup on the patch and updated some @todos. This is still stalled on #1987692: Convert all references to entity_get_form() as page callbacks to _entity_form w/ corresponding routing.yml entries, though.

Pasqualle’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, resend-welcome-message-5688-60.patch, failed testing.

mahaprasad’s picture

Status: Needs work » Needs review
FileSize
4.74 KB

Rerolled the patch for #60

Status: Needs review » Needs work

The last submitted patch, resend-welcome-message-5688-63.patch, failed testing.

mahaprasad’s picture

Status: Needs work » Needs review
FileSize
740 bytes
4.74 KB

Fixed for simpletest.

Status: Needs review » Needs work

The last submitted patch, resend-welcome-message-5688-65.patch, failed testing.

joestewart’s picture

Working on a reroll at DC Atlanta 2013 code sprint.

joestewart’s picture

Status: Needs work » Needs review
FileSize
5.06 KB
4.26 KB

rerolled.

Mentored by kbasarab.

Stefan Lehmann’s picture

Stefan Lehmann’s picture

Status: Needs review » Needs work

I tested the patch and well, it works. I got a new button on the user edit form to send the mail, BUT .. the button is shown regardless if the user is active or blocked. Even if the user is active it will send a mail like:

user name,

Thank you for registering at Name of Website. Your application for an
account is currently pending approval. Once it has been approved, you will
receive another e-mail containing information about how to log in, set your
password, and other details.

.. which makes no sense imho. So either the button should only be shown for in-active users or the mail text should respect the current state of the user. Although I'm not sure if it makes sense to send welcome mails to active users at all.

joestewart’s picture

Status: Needs work » Needs review

@Stefan Lehmann which email is sent corresponds to the setting in the $user_register variable.

At /admin/config/people/accounts

Who can register accounts?

Administrators only
Visitors
Visitors, but administrator approval is required

Each choice sends the email corresponding to that setting for me.

rakeshfalke’s picture

Patch resend-welcome-message-5688-68.patch By joestewart working properly.
Find attached screenshot.
Thanks!

rakeshfalke’s picture

Issue summary: View changes
FileSize
51.97 KB
kattekrab’s picture

thedavidmeister’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
thedavidmeister’s picture

Issue summary: View changes
thedavidmeister’s picture

Status: Needs review » Needs work

+ $cancel_own = ($account->id() == $this->user->id()) && $this->user->hasPermission('cancel account');

I see no need for this change here, it is not relevant to this new functionality and so should not be in this patch.

+ $element['resend']['#access'] = $normal_user && $account->getEmail() && $this->user->hasPermission('administer users');

I don't see any reason to remove the ability to re-send the welcome email to user 1. Maybe you were using user 1 to build a site and your client wants to use it, and you want to send them a welcome email?

Restricting deleting of user 1 makes sense because you break your site without it, this seems harmless so we might as well leave it in.

+      case USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL:
+      default:
+        $op = 'register_pending_approval'

What @Stefan Lehman said in #70 is correct. Sending an email that says "your account is awaiting approval" makes zero sense if a user is active.

The use-case is "we want to send a welcome email to users" but whatever we send has to make logical sense to the end user in this scenario:

1. User registrations require administrative approval
2. An administrator creates a user and does not send them an email
3. The user is set to "active" by an administrator
4. The administrator clicks "Re-send welcome email"

The issue here is that the user never registered themselves, an administrator created their account, so it *never* makes sense to send them a "you need to be approved" email, but it *could* make sense to want to send them a welcome email manually.

I think this is ideally how the email should be selected:

- 'register_pending_approval' should always and only ever be sent to users that are currently blocked
- Whether the user was originally created by an administrator, or whether they created their own account should be logged against the user entity and *that* should determine what welcome email is sent to active users
- The button on the user edit page should have different text depending on whether the user is blocked or not "Resend welcome message" for active users and "Resend awaiting approval message" for blocked users.

#2094585: [policy, no patch] Core review bonus for #313145: Support X-Forwarded-* HTTP headers alternates

jhedstrom’s picture

Version: 8.0.x-dev » 8.1.x-dev

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andreasderijcke’s picture

Updated the patch taking into account the feedback from #77.

Only, for

- Whether the user was originally created by an administrator, or whether they created their own account should be logged against the user entity and *that* should determine what welcome email is sent to active users

there seems to be no solution in core.

andreasderijcke’s picture

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Erik Frèrejean’s picture

Removed the $language global from ::editResendSubmit and use the language manager, and a couple of small coding style changes.

jeffschuler’s picture

Status: Needs work » Needs review
Erik Frèrejean’s picture

Removed the deprecated constants in favor of the \Drupal\user\UserInterface class constants.
Also added an a new action so that it is possible to resend the welcome email in bulk.

Status: Needs review » Needs work

The last submitted patch, 88: resend-welcome-message-5688-88.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Erik Frèrejean’s picture

Status: Needs work » Needs review
FileSize
10.69 KB

Fixes the tests.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jeffschuler’s picture

The patch in #90 is working well for me, both in sending individual emails and in bulk from the /admin/people view.

weseze’s picture

Confirmed it is working on 8.6.1: both individual mails as bulk mail.

Also works fine with the contrib module views_bulk_operations.

wengerk’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Thanks all for the amazing work on this issue !

The patch #90 apply perfectly fine on 8.6.x but need a reroll for 8.7.x

Plus, we need to add a new tests on UserAdminTest to asserts the bulk operation works properly.
I would suggestion to add UserAdminTest::testBulkResendEmailsNotifications.

I may work on it during this week if nobody work on it before.

Erik Frèrejean’s picture

I won't have time to work on this in the short term, but I did see a couple of issues in my patch in #90.

+++ b/core/modules/user/src/ProfileForm.php
@@ -59,4 +65,42 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
+      \Drupal::logger('user')->notice('Welcome message has been re-sent to %name at %email.', ['%name' => $account->getUsername(), '%email' => $account->getEmail()]);
...
+      \Drupal::logger('user')->notice('There was an error re-sending welcome message to %name at %email', ['%name' => $account->getUsername(), '%email' => $account->getEmail()]);

These static calls should be replaced with the LoggerChannelTrait.

+++ b/core/modules/user/src/ProfileForm.php
@@ -59,4 +65,42 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
+      drupal_set_message($this->t('Welcome message has been re-sent to %name at %email', ['%name' => $account->getUsername(), '%email' => $account->getEmail()]));
...
+      drupal_set_message($this->t('There was an error re-sending welcome message to %name at %email', ['%name' => $account->getUsername(), '%email' => $account->getEmail()]), 'error');

drupal_set_message is deprecated. Should use the MessengerTrait.

wengerk’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs reroll
FileSize
12.58 KB
5.26 KB

Here the reroll request from #94 & the necessary changes from #95.
I also add the new tests to coverage the new "Bulk Action" from #88.

Many thanks for your review !

Status: Needs review » Needs work

The last submitted patch, 96: 5688-96.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

wengerk’s picture

Status: Needs work » Needs review
FileSize
12.82 KB
1.11 KB

Mmmhh .. Tests locally pass but fails on testbot ... Seems the order of people may vary according speed of users creation (Member for). Attempts to fix this by creating account in a fixed date in time.

Erik Frèrejean’s picture

Status: Needs review » Needs work

Just a small nitpick you might want to change when going back in to fix the test.

+++ b/core/modules/user/src/ProfileForm.php
@@ -11,6 +14,9 @@
+  use LoggerChannelTrait;
+  use MessengerTrait;

These are not necessary here as they are already part of \Drupal\Core\Form\FormBase

Besides that seems okay to me.

Erik Frèrejean’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
12.59 KB
593 bytes

Not sure what happened there, somehow we crossposted 13 hours apart...

Anyhow, rea-dd the patch from #98, with as only change the removal of the two traits from the profile form since they are defined in the form base.

Erik Frèrejean’s picture

A minor last minute thing I noticed, the schema version in hook update wasn't yet updated for 8.7.

The last submitted patch, 90: resend-welcome-message-5688-90.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

wengerk’s picture

Thanks for the review @erik-frèrejean !!

Do you think we should add another coverage of the Bulk Operation by POST 2 kind of users (actived & blocked) to asserts 2 kind of mails are sent to the end-users ? The Waiting approval is already covered by testResendAwaitingApprovalEmailNotification but we don't cover this in the Bulk action - we only cover the Welcome mail. What do you think ?

Erik Frèrejean’s picture

Well I suppose that for completeness we could add some tests to ensure that the action sends the correct email. But in that case I would advocate to test all cases (so including the difference between register_admin_created and register_no_approval_required), this would ensure that everything works as expected.

Erik Frèrejean’s picture

Component: user system » user.module
Status: Reviewed & tested by the community » Needs work

I just had a quick look through the current user module tests and it doesn't seem that there is a test in place that tests the actual sending of different emails based upon the register config. So yes I think that it is good to test the entire switch.

@wengerk, would you be able to update the tests?

wengerk’s picture

I could work on it in the next 2 weeks. If someone want to do it before - feel free - otherwise I will create them.

wengerk’s picture

Status: Needs work » Needs review
FileSize
7.12 KB
16.96 KB

Here is the new coverage.
Sorry for the delay (2 months instead of 2 weeks).

Status: Needs review » Needs work

The last submitted patch, 107: 5688-107.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

wengerk’s picture

Status: Needs work » Needs review
FileSize
2.01 KB
18.47 KB

Mmhhh... seems the only fails occur because of quality control.
Let's try to fix them.

I don't understand the

/var/lib/drupalci/workspace/jenkins-drupal_patches-78757/source/core/modules/user/src/ProfileForm.php ✗ 1 more
line 6 Unused use statement

Because the use use Drupal\user\UserInterface; is used in the switch:case as

      switch ($config->get('register')) {
        case UserInterface::REGISTER_ADMINISTRATORS_ONLY:
          $op = 'register_admin_created';
          break;

        case UserInterface::REGISTER_VISITORS:
        default:
          $op = 'register_no_approval_required';
      }

Is it a false positive ?

wengerk’s picture

My bad I found the issue: This code produced an 'Unused use statement' which leads me to the wrong direction. The problem is actually that the use is in the same namespace as the class.

https://www.drupal.org/project/coder/issues/2923699

Here a fix, sorry guys.

The last submitted patch, 109: 5688-109.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 110: 5688-110.patch, failed testing. View results

wengerk’s picture

Status: Needs work » Needs review
FileSize
18.37 KB
6.06 KB

Finally found the issue with the tests here are the changes:

  • use getAccountName instead of deprecated getUsername
  • use assertEquals instead of deprecated assertEqual
wengerk’s picture

jeffschuler’s picture

Status: Needs review » Needs work

MigrateUpgrade tests were renamed: #2987418: Rename MigrateUpgrade tests.

MigrateUpgrade6Test.php -> Upgrade6Test.php
MigrateUpgrade7Test.php ->Upgrade7Test.php

wengerk’s picture

Status: Needs work » Needs review
FileSize
18.11 KB
2.13 KB

Here the reroll, let's try it via testbot

Status: Needs review » Needs work

The last submitted patch, 116: 5688-116.patch, failed testing. View results

wengerk’s picture

Status: Needs work » Needs review

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

weseze’s picture

Status: Needs review » Needs work
FileSize
9.34 KB

The last patch does not consider the users language correctly. Drupal core handles this correctly so we should not pass the langcode at all.

_user_mail_notify($op, $account, $langcode);

should be replaced with

_user_mail_notify($op, $account);

EDIT: ignore the attached patch, it wasn't ment to be attached here. It is wrong :(

weseze’s picture

weseze’s picture

Please see attached patch to fix issue from comment 120. User mails should always be sent in the language for the user, not the language used by the admin that is sending mails.

weseze’s picture

Status: Needs work » Needs review
yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
FileSize
18.1 KB

Re-rolled the patch against 8.8.x, beacuse it's failed to apply on 8.8.x

andreasderijcke’s picture

Status: Needs review » Reviewed & tested by the community

#125 works, tested on 8.8.x with multiple languages and accounts with different preferred language. Mails are in appropriate language, both using user edit form or bulk action.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

According to d.o this fails to apply

Erik Frèrejean’s picture

Status: Needs work » Needs review
FileSize
18.09 KB

Rerolled the patch in #125, some of the expected entity counts in the migrate_drupal_ui functional tests have been changed, which caused the apply fail.

Lets see whether the tests pass this way.

Erik Frèrejean’s picture

Status: Needs review » Reviewed & tested by the community

With the tests passing again this can go back to RTBC.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

sokru’s picture

+1 for RTBC and importance of this feature. Manually tested using batch action in /admin/people, worked perfectly for multiple users with different langcode.

Attached patch is a reroll to 8.9.x

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/user/user.install
@@ -103,3 +105,17 @@ function user_update_8100() {
 }
+
+/**
+ * Add an action to resend activation emails to multiple users.
+ */
+function user_update_8700() {
+  $action = Action::create([
+    'id' => 'user_resend_action',
+    'type' => 'user',
+    'label' => t('Resend welcome message the selected user(s)'),
+    'configuration' => [],
+    'plugin' => 'user_resend_action',
+  ]);
+  $action->trustData()->save();
+}

This can happen in a hook_post_update_NAME() since it's just creating new configuration. Also prevents update numbering conflicts that way.

sokru’s picture

Status: Needs work » Needs review
FileSize
34.52 KB

Moved action creation from hook_update_N() into hook_post_update_NAME

sokru’s picture

Fix for coding standard error.

sokru’s picture

sokru’s picture

133,134 and 135 had code from another issue, sorry. This time only relevant code is in patch.

Erik Frèrejean’s picture

Status: Needs review » Reviewed & tested by the community

With that, this can go back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 136: 5688-resend-welcome-email-136-D8.patch, failed testing. View results

sokru’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 136: 5688-resend-welcome-email-136-D8.patch, failed testing. View results

sokru’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs product manager review
  1. I'm going to ping this to Drupal's product managers to make a decision. To me this functionality seems quite useful and pragmatic.
  2. We should provide as much information on the issue summary as possible including screenshots of the UI change plus a release note for the release this makes it into.
  3. We need a change record to detail the UI change and new action so people know about it.
  4. +++ b/core/modules/user/src/Plugin/Action/ResendWelcomeMessage.php
    @@ -0,0 +1,112 @@
    +  /**
    +   * The language manager.
    +   *
    +   * @var \Drupal\Core\Language\LanguageManagerInterface
    +   */
    +  protected $languageManager;
    ...
    +    $this->languageManager = $language_manager;
    ...
    +      $container->get('language_manager'),
    ...
    +    $langcode = $this->languageManager->getCurrentLanguage()->getId();
    

    I don't see where the langcode is used so I'm not sure we need the language manager.

  5. +++ b/core/modules/user/src/Plugin/Action/ResendWelcomeMessage.php
    @@ -0,0 +1,112 @@
    +  public function execute($account = NULL) {
    +    if (empty($account)) {
    +      return;
    +    }
    

    I think we also need to return if the account does not have an email. Potentially we should deny access to this action if the user doesn't have an email. We should also have test coverage for this action with accounts that don't have an email address.

  6. The form needs test coverage for an account without an email
sokru’s picture

Status: Needs work » Needs review
FileSize
17.66 KB

Attached the patch solves 142.4 and parts of 142.5.
About testing accounts without emails, not sure if the core supports it yet. At least #286401: Make email not required for a Drupal site account is open and BrowserTestBase::drupalCreateUser forces to create an email.

alexpott’s picture

@sokru an admin (eg user 1) can create an user without an email address via the UI in core already.

sokru’s picture

Here's screenshot of the action and form element.
In the attached patch I created new UserResendTest class to resolve missing tests for accounts without email address (#142.6).

sokru’s picture

Issue summary: View changes
Erik Frèrejean’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/user/src/Plugin/Action/ResendWelcomeMessage.php
    @@ -0,0 +1,98 @@
    +      $config = \Drupal::getContainer()->get('config.factory')->get('user.settings');
    

    The config is already available in the `userSettings` property.

  2. +++ b/core/modules/user/src/ProfileForm.php
    @@ -57,4 +62,42 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
    +      $config = \Drupal::getContainer()->get('config.factory')->get('user.settings');
    

    The config is already available in the `userSettings` property.

sokru’s picture

Status: Needs work » Needs review
FileSize
19.16 KB
656 bytes

Attached patch fixes #147.1, but I disagree of #147.2, because $this->userSettings is null in this case.

Erik Frèrejean’s picture

but I disagree of #147.2, because $this->userSettings is null in this case

You are correct, the patch was messed up in my editor, it seemed to be part of the previous block. Although in that case I'd say that the constructor of that form should be extended to inject the config as using the \Drupal global is generally frowned upon. However I did notice that the \Drupal\user\AccountForm::form() also uses the global so not sure what "official stance" is for that. @alexpott?

sokru’s picture

This resolves also #147.2 and #149

JurgenR’s picture

Patch #150 uses the admin language to send mails, which is not the desired behaviour as mails should always be sent in the language for the user.
Omitting the language parameter in language_mail_notify causes proper language handling.

sokru’s picture

Patch from #150 failed to apply. Reroll and taking taking account comment from #151.
@JurgenR Thanks for the review! I agree with _user_mail_notify usage, please review attached interdiff if it has all changes that was meant to be attached. Also if you find this now correct edit this issue and set its status to "Reviewed & tested by the community".

sokru’s picture

FileSize
1.87 KB

And missing interdiff.

JurgenR’s picture

@sokru with pleasure! The only thing I'm not 100% sure of is the creation of the action. When removing and re-adding the patch we get en update error on the post update add_resend_action.

Which results in [error] 'action' entity with ID 'user_resend_action' already exists.

Is this the desired behavior or is a check needed to see if the action exists?

sokru’s picture

Only way that there would be Action with id: user_resend_action is via this patch. So I don't think there's need to check its existence. If this patch gets into core (e.g. 8.9), and your site would have applied this patch earlier, there would be no issues when updating core to 8.9, simply remove the patch from composer-files.

andypost’s picture

  1. +++ b/core/modules/user/src/Plugin/Action/ResendWelcomeMessage.php
    @@ -0,0 +1,97 @@
    +  public function __construct(array $configuration, $plugin_id, array $plugin_definition, ConfigFactoryInterface $config_factory) {
    +    parent::__construct($configuration, $plugin_id, $plugin_definition);
    +    $this->userSettings = $config_factory->get('user.settings');
    ...
    +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    +    return new static(
    ...
    +      $container->get('config.factory')
    ...
    +      switch ($this->userSettings->get('register')) {
    

    no reason to read and store config on plugin creation

  2. +++ b/core/modules/user/src/Plugin/Action/ResendWelcomeMessage.php
    @@ -0,0 +1,97 @@
    +    _user_mail_notify($op, $account);
    
    +++ b/core/modules/user/src/ProfileForm.php
    @@ -57,4 +62,41 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
    +    $mail = _user_mail_notify($op, $account);
    

    better to wrap in protected method to allow unit testing

  3. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -122,13 +122,13 @@ public function testUserAdmin() {
    -    // Test filtering on admin page for blocked users
    +    // Test filtering on admin page for blocked users.
    ...
    -    // Test unblocking of a user from /admin/people page and sending of activation mail
    +    // Test unblocking of a user from /admin/people page and sending of activation mail.
    
    @@ -142,7 +142,7 @@ public function testUserAdmin() {
    -    // Test blocking and unblocking another user from /user/[uid]/edit form and sending of activation mail
    +    // Test blocking and unblocking another user from /user/[uid]/edit form and sending of activation mail.
    

    out of scope, please revert

  4. +++ b/core/modules/user/user.post_update.php
    @@ -20,3 +21,17 @@ function user_post_update_enforce_order_of_permissions() {
    +    'label' => t('Resend welcome message the selected user(s)'),
    

    Not sure t() needed here

andypost’s picture

Version: 8.9.x-dev » 9.1.x-dev

The right branch for features now

Erik Frèrejean’s picture

First off all a re-role on 9.x.

Erik Frèrejean’s picture

Status: Needs review » Needs work
Erik Frèrejean’s picture

Status: Needs work » Needs review
FileSize
19.57 KB
3.86 KB

New patch including the review from #156, however I left out point 2. _user_mail_notify is always called straight up.

sokru’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Manually tested that t-function is not needed here:

+++ b/core/modules/user/user.post_update.php
@@ -20,3 +21,17 @@ function user_post_update_enforce_order_of_permissions() {
'label' => t('Resend welcome message the selected user(s)'),
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/user/src/AccountForm.php
    @@ -3,6 +3,7 @@
     namespace Drupal\user;
     
     use Drupal\Component\Datetime\TimeInterface;
    +use Drupal\Core\Config\ConfigFactoryInterface;
     use Drupal\Core\Entity\ContentEntityForm;
     use Drupal\Core\Entity\EntityConstraintViolationListInterface;
     use Drupal\Core\Entity\EntityRepositoryInterface;
    @@ -29,6 +30,13 @@ abstract class AccountForm extends ContentEntityForm implements TrustedCallbackI
    
    @@ -29,6 +30,13 @@ abstract class AccountForm extends ContentEntityForm implements TrustedCallbackI
        */
       protected $languageManager;
     
    +  /**
    +   * The user settings.
    +   *
    +   * @var \Drupal\Core\Config\ImmutableConfig
    +   */
    +  protected $userSettings;
    +
       /**
        * Constructs a new EntityForm object.
        *
    @@ -40,10 +48,13 @@ abstract class AccountForm extends ContentEntityForm implements TrustedCallbackI
    
    @@ -40,10 +48,13 @@ abstract class AccountForm extends ContentEntityForm implements TrustedCallbackI
        *   The entity type bundle service.
        * @param \Drupal\Component\Datetime\TimeInterface $time
        *   The time service.
    +   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    +   *   The configuration factory.
        */
    -  public function __construct(EntityRepositoryInterface $entity_repository, LanguageManagerInterface $language_manager, EntityTypeBundleInfoInterface $entity_type_bundle_info = NULL, TimeInterface $time = NULL) {
    +  public function __construct(EntityRepositoryInterface $entity_repository, LanguageManagerInterface $language_manager, EntityTypeBundleInfoInterface $entity_type_bundle_info = NULL, TimeInterface $time = NULL, ConfigFactoryInterface $config_factory) {
         parent::__construct($entity_repository, $entity_type_bundle_info, $time);
         $this->languageManager = $language_manager;
    +    $this->userSettings = $config_factory->get('user.settings');
       }
     
       /**
    @@ -54,7 +65,8 @@ public static function create(ContainerInterface $container) {
    
    @@ -54,7 +65,8 @@ public static function create(ContainerInterface $container) {
           $container->get('entity.repository'),
           $container->get('language_manager'),
           $container->get('entity_type.bundle.info'),
    -      $container->get('datetime.time')
    +      $container->get('datetime.time'),
    +      $container->get('config.factory')
         );
       }
    

    Let's not make this changes here. In order to do this we need to do a deprecation for calling this without a config.factory object. Also we should use the injected settings in \Drupal\user\AccountForm::form() but that's out-of-scope here.

    Let's do that same as that method in

    +++ b/core/modules/user/src/ProfileForm.php
    @@ -57,4 +62,41 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
    +      switch ($this->userSettings->get('register')) {
    

    Can to use \Drupal::config('user.settings') instead. And we can file a follow up to do inject correctly in both places.

  2. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -208,4 +208,188 @@ public function testNotificationEmailAddress() {
    +    $this->drupalPostForm('user/' . $test_user->id() . '/edit', ['status' => 0], t('Resend welcome message'));
    ...
    +    $this->drupalPostForm('user/' . $blocked_user->id() . '/edit', ['status' => 0], t('Resend awaiting approval message'));
    ...
    +    $this->drupalPostForm('admin/people', $edit, t('Apply to selected items'));
    ...
    +    $this->drupalPostForm('admin/people', $edit, t('Apply to selected items'));
    

    Let's not use t() here. There's no way our tests would work if Drupal was installed in another language.

  3. +++ b/core/modules/user/tests/src/Functional/UserResendTest.php
    @@ -0,0 +1,46 @@
    +    $this->assertNoRaw(t('Resend welcome message'), 'Resend button is not displayed.');
    ...
    +    $this->assertRaw(t('Resend welcome message'), 'Resend button is displayed.');
    

    Does this need to be assert*Raw? - also let's not use t() here.

kishor_kolekar’s picture

address Comment #162
please review the patch.

kishor_kolekar’s picture

Status: Needs work » Needs review
Erik Frèrejean’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
@@ -208,4 +208,188 @@ public function testNotificationEmailAddress() {
+    $this->drupalPostForm('user/' . $test_user->id() . '/edit', ['status' => 0], ('Resend welcome message'));
...
+    $this->drupalPostForm('user/' . $blocked_user->id() . '/edit', ['status' => 0], ('Resend awaiting approval message'));
...
+    $this->drupalPostForm('admin/people', $edit, ('Apply to selected items'));

+++ b/core/modules/user/tests/src/Functional/UserResendTest.php
@@ -0,0 +1,46 @@
+    $this->drupalPostForm('admin/people/create', $edit, ('Create new account'));

Unneeded parentheses.

kishor_kolekar’s picture

Status: Needs work » Needs review
FileSize
19.48 KB
2.08 KB

Thanks, @Erik Frèrejean
removed Unneeded parentheses, please review the new patch.

weseze’s picture

  /**
   * Provides a submit handler for the 'Re-send welcome message' button.
   */
  public function editResendSubmit(array $form, FormStateInterface $form_state) {
    $account = $this->entity;
    $langcode = $this->languageManager->getCurrentLanguage()->getId();

$langcode does not seem te be used, so should be removed?

sokru’s picture

@weseze good catch! This will remove $langcode

quietone’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

I did a visual check of the code and spotted a few things. I was going to do a more thorough review but the patch needs a reroll.

  1. +++ b/core/modules/user/src/Plugin/Action/ResendWelcomeMessage.php
    @@ -0,0 +1,97 @@
    +    if (!$account->isActive()) {
    
    +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -208,4 +208,188 @@ public function testNotificationEmailAddress() {
    +    // Resend a alternate welcome message.
    

    s/a alternate/an alternate/
    Pardon, but I am not familiar with the patch and just wonder what alternate means here. Is this resending the original welcome email or something else?

  2. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -208,4 +208,188 @@ public function testNotificationEmailAddress() {
    +   * Tests the resending of a welcome e-mail notification.
    

    I learned the other day that Drupal uses email not e-mail. See #3051548: Fix spelling of "email". There are more usages of e-mail in the patch that need to be fixed.

  3. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -208,4 +208,188 @@ public function testNotificationEmailAddress() {
    +   * Collection of alternatives registrations configurations.
    

    I think this should be 'Returns a collection of alternative registration configurations'.

  4. +++ b/core/modules/user/tests/src/Functional/UserResendTest.php
    @@ -0,0 +1,46 @@
    +    $this->assertNoRaw(t('Resend welcome message'), 'Resend button is not displayed.');
    ...
    +    $this->assertRaw(('Resend welcome message'), 'Resend button is displayed.');
    

    The t() can be removed in tests. See #3145418: [November 9, 2020] Remove uses of t() in assertText() calls

kishor_kolekar’s picture

Assigned: Unassigned » kishor_kolekar

working on it

kishor_kolekar’s picture

Assigned: kishor_kolekar » Unassigned
Status: Needs work » Needs review
FileSize
19.46 KB
5.7 KB

@quietone I have addressed comment #169.
please review the patch.

Rob C’s picture

No way to influence / extend this? So other modules would have no way to use this code? And this would also send the wrong email to users created with other modules (eg user_registrationpassword and others have a separate email and token and are stuck on their own - again - and would have to really bypass all of it and implement their own class). (my vote for more radical TODOs - love the work, but we got a bigger fish somewhere). I previously shared some old code for an example in #31 - that now shows how nothing changed in 8+ years. Still the same basic logic for creating users. While we need something like user types / states - so you can resend a welcome email based on the registration type (core/urp/lt/etc) without having to replace core code in contrib.

kamkejj’s picture

Re-rolled for 8.8.x. I didn't see an 8.8 patch and the 8.7 and 8.9 patches don't apply.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

sokru’s picture

Not giving up a hope to get this into core someday... Patch is just a re-roll for 9.2 based on #171.

Status: Needs review » Needs work

The last submitted patch, 175: 5688-resend-welcome-email-175.patch, failed testing. View results

sokru’s picture

Status: Needs work » Needs review
FileSize
19.47 KB
1.07 KB

This should fix the deprecation notices from 9.2.

quietone’s picture

Status: Needs review » Needs work
FileSize
1.42 KB
1.51 KB

Came to do a review starting at #171. That interdiff looks wrong, it is adding lots of lines of code when the comment says it is just addressing points in #169 which is about comments and two lines of code. Therefor I have made a new diff to see what actually changed. The changes look good. Looking at the patch #171 I see that #169.2 is still to do. There are more instances of e-mail to change.

I have also added a diff for the reroll in #175 and that one looks fine.

I wanted to do more of a review but spend my time making diffs :-(

To do: #169.2

anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
19.46 KB
1.66 KB

Addressd #169.2

quietone’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs screenshots

@anmolgoyal74, thanks.

Began a review with a review of the code looking at standards and deprecations etc. I did not review the logic or if the tests were doing what they should.

  1. +++ b/core/modules/user/src/AccountForm.php
    @@ -281,7 +281,7 @@ public function form(array $form, FormStateInterface $form_state) {
    +      '#title' => $this->t('Locale settings'),
    
    @@ -291,10 +291,10 @@ public function form(array $form, FormStateInterface $form_state) {
    +      '#title' => $this->t('Time zone'),
    ...
    +      '#description' => $this->t('Select the desired local time and time zone. Dates and times throughout this site will be displayed using this time zone.'),
    
    +++ b/core/modules/user/src/AccountForm.php
    @@ -356,7 +356,6 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
    -
    

    These changes are out of scope and need to be removed from this patch.

  2. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -211,4 +211,188 @@ public function testNotificationEmailAddress() {
    +    $this->drupalPostForm('user/' . $test_user->id() . '/edit', ['status' => 0], 'Resend welcome message');
    

    drupalPostForm is deprecated.

  3. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -211,4 +211,188 @@ public function testNotificationEmailAddress() {
    +    $this->drupalPostForm('user/' . $blocked_user->id() . '/edit', ['status' => 0], 'Resend awaiting approval message');
    

    Same here. There are other uses of drupalPostForm in this patch that need to be changed.

Then, I though I would try the patch. I applied that patch and navigated to /user/3/edit?destination=/admin/people where I do see the 'Resend welcome message' button. When I click there is no confirmation that anything happened and there is nothing in the logs. I had to check the mail server to see if the message was sent. Is that the expected behavior?
Reading the tests it appears that one can bulk resend the welcome message. When I bulk select two users I do not see any option to resend emails. Please explain what is expected.

The I looked closer at the IS. The IS states that a new button, "Re-send welcome message" will be available. The button I see states 'Resend welcome message'. Is the patch or the IS correct? Since this is making changes to a user form so there should be some screenshots in the IS. The IS lists that there are screens shots in #44, #73 and #145. Why 3 sets? It is much better to have the laesst screen shots in the IS. That got me to look at the patch again and throughout there is 're-send' and 'resend'. Please decide on which is correct and be consist in the code/docs/UI.

Setting to NW and adding tags for an IS update and screenshots. Having up to date screen shots is really helpful.

sokru’s picture

Added screenshots and updated the issue summary.
The patch should resolve issues mentioned on #180. If one adds the patch to existing installation it needs run update.php, not sure if this should be on issue summary.

Nishat Ahmad made their first commit to this issue’s fork.

Erik Frèrejean’s picture

The patch in #181 removed the lines noted in #180 rather then not touching them at all.

This patch doesn't need to touch the AccountForm.

sokru’s picture

Status: Needs review » Needs work

core/modules/user/src/Plugin/Action/ResendWelcomeMessage.php needs small change on docblock to pass spell-checking.

- * Resends welcome message to a user.
+ * Resend welcome message to a user.
ayushmishra206’s picture

Status: Needs work » Needs review
FileSize
522 bytes
17.65 KB

Made the change suggested in #184. Please review.

sokru’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

jasonawant’s picture

I know this could very well be outside the scope of this issue; so let me know and I can create a separate support/bug/feature ticket.

Has anyone come across where user.settings notify.register_no_approval_required could be set via the UI? I could not trace any changes back in git history or find any relevant change records.

It appears to be set to TRUE via user module's user.settings.yml or by one of the core install profile's user.settings.yml on install. But, outside of that, I do not see where it could be set/changed in the UI.

The AccountSettingsForm::submitForm() does not set it.

As a result, if user.settings notify.register_no_approval_required becomes set to false, these emails do not send.

quietone’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

@jasonawant, your question sounds like a support request to me. So, yes open a new issue or ask in Slack. Thx.

If one adds the patch to existing installation it needs run update.php, not sure if this should be on issue summary.

Yes, the reviewer needs to know this and should not be expected to read the issue or the patch to figure this out. I have added this to the IS.

I applied the patch, ran update.php and then tested the patch. I added a new user and then went user/edit and scrolled to the bottom to find the 'resend' button. I clicked and then looked for the mail in MailHog and it was sent. I then went to admin/people and made two batch requests, one for resending the welcome message to all users and then to just one user. In both cases the emails were sent as expected. I did not test the case when sending the email failed.

The problem I found is that the text for the success messages needs work as does the text in the drop down. In general, the are quite a few situation where the text should be 'the welcome message' not 'welcome message'. I think I caught the ones that I think need to be changed in the following.

  1. +++ b/core/modules/user/config/install/system.action.user_resend_action.yml
    @@ -0,0 +1,10 @@
    +label: 'Resend welcome message the selected user(s)'
    

    Resend the welcome message to the selected user(s)

  2. +++ b/core/modules/user/config/schema/user.schema.yml
    @@ -150,6 +150,10 @@ action.configuration.user_remove_role_action:
    +  label: 'Resend welcome message to selected users configuration'
    

    s/welcome/the welcome/

  3. +++ b/core/modules/user/user.post_update.php
    @@ -13,3 +15,17 @@ function user_removed_post_updates() {
    +    'label' => 'Resend welcome message the selected user(s)',
    

    Resend the welcome message to the selected user(s)

  4. +++ b/core/modules/user/src/ProfileForm.php
    @@ -57,4 +62,40 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
    +      $this->getLogger('user')->notice('Welcome message has been resend to %name at %email.', ['%name' => $account->getAccountName(), '%email' => $account->getEmail()]);
    +      $this->messenger()->addMessage($this->t('Welcome message has been resend to %name at %email', ['%name' => $account->getAccountName(), '%email' => $account->getEmail()]));
    

    s/resend/resent/

  5. +++ b/core/modules/user/src/ProfileForm.php
    @@ -57,4 +62,40 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
    +      $this->getLogger('user')->notice('There was an error resending welcome message to %name at %email', ['%name' => $account->getAccountName(), '%email' => $account->getEmail()]);
    +      $this->messenger()->addMessage($this->t('There was an error resending welcome message to %name at %email', ['%name' => $account->getAccountName(), '%email' => $account->getEmail()]), 'error');
    

    s/welcome/the welcome/

  6. +++ b/core/modules/user/src/ProfileForm.php
    @@ -57,4 +62,40 @@ public function editCancelSubmit($form, FormStateInterface $form_state) {
    +  /**
    +   * Provides a submit handler for the 'Re-send welcome message' button.
    

    This should match the button text, 'Resend welcome message'

I don't see in the issue that the tests have been reviewed. That is still to do.

quietone’s picture

  1. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -211,4 +211,192 @@ public function testNotificationEmailAddress() {
    +    $config = $this->config('user.settings');
    

    The config parameter is not used, this can be simplified. This change is needed in other methods as well.

    $this->config('user.settings')
          ->set('register', $register_mode)
          ->save();
    
  2. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -211,4 +211,192 @@ public function testNotificationEmailAddress() {
    +    // Resend an alternate welcome message.
    

    What does 'alternate' mean here? I understood that this was simply resending the 'standard' message?

  3. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -211,4 +211,192 @@ public function testNotificationEmailAddress() {
    +    // Assert only the user receive a mail.
    

    We can only say that the email was sent, not that the user received it. So, lets change this to 'Assert that only one mail was sent to test_user.

  4. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -211,4 +211,192 @@ public function testNotificationEmailAddress() {
    +    // Assert the blocked user & admin receive a mail.
    

    Change to assert that email was sent, "Assert that an email was sent to a blocked user and admin."

  5. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -211,4 +211,192 @@ public function testNotificationEmailAddress() {
    +    // Resend welcome mail to user_c & user_b.
    

    Let's be alphabetical. 'Resend welcome message to user_b and user_c.

  6. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -211,4 +211,192 @@ public function testNotificationEmailAddress() {
    +    // Assert only 2 accounts have been notified.
    

    Assert that welcome emails were sent user_b and user_c.

  7. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -211,4 +211,192 @@ public function testNotificationEmailAddress() {
    +    // Resend waiting approval mail to user_c & user_b.
    

    As above

  8. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -211,4 +211,192 @@ public function testNotificationEmailAddress() {
    +    // Assert only 2 accounts & admin have been notified.
    

    As mentioned above

quietone’s picture

Found another one.

+++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
@@ -211,4 +211,192 @@ public function testNotificationEmailAddress() {
+    // Awaiting approval mail resend to user C.

I would change this to a larger comment at the top and remove all the single line comments. And I would do user_b first.
Suggested longer comment for this section.
// Assert that awaiting approval and notification emails have been sent to user_b and user_c.

I tests seem to be what they are supposed to but this is not my area.

jasonawant’s picture

adityasingh’s picture

Status: Needs work » Needs review
FileSize
17.69 KB
7.09 KB

Addressed #188, #189 and #190.

sokru’s picture

Status: Needs review » Needs work

Thanks @adityasingh, I think the idea in #189.1 was to get rid of unnecessary $config variables.
Required changes would look like this:

--- b/core/modules/user/tests/src/Functional/UserAdminTest.php
+++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
@@ -34,7 +34,6 @@
    * Registers a user and deletes it.
    */
   public function testUserAdmin() {
-    $config = $this->config('user.settings');
     $user_a = $this->drupalCreateUser();
     $user_a->name = 'User A';
     $user_a->mail = $this->randomMachineName() . '@example.com';
@@ -108,7 +107,7 @@
     $edit = [];
     $edit['action'] = 'user_block_user_action';
     $edit['user_bulk_form[4]'] = TRUE;
-    $config
+    $this->config('user.settings')
       ->set('notify.status_blocked', TRUE)
       ->save();
     $this->drupalPostForm('admin/people', $edit, 'Apply to selected items', [
abhisekmazumdar’s picture

Status: Needs work » Needs review
FileSize
18.41 KB
973 bytes

Updated the patch as per the suggestion on #193

sokru’s picture

Status: Needs review » Needs work

@abhisekmazumdar the change is needed for all instances on patch, not just the one mentioned.

abhisekmazumdar’s picture

Status: Needs work » Needs review
FileSize
18.71 KB
1.61 KB

Oh!! Apologies for not understanding it. Right now still I'm not sure if to remove the other $config from testResendWelcomeEmailNotification() and testResendAwaitingApprovalEmailNotification() as I can see they are not been used.

I have removed and updated all the $config = $this->config('user.settings'); in the patch. Kindly review.

Status: Needs review » Needs work

The last submitted patch, 196: 5688-196.patch, failed testing. View results

sokru’s picture

@abhisekmazumdar the tests are failing because you removed saving of user.settings config:

-    $config = $this->config('user.settings')
-      ->set('register', $register_mode)
-      ->save();

when a correct change would have been

-    $config = $this->config('user.settings')
+    $this->config('user.settings')
abhisekmazumdar’s picture

Status: Needs work » Needs review
FileSize
18.89 KB
1.09 KB
sokru’s picture

Status: Needs review » Reviewed & tested by the community

Setting back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Good to see progress on this issue that is more or less old enough to drive (!).

  1. 
    @@ -0,0 +1,10 @@
    +langcode: en
    +status: true
    +dependencies:
    +  module:
    +    - user
    +id: user_resend_action
    +label: 'Resend the welcome message to the selected user(s)'
    +type: user
    +plugin: user_resend_action
    +configuration: {  }
    

    'resend' out of context is not very self-documenting. Could this be 'user_welcome_message_action'?

  2. +++ b/core/modules/user/src/Plugin/Action/ResendWelcomeMessage.php
    @@ -0,0 +1,97 @@
    +
    +/**
    + * Resend welcome message to a user.
    + *
    + * @Action(
    + *   id = "user_resend_action",
    + *   label = @Translation("Resend welcome message to selected users"),
    + *   type = "user"
    + * )
    + */
    +class ResendWelcomeMessage extends ActionBase implements ContainerFactoryPluginInterface {
    +
    +  /**
    

    Similar here - is the 'Resend' necessary? Could it be 'Send welcome message'? There are situations where users get created without the welcome message being sent in the first place, so sending via this action could be the first time that happens- then it wouldn't be resending but just sending.

    Throughout the patch I think we should probably change 'Resend' to just 'Send' - only historical context determines whether we're resending a message, but we're always sending one.

  3. +++ b/core/modules/user/src/Plugin/Action/ResendWelcomeMessage.php
    @@ -0,0 +1,97 @@
    +    }
    +
    +    if (!$account->isActive()) {
    +      $op = 'register_pending_approval';
    +    }
    +    else {
    +      // Determine the user approval method.
    +      switch ($this->configFactory->get('user.settings')->get('register')) {
    +        case UserInterface::REGISTER_ADMINISTRATORS_ONLY:
    +          $op = 'register_admin_created';
    +          break;
    +
    +        case UserInterface::REGISTER_VISITORS:
    +        default:
    +          $op = 'register_no_approval_required';
    +      }
    +    }
    +
    +    _user_mail_notify($op, $account);
    +  }
    +
    

    It looks like this could lead to a situation where $op is undefined in practice - can we add a default?

MeenakshiG’s picture

Changed "resend" to "send".

MeenakshiG’s picture

Status: Needs work » Needs review
catch’s picture

Status: Needs review » Needs work
Issue tags: +Needs usability review

That changes some UI text, but I think we should consider making this change across the entire patch too - class names, action names etc. to remove the 'resend' terminology almost entirely. It might need some more discussion though.

Erik Frèrejean’s picture

I'd agree with @catch here for consistency sake the naming I'd say change the terminology across the patch.

For example

+++ b/core/modules/user/config/install/system.action.user_resend_action.yml
@@ -0,0 +1,10 @@
+id: user_resend_action
+label: 'Send the welcome message to the selected user(s)'

it looks strange that the id contains resend but the label just states "send".

Rob C’s picture

This issue can almost drink beer legally - but seriously: this will break multiple contrib modules and nobody responded to this. People are now filling up issues queues of submodules warning about this issues begin RTBC - like it's their problem to fix. #fun

sokru’s picture

Status: Needs work » Needs review
FileSize
20.05 KB
18.82 KB

Renamed the all "Resend" to "Send". Also takes care of #201.3.

@Rob C, I was not able to reproduce the issue mentioned on #3196665: Not compatible with Drupal core "Resend welcome message from admin user/edit" feature request at user_registrationpassword module issue queue. Is there other module's that have reported incompatibility with this issue? I kind of think that modules that override core's functionality are responsible to adapt to core changes. However I agree with you, that core should offer different user types / states, but I think it should be handled on a separate issue.

quietone’s picture

Status: Needs review » Needs work

There are test failures - setting to NW.

vsujeetkumar’s picture

Status: Needs work » Needs review
FileSize
18.85 KB
714 bytes

Fixing failing test, Please have a look.

vikashsoni’s picture

Applied #185 working fine now able to resend welcome message sharing screenshot

#209 #207 both patches are same and showing only send welcome message not resend welcome message

sokru’s picture

Found one "resent" text from patch. Updated also the issue summary about wording on latest patches.

chetanbharambe’s picture

Assigned: Unassigned » chetanbharambe
Status: Needs review » Needs work
FileSize
1.17 MB
700.61 KB

Verified and tested patch #211.
Patch Not applied successfully and gives error
Please see attached screenshots.

Can be a move to Needs Work.

chetanbharambe’s picture

Assigned: chetanbharambe » Unassigned
sokru’s picture

Status: Needs work » Needs review

@chetanbharambe thank you for reviewing. On the first image "Patch 5688 Error.png" I guess you have applied the patch already. On second image "Patch 5688 Error1.png" it verifies you're able to apply the patch. So setting back to NR.

chetanbharambe’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

I tried to apply the patch and have the following message on the terminal.

git apply -v 5688-211.patch
Checking patch core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php...
Checking patch core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php...
Hunk #1 succeeded at 95 (offset 2 lines).
Checking patch core/modules/user/config/install/system.action.user_welcome_message_action.yml...
Checking patch core/modules/user/config/schema/user.schema.yml...
Checking patch core/modules/user/src/Plugin/Action/SendWelcomeMessage.php...
Checking patch core/modules/user/src/ProfileForm.php...
Checking patch core/modules/user/tests/src/Functional/UserAdminTest.php...
Checking patch core/modules/user/tests/src/Functional/UserSendTest.php...
Checking patch core/modules/user/user.post_update.php...
Applied patch core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php cleanly.
Applied patch core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php cleanly.
Applied patch core/modules/user/config/install/system.action.user_welcome_message_action.yml cleanly.
Applied patch core/modules/user/config/schema/user.schema.yml cleanly.
Applied patch core/modules/user/src/Plugin/Action/SendWelcomeMessage.php cleanly.
Applied patch core/modules/user/src/ProfileForm.php cleanly.
Applied patch core/modules/user/tests/src/Functional/UserAdminTest.php cleanly.
Applied patch core/modules/user/tests/src/Functional/UserSendTest.php cleanly.
Applied patch core/modules/user/user.post_update.php cleanly.

The message you see Hunk #1 succeeded at 95 (offset 2 lines) means the files were successfully patched, but the first section that was patched was 2 lines after than was originally specified.
I think This patch needs re-roll.

Suresh Prabhu Parkala’s picture

Status: Needs work » Needs review
FileSize
18.86 KB

Re-rolled patch is here. Please review.

chetanbharambe’s picture

Status: Needs review » Needs work

I tried to apply the patch and have the following message on the terminal.

git apply -v 5688-216.patch
Checking patch core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php...
Checking patch core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php...
Hunk #1 succeeded at 93 (offset -2 lines).
Checking patch core/modules/user/config/install/system.action.user_welcome_message_action.yml...
error: core/modules/user/config/install/system.action.user_welcome_message_action.yml: already exists in working directory
Checking patch core/modules/user/config/schema/user.schema.yml...
Checking patch core/modules/user/src/Plugin/Action/SendWelcomeMessage.php...
error: core/modules/user/src/Plugin/Action/SendWelcomeMessage.php: already exists in working directory
Checking patch core/modules/user/src/ProfileForm.php...
Checking patch core/modules/user/tests/src/Functional/UserAdminTest.php...
Checking patch core/modules/user/tests/src/Functional/UserSendTest.php...
error: core/modules/user/tests/src/Functional/UserSendTest.php: already exists in working directory
Checking patch core/modules/user/user.post_update.php...

I think This patch needs re-roll again.

quietone’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.88 KB
19.02 KB

The patch in #216 applies cleanly. If you look at the results from the test bot click on 'PHP 7.3 & MySQL 5.7 Custom Commands Failed' in #215 you will see that it failed the coding standard check. This check can be run locally, before a patch is uploaded. Have a look at the instructions for running the coding standard checks locally so you can be sure the tests will run before uploading a patch. It also has the advantage of saving resources, including money, for the Drupal association. There are real costs for running the tests.

I have made a new patch, fixing the coding standards errors and reformatted some long lines core/modules/user/src/ProfileForm.php for readability.

quietone’s picture

Made new screenshots and added them to the IS.

The next step here is the Usability review, which was requested in #204. I will ask in #us for a review.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

chetanbharambe’s picture

VladimirAus’s picture

FileSize
19.02 KB

Upgraded to work with latest version of 9.3.

sokru’s picture

Version: 9.3.x-dev » 9.4.x-dev
FileSize
19.06 KB
64.01 KB
82.29 KB

Reroll for 9.4.x, based on #218. Added reference screenshots from Pantheon and Browserstack, in case usability review would need some.

Anybody’s picture

Still needs usability review, while the patch worked great for us (RTBC+1).

Just a note for others who only require the patch intermediately: Do not forget to also remove the created action from config if you remove the patch again! Otherwise, you'll run into an error like this: #3266996: Drupal\Component\Plugin\Exception\PluginNotFoundException: The "user_welcome_message_action" plugin does not exist.
This is of course expected behavior and not a mistake of this patch.
(While it might make sense to fall back more graceful than with a WSOD if an (action) plugin is not found, but that's a different topic. I didn't find an issue for that yet - just #3095427: Handle missing action plugin during migration gracefully)

But back to topic, as written in the beginning I just wanted to leave the note for others as stupid as me ;)

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

weseze’s picture

Rerolled patch for 9.4.1.

Only change was in the the migrations tests for D6/D7. The number of actions changed + also the line number where the actions were defined.

weseze’s picture

Fixed a the missing $account object to use $this->entity instead.
Also fixed new line numbers for 9.4.

Status: Needs review » Needs work

The last submitted patch, 227: 5688-227.patch, failed testing. View results

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

JurgenR’s picture

Updated patch from #228.
When register settings is not 'visitors_admin_approval', the 'Send awaiting approval message' should not be available on the user edit form.
This leads to confusion when an admin presses the button on blocked users when only administrators are allowed to create profiles.

pradhumanjain2311’s picture

Try to fix CS Erros.

Status: Needs review » Needs work

The last submitted patch, 231: 5688-231.patch, failed testing. View results

Christopher Riley’s picture

I tried to use this patch on 9.5 Beta 2, however I get the following which I try doing a drush updb.

-> drush updb
-------- ----------------- ------------- ----------------------------------
Module Update ID Type Description
-------- ----------------- ------------- ----------------------------------
user add_send_action post-update Add an action to send activation
emails to multiple users.
-------- ----------------- ------------- ----------------------------------

Do you wish to run the specified pending updates? (yes/no) [yes]:
> yes

> [notice] Update started: user_post_update_add_send_action
> [error] The "user_welcome_message_action" plugin does not exist. Valid plugin IDs for Drupal\Core\Action\ActionManager are: comment_unpublish_by_keyword_action, feeds_feed_delete_action, flag_delete_flagging, flag_action:bookmark_flag, flag_action:bookmark_unflag, flag_action:following_flag, flag_action:following_unflag, flag_action:friend_flag, flag_action:friend_unflag, node_promote_action, node_unpublish_by_keyword_action, node_make_sticky_action, node_unpromote_action, node_assign_owner_action, node_make_unsticky_action, simplenews_send_action, simplenews_stop_action, user_remove_role_action, user_cancel_user_action, user_block_user_action, user_add_role_action, user_unblock_user_action, views_bulk_operations_delete_entity, vbo_cancel_user_action, webform_submission_make_unsticky_action, webform_submission_make_sticky_action, webform_archive_action, webform_submission_make_unlock_action, webform_close_action, webform_delete_action, webform_open_action, webform_submission_make_lock_action, webform_unarchive_action, webform_submission_delete_action, pathauto_update_alias, action_message_action, entity:unpublish_action:block_content, entity:unpublish_action:comment, entity:unpublish_action:media, entity:unpublish_action:menu_link_content, entity:unpublish_action:node, entity:unpublish_action:path_alias, entity:unpublish_action:taxonomy_term, entity:unpublish_action:paragraph, action_goto_action, action_send_email_action, entity:save_action:block_content, entity:save_action:comment, entity:save_action:feeds_feed, entity:save_action:file, entity:save_action:ingredient, entity:save_action:invite, entity:save_action:media, entity:save_action:menu_link_content, entity:save_action:node, entity:save_action:section_library_template, entity:save_action:taxonomy_term, entity:save_action:user, entity:publish_action:block_content, entity:publish_action:comment, entity:publish_action:media, entity:publish_action:menu_link_content, entity:publish_action:node, entity:publish_action:path_alias, entity:publish_action:taxonomy_term, entity:publish_action:paragraph, entity:delete_action:comment, entity:delete_action:media, entity:delete_action:node, entity:delete_action:webform
> [error] Update failed: user_post_update_add_send_action
[error] Update aborted by: user_post_update_add_send_action
[error] Finished performing updates.

Bhanu951’s picture

Assigned: Unassigned » Bhanu951
Issue tags: +Needs reroll

Bhanu951’s picture

Assigned: Bhanu951 » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
Gábor Hojtsy’s picture

@smustgrave pinged me about this.

I agree with @alexpott that its a useful feature.

Looking at the suggested feature itself, are we using the "Welcome message" terminology for their registration email? (Is this clear its about an email in the first place, not a Drupal message showing on the website?). I believe when a user registers they get that feedback text (https://git.drupalcode.org/search?search=welcome%20message&nav_source=na...), but not sure admins know or recognize this terminology? Maybe "Resend welcome email" would be clearer?

Otherwise the feature looks great.

smustgrave’s picture

Status: Needs review » Needs work

Thanks @Gábor Hojtsy!

Am moving to NW as there seem to be failures in the MR.

rkoller’s picture

I've queued the issue for review at #3342717: Drupal Usability Meeting 2023-02-24 or a future meeting. And for sure if anyone familiar with the issue would have the time to present would be more than welcome and helpful. Meeting takes place every Friday at 14:00 UTC (currently 7:00am PT, 10:00am ET). See time.is to see what that is in your timezone.

rkoller’s picture

Issue tags: -Needs usability review

Usability review

We've discussed this issue at #3342717: Drupal Usability Meeting 2023-02-24. The recording of the meeting can be found at https://youtu.be/ylo5SvxbbmI

For the record, the attendees on friday's usability meeting were @AaronMcHale, @benjifisher, @rkoller, and @simohell.

In the following a summary of the points we came up during the meeting:

  1. It might be a reasonable step to add an option to disable the send message functionality with a checkbox on for example /admin/config/people/accounts - per default the option should be enabled.
  2. If you visit the user edit page of another user (for example /user/2/edit) there are several aspects to note about the three buttons at the bottom of the page: Save, Send welcome message and Cancel account:
    • Function-wise Send welcome message is fundamentally different from Save and Cancel account. We should try to follow the principle that one form shouldn't do too many things at once and the user edit form is already doing many things at once. By adding a third button at the bottom we are further adding to the "chaos".
    • Visually the primary button Save is highlighted in blue while the action link Cancel account is highlighted in red. The button in the middle is just grey and doesn't provide any other visual cue. The button has to be noticed and then has to be read - there aren't any visual cues aside that and the cognitive load is challenging.
    • Behind the scene there is also logic in place deciding which of the three welcome messages defined on /admin/config/people/accounts is sent out to the user. The user has only a single Send welcome message button and is potentially unaware, which of the messages is sent out to the user, and or even unaware that there is a differentiation taking place at all. This applies to the button on the user edit page as well as the bulk actions in point three.
    • In the context of the user edit page the following potential edge case has also to be kept in mind. In case the user wants to change the password and at the same time send out the welcome message, he or she might be unaware that the password is only changed after the form is saved. So in case the user is changing the password, then clicking the Send welcome message before hitting the save button, the one time login url would be referring to the old password and not to the newly entered one.

    All those potential stepping stones lead to the following recommendation following a pattern that was applied for example in #987978: Move "administrator role" setting to new Role Settings form:

    • Add a new tab to the user profile page - that way a single page would be focused and dedicated to a single task.
    • By moving the send functionality from a sole button to a dedicated tab it would be possible to provide some more descriptive help text on top to the users.
    • Underneath the help text add vertical tabs, the pattern used on the /admin/config/people/accounts page in the emails section.
    • Within each tab have the none editable preview of the to be send message as well as a send button.

    Out of the scope for this issue, but the group wondered why there is only the option for sending a welcome message? If you take a look at the list of available email templates on the account settings page, then there would be at least a reasonable need for for example the ability to send a password recovery email. By the addition of such a "send message" tab it would be easily possible to extend the list of available options there. It would make sense to open up a follow-up issue for that.

  3. There was a consensus in the group to provide the level of control for bulk actions as well. Problem is that it is probably not possible to group options in the bulk action select list and adding all available options ungrouped wouldn't an option either due to bad readability. Therefore recommendation is to use a config page similar to the one if the option Cancel selected user account(s) is selected:
    • On top the list of accounts the email is sent to.
    • Then a list of radio buttons where the user is able to select the type of email to be send.
    • For the selected option a preview of the message being send should be displayed
    • At the bottom a send and cancel button.

In the end one general question that applies to user profile pages as well as the block actions. What is the reasoning behind that the Send welcome message button is visible on user profiles of users that never logged into their account, on profiles where users have already logged into their account and also on the user profile of the account i am currently logged in? Indirectly that applies to bulk actions as well. There is also no distinction made between accounts that never got logged into and others that already got logged into. Is there a purpose and need that already logged in accounts could get a welcome message resend? Is it due to the fact that some administrators have reutilized the welcome message for other purposes?

The issue status is already set to needs work so I only remove the needs usability review tag.

benjifisher’s picture

I attended the usability meeting where we reviewed this issue. (See Comment #240.)

It is outside the scope of usability, but my personal opinion is that this issue should be closed as "won't fix". It can be implemented in a contributed module, and I think that most sites do not need this feature.

Whether it is done in Drupal core or in a separate module, I agree with the usability recommendations in #240. Most important:

  1. Create a separate page (a tab on the user edit page, also known as a local task) instead of adding a submit button to the form.
  2. Let the site administrator choose which e-mail to send, instead of making the decision in code.

The second point applies to both the form on the user-edit page and to the bulk operation. Create a configurable bulk operation, so that the site administrator can select user accounts from the list, then select the "bulk email" action, and then, on the confirmation form, select which e-mail to send.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

svendecabooter’s picture

As per the suggestion by benjifisher I have created a contrib project to provide the functionality that is added in this issue:
https://www.drupal.org/project/resend_register_mail

The 2 mentioned usability improvements still need to be incorporated into this contrib project. Patches welcome there.