Patch: AlwaysNotifyActor usable in RT::Action::NotifyGroup

RT Developers
1

Hi RT Devs,

Many thanks for a great product.

I wanted the AlwaysNotifyActor feature from RT::Action::Notify in
NotifyGroup and found it pretty easy to implement.

Removed NotifyActor checking from NotifyGroup and left it to the parent
Notify class.

There were no tests related to these files so I have not added any.

Hope this helps. This would be my first open source contribution so any
feedback appreciated.

Regards,

Nathan

0001-AlwaysNotifyActor-can-now-be-used-in-RT-Action-Notif.patch (1.69 KB)

2

Many thanks for a great product.

Thanks for the patch!

I wanted the AlwaysNotifyActor feature from RT::Action::Notify in
NotifyGroup and found it pretty easy to implement.

Removed NotifyActor checking from NotifyGroup and left it to the parent
Notify class.

There were no tests related to these files so I have not added any.

Hope this helps. This would be my first open source contribution so any
feedback appreciated.

The code is good! I’ve a few notes on the commit message and comments,
of the sort I’d provide in an internal review of a branch like this.

commit 94e58d51e2d6302e4773f4d990ba4c93901dcac1
Author: Nathan A Boddy nathan.boddy@gmail.com
Date: Thu Dec 4 05:24:33 2014 -0500

AlwaysNotifyActor can now be used in RT::Action::NotifyGroup arguments.

Handled by the parent class RT::Action::Notify.

This needs a little more of “why”, in addition to “how”. That is, first
describe the problem that this commit is fixing, then argue why this is
the correct way to do so. I might phrase it as:

Allow the NotifyGroup action to respect AlwaysNotifyActor argument

RT::Action::NotifyGroup currently respects NotifyActor by
explicitly removing the creator from its recipients, instead of
relying on the existing RemoveInappropriateRecipients codepath.
This prevents any 'AlwaysNotifyActor' property in the Argument from
taking effect.

Leave the removal of the Actor to Notify, the behavior of which
NotifyGroup inherits.  This removes duplicate code, as well as
leaving RemoveInappropriateRecipients to log the action, instead of
doing so silently.  The AlwaysNotifyActor flag is supported by
merely skipping its presence, if found, in Argument.

diff --git a/lib/RT/Action/NotifyGroup.pm b/lib/RT/Action/NotifyGroup.pm
index 789c182…6d2b9d7 100644
— a/lib/RT/Action/NotifyGroup.pm
+++ b/lib/RT/Action/NotifyGroup.pm
@@ -73,6 +73,10 @@ require RT::Group;
=head2 SetRecipients

Sets the recipients of this message to Groups and/or Users.
+Explicitly B notify the creator of the transaction by default.

This is somewhat misleading. It is not that it “explicitly” doesn’t
notify the actor – that implies that we’re removing the actor. I’d say
it “respects the creator’s NotifyActor prefernce, unless
AlwaysNotifyActor is in the Arguments of the action” or something similar.

=cut

@@ -84,16 +88,6 @@ sub SetRecipients {
$self->HandleArgument( $ );
}

  • my $creatorObj = $self->TransactionObj->CreatorObj;
  • my $creator = $creatorObj->EmailAddress();
  • my $TransactionCurrentUser = RT::CurrentUser->new;
  • $TransactionCurrentUser->LoadByName($creatorObj->Name);
  • unless (RT->Config->Get(‘NotifyActor’,$TransactionCurrentUser)) {
  •    @{ $self->{'To'} } = grep ( !/^\Q$creator\E$/, @{ $self->{'To'} } );
  • }

Mmmm, code removal for functionality increase. I’m always a fan.

 $self->{'seen_ueas'} = {};

 return 1;

@@ -103,6 +97,8 @@ sub _HandleArgument {
my $self = shift;
my $instance = shift;

  • return if ( $instance =~ /^AlwaysNotifyActor$/ );

As a nitpick, there’s no compelling reason to use a regex here – you’re
using it just as a straight-up case-sensitive whole-string match, so
it’s probably clearer to use eq.

Send along a fixed-up version, and I’ll apply to 4.2-trunk. Thanks again!

  • Alex