add settings to notify users when account is active, blocked or deleted (move user_status.module into core)

screenshot of admin/user/settings when you've got "Visitors can create accounts but administrator approval is required." selected as your current registration method...

and here's what the fieldset for "Account activation notification message" looks like by default.

new patch, now ready for review:

• actually works. ;) i cleaned up, simplified, and unified the token generation and email sending. given that, adding the notifications for blocked/activated/deleted took about 4 lines of code total. ;)
• better wording of some titles and descriptions in the admin UI based on feedback in IRC.
• more appropriately sized defaults for the size of the text areas, based on the default texts.

the remaining TODO items:

1. all of the welcome email sending is now sharing _user_mail_tokens(), but it isn't yet using the _user_mail_notify() goodness.
2. the drupal_set_message() when you create an account that's pending approval is lying about having your password sent to you. see http://drupal.org/node/110474
3. the UI changes on the account edit page (and maybe admin/user/user) for exceptions to the site-wide defaults for notifications.

however, i don't think any of those are necessarily worth holding up this patch for, so i'm setting this to CNR.

this patch moves all of the email notifications under the control of _user_mail_notify(). i haven't had a chance to test it as thoroughly as i'd like, but it should be working fine. even though this version has to complicate _user_mail_notify() a little more, overall, i think this is cleaner than the previous patch.

well, the reason (perhaps bad) that i stuck with the cryptic stuff was to match all the existing variable names for these settings, to avoid any upgrade issues... same with the complication on setting $mail_id... i just didn't want to change anything, so existing code or settings that are dealing with these mail messages wouldn't break.

if you'd rather, i could use more verbose $op flags at the call sites, and just make a big switch statement at the top of _user_mail_notify() to set the $mail_id, variable names to lookup, and default notification behavior for each operation separately, to make it more clear but still comply with the old names/values for everything.

or, for a slightly more radical change, we could rename all the $op flags to be verbose, then *change* all the $mail_id and variable names completely, and forget about sites that have already customized these settings. then, the code to _user_mail_notify() wouldn't need a switch at all, and all the mail_ids and variable names would just be directly derived from the $op. i suppose i could add a db update to rename the old variables to their new equivalents, to be really kind to sites that are upgrading...

just let me know what you'd prefer.

thanks,
-derek

• uses more verbose $op values at all call sites. there's now a giant switch statement to map these back into the same variable names and mail ids we always used to use, to avoid the need for a db update, etc.
• s/$rval/$result/
• adds CHANGELOG.txt entry

if you'd rather remove the switch, change all the mail ids and variables to the new verbose names, and add a DB update, we can do that, too. just set back to CNW with clear direction on exactly how you want it. ;)

feedback on the specific choices for the new verbose names would be good, too, if you don't like these:

 *  'register-admin-created': Welcome message for users created by the admin
 *  'register-welcome': Welcome message when users can register themselves
 *  'register-pending-approval': Welcome message, users pending admin approval
 *  'password-reset': Password recovery request
 *  'status-active': Account activated
 *  'status-blocked': Account blocked
 *  'status-deleted': Account deleted

thanks,
-derek

well, removing the switch is obviously nice. ;) the downsides are a) requires a DB update, and b) makes some of the variable names a lot longer, which therefore tends to cause various parts of the source to overflow the nice 80 chars i try to stay under. ;) however, i guess those aren't good enough reasons for the added potential confusion. so, here's a new patch:

• all $op values renamed to use underscores, not hyphens, to facilitate using them directly in variable names, etc
• renames all variable names (and therefore, FAPI elements on the settings page) to match their corresponding $op values
• renames all mail IDs to match the $op values (though using hypens instead of underscores to be consistent with other mail id strings in core).
• renames "status_active" to "status_activated" at the request of webchick
• adds system_update_6017() to rename the old values to the new names (and deletes the old variables, of course).

i tested the upgrade path and a lot of other stuff, but more thorough testing (especially the actual emails themselves) would be helpful. otherwise, i think this should pretty much be RTBC at this point.

actually, i'd like to see a separate cleanup of the drupal_set_message() stuff associated with all of this. in some cases, the text of the message should itself be another setting. see http://drupal.org/node/110474. it wouldn't hurt to check over the watchdog() calls related to all of this, too. however, i don't think either is required before the feature/api freeze, those are just minor usability enhancements. so, i'd prefer to see this patch go in, and i promise i'll get the other stuff in well before the first beta.

thanks,
-derek

