[Rt-commit] rt branch, 4.0/transaction-batch-twice, created. rt-4.0.2-140-g9c73447

Kevin Falcone falcone at bestpractical.com
Thu Oct 13 12:45:29 EDT 2011


The branch, 4.0/transaction-batch-twice has been created
        at  9c7344778946ba83ce4e3b0f8787e955227442d1 (commit)

- Log -----------------------------------------------------------------
commit 1d6bfed3ad9e2e33a5af38e70cc067b169d01500
Author: Kevin Falcone <falcone at bestpractical.com>
Date:   Fri Sep 30 17:23:20 2011 -0400

    TransactionBatch scrips are triggered twice
    
    As a result of 2338cd19ed7a7f4c1e94f639ab2789d6586d01f3
    we now clone the TicketObj and end up with two tickets being DESTROYed
    and firing TransactionBatch.  If your condition is clever, you may not
    have noticed (this is why Approvals weren't affected)
    
    We probably need to fix this clone to skip _TransactionBatch, although
    we aren't sure why we're cloning rather than a simple second load
    anyway.
    
    We also need tests for mail notifications from TransactionBatch
    
    It would also be nice if changes to a ticket on Modify.html that were
    done by a TransactionBatch scrip were displayed in the UI without
    reloading the Basics page. This behavior was changed in
    b5aedb8db1cfcd46162785587002ecf8c27b1335

diff --git a/t/web/transaction_batch.t b/t/web/transaction_batch.t
new file mode 100644
index 0000000..8c032d2
--- /dev/null
+++ b/t/web/transaction_batch.t
@@ -0,0 +1,36 @@
+use strict;
+use warnings;
+use RT;
+use RT::Test tests => 7;
+
+
+my $q = RT::Test->load_or_create_queue ( Name => 'General' );
+
+my $s1 = RT::Scrip->new(RT->SystemUser);
+my ($val, $msg) =$s1->Create( Queue => $q->Id,
+             ScripCondition    => 'User Defined',
+             ScripAction       => 'User Defined',
+             CustomIsApplicableCode => 'return $self->TransactionObj->Field eq "TimeEstimated"',
+             CustomPrepareCode => 'return 1',
+             CustomCommitCode  => '$self->TicketObj->SetPriority($self->TicketObj->Priority + 2); return 1;',
+             Template          => 'Blank',
+             Stage             => 'TransactionBatch',
+    );
+ok($val,$msg);
+
+my $ticket = RT::Ticket->new(RT->SystemUser);
+my ($tv,$ttv,$tm) = $ticket->Create(Queue => $q->Id,
+                                    Subject => "hair on fire",
+                                    );
+ok($tv, $tm);
+
+my ($baseurl, $m) = RT::Test->started_ok;
+$m->login;
+$m->get_ok("$baseurl/Ticket/Modify.html?id=".$ticket->Id);
+    $m->submit_form( form_name => 'TicketModify',
+        fields => { TimeEstimated => 5 }
+    );
+
+
+$ticket->Load($ticket->Id);
+is ($ticket->Priority , '2', "Ticket priority is set right");

commit dee4d21c7bbd8aead13f52000c675907fb261071
Author: Kevin Falcone <falcone at bestpractical.com>
Date:   Tue Oct 11 15:42:01 2011 -0400

    Add a test that confirms that the CurrentUser isn't changed
    
    This avoids the bug which 2338cd19ed7a7f4c1e94f639ab2789d6586d01f3
    was fixing when it introduced the new "run twice" bug.

diff --git a/t/web/transaction_batch.t b/t/web/transaction_batch.t
index 8c032d2..3d1db73 100644
--- a/t/web/transaction_batch.t
+++ b/t/web/transaction_batch.t
@@ -1,7 +1,7 @@
 use strict;
 use warnings;
 use RT;
-use RT::Test tests => 7;
+use RT::Test tests => 11;
 
 
 my $q = RT::Test->load_or_create_queue ( Name => 'General' );
@@ -24,8 +24,23 @@ my ($tv,$ttv,$tm) = $ticket->Create(Queue => $q->Id,
                                     );
 ok($tv, $tm);
 
+my $testuser = RT::Test->load_or_create_user( Name => 'bob', EmailAddress => 'bob at example.com', Password => 'password' );
+ok($testuser->Id, "Created test user bob");
+
+ok( RT::Test->add_rights({ Principal => 'Privileged', Right => [qw(ShowTicket ModifyTicket SeeQueue)]}), 'Granted ticket management rights');
+
+my $test_current_user = RT::CurrentUser->new();
+$test_current_user->LoadByName($testuser->Name);
+my $api_test = RT::Ticket->new($test_current_user);
+$api_test->Load($ticket->Id);
+$api_test->SetTimeEstimated(12);
+$api_test->ApplyTransactionBatch;
+is($api_test->CurrentUser->UserObj->Name, $testuser->Name,"User didn't change running Transaction Batch scrips");
+$api_test->Load($api_test->Id);
+is($api_test->Priority,2,"Ticket priority updated");
+
 my ($baseurl, $m) = RT::Test->started_ok;
-$m->login;
+$m->login('bob','password');
 $m->get_ok("$baseurl/Ticket/Modify.html?id=".$ticket->Id);
     $m->submit_form( form_name => 'TicketModify',
         fields => { TimeEstimated => 5 }
@@ -33,4 +48,4 @@ $m->get_ok("$baseurl/Ticket/Modify.html?id=".$ticket->Id);
 
 
 $ticket->Load($ticket->Id);
-is ($ticket->Priority , '2', "Ticket priority is set right");
+is ($ticket->Priority , 4, "Ticket priority is set right");

commit 9c7344778946ba83ce4e3b0f8787e955227442d1
Author: Kevin Falcone <falcone at bestpractical.com>
Date:   Tue Oct 11 17:10:07 2011 -0400

    Stop TransactionBatch scrips from running twice.
    
    Also prevent the privilege leak originally fixed in
    2338cd19ed7a7f4c1e94f639ab2789d6586d01f3.
    
    Unfortunately, cloning the ticket passed in to ApplyTransactionBatch
    into $scrips->{TicketObj} means that we run TransactionBatch Scrips once
    for the ticket that called ApplyTransactionBatch and once for
    $self->{TicketObj}.
    
    Trying to solve this with a Load that makes $self->{TicketObj}
    completely separate from $args{TicketObj} means that $self->{TicketObj}
    doesn't get the _TransactionBatch cache and when we get into
    RT::Scrip->IsApplicable we won't have the Transactions handy to check
    our Condition against.
    
    Copying the _TransactionBatch cache into $self->{TicketObj} and deleting
    it from $args{TicketObj} results in TransactionBatch scrips running
    twice again because $self->{TicketObj} will be DESTROYed, triggering
    ApplyTransactionBatch.
    
    The only non-bandaid and smallish solution to this is having a flag for
    'run transaction batch' which is separate from the cache of Transactions
    which are batched.
    
    The correct long term solution is most likely a separate table to track
    which transactions are batched together.

diff --git a/lib/RT/Scrips.pm b/lib/RT/Scrips.pm
index 077dbfe..16675dc 100644
--- a/lib/RT/Scrips.pm
+++ b/lib/RT/Scrips.pm
@@ -164,9 +164,20 @@ sub Apply {
                  Type           => undef,
                  @_ );
 
+    # This is a giant bandaid.
+    # RT::Scrips->_SetupSourceObjects will clobber
+    # the CurrentUser, but we need to keep this ticket
+    # so that the _TransactionBatch cache is maintained
+    # and doesn't run twice.  sigh.
+    my $old_current_user = $self->CurrentUser;
+    $old_current_user = $args{TicketObj}->CurrentUser if $args{TicketObj};
+
     $self->Prepare(%args);
     $self->Commit();
 
+    # Apply the bandaid.
+    $args{TicketObj}->CurrentUser($old_current_user) if $args{TicketObj};
+
 }
 
 =head2 Commit
@@ -280,9 +291,13 @@ sub _SetupSourceObjects {
             @_ );
 
 
-    if ( $args{'TicketObj'} ) {
-        # clone the ticket here as we need to change CurrentUser
-        $self->{'TicketObj'} = bless { %{$args{'TicketObj'} } }, 'RT::Ticket';
+    if ( $self->{'TicketObj'} = $args{'TicketObj'} ) {
+        # This clobbers the passed in TicketObj by turning it into one
+        # whose current user is RT_System.  Anywhere in the Web UI
+        # currently calling into this is thus susceptable to a privilege
+        # leak; the only current call site is ->Apply, which bandaids
+        # over the top of this by re-asserting the CurrentUser
+        # afterwards.
         $self->{'TicketObj'}->CurrentUser( $self->CurrentUser );
     }
     else {

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


More information about the Rt-commit mailing list