[Rt-commit] rt branch, 3.9-trunk, updated. rt-3.9.4-109-g5517022

Jesse Vincent jesse at bestpractical.com
Mon Oct 4 02:44:12 EDT 2010


The branch, 3.9-trunk has been updated
       via  5517022af2fd1eab715e33013920b0beae148de0 (commit)
       via  760d91a08577c005a393cfb12758861347fae7d3 (commit)
       via  69095043a251871f1e579a2ff180aa3cda9cce40 (commit)
       via  e0ae21da93c26ec77438ffd5f28211a4f3b4f7ef (commit)
       via  c886bb711ecf90f8de0dbd29d2ac784ef0700055 (commit)
       via  bfc193ee2c3a73b5863beace7029dff734493368 (commit)
       via  ca5f4829390d8a33218ca65616d3704c09730097 (commit)
       via  58ec09aa899b3517927f7a020e4c8ddfbe590d15 (commit)
       via  5c5f4bd22c32f97f1d2ff1e5f20ce59f2fe7b893 (commit)
       via  db1ea16edb2c8a00e31d9b34eaa9f3d0b3c77c34 (commit)
       via  b4bee8918120bcf5eb434faa4d2ad40d2b2f4c0a (commit)
       via  3621ddfa0376427c95910e7418e0189a98679149 (commit)
       via  eb59c0278dfef8196f9d6951adb38e722fa9f78c (commit)
       via  029c36c20d86d5d2f762d3af9ebaf235aedfb48f (commit)
       via  eac37c4b00fcabd1554043c47fb84008faa9bb2f (commit)
       via  e444005cff1bb184a116b39a9ec2e8504387bdc8 (commit)
       via  b26f64206dd19cf28994cf77bac1d255bc41d50f (commit)
       via  e55f502d39f1489a19cbff1d227357574d3566bf (commit)
      from  c7b0a5050f83a2d3fc3e7980e6c66206fe5d358c (commit)

Summary of changes:
 lib/RT/ACL_Overlay.pm                              |   71 +++---
 lib/RT/Principal_Overlay.pm                        |  266 ++++++++++----------
 .../html/Search/Elements/SelectSearchesForObjects  |   13 +-
 share/html/Ticket/Elements/ShowHistory             |   20 +-
 .../Ticket/Elements/ShowTransactionAttachments     |    5 +-
 t/ticket/search_by_watcher.t                       |    4 +-
 6 files changed, 190 insertions(+), 189 deletions(-)

- Log -----------------------------------------------------------------
commit e55f502d39f1489a19cbff1d227357574d3566bf
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Thu Sep 30 14:53:23 2010 -0400

    Slightly clarify a conditional and more tightly scope a varialbe

