[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