[Rt-commit] rt branch, 4.0/columnmap-cache-cf, created. rt-4.0.8-64-g15d4da6
Alex Vandiver
alexmv at bestpractical.com
Thu Nov 1 18:29:05 EDT 2012
The branch, 4.0/columnmap-cache-cf has been created
at 15d4da6fb6707f26228c301cd33cbb2210c0dd41 (commit)
- Log -----------------------------------------------------------------
commit db2a6ae274a580644ac96ed88850168c9283a4f0
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Jun 12 13:57:27 2012 -0400
RT::ObjectCustomFieldValues calls LimitToEnabled by default
This cleans up an unnecessary (Disabled = 0 OR Disabled = 0)
diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index 2002d4e..d065b7f 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -1656,7 +1656,6 @@ sub ValuesForObject {
$values->LimitToCustomField($self->Id);
- $values->LimitToEnabled();
$values->LimitToObject($object);
return ($values);
commit 3d91316bfc2e0e7690c2cecef549e7d8dda1b85e
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Jun 12 16:30:27 2012 -0400
Move "global or queue" logic to LoadCustomFieldByIdentifier
The logic for attempting to load a queue-specific queue, and then the
global one, was on the CustomFieldValues method. Move the logic to
LoadCustomFieldByIdentifier, which is called by
RT::Record->CustomFieldValues, rather than overriding CustomFieldValues
directly; this allows other methods to simple call
->LoadCustomFieldByIdentifier and gain the queue-or-global
functionality.
This change also causes ->CustomFieldValues (and thus both
->FirstCustomFieldValue and ->CustomFieldValuesAsString), when called on
tickets with an invalid CF name, to warn. This makes it consistent with
->CustomFieldValues called on other types of objects, and may provide
useful feedback when a CF name is improperly specified.
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 2109d71..f9c587a 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -3710,37 +3710,29 @@ sub TransactionCustomFields {
}
+=head2 LoadCustomFieldByIdentifier
-=head2 CustomFieldValues
-
-# Do name => id mapping (if needed) before falling back to
-# RT::Record's CustomFieldValues
-
-See L<RT::Record>
+Finds and returns the custom field of the given name for the ticket,
+overriding L<RT::Record/LoadCustomFieldByIdentifier> to look for
+queue-specific CFs before global ones.
=cut
-sub CustomFieldValues {
+sub LoadCustomFieldByIdentifier {
my $self = shift;
my $field = shift;
- return $self->SUPER::CustomFieldValues( $field ) if !$field || $field =~ /^\d+$/;
+ return $self->SUPER::LoadCustomFieldByIdentifier($field)
+ if ref $field or $field =~ /^\d+$/;
my $cf = RT::CustomField->new( $self->CurrentUser );
$cf->SetContextObject( $self );
$cf->LoadByNameAndQueue( Name => $field, Queue => $self->Queue );
- unless ( $cf->id ) {
- $cf->LoadByNameAndQueue( Name => $field, Queue => 0 );
- }
-
- # If we didn't find a valid cfid, give up.
- return RT::ObjectCustomFieldValues->new( $self->CurrentUser ) unless $cf->id;
-
- return $self->SUPER::CustomFieldValues( $cf->id );
+ $cf->LoadByNameAndQueue( Name => $field, Queue => 0 ) unless $cf->id;
+ return $cf;
}
-
=head2 CustomFieldLookupType
Returns the RT::Ticket lookup type, which can be passed to
diff --git a/lib/RT/Transaction.pm b/lib/RT/Transaction.pm
index e01b257..b1e818c 100644
--- a/lib/RT/Transaction.pm
+++ b/lib/RT/Transaction.pm
@@ -1180,37 +1180,31 @@ sub UpdateCustomFields {
}
}
+=head2 LoadCustomFieldByIdentifier
-
-=head2 CustomFieldValues
-
- Do name => id mapping (if needed) before falling back to RT::Record's CustomFieldValues
-
- See L<RT::Record>
+Finds and returns the custom field of the given name for the
+transaction, overriding L<RT::Record/LoadCustomFieldByIdentifier> to
+look for queue-specific CFs before global ones.
=cut
-sub CustomFieldValues {
+sub LoadCustomFieldByIdentifier {
my $self = shift;
my $field = shift;
- if ( UNIVERSAL::can( $self->Object, 'QueueObj' ) ) {
-
- # XXX: $field could be undef when we want fetch values for all CFs
- # do we want to cover this situation somehow here?
- unless ( defined $field && $field =~ /^\d+$/o ) {
- my $CFs = RT::CustomFields->new( $self->CurrentUser );
- $CFs->SetContextObject( $self->Object );
- $CFs->Limit( FIELD => 'Name', VALUE => $field );
- $CFs->LimitToLookupType($self->CustomFieldLookupType);
- $CFs->LimitToGlobalOrObjectId($self->Object->QueueObj->id);
- $field = $CFs->First->id if $CFs->First;
- }
- }
- return $self->SUPER::CustomFieldValues($field);
-}
+ return $self->SUPER::LoadCustomFieldByIdentifier($field)
+ if ref $field or $field =~ /^\d+$/;
+ return $self->SUPER::LoadCustomFieldByIdentifier($field)
+ unless UNIVERSAL::can( $self->Object, 'QueueObj' );
+ my $CFs = RT::CustomFields->new( $self->CurrentUser );
+ $CFs->SetContextObject( $self->Object );
+ $CFs->Limit( FIELD => 'Name', VALUE => $field );
+ $CFs->LimitToLookupType($self->CustomFieldLookupType);
+ $CFs->LimitToGlobalOrObjectId($self->Object->QueueObj->id);
+ return $CFs->First || RT::CustomField->new( $self->CurrentUser );
+}
=head2 CustomFieldLookupType
diff --git a/t/customfields/api.t b/t/customfields/api.t
index d739a57..228693f 100644
--- a/t/customfields/api.t
+++ b/t/customfields/api.t
@@ -3,7 +3,8 @@
use strict;
use warnings FATAL => 'all';
-use RT::Test nodata => 1, tests => 139;
+use RT::Test nodata => 1, tests => 145;
+use Test::Warn;
# Before we get going, ditch all object_cfs; this will remove
# all custom fields systemwide;
@@ -69,13 +70,21 @@ is( $cfvs->Count, 0 );
is( $ticket->FirstCustomFieldValue, undef );
# CF with ID -1 shouldnt exist at all
-$cfvs = $ticket->CustomFieldValues( -1 );
+warning_like {
+ $cfvs = $ticket->CustomFieldValues( -1 );
+} qr{Couldn't load custom field};
is( $cfvs->Count, 0 );
-is( $ticket->FirstCustomFieldValue( -1 ), undef );
+warning_like {
+ is( $ticket->FirstCustomFieldValue( -1 ), undef );
+} qr{Couldn't load custom field};
-$cfvs = $ticket->CustomFieldValues( 'SomeUnexpedCustomFieldName' );
+warning_like {
+ $cfvs = $ticket->CustomFieldValues( 'SomeUnexpedCustomFieldName' );
+} qr{Couldn't load custom field};
is( $cfvs->Count, 0 );
-is( $ticket->FirstCustomFieldValue( 'SomeUnexpedCustomFieldName' ), undef );
+warning_like {
+ is( $ticket->FirstCustomFieldValue( 'SomeUnexpedCustomFieldName' ), undef );
+} qr{Couldn't load custom field};
for (@custom_fields) {
$cfvs = $ticket->CustomFieldValues( $_->id );
@@ -183,9 +192,13 @@ $test_add_delete_cycle->( sub { return $_[0] } );
$ticket->AddCustomFieldValue( Field => $local_cf2->id , Value => 'Baz' );
$ticket->AddCustomFieldValue( Field => $global_cf3->id , Value => 'Baz' );
# now if we ask for cf values on RecordCustomFields4 we should not get any
-$cfvs = $ticket->CustomFieldValues( 'RecordCustomFields4' );
+warning_like {
+ $cfvs = $ticket->CustomFieldValues( 'RecordCustomFields4' );
+} qr{Couldn't load custom field};
is( $cfvs->Count, 0, "No custom field values for non-Queue cf" );
-is( $ticket->FirstCustomFieldValue( 'RecordCustomFields4' ), undef, "No first custom field value for non-Queue cf" );
+warning_like {
+ is( $ticket->FirstCustomFieldValue( 'RecordCustomFields4' ), undef, "No first custom field value for non-Queue cf" );
+} qr{Couldn't load custom field};
{
my $cfname = $global_cf3->Name;
commit a64a16d1cf32a0fc2d9ca298e7323891759ae739
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Nov 1 18:20:42 2012 -0400
Promote _LookupId to a public method, and optimize slightly
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 8e4ce0e..6d0a55b 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -1563,29 +1563,32 @@ sub CustomFields {
$cfs->SetContextObject( $self );
# XXX handle multiple types properly
$cfs->LimitToLookupType( $self->CustomFieldLookupType );
- $cfs->LimitToGlobalOrObjectId(
- $self->_LookupId( $self->CustomFieldLookupType )
- );
+ $cfs->LimitToGlobalOrObjectId( $self->CustomFieldLookupId );
$cfs->ApplySortOrder;
return $cfs;
}
-# TODO: This _only_ works for RT::Class classes. it doesn't work, for example,
-# for RT::IR classes.
+# TODO: This _only_ works for RT::Foo classes. it doesn't work, for
+# example, for RT::IR::Foo classes.
-sub _LookupId {
+sub CustomFieldLookupId {
my $self = shift;
- my $lookup = shift;
+ my $lookup = shift || $self->CustomFieldLookupType;
my @classes = ($lookup =~ /RT::(\w+)-/g);
+ # Work on "RT::Queue", for instance
+ return $self->Id unless @classes;
+
my $object = $self;
+ # Save a ->Load call by not calling ->FooObj->Id, just ->Foo
+ my $final = shift @classes;
foreach my $class (reverse @classes) {
my $method = "${class}Obj";
$object = $object->$method;
}
- return $object->Id;
+ return $object->$final;
}
commit da1736e54204ec7dd09edf444a8cc03737058ed5
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Thu Nov 1 18:24:04 2012 -0400
Remove unnecessary RT::Class::_LookupId
It was previously required because the class was RT::FM::Foo, and hence
the generic method in RT::Record did not work (as noted in the comment
there). With the merge of RTFM into core, and the rename of
RT::FM::Class -> RT::Class, this method is no longer necessary.
diff --git a/lib/RT/Article.pm b/lib/RT/Article.pm
index 678aa11..58dd94b 100644
--- a/lib/RT/Article.pm
+++ b/lib/RT/Article.pm
@@ -611,15 +611,6 @@ sub CustomFieldLookupType {
"RT::Class-RT::Article";
}
-# _LookupId is the id of the toplevel type object the customfield is joined to
-# in this case, that's an RT::Class.
-
-sub _LookupId {
- my $self = shift;
- return $self->ClassObj->id;
-
-}
-
=head2 LoadByInclude Field Value
Takes the name of a form field from "Include Article"
commit 15d4da6fb6707f26228c301cd33cbb2210c0dd41
Author: Alex Vandiver <alexmv at bestpractical.com>
Date: Tue Jun 12 16:41:38 2012 -0400
Cache CFs in ColumnMap on a per-request basis
When displaying /n/ rows of search results with /m/ columns of global CF
values displayed, custom field queries accounted for /n/ * /m/ * 3
queries; one to try to load the queue-specific CF with the column name,
one to load the global CF with that name, and one to load the OCFV for
that CF and ticket. With sufficient CFs in a search result format,
these account for a significant performance impact, despite being
relatively well-indexed.
Cache the CF objects in the per-request $m->notes() cache to eliminate
the CF loads, which account for ½ to ⅔ of those queries, depending on
the prevalence of global CFs. The cache key must also include the
lookup id (queue id, in most cases) -- as a single search may include
results from multiple queues, each of which may resolve a CF name
differently. It must also include the lookup type, such that a ticket
CF and a transaction CF with the same name can be differentiated.
diff --git a/share/html/Elements/ColumnMap b/share/html/Elements/ColumnMap
index 7295e3f..fbe6a9d 100644
--- a/share/html/Elements/ColumnMap
+++ b/share/html/Elements/ColumnMap
@@ -96,13 +96,25 @@ my $COLUMN_MAP = {
attribute => sub { return shift @_ },
title => sub { return pop @_ },
value => sub {
+ # Cache the CF object on a per-request basis, to avoid
+ # having to load it for every row
+ my $key = join("-","CF",
+ $_[0]->CustomFieldLookupType,
+ $_[0]->CustomFieldLookupId,
+ $_[-1]);
+
+ my $cf = $m->notes($key);
+ unless ($cf) {
+ $cf = $_[0]->LoadCustomFieldByIdentifier($_[-1]);
+ $m->notes($key, $cf);
+ }
+
# Display custom field contents, separated by newlines.
# For Image custom fields we also show a thumbnail here.
-
- my $values = $_[0]->CustomFieldValues( $_[-1] );
+ my $values = $cf->ValuesForObject( $_[0] );
my @values = map {
(
- ($_->CustomFieldObj->Type eq 'Image')
+ ($cf->Type eq 'Image')
? \($m->scomp( '/Elements/ShowCustomFieldImage', Object => $_ ))
: $_->Content
),
-----------------------------------------------------------------------
More information about the Rt-commit
mailing list