[Rt-commit] rt branch, 4.2/fix-organization-for-all-links, created. rt-4.2.3-14-gfc363d2

Jim Brandt jbrandt at bestpractical.com
Thu Feb 27 11:18:19 EST 2014


The branch, 4.2/fix-organization-for-all-links has been created
        at  fc363d25eae8e7f05257a5d4d3c3dd263f642617 (commit)

- Log -----------------------------------------------------------------
commit fb647e0f74487c40b6b39b7478bb789f52368e45
Author: Jim Brandt <jbrandt at bestpractical.com>
Date:   Wed Feb 26 15:30:51 2014 -0500

    Update tests to reflect API consistent with ticket links
    
    Also expand test coverage to handle additional failure cases.

diff --git a/t/articles/uri-articles.t b/t/articles/uri-articles.t
index b6a1c8d..9ad4b07 100644
--- a/t/articles/uri-articles.t
+++ b/t/articles/uri-articles.t
@@ -2,7 +2,8 @@
 use strict;
 use warnings;
 
-use RT::Test tests => 9;
+use RT::Test tests => undef;
+use Test::Warn;
 
 use_ok "RT::URI::fsck_com_article";
 my $uri = RT::URI::fsck_com_article->new( $RT::SystemUser );
@@ -12,7 +13,7 @@ isa_ok $uri, 'RT::URI::fsck_com_article';
 isa_ok $uri, 'RT::URI::base';
 isa_ok $uri, 'RT::Base';
 
-is $uri->LocalURIPrefix, 'fsck.com-article://example.com/article/';
+is $uri->LocalURIPrefix, 'fsck.com-article://example.com';
 
 my $class = RT::Class->new( $RT::SystemUser );
 $class->Create( Name => 'URItest - '. $$ );
