Merge branch '5.0/update-ticket-owner-before-message' into 5.0-trunk

docs/UPGRADING-5.0

@@ -634,4 +634,28 @@ messages, you may need to update your system to match the new format.
 
 =back
 
+=head1 UPGRADING FROM 5.0.5 AND EARLIER
+
+=over 4
+
+=item Ticket Owner Updates and Notifications
+
+When processing ticket updates, RT previously processed the reply/comment
+before processing other ticket updates, including owner changes. For RTs
+configured to send email to some users only when they are the ticket owner,
+this processing order resulted in new owners not seeing the reply/comment associated
+with the update that made them the new owner. This could cause confusion, especially
+if the reply/comment included information specifically addressed to the new
+owner.
+
+To fix this issue, owner changes are now processed before messages, so new
+owners will now see the message associated with the change that made them
+owner.
+
+This will result in new email being sent when it wasn't previously. We believe
+this fixes the previous incorrect behavior. However, if you relied on this behavior,
+and don't want the new email, you may need to modify your scrip configurations.
+
+=back
+
 =cut

lib/RT/Interface/Web.pm

@@ -3171,14 +3171,70 @@ sub ProcessCustomFieldUpdates {
     return (@results);
 }
 
+=head2 ProcessTicketOwnerUpdate ( TicketObj => $Ticket, ARGSRef => \%ARGS );
 
+Processes just Owner updates on the provided ticket, based
+on the provided ARGS.
+
+Returns an array of results messages.
+
+=cut
+
+sub ProcessTicketOwnerUpdate {
+
+    my %args = (
+        TicketObj => undef,
+        ARGSRef   => undef,
+        @_
+    );
+
+    my $TicketObj = $args{'TicketObj'};
+    my $ARGSRef   = $args{'ARGSRef'};
+    my @results;
+
+    my $OrigOwner = $TicketObj->Owner;
+
+    # Canonicalize Owner to ID if it's not numeric
+    if ( $ARGSRef->{'Owner'} and ( $ARGSRef->{'Owner'} !~ /^(\d+)$/ ) ) {
+        my $temp = RT::User->new(RT->SystemUser);
+        $temp->Load( $ARGSRef->{'Owner'} );
+        if ( $temp->id ) {
+            $ARGSRef->{'Owner'} = $temp->Id;
+        }
+    }
+
+    # We special case owner changing, so we can use ForceOwnerChange
+    if ( $ARGSRef->{'Owner'}
+      && $ARGSRef->{'Owner'} !~ /\D/
+      && ( $OrigOwner != $ARGSRef->{'Owner'} ) ) {
+        my ($ChownType);
+        if ( $ARGSRef->{'ForceOwnerChange'} ) {
+            $ChownType = "Force";
+        }
+        else {
+            $ChownType = "Set";
+        }
+
+        my ( $val, $msg ) = $TicketObj->SetOwner( $ARGSRef->{'Owner'}, $ChownType );
+        push( @results, $msg );
+    }
+
+    return (@results);
+}
 
 =head2 ProcessTicketBasics ( TicketObj => $Ticket, ARGSRef => \%ARGS );
 
 Returns an array of results messages.
 
 =cut
 
