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.
Comment | File | Size | Author |
---|---|---|---|
#231 | interdiff_230-231.txt | 1.03 KB | pradhumanjain2311 |
#231 | 5688-231.patch | 14.43 KB | pradhumanjain2311 |
#230 | 5688-230.patch | 19.59 KB | JurgenR |
#227 | 5688-227.patch | 19.01 KB | weseze |
#226 | 5688-226.patch | 19.06 KB | weseze |
Issue fork drupal-5688
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:
Comments
Comment #1
coreb CreditAttribution: coreb commentedMoving from x.y.z queue to 6.x-dev.
Comment #2
PasqualleComment #3
Anonymous (not verified) CreditAttribution: Anonymous commented+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 !
Comment #4
jjma CreditAttribution: jjma commentedI would like this functionality as well.
Jon
Comment #5
karschsp CreditAttribution: karschsp commentedHere's a patch that adds a "Re-send welcome e-mail" checkbox to the user/edit form.
Comment #7
babbage CreditAttribution: babbage commentedSubscribing—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. :)
Comment #8
jeffschulerThe 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..."
Comment #9
milerosu CreditAttribution: milerosu commentedHi, thanks for the patch, it worked for me with a small fix:
$array['resend'] == 1
instead of$edit['resend'] == 1
Comment #10
babbage CreditAttribution: babbage commentedI 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.
Comment #11
hornetnz CreditAttribution: hornetnz commentedsubscribing
Comment #12
izmeez CreditAttribution: izmeez commentedsubscribing
Comment #13
jfmoore CreditAttribution: jfmoore commented#5: 5688.001.patch queued for re-testing.
Comment #14
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedThe change in #9 to the patch worked for me.
I am surprised this was not already in there.
Comment #15
fjen CreditAttribution: fjen commented+1
Comment #16
lubnax CreditAttribution: lubnax commentedsubscribe
Comment #17
servantleader CreditAttribution: servantleader commented+1
Comment #19
axaubet CreditAttribution: axaubet commented#5: 5688.001.patch queued for re-testing.
Comment #21
seanmcginley CreditAttribution: seanmcginley commentedPatch adds the checkbox to resend welcome email.... but does not send the email.
Comment #22
John Bickar CreditAttribution: John Bickar commentedAs a workaround, you can deactivate and re-activate the user(s) account(s). Reactivating it will re-send the activation welcome message.
Comment #23
izmeez CreditAttribution: izmeez commentedIt 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.
Comment #24
GeminiAgaloos CreditAttribution: GeminiAgaloos commented+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.
Comment #25
karschsp CreditAttribution: karschsp commentedTwo and a half years later, here's a patch that adds a "Re-send welcome message" button to the user edit form.
Comment #26
meba CreditAttribution: meba commentedIn your patch "$account->uid > 1 &&" why can't a user 1 send a welcome message?
Comment #27
bass28 CreditAttribution: bass28 commentedHere a version of this patch for the Drupal 7.x branch.
Comment #28
BrockBoland CreditAttribution: BrockBoland commentedAppreciate 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.
Comment #29
ratinakage CreditAttribution: ratinakage commentedI'm going to port this patch for Drupal 8.x
Comment #30
AnybodyIs 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.
Comment #31
Rob C CreditAttribution: Rob C commentedI 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.
Comment #32
YesCT CreditAttribution: YesCT commentedany update?
Comment #33
marji CreditAttribution: marji commentedforward 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
Comment #34
adammaloneIn 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.
Comment #35
YesCT CreditAttribution: YesCT commentedFrom 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.
Comment #36
jlscott CreditAttribution: jlscott commentedRerolled the patch from #33 to suppress the "resend" button if the account does not have an email address defined.
Comment #38
jlscott CreditAttribution: jlscott commentedInterdiff 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?
Comment #39
jrsinclair CreditAttribution: jrsinclair commentedAssigning to myself for Drupal Sprint weekend D8 work.
Comment #40
jrsinclair CreditAttribution: jrsinclair commentedPatch from #38 appears to be generating an error message like the following:
Will do some investigation.
...
And it appears that I applied the patch wrong. Mia culpa.
Comment #41
jrsinclair CreditAttribution: jrsinclair commented#36: resend-welcome-message-5688-36.patch queued for re-testing.
Comment #42
jrsinclair CreditAttribution: jrsinclair commentedTest passed after re-submitting. In my review, it worked as advertised. Marking as RTBC.
Comment #43
YesCT CreditAttribution: YesCT commenteda before and after screenshot would be nice.
Comment #44
adammaloneBefore
and After
Comment #45
webchickThis 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...
My goodness, that's a long condition. :) Probably worth a comment, or even better to break it up somehow.
(nitpick) That first line needs to be /** rather than /*
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.
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.
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.
Comment #46
msypolt CreditAttribution: msypolt commentedI am working on this as part of Portland2013 Code sprint.
Comment #47
msypolt CreditAttribution: msypolt commentedThe attached patch improves commenting for the code. Still needs and automated test written for it, which someone else should do.
Comment #48
heddnComment #50
Andre-Bgoing to reroll this in the next days
Comment #51
becw CreditAttribution: becw commentedHere's a rerolled patch for the testbot.
Comment #52
becw CreditAttribution: becw commentedThere are a couple more issues with this patch. I'm going to steal this issue and work on it now.
Comment #54
becw CreditAttribution: becw commentedI 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()
andgetUsername()
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 ofuser_access()
. Also,variable_get()
might be the wrong way to retrieve module configuration variables now.Comment #55
becw CreditAttribution: becw commentedHere's a patch without
variable_get()
. Almost there.Comment #56
becw CreditAttribution: becw commentedIn 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.
Comment #58
kbentham CreditAttribution: kbentham commentedHere 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.
Comment #59
becw CreditAttribution: becw commentedThat test was supposed to fail! Something is wrong here.
Comment #60
becw CreditAttribution: becw commentedOk, 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.
Comment #61
PasqualleComment #63
mahaprasad CreditAttribution: mahaprasad commentedRerolled the patch for #60
Comment #65
mahaprasad CreditAttribution: mahaprasad commentedFixed for simpletest.
Comment #67
joestewart CreditAttribution: joestewart commentedWorking on a reroll at DC Atlanta 2013 code sprint.
Comment #68
joestewart CreditAttribution: joestewart commentedrerolled.
Mentored by kbasarab.
Comment #69
Stefan Lehmann CreditAttribution: Stefan Lehmann commented#68: resend-welcome-message-5688-68.patch queued for re-testing.
Comment #70
Stefan Lehmann CreditAttribution: Stefan Lehmann commentedI 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.
Comment #71
joestewart CreditAttribution: joestewart commented@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.
Comment #72
rakeshfalke CreditAttribution: rakeshfalke commentedPatch resend-welcome-message-5688-68.patch By joestewart working properly.
Find attached screenshot.
Thanks!
Comment #73
rakeshfalke CreditAttribution: rakeshfalke commentedComment #74
kattekrab CreditAttribution: kattekrab commentedComment #75
thedavidmeister CreditAttribution: thedavidmeister commentedComment #76
thedavidmeister CreditAttribution: thedavidmeister commentedComment #77
thedavidmeister CreditAttribution: thedavidmeister commented+ $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.
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
Comment #78
jhedstromComment #83
andreasderijckeUpdated the patch taking into account the feedback from #77.
Only, for
there seems to be no solution in core.
Comment #84
andreasderijckeFix test failing.
Comment #86
Erik FrèrejeanRemoved the
$language
global from::editResendSubmit
and use the language manager, and a couple of small coding style changes.Comment #87
jeffschulerComment #88
Erik FrèrejeanRemoved 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.
Comment #90
Erik FrèrejeanFixes the tests.
Comment #92
jeffschulerThe patch in #90 is working well for me, both in sending individual emails and in bulk from the /admin/people view.
Comment #93
weseze CreditAttribution: weseze as a volunteer commentedConfirmed it is working on 8.6.1: both individual mails as bulk mail.
Also works fine with the contrib module views_bulk_operations.
Comment #94
wengerkThanks 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.
Comment #95
Erik FrèrejeanI 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.
These static calls should be replaced with the
LoggerChannelTrait
.drupal_set_message
is deprecated. Should use theMessengerTrait
.Comment #96
wengerkHere 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 !
Comment #98
wengerkMmmhh .. 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.
Comment #99
Erik FrèrejeanJust a small nitpick you might want to change when going back in to fix the test.
These are not necessary here as they are already part of
\Drupal\Core\Form\FormBase
Besides that seems okay to me.
Comment #100
Erik FrèrejeanNot 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.
Comment #101
Erik FrèrejeanA minor last minute thing I noticed, the schema version in hook update wasn't yet updated for 8.7.
Comment #103
wengerkThanks 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 ?Comment #104
Erik FrèrejeanWell 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
andregister_no_approval_required
), this would ensure that everything works as expected.Comment #105
Erik FrèrejeanI 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?
Comment #106
wengerkI could work on it in the next 2 weeks. If someone want to do it before - feel free - otherwise I will create them.
Comment #107
wengerkHere is the new coverage.
Sorry for the delay (2 months instead of 2 weeks).
Comment #109
wengerkMmhhh... seems the only fails occur because of quality control.
Let's try to fix them.
I don't understand the
Because the use
use Drupal\user\UserInterface;
is used in theswitch:case
asIs it a false positive ?
Comment #110
wengerkMy 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.
Comment #113
wengerkFinally found the issue with the tests here are the changes:
getAccountName
instead of deprecatedgetUsername
assertEquals
instead of deprecatedassertEqual
Comment #114
wengerkComment #115
jeffschulerMigrateUpgrade tests were renamed: #2987418: Rename MigrateUpgrade tests.
MigrateUpgrade6Test.php -> Upgrade6Test.php
MigrateUpgrade7Test.php ->Upgrade7Test.php
Comment #116
wengerkHere the reroll, let's try it via testbot
Comment #118
wengerkComment #120
weseze CreditAttribution: weseze as a volunteer commentedThe last patch does not consider the users language correctly. Drupal core handles this correctly so we should not pass the langcode at all.
should be replaced with
EDIT: ignore the attached patch, it wasn't ment to be attached here. It is wrong :(
Comment #121
weseze CreditAttribution: weseze as a volunteer commentedComment #122
weseze CreditAttribution: weseze as a volunteer commentedPlease 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.
Comment #123
weseze CreditAttribution: weseze as a volunteer commentedComment #124
yogeshmpawarComment #125
yogeshmpawarRe-rolled the patch against 8.8.x, beacuse it's failed to apply on 8.8.x
Comment #126
andreasderijcke#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.
Comment #127
larowlanAccording to d.o this fails to apply
Comment #128
Erik FrèrejeanRerolled 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.
Comment #129
Erik FrèrejeanWith the tests passing again this can go back to RTBC.
Comment #131
sokru CreditAttribution: sokru as a volunteer commented+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
Comment #132
catchThis can happen in a hook_post_update_NAME() since it's just creating new configuration. Also prevents update numbering conflicts that way.
Comment #133
sokru CreditAttribution: sokru as a volunteer commentedMoved action creation from
hook_update_N()
intohook_post_update_NAME
Comment #134
sokru CreditAttribution: sokru as a volunteer commentedFix for coding standard error.
Comment #135
sokru CreditAttribution: sokru as a volunteer commentedJust a reroll.
Comment #136
sokru CreditAttribution: sokru as a volunteer commented133,134 and 135 had code from another issue, sorry. This time only relevant code is in patch.
Comment #137
Erik FrèrejeanWith that, this can go back to RTBC.
Comment #139
sokru CreditAttribution: sokru as a volunteer commentedComment #141
sokru CreditAttribution: sokru as a volunteer commentedComment #142
alexpottI don't see where the langcode is used so I'm not sure we need the language manager.
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.
Comment #143
sokru CreditAttribution: sokru as a volunteer commentedAttached 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.Comment #144
alexpott@sokru an admin (eg user 1) can create an user without an email address via the UI in core already.
Comment #145
sokru CreditAttribution: sokru as a volunteer commentedHere'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).Comment #146
sokru CreditAttribution: sokru as a volunteer commentedChange record draft: https://www.drupal.org/node/3109478.
Comment #147
Erik FrèrejeanThe config is already available in the `userSettings` property.
The config is already available in the `userSettings` property.
Comment #148
sokru CreditAttribution: sokru as a volunteer commentedAttached patch fixes #147.1, but I disagree of #147.2, because
$this->userSettings
is null in this case.Comment #149
Erik FrèrejeanYou 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?Comment #150
sokru CreditAttribution: sokru as a volunteer commentedThis resolves also #147.2 and #149
Comment #151
JurgenR CreditAttribution: JurgenR commentedPatch #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.
Comment #152
sokru CreditAttribution: sokru as a volunteer commentedPatch 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".Comment #153
sokru CreditAttribution: sokru as a volunteer commentedAnd missing interdiff.
Comment #154
JurgenR CreditAttribution: JurgenR commented@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?
Comment #155
sokru CreditAttribution: sokru as a volunteer commentedOnly 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.Comment #156
andypostno reason to read and store config on plugin creation
better to wrap in protected method to allow unit testing
out of scope, please revert
Not sure t() needed here
Comment #157
andypostThe right branch for features now
Comment #158
Erik FrèrejeanFirst off all a re-role on 9.x.
Comment #159
Erik FrèrejeanComment #160
Erik FrèrejeanNew patch including the review from #156, however I left out point 2.
_user_mail_notify
is always called straight up.Comment #161
sokru CreditAttribution: sokru as a volunteer commentedLooks good. Manually tested that t-function is not needed here:
Comment #162
alexpottLet'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
Can to use \Drupal::config('user.settings') instead. And we can file a follow up to do inject correctly in both places.
Let's not use t() here. There's no way our tests would work if Drupal was installed in another language.
Does this need to be assert*Raw? - also let's not use t() here.
Comment #163
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedaddress Comment #162
please review the patch.
Comment #164
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedComment #165
Erik FrèrejeanUnneeded parentheses.
Comment #166
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedThanks, @Erik Frèrejean
removed Unneeded parentheses, please review the new patch.
Comment #167
weseze CreditAttribution: weseze as a volunteer commented$langcode does not seem te be used, so should be removed?
Comment #168
sokru CreditAttribution: sokru as a volunteer commented@weseze good catch! This will remove
$langcode
Comment #169
quietone CreditAttribution: quietone as a volunteer commentedI 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.
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?
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.
I think this should be 'Returns a collection of alternative registration configurations'.
The t() can be removed in tests. See #3145418: [November 9, 2020] Remove uses of t() in assertText() calls
Comment #170
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedworking on it
Comment #171
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commented@quietone I have addressed comment #169.
please review the patch.
Comment #172
Rob C CreditAttribution: Rob C commentedNo 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.
Comment #173
kamkejj CreditAttribution: kamkejj at Nerdery for Nestle Purina PetCare - United States commentedRe-rolled for 8.8.x. I didn't see an 8.8 patch and the 8.7 and 8.9 patches don't apply.
Comment #175
sokru CreditAttribution: sokru as a volunteer commentedNot giving up a hope to get this into core someday... Patch is just a re-roll for 9.2 based on #171.
Comment #177
sokru CreditAttribution: sokru as a volunteer commentedThis should fix the deprecation notices from 9.2.
Comment #178
quietone CreditAttribution: quietone as a volunteer commentedCame 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
Comment #179
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedAddressd #169.2
Comment #180
quietone CreditAttribution: quietone as a volunteer commented@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.
These changes are out of scope and need to be removed from this patch.
drupalPostForm is deprecated.
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.
Comment #181
sokru CreditAttribution: sokru as a volunteer commentedAdded 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.
Comment #183
Erik FrèrejeanThe 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
.Comment #184
sokru CreditAttribution: sokru as a volunteer commentedcore/modules/user/src/Plugin/Action/ResendWelcomeMessage.php needs small change on docblock to pass spell-checking.
Comment #185
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedMade the change suggested in #184. Please review.
Comment #186
sokru CreditAttribution: sokru as a volunteer commentedLooks good to me.
Comment #187
jasonawantI 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.Comment #188
quietone CreditAttribution: quietone as a volunteer commented@jasonawant, your question sounds like a support request to me. So, yes open a new issue or ask in Slack. Thx.
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.
Resend the welcome message to the selected user(s)
s/welcome/the welcome/
Resend the welcome message to the selected user(s)
s/resend/resent/
s/welcome/the welcome/
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.
Comment #189
quietone CreditAttribution: quietone as a volunteer commentedThe config parameter is not used, this can be simplified. This change is needed in other methods as well.
What does 'alternate' mean here? I understood that this was simply resending the 'standard' message?
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.
Change to assert that email was sent, "Assert that an email was sent to a blocked user and admin."
Let's be alphabetical. 'Resend welcome message to user_b and user_c.
Assert that welcome emails were sent user_b and user_c.
As above
As mentioned above
Comment #190
quietone CreditAttribution: quietone as a volunteer commentedFound another one.
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.
Comment #191
jasonawantThanks, done, created #3194013: No way to configure the user.settings notify.register_no_approval_required setting in the UI
Comment #192
adityasingh CreditAttribution: adityasingh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedAddressed #188, #189 and #190.
Comment #193
sokru CreditAttribution: sokru as a volunteer commentedThanks @adityasingh, I think the idea in #189.1 was to get rid of unnecessary
$config
variables.Required changes would look like this:
Comment #194
abhisekmazumdarUpdated the patch as per the suggestion on #193
Comment #195
sokru CreditAttribution: sokru as a volunteer commented@abhisekmazumdar the change is needed for all instances on patch, not just the one mentioned.
Comment #196
abhisekmazumdarOh!! 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.
Comment #198
sokru CreditAttribution: sokru as a volunteer commented@abhisekmazumdar the tests are failing because you removed saving of user.settings config:
when a correct change would have been
Comment #199
abhisekmazumdarComment #200
sokru CreditAttribution: sokru as a volunteer commentedSetting back to RTBC.
Comment #201
catchGood to see progress on this issue that is more or less old enough to drive (!).
'resend' out of context is not very self-documenting. Could this be 'user_welcome_message_action'?
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.
It looks like this could lead to a situation where $op is undefined in practice - can we add a default?
Comment #202
MeenakshiG CreditAttribution: MeenakshiG at OpenSense Labs for Drupal India Association commentedChanged "resend" to "send".
Comment #203
MeenakshiG CreditAttribution: MeenakshiG at OpenSense Labs for Drupal India Association commentedComment #204
catchThat 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.
Comment #205
Erik FrèrejeanI'd agree with @catch here for consistency sake the naming I'd say change the terminology across the patch.
For example
it looks strange that the id contains
resend
but the label just states "send".Comment #206
Rob C CreditAttribution: Rob C commentedThis 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
Comment #207
sokru CreditAttribution: sokru as a volunteer commentedRenamed 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.Comment #208
quietone CreditAttribution: quietone as a volunteer commentedThere are test failures - setting to NW.
Comment #209
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedFixing failing test, Please have a look.
Comment #210
vikashsoni CreditAttribution: vikashsoni as a volunteer and at Zyxware Technologies commentedApplied #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
Comment #211
sokru CreditAttribution: sokru as a volunteer commentedFound one "resent" text from patch. Updated also the issue summary about wording on latest patches.
Comment #212
chetanbharambe CreditAttribution: chetanbharambe for Drupal India Association commentedVerified and tested patch #211.
Patch Not applied successfully and gives error
Please see attached screenshots.
Can be a move to Needs Work.
Comment #213
chetanbharambe CreditAttribution: chetanbharambe for Drupal India Association commentedComment #214
sokru CreditAttribution: sokru as a volunteer commented@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.
Comment #215
chetanbharambe CreditAttribution: chetanbharambe for Drupal India Association commentedI tried to apply the patch and have the following message on the terminal.
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.
Comment #216
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedRe-rolled patch is here. Please review.
Comment #217
chetanbharambe CreditAttribution: chetanbharambe for Drupal India Association commentedI tried to apply the patch and have the following message on the terminal.
I think This patch needs re-roll again.
Comment #218
quietone CreditAttribution: quietone as a volunteer commentedThe 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.
Comment #219
quietone CreditAttribution: quietone as a volunteer commentedMade 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.
Comment #221
chetanbharambe CreditAttribution: chetanbharambe at QED42 for Drupal India Association commentedComment #222
VladimirAusUpgraded to work with latest version of 9.3.
Comment #223
sokru CreditAttribution: sokru as a volunteer commentedReroll for 9.4.x, based on #218. Added reference screenshots from Pantheon and Browserstack, in case usability review would need some.
Comment #224
AnybodyStill 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 ;)
Comment #226
weseze CreditAttribution: weseze as a volunteer commentedRerolled 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.
Comment #227
weseze CreditAttribution: weseze as a volunteer commentedFixed a the missing $account object to use $this->entity instead.
Also fixed new line numbers for 9.4.
Comment #230
JurgenR CreditAttribution: JurgenR commentedUpdated 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.
Comment #231
pradhumanjain2311 CreditAttribution: pradhumanjain2311 at OpenSense Labs for DrupalFit commentedTry to fix CS Erros.
Comment #233
Christopher Riley CreditAttribution: Christopher Riley commentedI 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.
Comment #234
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #236
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #237
Gábor Hojtsy@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.
Comment #238
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks @Gábor Hojtsy!
Am moving to NW as there seem to be failures in the MR.
Comment #239
rkollerI'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.
Comment #240
rkollerUsability 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:
/admin/config/people/accounts
- per default the option should be enabled./user/2/edit
) there are several aspects to note about the three buttons at the bottom of the page:Save
,Send welcome message
andCancel account
:Send welcome message
is fundamentally different fromSave
andCancel 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".Save
is highlighted in blue while the action linkCancel 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./admin/config/people/accounts
is sent out to the user. The user has only a singleSend 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.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:
/admin/config/people/accounts
page in the emails section.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.
Cancel selected user account(s)
is selected: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.
Comment #241
benjifisherI 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:
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.
Comment #243
svendecabooterAs 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.