Associate threads with in_reply_to in gr-diff-comment-thread-group

The UI does not allow creating multiple comments at the same diff
location, but instead encourages contributing to the comment threads
already at a location. The uniqueness of thread locations was leveraged
to simplify the collection of comments into threads by mapping their
locations to keys.

However, the REST API does permit multiple threads starting from the
same diff location, so this expectation in the key-based UI threading
logic can be wrong. In such scenarios, an unresolved thread may be
mistakenly inserted into a resolved thread, resulting in a comment that
is not resolvable through the UI.

With this change, the threading logic in gr-diff-comment-thread-group is
updated to associate comments into threads via the `in_reply_to` graph
rather than location map in the same way that the
`unresolved_comment_count` detail property is determined server-side.

Bug: Issue 6779
Change-Id: Ibb29d4ba5c5de9e92dfbfba8ac7ac8037c332028

polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js

@@ -111,9 +111,11 @@
     },
 
     _getThreadGroups(comments) {
-      const threadGroups = {};
+      const sortedComments = comments.slice(0).sort((a, b) =>
+          util.parseDate(a.updated) - util.parseDate(b.updated));
 
-      for (const comment of comments) {
+      const threads = [];
+      for (const comment of sortedComments) {
         let locationRange;
         if (!comment.range) {
           locationRange = 'line-' + comment.__commentSide;
@@ -121,26 +123,27 @@
           locationRange = this._calculateLocationRange(comment.range, comment);
         }
 
-        if (threadGroups[locationRange]) {
-          threadGroups[locationRange].comments.push(comment);
-        } else {
-          threadGroups[locationRange] = {
-            start_datetime: comment.updated,
-            comments: [comment],
-            locationRange,
-            commentSide: comment.__commentSide,
-            patchNum: this._getPatchNum(comment),
-          };
+        // If the comment is in reply to another comment, find that comment's
+        // thread and append to it.
+        if (comment.in_reply_to) {
+          const thread = threads.find(thread =>
+              thread.comments.some(c => c.id === comment.in_reply_to));
+          if (thread) {
+            thread.comments.push(comment);
+            continue;
+          }
         }
-      }
 
-      const threadGroupArr = [];
-      const threadGroupKeys = Object.keys(threadGroups);
-      for (const threadGroupKey of threadGroupKeys) {
-        threadGroupArr.push(threadGroups[threadGroupKey]);
+        // Otherwise, this comment starts its own thread.
+        threads.push({
+          start_datetime: comment.updated,
+          comments: [comment],
+          locationRange,
+          commentSide: comment.__commentSide,
+          patchNum: this._getPatchNum(comment),
+        });
       }
-
-      return this._sortByDate(threadGroupArr);
+      return threads;
     },
   });
 })();

polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html

@@ -61,8 +61,9 @@ limitations under the License.
         }, {
           id: 'jacks_reply',
           message: 'i like you, too',
-          updated: '2015-12-24 15:00:20.396000000',
+          updated: '2015-12-24 15:01:20.396000000',
           __commentSide: 'left',
+          in_reply_to: 'sallys_confession',
         },
       ];
 
@@ -78,8 +79,9 @@ limitations under the License.
           }, {
             id: 'jacks_reply',
             message: 'i like you, too',
-            updated: '2015-12-24 15:00:20.396000000',
+            updated: '2015-12-24 15:01:20.396000000',
             __commentSide: 'left',
+            in_reply_to: 'sallys_confession',
           }],
           locationRange: 'line-left',
           patchNum: 3,
@@ -114,8 +116,9 @@ limitations under the License.
             __commentSide: 'left',
           }, {
             id: 'jacks_reply',
+            in_reply_to: 'sallys_confession',
             message: 'i like you, too',
-            updated: '2015-12-24 15:00:20.396000000',
+            updated: '2015-12-24 15:01:20.396000000',
             __commentSide: 'left',
           }],
           patchNum: 3,
@@ -145,6 +148,24 @@ limitations under the License.
           expectedThreadGroups);
     });
 
+    test('multiple comments at same location but not threaded', () => {
+      element.patchForNewThreads = 3;
+      const comments = [
+        {
+          id: 'sallys_confession',
+          message: 'i like you, jack',
+          updated: '2015-12-23 15:00:20.396000000',
+          __commentSide: 'left',
+        }, {
+          id: 'jacks_reply',
+          message: 'i like you, too',
+          updated: '2015-12-24 15:01:20.396000000',
+          __commentSide: 'left',
+        },
+      ];
+      assert.equal(element._getThreadGroups(comments).length, 2);
+    });
+
     test('_sortByDate', () => {
       let threadGroups = [
         {