+# ProcessTicketOwnerUpdate updates Owner only and was created to run
+# earlier in the ticket update process. Keep Owner update code here
+# also for any existing code that might call ProcessTicketBasics.
+#
+# If ProcessTicketOwnerUpdate handles the update first, it should be
+# a noop here.
+
 sub ProcessTicketBasics {
 
     my %args = (

lib/RT/Ticket.pm

@@ -1806,6 +1806,28 @@ sub Atomic {
     my $context = wantarray;
     my @ret;
 
+    my $orig_current_user_has_right = $self->can('CurrentUserHasRight');
+    no warnings 'redefine';
+
+    local *CurrentUserHasRight = $orig_current_user_has_right;
+    if ( $self->{Atomic} == 1 ) {
+        my %granted;
+        for my $right ( keys %{ RT::Queue->AvailableRights } ) {
+            if ( $self->CurrentUserHasRight($right) ) {
+                $granted{$right} = 1;
+            }
+        }
+
+        if (%granted) {
+            *CurrentUserHasRight = sub {
+                my $self  = shift;
+                my $right = shift;
+                return 1 if $granted{$right};
+                return $orig_current_user_has_right->( $self, $right, @_ );
+            };
+        }
+    }
+
     local $@;
     eval {
         if ($context) {

share/html/Helpers/PreviewScrips

@@ -63,6 +63,7 @@ else {
 my @dryrun = $TicketObj->DryRun(
     sub {
         local $ARGS{UpdateContent} ||= "Content";
+        ProcessTicketOwnerUpdate(ARGSRef => \%ARGS, TicketObj => $TicketObj );
         ProcessUpdateMessage(ARGSRef  => \%ARGS, TicketObj => $TicketObj );
         ProcessTicketWatchers(ARGSRef => \%ARGS, TicketObj => $TicketObj );
         ProcessTicketBasics(  ARGSRef => \%ARGS, TicketObj => $TicketObj );

share/html/Helpers/ShowSimplifiedRecipients

@@ -64,6 +64,7 @@ else {
 my @dryrun = $TicketObj->DryRun(
     sub {
         local $ARGS{UpdateContent} ||= "Content";
+        ProcessTicketOwnerUpdate(ARGSRef => \%ARGS, TicketObj => $TicketObj );
         ProcessUpdateMessage(ARGSRef  => \%ARGS, TicketObj => $TicketObj );
         ProcessTicketWatchers(ARGSRef => \%ARGS, TicketObj => $TicketObj );
         ProcessTicketBasics(  ARGSRef => \%ARGS, TicketObj => $TicketObj );

share/html/Helpers/TicketUpdate

@@ -66,6 +66,12 @@ $m->callback(CallbackName => 'ProcessArguments',
              ARGSRef => \%ARGS,
              Actions => \@Actions);
 
+# It's common to change owner and add a reply/comment in the same
+# update. Process the owner change before the message update so the
+# new owner will see the message if they only see notifications when
+# they are the owner.
+push @Actions, ProcessTicketOwnerUpdate(ARGSRef => \%ARGS, TicketObj => $TicketObj );
+
 push @Actions, ProcessUpdateMessage(
     ARGSRef   => \%ARGS,
     Actions   => \@Actions,

share/html/Ticket/Display.html

@@ -186,6 +186,12 @@
                 ARGSRef => \%ARGS,
                 Actions => \@Actions);
 
+        # It's common to change owner and add a reply/comment in the same
+        # update. Process the owner change before the message update so the
+        # new owner will see the message if they only see notifications when
+        # they are the owner.
+        push @Actions, ProcessTicketOwnerUpdate(ARGSRef => \%ARGS, TicketObj => $TicketObj );
+
         push @Actions, ProcessUpdateMessage(
             ARGSRef   => \%ARGS,
             Actions   => \@Actions,

t/web/ticket_owner.t

@@ -509,4 +509,38 @@ diag "user can take/steal ticket with ReassignTicket+OwnTicket right";
     ok !($agent_c->find_all_links( text => 'Steal' ))[0], 'no Steal link';
 }
 
+ok(
+    RT::Test->add_rights(
+        { Principal => 'Owner', Right => [qw(ModifyTicket)] },
+    ),
+    'add ModifyTicket to Owner'
+);
+
+diag "user can update ticket with owner change when rights are granted via Owner";
+{
+    my $ticket = RT::Ticket->new($user_a);
+    my ( $id, $txn, $msg ) = $ticket->Create(
+        Queue   => $queue->id,
+        Subject => 'Test reply with owner change',
+        Owner   => $user_b,
+    );
+    ok( $id, 'created a ticket #' . $id ) or diag "error: $msg";
+    is( $ticket->Owner, $user_b->id, 'correct owner' );
+
+    $agent_b->goto_ticket($id);
+    $agent_b->follow_link_ok( { text => 'Reply' } );
+    $agent_b->submit_form(
+        form_name => 'TicketUpdate',
+        fields    => {
+            Owner         => $user_a->id,
+            UpdateContent => 'Reply content along with owner change',
+        },
+        button => 'SubmitTicket',
+    );
+    $agent_b->text_contains( 'Owner changed from user_b to user_a',   'got owner change message' );
+    $agent_b->text_contains( 'Correspondence added',                  'got correspondence message' );
+    $agent_b->text_contains( 'Reply content along with owner change', 'got correspondence' );
+    ok( !$agent_b->find_link(text => 'Reply'), 'No reply link any more' );
+}
+
 done_testing;