[rt-devel] Patch: AlwaysNotifyActor usable in RT::Action::NotifyGroup

[Alex Vandiver]

On 12/04/2014 06:05 AM, Nathan Boddy wrote:
> 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 at 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<does not> 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