[Rt-commit] rt branch, 4.2/watcher-transactions, created. rt-4.1.6-384-g0a1a8c1
Thomas Sibley
trs at bestpractical.com
Fri Mar 29 18:45:12 EDT 2013
The branch, 4.2/watcher-transactions has been created
at 0a1a8c1409bcf4158749000863c90218d91b7a1a (commit)
- Log -----------------------------------------------------------------
commit 0a1a8c1409bcf4158749000863c90218d91b7a1a
Author: Thomas Sibley <trs at bestpractical.com>
Date: Thu Mar 28 15:30:53 2013 -0700
Consistent transactions for all role group membership changes
A new transaction type, SetWatcher, is now created by single member role
groups when the assigned member changes. SetWatcher is more useful than
a pair of Add/DelWatcher transactions since the old and new members are
on the same transaction object. Such role groups which are also
denormalized into a column still record a Set <Column> transaction on
the record (i.e. tickets record a SetWatcher Owner and Set Owner
transaction for each Owner change). While redundant on the surface, the
SetWatcher and Set transactions are distinct because they mark,
respectively, the group membership change and the record column change.
Add/DelWatcher is still used for all other role groups but is more
consistently set from deeper in the API: RT::Group->_Add/DeleteMember
creates the transactions instead of Add/DeleteRoleMember in the Roles
role. Aside from greater consistency no matter which API method is
called, the move keeps all transaction creation alongside that of
SetWatcher and Set which must be in RT::Group.
Display of SetWatcher transactions is supported by core RT but not
currently used. Without backdating new transactions to fill in existing
ticket history, it's impossible to solely use SetWatcher for ticket
owner history display. Instead, displaying SetWatcher is skipped when
it applies to the Owner role on a ticket. All other objects and roles,
such as those provided by extensions, will display SetWatcher
transactions.
diff --git a/lib/RT/Condition/OwnerChange.pm b/lib/RT/Condition/OwnerChange.pm
index 707eb0d..4d39974 100644
--- a/lib/RT/Condition/OwnerChange.pm
+++ b/lib/RT/Condition/OwnerChange.pm
@@ -62,7 +62,9 @@ If we're changing the owner return true, otherwise return false
sub IsApplicable {
my $self = shift;
- if ( ( $self->TransactionObj->Field || '' ) eq 'Owner' ) {
+ if ( ( $self->TransactionObj->Field || '' ) eq 'Owner'
+ and ($self->TransactionObj->ObjectType ne "RT::Ticket"
+ or $self->TransactionObj->Type ne "SetWatcher" )) {
return(1);
}
else {
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 4b6217b..0d95ff6 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -1053,6 +1053,11 @@ sub _AddMember {
InsideTransaction => undef,
RecordTransaction => 1,
@_);
+
+ # RecordSetTransaction is used by _DeleteMember to get one txn but not the other
+ $args{RecordSetTransaction} = $args{RecordTransaction}
+ unless exists $args{RecordSetTransaction};
+
my $new_member = $args{'PrincipalId'};
unless ($self->Id) {
@@ -1100,11 +1105,18 @@ sub _AddMember {
return(0, $self->loc("Couldn't add member to group"))
unless $id;
- # Purge all previous members
+ # Purge all previous members (we're a single member role group)
+ my $old_member_id;
for my $member (@purge) {
+ my $old_member = $member->MemberId;
my ($ok, $msg) = $member->Delete();
return(0, $self->loc("Couldn't remove previous member: [_1]", $msg))
unless $ok;
+
+ # We remove all members in this loop, but there should only ever be one
+ # member. Keep track of the last one successfully removed for the
+ # SetWatcher transaction below.
+ $old_member_id = $old_member;
}
# Update the column
@@ -1114,12 +1126,32 @@ sub _AddMember {
Field => $col,
Value => $new_member_obj->Id,
CheckACL => 0, # don't check acl
- RecordTransaction => $args{'RecordTransaction'},
+ RecordTransaction => $args{'RecordSetTransaction'},
);
return (0, $self->loc("Could not update column [_1]: [_2]", $col, $msg))
unless $ok;
}
+ # Record an Add/SetWatcher txn on the object if we're a role group
+ if ($args{RecordTransaction} and $self->RoleClass) {
+ my $obj = $args{Object} || $self->RoleGroupObject;
+
+ if ($self->SingleMemberRoleGroup) {
+ $obj->_NewTransaction(
+ Type => 'SetWatcher',
+ OldValue => $old_member_id,
+ NewValue => $new_member_obj->Id,
+ Field => $self->Type,
+ );
+ } else {
+ $obj->_NewTransaction(
+ Type => 'AddWatcher', # use "watcher" for history's sake
+ NewValue => $new_member_obj->Id,
+ Field => $self->Type,
+ );
+ }
+ }
+
return ( 1, $self->loc("Member added: [_1]", $new_member_obj->Object->Name) );
}
@@ -1216,6 +1248,8 @@ removes that GroupMember from this group.
Returns a two value array. the first value is true on successful
addition or 0 on failure. The second value is a textual status msg.
+Optionally takes a hash of key value flags, such as RecordTransaction.
+
=cut
sub DeleteMember {
@@ -1233,7 +1267,7 @@ sub DeleteMember {
#User has no permission to be doing this
return ( 0, $self->loc("Permission Denied") );
}
- $self->_DeleteMember($member_id);
+ $self->_DeleteMember($member_id, @_);
}
# A helper subroutine for DeleteMember that bypasses the ACL checks
@@ -1245,6 +1279,11 @@ sub DeleteMember {
sub _DeleteMember {
my $self = shift;
my $member_id = shift;
+ my %args = (
+ RecordTransaction => 1,
+ @_,
+ );
+
my $member_obj = RT::GroupMember->new( $self->CurrentUser );
@@ -1258,6 +1297,8 @@ sub _DeleteMember {
return ( 0,$self->loc( "Group has no such member" ));
}
+ my $old_member = $member_obj->MemberId;
+
#Now that we've checked ACLs and sanity, delete the groupmember
my $val = $member_obj->Delete();
@@ -1266,10 +1307,31 @@ sub _DeleteMember {
return ( 0, $self->loc("Member not deleted" ));
}
- $self->_AddMember(
- PrincipalId => RT->Nobody->Id,
- RecordTransaction => 0,
- ) if $self->SingleMemberRoleGroup;
+ if ($self->RoleClass) {
+ my %txn = (
+ OldValue => $old_member,
+ Field => $self->Type,
+ );
+
+ if ($self->SingleMemberRoleGroup) {
+ # _AddMember creates the Set-Owner txn (for example) but we handle
+ # the SetWatcher-Owner txn below.
+ $self->_AddMember(
+ PrincipalId => RT->Nobody->Id,
+ RecordTransaction => 0,
+ RecordSetTransaction => $args{RecordTransaction},
+ );
+ $txn{Type} = "SetWatcher";
+ $txn{NewValue} = RT->Nobody->id;
+ } else {
+ $txn{Type} = "DelWatcher";
+ }
+
+ if ($args{RecordTransaction}) {
+ my $obj = $args{Object} || $self->RoleGroupObject;
+ $obj->_NewTransaction(%txn);
+ }
+ }
return ( $val, $self->loc("Member deleted") );
}
diff --git a/lib/RT/Record/Role/Roles.pm b/lib/RT/Record/Role/Roles.pm
index f8aca0b..2d2c8e9 100644
--- a/lib/RT/Record/Role/Roles.pm
+++ b/lib/RT/Record/Role/Roles.pm
@@ -415,7 +415,7 @@ sub AddRoleMember {
$principal->Object->Name, $self->loc($type)) )
if $group->HasMember( $principal );
- my ( $ok, $msg ) = $group->_AddMember( %args );
+ my ( $ok, $msg ) = $group->_AddMember( %args, RecordTransaction => !$args{Silent} );
unless ($ok) {
$RT::Logger->error("Failed to add $args{PrincipalId} as a member of group ".$group->Id.": ".$msg);
@@ -423,14 +423,6 @@ sub AddRoleMember {
$principal->Object->Name, $self->loc($type)) );
}
- unless ($args{Silent}) {
- $self->_NewTransaction(
- Type => 'AddWatcher', # use "watcher" for history's sake
- NewValue => $args{PrincipalId},
- Field => $type,
- );
- }
-
return ($principal, $msg);
}
@@ -498,7 +490,7 @@ sub DeleteRoleMember {
$principal->Object->Name, $self->loc($args{Type}) ) )
unless $group->HasMember($principal);
- my ($ok, $msg) = $group->_DeleteMember($args{PrincipalId});
+ my ($ok, $msg) = $group->_DeleteMember($args{PrincipalId}, RecordTransaction => !$args{Silent});
unless ($ok) {
$RT::Logger->error("Failed to remove $args{PrincipalId} as a member of group ".$group->Id.": ".$msg);
@@ -506,13 +498,6 @@ sub DeleteRoleMember {
$principal->Object->Name, $self->loc($args{Type})) );
}
- unless ($args{Silent}) {
- $self->_NewTransaction(
- Type => 'DelWatcher', # use "watcher" for history's sake
- OldValue => $args{PrincipalId},
- Field => $args{Type},
- );
- }
return ($principal, $msg);
}
diff --git a/lib/RT/Transaction.pm b/lib/RT/Transaction.pm
index 13635ec..c4356b3 100644
--- a/lib/RT/Transaction.pm
+++ b/lib/RT/Transaction.pm
@@ -835,6 +835,12 @@ sub _FormatUser {
$principal->Load($self->OldValue);
return ( "[_1] [_2] deleted", $self->loc($self->Field), $self->_FormatUser($principal->Object)); #loc
},
+ SetWatcher => sub {
+ my $self = shift;
+ my $principal = RT::Principal->new($self->CurrentUser);
+ $principal->Load($self->NewValue);
+ return ( "[_1] set to [_2]", $self->loc($self->Field), $self->_FormatUser($principal->Object)); #loc
+ },
Subject => sub {
my $self = shift;
return ( "Subject changed to [_1]", $self->Data ); #loc
diff --git a/share/html/Elements/ShowHistory b/share/html/Elements/ShowHistory
index 7659b9e..f876124 100644
--- a/share/html/Elements/ShowHistory
+++ b/share/html/Elements/ShowHistory
@@ -82,6 +82,30 @@ if ( $ShowDisplayModes or $ShowTitle ) {
my $i = 1;
while ( my $Transaction = $Transactions->Next ) {
my $skip = 0;
+
+ # Skip display of SetWatcher transactions for ticket Owner groups. Owner
+ # was a single member role group and denormalized into a column well before
+ # the generic role group handling and transactions came about. For
+ # tickets, we rely on rendering ownership changes using the Set-Owner
+ # transaction. For all other record types, or even potential ticket single
+ # role groups which aren't Owner, we use SetWatcher to render history and
+ # skip the Set transactions. This complication is necessary to avoid
+ # creating backdated transactions on upgrade which normalize to one type or
+ # another.
+ #
+ # These conditions assumes ticket Owner is a single-member denormalized
+ # role group, which is safe since that is unlikely to ever change in the
+ # future.
+ if ($Object->isa("RT::Ticket") and ($Transaction->Field || '') eq "Owner") {
+ $skip = 1 if $Transaction->Type eq "SetWatcher";
+ } else {
+ $skip = 1 if $Transaction->Type eq "Set"
+ and $Transaction->Field
+ and $Object->DOES("RT::Record::Role::Roles")
+ and $Object->HasRole( $Transaction->Field )
+ and $Object->RoleGroup( $Transaction->Field )->SingleMemberRoleGroupColumn;
+ }
+
$m->callback(
%ARGS,
Transaction => $Transaction,
diff --git a/t/mail/gateway.t b/t/mail/gateway.t
index 9f1d288..eb0283d 100644
--- a/t/mail/gateway.t
+++ b/t/mail/gateway.t
@@ -646,8 +646,8 @@ $tick->Load( $id );
is( $tick->Id, $id, 'load correct ticket');
is( $tick->OwnerObj->EmailAddress, 'root at localhost', 'successfuly take ticket via email');
-# check that there is no text transactions writen
-is( $tick->Transactions->Count, 2, 'no superfluous transactions');
+# check that there is no text transactions writen (create + 2 for take)
+is( $tick->Transactions->Count, 3, 'no superfluous transactions');
my $status;
($status, $msg) = $tick->SetOwner( RT->Nobody->Id, 'Force' );
@@ -677,8 +677,8 @@ is( $tick->OwnerObj->EmailAddress, 'root at localhost', 'successfuly take ticket vi
my $txns = $tick->Transactions;
$txns->Limit( FIELD => 'Type', VALUE => 'Correspond');
$txns->OrderBy( FIELD => 'id', ORDER => 'DESC' );
-# +1 because of auto open
-is( $tick->Transactions->Count, 6, 'no superfluous transactions');
+# +2 from owner to nobody, +1 because of auto open, +2 from take, +1 from correspond
+is( $tick->Transactions->Count, 9, 'no superfluous transactions');
is( $txns->First->Subject, "[$RT::rtname \#$id] correspondence", 'successfuly add correspond within take via email' );
$m->no_warnings_ok;
@@ -700,7 +700,8 @@ $tick = RT::Ticket->new(RT->SystemUser);
$tick->Load( $id );
is( $tick->Id, $id, 'load correct ticket');
is( $tick->Status, 'resolved', 'successfuly resolved ticket via email');
-is( $tick->Transactions->Count, 7, 'no superfluous transactions');
+# +1 from resolve
+is( $tick->Transactions->Count, 10, 'no superfluous transactions');
use RT::User;
my $user = RT::User->new( RT->SystemUser );
@@ -821,7 +822,8 @@ DBIx::SearchBuilder::Record::Cachable->FlushCache;
$tick->Load( $id );
is( $tick->Owner, $user->id, "we changed owner" );
ok( $user->HasRight( Right => 'ReplyToTicket', Object => $tick ), "owner can reply to ticket" );
-is( $tick->Transactions->Count, 5, "transactions added" );
+# +2 from take, +1 from correspond
+is( $tick->Transactions->Count, 6, "transactions added" );
$m->no_warnings_ok;
-----------------------------------------------------------------------
More information about the Rt-commit
mailing list