commit 6b048edeffe108d845413f7a213e0f3bd49fb58c
author Alice Kober-Sotzek <aliceks@google.com> Fri Sep 11 10:12:10 2020 +0200
committer Alice Kober-Sotzek <aliceks@google.com> Wed Sep 16 11:20:09 2020 +0200
tree 54604bcdf0c6bd3108841792c420d5a8246a84d4
parent 159e70fd80e77327c588fc06b6f069d3d66ad7b6

Only port unresolved comment threads

Porting all comments from previous patchsets to the requested patchset
could be a large amount of comments especially if there was a lot of
activity (conversations + code adjustments) on the change in the past.
In addition, some of the comment threads might not even apply to the
later patchsets anymore as they were already addressed. To increase
the overview for users, we decide to port only unresolved comment
threads. We filter the comments already in the backend as we can thus
achieve the best performance (less comments to port -> less diffs to
compute, which can be a big amount otherwise for higher patchset
numbers).

We apply that filtering approach both to published and draft comments.
However, we don't factor in the unresolved state of draft comments which
are a reply to published comments when deciding whether to port the
published comment thread. We could have done this but it would have
added additional complexity. Furthermore, this would have introduced
a dependency of published comments on draft comments which we typically
don't have for endpoints of published comments. There's also always
the risk that Gerrit's state changes between the calls to the two
different endpoints for published and draft comments (e.g. draft
comment which changed the unresolved state is discarded), in which case
the published comment endpoint would return the wrong result.

For the frontend, this behavior means that it might get a ported draft
comment for which the published comment thread was not ported (or the
other way around). In such a case, the frontend should simply re-use the
original comment thread (which it loaded anyway as the frontend loads
all comments on a change) and put it to the file/range indicated by the
ported draft comment. The frontend needs such an approach anyway as we
don't port robot comments to which a published/draft comment can also
be a reply.

There is one corner case not implemented right now: If a robot comment
is in the middle of a comment thread, we might derive the wrong
unresolved state for the thread as we internally consider the thread
as two threads (-> one before the robot comment, one afterwards).
In theory, comment threads involving robot comments in the middle
are possible with the current APIs. However, we don't know of anybody
using such a bot at the moment. If somebody does, they might see
other issues as well as this corner case is not explicitly supported in
other places of Gerrit either. If we wanted to fix this corner case,
we'd need to load robot comments too and then filter them out before
porting the comments (or returning them). We didn't want to add this
additional complexity at the moment but can do so whenever we need it.

The same situation shouldn't happen for draft comments as drafts will
be considered to be at the end of a comment thread. Even if a draft
comment was added prior to another reply on the thread, its date will
be adjusted when published and hence it will move to the end of the
thread. In addition, published comments can't be a reply to a draft
comment.

Filtering out all resolved comment threads might be too aggressive in
some situations. Reviewers might especially want to see comment threads
which were resolved for the target patchset. The current fields on
comments don't allow us to identify this situation, though, as the
patchset field of a comment always points to the patchset on which the
root of the comment thread was left. We also shouldn't change this,
especially as it might break other tools. We'd need another solution
which we can add when we see that the current, simpler approach isn't
sufficient enough.

Change-Id: I4af5946ce34b9f4998d4922c6b51c585bc687c66