change email recipient

I second this. We are a developing party, and in control of UID 1, but these notifications should go to the clients appointed user management person.

These seems to not be much activity in this module. Is this module still maintained?

Just because a feature request is made, doesn't mean we're going to jump all over it. More appropriate yet might even be to use actions/triggers, but there is no patch here for anyone to review.

Yes, I second that request as well. I am relatively new to Drupal and know just enough PHP to be dangerous, so I won't even try to work up a patch, but it would be good to be able to make the sender of notifications to users (password resets, approvals, etc.) to be other than UID 1. I am having that very same problem with a client website right now. I am UID 1, and getting email from the users. Client would like me to change it and I can't!

If somebody knows how to get around this, it would be great.

Thanks very much & regards,

deekayen:

Thanks for jumping in, how ever I do not understand your reaction. We are all using this module and we see something that in our eye's could be improved.

You could have said:
"I am quite busy right now, if you need this, please supply a patch and we will review it"

I am not expecting you to complete this at all in a instant, would be nice, but not expecting. I need something, you maybe not.

So, I have asked one of our developers to come-up with a patch, in the coming weeks, between the projects.

I hope you are willing to review it when it is there, so we both can improve this module.

Btw: we can offer 1 hour paid support per week for maintaining this module. If you are interested, we can discuss that.

Thanks for your consideration,

Jons

hey there,

@jbenjamin: of course you're NOT expected to submit a patch when you submit a feature request. On the other hand, module maintainers are also not obliged to satisfy every request, although it is always nice to hear a friendly response :/

Anyway, here's a patch that replace the simple checkbox "send notification to admin when.." with a selection of a user who should get this notification, if any.. I could have left the old checkbox and added a user selection too, but that just seemed pointless..

Let me know if you see any issues with the patch and if you want to have anything changed.

Cheers

PS. The original request suggested that setting these to a particular email, rather than a user, would be even better. However, I personally think that's not a good idea because just imagine if every module started having an email address field for its notifications - that would be one big mess. So, following the CMS principle, it's probably best to tie the notifications to a specific user, who can easily change his email in his profile later without having to change it in some modules as well.

If you're going to make new variables and forget the old, create an update function in the .install file to variable_del the old ones.

This module is used by sites with hundreds of users. Make the user select an autocomplete textfield box.

Are there any tests you could or should add to the .test file for this change?

right, deleting the old variables now, and also setting the new one to the uid=1 user's name if the old variable was checked, so nothing will get lost on update.

good point, changed to autocomplete textfield with username validation.

adjusted the tests to use the new variables.

Let me know if there's anything else wrong with it.

I've updated the patch.. there is a typo in the update function about the variable holding the username to notify of ongoing attacks:

This part is from the update_6002 function.

+    variable_set('login_security_user_blocked_email_user', $admin_name);
+    variable_set('login_security_user_activity_email_user', $admin_name);

but later in the UI, the form asks for this:

  $form['login_messages']['login_security_login_activity_email_user'] = array(
    '#type' => 'textfield',
    '#title' => t('Select who should get an email message when an ongoing attack is detected'),

it is using two differente variable names.

Also, I've included some code to setup admin user for both variables just in case a module is 'installed', not just 'updated', because the fields appear empty after a clean install.

After some tests, the only think I'm missing is trying to use different users for the email notification.. could any one do some real testing before committing?

Hey, thanks for the typo fix and the adjustments in the text.

I'm not sure if the setting of the variable on install is the way to go though - in previous versions the email notification was turned off by default - that would equal to that variable simply not being set.. So to make things consistent I would remove the stuff you added to hook_install.. (I really don't care if the notifications are set to 'on' or 'off' by default, just pointing out that by adding that code you've changed the default setting.)

Cheers

mmm I just modified the fields of the email recipients in the install process. I did not enable the feature by default. I did this just because notifications are separated from the 'enable' checkboxes', and it may happen that users would enable the feature with empty recipients in the email notifications if they don't realize that fields are empty.

Basically, I just ensured that "uid = 1" for recipient still works when enabling the feature. Anyway, go ahead, review, change, update a patch, and lets get this commited.

Hey, well actually my/our patch removes the "enable" checkboxes completely, so notifications now fully depend on the new username textfield: if there is nothing there - notifications are disabled, and if there is a name there - then notifs are sent to that user name. So by setting that field to the admin username you actually enabled the notifications by default. So considering that, I think the notification username field should be simply empty, which is equivalent to having the "enable" checkbox unticked in the older version.

Sorry if my original patch was unclear and let me know if you have any further questions.

Attached patch is equivalent to yours without the changes to hook_install.

Actually scratch the last patch. Some of the variable_del calls needed to be fixed - they didn't have the 'login_' prefix for the 'login_activity_' stuff (was errorneous in the module previously, not introduced by us).

Should be perfect now :)

I've included:
- changes in readme.txt
- descriptions in the fields (no notification will be sent if the field is in blank)
- error checking for the users autocomplete if users has no access to profiles (I know, probably uncommon, but..)

Patch looks good to me, if someone has already tested in a live site I'd set rtbc.. when this one gets commited probably we should also update the doc page http://drupal.org/node/595068 also.

Apologies arski, you were right, I missunderstood the fact that username fields replaced the checkboxes. Thanks for the tip and the updated patch.