diff --git a/lib/RT/Principal_Overlay.pm b/lib/RT/Principal_Overlay.pm
index 295f491..cd358f7 100755
--- a/lib/RT/Principal_Overlay.pm
+++ b/lib/RT/Principal_Overlay.pm
@@ -501,12 +501,12 @@ sub RolesWithRight {
 
     my (@object_clauses);
     foreach my $obj ( @{ $args{'EquivObjects'} } ) {
-        my $type = ref($obj)? ref($obj): $obj;
-        my $id;
-        $id = eval {$obj->id};
+        my $type = ref($obj) ? ref($obj) : $obj;
 
         my $object_clause = "ObjectType = '$type'";
-        $object_clause   .= " AND ObjectId = $id" if $id;
+        if ( my $id = eval { $obj->id } ) {
+            $object_clause .= " AND ObjectId = $id";
+        }
         push @object_clauses, "($object_clause)";
     }
     # find ACLs that are related to our objects only

commit b26f64206dd19cf28994cf77bac1d255bc41d50f
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Thu Sep 30 14:54:25 2010 -0400

    remove a temp variable

diff --git a/lib/RT/Principal_Overlay.pm b/lib/RT/Principal_Overlay.pm
index cd358f7..35befab 100755
--- a/lib/RT/Principal_Overlay.pm
+++ b/lib/RT/Principal_Overlay.pm
@@ -513,10 +513,9 @@ sub RolesWithRight {
     $query .= " AND (". join( ' OR ', @object_clauses ) .")"
         if @object_clauses;
 
-    my $dbh = $RT::Handle->dbh;
-    my $roles = $dbh->selectcol_arrayref($query);
+    my $roles = $RT::Handle->dbh->selectcol_arrayref($query);
     unless ( $roles ) {
-        $RT::Logger->warning( $dbh->errstr );
+        $RT::Logger->warning( $RT::Handle->dbh->errstr );
         return ();
     }
     return @$roles;

commit e444005cff1bb184a116b39a9ec2e8504387bdc8
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Thu Sep 30 14:58:03 2010 -0400

    remove a temp variable

diff --git a/lib/RT/Principal_Overlay.pm b/lib/RT/Principal_Overlay.pm
index 35befab..1b36886 100755
--- a/lib/RT/Principal_Overlay.pm
+++ b/lib/RT/Principal_Overlay.pm
@@ -481,12 +481,11 @@ sub RolesWithRight {
         EquivObjects        => [],
         @_
     );
-    my $right = $args{'Right'};
 
     my $query =
         "SELECT DISTINCT PrincipalType FROM ACL"
-        # Only find superuser or rights with the name $right
-        ." WHERE ( RightName = '$right' "
+        # Only find superuser or rights with the requested right
+        ." WHERE ( RightName = '".$args{'Right'}."' "
         # Check SuperUser if we were asked to
         . ($args{'IncludeSuperusers'}? "OR RightName = 'SuperUser' " : '' )
         .")"

commit eac37c4b00fcabd1554043c47fb84008faa9bb2f
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Thu Sep 30 14:59:27 2010 -0400

    perltidy

diff --git a/lib/RT/Principal_Overlay.pm b/lib/RT/Principal_Overlay.pm
index 1b36886..3e5c9f7 100755
--- a/lib/RT/Principal_Overlay.pm
+++ b/lib/RT/Principal_Overlay.pm
@@ -261,8 +261,9 @@ sub HasRight {
     }
 
     $args{'Right'} = RT::ACE->CanonicalizeRightName( $args{'Right'} );
-    unless ($args{'Right'}) {
-        $RT::Logger->error( "Invalid right. Couldn't canonicalize right '$args{'Right'}'");
+    unless ( $args{'Right'} ) {
+        $RT::Logger->error(
+               "Invalid right. Couldn't canonicalize right '$args{'Right'}'");
         return undef;
     }
 
@@ -270,11 +271,14 @@ sub HasRight {
         if $args{'EquivObjects'};
 
     if ( $self->__Value('Disabled') ) {
-        $RT::Logger->debug("Disabled User #" . $self->id." failed access check for " . $args{'Right'} );
+        $RT::Logger->debug(   "Disabled User #"
+                            . $self->id
+                            . " failed access check for "
+                            . $args{'Right'} );
         return (undef);
     }
 
-    if (  eval { $args{'Object'}->id} ) {
+    if ( eval { $args{'Object'}->id } ) {
         push @{ $args{'EquivObjects'} }, $args{'Object'};
     } else {
         $RT::Logger->crit("HasRight called with no valid object");
@@ -290,10 +294,10 @@ sub HasRight {
 
     # If we've cached a win or loss for this lookup say so
 
-    # Construct a hashkeys to cache decisions:
-    # 1) full_hashkey - key for any result and for full combination of uid, right and objects
-    # 2) short_hashkey - one key for each object to store positive results only, it applies
-    # only to direct group rights and partly to role rights
+# Construct a hashkeys to cache decisions:
+# 1) full_hashkey - key for any result and for full combination of uid, right and objects
+# 2) short_hashkey - one key for each object to store positive results only, it applies
+# only to direct group rights and partly to role rights
     my $self_id = $self->id;
     my $full_hashkey = join ";:;", $self_id, $args{'Right'};
     foreach ( @{ $args{'EquivObjects'} } ) {
@@ -325,16 +329,15 @@ Low level HasRight implementation, use HasRight method instead.
 
 =cut
 
-sub _HasRight
-{
+sub _HasRight {
     my $self = shift;
     {
-        my ($hit, @other) = $self->_HasGroupRight( @_ );
-        return ($hit, @other) if $hit;
+        my ( $hit, @other ) = $self->_HasGroupRight(@_);
+        return ( $hit, @other ) if $hit;
     }
     {
-        my ($hit, @other) = $self->_HasRoleRight( @_ );
-        return ($hit, @other) if $hit;
+        my ( $hit, @other ) = $self->_HasRoleRight(@_);
+        return ( $hit, @other ) if $hit;
     }
     return (0);
 }
@@ -343,115 +346,112 @@ sub _HasRight
 # where user plays role X on an object and as well the right is
 # assigned to this role X of the object, for example right CommentOnTicket
 # is granted to Cc role of a queue and user is in cc list of the queue
-sub _HasGroupRight
-{
+sub _HasGroupRight {
     my $self = shift;
-    my %args = (
-        Right        => undef,
-        EquivObjects => [],
-        @_
-    );
+    my %args = ( Right        => undef,
+                 EquivObjects => [],
+                 @_
+               );
     my $right = $args{'Right'};
 
-    my $query =
-      "SELECT ACL.id, ACL.ObjectType, ACL.ObjectId " .
-      "FROM ACL, Principals, CachedGroupMembers WHERE " .
-
-      # Only find superuser or rights with the name $right
-      "(ACL.RightName = 'SuperUser' "
-      . ( $right ne 'SuperUser' ? "OR ACL.RightName = '$right'" : '')
-      .") "
-
-      # Never find disabled groups.
-      . "AND Principals.id = ACL.PrincipalId "
-      . "AND Principals.PrincipalType = 'Group' "
-      . "AND Principals.Disabled = 0 "
-
-      # See if the principal is a member of the group recursively or _is the rightholder_
-      # never find recursively disabled group members
-      # also, check to see if the right is being granted _directly_ to this principal,
-      #  as is the case when we want to look up group rights
-      . "AND CachedGroupMembers.GroupId  = ACL.PrincipalId "
-      . "AND CachedGroupMembers.GroupId  = Principals.id "
-      . "AND CachedGroupMembers.MemberId = ". $self->Id ." "
-      . "AND CachedGroupMembers.Disabled = 0 ";
+    my $query
+        = "SELECT ACL.id, ACL.ObjectType, ACL.ObjectId "
+        . "FROM ACL, Principals, CachedGroupMembers WHERE "
+        .
+
+        # Only find superuser or rights with the name $right
+        "(ACL.RightName = 'SuperUser' "
+        . ( $right ne 'SuperUser' ? "OR ACL.RightName = '$right'" : '' )
+        . ") "
+
+        # Never find disabled groups.
+        . "AND Principals.id = ACL.PrincipalId "
+        . "AND Principals.PrincipalType = 'Group' "
+        . "AND Principals.Disabled = 0 "
+
+# See if the principal is a member of the group recursively or _is the rightholder_
+# never find recursively disabled group members
+# also, check to see if the right is being granted _directly_ to this principal,
+#  as is the case when we want to look up group rights
+        . "AND CachedGroupMembers.GroupId  = ACL.PrincipalId "
+        . "AND CachedGroupMembers.GroupId  = Principals.id "
+        . "AND CachedGroupMembers.MemberId = "
+        . $self->Id . " "
+        . "AND CachedGroupMembers.Disabled = 0 ";
 
     my @clauses;
     foreach my $obj ( @{ $args{'EquivObjects'} } ) {
-        my $type = ref( $obj ) || $obj;
+        my $type = ref($obj) || $obj;
         my $clause = "ACL.ObjectType = '$type'";
 
-        if ( defined eval { $obj->id }) { # it might be 0
-            $clause .= " AND ACL.ObjectId = ". $obj->id;
+        if ( defined eval { $obj->id } ) {    # it might be 0
+            $clause .= " AND ACL.ObjectId = " . $obj->id;
         }
 
         push @clauses, "($clause)";
     }
-    if ( @clauses ) {
-        $query .= " AND (". join( ' OR ', @clauses ) .")";
+    if (@clauses) {
+        $query .= " AND (" . join( ' OR ', @clauses ) . ")";
     }
 
     $self->_Handle->ApplyLimits( \$query, 1 );
-    my ($hit, $obj, $id) = $self->_Handle->FetchResult( $query );
+    my ( $hit, $obj, $id ) = $self->_Handle->FetchResult($query);
     return (0) unless $hit;
 
     $obj .= "-$id" if $id;
-    return (1, $obj);
+    return ( 1, $obj );
 }
 
-sub _HasRoleRight
-{
+sub _HasRoleRight {
     my $self = shift;
-    my %args = (
-        Right        => undef,
-        EquivObjects => [],
-        @_
-    );
+    my %args = ( Right        => undef,
+                 EquivObjects => [],
+                 @_
+               );
 
-    my @roles = $self->RolesWithRight( %args );
+    my @roles = $self->RolesWithRight(%args);
     return 0 unless @roles;
 
     my $right = $args{'Right'};
 
-    my $query =
-      "SELECT Groups.id "
-      . "FROM Groups, Principals, CachedGroupMembers WHERE "
+    my $query = "SELECT Groups.id "
+        . "FROM Groups, Principals, CachedGroupMembers WHERE "
 
-      # Never find disabled things
-      . "Principals.Disabled = 0 "
-      . "AND CachedGroupMembers.Disabled = 0 "
+        # Never find disabled things
+        . "Principals.Disabled = 0 " . "AND CachedGroupMembers.Disabled = 0 "
 
-      # We always grant rights to Groups
-      . "AND Principals.id = Groups.id "
-      . "AND Principals.PrincipalType = 'Group' "
+        # We always grant rights to Groups
+        . "AND Principals.id = Groups.id "
+        . "AND Principals.PrincipalType = 'Group' "
 
-      # See if the principal is a member of the group recursively or _is the rightholder_
-      # never find recursively disabled group members
-      # also, check to see if the right is being granted _directly_ to this principal,
-      #  as is the case when we want to look up group rights
-      . "AND Principals.id = CachedGroupMembers.GroupId "
-      . "AND CachedGroupMembers.MemberId = ". $self->Id ." "
+# See if the principal is a member of the group recursively or _is the rightholder_
+# never find recursively disabled group members
+# also, check to see if the right is being granted _directly_ to this principal,
+#  as is the case when we want to look up group rights
+        . "AND Principals.id = CachedGroupMembers.GroupId "
+        . "AND CachedGroupMembers.MemberId = "
+        . $self->Id . " "
 
-      . "AND (". join(' OR ', map "Groups.Type = '$_'", @roles ) .")"
-    ;
+        . "AND (" . join( ' OR ', map "Groups.Type = '$_'", @roles ) . ")";
 
     my (@object_clauses);
     foreach my $obj ( @{ $args{'EquivObjects'} } ) {
-        my $type = ref($obj)? ref($obj): $obj;
+        my $type = ref($obj) ? ref($obj) : $obj;
         my $id;
-        $id = eval {$obj->id};
+        $id = eval { $obj->id };
 
         my $clause = "Groups.Domain = '$type-Role'";
+
         # XXX: Groups.Instance is VARCHAR in DB, we should quote value
         # if we want mysql 4.0 use indexes here. we MUST convert that
         # field to integer and drop this quotes.
         $clause .= " AND Groups.Instance = '$id'" if $id;
         push @object_clauses, "($clause)";
     }
-    $query .= " AND (". join( ' OR ', @object_clauses ) .")";
+    $query .= " AND (" . join( ' OR ', @object_clauses ) . ")";
 
     $self->_Handle->ApplyLimits( \$query, 1 );
-    my ($hit) = $self->_Handle->FetchResult( $query );
+    my ($hit) = $self->_Handle->FetchResult($query);
     return (1) if $hit;
 
     return 0;
@@ -474,24 +474,24 @@ is not checked if it's set to false value.
 
 sub RolesWithRight {
     my $self = shift;
-    my %args = (
-        Right               => undef,
-        IncludeSystemRights => 1,
-        IncludeSuperusers   => 1,
-        EquivObjects        => [],
-        @_
-    );
+    my %args = ( Right               => undef,
+                 IncludeSystemRights => 1,
+                 IncludeSuperusers   => 1,
+                 EquivObjects        => [],
+                 @_
+               );
+
+    my $query = "SELECT DISTINCT PrincipalType FROM ACL"
 
-    my $query =
-        "SELECT DISTINCT PrincipalType FROM ACL"
         # Only find superuser or rights with the requested right
-        ." WHERE ( RightName = '".$args{'Right'}."' "
+        . " WHERE ( RightName = '" . $args{'Right'} . "' "
+
         # Check SuperUser if we were asked to
-        . ($args{'IncludeSuperusers'}? "OR RightName = 'SuperUser' " : '' )
-        .")"
+        . ( $args{'IncludeSuperusers'} ? "OR RightName = 'SuperUser' " : '' )
+        . ")"
+
         # we need only roles
-        ." AND PrincipalType != 'Group'"
-    ;
+        . " AND PrincipalType != 'Group'";
 
     # skip rights granted on system level if we were asked to
     unless ( $args{'IncludeSystemRights'} ) {
@@ -508,12 +508,13 @@ sub RolesWithRight {
         }
         push @object_clauses, "($object_clause)";
     }
+
     # find ACLs that are related to our objects only
-    $query .= " AND (". join( ' OR ', @object_clauses ) .")"
+    $query .= " AND (" . join( ' OR ', @object_clauses ) . ")"
         if @object_clauses;
 
     my $roles = $RT::Handle->dbh->selectcol_arrayref($query);
-    unless ( $roles ) {
+    unless ($roles) {
         $RT::Logger->warning( $RT::Handle->dbh->errstr );
         return ();
     }
@@ -577,12 +578,12 @@ sub _ReferenceId {
     my $scalar = shift;
 
     # just return the value for non-objects
-    return $scalar unless UNIVERSAL::can($scalar, 'id');
+    return $scalar unless UNIVERSAL::can( $scalar, 'id' );
 
     return ref($scalar) unless $scalar->id;
 
     # an object -- return the class and id
-    return(ref($scalar)."-". $scalar->id);
+    return ( ref($scalar) . "-" . $scalar->id );
 }
 
 

commit 029c36c20d86d5d2f762d3af9ebaf235aedfb48f
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Thu Sep 30 15:09:57 2010 -0400

    small bits of cache key cleanup in ACL code.

diff --git a/lib/RT/Principal_Overlay.pm b/lib/RT/Principal_Overlay.pm
index 3e5c9f7..8e65913 100755
--- a/lib/RT/Principal_Overlay.pm
+++ b/lib/RT/Principal_Overlay.pm
@@ -317,7 +317,7 @@ sub HasRight {
     my ( $hitcount, $via_obj ) = $self->_HasRight(%args);
 
     $_ACL_CACHE->set( $full_hashkey => $hitcount ? 1 : -1 );
-    $_ACL_CACHE->set( "$self_id;:;$args{'Right'};:;$via_obj" => 1 )
+    $_ACL_CACHE->set( join(';:;',  $self->id, $args{'Right'},$via_obj) => 1 )
         if $via_obj && $hitcount;
 
     return ($hitcount);

commit eb59c0278dfef8196f9d6951adb38e722fa9f78c
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Thu Sep 30 15:13:35 2010 -0400

    remove some quasi-debugging code from search_by_watcher.t.  50%
    performance speedup. The (small) downside is that the test file is no longer
    reentrant, per ruz.

diff --git a/t/ticket/search_by_watcher.t b/t/ticket/search_by_watcher.t
index c1ba254..171eb23 100644
--- a/t/ticket/search_by_watcher.t
+++ b/t/ticket/search_by_watcher.t
@@ -60,11 +60,9 @@ sub run_tests {
 
 sub run_test {
     my ($query, %checks) = @_;
-    my $query_prefix = join ' OR ', map 'id = '. $_->id, @tickets;
 
     my $tix = RT::Tickets->new(RT->SystemUser);
-    $tix->FromSQL( "( $query_prefix ) AND ( $query )" );
-
+    $tix->FromSQL($query);
     my $error = 0;
 
     my $count = 0;

commit 3621ddfa0376427c95910e7418e0189a98679149
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Thu Sep 30 15:16:56 2010 -0400

    cache key cleanup

diff --git a/lib/RT/Principal_Overlay.pm b/lib/RT/Principal_Overlay.pm
index 8e65913..464a505 100755
--- a/lib/RT/Principal_Overlay.pm
+++ b/lib/RT/Principal_Overlay.pm
@@ -298,13 +298,12 @@ sub HasRight {
 # 1) full_hashkey - key for any result and for full combination of uid, right and objects
 # 2) short_hashkey - one key for each object to store positive results only, it applies
 # only to direct group rights and partly to role rights
-    my $self_id = $self->id;
-    my $full_hashkey = join ";:;", $self_id, $args{'Right'};
+    my $full_hashkey = join (";:;", $self->id, $args{'Right'});
     foreach ( @{ $args{'EquivObjects'} } ) {
-        my $ref_id = _ReferenceId($_);
+        my $ref_id = $self->_ReferenceId($_);
         $full_hashkey .= ";:;$ref_id";
 
-        my $short_hashkey = join ";:;", $self_id, $args{'Right'}, $ref_id;
+        my $short_hashkey = join(";:;", $self->id, $args{'Right'}, $ref_id);
         my $cached_answer = $_ACL_CACHE->fetch($short_hashkey);
         return $cached_answer > 0 if defined $cached_answer;
     }

commit b4bee8918120bcf5eb434faa4d2ad40d2b2f4c0a
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Thu Sep 30 15:17:10 2010 -0400

    slight cleanup to _ReferenceId

diff --git a/lib/RT/Principal_Overlay.pm b/lib/RT/Principal_Overlay.pm
index 464a505..f166e7d 100755
--- a/lib/RT/Principal_Overlay.pm
+++ b/lib/RT/Principal_Overlay.pm
@@ -568,21 +568,23 @@ sub _GetPrincipalTypeForACL {
 
 Returns a list uniquely representing an object or normal scalar.
 
-For scalars, its string value is returned; for objects that has an
-id() method, its class name and Id are returned as a string separated by a "-".
+For a scalar, its string value is returned.
+For an object that has an id() method which returns a value, its class name and id are returned as a string separated by a "-".
+For an object that has an id() method which returns false, its class name is returned.
 
 =cut
 
 sub _ReferenceId {
+    my $self = shift;
     my $scalar = shift;
-
-    # just return the value for non-objects
-    return $scalar unless UNIVERSAL::can( $scalar, 'id' );
-
-    return ref($scalar) unless $scalar->id;
-
-    # an object -- return the class and id
-    return ( ref($scalar) . "-" . $scalar->id );
+    my $id = eval { $scalar->id };
+    if ($@) {
+        return $scalar;
+    } elsif ($id) {
+        return ref($scalar) . "-" . $id;
+    } else {
+        return ref($scalar);
+    }
 }
 
 

commit db1ea16edb2c8a00e31d9b34eaa9f3d0b3c77c34
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Thu Sep 30 15:18:23 2010 -0400

    eliminate a temp variable

diff --git a/lib/RT/Principal_Overlay.pm b/lib/RT/Principal_Overlay.pm
index f166e7d..8c89924 100755
--- a/lib/RT/Principal_Overlay.pm
+++ b/lib/RT/Principal_Overlay.pm
@@ -551,15 +551,11 @@ return that. if it has no type, return group.
 
 sub _GetPrincipalTypeForACL {
     my $self = shift;
-    my $type;    
     if ($self->PrincipalType eq 'Group' && $self->Object->Domain =~ /Role$/) {
-        $type = $self->Object->Type;
-    }
-    else {
-        $type = $self->PrincipalType;
+        return $self->Object->Type;
+    } else {
+        return $self->PrincipalType;
     }
-
-    return($type);
 }
 
 

commit 5c5f4bd22c32f97f1d2ff1e5f20ce59f2fe7b893
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Fri Oct 1 13:51:30 2010 -0400

    minor style cleanup

diff --git a/lib/RT/Principal_Overlay.pm b/lib/RT/Principal_Overlay.pm
index 8c89924..1c1af6d 100755
--- a/lib/RT/Principal_Overlay.pm
+++ b/lib/RT/Principal_Overlay.pm
@@ -301,7 +301,7 @@ sub HasRight {
     my $full_hashkey = join (";:;", $self->id, $args{'Right'});
     foreach ( @{ $args{'EquivObjects'} } ) {
         my $ref_id = $self->_ReferenceId($_);
-        $full_hashkey .= ";:;$ref_id";
+        $full_hashkey .= ";:;".$ref_id;
 
         my $short_hashkey = join(";:;", $self->id, $args{'Right'}, $ref_id);
         my $cached_answer = $_ACL_CACHE->fetch($short_hashkey);

commit 58ec09aa899b3517927f7a020e4c8ddfbe590d15
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Fri Oct 1 13:51:43 2010 -0400

    remove an unused varialbe

diff --git a/lib/RT/Principal_Overlay.pm b/lib/RT/Principal_Overlay.pm
index 1c1af6d..a59b2cc 100755
--- a/lib/RT/Principal_Overlay.pm
+++ b/lib/RT/Principal_Overlay.pm
@@ -411,8 +411,6 @@ sub _HasRoleRight {
     my @roles = $self->RolesWithRight(%args);
     return 0 unless @roles;
 
-    my $right = $args{'Right'};
-
     my $query = "SELECT Groups.id "
         . "FROM Groups, Principals, CachedGroupMembers WHERE "
 

commit ca5f4829390d8a33218ca65616d3704c09730097
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Fri Oct 1 13:51:56 2010 -0400

    minor grammar fixes

diff --git a/lib/RT/Principal_Overlay.pm b/lib/RT/Principal_Overlay.pm
index a59b2cc..5ad9dae 100755
--- a/lib/RT/Principal_Overlay.pm
+++ b/lib/RT/Principal_Overlay.pm
@@ -461,11 +461,11 @@ set of objects. Takes Right, EquiveObjects,
 IncludeSystemRights and IncludeSuperusers arguments.
 
 IncludeSystemRights is true by default, rights
-granted on system level are not accouned when option
-is set to false value.
+granted systemwide are ignored when IncludeSystemRights
+is set to a false value.
 
 IncludeSuperusers is true by default, SuperUser right
-is not checked if it's set to false value.
+is not checked if it's set to a false value.
 
 =cut
 

commit bfc193ee2c3a73b5863beace7029dff734493368
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Fri Oct 1 13:53:23 2010 -0400

    minor refactoring

diff --git a/lib/RT/Principal_Overlay.pm b/lib/RT/Principal_Overlay.pm
index 5ad9dae..b5fe771 100755
--- a/lib/RT/Principal_Overlay.pm
+++ b/lib/RT/Principal_Overlay.pm
@@ -426,23 +426,22 @@ sub _HasRoleRight {
 # also, check to see if the right is being granted _directly_ to this principal,
 #  as is the case when we want to look up group rights
         . "AND Principals.id = CachedGroupMembers.GroupId "
-        . "AND CachedGroupMembers.MemberId = "
-        . $self->Id . " "
+        . "AND CachedGroupMembers.MemberId = " . $self->Id . " "
 
         . "AND (" . join( ' OR ', map "Groups.Type = '$_'", @roles ) . ")";
 
     my (@object_clauses);
     foreach my $obj ( @{ $args{'EquivObjects'} } ) {
         my $type = ref($obj) ? ref($obj) : $obj;
-        my $id;
-        $id = eval { $obj->id };
 
         my $clause = "Groups.Domain = '$type-Role'";
 
         # XXX: Groups.Instance is VARCHAR in DB, we should quote value
         # if we want mysql 4.0 use indexes here. we MUST convert that
         # field to integer and drop this quotes.
-        $clause .= " AND Groups.Instance = '$id'" if $id;
+        if ( my $id = eval { $obj->id } ) {
+            $clause .= " AND Groups.Instance = '$id'";
+        }
         push @object_clauses, "($clause)";
     }
     $query .= " AND (" . join( ' OR ', @object_clauses ) . ")";

commit c886bb711ecf90f8de0dbd29d2ac784ef0700055
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Fri Oct 1 13:55:39 2010 -0400

    remove a redundant check on $args{Right}

diff --git a/lib/RT/Principal_Overlay.pm b/lib/RT/Principal_Overlay.pm
index b5fe771..c3998ec 100755
--- a/lib/RT/Principal_Overlay.pm
+++ b/lib/RT/Principal_Overlay.pm
@@ -255,11 +255,6 @@ sub HasRight {
         return 1;
     }
 
-    unless ( $args{'Right'} ) {
-        $RT::Logger->crit("HasRight called without a right");
-        return (undef);
-    }
-
     $args{'Right'} = RT::ACE->CanonicalizeRightName( $args{'Right'} );
     unless ( $args{'Right'} ) {
         $RT::Logger->error(

commit e0ae21da93c26ec77438ffd5f28211a4f3b4f7ef
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Fri Oct 1 14:53:42 2010 -0400

    perltidy

diff --git a/lib/RT/ACL_Overlay.pm b/lib/RT/ACL_Overlay.pm
index 9e7b511..9f8dab3 100755
--- a/lib/RT/ACL_Overlay.pm
+++ b/lib/RT/ACL_Overlay.pm
@@ -160,44 +160,49 @@ if IncludeGroupMembership => 1 is specified, ACEs which apply to the principal d
 
 sub LimitToPrincipal {
     my $self = shift;
-    my %args = ( Type                               => undef,
-                 Id                                 => undef,
+    my %args = ( Type                   => undef,
+                 Id                     => undef,
                  IncludeGroupMembership => undef,
-                 @_ );
+                 @_
+               );
     if ( $args{'IncludeGroupMembership'} ) {
         my $cgm = $self->NewAlias('CachedGroupMembers');
         $self->Join( ALIAS1 => 'main',
                      FIELD1 => 'PrincipalId',
                      ALIAS2 => $cgm,
-                     FIELD2 => 'GroupId' );
+                     FIELD2 => 'GroupId'
+                   );
         $self->Limit( ALIAS           => $cgm,
                       FIELD           => 'MemberId',
                       OPERATOR        => '=',
                       VALUE           => $args{'Id'},
-                      ENTRYAGGREGATOR => 'OR' );
-    }
-    else {
+                      ENTRYAGGREGATOR => 'OR'
+                    );
+    } else {
         if ( defined $args{'Type'} ) {
             $self->Limit( FIELD           => 'PrincipalType',
                           OPERATOR        => '=',
                           VALUE           => $args{'Type'},
-                          ENTRYAGGREGATOR => 'OR' );
+                          ENTRYAGGREGATOR => 'OR'
+                        );
+        }
+
+        # if the principal id points to a user, we really want to point
+        # to their ACL equivalence group. The machinations we're going through
+        # lead me to start to suspect that we really want users and groups
+        # to just be the same table. or _maybe_ that we want an object db.
+        my $princ = RT::Principal->new( RT->SystemUser );
+        $princ->Load( $args{'Id'} );
+        if ( $princ->PrincipalType eq 'User' ) {
+            my $group = RT::Group->new( RT->SystemUser );
+            $group->LoadACLEquivalenceGroup($princ);
+            $args{'Id'} = $group->PrincipalId;
         }
-    # if the principal id points to a user, we really want to point
-    # to their ACL equivalence group. The machinations we're going through
-    # lead me to start to suspect that we really want users and groups
-    # to just be the same table. or _maybe_ that we want an object db.
-    my $princ = RT::Principal->new(RT->SystemUser);
-    $princ->Load($args{'Id'});
-    if ($princ->PrincipalType eq 'User') {
-    my $group = RT::Group->new(RT->SystemUser);
-        $group->LoadACLEquivalenceGroup($princ);
-        $args{'Id'} = $group->PrincipalId;
-    }
         $self->Limit( FIELD           => 'PrincipalId',
                       OPERATOR        => '=',
                       VALUE           => $args{'Id'},
-                      ENTRYAGGREGATOR => 'OR' );
+                      ENTRYAGGREGATOR => 'OR'
+                    );
     }
 }
 

commit 69095043a251871f1e579a2ff180aa3cda9cce40
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Fri Oct 1 14:53:50 2010 -0400

    RT::ACL::LimitToObject update to be able to deal with object names as
    scalars

diff --git a/lib/RT/ACL_Overlay.pm b/lib/RT/ACL_Overlay.pm
index 9f8dab3..bfc35f6 100755
--- a/lib/RT/ACL_Overlay.pm
+++ b/lib/RT/ACL_Overlay.pm
@@ -87,26 +87,30 @@ Limit the ACL to rights for the object $object. It needs to be an RT::Record cla
 sub LimitToObject {
     my $self = shift;
     my $obj  = shift;
-    unless ( defined($obj)
-        && ref($obj)
-        && UNIVERSAL::can( $obj, 'id' )
-        && $obj->id )
-    {
-        return undef;
-    }
+
+    my $obj_type = ref($obj)||$obj;
+    my $obj_id = eval { $obj->id};
+
+    my $object_clause = 'possible_objects';
+    $self->_OpenParen($object_clause);
     $self->Limit(
+        SUBCLAUSE       => $object_clause,
         FIELD           => 'ObjectType',
         OPERATOR        => '=',
-        VALUE           => ref($obj),
-        ENTRYAGGREGATOR => 'OR'
+        VALUE           => (ref($obj)||$obj),
+        ENTRYAGGREGATOR => 'OR' # That "OR" applies to the separate objects we're searching on, not "Type Or ID"
     );
+    if ($obj_id) {
     $self->Limit(
+        SUBCLAUSE       => $object_clause,
         FIELD           => 'ObjectId',
         OPERATOR        => '=',
-        VALUE           => $obj->id,
-        ENTRYAGGREGATOR => 'OR',
+        VALUE           => $obj_id,
+        ENTRYAGGREGATOR => 'AND',
         QUOTEVALUE      => 0
     );
+    }
+    $self->_CloseParen($object_clause);
 
 }
 

commit 760d91a08577c005a393cfb12758861347fae7d3
Author: Jesse Vincent <jesse at bestpractical.com>
Date:   Sun Oct 3 23:23:21 2010 -0400

    A variant of a patch from Tim Cutts to cut down the work done per
    transaction in history display.

diff --git a/share/html/Ticket/Elements/ShowHistory b/share/html/Ticket/Elements/ShowHistory
index ac4dfbd..d0e7fc5 100755
--- a/share/html/Ticket/Elements/ShowHistory
+++ b/share/html/Ticket/Elements/ShowHistory
@@ -82,8 +82,16 @@ if ($ShowDisplayModes or $ShowTitle) {
 
 <div id="ticket-history">
 <%perl>
-my @attachments = @{$Attachments->ItemsArrayRef()};
-my @attachment_content = @{$AttachmentContent->ItemsArrayRef()};
+my $trans_content = {};
+my $trans_attachments = {};
+
+for my $content (@{$AttachmentContent->ItemsArrayRef()}) {
+    $trans_content->{$content->TransactionId}->{$content->Id} = $content;
+}
+
+for my $attachment (@{$Attachments->ItemsArrayRef()}) {
+    push (@{$trans_attachments->{$attachment->TransactionId}}, $attachment)
+}
 
 while ( my $Transaction = $Transactions->Next ) {
     my $skip = 0;
@@ -97,12 +105,6 @@ while ( my $Transaction = $Transactions->Next ) {
 
     $i++;
 
-    my @trans_attachments = grep { $_->TransactionId == $Transaction->Id } @attachments;
-
-    my $trans_content = {};
-    grep { ($_->TransactionId == $Transaction->Id ) && ($trans_content->{$_->Id} = $_)  } @attachment_content;
-
-   
     my $IsLastTransaction = 0;
     if ( RT->Config->Get( 'OldestTransactionsFirst', $session{'CurrentUser'} )){
         $IsLastTransaction = $Transactions->IsLast;
@@ -118,7 +120,7 @@ while ( my $Transaction = $Transactions->Next ) {
               Transaction          => $Transaction,
               ShowHeaders          => $ShowHeaders,
               RowNum               => $i,
-              Attachments          => \@trans_attachments,
+              Attachments          => $trans_attachments->{$Transaction->id},
               AttachmentContent    => $trans_content,
               LastTransaction      => $IsLastTransaction
  );
diff --git a/share/html/Ticket/Elements/ShowTransactionAttachments b/share/html/Ticket/Elements/ShowTransactionAttachments
index 4151cae..ab904eb 100644
--- a/share/html/Ticket/Elements/ShowTransactionAttachments
+++ b/share/html/Ticket/Elements/ShowTransactionAttachments
@@ -191,8 +191,9 @@ my $render_attachment = sub {
         {
 
             my $content;
-            if ( $AttachmentContent->{ $message->id } ) {
-                $content = $AttachmentContent->{ $message->id }->Content;
+            # If we've cached the content, use it from there
+            if (my $x = $AttachmentContent->{ $Transaction->id }->{$message->id}) {
+                $content = $x->Content;
             }
             else {
                 $content = $message->Content;

commit 5517022af2fd1eab715e33013920b0beae148de0
Author: Christian Loos <cloos at netsandbox.de>
Date:   Fri Sep 10 23:51:28 2010 +0200

    cleanup the load saved search select list
    
    - don't show objects without searches
    - use optgroup to display the objects

diff --git a/share/html/Search/Elements/SelectSearchesForObjects b/share/html/Search/Elements/SelectSearchesForObjects
index 120f36b..c3202ac 100644
--- a/share/html/Search/Elements/SelectSearchesForObjects
+++ b/share/html/Search/Elements/SelectSearchesForObjects
@@ -51,19 +51,22 @@ $Name => undef
 $SearchType => 'Ticket',
 </%args>
 <select id="<%$Name%>" name="<%$Name%>">
+<option value="">-</option>
 % foreach my $object (@Objects) {
+% my @searches = $object->Attributes->Named('SavedSearch');
+% if ( @searches ) {
 % if (ref($object) eq 'RT::User' && $object->id == $session{'CurrentUser'}->Id) {
-<option value=""><&|/l&>My saved searches</&></option>
+<optgroup label="<&|/l&>My saved searches</&>">
 % } else {
-<option value=""></option>
-<option value=""><&|/l, $object->Name&>[_1]'s saved searches</&></option>
+<optgroup label="<&|/l, $object->Name&>[_1]'s saved searches</&>">
 % }
-% my @searches = $object->Attributes->Named('SavedSearch');
 % foreach my $search (@searches) { 
 %     # Skip it if it is not of search type we want.
 %     next if ($search->SubValue('SearchType')
 %              && $search->SubValue('SearchType') ne $SearchType);
-<option value="<%ref($object)%>-<%$object->id%>-SavedSearch-<%$search->Id%>"> -<%$search->Description||loc('Unnamed search')%></option>
+<option value="<%ref($object)%>-<%$object->id%>-SavedSearch-<%$search->Id%>"><%$search->Description||loc('Unnamed search')%></option>
+% }
+</optgroup>
 % }
 % }
 </select>

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


More information about the Rt-commit mailing list