Thanks for the great module. I'd love the ability to specify different notification messages for each role. For instance, in my use case I want to send one notification message to Affiliates (with specification instructions just for them) and a different notification to Blog Contributors (with a separate set of instructions).

Thanks again!

--Ben

Comments

rfay’s picture

This is a perfectly reasonable idea. I won't plan on doing it any time soon, but patches are welcome.

rfay’s picture

Using triggers and actions and rules you can do what you want with a php snippet. I think that may be your best path forward on this.

In general, I'd like the triggers-and-actions feature to be the future of this module. That way everybody can get what they want done without adding new features to the module.

dspring0021’s picture

@rfay, what kind of PHP snippet are you referring to. I need to do exactly that, but I can't think of how to do this. Furthermore, when using the Role Change Notify trigger in 'Triggers & Actions', I can't seem to find a way to get the correct tokens for the user whose account is having the role added. There's just stuff like 'current logged in user', 'author', 'owner', etc. See my comment on this post for more info.

ColinMctavish’s picture

subscribing

teliseo’s picture

Version: 6.x-2.0-beta3 » 6.x-2.x-dev
Status: Active » Needs review
StatusFileSize
new2.28 KB

Attached is a patch to create separate add and remove triggers for each role (in addition to the global ones). By attaching these to different actions, different emails can be sent for each role addition (or removal).

Status: Needs review » Needs work

The last submitted patch, role_change_notify-triggers_per_role.patch, failed testing.

rfay’s picture

Thanks, teliseo!

I see there's a thing or two to clean up still.

Way cool, and thanks so much!

I also hope you'll be willing to do this for the D7 version.

teliseo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, role_change_notify-triggers_per_role.patch, failed testing.

rfay’s picture

Hmm. A retest of the committed code shows that your patch is not at fault. Something changed about the testbots and now we don't pass: http://qa.drupal.org/pifr/test/37173

I opened #1010320: DRUPAL-6--2 tests are broken (token problems with undefined indexes). Something has changed about the testbot....

Note that before this patch can go in it will need a test.

Thanks for your work on this!

-Randy

teliseo’s picture

Status: Needs work » Needs review
StatusFileSize
new15.42 KB

Here’s a new patch that incorporates several things:

  • The broken test was not due to code in my patch, however it was due to RCN code in role_change_notify_action_info_alter(). I’ve fixed this and I hope it will now pass the auto-testing. If it does, #1010320: DRUPAL-6--2 tests are broken (token problems with undefined indexes) should be closed.
  • My original changes for triggers per role, of course.
  • Tests that exercise my new triggers (as well as some global fixes to the test code). I’ve applied your patch #736598: Backport triggers patch from D7 and defined Issue736598Committed for my local testing, which runs without error. I assume the auto-test bot won’t be doing this.
  • Additions to the actions that may be associated with RCN triggers, such as views_bulk_operations_user_roles_action, which allows modification of other roles to be triggered. These didn’t make much sense when there were only global triggers.
  • Removal of user_load() in role_change_notify_user_update(). I can’t find any reason for this, as a perfectly good user object arrives as a function argument. Besides for being redundant, doing user_load() clobbers any extra properties that may have been set previously, such as ones used for operation-specific tokens. I ran into this bug while making some trigger and token-related changes to Role Expire. Your first 1.1 version of RCN (for Drupal 5) does this also—is it a remnant, or do you still have a good reason for doing user_load() now?
  • Proper token formatting for user profile fields of type date. Previously these would generate an error. Yes, I realize that the profile token stuff doesn’t really belong in RCN anyway.

You had asked whether I intend to port my changes to D7. I don’t have time to do this now, as getting D6 to work the way I want is keeping me busy, but I might at some time in the future.

rfay’s picture

Wow, excellent.

Here's a reroll now that #1010320: DRUPAL-6--2 tests are broken (token problems with undefined indexes) is resolved and testing is working again. Unfortunately we can't test token-related stuff.

@teliseo, I think the reason for the user_load() is to make profile values appear for the profile support. Did you test it that way?

You seem to be invested in this stuff enough that you might be interested in being a comaintainer. Is that true?

teliseo’s picture

I guess your HEAD already had part of my fixes for the broken test that I was patching in role_change_notify_action_info_alter()—thanks for re-rolling the patch.

I have tested profile tokens without the user_load(), and they seem to work properly. I can’t find any difference between the user object arriving at nodeapi update and a new one obtained from user_load(), except that the original object has modifications that previous modules, or the calling module, have made. Unless I’m missing something, where else could a user object have come from at nodeapi update if not from a previous user_load()? I’ve been doing some work recently on Node clone and Node Repeat, so I think I understand this stuff pretty well.

Yes, I’d be happy to be a comaintainer of this module. Is there anything I have to do to initiate this?

rfay’s picture

Great, and Welcome!

I added your privileges for everything except CVS commit. You have to apply for CVS, but it's easy when you're invited to comaintain. Please apply at http://drupal.org/cvs-application/requirements. Once you've applied, give me the issue and I'll RTBC it and somebody will grant it for you. But you already have the privileges necessary to grant yourself CVS at http://drupal.org/node/333543/maintainers.

It will be fantastic to have somebody maintaining who is actually using the module, which I'm not.

Here's what I ask of comaintainers: Do your work publicly in the issue queue. Post patches in "needs review", rather than just committing. Make sure that things have tests if they can.

The reality is that most of the time nobody will review your work, so you'll be the reviewer (or with two of us maybe we'll be luckier than that!)

I'm going to test this patch one more round, but I'll leave it to you to finish it off once you have your CVS access.

---

Back on the user_load() issue, I think the case where you have to do a full load is when you're not using tokens. I'll check it.

rfay’s picture

Oh, if you're new to CVS... I recommend using it only to commit, as described here: http://groups.drupal.org/node/91424. We're not going to be there long, so it is not worth the effort of coming up to speed on it.

rfay’s picture

Status: Needs review » Reviewed & tested by the community

OK, this looks good to me.

Are you also fixing #891214: warning: htmlspecialchars() expects parameter 1 to be string, array given in includes/bootstrap.inc on line 857, or am I mistaken?

I found no trouble with the profile fields with token on or off.

When you commit this, please add a note to the project page explaining the new feature and how it impacts existing users.

I'll be gone for the next couple of weeks so probably won't be looking at this.

rfay’s picture

Status: Reviewed & tested by the community » Needs review

Guess I shouldn't in general RTBC my own patch (but it's really yours). But I'll let you RTBC and then "fix" it.

teliseo’s picture

Thanks for making me a co-maintainer. I will do my best to act responsibly. Here is a link to my CVS account request. I’m already quite familiar with CVS, but will be much happier when everything moves to Git.

Yes, my patch also fixes #891214: warning: htmlspecialchars() expects parameter 1 to be string, array given in includes/bootstrap.inc on line 857—sorry I missed that. Note that the patch submitted there only prevents the PHP warning, but doesn’t actually decode the date profile field. My fix decodes the date field. I’ll update the issue after my patch is committed.

avpaderno’s picture

FYI, teliseo has now CVS access to this project.

teliseo’s picture

Status: Needs review » Reviewed & tested by the community

No further comments, so it’s now RTBC. Patch has been committed—waiting for snapshot update.

teliseo’s picture

Status: Reviewed & tested by the community » Fixed

Committed to DRUPAL-6--2: http://drupal.org/cvs?commit=477344

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

simone960’s picture

Issue summary: View changes

Any patch for D7 ?

simone960’s picture

Found a way to do it with Rules module instead.

http://goo.gl/30hJvo

rfay’s picture

Added that to the project page.