Fix timeline group order bug

When a timeline was initially viewed (without specific dates) the
ordering is by date order not by the order in the view. This caused the
grouping to be in the incorrect order. This commit adds tests for this
and fixes it by re-retrieving the items.

Author: Andy Beverley

lib/GADS/Records.pm

@@ -1321,14 +1321,14 @@ sub single
 
     my $next_id = $self->_next_single_id;
 
-    # Check return if limiting to a set number of results
-    return if $self->max_results && $self->records_retrieved_count >= $self->max_results;
-
     if (!$self->is_group) # Don't retrieve in chunks for group records
     {
         if (
             ($next_id == 0 && $self->_single_page == 0) # First run
-            || $next_id >= $chunk # retrieved all of current chunk
+            # Retrieved all of current chunk. Make sure there aren't more
+            # results than the chunk (possible with
+            # separate_records_for_multicol)
+            || ($next_id >= $chunk && $next_id >= @{$self->results})
         )
         {
             $self->_single_page($self->_single_page + 1) # increase to next page
@@ -1561,6 +1561,7 @@ sub clear
     $self->clear_aggregate_results;
     $self->clear_search;
     $self->_set__next_single_id(0);
+    $self->_single_page(0);
     $self->_set_current_ids(undef);
     $self->_clear_all_cids_store;
     $self->_clear_cid_search_query_cache
@@ -2510,20 +2511,48 @@ sub data_timeline
         # E.g. if a day is subtracted from 26th March 2018 01:00 London then it will be an
         # invalid time and DateTime will bork.
         $min = min map $_->{dt}, @items;
-        $min->set_time_zone('UTC')->subtract(days => 1) if $min;
+        if ($min)
+        {
+            # There is a chance that $min will be the same object as $max.
+            # Clone it to ensure we don't change both below
+            $min = $min->clone;
+            $min->set_time_zone('UTC'); # ->subtract(days => 1);
+        }
 
         # Fix the max to no longer be where we retrieved to, but actually the
         # max value of the finalised items array. Then add some padding.
         $max = max map $_->{dt}, @items;
-        # one day already added to show period to end of day
-        $max->set_time_zone('UTC')->add(days => 2) if $max;
-    }
-    elsif($original_from && $original_to)
-    {   @items = @{$timeline->items};
-        ($min, $max) = ($original_from, $original_to);
-    }
-    else
-    {   @items = @{$timeline->items};
+        if ($max)
+        {
+            $max = $max->clone;
+            $max->set_time_zone('UTC');
+        }
+
+        # Set from and to so that a re-retrieval retrieves the same results
+        $self->to($max->clone->add(days => 1)) # one day already added to show period to end of day
+            if $max;
+        $self->from($min->clone)
+            if $min;
+
+        # Set the display range of the timeline
+        $original_from = $min && $min->subtract(days => 1);
+        $max->add(days => 2) if $max;
+        $original_to = $max;
+    }
+    # If the $limit_qty routine above was used, re-retrieve all of the items to
+    # ensure correct sorting (in the case of grouping). XXX This only needs to
+    # be done if there was a grouping, but at the moment it is done for all
+    # conditions, to ensure they are all fully tested in the tests. What is
+    # needed is for the tests to be run twice, once for no re-retrieval and
+    # once for re-retrieval. The performance hit is probably quite low, as the
+    # above routine is only used when initially looking at the timeline, before
+    # a range has been selected by the user.
+    if (!$limit_qty || @items)
+    {
+        @items = @{$timeline->items};
+        my $m = min map $_->{dt}, @items;
+        ($min, $max) = ($original_from, $original_to)
+            if $original_from && $original_to;
     }
 
     # Remove dt (DateTime) value, otherwise JSON encoding borks

t/013_datatime.t

@@ -694,18 +694,37 @@ is( @{$records->data_timeline->{items}}, 1, "Filter, single column and limited r
 
 # Multiple items in group field, ordered correctly
 {
-    my $data = [
-        {
+    # Create a set of data where a multivalue record is in the middle of the
+    # right-half of the timeline. The multivalue record contains 2 values that
+    # are at the top and bottom of the grouped items. All the other values of
+    # the timeline are in the middle of the grouping. Because the multivalue
+    # record is in the middle of the date range, both should show on the
+    # grouping, one at the begninning and one at the end.
+    my $data = [];
+    my $date = DateTime->new(year => 2010, month => 1, day => 1);
+    for my $i (1..100)
+    {
+        push @$data, {
+            string1 => 'foo2',
+            enum1   => [2],
+            date1   => $date->add(days => 1)->ymd,
+        };
+    }
+    push @$data,{
             string1 => 'foo1',
             enum1   => [1,3],
-            date1   => '2010-06-01',
-        },
-        {
+            date1   => '2010-07-01',
+    };
+    $date = DateTime->new(year => 2010, month => 8, day => 1);
+    for my $i (1..120)
+    {
+        push @$data, {
             string1 => 'foo2',
             enum1   => [2],
-            date1   => '2010-06-05',
-        },
-    ];
+            date1   => $date->add(days => 1)->ymd,
+        };
+    }
+
     my $sheet = Test::GADS::DataSheet->new(data => $data, multivalue => 1);
 
     $sheet->create_records;
@@ -728,12 +747,14 @@ is( @{$records->data_timeline->{items}}, 1, "Filter, single column and limited r
         view    => $view,
         user    => undef,
         layout  => $layout,
+        from    => DateTime->new(year => 2010, month => 3, day => 1),
         schema  => $schema,
     );
 
     my $return = $records->data_timeline(group => $enum1->id);
 
-    is( @{$return->{items}}, 3, "Correct number of items for group ordering" );
+    # 149 normal results as per other tests plus 1 extra result for multivalue
+    is( @{$return->{items}}, 149, "Correct number of items for group ordering" );
 
     # Check that the groups are in the right order. Because we are ordering by
     # the enumvals (foo1, foo2, foo3) then number in the value should match the
@@ -1067,6 +1088,8 @@ is( @{$records->data_timeline->{items}}, 1, "Filter, single column and limited r
 
     my $layout = $sheet->layout;
     $layout->clear;
+    $layout->user($sheet->user_normal1);
+    $curval_sheet->layout->user($sheet->user_normal1);
 
     my $columns = $sheet->columns;