[Rt-commit] rt branch, 4.2/fix-can-set-owner-logic-for-take-and-steal, created. rt-4.2.1-69-gb314742

? sunnavy sunnavy at bestpractical.com
Tue Dec 31 00:26:51 EST 2013


The branch, 4.2/fix-can-set-owner-logic-for-take-and-steal has been created
        at  b314742a36bc04b92a87d68b97fcea4f14640b42 (commit)

- Log -----------------------------------------------------------------
commit b314742a36bc04b92a87d68b97fcea4f14640b42
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Tue Dec 31 12:54:44 2013 +0800

    fix the logic of RT::Ticket::CurrentUserCanSetOwner for Take/Steal types
    
    previously, as long as user has ReassignTicket, RT::Ticket::CurrentUserCanSetOwner
    would return true for both "Take" and "Steal" types no matter what the ticket's
    current owner is, which is not good, as we want to use it like(in /Elements/Tabs):
    
        my ($can_take, $msg) = $ticket->CurrentUserCanSetOwner( Type => 'Take' );
        if ( $can_take ) {
            # show "Take" link
        }
    
    this commit merges the logic of "check ticket's current owner too" for Take/Steal
    into RT::Ticket::CurrentUserCanSetOwner.
    
    see also #28211

diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 80af7f5..b41b964 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -2079,6 +2079,10 @@ Owner lock held by another user (see below) and can be a convenient
 right for managers or administrators who need to assign tickets
 without necessarily owning them.
 
+However, if you are checking if current user can "Take" or "Steal"
+the ticket, it will also check the ticket's current owner to determine
+if "Take" or "Steal" action could be operated.
+
 =item *
 
 ModifyTicket grants the right to set the owner to any user who
@@ -2145,8 +2149,10 @@ sub CurrentUserCanSetOwner {
         return ($ok, $message) if not $ok;
     }
 
-    # ReassignTicket unconditionally allows you to SetOwner
-    return (1, undef) if $self->CurrentUserHasRight('ReassignTicket');
+    # ReassignTicket allows you to SetOwner, but we also need to check ticket's
+    # current owner for Take and Steal Types
+    return ( 1, undef ) if $self->CurrentUserHasRight('ReassignTicket')
+        && $args{Type} ne 'Take' && $args{Type} ne 'Steal';
 
     # Ticket is unowned
     # Can set owner to yourself withn ModifyTicket or TakeTicket
