[Rt-commit] rt branch, 4.2/confirm-loaded-queue, updated. rt-4.2.4-20-gb30020a

Alex Vandiver alexmv at bestpractical.com
Fri May 16 01:10:26 EDT 2014


The branch, 4.2/confirm-loaded-queue has been updated
       via  b30020aa119ef32131c4c80e2109a30720cc8f79 (commit)
       via  4607681b563aed95d0b77a77875915dc70957401 (commit)
       via  9045cdd3a58468a13a22176ba565046daa99d5c9 (commit)
       via  f01afb877e171a8e95b9586e97dfbecde840f7a8 (commit)
       via  3788025756028dad93f3a2a7bdaee22e6256120d (commit)
       via  3dce69054094c9b104494943754c738373661a87 (commit)
       via  bba5d6780ae285c44f69613441965e6679b0c7a3 (commit)
       via  4daccd2496e1dc6dd5654844464aa5b26e8ab3d2 (commit)
       via  c5e4da6829ef63f9a4c4d58cc4bcee42883f0b15 (commit)
       via  7463ce8599788c7460f651e8eb23d5dcd230c2b4 (commit)
       via  20f91394415cfbf76cd0fe347918b0fdfa31ad81 (commit)
      from  a38b879928f021e6c28ee2149a0c9661a6eb07ff (commit)

Summary of changes:
 lib/RT/Action/CreateTickets.pm           |  16 +-
 lib/RT/CustomField.pm                    | 183 +++++++++----
 lib/RT/Handle.pm                         |   2 +-
 lib/RT/Migrate/Importer.pm               |   2 +-
 lib/RT/Queue.pm                          |   6 +-
 lib/RT/SearchBuilder/AddAndSort.pm       |   3 +-
 lib/RT/Test.pm                           |   6 +-
 lib/RT/Test/Web.pm                       |   6 +-
 lib/RT/Ticket.pm                         |   8 +-
 lib/RT/Tickets.pm                        |   7 +-
 share/html/REST/1.0/Forms/ticket/default |  21 +-
 t/api/customfield.t                      | 425 +++++++++++++++++++++++++++----
 t/web/query_builder.t                    |   2 +-
 13 files changed, 563 insertions(+), 124 deletions(-)

- Log -----------------------------------------------------------------
commit 20f91394415cfbf76cd0fe347918b0fdfa31ad81
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Fri May 16 00:44:05 2014 -0400

    These statements simplify to "skip if the queue is undef or 0"

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 08f7ad8..0e4a476 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -414,10 +414,12 @@ sub LoadByName {
         return wantarray ? (0, $self->loc("No name provided")) : 0;
     }
 
