Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
I'm using the 'Send a mail to all users of a role' action to send notification emails upon certain conditions. I noticed user accounts that are blocked also receive the emails. Surely this should not be the case? If this is by design, there should be an option to send to active/blocked user accounts.
Comments
Comment #1
abwc CreditAttribution: abwc commented+1
Comment #2
elBradford CreditAttribution: elBradford commentedI'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
to
My sql skills are lacking, but this should check so emails aren't sent to blocked users.
Comment #3
elBradford CreditAttribution: elBradford commentedChanging 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.
Comment #4
elBradford CreditAttribution: elBradford commentedMy patch in #2 above works well, we just need this committed.
Comment #5
mitchell CreditAttribution: mitchell commented@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.)?
Comment #6
elBradford CreditAttribution: elBradford commented@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.
Comment #7
mitchell CreditAttribution: mitchell commented> 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.
Comment #8
elBradford CreditAttribution: elBradford commentedThanks for the heads up, I'll start on this next week hopefully.
Comment #9
elBradford CreditAttribution: elBradford commentedI 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).
Comment #10
elBradford CreditAttribution: elBradford commentedWhat's holding this from being reviewed / committed?
Comment #11
elBradford CreditAttribution: elBradford commentedThis is an answer to why Rules patches aren't being tested - http://drupal.stackexchange.com/questions/36167/what-does-test-status-po...
Comment #12
kristiaanvandeneyndeYou 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.
Comment #13
elBradford CreditAttribution: elBradford commentedI 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?
Comment #14
kristiaanvandeneyndeI 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.
Comment #15
elBradford CreditAttribution: elBradford commentedK, thanks for the feedback Kristiaan, I'll reroll this patch again.
Comment #16
mitchell CreditAttribution: mitchell commentedOkay, 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.
Comment #17
elBradford CreditAttribution: elBradford commentedChanges made per #12 and #16 on latest dev. Looking for reviewers. Thanks!
Comment #18
elBradford CreditAttribution: elBradford commentedI should probably upload the patch.
Comment #19
elBradford CreditAttribution: elBradford commentedNew patch, sql syntax was a little off. This one works.
Comment #20
sgabe CreditAttribution: sgabe commentedI think this should be done with the Database API like in #1780412: Option to exclude blocked users from a role
Comment #21
elBradford CreditAttribution: elBradford commented@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.
Comment #22
iaminawe CreditAttribution: iaminawe commentedThis 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
Comment #24
fonant CreditAttribution: fonant commentedPatch #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.
Comment #25
fagohm, 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.
Comment #26
bhosmer CreditAttribution: bhosmer as a volunteer commented@fago: Is this more what you're looking for?
Comment #27
bhosmer CreditAttribution: bhosmer as a volunteer commentedComment #28
kristiaanvandeneyndeHe 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.
Comment #29
t_stallmann CreditAttribution: t_stallmann at Savas Labs commented@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.
Comment #30
bhosmer CreditAttribution: bhosmer as a volunteer commented@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.
Comment #31
kristiaanvandeneyndeIt's still not at the end, though.
Comment #32
bhosmer CreditAttribution: bhosmer as a volunteer commented@kristiaanvandeneynde that's the part that apparently isn't clear to a few of us still.
Are you meaning
Like this:
?
Comment #33
kristiaanvandeneyndeThe 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.
Comment #34
bhosmer CreditAttribution: bhosmer as a volunteer commented@kristiaanvandeneynde thanks for the clarification. That makes it much more clear.
Comment #35
bhosmer CreditAttribution: bhosmer as a volunteer commentedComment #36
kristiaanvandeneyndeThe latest patch still adds $blocked somewhere in the middle of the parameters.
Comment #37
bhosmer CreditAttribution: bhosmer as a volunteer commentedComment #38
bhosmer CreditAttribution: bhosmer as a volunteer commentedComment #40
kristiaanvandeneyndeIt 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?
Comment #41
bhosmer CreditAttribution: bhosmer as a volunteer commentedI pinged @fago. Let's see what he has to say about this.
Comment #42
kmajzlik CreditAttribution: kmajzlik commentedFound 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.
Comment #43
ak55 CreditAttribution: ak55 commentedLike 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?
Comment #44
bhosmer CreditAttribution: bhosmer as a volunteer commented@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?
Comment #45
TR CreditAttribution: TR commentedClosed #2454045: Don't send mail to blocked users in the action "Send mail to all users of a role" as a duplicate.
Comment #46
julienjoye CreditAttribution: julienjoye at Ekino commentedHi, I re-rolled #29's patch for the 2.12 version ; if someone still needs this.