[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