Bug 885646: Bugzilla::BugMail::_get_diff should rejoin split activity entries

r=glob, a=justdave

3 files changed, 60 insertions, 34 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index fc704df5e..40acca871 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -3942,8 +3942,8 @@ sub get_activity {
                 && ($attachid || 0) == ($operation->{'attachid'} || 0))
             {
                 my $old_change = pop @$changes;
-                $removed = _join_activity_entries($fieldname, $old_change->{'removed'}, $removed);
-                $added = _join_activity_entries($fieldname, $old_change->{'added'}, $added);
+                $removed = join_activity_entries($fieldname, $old_change->{'removed'}, $removed);
+                $added = join_activity_entries($fieldname, $old_change->{'added'}, $added);
             }
             $operation->{'who'} = $who;
             $operation->{'when'} = $when;
@@ -3968,35 +3968,6 @@ sub get_activity {
     return(\@operations, $incomplete_data);
 }
 
-sub _join_activity_entries {
-    my ($field, $current_change, $new_change) = @_;
-    # We need to insert characters as these were removed by old
-    # LogActivityEntry code.
-
-    return $new_change if $current_change eq '';
-
-    # Buglists and see_also need the comma restored
-    if ($field eq 'dependson' || $field eq 'blocked' || $field eq 'see_also') {
-        if (substr($new_change, 0, 1) eq ',' || substr($new_change, 0, 1) eq ' ') {
-            return $current_change . $new_change;
-        } else {
-            return $current_change . ', ' . $new_change;
-        }
-    }
-
-    # Assume bug_file_loc contain a single url, don't insert a delimiter
-    if ($field eq 'bug_file_loc') {
-        return $current_change . $new_change;
-    }
-
-    # All other fields get a space
-    if (substr($new_change, 0, 1) eq ' ') {
-        return $current_change . $new_change;
-    } else {
-        return $current_change . ' ' . $new_change;
-    }
-}
-
 # Update the bugs_activity table to reflect changes made in bugs.
 sub LogActivityEntry {
     my ($i, $col, $removed, $added, $whoid, $timestamp, $comment_id) = @_;
diff --git a/Bugzilla/BugMail.pm b/Bugzilla/BugMail.pm
index 68962fc66..ee25613b7 100644
--- a/Bugzilla/BugMail.pm
+++ b/Bugzilla/BugMail.pm
@@ -416,7 +416,8 @@ sub _get_diffs {
                 ON fielddefs.id = bugs_activity.fieldid
              WHERE bugs_activity.bug_id = ?
                    $when_restriction
-          ORDER BY bugs_activity.bug_when", {Slice=>{}}, @args);
+          ORDER BY bugs_activity.bug_when, bugs_activity.id",
+        {Slice=>{}}, @args);
 
     foreach my $diff (@$diffs) {
         $user_cache->{$diff->{who}} ||= new Bugzilla::User($diff->{who}); 
@@ -433,7 +434,25 @@ sub _get_diffs {
          }
     }
 
-    return @$diffs;
+    my @changes = ();
+    foreach my $diff (@$diffs) {
+        # If this is the same field as the previous item, then concatenate
+        # the data into the same change.
+        if (scalar(@changes)
+            && $diff->{field_name}        eq $changes[-1]->{field_name}
+            && $diff->{bug_when}          eq $changes[-1]->{bug_when}
+            && $diff->{who}               eq $changes[-1]->{who}
+            && ($diff->{attach_id} // 0)  == ($changes[-1]->{attach_id} // 0)
+            && ($diff->{comment_id} // 0) == ($changes[-1]->{comment_id} // 0)
+        ) {
+            my $old_change = pop @changes;
+            $diff->{old} = join_activity_entries($diff->{field_name}, $old_change->{old}, $diff->{old});
+            $diff->{new} = join_activity_entries($diff->{field_name}, $old_change->{new}, $diff->{new});
+        }
+        push @changes, $diff;
+    }
+
+    return @changes;
 }
 
 sub _get_new_bugmail_fields {
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index cee12ee21..58d6ab365 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -22,7 +22,8 @@ use parent qw(Exporter);
                              is_7bit_clean bz_crypt generate_random_password
                              validate_email_syntax check_email_syntax clean_text
                              get_text template_var disable_utf8
-                             detect_encoding email_filter);
+                             detect_encoding email_filter
+                             join_activity_entries);
 
 use Bugzilla::Constants;
 use Bugzilla::RNG qw(irand);
@@ -464,6 +465,35 @@ sub find_wrap_point {
     return $wrappoint;
 }
 
+sub join_activity_entries {
+    my ($field, $current_change, $new_change) = @_;
+    # We need to insert characters as these were removed by old
+    # LogActivityEntry code.
+
+    return $new_change if $current_change eq '';
+
+    # Buglists and see_also need the comma restored
+    if ($field eq 'dependson' || $field eq 'blocked' || $field eq 'see_also') {
+        if (substr($new_change, 0, 1) eq ',' || substr($new_change, 0, 1) eq ' ') {
+            return $current_change . $new_change;
+        } else {
+            return $current_change . ', ' . $new_change;
+        }
+    }
+
+    # Assume bug_file_loc contain a single url, don't insert a delimiter
+    if ($field eq 'bug_file_loc') {
+        return $current_change . $new_change;
+    }
+
+    # All other fields get a space
+    if (substr($new_change, 0, 1) eq ' ') {
+        return $current_change . $new_change;
+    } else {
+        return $current_change . ' ' . $new_change;
+    }
+}
+
 sub wrap_hard {
     my ($string, $columns) = @_;
     local $Text::Wrap::columns = $columns;
@@ -1012,6 +1042,12 @@ Search for a comma, a whitespace or a hyphen to split $string, within the first
 $maxpos characters. If none of them is found, just split $string at $maxpos.
 The search starts at $maxpos and goes back to the beginning of the string.
 
+=item C<join_activity_entries($field, $current_change, $new_change)>
+
+Joins two strings together so they appear as one. The field name is specified
+as the method of joining the two strings depends on this. Returns the
+combined string.
+
 =item C<is_7bit_clean($str)>
 
 Returns true is the string contains only 7-bit characters (ASCII 32 through 126,