-    # if we're looking for a queue by name, make it a number
-    # Exclude 0 since it is a valid parameter, but will not load a valid queue
-    if ( defined $args{'Queue'}
-         && ($args{'Queue'} =~ /^\d+$/ ? $args{'Queue'} != 0 : 1)
+    # Resolve the Queue; this is necessary to properly limit ObjectId,
+    # and also possibly useful to set a ContextObj if we are currently
+    # lacking one.  It is unnecessary if we have a context object and
+    # were passed a numeric Queue.  Also skip if we have a false
+    # Queue, which means "global" (0) or "not a queue" (undef)
+    if ( $args{Queue}
          && ($args{'Queue'} =~ /\D/ || !$self->ContextObject) ) {
         my $QueueObj = RT::Queue->new( $self->CurrentUser );
         my ($ret, $msg) = $QueueObj->Load( $args{'Queue'} );

commit 7463ce8599788c7460f651e8eb23d5dcd230c2b4
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Fri May 16 00:47:30 2014 -0400

    Short-circuit if a bogus Queue was passed

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 0e4a476..abe4114 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -435,6 +435,9 @@ sub LoadByName {
         }
         else {
             RT::Logger->warning("Unable to load queue with id " . $args{'Queue'});
+            # Since we don't also include global results, there's no
+            # point in searching; abort
+            return wantarray ? (0, $self->loc("Not found")) : 0;
         }
     }
     if ( defined $args{'Queue'} ) {

commit c5e4da6829ef63f9a4c4d58cc4bcee42883f0b15
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Fri May 16 00:48:20 2014 -0400

    Simplify logic by loading the queue even if not strictly necessary
    
    The existing logic i nthe "if" statement serves to skip the block in
    precisely one situation -- the passed Queue is numeric, and there
    already exists a ContextObject.  In such situation, it is not necessary
    to attempt to load the Queue -- but it is not harmful, either.  In fact,
    it may allow a better warning in cases where a numeric but non-existant
    Queue is passed in.  It also clarifies the logic immeasurably.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index abe4114..baa8ca3 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -416,11 +416,11 @@ sub LoadByName {
 
     # Resolve the Queue; this is necessary to properly limit ObjectId,
     # and also possibly useful to set a ContextObj if we are currently
-    # lacking one.  It is unnecessary if we have a context object and
-    # were passed a numeric Queue.  Also skip if we have a false
-    # Queue, which means "global" (0) or "not a queue" (undef)
-    if ( $args{Queue}
-         && ($args{'Queue'} =~ /\D/ || !$self->ContextObject) ) {
+    # lacking one.  It is not strictly necessary if we have a context
+    # object and were passed a numeric Queue, but it cannot hurt to
+    # verify its sanity.  Skip if we have a false Queue, which means
+    # "global" (0) or "not a queue" (undef)
+    if ($args{Queue}) {
         my $QueueObj = RT::Queue->new( $self->CurrentUser );
         my ($ret, $msg) = $QueueObj->Load( $args{'Queue'} );
 

commit 4daccd2496e1dc6dd5654844464aa5b26e8ab3d2
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Fri May 16 00:54:16 2014 -0400

    Move LookupType defaulting earlier

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index baa8ca3..31f9d5e 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -414,6 +414,13 @@ sub LoadByName {
         return wantarray ? (0, $self->loc("No name provided")) : 0;
     }
 
+    if ( defined $args{'Queue'} ) {
+        # Set a LookupType for backcompat, otherwise we'll calculate
+        # one of RT::Queue from your ContextObj.  Older code was relying
+        # on us defaulting to RT::Queue-RT::Ticket in old LimitToQueue call.
+        $args{LookupType} ||= 'RT::Queue-RT::Ticket';
+    }
+
     # Resolve the Queue; this is necessary to properly limit ObjectId,
     # and also possibly useful to set a ContextObj if we are currently
     # lacking one.  It is not strictly necessary if we have a context
@@ -440,12 +447,6 @@ sub LoadByName {
             return wantarray ? (0, $self->loc("Not found")) : 0;
         }
     }
-    if ( defined $args{'Queue'} ) {
-        # Set a LookupType for backcompat, otherwise we'll calculate
-        # one of RT::Queue from your ContextObj.  Older code was relying
-        # on us defaulting to RT::Queue-RT::Ticket in old LimitToQueue call.
-        $args{LookupType} ||= 'RT::Queue-RT::Ticket';
-    }
 
     # XXX - really naive implementation.  Slow. - not really. still just one query
 

commit bba5d6780ae285c44f69613441965e6679b0c7a3
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu May 15 22:47:41 2014 -0400

    Allow for non-Queue context objects
    
    This opens the door for using LoadByName for custom fields not on
    Transactions, Tickets, or Queues.  Callers specify a LookupType and
    ObjectId, from which the class to load (if not RT::Queue) can be
    inferred.  Consequently, using LoadByName for User (or Asset) custom
    fields no longer requires writing an alternate and parallel
    implementation of LoadByName.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 31f9d5e..e6a0d76 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -378,20 +378,37 @@ sub Load {
 
 
 
-=head2 LoadByName (Queue => QUEUEID, Name => NAME)
+=head2 LoadByName Name => C<NAME>, [...]
 
-Loads the Custom field named NAME.
+Loads the Custom field named NAME.  As other optional parameters, takes:
 
-Will load a Disabled Custom Field even if there is a non-disabled Custom Field
-with the same Name.
+=over
 
-If a Queue parameter is specified, only look for ticket custom fields tied to that Queue.
+=item LookupType => C<LOOKUPTYPE>
 
-If the Queue parameter is '0', look for global ticket custom fields.
+The type of Custom Field to look for; while this parameter is not
+required, it is highly suggested, or you may not find the Custom Field
+you are expecting.  It should be passed a C<LookupType> such as
+L<RT::Ticket/CustomFieldLookupType> or
+L<RT::User/CustomFieldLookupType>.
 
-If no queue parameter is specified, look for any and all custom fields with this name.
+=item ObjectType => C<CLASS>
 
-BUG/TODO, this won't let you specify that you only want user or group CFs.
+The class of object that the custom field is applied to.  This can be
+intuited from the provided C<LookupType>.
+
+=item ObjectId => C<ID>
+
+limits the custom field search to one applied to the relevant id.  For
+example, if a C<LookupType> of C<< RT::Ticket->CustomFieldLookupType >>
+is used, this is which Queue the CF must be applied to.  Pass 0 to only
+search custom fields that are applied globally.
+
+=back
+
+For backwards compatibility, a value passed for C<Queue> is equivalent
+to specifying a C<LookupType> of L<RT::Ticket/CustomFieldLookupType>,
+and a C<ObjectId> of the value passed as C<Queue>.
 
 =cut
 
@@ -403,9 +420,14 @@ BUG/TODO, this won't let you specify that you only want user or group CFs.
 sub LoadByName {
     my $self = shift;
     my %args = (
-        Queue => undef,
-        Name  => undef,
+        Name       => undef,
         LookupType => undef,
+        ObjectType => undef,
+        ObjectId   => undef,
+
+        # Back-compat
+        Queue => undef,
+
         @_,
     );
 
@@ -419,37 +441,43 @@ sub LoadByName {
         # one of RT::Queue from your ContextObj.  Older code was relying
         # on us defaulting to RT::Queue-RT::Ticket in old LimitToQueue call.
         $args{LookupType} ||= 'RT::Queue-RT::Ticket';
-    }
-
-    # Resolve the Queue; this is necessary to properly limit ObjectId,
-    # and also possibly useful to set a ContextObj if we are currently
-    # lacking one.  It is not strictly necessary if we have a context
-    # object and were passed a numeric Queue, but it cannot hurt to
-    # verify its sanity.  Skip if we have a false Queue, which means
-    # "global" (0) or "not a queue" (undef)
-    if ($args{Queue}) {
-        my $QueueObj = RT::Queue->new( $self->CurrentUser );
-        my ($ret, $msg) = $QueueObj->Load( $args{'Queue'} );
-
-        # Only set the context object if we successfully loaded a queue.
-        # This avoids creating a LookupType of RT::Queue below based on an empty
-        # queue object.
+        $args{ObjectId}   //= delete $args{Queue};
+    }
+
+    # Default the ObjectType to the top category of the LookupType; it's
+    # what the CFs are assigned on.
+    $args{ObjectType} ||= $1 if $args{LookupType} and $args{LookupType} =~ /^([^-]+)/;
+
+    # Resolve the ObjectId/ObjectType; this is necessary to properly
+    # limit ObjectId, and also possibly useful to set a ContextObj if we
+    # are currently lacking one.  It is not strictly necessary if we
+    # have a context object and were passed a numeric ObjectId, but it
+    # cannot hurt to verify its sanity.  Skip if we have a false
+    # ObjectId, which means "global", or if we lack an ObjectType
+    if ($args{ObjectId} and $args{ObjectType}) {
+        my ($obj, $ok, $msg);
+        eval {
+            $obj = $args{ObjectType}->new( $self->CurrentUser );
+            ($ok, $msg) = $obj->Load( $args{ObjectId} );
+        };
 
-        if ( $ret ){
-            $args{'Queue'} = $QueueObj->Id;
-            $self->SetContextObject( $QueueObj )
+        if ($ok) {
+            $args{ObjectId} = $obj->id;
+            $self->SetContextObject( $obj )
                 unless $self->ContextObject;
-        }
-        else {
-            RT::Logger->warning("Unable to load queue with id " . $args{'Queue'});
+        } else {
+            $RT::Logger->warning("Failed to load $args{ObjectType} '$args{ObjectId}'");
             # Since we don't also include global results, there's no
             # point in searching; abort
             return wantarray ? (0, $self->loc("Not found")) : 0;
         }
+    } elsif (not $args{ObjectType} and $args{ObjectId}) {
+        # If we skipped out on the above due to lack of ObjectType, make
+        # sure we clear out ObjectId of anything lingering
+        $RT::Logger->warning("No LookupType or ObjectType passed; ignoring ObjectId");
+        delete $args{ObjectId};
     }
 
-    # XXX - really naive implementation.  Slow. - not really. still just one query
-
     my $CFs = RT::CustomFields->new( $self->CurrentUser );
     $CFs->SetContextObject( $self->ContextObject );
     my $field = $args{'Name'} =~ /\D/? 'Name' : 'id';
@@ -463,16 +491,21 @@ sub LoadByName {
             ($self->ContextObject, $self->ContextObject->ACLEquivalenceObjects) ]
         if $self->ContextObject;
 
+    # Apply LookupType limits
     $args{LookupType} = [ $args{LookupType} ]
         if $args{LookupType} and not ref($args{LookupType});
     $CFs->Limit( FIELD => "LookupType", OPERATOR => "IN", VALUE => $args{LookupType} )
         if $args{LookupType};
 
-    # Don't limit to queue if queue is 0.  Trying to do so breaks
-    # RT::Group type CFs.
-    if ( defined $args{'Queue'} ) {
-        # don't use LimitToQueue because it forces a LookupType
-        $CFs->Limit ( ALIAS => $CFs->_OCFAlias, FIELD => 'ObjectId', VALUE => $args{'Queue'} );
+    # Limit to the specified object.  For backwards compatibility, we
+    # limit even for ObjectId 0, forcing callers to try loading twice to
+    # find Queue-specific and then Global CFs.
+    if (defined $args{ObjectId}) {
+        $CFs->Limit(
+            ALIAS => $CFs->_OCFAlias,
+            FIELD => 'ObjectId',
+            VALUE => $args{ObjectId},
+        );
     }
 
     # When loading by name, we _can_ load disabled fields, but prefer

commit 3dce69054094c9b104494943754c738373661a87
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu May 15 22:49:49 2014 -0400

    Allow callers to not get disabled CFs
    
    For backwards compatibility, LoadByName has always maintained the
    property that it may return a disabled custom field -- though it does so
    only if there are no matching enabled fields.
    
    Provide an option to control this behavior; the default is maintained as
    it was previously, for backwards compatibility.
    
    Note that previously, the only OrderBy was the Disabled flag -- this
    means that if there were two non-disabled custom fields with the same
    name, which one was returned was possibily non-deterministic.  This
    commit places a predictable ordering on the resolution; namely, lower
    SortOrder and lower id take precedence.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index e6a0d76..1e2b123 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -404,12 +404,21 @@ example, if a C<LookupType> of C<< RT::Ticket->CustomFieldLookupType >>
 is used, this is which Queue the CF must be applied to.  Pass 0 to only
 search custom fields that are applied globally.
 
+=item IncludeDisabled => C<BOOLEAN>
+
+Whether it should return Disabled custom fields if they match; defaults
+to on, though non-Disabled custom fields are returned preferentially.
+
 =back
 
 For backwards compatibility, a value passed for C<Queue> is equivalent
 to specifying a C<LookupType> of L<RT::Ticket/CustomFieldLookupType>,
 and a C<ObjectId> of the value passed as C<Queue>.
 
+If multiple custom fields match the above constraints, the first
+according to C<SortOrder> will be returned; ties are broken by C<id>,
+lowest-first.
+
 =cut
 
 # Compatibility for API change after 3.0 beta 1
@@ -425,6 +434,8 @@ sub LoadByName {
         ObjectType => undef,
         ObjectId   => undef,
 
+        IncludeDisabled => 1,
+
         # Back-compat
         Queue => undef,
 
@@ -497,9 +508,16 @@ sub LoadByName {
     $CFs->Limit( FIELD => "LookupType", OPERATOR => "IN", VALUE => $args{LookupType} )
         if $args{LookupType};
 
-    # Limit to the specified object.  For backwards compatibility, we
-    # limit even for ObjectId 0, forcing callers to try loading twice to
-    # find Queue-specific and then Global CFs.
+    # Default to by SortOrder and id; this mirrors the standard ordering
+    # of RT::CustomFields (minus the Name, which is guaranteed to be
+    # fixed)
+    my @order = (
+        { FIELD => 'SortOrder',
+          ORDER => 'ASC' },
+        { FIELD => 'id',
+          ORDER => 'ASC' },
+    );
+
     if (defined $args{ObjectId}) {
         $CFs->Limit(
             ALIAS => $CFs->_OCFAlias,
@@ -508,12 +526,16 @@ sub LoadByName {
         );
     }
 
-    # When loading by name, we _can_ load disabled fields, but prefer
-    # non-disabled fields.
-    $CFs->FindAllRows;
-    $CFs->OrderByCols(
-        { FIELD => "Disabled", ORDER => 'ASC' },
-    );
+    if ($args{IncludeDisabled}) {
+        # Load disabled fields, but return them only as a last resort.
+        # This goes at the front of @order, as we prefer the
+        # non-disabled global CF to the disabled Queue-specific CF.
+        $CFs->FindAllRows;
+        unshift @order, { FIELD => "Disabled", ORDER => 'ASC' };
+    }
+
+    # Apply the above orderings
+    $CFs->OrderByCols( @order );
 
     # We only want one entry.
     $CFs->RowsPerPage(1);

commit 3788025756028dad93f3a2a7bdaee22e6256120d
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu May 15 22:50:56 2014 -0400

    Allow callers to look in Queue and global CFs
    
    Previously, in order to respect global CFs, callers were forced to
    explicitly attempt to LoadByName with a queue, and then attempt again
    with Queue => 0 if it failed.
    
    Provide an option, defaulting to off, to first search queue CFs,
    followed by global CFs.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 1e2b123..29d9424 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -409,6 +409,12 @@ search custom fields that are applied globally.
 Whether it should return Disabled custom fields if they match; defaults
 to on, though non-Disabled custom fields are returned preferentially.
 
+=item IncludeGlobal => C<BOOLEAN>
+
+Whether to also search global custom fields, even if a value is provided
+for C<ObjectId>; defaults to off.  Non-global custom fields are returned
+preferentially.
+
 =back
 
 For backwards compatibility, a value passed for C<Queue> is equivalent
@@ -435,6 +441,7 @@ sub LoadByName {
         ObjectId   => undef,
 
         IncludeDisabled => 1,
+        IncludeGlobal   => 0,
 
         # Back-compat
         Queue => undef,
@@ -478,9 +485,15 @@ sub LoadByName {
                 unless $self->ContextObject;
         } else {
             $RT::Logger->warning("Failed to load $args{ObjectType} '$args{ObjectId}'");
-            # Since we don't also include global results, there's no
-            # point in searching; abort
-            return wantarray ? (0, $self->loc("Not found")) : 0;
+            if ($args{IncludeGlobal}) {
+                # Fall back to acting like we were only asked about the
+                # global case
+                $args{ObjectId} = 0;
+            } else {
+                # If they didn't also want global results, there's no
+                # point in searching; abort
+                return wantarray ? (0, $self->loc("Not found")) : 0;
+            }
         }
     } elsif (not $args{ObjectType} and $args{ObjectId}) {
         # If we skipped out on the above due to lack of ObjectType, make
@@ -519,11 +532,22 @@ sub LoadByName {
     );
 
     if (defined $args{ObjectId}) {
-        $CFs->Limit(
-            ALIAS => $CFs->_OCFAlias,
-            FIELD => 'ObjectId',
-            VALUE => $args{ObjectId},
-        );
+        if ($args{IncludeGlobal}) {
+            $CFs->Limit(
+                ALIAS    => $CFs->_OCFAlias,
+                FIELD    => 'ObjectId',
+                OPERATOR => 'IN',
+                VALUE    => [ $args{ObjectId}, 0 ],
+            );
+            # Find the queue-specific first
+            unshift @order, { ALIAS => $ocfs, FIELD => "ObjectId", ORDER => "DESC" };
+        } else {
+            $CFs->Limit(
+                ALIAS => $CFs->_OCFAlias,
+                FIELD => 'ObjectId',
+                VALUE => $args{ObjectId},
+            );
+        }
     }
 
     if ($args{IncludeDisabled}) {

commit f01afb877e171a8e95b9586e97dfbecde840f7a8
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu May 15 22:51:32 2014 -0400

    Optimize the query slightly, by eliminating an unnecessary DISTINCT
    
    The join from CustomFields to ObjectCustomFields is guaranteed to be
    distinct, by dint of the fat that either it is global, or it matches the
    objectid we desire; as such, we can eliminate a cumbersome DISTINCT by
    informing DBIx::SearchBuilder of this property at JOIN time.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 29d9424..b2563e8 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -532,9 +532,13 @@ sub LoadByName {
     );
 
     if (defined $args{ObjectId}) {
+        # The join to OCFs is distinct -- either we have a global
+        # application or an objectid match, but never both.  Even if
+        # this were not the case, we care only for the first row.
+        my $ocfs = $CFs->_OCFAlias( Distinct => 1);
         if ($args{IncludeGlobal}) {
             $CFs->Limit(
-                ALIAS    => $CFs->_OCFAlias,
+                ALIAS    => $ocfs,
                 FIELD    => 'ObjectId',
                 OPERATOR => 'IN',
                 VALUE    => [ $args{ObjectId}, 0 ],
@@ -543,7 +547,7 @@ sub LoadByName {
             unshift @order, { ALIAS => $ocfs, FIELD => "ObjectId", ORDER => "DESC" };
         } else {
             $CFs->Limit(
-                ALIAS => $CFs->_OCFAlias,
+                ALIAS => $ocfs,
                 FIELD => 'ObjectId',
                 VALUE => $args{ObjectId},
             );
diff --git a/lib/RT/SearchBuilder/AddAndSort.pm b/lib/RT/SearchBuilder/AddAndSort.pm
index 9eae80d..db8b417 100644
--- a/lib/RT/SearchBuilder/AddAndSort.pm
+++ b/lib/RT/SearchBuilder/AddAndSort.pm
@@ -196,7 +196,7 @@ this join. Use Left to create LEFT JOIN rather than inner.
 sub JoinTargetToThis {
     my $self = shift;
     my $collection = shift;
-    my %args = ( New => 0, Left => 0, @_ );
+    my %args = ( New => 0, Left => 0, Distinct => 0, @_ );
 
     my $table = $self->Table;
     my $key = "_sql_${table}_alias";
@@ -209,6 +209,7 @@ sub JoinTargetToThis {
         FIELD1 => 'id',
         TABLE2 => $table,
         FIELD2 => $self->RecordClass->TargetField,
+        DISTINCT => $args{Distinct},
     );
     return $alias if $args{'New'};
     return $collection->{ $key } = $alias;

commit 9045cdd3a58468a13a22176ba565046daa99d5c9
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu May 15 22:52:03 2014 -0400

    Document but deprecate the old names for LoadByName
    
    While it is tempting to officially deprecate these, they are in use in
    core RT and in extensions, and introducing new deprecations during a
    stable series is unwise.  Suffice to document them as deprecated, and
    migrate exsting uses away from the, though not produce deprecation
    warnings.

diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index b2563e8..5e2b90b 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -425,6 +425,12 @@ If multiple custom fields match the above constraints, the first
 according to C<SortOrder> will be returned; ties are broken by C<id>,
 lowest-first.
 
+=head2 LoadNameAndQueue
+
+=head2 LoadByNameAndQueue
+
+Deprecated alternate names for L</LoadByName>.
+
 =cut
 
 # Compatibility for API change after 3.0 beta 1
diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 0a642bf..b2e22cc 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -449,7 +449,7 @@ sub CustomField {
     my $self = shift;
     my $name = shift;
     my $cf = RT::CustomField->new($self->CurrentUser);
-    $cf->LoadByNameAndQueue(Name => $name, Queue => $self->Id); 
+    $cf->LoadByName(Name => $name, Queue => $self->Id);
     return ($cf);
 }
 
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index c804270..a2a5c9a 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -2975,8 +2975,8 @@ sub LoadCustomFieldByIdentifier {
 
     my $cf = RT::CustomField->new( $self->CurrentUser );
     $cf->SetContextObject( $self );
-    $cf->LoadByNameAndQueue( Name => $field, Queue => $self->Queue );
-    $cf->LoadByNameAndQueue( Name => $field, Queue => 0 ) unless $cf->id;
+    $cf->LoadByName( Name => $field, Queue => $self->Queue );
+    $cf->LoadByName( Name => $field, Queue => 0 ) unless $cf->id;
     return $cf;
 }
 
diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index a898e17..2defde4 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -2189,7 +2189,7 @@ sub LimitCustomField {
         $CF->Load( $args{CUSTOMFIELD} );
     }
     else {
-        $CF->LoadByNameAndQueue(
+        $CF->LoadByName(
             Name  => $args{CUSTOMFIELD},
             Queue => $args{QUEUE}
         );

commit 4607681b563aed95d0b77a77875915dc70957401
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu May 15 22:52:32 2014 -0400

    Add tests for the new LoadByName functionality
    
    This throughly exercises the new codepath.  It leaves the existing
    t/customfields/api.t untouched, though it may be sensible to merge them
    in the future.

diff --git a/t/api/customfield.t b/t/api/customfield.t
index 0e635a6..df8f66d 100644
--- a/t/api/customfield.t
+++ b/t/api/customfield.t
@@ -2,80 +2,393 @@
 use strict;
 use warnings;
 use RT;
-use RT::Test nodata => 1, tests => undef;
+use RT::Test tests => undef;
 use Test::Warn;
 
+use_ok('RT::CustomField');
 
-{
+my $queue = RT::Queue->new( RT->SystemUser );
+$queue->Load( "General" );
+ok( $queue->id, "found the General queue" );
 
-use_ok('RT::CustomField');
-ok(my $cf = RT::CustomField->new(RT->SystemUser));
-ok(my ($id, $msg)=  $cf->Create( Name => 'TestingCF',
-                                 Queue => '0',
-                                 SortOrder => '1',
-                                 Description => 'A Testing custom field',
-                                 Type=> 'SelectSingle'), 'Created a global CustomField');
-isnt($id , 0, 'Global custom field correctly created');
-ok ($cf->SingleValue);
-is($cf->Type, 'Select');
-is($cf->MaxValues, 1);
-
-(my $val, $msg) = $cf->SetMaxValues('0');
-ok($val, $msg);
-is($cf->Type, 'Select');
-is($cf->MaxValues, 0);
-ok(!$cf->SingleValue );
-ok(my ($bogus_val, $bogus_msg) = $cf->SetType('BogusType') , "Trying to set a custom field's type to a bogus type");
-is($bogus_val , 0, "Unable to set a custom field's type to a bogus type");
-
-ok(my $bad_cf = RT::CustomField->new(RT->SystemUser));
-ok(my ($bad_id, $bad_msg)=  $cf->Create( Name => 'TestingCF-bad',
-                                 Queue => '0',
-                                 SortOrder => '1',
-                                 Description => 'A Testing custom field with a bogus Type',
-                                 Type=> 'SelectSingleton'), 'Created a global CustomField with a bogus type');
-is($bad_id , 0, 'Global custom field correctly decided to not create a cf with a bogus type ');
-
-}
-
-{
-
-ok(my $cf = RT::CustomField->new(RT->SystemUser));
+my $cf = RT::CustomField->new(RT->SystemUser);
+ok($cf, "Have a CustomField object");
+
+# Use the old Queue field to set up a ticket CF
+my ($ok, $msg) =  $cf->Create(
+    Name        => 'TestingCF',
+    Queue       => '0',
+    Description => 'A Testing custom field',
+    Type        => 'SelectSingle'
+);
+ok($ok, 'Global custom field correctly created');
+is($cf->Type, 'Select', "Is a select CF");
+ok($cf->SingleValue, "Also a single-value CF");
+is($cf->MaxValues, 1, "...meaning only one value, max");
+
+($ok, $msg) = $cf->SetMaxValues('0');
+ok($ok, "Set to infinite values: $msg");
+is($cf->Type, 'Select', "Still a select CF");
+ok( ! $cf->SingleValue, "No longer single-value" );
+is($cf->MaxValues, 0, "...meaning no maximum values");
+
+# Test our sanity checking of CF types
+($ok, $msg) = $cf->SetType('BogusType');
+ok( ! $ok, "Unable to set a custom field's type to a bogus type: $msg");
+
+$cf = RT::CustomField->new(RT->SystemUser);
+($ok, $msg) = $cf->Create(
+    Name => 'TestingCF-bad',
+    Queue => '0',
+    SortOrder => '1',
+    Description => 'A Testing custom field with a bogus Type',
+    Type=> 'SelectSingleton'
+);
+ok( ! $ok, "Correctly could not create with bogus type: $msg");
+
+
+# Deprecated types
+warning_like {
+    ok($cf->ValidateType('SelectSingle'), "ValidateType accepts SelectSingle");
+} qr/deprecated/, "...but warns of deprecation";
+
+warning_like {
+    ok($cf->ValidateType('SelectMultiple'), "ValidateType accepts SelectMultiple");
+} qr/deprecated/, "...but warns of deprecation";
+
+warning_like {
+    ok( ! $cf->ValidateType('SelectFooMultiple'), "ValidateType does not accept SelectFooMultiple");
+} qr/deprecated/, "...and also warns of deprecation";
+
+
+# Test adding and removing CFVs
 $cf->Load(1);
-is($cf->Id , 1);
-ok(my ($val,$msg)  = $cf->AddValue(Name => 'foo' , Description => 'TestCFValue', SortOrder => '6'));
-isnt($val , 0);
-ok (my ($delval, $delmsg) = $cf->DeleteValue($val));
-ok ($delval,"Deleting a cf value: $delmsg");
+($ok, $msg) = $cf->AddValue(Name => 'foo' , Description => 'TestCFValue', SortOrder => '6');
+ok($ok, "Added a new value to the select options");
+($ok, $msg) = $cf->DeleteValue($ok);
+ok($ok, "Deleting it again");
+
+
+# Loading, and context objects
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName( Name => "TestingCF" );
+ok($cf->id, "Load finds it, given just a name" );
+ok( ! $cf->ContextObject, "Did not get a context object");
+
+# Old Queue => form should find the global, gain no context object
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 0);
+ok($cf->id, "Load finds it, given a Name and Queue => 0" );
+ok( ! $cf->ContextObject, 'Context object not set when queue is 0');
+
+# We don't default to also searching global -- but do pick up a contextobject
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 1);
+ok( ! $cf->id, "Load does not finds it, given a Name and Queue => 1" );
+ok($cf->ContextObject->id, 'Context object is now set');
 
+# If we IncludeGlobal, we find it
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 1, IncludeGlobal => 1 );
+ok($cf->id, "Load now finds it, given a Name and Queue => 1 and IncludeGlobal" );
+ok($cf->ContextObject->id, 'Context object is also set');
 
-}
+# The explicit LookupType works
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', LookupType => RT::Ticket->CustomFieldLookupType );
+ok($cf->id, "Load now finds it, given a Name and LookupType" );
+ok( ! $cf->ContextObject, 'No context object gained');
 
-{
+# The explicit LookupType, ObjectId, and IncludeGlobal -- what most folks want
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', LookupType => RT::Ticket->CustomFieldLookupType,
+                ObjectId => 1, IncludeGlobal => 1 );
+ok($cf->id, "Load now finds it, given a Name, LookupType, ObjectId, IncludeGlobal" );
+ok($cf->ContextObject->id, 'And gains a context obj');
 
-ok(my $cf = RT::CustomField->new(RT->SystemUser));
+# Look for a queue by name
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => "General" );
+ok( ! $cf->id, "No IncludeGlobal, so queue by name fails" );
+ok($cf->ContextObject->id, 'But gains a context object');
 
+# Look for a queue by name, include global
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => "General", IncludeGlobal => 1 );
+ok($cf->id, "By name, and queue name works with IncludeGlobal" );
+ok($cf->ContextObject->id, 'And gains a context object');
+
+
+
+# A bogus Queue gets you no results, but a warning
+$cf = RT::CustomField->new( RT->SystemUser );
 warning_like {
-ok($cf->ValidateType('SelectSingle'));
-} qr/deprecated/;
+    $cf->LoadByName(Name => 'TestingCF', Queue => "Bogus" );
+    ok( ! $cf->id, "With a bogus queue name gets no results" );
+    ok( ! $cf->ContextObject, 'And also no context object');
+} qr/Failed to load RT::Queue 'Bogus'/, "Generates a warning";
 
+# Ditto by number which is bogus
+$cf = RT::CustomField->new( RT->SystemUser );
 warning_like {
-ok($cf->ValidateType('SelectMultiple'));
-} qr/deprecated/;
+    $cf->LoadByName(Name => 'TestingCF', Queue => "9000" );
+    ok( ! $cf->id, "With a bogus queue number gets no results" );
+    ok( ! $cf->ContextObject, 'And also no context object');
+} qr/Failed to load RT::Queue '9000'/, "Generates a warning";
 
+# But if they also wanted global results, we might have an answer
+$cf = RT::CustomField->new( RT->SystemUser );
 warning_like {
-ok(!$cf->ValidateType('SelectFooMultiple'));
-} qr/deprecated/;
+    $cf->LoadByName(Name => 'TestingCF', Queue => "9000", IncludeGlobal => 1 );
+    ok($cf->id, "Bogus queue but IncludeGlobal founds it" );
+    ok( ! $cf->ContextObject, 'But no context object');
+} qr/Failed to load RT::Queue '9000'/, "And generates a warning";
+
+
+# Make it only apply to one queue
+$cf->Load(1);
+my $ocf = RT::ObjectCustomField->new( RT->SystemUser );
+( $ok, $msg ) = $ocf->LoadByCols( CustomField => $cf->id, ObjectId => 0 );
+ok( $ok, "Found global application of CF" );
+( $ok, $msg ) = $ocf->Delete;
+ok( $ok, "...and deleted it");
+( $ok, $msg ) = $ocf->Add( CustomField => $cf->id, ObjectId => 1 );
+ok($ok, "Applied to just queue 1" );
+
+# Looking for it globally with Queue => 0 should fail, gain no context object
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 0);
+ok( ! $cf->id, "Load fails to find, given a Name and Queue => 0" );
+ok( ! $cf->ContextObject, 'Context object not set when queue is 0');
+
+# Looking it up by Queue => 1 works fine, and gets context object
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 1);
+ok($cf->id, "Load does finds it, given a Name and Queue => 1" );
+ok($cf->ContextObject->id, 'Context object is now set');
+
+# Also find it with IncludeGlobal
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 1, IncludeGlobal => 1 );
+ok($cf->id, "Load also finds it, given a Name and Queue => 1 and IncludeGlobal" );
+ok($cf->ContextObject->id, 'Context object is also set');
+
+# The explicit LookupType works
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', LookupType => RT::Ticket->CustomFieldLookupType );
+ok($cf->id, "Load also finds it, given a Name and LookupType" );
+ok( ! $cf->ContextObject, 'But no context object gained');
+
+# Explicit LookupType, ObjectId works
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', LookupType => RT::Ticket->CustomFieldLookupType,
+                ObjectId => 1 );
+ok($cf->id, "Load still finds it, given a Name, LookupType, ObjectId" );
+ok($cf->ContextObject->id, 'And gains a context obj');
+
+# Explicit LookupType, ObjectId works
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', LookupType => RT::Ticket->CustomFieldLookupType,
+                ObjectId => 1, IncludeGlobal => 1 );
+ok($cf->id, "Load also finds it, given a Name, LookupType, ObjectId, and IncludeGlobal" );
+ok($cf->ContextObject->id, 'And gains a context obj');
+
+# Look for a queue by name
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => "General" );
+ok($cf->id, "Finds it by queue name" );
+ok($cf->ContextObject->id, 'But gains a context object');
+
+# Look for a queue by name, include global
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => "General", IncludeGlobal => 1 );
+ok($cf->id, "By name, and queue name works with IncludeGlobal" );
+ok($cf->ContextObject->id, 'And gains a context object');
+
+
+
+
+# Change the lookup type to be a _queue_ CF
+($ok, $msg) = $cf->SetLookupType( RT::Queue->CustomFieldLookupType );
+ok($ok, "Changed CF type to be a CF on queues" );
+$ocf = RT::ObjectCustomField->new( RT->SystemUser );
+( $ok, $msg ) = $ocf->Add( CustomField => $cf->id, ObjectId => 0 );
+ok($ok, "Applied globally" );
+
+# Just looking by name gets you CFs of any type
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF');
+ok($cf->id, "Find the CF by name, with no queue" );
+
+# Queue => 0 means "ticket CF", so doesn't find it
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 0);
+ok( ! $cf->id, "Wrong lookup type to find with Queue => 0" );
+
+# Queue => 1 and IncludeGlobal also doesn't find it
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 0, IncludeGlobal => 1);
+ok( ! $cf->id, "Also doesn't find with Queue => 0 and IncludeGlobal" );
+
+# Find it with the right LookupType
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', LookupType => RT::Queue->CustomFieldLookupType );
+ok($cf->id, "Found for the right lookup type" );
+
+# Found globally
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', LookupType => RT::Queue->CustomFieldLookupType, ObjectId => 0 );
+ok($cf->id, "Found for the right lookup type and ObjectId 0" );
+
+# Also works with Queue instead of ObjectId
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', LookupType => RT::Queue->CustomFieldLookupType, Queue => 0 );
+ok($cf->id, "Found for the right lookup type and Queue 0" );
+
+# Not found without IncludeGlobal
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', LookupType => RT::Queue->CustomFieldLookupType, ObjectId => 1 );
+ok( ! $cf->id, "Not found for ObjectId 1 and no IncludeGlobal" );
+
+# Found with IncludeGlobal
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', LookupType => RT::Queue->CustomFieldLookupType,
+                ObjectId => 1, IncludeGlobal => 1 );
+ok($cf->id, "Found for ObjectId 1 and IncludeGlobal" );
+
+# Found with IncludeGlobal and Queue instead of ObjectId
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', LookupType => RT::Queue->CustomFieldLookupType,
+                ObjectId => 1, IncludeGlobal => 1 );
+ok($cf->id, "Found for Queue 1 and IncludeGlobal" );
+
+
+
+# Change the lookup type to be a _transaction_ CF
+($ok, $msg) = $cf->SetLookupType( RT::Transaction->CustomFieldLookupType );
+ok($ok, "Changed CF type to be a CF on transactions" );
+$ocf = RT::ObjectCustomField->new( RT->SystemUser );
+( $ok, $msg ) = $ocf->Add( CustomField => $cf->id, ObjectId => 0 );
+ok($ok, "Applied globally" );
+
+# Just looking by name gets you CFs of any type
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF');
+ok($cf->id, "Find the CF by name, with no queue" );
+
+# Queue => 0 means "ticket CF", so doesn't find it
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 0);
+ok( ! $cf->id, "Wrong lookup type to find with Queue => 0" );
+
+# Queue => 1 and IncludeGlobal also doesn't find it
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 0, IncludeGlobal => 1);
+ok( ! $cf->id, "Also doesn't find with Queue => 0 and IncludeGlobal" );
+
+
+# Change the lookup type to be a _user_ CF
+$cf->Load(1);
+($ok, $msg) = $cf->SetLookupType( RT::User->CustomFieldLookupType );
+ok($ok, "Changed CF type to be a CF on users" );
+$ocf = RT::ObjectCustomField->new( RT->SystemUser );
+( $ok, $msg ) = $ocf->Add( CustomField => $cf->id, ObjectId => 0 );
+ok($ok, "Applied globally" );
+
+# Just looking by name gets you CFs of any type
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF');
+ok($cf->id, "Find the CF by name, with no queue" );
+
+# Queue => 0 means "ticket CF", so doesn't find it
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 0);
+ok( ! $cf->id, "Wrong lookup type to find with Queue => 0" );
+
+# Queue => 1 and IncludeGlobal also doesn't find it
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 0, IncludeGlobal => 1);
+ok( ! $cf->id, "Also doesn't find with Queue => 0 and IncludeGlobal" );
+
+# But RT::User->CustomFieldLookupType does
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', LookupType => RT::User->CustomFieldLookupType );
+ok($cf->id, "User lookuptype does" );
+
+# Also with an explicit global
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', LookupType => RT::User->CustomFieldLookupType, ObjectId => 0 );
+ok($cf->id, "Also with user CF and explicit global" );
+
+
+
+# Add a second, queue-specific CF to test load order
+$cf->Load(1);
+($ok, $msg) = $cf->SetLookupType( RT::Ticket->CustomFieldLookupType );
+ok($ok, "Changed CF type back to be a CF on tickets" );
+$ocf = RT::ObjectCustomField->new( RT->SystemUser );
+( $ok, $msg ) = $ocf->Add( CustomField => $cf->id, ObjectId => 0 );
+ok($ok, "Applied globally" );
+($ok, $msg) = $cf->SetDescription( "Global CF" );
+ok($ok, "Changed CF type back to be a CF on tickets" );
+
+($ok, $msg) = $cf->Create(
+    Name        => 'TestingCF',
+    Queue       => '1',
+    Description => 'Queue-specific CF',
+    Type        => 'SelectSingle'
+);
+ok($ok, "Created second CF successfully");
+
+# If passed just a name, you get the first by id
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF' );
+like($cf->Description, qr/Global/, "Gets the first (global) one if just loading by name" );
+
+# Ditto if also limited to lookuptype
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', LookupType => RT::Ticket->CustomFieldLookupType );
+like($cf->Description, qr/Global/, "Same, if one adds a LookupType" );
+
+# Gets the global with Queue => 0
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 0 );
+like($cf->Description, qr/Global/, "Specify Queue => 0 and get global" );
+
+# Gets the queue with Queue => 1
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 1 );
+like($cf->Description, qr/Queue/, "Specify Queue => 1 and get the queue" );
+
+# Gets the queue with Queue => 1 and IncludeGlobal
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 1, IncludeGlobal => 1 );
+like($cf->Description, qr/Queue/, "Specify Queue => 1 and IncludeGlobal and get the queue" );
+
+
+# Disable one of them
+($ok, $msg) = $cf->SetDisabled(1);
+ok($ok, "Disabled the Queue-specific one");
+
+# With just a name, prefers the non-disabled
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF' );
+like($cf->Description, qr/Global/, "Prefers non-disabled CFs" );
 
