Emails being sent to blocked user accounts

+1

I'm changing this to drupal 7 because the original poster hasn't done anything since last year. I think I fixed it by changing lines 101-109 of system.eval.inc in rules/modules from

  // All authenticated users, which is everybody.
  if (in_array(DRUPAL_AUTHENTICATED_RID, $roles)) {
    $result = db_query('SELECT mail FROM {users} WHERE uid > 0');
  }
  else {
    $rids = implode(',', $roles);
    // Avoid sending emails to members of two or more target role groups.
    $result = db_query('SELECT DISTINCT u.mail FROM {users} u INNER JOIN {users_roles} r ON u.uid = r.uid WHERE r.rid IN ('. $rids .')');
  }

to

  // All authenticated users, which is everybody.
  if (in_array(DRUPAL_AUTHENTICATED_RID, $roles)) {
    $result = db_query('SELECT mail FROM {users} WHERE uid > 0 AND status = 1');
  }
  else {
    $rids = implode(',', $roles);
    // Avoid sending emails to members of two or more target role groups.
    $result = db_query('SELECT DISTINCT u.mail FROM {users} u WHERE status = 1 INNER JOIN {users_roles} r ON u.uid = r.uid WHERE r.rid IN ('. $rids .')');
  }

My sql skills are lacking, but this should check so emails aren't sent to blocked users.

Changing this to dev because it's still an issue. I'd roll a patch but I'm on a very restricted shared hosting and can't install git.

My patch in #2 above works well, we just need this committed.

@elBradford: This looks like a wise change. Could you please post a patch and set this to 'needs review'? See Making a Drupal patch with Git for instructions.

What about alternatively doing this as an option? "Check if user is active?" (description: If user account is inactive, the email will not be sent.)?

@mitchell: I'm willing to look into adding the option when I have some free time, but I don't know of a way to make a patch without git access - maybe I can do it offline but I'll have to look into that.

> I'm willing to look into adding the option when I have some free time
Cool. I'm looking forward to testing for you.

> I don't know of a way to make a patch without git access
* Git access is available to all Drupal accounts who agree to a few guidelines. It's available in your profile edit page.
* Local git development is all that's necessary to make a patch. If this was more complex, then you might need a sandbox on d.o to fork Rules, but making a patch and posting it here is all that's necessary. The linked instructions should all work for you if you have git installed locally.

I didn't write the guidelines for setting statuses of issues, I'm only following it. It's also better for long term growth to encourage development through the git tools and in line with the expectations of the maintainers. fago has set the status accordingly in the past when valid code blocks are posted, so that he can stick to reviewing patches, so I'm following his lead.

Thanks for the heads up, I'll start on this next week hopefully.