arg! arski, could you update the .txt, descriptions and error checkings into notifuserselect_v3.patch patch? login_security-866068-14.patch was based in the wrong one then.

Hey, updated.. just one question first though - you sure about adding that "access user profiles" check? Is that permission meant to protect the actual profile data (which we're not accessing here) or the list of usernames? I'm just thinking that if you're editing a node, you can set the authorship of that node using the autocomplete field too (and possibly somewhere else) and that permission is not checked there.. so maybe it would be more consistent to keep it as it was. Or do you think it's really necessary?

Actually, it is because this permission is required to access the autocomplete function without getting an error.. No any other particular reason.

/**
 * Implementation of hook_menu().
 */
function user_menu() {
  $items['user/autocomplete'] = array(
    'title' => 'User autocomplete',
    'page callback' => 'user_autocomplete',
    'access callback' => 'user_access',
    'access arguments' => array('access user profiles'),
    'type' => MENU_CALLBACK,
    'file' => 'user.pages.inc',
  );

I see.. maybe the person should get an error then? It feels weird to me to allow some semi-admin user to set the notification username manually (which he would still be able to do), but not allow him to use the autocomplete function..

Then again, you decide, I don't have any more arguments, so if you say it's better this way I'll put put it in the patch and we can get on with it :)

Please, include it, this way user autocomplete feature will be included only if user has permissions to access that callback, otherwise a nasty 403 error will show up everytime a user types a letter in any of the name fields..

You can just try, remove the 'access user profiles' permissions for that 'semi-admin' user and try to complete a name in the login_security form.

all right, here we go :)

arski, have you already tested in a live site? it is the only step before we mark this as rtbc.

yep, not a live site of course, but my development site. The update from older version works perfect (unchecked->empty field, checked->admin user selected). And the notifications go out to the email of the selected user. so let's mark it rtbc

Thanks for the good work on this! Now, lets wait a pair of days to see if someone concerns about this. In the mean time, would you help me to verify this other one pending review?

#730288: Some notification text changes aren't saving

Applied to DRUPAL-6--1. Needs update for HEAD.

Just wondering, what the current status of this is now and if still the same, how can we assist to make this in the next release?

Hi j.slemmer, I'd love to have also the grammar issues fixed before creating a new release, unfortunatelly my natural language is not english, and this is the reason why that issue is still open.

Hi ilo,

I could assist with that. But I do not know on how to create a patch.
Is there a way you supply ALL the texts used in the module?

If I have that, I can review them all in 1 time for you.

Is something like that possible?

For sure it is ok for me!

Get this issue #620238: Spelling and grammar issues and review the proposed changes.
Install the http://drupal.org/project/potx module, and extract a pot file for the module, this will get you a list of all the strings in the module.

Get that pot file, update the strings and attach the file in the issue queue. After that I'll provide a patch with your text strings and update the module code.

I'll be reviewing this bug in the mean time: #730288: Some notification text changes aren't saving

When these two issues are fixed I think it is time to release a new version of the module. Thanks a lot for your help.

Important note: considering the usage of the module, try to do the minimun changes as possible, because it might break translations into other languages, I mean, avoid improving current texts and focus on fixing only spelling grammar issues. README file also deserves special attention I'd say (this one will not be covered by the .pot file generated with potx.)

And of course, thanks so much for helping in this!

ilo,

Would it be possible, to commit tot -dev?
Then I can use that fix for now and will do the work as soon as I am not in China anymore (ie next week), so you can commit for the real deal.

Thanks for your support

I committed #21 to -dev on September 20. Which patch are you wanting committed from this issue?

I was talking about that one, ignore my last comment

Actually, rather than (or as an alternative to) select the recipient from the users, I would find it more logical to link this module with http://drupal.org/project/logging_alerts and use the e-mail addresses stored there for notification (eg. in my actual case, I use a special e-mail address that goes directly to my cellphone for immediate notification and I don't have that address as a user on any site I administer). If there is interest, I can upload the patch.

I'm not sure if I understand. Are there not enough unique watchdog messages in the module for the loggin_alerts module to know what is happening already? In any case probably there are a ton of email routing/altering modules in contrib to get this done, however to get a new release the problem is not changing the email address, is about the translatable, as long as dekayeen already committed this: http://drupal.org/cvs?commit=425120

In any case, please, djg_tram, fill a new issue for this to be discussed.

No, what I meant was that instead of adding yet another place to specify the recipient, I prefer to use one already set for this purpose in a logically connected module. Out of the box, Login Sec sends warnings about users blocked and ongoing attack to user #1. The suggestions here will allow you to select another user from the users present on the site. On my sites, I don't use any of these schemes. I have a third address, a special e-mail set up to send immediate SMS notification to my cellphone. This e-mail address doesn't appear as a user on any site.

I already use this warning address in Logging Alerts for critical and emergency warnings. So, I simply modified Login Sec to send to those addresses instead of user #1.

Basically, what I suggest is that if Login Sec finds Logging Alerts also installed and present, it should assume that the warning addresses set there are probably the ones the admin prefers to use for warnings. In real life, of course, it would be best to provide the user with a selection of alternatives as:

- use the appropriate address from Logging Alerts
- send to user #1
- send to user #n
- send to an e-mail directly specified on spot

For my own limited use, hardwiring to item 1 was good enough.

I suppose this can be closed since it's in the latest stable release?