+# Still finds the queue one, if asked
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 1 );
+like($cf->Description, qr/Queue/, "Still loads the disabled queue CF" );
 
-}
+# Prefers the global one if IncludeGlobal
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 1, IncludeGlobal => 1 );
+like($cf->Description, qr/Global/, "Prefers the global one with IncludeGlobal" );
 
-{
+# IncludeDisabled allows filtering out the disabled one
+$cf = RT::CustomField->new( RT->SystemUser );
+$cf->LoadByName(Name => 'TestingCF', Queue => 1, IncludeDisabled => 0 );
+ok( ! $cf->id, "Doesn't find it if IncludeDisabled => 0" );
 
-    my $cf = RT::CustomField->new(RT->SystemUser);
-    $cf->LoadByName(Queue => 0, Name => 'TestingCF');
-    ok($cf->Id, 'Loaded CF ' . $cf->Id);
-    ok(!$cf->ContextObject, 'Context object not set when queue is 0');
-}
 
 done_testing;

commit b30020aa119ef32131c4c80e2109a30720cc8f79
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Fri May 16 00:04:51 2014 -0400

    Switch existing code to using LookupType with LoadByName, to be explicit
    
    To set an example for future code, transition away from the 'Queue'
    argument, in preference to LookupType and ObjectId.  This helps clarify
    the type of custom field that is expected to be returned.
    Simultaneously, switch to using IncludeGlobal where the code would
    benefit from such.

