[Rt-devel] PATCH: Link symmetry

[Jesse Vincent]

I've dropped my comments inline.

Jesse


> Index: lib/RT/Ticket_Overlay.pm
> ===================================================================
> --- lib/RT/Ticket_Overlay.pm	(revision 4013)
> +++ lib/RT/Ticket_Overlay.pm	(working copy)
> @@ -2501,8 +2501,8 @@
>          $direction='Base';
>      }
>  
> -    if ( $val ) {
> -	my $remote_uri = RT::URI->new( $RT::SystemUser );
> +    if ( ! $args{'Silent'} ) {

(I'm working backwards and said this below, but I'd make this a positive
test case rather than a negative one)

> +	my $remote_uri = RT::URI->new( $self->CurrentUser );
>      	$remote_uri->FromURI( $remote_link );
>  
>          my ( $Trans, $Msg, $TransObj ) = $self->_NewTransaction(
> @@ -2512,8 +2512,22 @@
>              TimeTaken => 0
>          );
>  
> +        if ( $remote_uri->IsLocal ) {
> +
> +            my $OtherObj = $remote_uri->Object;
> +            my ( $val, $Msg ) = $OtherObj->_NewTransaction(Type  => 'DeleteLink',
> +                                                           Field => $direction eq 'Target' ? $LINKDIRMAP{$args{'Type'}}->{Base}
> +                                                                                           : $LINKDIRMAP{$args{'Type'}}->{Target},
> +                                                           OldValue => $self->URI,
> +                                                           ActtivateScrips => $RT::LinkTransactionsRun2Scrips,
> +                                                           TimeTaken => 0 );
> +        }
> +
>          return ( $Trans, $Msg );
>      }
> +    else {
> +        return ( $val, $Msg );
> +    }
>  }
>  
>  # }}}
> @@ -2568,6 +2582,20 @@
>  ($id,$msg) =$ticket->AddLink(Type => 'RefersTo', Target => -1);
>  ok(!$id,$msg);

Can you throw the new tests into a test script, rather than inline
testing? I'm trying to get us away from the inline stuff, which has
turned out to be more trouble than its worth and is harder to run while
testing individual features.

> +my $transactions = $ticket2->Transactions;
> +$transactions->Limit( FIELD => 'Type', VALUE => 'AddLink' );
> +ok( $transactions->Count == 1, "Transaction found in other ticket" );
> +ok( $transactions->First->Field eq 'ReferredToBy');
> +ok( $transactions->First->NewValue eq $ticket->URI );
> +
> +($id,$msg) =$ticket->DeleteLink(Type => 'RefersTo', Target => $ticket2->id);
> +ok($id,$msg);
> +$transactions = $ticket2->Transactions;
> +$transactions->Limit( FIELD => 'Type', VALUE => 'DeleteLink' );
> +ok( $transactions->Count == 1, "Transaction found in other ticket" );
> +ok( $transactions->First->Field eq 'ReferredToBy');
> +ok( $transactions->First->OldValue eq $ticket->URI );
> +
>  =end testing 
>  
>  =cut
> @@ -2653,11 +2681,8 @@
>      }
>  
>      # Don't write the transaction if we're doing this on create
> -    if ( $args{'Silent'} ) {
> -        return ( $val, $Msg );
> -    }
> -    else {
> -	my $remote_uri = RT::URI->new( $RT::SystemUser );
> +    if ( ! $args{'Silent'} ) {
> +	my $remote_uri = RT::URI->new( $self->CurrentUser );

This logic inversion doesn't make sense to me. Negative logic is
somewhat harder to read. And yeah, getting out earlier in the case of a
single conditional maes it easier to read the code, I think.

>      	$remote_uri->FromURI( $remote_link );
>  
>          #Write the transaction
> @@ -2666,8 +2691,22 @@
>  				   Field => $LINKDIRMAP{$args{'Type'}}->{$direction},
>  				   NewValue =>  $remote_uri->URI || $remote_link,
>  				   TimeTaken => 0 );
> +
> +        if ( $remote_uri->IsLocal ) {
> +
> +            my $OtherObj = $remote_uri->Object;
> +            my ( $val, $Msg ) = $OtherObj->_NewTransaction(Type  => 'AddLink',
> +                                                           Field => $direction eq 'Target' ? $LINKDIRMAP{$args{'Type'}}->{Base} 
> +                                                                                           : $LINKDIRMAP{$args{'Type'}}->{Target},
> +                                                           NewValue => $self->URI,
> +                                                           ActtivateScrips => $RT::LinkTransactionsRun2Scrips,
> +                                                           TimeTaken => 0 );
> +        }
>          return ( $val, $Msg );
>      }
> +    else {
> +        return ( $val, $Msg );
> +    }
>  
>  }
>  
> Index: etc/RT_Config.pm.in
> ===================================================================
> --- etc/RT_Config.pm.in	(revision 4013)
> +++ etc/RT_Config.pm.in	(working copy)
> @@ -468,6 +468,11 @@
>  @ActiveStatus = qw(new open stalled) unless @ActiveStatus;
>  @InactiveStatus = qw(resolved rejected deleted) unless @InactiveStatus;
>  
> +# Backward compatability setting. Add/Delete Link used to record one
> +# transaction and run one scrip. Set this value to 1 if you want
> +# both link transactions to have a scrip run.
> +Set($LinkTransactionsRun2Scrips , 0);

I'd probably reverse this variable, so that the false case is the common
one in new code. "You enable strange behaviour"


> +
>  # }}}
>  
>  

> _______________________________________________
> Rt-devel mailing list
> Rt-devel at lists.bestpractical.com
> http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel


--