[Rt-commit] rt branch, 4.2/extensible-roles, updated. rt-4.0.8-596-g1e7babb

Alex Vandiver alexmv at bestpractical.com
Mon Nov 26 18:36:29 EST 2012


The branch, 4.2/extensible-roles has been updated
       via  1e7babb1f201b5dbcd1f0f574f6e49c8e4f95026 (commit)
       via  b1e35a8c0d47eee4303fa2ee83a07746f70d2f77 (commit)
       via  b8b674f4c23c42edbea933881c951ad78f0a4d8e (commit)
      from  deadc8fb542931928785be90206c12d1588bfb29 (commit)

Summary of changes:
 lib/RT/Ticket.pm | 124 ++++++++++++++++++++-----------------------------------
 1 file changed, 45 insertions(+), 79 deletions(-)

- Log -----------------------------------------------------------------
commit b8b674f4c23c42edbea933881c951ad78f0a4d8e
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Mon Nov 26 17:35:04 2012 -0500

    Refactor DeferOwner logic
    
    The original phrasing confusingly calls ->OwnerGroup->_AddMember even if
    the user does not have OwnTicket.  However, $Owner is still set to
    Nobody in this case, which is a no-op, as _AddMember prevents
    duplicates.  Clean up the logic to make this path clearer.

diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 6c102fd..38878ba 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -615,27 +615,28 @@ sub Create {
     }
 
     # }}}
