[Rt-commit] rt branch, 4.0/columnmap-cache-cf, created. rt-4.0.6-169-g47fc745
Alex Vandiver
alexmv at bestpractical.com
Tue Jun 12 17:15:52 EDT 2012
The branch, 4.0/columnmap-cache-cf has been created
at 47fc745e4f578d3e4ad2eb914af8dd9472d4fbb9 (commit)
- Log -----------------------------------------------------------------
commit dda0fa00bb70334a770d2cc35f012bad2ff61eba
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 ce698ecd6bb2c5198442e2ca65cbbb84ad12e764
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 c6a5e5b..e4c5384 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -3666,37 +3666,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 5b3641f..fa2c56f 100644
--- a/lib/RT/Transaction.pm
+++ b/lib/RT/Transaction.pm
@@ -1175,37 +1175,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 47fc745e4f578d3e4ad2eb914af8dd9472d4fbb9
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 beign
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 queue
name, as a single search may include results from multiple queues, each
of which may resolve a CF name differently.
diff --git a/share/html/Elements/ColumnMap b/share/html/Elements/ColumnMap
index 87fd61b..889053b 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 = "CF--$_[-1]";
+ $key = "CF-".$_[0]->Queue."-$_[-1]" if $_[0]->isa("RT::Ticket");
+ $key = "CF-".$_[0]->Object->Queue."-$_[-1]" if $_[0]->isa("RT::Transaction")
+ and $_[0]->Object->isa("RT::Ticket");
+
+ 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