[Rt-devel] [PATCH] Results.tsv - Custom Fields Sorting Bug

[Brian D]

On our RT 3.8.6 install, I noted there is a bug in the new code to show all
Custom Fields in the tsv file that results in incorrect data in columns
(same code in 3.8.7 fwiw).

Because cf_name_to_pos and @header were built using the different sorts, the
output comes out with data all out of whack.  Fixing the $a is a quick fix,
but I think this rework clarifies the code and removes somewhat confusing
calculations to build cf_name_to_pos.

I changed this code around so that the calculation of cf_name_to_pos is more
explicitly done closer to the @header build-up.

While working on this, I am a bit puzzled why this change was made to
include ALL custom fields for the relevant queues.  While I was
investigating this, we had backrev'd to a 3.8.2 version.  In that release,
it scans to see what "known_cfs"  are in the results before returning the
data and includes a reduced set of columns as a result.  This seems like a
better approach to avoid a lot of empty columns in queues with a large
number of not always used CFs.

Additionally, I had thought about taking a look at making a version of this
that returns only the columns the User was requesting, but it is not exactly
straightforward and I'm unsure of the best approach.  Using $Format this
could be done in a quick (hackish) way, but it looks like the right way
would be for a version of /Elements/CollectionAsTable/Row and friends that
returns html-less row data.  Optionally with newlines in the format
suppressed.

Anyone else have thoughts on this?  A WYSIWYG screen-to-spreadsheet is what
I think some people are expecting when they click on that Spreadsheet link.

Below is a patch with comments left in for reference, most should be removed
from a release integration:

--- Results.tsv 2010/01/04 21:25:03     3.8.6
+++ Results.tsv 2010/01/04 21:25:15
@@ -67,6 +67,7 @@
     $Tickets->OrderBy( FIELD => $OrderBy, ORDER => $Order );
 }

+# XXX This code gets all custom fields for queue, not necessarily those
populated in these results.
 my %cf_id_to_name;
 my %cf_name_to_pos;
 {
@@ -76,10 +77,11 @@
     while ( my $cf = $cfs->Next ) {
         my $name = $cf->Name;
         $cf_id_to_name{ $cf->id } = $name;
-        next if $cf_name_to_pos{ $name };
-
-        $cf_name_to_pos{ $name } =
-            (sort { $b <=> $a } values %cf_name_to_pos)[0] + 1;
+#XXX Calculating CF positions moved below to be more explicit and accurate.
+#        next if $cf_name_to_pos{ $name };
+#
+#        $cf_name_to_pos{ $name } =
+#            (sort { $b <=> $a } values %cf_name_to_pos)[0] + 1;
     }
 }

@@ -105,10 +107,17 @@
         push @header, $label;
     }

-    $_ += @header - 1 foreach values %cf_name_to_pos;
-
-    foreach my $name ( sort { $cf_name_to_pos{$a} <=> $cf_name_to_pos{$a} }
keys %cf_name_to_pos ) {
+#XXX Done more explicitly below..    $_ += @header - 1 foreach values
%cf_name_to_pos;
+#
+#XXX CF COLUMN SORTING BUG WAS HERE, note $a twice:    foreach my $name (
sort { $cf_name_to_pos{$a} <=> $cf_name_to_pos{$a} } keys %cf_name_to_pos )
{
+# Instead, let us sort by the customfieldnames instead anyway.
+    foreach my $name ( sort { lc($a) cmp lc($b) } values %cf_id_to_name ) {
         push @header, "CF-". $name;
+       if ( defined $cf_name_to_pos{ $name } ) {
+         # debug - duplicate custom field by name?
+         # Not sure if this can occur, this was checked in the code above
though.
+        }
+       $cf_name_to_pos{$name} = $#header;
     }
     $m->out(join("\t", @header));
     $m->out("\n");
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.bestpractical.com/pipermail/rt-devel/attachments/20100104/3b2f4bfe/attachment.htm