Count unresolved threads within thread groups rather than by leaves

Diff comments are threaded together based on the in_reply_to relation
(which potentially expresses a tree structure) but are always displayed
linearly in the UI. This means that some comments in the middle of a
linear thread may be actually stored as leaves of a tree.

For example, the following thread of comments can be created if comments
two and three are created at nearly the same time.

Comment 1: thread root, unresolved,
┣ Comment 2: in reply to comment 1, unresolved,
┗ Comment 3: also in reply to comment 1, unresolved,
  ┗ Comment 4: in reply to comment 3, resolved

Because the thread is flattened, the resolved state of the thread should
be determined by the state of the chronologically latest comment (#4),
resulting in this thread being considered as resolved.

However, in a couple of locations, the resolved state is counted
differently. Namely, it finds the "leaf" comments -- that is, the
comments that are not marked as the parent of any other comment -- and
the number of unresolved threads is determined as the number of
unresolved leaves.

This approach was used by:
- ChangeComments#computeUnresolvedNum in the UI, which determines
  the string stating the number of unresolved threads in a file row.
- ChangeData#unresolvedCommentCount in the Java server code, which
  determines, among other things, the value of the
  unresolved_comment_count change detail property, as well as the Prolog
  fact used by the Prolog recipe that requires all comments to be
  resolved before a change can be submitted.

Instead, the unresolved thread logic is modified to group comments into
flat threads, and consider the resolved state of each one based on the
chronologically final comment, irregardless of the leaves.

Bug: Issue 8472
Change-Id: I2788fdb22ecfd56f0b3da763790a7732ec73be33

java/com/google/gerrit/server/query/change/ChangeData.java

@@ -18,7 +18,6 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.gerrit.server.ApprovalsUtil.sortApprovals;
 import static java.util.stream.Collectors.toList;
 import static java.util.stream.Collectors.toMap;
-import static java.util.stream.Collectors.toSet;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.base.MoreObjects;
@@ -882,15 +881,48 @@ public class ChangeData {
 
       List<Comment> comments =
           Stream.concat(publishedComments().stream(), robotComments().stream()).collect(toList());
-      Set<String> nonLeafSet = comments.stream().map(c -> c.parentUuid).collect(toSet());
 
-      Long count =
-          comments.stream().filter(c -> (c.unresolved && !nonLeafSet.contains(c.key.uuid))).count();
-      unresolvedCommentCount = count.intValue();
+      // Build a map of uuid to list of direct descendants.
+      Map<String, List<Comment>> forest = new HashMap<>();
+      for (Comment comment : comments) {
+        List<Comment> siblings = forest.get(comment.parentUuid);
+        if (siblings == null) {
+          siblings = new ArrayList<>();
+          forest.put(comment.parentUuid, siblings);
+        }
+        siblings.add(comment);
+      }
+
+      // Find latest comment in each thread and apply to unresolved counter.
+      int unresolved = 0;
+      if (forest.containsKey(null)) {
+        for (Comment root : forest.get(null)) {
+          if (getLatestComment(forest, root).unresolved) {
+            unresolved++;
+          }
+        }
+      }
+      unresolvedCommentCount = unresolved;
     }
+
     return unresolvedCommentCount;
   }
 
+  protected Comment getLatestComment(Map<String, List<Comment>> forest, Comment root) {
+    List<Comment> children = forest.get(root.key.uuid);
+    if (children == null) {
+      return root;
+    }
+    Comment latest = null;
+    for (Comment comment : children) {
+      Comment branchLatest = getLatestComment(forest, comment);
+      if (latest == null || branchLatest.writtenOn.after(latest.writtenOn)) {
+        latest = branchLatest;
+      }
+    }
+    return latest;
+  }
+
   public void setUnresolvedCommentCount(Integer count) {
     this.unresolvedCommentCount = count;
   }

polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.js

@@ -382,33 +382,26 @@
 
     comments = comments.concat(drafts);
 
-    // Create an object where every comment ID is the key of an unresolved
-    // comment.
-    const idMap = comments.reduce((acc, comment) => {
-      if (comment.unresolved) {
-        acc[comment.id] = true;
-      }
-      return acc;
-    }, {});
+    const threads = this.getCommentThreads(this._sortComments(comments));
 
-    // Set false for the comments that are marked as parents.
-    for (const comment of comments) {
-      idMap[comment.in_reply_to] = false;
-    }
+    const unresolvedThreads = threads
+      .filter(thread =>
+          thread.comments.length &&
+          thread.comments[thread.comments.length - 1].unresolved);
 
-    // The unresolved comments are the comments that still have true.
-    const unresolvedLeaves = Object.keys(idMap).filter(key => {
-      return idMap[key];
-    });
-    return unresolvedLeaves.length;
+    return unresolvedThreads.length;
   };
 
   ChangeComments.prototype.getAllThreadsForChange = function() {
     const comments = this._commentObjToArrayWithFile(this.getAllComments(true));
-    const sortedComments = comments.slice(0).sort((c1, c2) => {
+    const sortedComments = this._sortComments(comments);
+    return this.getCommentThreads(sortedComments);
+  };
+
+  ChangeComments.prototype._sortComments = function(comments) {
+    return comments.slice(0).sort((c1, c2) => {
       return util.parseDate(c1.updated) - util.parseDate(c2.updated);
     });
-    return this.getCommentThreads(sortedComments);
   };
 
   /**

polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.html

@@ -380,6 +380,39 @@ limitations under the License.
               .computeUnresolvedNum(2, 'file/three'), 1);
         });
 
+        test('computeUnresolvedNum w/ non-linear thread', () => {
+          element._changeComments._drafts = {};
+          element._changeComments._robotComments = {};
+          element._changeComments._comments = {
+            path: [{
+              id: '9c6ba3c6_28b7d467',
+              patch_set: 1,
+              updated: '2018-02-28 14:41:13.000000000',
+              unresolved: true,
+            }, {
+              id: '3df7b331_0bead405',
+              patch_set: 1,
+              in_reply_to: '1c346623_ab85d14a',
+              updated: '2018-02-28 23:07:55.000000000',
+              unresolved: false,
+            }, {
+              id: '6153dce6_69958d1e',
+              patch_set: 1,
+              in_reply_to: '9c6ba3c6_28b7d467',
+              updated: '2018-02-28 17:11:31.000000000',
+              unresolved: true,
+            }, {
+              id: '1c346623_ab85d14a',
+              patch_set: 1,
+              in_reply_to: '9c6ba3c6_28b7d467',
+              updated: '2018-02-28 23:01:39.000000000',
+              unresolved: false,
+            }],
+          };
+          assert.equal(
+              element._changeComments.computeUnresolvedNum(1, 'path'), 0);
+        });
+
         test('computeCommentCount', () => {
           assert.equal(element._changeComments
               .computeCommentCount(2, 'file/one'), 4);

polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.html

@@ -375,16 +375,19 @@ limitations under the License.
           message: 'test',
           patch_set: 1,
           unresolved: true,
+          updated: '2017-10-11 20:48:40.000000000',
         }],
         bar: [{
           id: '27dcee4d_f7b77cfa',
           message: 'test',
           patch_set: 1,
+          updated: '2017-10-12 20:48:40.000000000',
         },
         {
           id: '27dcee4d_f7b77cfa',
           message: 'test',
           patch_set: 1,
+          updated: '2017-10-13 20:48:40.000000000',
         }],
         abc: [],
       };