@@ -26,5 +27,24 @@ my ($id, $msg) = $article->Create(
 ok($id,$msg);
 
 $uri = RT::URI::fsck_com_article->new( $article->CurrentUser );
-is $uri->LocalURIPrefix . $article->id, $uri->URIForObject( $article );
+is $uri->URIForObject( $article ),
+    'fsck.com-article://example.com/article/' . $article->id,
+        'Got correct URIForObject';
+my $article_id = $article->Id;
+ok ($uri->ParseURI("fsck.com-article://example.com/article/$article_id"),
+    'Parsed URI');
 
+ok ($article->Delete(), 'Deleted article');
+
+my $ret;
+warning_like {
+    $ret = $uri->ParseURI("fsck.com-article://example.com/article/$article_id");
+} qr/Unable to load article for id $article_id. It may have been deleted/,
+    "Warned about missing article";
+
+ok (!$ret, 'Returned false on missing article');
+
+ok (!$uri->ParseURI("fsck.com-article://foo.com/article/$article_id"),
+    'ParseURI returned false with incorrect Organization');
+
+done_testing();

commit 6d5a07560c656c7d1e0cd1f0b6b372de943d15e3
Author: Jim Brandt <jbrandt at bestpractical.com>
Date:   Wed Feb 26 15:31:07 2014 -0500

    Update article URI API to be consistent with ticket URI API
    
    Previously the article implementation of LocalURIPrefix added
    /article/ to the returned prefix where the ticket version
    implemented in fsck_com_rt.pm did not add /ticket/ in
    LocalURIPrefix but rather in URIForObject. LocalURIPrefix is
    largely an internal method used by URIForObject, however
    rt-validator uses it when fixing links for Organization
    changes. Make the APIs consistent to avoid special case code
    elsewhere.

diff --git a/lib/RT/URI/fsck_com_article.pm b/lib/RT/URI/fsck_com_article.pm
index 0c61623..031f504 100644
--- a/lib/RT/URI/fsck_com_article.pm
+++ b/lib/RT/URI/fsck_com_article.pm
@@ -63,8 +63,7 @@ Returns the prefix for a local article URI
 
 sub LocalURIPrefix {
     my $self = shift;
-    my $prefix = $self->Scheme. "://". RT->Config->Get('Organization')
-        . "/article/";
+    my $prefix = $self->Scheme. "://". RT->Config->Get('Organization');
     return ($prefix);
 }
 
@@ -79,7 +78,7 @@ sub URIForObject {
     my $self = shift;
 
     my $obj = shift;
-    return ($self->LocalURIPrefix. $obj->Id);
+    return ($self->LocalURIPrefix . "/article/" . $obj->Id);
 }
 
 
@@ -114,7 +113,7 @@ sub ParseURI {
        #If it's a local URI, load the article object and return its URI
     if ( $self->IsLocal) {
         my $local_uri_prefix = $self->LocalURIPrefix;
-        if ($self->{'uri'} =~ /^$local_uri_prefix(\d+)$/) {
+        if ($self->{'uri'} =~ /^$local_uri_prefix\/article\/(\d+)$/) {
             my $id = $1;
             $article = RT::Article->new( $self->CurrentUser );
             $article->Load($id);

commit d4a4317cfbbbe919881da3131c4a4b738741aa92
Author: Jim Brandt <jbrandt at bestpractical.com>
Date:   Wed Feb 26 17:26:17 2014 -0500

    Warn if we parse an id but can't load the article

diff --git a/lib/RT/URI/fsck_com_article.pm b/lib/RT/URI/fsck_com_article.pm
index 031f504..5819ab6 100644
--- a/lib/RT/URI/fsck_com_article.pm
+++ b/lib/RT/URI/fsck_com_article.pm
@@ -116,10 +116,14 @@ sub ParseURI {
         if ($self->{'uri'} =~ /^$local_uri_prefix\/article\/(\d+)$/) {
             my $id = $1;
             $article = RT::Article->new( $self->CurrentUser );
-            $article->Load($id);
+            my ($ret, $msg) = $article->Load($id);
 
             #If we couldn't find a article, return undef.
-            unless ( defined $article->Id ) {
+            unless ( $article and $article->Id ) {
+                # We got an id, but couldn't load it, so warn that it may
+                # have been deleted.
+                RT::Logger->warning("Unable to load article for id $id. It may"
+                    . " have been deleted: $msg");
                 return undef;
             }
             } else {

commit 71ab3bfd749c9f70ce4cd97e23e67d79a843ec05
Author: Jim Brandt <jbrandt at bestpractical.com>
Date:   Wed Feb 26 16:55:51 2014 -0500

    Move article object validation to cover entire method
    
    Add a check on the $article object to the end of the method to
    catch the case where Organization may have changed. Previously
    this would result in a "Can't call method "Id" on an undefined
    value" warning.

diff --git a/lib/RT/URI/fsck_com_article.pm b/lib/RT/URI/fsck_com_article.pm
index 5819ab6..931c55f 100644
--- a/lib/RT/URI/fsck_com_article.pm
+++ b/lib/RT/URI/fsck_com_article.pm
@@ -131,6 +131,11 @@ sub ParseURI {
             }
     }
 
+    #If we couldn't find a article, return undef.
+    unless ( $article and $article->Id ) {
+        return undef;
+    }
+
     $self->{'object'} = $article;
     return ($article->Id);
 }

commit 92c6e25fecbe4fc36cca0ceebfc0ea61ae77e672
Author: Jim Brandt <jbrandt at bestpractical.com>
Date:   Tue Feb 25 15:23:04 2014 -0500

    Validate article and other types of links
    
    Check non-ticket links for a changed Organization and
    for links to missing objects like Articles. Use %INC
    to find loaded RT::URI subclasses to catch link types that
    might be loaded from an extension like Assets.
    
    Add Article to the models checked since Articles are part of
    RT now.

diff --git a/sbin/rt-validator.in b/sbin/rt-validator.in
old mode 100644
new mode 100755
index db89dce..d24b777
--- a/sbin/rt-validator.in
+++ b/sbin/rt-validator.in
@@ -115,6 +115,7 @@ my %TYPE = (
 
 my @models = qw(
     ACE
+    Article
     Attachment
     Attribute
     CachedGroupMember
@@ -930,7 +931,10 @@ push @CHECKS, 'Links: wrong organization' => sub {
         { model => 'Link', column => 'Base' },
     );
 
-    my $rt_uri = RT::URI::fsck_com_rt->new( $RT::SystemUser );
+    my @rt_uris = rt_uri_modules();
+    foreach my $package (@rt_uris) {
+
+    my $rt_uri = $package->new( $RT::SystemUser );
     my $scheme = $rt_uri->Scheme;
     my $prefix = $rt_uri->LocalURIPrefix;
 
@@ -972,6 +976,7 @@ push @CHECKS, 'Links: wrong organization' => sub {
             last; # plenty of chances we covered all cases with one update
         }
     }
+    } # end foreach my $package (@rt_uris)
     return $res;
 };
 
@@ -1055,7 +1060,10 @@ push @CHECKS, 'Links: missing object' => sub {
         { model => 'Link', column => 'Base' },
     );
 
-    my $rt_uri = RT::URI::fsck_com_rt->new( $RT::SystemUser );
+    my @rt_uris = rt_uri_modules();
+    foreach my $package (@rt_uris) {
+
+    my $rt_uri = $package->new( $RT::SystemUser );
     my $scheme = $rt_uri->Scheme;
     my $prefix = $rt_uri->LocalURIPrefix;
 
@@ -1069,6 +1077,8 @@ push @CHECKS, 'Links: missing object' => sub {
 
             my $tprefix = $prefix .'/'. ($tclass eq 'RT::Ticket'? 'ticket' : $tclass) .'/';
 
+            $tprefix = $prefix . '/article/' if $tclass eq 'RT::Article';
+
             my $query = "SELECT s.id FROM $stable s LEFT JOIN $ttable t "
                 ." ON t.id = ". sql_str2int("SUBSTR(s.$scolumn, ?)")
                 ." WHERE s.$scolumn LIKE ? AND t.id IS NULL";
@@ -1096,6 +1106,7 @@ push @CHECKS, 'Links: missing object' => sub {
             }
         }
     }
+    } # end foreach my $package (@rt_uris)
     return $res;
 };
 
@@ -1375,6 +1386,20 @@ sub prompt_integer {
     return $cached_answer{ $token } = $a;
 } }
 
+# Find all RT::URI modules RT has loaded
+
+sub rt_uri_modules {
+    my @uris = grep /^RT\/URI\/.+\.pm$/, keys %INC;
+    my @uri_modules;
+    foreach my $uri_path (@uris){
+        next if $uri_path =~ /base\.pm$/; # Skip base RT::URI object
+        $uri_path = substr $uri_path, 0, -3; # chop off .pm
+        push @uri_modules, join '::', split '/', $uri_path;
+    }
+
+    return @uri_modules;
+}
+
 1;
 
 __END__

commit 747a1355475b661a78665a64208b8d68286638f3
Author: Jim Brandt <jbrandt at bestpractical.com>
Date:   Tue Feb 25 16:44:11 2014 -0500

    Add indent for new foreach loops

diff --git a/sbin/rt-validator.in b/sbin/rt-validator.in
index d24b777..e60328d 100755
--- a/sbin/rt-validator.in
+++ b/sbin/rt-validator.in
@@ -934,48 +934,48 @@ push @CHECKS, 'Links: wrong organization' => sub {
     my @rt_uris = rt_uri_modules();
     foreach my $package (@rt_uris) {
 
-    my $rt_uri = $package->new( $RT::SystemUser );
-    my $scheme = $rt_uri->Scheme;
-    my $prefix = $rt_uri->LocalURIPrefix;
-
-    foreach my $use ( @URI_USES ) {
-        my $table = m2t( $use->{'model'} );
-        my $column = $use->{'column'};
-
-        my $query = "SELECT id, $column FROM $table WHERE"
-            . " $column LIKE ? AND $column NOT LIKE ?";
-        my @binds = ($scheme ."://%", $prefix ."%");
+        my $rt_uri = $package->new( $RT::SystemUser );
+        my $scheme = $rt_uri->Scheme;
+        my $prefix = $rt_uri->LocalURIPrefix;
 
-        while ( my ($k, $v) = each %{ $use->{'Additional'} || {} } ) {
-            $query .= " AND $k = ?";
-            push @binds, $v;
-        }
-        my $sth = execute_query( $query, @binds );
-        while ( my ($id, $value) = $sth->fetchrow_array ) {
-            $res = 0;
-            print STDERR "Record #$id in $table. Value of $column column most probably is an incorrect link\n";
-            my ($wrong_org) = ( $value =~ m{^\Q$scheme\E://(.+)/[^/]+/[0-9]*$} );
-            next unless my $replace_with = prompt(
-                'Replace',
-                "Column $column in $table is a link. Local links has scheme '$scheme'"
-                ." followed by organization name from the config file. There is record"
-                ." #$id that has scheme '$scheme', but organization is '$wrong_org'."
-                ." Most probably you changed organization, but didn't update links."
-                ." It's ok to replace these wrong links.\n",
-                "Links: wrong organization $wrong_org"
-            );
+        foreach my $use ( @URI_USES ) {
+            my $table = m2t( $use->{'model'} );
+            my $column = $use->{'column'};
 
-            print "Updating record(s) in $table\n" if $opt{'verbose'};
-            my $wrong_prefix = $scheme . '://'. $wrong_org;
-            my $query = "UPDATE $table SET $column = ". sql_concat('?', "SUBSTR($column, ?)")
-                ." WHERE $column LIKE ?";
-            execute_query( $query, $prefix, length($wrong_prefix)+1, $wrong_prefix .'/%' );
+            my $query = "SELECT id, $column FROM $table WHERE"
+              . " $column LIKE ? AND $column NOT LIKE ?";
+            my @binds = ($scheme ."://%", $prefix ."%");
 
-            $redo_check{'Links: wrong organization'} = 1;
-            $redo_check{'Links: LocalX for non-ticket'} = 1;
-            last; # plenty of chances we covered all cases with one update
+            while ( my ($k, $v) = each %{ $use->{'Additional'} || {} } ) {
+                $query .= " AND $k = ?";
+                push @binds, $v;
+            }
+            my $sth = execute_query( $query, @binds );
+            while ( my ($id, $value) = $sth->fetchrow_array ) {
+                $res = 0;
+                print STDERR "Record #$id in $table. Value of $column column most probably is an incorrect link\n";
+                my ($wrong_org) = ( $value =~ m{^\Q$scheme\E://(.+)/[^/]+/[0-9]*$} );
+                next unless my $replace_with = prompt(
+                    'Replace',
+                    "Column $column in $table is a link. Local links has scheme '$scheme'"
+                    ." followed by organization name from the config file. There is record"
+                    ." #$id that has scheme '$scheme', but organization is '$wrong_org'."
+                    ." Most probably you changed organization, but didn't update links."
+                    ." It's ok to replace these wrong links.\n",
+                    "Links: wrong organization $wrong_org"
+                                                     );
+
+                print "Updating record(s) in $table\n" if $opt{'verbose'};
+                my $wrong_prefix = $scheme . '://'. $wrong_org;
+                my $query = "UPDATE $table SET $column = ". sql_concat('?', "SUBSTR($column, ?)")
+                  ." WHERE $column LIKE ?";
+                execute_query( $query, $prefix, length($wrong_prefix)+1, $wrong_prefix .'/%' );
+
+                $redo_check{'Links: wrong organization'} = 1;
+                $redo_check{'Links: LocalX for non-ticket'} = 1;
+                last; # plenty of chances we covered all cases with one update
+            }
         }
-    }
     } # end foreach my $package (@rt_uris)
     return $res;
 };
@@ -1063,49 +1063,49 @@ push @CHECKS, 'Links: missing object' => sub {
     my @rt_uris = rt_uri_modules();
     foreach my $package (@rt_uris) {
 
-    my $rt_uri = $package->new( $RT::SystemUser );
-    my $scheme = $rt_uri->Scheme;
-    my $prefix = $rt_uri->LocalURIPrefix;
+        my $rt_uri = $package->new( $RT::SystemUser );
+        my $scheme = $rt_uri->Scheme;
+        my $prefix = $rt_uri->LocalURIPrefix;
 
-    foreach my $use ( @URI_USES ) {
-        my $stable = m2t( $use->{'model'} );
-        my $scolumn = $use->{'column'};
+        foreach my $use ( @URI_USES ) {
+            my $stable = m2t( $use->{'model'} );
+            my $scolumn = $use->{'column'};
 
-        foreach my $tmodel ( @models ) {
-            my $tclass = 'RT::'. $tmodel;
-            my $ttable = m2t($tmodel);
+            foreach my $tmodel ( @models ) {
+                my $tclass = 'RT::'. $tmodel;
+                my $ttable = m2t($tmodel);
 
-            my $tprefix = $prefix .'/'. ($tclass eq 'RT::Ticket'? 'ticket' : $tclass) .'/';
+                my $tprefix = $prefix .'/'. ($tclass eq 'RT::Ticket'? 'ticket' : $tclass) .'/';
 
-            $tprefix = $prefix . '/article/' if $tclass eq 'RT::Article';
+                $tprefix = $prefix . '/article/' if $tclass eq 'RT::Article';
 
-            my $query = "SELECT s.id FROM $stable s LEFT JOIN $ttable t "
-                ." ON t.id = ". sql_str2int("SUBSTR(s.$scolumn, ?)")
-                ." WHERE s.$scolumn LIKE ? AND t.id IS NULL";
-            my @binds = (length($tprefix) + 1, $tprefix.'%');
+                my $query = "SELECT s.id FROM $stable s LEFT JOIN $ttable t "
+                  ." ON t.id = ". sql_str2int("SUBSTR(s.$scolumn, ?)")
+                    ." WHERE s.$scolumn LIKE ? AND t.id IS NULL";
+                my @binds = (length($tprefix) + 1, $tprefix.'%');
 
-            while ( my ($k, $v) = each %{ $use->{'Additional'} || {} } ) {
-                $query .= " AND s.$k = ?";
-                push @binds, $v;
-            }
-
-            my $sth = execute_query( $query, @binds );
-            while ( my ($sid) = $sth->fetchrow_array ) {
-                $res = 0;
-                print STDERR "Link in $scolumn column in record #$sid in $stable table points"
-                    ." to not existing object.\n";
-                next unless prompt(
-                    'Delete',
-                    "Column $scolumn in $stable table is a link to an object that doesn't exist."
-                    ." You can delete such records, however make sure there is no other"
-                    ." errors with links.\n",
-                    'Link to a missing object in $ttable'
-                );
+                while ( my ($k, $v) = each %{ $use->{'Additional'} || {} } ) {
+                    $query .= " AND s.$k = ?";
+                    push @binds, $v;
+                }
 
-                delete_record($stable, $sid);
+                my $sth = execute_query( $query, @binds );
+                while ( my ($sid) = $sth->fetchrow_array ) {
+                    $res = 0;
+                    print STDERR "Link in $scolumn column in record #$sid in $stable table points"
+                      ." to not existing object.\n";
+                    next unless prompt(
+                        'Delete',
+                        "Column $scolumn in $stable table is a link to an object that doesn't exist."
+                        ." You can delete such records, however make sure there is no other"
+                        ." errors with links.\n",
+                        'Link to a missing object in $ttable'
+                                      );
+
+                    delete_record($stable, $sid);
+                }
             }
         }
-    }
     } # end foreach my $package (@rt_uris)
     return $res;
 };

commit fc363d25eae8e7f05257a5d4d3c3dd263f642617
Author: Jim Brandt <jbrandt at bestpractical.com>
Date:   Wed Feb 26 17:36:02 2014 -0500

    Fix indent. Whitespace-only change

diff --git a/lib/RT/URI/fsck_com_article.pm b/lib/RT/URI/fsck_com_article.pm
index 931c55f..2667480 100644
--- a/lib/RT/URI/fsck_com_article.pm
+++ b/lib/RT/URI/fsck_com_article.pm
@@ -126,9 +126,9 @@ sub ParseURI {
                     . " have been deleted: $msg");
                 return undef;
             }
-            } else {
-                return undef;
-            }
+        } else {
+            return undef;
+        }
     }
 
     #If we couldn't find a article, return undef.

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


More information about the rt-commit mailing list