diff --git a/docs/UPGRADING-5.0 b/docs/UPGRADING-5.0 index d12cb26e195..a0bb52dba34 100644 --- a/docs/UPGRADING-5.0 +++ b/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 diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm index b727dbb606c..e47fa96069d 100644 --- a/lib/RT/Interface/Web.pm +++ b/lib/RT/Interface/Web.pm @@ -3171,7 +3171,56 @@ 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 ); @@ -3179,6 +3228,13 @@ 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 = ( diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm index 933d315f7ac..b204a815d1b 100644 --- a/lib/RT/Ticket.pm +++ b/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) { diff --git a/share/html/Helpers/PreviewScrips b/share/html/Helpers/PreviewScrips index 9a294027b56..b2b7a42b7c4 100644 --- a/share/html/Helpers/PreviewScrips +++ b/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 ); diff --git a/share/html/Helpers/ShowSimplifiedRecipients b/share/html/Helpers/ShowSimplifiedRecipients index aef51fc34c5..a8b0c0bc9da 100644 --- a/share/html/Helpers/ShowSimplifiedRecipients +++ b/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 ); diff --git a/share/html/Helpers/TicketUpdate b/share/html/Helpers/TicketUpdate index 2e56c7e70dd..23d5e35250e 100644 --- a/share/html/Helpers/TicketUpdate +++ b/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, diff --git a/share/html/Ticket/Display.html b/share/html/Ticket/Display.html index c08d37b90a3..f831d98305e 100644 --- a/share/html/Ticket/Display.html +++ b/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, diff --git a/t/web/ticket_owner.t b/t/web/ticket_owner.t index e7b7f1a8064..6c7afd531f8 100644 --- a/t/web/ticket_owner.t +++ b/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;