@@ -2159,6 +2165,7 @@ sub CurrentUserCanSetOwner {
         }
 
         unless ( (  $self->CurrentUserHasRight('ModifyTicket')
+                 or $self->CurrentUserHasRight('ReassignTicket')
                  or $self->CurrentUserHasRight('TakeTicket') )
                  and $self->CurrentUserHasRight('OwnTicket') ) {
             return ( 0, $self->loc("Permission Denied") );
@@ -2172,6 +2179,7 @@ sub CurrentUserCanSetOwner {
             && $OldOwnerObj->Id != $self->CurrentUser->id ) {
 
         unless (    $self->CurrentUserHasRight('ModifyTicket')
+                 || $self->CurrentUserHasRight('ReassignTicket')
                  || $self->CurrentUserHasRight('StealTicket') ) {
             return ( 0, $self->loc("Permission Denied") )
         }
@@ -2187,7 +2195,8 @@ sub CurrentUserCanSetOwner {
                   and $args{'NewOwnerObj'}->id == $self->CurrentUser->id )) {
             return ( 0, $self->loc("You can only take tickets that are unowned") );
         }
-        else {
+
+        unless ( $self->CurrentUserHasRight('ReassignTicket') )  {
             return ( 0, $self->loc( "You can only reassign tickets that you own or that are unowned"));
         }
 
@@ -2195,7 +2204,12 @@ sub CurrentUserCanSetOwner {
     # You own the ticket
     # Untake falls through to here, so we don't need to explicitly handle that Type
     else {
-        unless ( $self->CurrentUserHasRight('ModifyTicket') ) {
+        if ( $args{'Type'} eq 'Take' || $args{'Type'} eq 'Steal' ) {
+            return ( 0, $self->loc("You already own this ticket") );
+        }
+
+        unless ( $self->CurrentUserHasRight('ModifyTicket')
+            || $self->CurrentUserHasRight('ReassignTicket') ) {
             return ( 0, $self->loc("Permission Denied") );
         }
     }
diff --git a/t/web/ticket_owner.t b/t/web/ticket_owner.t
index 37716ea..782e68f 100644
--- a/t/web/ticket_owner.t
+++ b/t/web/ticket_owner.t
@@ -440,6 +440,10 @@ diag "user can assign ticket to new owner with ReassignTicket right";
     ok $id, 'created a ticket #' . $id or diag "error: $msg";
     is $ticket->Owner, RT->Nobody->id, 'correct owner';
 
+    $agent_c->goto_ticket($id);
+    ok !($agent_c->find_all_links( text => 'Take' ))[0], 'no Take link';
+    ok !($agent_c->find_all_links( text => 'Steal' ))[0], 'no Steal link';
+
     $agent_a->goto_ticket($id);
     $agent_a->content_lacks('Taken', 'no Taken');
     $agent_a->follow_link_ok( { text => 'Basics' }, 'Ticket -> Basics' );
@@ -454,6 +458,8 @@ diag "user can assign ticket to new owner with ReassignTicket right";
          qr{user_a\s*-\s*Taken}, 'got user_a Taken message' );
 
     $agent_c->goto_ticket($id);
+    ok !($agent_c->find_all_links( text => 'Take' ))[0], 'no Take link';
+    ok !($agent_c->find_all_links( text => 'Steal' ))[0], 'no Steal link';
     $agent_c->follow_link_ok( { text => 'Basics' }, 'Ticket -> Basics' );
     my $form = $agent_c->form_name('TicketModify');
     is $form->value('Owner'), $user_a->id, 'correct owner selected';
@@ -466,7 +472,49 @@ diag "user can assign ticket to new owner with ReassignTicket right";
         'got set message in Basics' );
     $agent_c->goto_ticket($id);
     $agent_c->content_like( qr{Owner forcibly changed}, 'got owner forcibly changed message' );
+    ok !($agent_c->find_all_links( text => 'Take' ))[0], 'no Take link';
+}
+
+ok(
+    RT::Test->add_rights(
+        { Principal => $user_c, Right => [qw(OwnTicket)] },
+    ),
+    'add rights'
+);
+diag "user can take/steal ticket with ReassignTicket+OwnTicket right";
+{
+    my $ticket = RT::Ticket->new($user_a);
+    my ( $id, $txn, $msg ) = $ticket->Create(
+        Queue   => $queue->id,
+        Subject => 'test',
+    );
+    ok $id, 'created a ticket #' . $id or diag "error: $msg";
+    is $ticket->Owner, RT->Nobody->id, 'correct owner';
 
+    $agent_c->goto_ticket($id);
+    ok( ($agent_c->find_all_links( text => 'Take' ))[0], 'has Take link' );
+    ok !($agent_c->find_all_links( text => 'Steal' ))[0], 'no Steal link';
+
+    $agent_a->goto_ticket($id);
+    $agent_a->content_lacks('Taken', 'no Taken');
+    $agent_a->follow_link_ok( { text => 'Basics' }, 'Ticket -> Basics' );
+    $agent_a->submit_form(
+        form_name => 'TicketModify',
+        fields    => { Owner => $user_a->id },
+    );
+    $agent_a->content_contains( 'Owner changed from Nobody to user_a',
+        'got set message in Basics' );
+    $agent_a->goto_ticket($id);
+    like($agent_a->dom->at('.transaction.people .description')->all_text,
+         qr{user_a\s*-\s*Taken}, 'got user_a Taken message' );
+
+    $agent_c->goto_ticket($id);
+    ok !($agent_c->find_all_links( text => 'Take' ))[0], 'no Take link';
+    ok( ($agent_c->find_all_links( text => 'Steal' ))[0], 'has Steal link' );
+    $agent_c->follow_link_ok( { text => 'Steal' }, 'Ticket -> Steal' );
+    $agent_c->content_contains( 'Owner changed from user_a to user_c', 'steal message' );
+    ok !($agent_c->find_all_links( text => 'Take' ))[0], 'no Take link';
+    ok !($agent_c->find_all_links( text => 'Steal' ))[0], 'no Steal link';
 }
 
 

-----------------------------------------------------------------------


More information about the rt-commit mailing list