diff --git a/lib/RT/Action/CreateTickets.pm b/lib/RT/Action/CreateTickets.pm
index a88a6ee..5cb1256 100644
--- a/lib/RT/Action/CreateTickets.pm
+++ b/lib/RT/Action/CreateTickets.pm
@@ -756,14 +756,22 @@ sub ParseLines {
             $ticketargs{ "CustomField-" . $1 } = $args{$tag};
         } elsif ( $orig_tag =~ /^(?:customfield|cf)-?(.+)$/i ) {
             my $cf = RT::CustomField->new( $self->CurrentUser );
-            $cf->LoadByName( Name => $1, Queue => $ticketargs{Queue} );
-            $cf->LoadByName( Name => $1, Queue => 0 ) unless $cf->id;
+            $cf->LoadByName(
+                Name          => $1,
+                LookupType    => RT::Ticket->CustomFieldLookupType,
+                ObjectId      => $ticketargs{Queue},
+                IncludeGlobal => 1,
+            );
             next unless $cf->id;
             $ticketargs{ "CustomField-" . $cf->id } = $args{$tag};
         } elsif ($orig_tag) {
             my $cf = RT::CustomField->new( $self->CurrentUser );
-            $cf->LoadByName( Name => $orig_tag, Queue => $ticketargs{Queue} );
-            $cf->LoadByName( Name => $orig_tag, Queue => 0 ) unless $cf->id;
+            $cf->LoadByName(
+                Name          => $orig_tag,
+                LookupType    => RT::Ticket->CustomFieldLookupType,
+                ObjectId      => $ticketargs{Queue},
+                IncludeGlobal => 1,
+            );
             next unless $cf->id;
             $ticketargs{ "CustomField-" . $cf->id } = $args{$tag};
 
diff --git a/lib/RT/Handle.pm b/lib/RT/Handle.pm
index 351478e..773e3c2 100644
--- a/lib/RT/Handle.pm
+++ b/lib/RT/Handle.pm
@@ -1058,7 +1058,7 @@ sub InsertData {
                 $object = RT::CustomField->new( RT->SystemUser );
                 my @columns = ( Name => $item->{'CF'} );
                 push @columns, LookupType => $item->{'LookupType'} if $item->{'LookupType'};
-                push @columns, Queue => $item->{'Queue'} if $item->{'Queue'} and not ref $item->{'Queue'};
+                push @columns, ObjectId => $item->{'Queue'} if $item->{'Queue'} and not ref $item->{'Queue'};
                 my ($ok, $msg) = $object->LoadByName( @columns );
                 unless ( $ok ) {
                     RT->Logger->error("Unable to load CF ".$item->{CF}.": $msg");
diff --git a/lib/RT/Migrate/Importer.pm b/lib/RT/Migrate/Importer.pm
index 14fb23a..0a159be 100644
--- a/lib/RT/Migrate/Importer.pm
+++ b/lib/RT/Migrate/Importer.pm
@@ -145,7 +145,7 @@ sub InitStream {
     if ($self->{OriginalId}) {
         # Where to shove the original ticket ID
         my $cf = RT::CustomField->new( RT->SystemUser );
-        $cf->LoadByName( Queue => 0, Name => $self->{OriginalId} );
+        $cf->LoadByName( Name => $self->{OriginalId}, LookupType => RT::Ticket->CustomFieldLookupType, ObjectId => 0 );
         unless ($cf->Id) {
             warn "Failed to find global CF named $self->{OriginalId} -- creating one";
             $cf->Create(
diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index b2e22cc..8ed820c 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -449,7 +449,11 @@ sub CustomField {
     my $self = shift;
     my $name = shift;
     my $cf = RT::CustomField->new($self->CurrentUser);
-    $cf->LoadByName(Name => $name, Queue => $self->Id);
+    $cf->LoadByName(
+        Name       => $name,
+        LookupType => RT::Ticket->CustomFieldLookupType,
+        ObjectId   => $self->id,
+    );
     return ($cf);
 }
 
diff --git a/lib/RT/Test.pm b/lib/RT/Test.pm
index 0261f42..f08734f 100644
--- a/lib/RT/Test.pm
+++ b/lib/RT/Test.pm
@@ -945,7 +945,11 @@ sub load_or_create_custom_field {
     my %args = ( Disabled => 0, @_ );
     my $obj = RT::CustomField->new( RT->SystemUser );
     if ( $args{'Name'} ) {
-        $obj->LoadByName( Name => $args{'Name'}, Queue => $args{'Queue'} );
+        $obj->LoadByName(
+            Name       => $args{'Name'},
+            LookupType => RT::Ticket->CustomFieldLookupType,
+            ObjectId   => $args{'Queue'},
+        );
     } else {
         die "Name is required";
     }
diff --git a/lib/RT/Test/Web.pm b/lib/RT/Test/Web.pm
index 872cd29..b5c7229 100644
--- a/lib/RT/Test/Web.pm
+++ b/lib/RT/Test/Web.pm
@@ -339,7 +339,11 @@ sub custom_field_input {
     my $cf_name = shift;
 
     my $cf_obj = RT::CustomField->new( $RT::SystemUser );
-    $cf_obj->LoadByName( Queue => $queue, Name => $cf_name );
+    $cf_obj->LoadByName(
+        Name => $cf_name,
+        LookupType => RT::Ticket->CustomFieldLookupType,
+        ObjectId => $queue,
+    );
     unless ( $cf_obj->id ) {
         Test::More::diag("Can not load custom field '$cf_name' in queue '$queue'");
         return undef;
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index a2a5c9a..d73e9d2 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -2975,8 +2975,12 @@ sub LoadCustomFieldByIdentifier {
 
     my $cf = RT::CustomField->new( $self->CurrentUser );
     $cf->SetContextObject( $self );
-    $cf->LoadByName( Name => $field, Queue => $self->Queue );
-    $cf->LoadByName( Name => $field, Queue => 0 ) unless $cf->id;
+    $cf->LoadByName(
+        Name          => $field,
+        LookupType    => $self->CustomFieldLookupType,
+        ObjectId      => $self->Queue,
+        IncludeGlobal => 1,
+    );
     return $cf;
 }
 
diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm
index 2defde4..c30e76e 100644
--- a/lib/RT/Tickets.pm
+++ b/lib/RT/Tickets.pm
@@ -2190,8 +2190,9 @@ sub LimitCustomField {
     }
     else {
         $CF->LoadByName(
-            Name  => $args{CUSTOMFIELD},
-            Queue => $args{QUEUE}
+            Name       => $args{CUSTOMFIELD},
+            LookupType => RT::Ticket->CustomFieldLookupType,
+            ObjectId   => $args{QUEUE},
         );
         $args{CUSTOMFIELD} = $CF->Id;
     }
diff --git a/share/html/REST/1.0/Forms/ticket/default b/share/html/REST/1.0/Forms/ticket/default
index 3bf6fb7..9476871 100644
--- a/share/html/REST/1.0/Forms/ticket/default
+++ b/share/html/REST/1.0/Forms/ticket/default
@@ -153,10 +153,12 @@ else {
                 my $key = $1 || $2;
 
                 my $cf = RT::CustomField->new( $session{CurrentUser} );
-                $cf->LoadByName( Name => $key, Queue => $data{Queue} || $v{Queue} );
-                unless ( $cf->id ) {
-                    $cf->LoadByName( Name => $key, Queue => 0 );
-                }
+                $cf->LoadByName(
+                    Name          => $key,
+                    LookupType    => RT::Ticket->CustomFieldLookupType,
+                    ObjectId      => $data{Queue} || $v{Queue},
+                    IncludeGlobal => 1,
+                );
 
                 if (not $cf->id) {
                     push @comments, "# Invalid custom field name ($key)";
@@ -379,10 +381,13 @@ else {
             $key = $1 || $2;
 
             my $cf = RT::CustomField->new( $session{CurrentUser} );
-            $cf->LoadByName( Name => $key, Queue => $ticket->Queue );
-            unless ( $cf->id ) {
-                $cf->LoadByName( Name => $key, Queue => 0 );
-            }
+            $cf->ContextObject( $ticket );
+            $cf->LoadByName(
+                Name          => $key,
+                LookupType    => RT::Ticket->CustomFieldLookupType,
+                ObjectId      => $ticket->Queue,
+                IncludeGlobal => 1,
+            );
 
             if (not $cf->id) {
                 $n = 0;
diff --git a/t/web/query_builder.t b/t/web/query_builder.t
index e605dc6..05a4b0a 100644
--- a/t/web/query_builder.t
+++ b/t/web/query_builder.t
@@ -197,7 +197,7 @@ diag "click advanced, enter 'C1 OR ( C2 AND C3 )', apply, aggregators should sta
 # create a custom field with nonascii name and try to add a condition
 {
     my $cf = RT::CustomField->new( RT->SystemUser );
-    $cf->LoadByName( Name => "\x{442}", Queue => 0 );
+    $cf->LoadByName( Name => "\x{442}", LookupType => RT::Ticket->CustomFieldLookupType, ObjectId => 0 );
     if ( $cf->id ) {
         is($cf->Type, 'Freeform', 'loaded and type is correct');
     } else {

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


More information about the rt-commit mailing list