I rolled this as a patch and I included the patch found in [#156119] because for some reason it wasn't being committed or tested. Apologies if it was wrong to roll these two together, this is my first patch.

I included the option to send to blocked users which is false by default (which makes sense to me - sending emails to blocked users was a bad assumption imo).

What's holding this from being reviewed / committed?

This is an answer to why Rules patches aren't being tested - http://drupal.stackexchange.com/questions/36167/what-does-test-status-po...

You should not roll another issue's patch into your own.

The reason for this is because one issue could already have been agreed upon, while the other is still being discussed.

Please remove the code of #1325834: Rules action: Send mail to all users of a role, authenticated missing from list from this patch.

I can do that, but that issue was still open but appeared abandoned because there was a patch there for 2 MONTHS with no response besides me saying it worked well. You want people to post patches then please be more responsive. If I post a patch again what's the point if, by the time it's reviewed, I need to roll it again against a new dev version? Are the maintainers just waiting for the testing system to be fixed before they even look at these patches?

I know, it sucks sometimes. Do keep in mind however that most contributors do this in their spare time.

If you really feel that this module is being neglected by its creator(s), you could always send them a PM to inquire what's keeping them from reviewing patches.

K, thanks for the feedback Kristiaan, I'll reroll this patch again.

Okay, finally making a review (this assumes changes implied by #12):

+ //Send to blocked users?
Missing a space.

+ 'description' => t("Send emails to blocked users?"),
This should be worded as a statement.

code
It looks good, but I didn't actually test. Yes, looking to other reviewers to help.

> Are the maintainers just waiting for the testing system to be fixed before they even look at these patches?
They're working on other projects like Core and Entity API. Basically, Rules needs you! There are many issues that need solutions, so please don't feel discouraged, fix those other issues :) "A rising tide lifts all boats."

> If I post a patch again what's the point if, by the time it's reviewed, I need to roll it again against a new dev version?
Git tools take care of this problem.

Changes made per #12 and #16 on latest dev. Looking for reviewers. Thanks!

I should probably upload the patch.

New patch, sql syntax was a little off. This one works.

I think this should be done with the Database API like in #1780412: Option to exclude blocked users from a role

@sgabe - That should be a separate issue since changing just this part of rules to use the Database API would be inconsistent with the rest of the module.

I'd like some more feedback, since each time Rules updates I have to reapply this patch. Hopefully we can get this committed soon.

This seems to work for me - I think it would make sense for it to be applied to the normal "Send Mail" rule too. Thats the use case I am looking to use it in. Any chance of updating the patch to include the normal send mail rule too? Thanks for your work on this

Patch #19 works nicely for me. Once patched you need to edit your rules to set the new "blocked" option, the rules UI gives a red warning message to this effect.

hm, the approach makes sense but we need to take BC into account here. Best, just make the parameter optional, add a default value and move it to end.

@fago: Is this more what you're looking for?

He means you should add the $blocked parameter to the end of the function's parameter list with a default provided. This way, the code of people who implemented rules_action_mail_to_users_of_role() themselves doesn't break.

@bhosmer sorry if this is stepping on toes but needed this functionality for a client as well and implemented @kristiaan's suggested modification to your patch.

@t_stallmann not at all! Honestly, I still wasn't sure what was needed for it. I wound up using hook_mail_alter instead, but thanks for doing the patch.

It's still not at the end, though.

@kristiaanvandeneynde that's the part that apparently isn't clear to a few of us still.

Are you meaning

function rules_action_mail_to_users_of_role($roles, $subject, $message, $from = NULL, $blocked = TRUE, $settings, RulesState $state, RulesPlugin $element)

Like this:

function rules_action_mail_to_users_of_role($roles, $subject, $message, $from = NULL,  $settings, RulesState $state, RulesPlugin $element, $blocked = TRUE)

?

The latter is what fago meant. That way any call to rules_action_mail_to_users_of_role() won't break because you change the order of parameters.

@kristiaanvandeneynde thanks for the clarification. That makes it much more clear.

The latest patch still adds $blocked somewhere in the middle of the parameters.

It fails logically because Rules automatically hands off the variables it knows from hook_action_info(). This means $blocked is passed along before the RulesState.

So either we have to go with the patch from #29 and break BC for people who implemented rules_action_mail_to_users_of_role() manually or we need to write an entirely new one (example: rules_action_mail_to_users_of_role_and_status() ) and deprecate the old one.

That being said, I wonder how many people manually implemented just the callback rules_action_mail_to_users_of_role()? I'm guessing this is up to fago. Perhaps you should ping him about this issue?

I pinged @fago. Let's see what he has to say about this.

Found this issue after small searching. First i wanted to say: add status variable same as MimeMail does and it will be easy fast patch.
Now reading #40 made me unhappy :-( kristiaanvandeneynde you are completely right about backwards compatibility.

The best solution without breaking anything to future on my site seems to be use MimeMail's Send HTML to all users of role action.

Like bhosmer wrote in #30, I ended up writing the hook_mail_alter in our custom module.
Then I was wondering why we can't have the hook in this module itself to modify $message['send'] = FALSE if its user is blocked?
Is that a bad practice to do so?

@ak55: I haven't heard anything from @fago yet regarding direction with this.

I don't think adding a hook_mail_alter is an altogether entirely bad idea. It might preserve backwards compatibility. This might limit some functionality though.

Maybe even a small rules plugin that could be enabled is the solution?

Closed #2454045: Don't send mail to blocked users in the action "Send mail to all users of a role" as a duplicate.

Hi, I re-rolled #29's patch for the 2.12 version ; if someone still needs this.