-    # Now that we've created the ticket and set up its metadata, we can actually go and check OwnTicket on the ticket itself. 
-    # This might be different than before in cases where extensions like RTIR are doing clever things with RT's ACL system
-    if (  $DeferOwner ) { 
-            if (!$DeferOwner->HasRight( Object => $self, Right  => 'OwnTicket')) {
-    
-            $RT::Logger->warning( "User " . $DeferOwner->Name . "(" . $DeferOwner->id 
+
+
+    # Now that we've created the ticket and set up its metadata, we can
+    # actually go and check OwnTicket on the ticket itself.  This might
+    # be different than before in cases where extensions like RTIR are
+    # doing clever things with RT's ACL system.
+    if ($DeferOwner) {
+        if ( $DeferOwner->HasRight( Object => $self, Right => 'OwnTicket' ) ) {
+            $self->__Set( Field => 'Owner', Value => $DeferOwner->id );
+            $self->OwnerGroup->_AddMember(
+                PrincipalId       => $DeferOwner->PrincipalId,
+                InsideTransaction => 1
+            );
+        } else {
+            $RT::Logger->warning( "User " . $DeferOwner->Name . "(" . $DeferOwner->id
                 . ") was proposed as a ticket owner but has no rights to own "
                 . "tickets in " . $QueueObj->Name );
             push @non_fatal_errors, $self->loc(
                 "Owner '[_1]' does not have rights to own this ticket.",
                 $DeferOwner->Name
             );
-        } else {
-            $Owner = $DeferOwner;
-            $self->__Set(Field => 'Owner', Value => $Owner->id);
-
         }
-        $self->OwnerGroup->_AddMember(
-            PrincipalId       => $Owner->PrincipalId,
-            InsideTransaction => 1
-        );
     }
 
     if ( $args{'_RecordTransaction'} ) {

commit b1e35a8c0d47eee4303fa2ee83a07746f70d2f77
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Mon Nov 26 17:38:10 2012 -0500

    Stop splitting SetOwner into _Set and _NewTransaction
    
    This split, along with the comment that goes with it, originated in
    4137c44, where the ->Commit statement occurred between two calls to
    ->_Set, one which changed the record, the other which added a
    Transaction.  This was due to scrips, at the time, needing to be
    triggered outside of a database transaction, due to the lack of nested
    transactions.
    
    Long after this restriction had been lifted, 6061508 moved ->Commit to
    the more sensible place at the end of the updates, but left the two
    calls to _Set (one of which had changed into _NewTransaction in e04e4a0)
    in place, along with the misleading comment.
    
    Replace both calls with one straightforward call to ->_Set.

diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 38878ba..aa9eac5 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -2811,16 +2811,10 @@ sub SetOwner {
         return ( 0, $self->loc("Could not change owner: [_1]", $add_msg ) );
     }
 
-    # We call set twice with slightly different arguments, so
-    # as to not have an SQL transaction span two RT transactions
-
     ( $val, $msg ) = $self->_Set(
-                      Field             => 'Owner',
-                      RecordTransaction => 0,
-                      Value             => $NewOwnerObj->Id,
-                      TimeTaken         => 0,
-                      TransactionType   => 'Set',
-                      CheckACL          => 0,                  # don't check acl
+        Field    => 'Owner',
+        Value    => $NewOwnerObj->Id,
+        CheckACL => 0,                  # don't check acl
     );
 
     unless ($val) {
@@ -2828,22 +2822,8 @@ sub SetOwner {
         return ( 0, $self->loc("Could not change owner: [_1]", $msg) );
     }
 
-    ($val, $msg) = $self->_NewTransaction(
-        Type      => 'Set',
-        Field     => 'Owner',
-        NewValue  => $NewOwnerObj->Id,
-        OldValue  => $OldOwnerObj->Id,
-        TimeTaken => 0,
-    );
-
-    if ( $val ) {
-        $msg = $self->loc( "Owner changed from [_1] to [_2]",
-                           $OldOwnerObj->Name, $NewOwnerObj->Name );
-    }
-    else {
-        $RT::Handle->Rollback();
-        return ( 0, $msg );
-    }
+    $msg = $self->loc( "Owner changed from [_1] to [_2]",
+                       $OldOwnerObj->Name, $NewOwnerObj->Name );
 
     $RT::Handle->Commit();
 

commit 1e7babb1f201b5dbcd1f0f574f6e49c8e4f95026
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Mon Nov 26 18:32:03 2012 -0500

    Remove UpdateTicket argument to _Set, simplifying logic
    
    The last existing call site which used UpdateTicket => 0 was removed in
    e04e4a0 -- which was the call that UpdateTicket's functionality was
    originally added for in 4137c44.
    
    Remove the argument, and simplify the logic of _Set considerably.  The
    equivalent functionality can be accomplished via _NewTransaction, which
    is what other codepaths do when similar effects are desired.

diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index aa9eac5..fc3f3da 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -3309,57 +3309,42 @@ sub _Set {
                  Value             => undef,
                  TimeTaken         => 0,
                  RecordTransaction => 1,
-                 UpdateTicket      => 1,
                  CheckACL          => 1,
                  TransactionType   => 'Set',
                  @_ );
 
     if ($args{'CheckACL'}) {
-      unless ( $self->CurrentUserHasRight('ModifyTicket')) {
-          return ( 0, $self->loc("Permission Denied"));
-      }
-   }
-
-    unless ($args{'UpdateTicket'} || $args{'RecordTransaction'}) {
-        $RT::Logger->error("Ticket->_Set called without a mandate to record an update or update the ticket");
-        return(0, $self->loc("Internal Error"));
+        unless ( $self->CurrentUserHasRight('ModifyTicket')) {
+            return ( 0, $self->loc("Permission Denied"));
+        }
     }
 
-    #if the user is trying to modify the record
+    # Avoid ACL loops using _Value
+    my $Old = $self->SUPER::_Value($args{'Field'});
 
-    #Take care of the old value we really don't want to get in an ACL loop.
-    # so ask the super::_Value
-    my $Old = $self->SUPER::_Value("$args{'Field'}");
-    
-    my ($ret, $msg);
-    if ( $args{'UpdateTicket'}  ) {
+    # Set the new value
+    my ( $ret, $msg ) = $self->SUPER::_Set(
+        Field => $args{'Field'},
+        Value => $args{'Value'}
+    );
+    return ( 0, $msg ) unless $ret;
 
-        #Set the new value
-        ( $ret, $msg ) = $self->SUPER::_Set( Field => $args{'Field'},
-                                                Value => $args{'Value'} );
-    
-        #If we can't actually set the field to the value, don't record
-        # a transaction. instead, get out of here.
-        return ( 0, $msg ) unless $ret;
-    }
+    return ( $ret, $msg ) unless $args{'RecordTransaction'};
 
-    if ( $args{'RecordTransaction'} == 1 ) {
+    my $trans;
+    ( $ret, $msg, $trans ) = $self->_NewTransaction(
+        Type      => $args{'TransactionType'},
+        Field     => $args{'Field'},
+        NewValue  => $args{'Value'},
+        OldValue  => $Old,
+        TimeTaken => $args{'TimeTaken'},
+    );
 
-        my ( $Trans, $Msg, $TransObj ) = $self->_NewTransaction(
-                                               Type => $args{'TransactionType'},
-                                               Field     => $args{'Field'},
-                                               NewValue  => $args{'Value'},
-                                               OldValue  => $Old,
-                                               TimeTaken => $args{'TimeTaken'},
-        );
-        # Ensure that we can read the transaction, even if the change
-        # just made the ticket unreadable to us
-        $TransObj->{ _object_is_readable } = 1;
-        return ( $Trans, scalar $TransObj->BriefDescription );
-    }
-    else {
-        return ( $ret, $msg );
-    }
+    # Ensure that we can read the transaction, even if the change
+    # just made the ticket unreadable to us
+    $trans->{ _object_is_readable } = 1;
+
+    return ( $ret, scalar $trans->BriefDescription );
 }
 
 

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


More information about the Rt-commit mailing list