thinking ahead is good. ;) yeah, helping contrib with this wasn't on my radar, since this patch started as my attempt to just move one of my contribs into core so i wouldn't have to maintain it myself anymore. ;) that said, yeah, we should probably make user_mail_tokens() a publically-facing API (without the leading underscore), so that other people can use it, too. i guess eaton and greggles aren't going to push for token API in D6 core, so i wasn't even thinking of doing a partial solution like this, but i guess we could do a little better than nothing with this patch.

the actual code for _user_mail_notify() is very specific to user.module. first of all, it doesn't do much at all. what it does depends almost entirely on _user_mail_text(), which is also private to user.module (unless http://drupal.org/node/145164 happens). so, i'd be happy keeping that private.

in terms of the UI, if you put all the email stuff on a separate page, i'm a little worried, since for example, the settings you care about customizing depend on your user registration policy (open, admin-only, requires approval). if you look closely, my patch automatically expands the right fieldset based on the current value of that variable (and with a little jquery we could even consider dynamically expanding/collapsing the fieldsets if you change the radio on the fly). they really are related, so i'd rather not sprinkle them onto different pages if at all possible. i agree that the fieldset of fieldsets isn't the most elegant, but in this case, i think it's probably about as clean a solution as we can get...

contribs *can* hook_form_alter() this settings page themselves, and they can insert their own fieldsets into the user email settings fieldset using the same basic design. and, just like user_status.module has done for a long time, they can use hook_user to know when to trigger a certain action (e.g. maybe the D6 port of user_status.module in contrib will *just* be for automatic notifications on role changes -- http://drupal.org/node/98107)...

new patch:

• user_mail_tokens() is now a public-facing API call, not private to user.module
• fixed a bug in the DB update (forgot i had renamed the "register-welcome" op to be less ambiguous).
• since i hate it when DB updates do queries but don't actually tell you, i changed it to populate $ret with info on each of the variables we're setting and deleting (just the names, not the values)

so, update.php now returns the following for 6017:

system module

Update #6017

• variable_set(user_mail_register_admin_created_subject)
• variable_del(user_mail_admin_subject)
• variable_set(user_mail_register_admin_created_body)
• variable_del(user_mail_admin_body)
• variable_set(user_mail_register_pending_approval_subject)
• variable_del(user_mail_approval_subject)
• variable_set(user_mail_register_pending_approval_body)
• variable_del(user_mail_approval_body)
• variable_set(user_mail_register_no_approval_required_subject)
• variable_del(user_mail_welcome_subject)
• variable_set(user_mail_register_no_approval_required_body)
• variable_del(user_mail_welcome_body)
• variable_set(user_mail_password_reset_subject)
• variable_del(user_mail_pass_subject)
• variable_set(user_mail_password_reset_body)
• variable_del(user_mail_pass_body)

if folks think that's too much noise, it'd be easy to rip out the 2 lines that do this...

whoops, last patch didn't include the CHANGELOG.txt diff.

excellent, thanks folks!

whoops:

modules/comment/comment.js
----------------------------
revision 1.2
date: 2007-05-20 12:34:47 +0000;  author: dries;  state: Exp;  lines: +2 -2;  co
mmitid: 74414650400f4567;
- Patch #144859 by dww: added optional e-mail notifications when user are approved, blocked, or deleted.

which included this diff:

diff -u -p -r1.1 -r1.2
--- modules/comment/comment.js  17 May 2007 21:05:38 -0000  1.1
+++ modules/comment/comment.js  20 May 2007 12:34:47 -0000  1.2
@@ -1,4 +1,4 @@
-// $Id: comment.js,v 1.1 2007/05/17 21:05:38 dries Exp $
+// $Id: comment.js,v 1.2 2007/05/20 12:34:47 dries Exp $
 if (Drupal.jsEnabled) {
   $(document).ready(function() {
     var parts = new Array("name", "homepage", "mail");
@@ -26,7 +26,7 @@ Drupal.comment.getCookie = function(name
       if (end == -1) {
         end = document.cookie.length;
       }
-      returnValue = unescape(document.cookie.substring(offset, end));
+      returnValue = decodeURIComponent(document.cookie.substring(offset, end).r
eplace(/\+/g, '%20'));
     }
   }

which wasn't anywhere in this patch. not sure what that's about, but it should probably be undone. attached patch is just the reverse of this diff, so it should apply cleanly to undo revision 1.2.

thanks,
-derek

Huh? Core notifies the site admin via email when there are new users pending approval... I don't see why http://drupal.org/project/user_register_notify needs to exist at all. Perhaps it could provide the flexibility to notify multiple admins (a user-approval team of some sort), but then again, you could always make the site admin email address a mailing list. Anyway, this thread is *not* the place to discuss it, please don't re-open old issues like this. This issue is about moving user_status.module into core, which we did, and it's now fixed and closed. Let's leave it that way. ;) Thanks.