OK gang,
Here is my stab at fixing the asymmetrical link
transaction problem. Added a couple of tests
and everything passes. See any problems?
(attached)
-Todd
link_symmetry.patch (4.05 KB)
Todd Chapman wrote:
OK gang,
Here is my stab at fixing the asymmetrical link
transaction problem. Added a couple of tests
and everything passes. See any problems?
You’ve been a busy beaver, thank you.
I guess you didn’t test the other scrips actually not running (the
default would be on, right?), because there is a typo in you patch:
ActtivateScrips (note the extra t) on both _NewTransaction calls.
Also, I usually prefer to have the simpler branch of a condition handled
first, especially when it involves a return clause. You’ve switched that
for the one
if ( $args{‘Silent’} ) {
I guess you wanted to collect the returning at the end of the function.
Ok, now that I’m done nitpicking (sorry for that ;)), I’d also like to
remark what you’re suggesting now is to always have the second
transaction, but to only conditionally run the scrips for it. I don’t
think that’s the right thing to do. I think the argument was to always
run scrips with a transaction. If you’re being backwards compatible, you
can do without the extra transaction aswell. So I’d suggest to
completely scrap the extra (misspelled ;)) option and add the condition
with the
if ( $remote_uri->IsLocal )
like this:
if ( $remote_uri->IsLocal && $RT::LinkMakes2Transactions)
What do you think?
Rolf.
Todd Chapman wrote:
OK gang,
Here is my stab at fixing the asymmetrical link
transaction problem. Added a couple of tests
and everything passes. See any problems?You’ve been a busy beaver, thank you.
I guess you didn’t test the other scrips actually not running (the
default would be on, right?), because there is a typo in you patch:
ActtivateScrips (note the extra t) on both _NewTransaction calls.Also, I usually prefer to have the simpler branch of a condition handled
first, especially when it involves a return clause. You’ve switched that
for the one
if ( $args{‘Silent’} ) {
I guess you wanted to collect the returning at the end of the function.
IMHO would be better to handle simple branch first and get rid of one
indent level with
return bla-bla if $args{‘Silent’};
or
if( $args{‘Silent’} ) {
return bla-bla;
}
without else block.
Ok, now that I’m done nitpicking (sorry for that ;)), I’d also like to
remark what you’re suggesting now is to always have the second
transaction, but to only conditionally run the scrips for it. I don’t
think that’s the right thing to do. I think the argument was to always
run scrips with a transaction. If you’re being backwards compatible, you
can do without the extra transaction aswell. So I’d suggest to
completely scrap the extra (misspelled ;)) option and add the condition
with the
if ( $remote_uri->IsLocal )
like this:
if ( $remote_uri->IsLocal && $RT::LinkMakes2Transactions)What do you think?
No, you aren’t right, “one transaction” is bug and bugs must be fixed,
if someone poke at old behaviour and used wrong thing(didn’t report
bug) then it’s thier problems. IMHO concept of the Todd’s patch is ok.
Rolf.
Rt-devel mailing list
Rt-devel@lists.bestpractical.com
The rt-devel Archives
Best regards, Ruslan.
Todd Chapman wrote:
OK gang,
Here is my stab at fixing the asymmetrical link
transaction problem. Added a couple of tests
and everything passes. See any problems?You’ve been a busy beaver, thank you.
I guess you didn’t test the other scrips actually not running (the
default would be on, right?), because there is a typo in you patch:
ActtivateScrips (note the extra t) on both _NewTransaction calls.Also, I usually prefer to have the simpler branch of a condition handled
first, especially when it involves a return clause. You’ve switched that
for the one
if ( $args{‘Silent’} ) {
I guess you wanted to collect the returning at the end of the function.
IMHO would be better to handle simple branch first and get rid of one
indent level with
return bla-bla if $args{‘Silent’};
or
if( $args{‘Silent’} ) {
return bla-bla;
}
without else block.
OK I just erased a much less friendly reply. Suffice it to say that if
you guys want to code to look different (but be functionally the same) then
you write the code.
Ok, now that I’m done nitpicking (sorry for that ;)), I’d also like to
remark what you’re suggesting now is to always have the second
transaction, but to only conditionally run the scrips for it. I don’t
think that’s the right thing to do. I think the argument was to always
run scrips with a transaction. If you’re being backwards compatible, you
can do without the extra transaction aswell. So I’d suggest to
completely scrap the extra (misspelled ;)) option and add the condition
with the
if ( $remote_uri->IsLocal )
like this:
if ( $remote_uri->IsLocal && $RT::LinkMakes2Transactions)What do you think?
No, you aren’t right, “one transaction” is bug and bugs must be fixed,
if someone poke at old behaviour and used wrong thing(didn’t report
bug) then it’s thier problems. IMHO concept of the Todd’s patch is ok.
I agree. The bug needs to be fixed.
Todd Chapman wrote:
OK gang,
Here is my stab at fixing the asymmetrical link
transaction problem. Added a couple of tests
and everything passes. See any problems?You’ve been a busy beaver, thank you.
Yes I have. 
I also submitted 3 other patches to rt-bugs today,
including a bug in HasRight that bit me while writing RightsMatrix.
I guess you didn’t test the other scrips actually not running (the
default would be on, right?), because there is a typo in you patch:
ActtivateScrips (note the extra t) on both _NewTransaction calls.
Your right. I didn’t test the scrips firing/not firing. Not sure how
to do that in regression tests. I’ll fix the typo and resubmit the patch.
Also, I usually prefer to have the simpler branch of a condition handled
first, especially when it involves a return clause. You’ve switched that
for the one
if ( $args{‘Silent’} ) {
I guess you wanted to collect the returning at the end of the function.
See other mail. Nitpicking is not appreciated.
Ok, now that I’m done nitpicking (sorry for that ;)), I’d also like to
remark what you’re suggesting now is to always have the second
transaction, but to only conditionally run the scrips for it. I don’t
think that’s the right thing to do. I think the argument was to always
run scrips with a transaction. If you’re being backwards compatible, you
can do without the extra transaction aswell. So I’d suggest to
completely scrap the extra (misspelled ;)) option and add the condition
with the
if ( $remote_uri->IsLocal )
like this:
if ( $remote_uri->IsLocal && $RT::LinkMakes2Transactions)What do you think?
I agree with Ruslan. One transaction is a bug. 2 is the right way. Defaulting
to one scrip is necessary for backwards compatability.
Todd Chapman wrote:
See other mail. Nitpicking is not appreciated.
Since this is an open source project, you should see it as a suggestion
as to what others find to be more readable. I’ve included it, because
you explicitly rewrote the code to look the other way. In no way was it
meant as a disrespect or rejection of your code.
I agree with Ruslan. One transaction is a bug. 2 is the right way. Defaulting
to one scrip is necessary for backwards compatability.
If you’re considering 2 transactions a bug fix, I’d argue the default
should be for running 2 scrips right away, because I believe we’ve also
established that a transaction that doesn’t run scrips is a bug aswell.
In reality, I don’t really care one way or the other as long as some
form of your patch makes it into the distribution 
Rolf.
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”
None of the current test files seem to apply. I’ll create a
new one unless you suggest otherwise. Do you want one aimed
specifically at linking, or should I create one that is
aimed at testing all of the RT::Ticket API?
Go for one that tests links. smaller testfiles feel easier to work with
and extend.
[snip]
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
[snip]
None of the current test files seem to apply. I’ll create a
new one unless you suggest otherwise. Do you want one aimed
specifically at linking, or should I create one that is
aimed at testing all of the RT::Ticket API?
-Todd
Attached is my second attempt at this.
New Features:
- if/else condition reordering
- RT config variable renamed
- All RT::Ticket Link tests were moved to a new file
- I actually test that the config file setting works and
causes 1/2 scrips to run depending…
link_symmetry.patch (9.39 KB)
Attached is my second attempt at this.
New Features:
- if/else condition reordering
- RT config variable renamed
- All RT::Ticket Link tests were moved to a new file
- I actually test that the config file setting works and
causes 1/2 scrips to run depending…
That looks a lot better. bounce it to rt-bugs?
Attached is my second attempt at this.
New Features:
- if/else condition reordering
- RT config variable renamed
- All RT::Ticket Link tests were moved to a new file
- I actually test that the config file setting works and
causes 1/2 scrips to run depending…That looks a lot better. bounce it to rt-bugs?
I just had a little thought. Do we care that when a link
is created during a ticket create, only an AddLink
transaction is recorded for the other object and not the
new ticket?
Todd Chapman wrote:
I just had a little thought. Do we care that when a link
is created during a ticket create, only an AddLink
transaction is recorded for the other object and not the
new ticket?
I believe I saw a comment somewhere that this is a feature, i.e. there
intentionally is no transaction for it. Since there is no transaction
for any value set at ticket creation time (like e.g. Owner), I’m not
inclined to ask for a change. YMMV.
Rolf.
I think on create is a special circumstance where we don’t need another
transaction, since it is the initial state. This matches the behavior
with watcher/owner, priority, etc.
Joby Walker
ITI SSG, University of Washington
Todd Chapman wrote:> On Tue, Nov 08, 2005 at 11:39:12AM -0500, Jesse Vincent wrote:
On Mon, Nov 07, 2005 at 06:14:16PM -0500, Todd Chapman wrote:
Attached is my second attempt at this.
New Features:
- if/else condition reordering
- RT config variable renamed
- All RT::Ticket Link tests were moved to a new file
- I actually test that the config file setting works and
causes 1/2 scrips to run depending…That looks a lot better. bounce it to rt-bugs?
I just had a little thought. Do we care that when a link
is created during a ticket create, only an AddLink
transaction is recorded for the other object and not the
new ticket?
Rt-devel mailing list
Rt-devel@lists.bestpractical.com
The rt-devel Archives
It struck me today that it would be cool to connect to an RT server
via IMAP.
Queues that you can access would be folders, tickets and appends as
threads.
Your own ticket appends would default to already-read messages, but
other correspondences would be unread.
I don’t have the development skill to write such an interface; I
thought I’d throw the idea out there to see what people thought.
A.
It struck me today that it would be cool to connect to an RT server
via IMAP.Queues that you can access would be folders, tickets and appends as
threads.Your own ticket appends would default to already-read messages, but
other correspondences would be unread.I don’t have the development skill to write such an interface; I
thought I’d throw the idea out there to see what people thought.
Yes. It would be very cool. The IMAP protocol is non-trivial. Sadly,
I’ve not found a good solid IMAP server library for perl that we could
make use of.
Yes. It would be very cool. The IMAP protocol is non-trivial. Sadly,
I’ve not found a good solid IMAP server library for perl that we could
make use of.
Interesting. And it wouldn’t have to be read-only – you
could use imap to change queues. And a “reply” could
be a real email reply, that in fact appended to the ticket.
(Wouldn’t be too hard to insert some sort of email header
that provided credentials that proved the sender got it
from imap, and therefore could resolve, delete, assign, etc.
Not that I have any time to throw at this, but were you
looking for a pure-perl imap server? Or a C library where you
could embed a perl interpreter for handling a “mailbox driver”?
Or would you do this similarly to the CLI, where the imap
server would make web calls?
bobg
Yes. It would be very cool. The IMAP protocol is non-trivial. Sadly,
I’ve not found a good solid IMAP server library for perl that we could
make use of.Interesting. And it wouldn’t have to be read-only – you
could use imap to change queues. And a “reply” could
be a real email reply, that in fact appended to the ticket.
(Wouldn’t be too hard to insert some sort of email header
that provided credentials that proved the sender got it
from imap, and therefore could resolve, delete, assign, etc.Not that I have any time to throw at this, but were you
looking for a pure-perl imap server? Or a C library where you
could embed a perl interpreter for handling a “mailbox driver”?
Or would you do this similarly to the CLI, where the imap
server would make web calls?
IMHO not really IMAP server is required, but library(or server) that
covers IMAP server protocol and runs external callbacks to do things
in storage. Solution shouldn’t be tied to some particular storage, but
should allow us to develop(embed) own storage engine(RT DB) with
custom plugin or driver(doesn’t matter how to name it).
bobg
Be sure to check out the RT Wiki at http://wiki.bestpractical.com
Buy your copy of our new book, RT Essentials, today!
Download a free sample chapter from http://rtbook.bestpractical.com
Best regards, Ruslan.