Update attention set behaviour when commenting on a thread.
Previously the rule worked like this:
When reply comment is added on a thread, every previous commenters
(except for the user leaving a reply) on the thread is added if:
- They are not the user leaving a reply
- If thread is unresolved
- If thread is resolved, and the commenter have not voted Code-Review
with a maximum vote yet.
After this change:
- If change owner replies and thread is unresolved: every commenter on
the thread is brought back to attention set
- If change owner replies and thread is resolved: only commenters that
haven't voted Code-Review are brought to attention set
- If reviewer replies and thread is unresolved, only bring owner to
attention set
- If reviewer replies and thread is resolved, bring all other commenters
who haven't voted Code-Review to attention set.
The logic doesn't account for users who don't have permissions to leave
a max CR vote. But this is the same for the current state as well. This
can be a further improvement in the future.
This is FE-only change, this will affect only comments published via
Web-UI. There will be a corresponding check to the BE logic in a
subsequent change.
Release-Notes: Attention set when commenting on a thread takes into
account owner and unresolved status.
Google-Bug-Id: b/347949247
Change-Id: I849a8dda069c74d97c75ce86706f35ea64717670
(cherry picked from commit f42012d837c1e15e0a598f41b66c999bae6ecb67)
diff --git a/Documentation/user-attention-set.txt b/Documentation/user-attention-set.txt
index 9825478..469fcdd 100644
--- a/Documentation/user-attention-set.txt
+++ b/Documentation/user-attention-set.txt
@@ -32,12 +32,20 @@
changing the attention set:
* If reviewers are added to a change, then they are added to the attention set.
- * Exception: A reviewer adding themselves along with a comment or vote.
+ ** Exception: A reviewer adding themselves along with a comment or vote.
* If an active change is submitted, abandoned or reset to "work in progress",
then all users are removed from the attention set.
* Replying (commenting, voting or just writing a change message) removes the
replying user from the attention set. And it adds all participants of comment
- conversations that the user is replying to.
+ conversations that the user is replying to. Specifically
+ ** If owner is replying and thread is resolved: all participants who have not
+ given Code-Review yet, are added to the attention set.
+ ** If owner is replying and thread is unresolved: all participants are added
+ to the attention set.
+ ** If non-owner is replying and thread is unresolved: only owner is added to
+ the attention set.
+ ** If non-owner is replying and thread is resolved: all participants who have
+ not given Code-Review yet, are added to the attention set.
* If a *reviewer* replies, then the change owner (and uploader) are added to the
attention set.
* For merged and abandoned changes the owner is added only when a human creates
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index c76e928..e6717b2 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -1687,9 +1687,10 @@
if (this.change.status === ChangeStatus.NEW) {
// Add everyone that the user is replying to in a comment thread.
- this.computeCommentAccounts(draftCommentThreads).forEach(id =>
- newAttention.add(id)
- );
+ this.computeCommentAccountsForAttention(
+ draftCommentThreads,
+ isUploader
+ ).forEach(id => newAttention.add(id));
// Remove the current user.
newAttention.delete(this.account._account_id);
// Add all new reviewers, but not the current reviewer, if they are also
@@ -1756,18 +1757,48 @@
return this.isOwner && addedIds.length >= minimum;
}
- computeCommentAccounts(threads: CommentThread[]) {
+ /**
+ * Pick previous commenters for addition to attention set.
+ *
+ * For every thread:
+ * - If owner replied and thread is unresolved: add all commenters.
+ * - If owner replied and thread is resolved: add commenters who need to vote.
+ * - If reviewer replied and thread is resolved: add commenters who need to vote.
+ * - If reviewer replied and thread is unresolved: only add owner
+ * (owner added outside this function).
+ */
+ computeCommentAccountsForAttention(
+ threads: CommentThread[],
+ isUploader: boolean
+ ) {
const crLabel = this.change?.labels?.[StandardLabels.CODE_REVIEW];
const maxCrVoteAccountIds = getMaxAccounts(crLabel).map(a => a._account_id);
const accountIds = new Set<AccountId>();
threads.forEach(thread => {
const unresolved = isUnresolved(thread);
+ let ignoreVoteCheck = false;
+ if (unresolved) {
+ if (this.isOwner || isUploader) {
+ // Owner replied but didn't resolve, we assume clarification was asked
+ // add everyone on the thread to attention set.
+ ignoreVoteCheck = true;
+ } else {
+ // Reviewer replied owner is still the one to act. No need to add
+ // commenters.
+ return;
+ }
+ }
+ // If thread is resolved, we only bring back the commenters who have not
+ // yet left max Code-Review vote.
thread.comments.forEach(comment => {
if (comment.author) {
// A comment author must have an account_id.
const authorId = comment.author._account_id!;
- const hasGivenMaxReviewVote = maxCrVoteAccountIds.includes(authorId);
- if (unresolved || !hasGivenMaxReviewVote) accountIds.add(authorId);
+ const needsToVote =
+ !maxCrVoteAccountIds.includes(authorId) && // Didn't give max-vote
+ this.uploader?._account_id !== authorId && // Not uploader
+ this.change?.owner._account_id !== authorId; // Not owner
+ if (ignoreVoteCheck || needsToVote) accountIds.add(authorId);
}
});
});
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
index 7c8aece..d7f8946 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
@@ -1161,7 +1161,72 @@
);
});
- test('computeCommentAccounts', () => {
+ test('computeCommentAccountsForAttention owner comments', () => {
+ element.change = {
+ ...createChange(),
+ labels: {
+ 'Code-Review': {
+ all: [
+ {_account_id: 1 as AccountId, value: 0},
+ {_account_id: 2 as AccountId, value: 1},
+ {_account_id: 3 as AccountId, value: 2},
+ ],
+ values: {
+ '-2': 'This shall not be submitted',
+ '-1': 'I would prefer this is not submitted as is',
+ ' 0': 'No score',
+ '+1': 'Looks good to me, but someone else must approve',
+ '+2': 'Looks good to me, approved',
+ },
+ },
+ },
+ };
+ element.isOwner = true;
+ const threads = [
+ {
+ ...createCommentThread([
+ {
+ ...createComment(),
+ id: '1' as UrlEncodedCommentId,
+ author: {_account_id: 1 as AccountId},
+ unresolved: false,
+ },
+ {
+ ...createComment(),
+ id: '2' as UrlEncodedCommentId,
+ in_reply_to: '1' as UrlEncodedCommentId,
+ author: {_account_id: 2 as AccountId},
+ unresolved: true,
+ },
+ ]),
+ },
+ {
+ ...createCommentThread([
+ {
+ ...createComment(),
+ id: '3' as UrlEncodedCommentId,
+ author: {_account_id: 3 as AccountId},
+ unresolved: false,
+ },
+ {
+ ...createComment(),
+ id: '4' as UrlEncodedCommentId,
+ in_reply_to: '3' as UrlEncodedCommentId,
+ author: {_account_id: 4 as AccountId},
+ unresolved: false,
+ },
+ ]),
+ },
+ ];
+ const actualAccounts = [
+ ...element.computeCommentAccountsForAttention(threads, false),
+ ];
+ // Account 3 is not included, because the comment is resolved *and* they
+ // have given the highest possible vote on the Code-Review label.
+ assert.sameMembers(actualAccounts, [1, 2, 4]);
+ });
+
+ test('computeCommentAccountsForAttention reviewer comments', () => {
element.change = {
...createChange(),
labels: {
@@ -1211,16 +1276,30 @@
...createComment(),
id: '4' as UrlEncodedCommentId,
in_reply_to: '3' as UrlEncodedCommentId,
+ author: element.change.owner,
+ unresolved: false,
+ },
+ {
+ ...createComment(),
+ id: '5' as UrlEncodedCommentId,
+ in_reply_to: '4' as UrlEncodedCommentId,
author: {_account_id: 4 as AccountId},
unresolved: false,
},
]),
},
];
- const actualAccounts = [...element.computeCommentAccounts(threads)];
+ const actualAccounts = [
+ ...element.computeCommentAccountsForAttention(threads, false),
+ ];
+ // Accounts 1 and 2 are not included, because the thread is still unresolved
+ // and the new comment is from another reviewer.
// Account 3 is not included, because the comment is resolved *and* they
// have given the highest possible vote on the Code-Review label.
- assert.sameMembers(actualAccounts, [1, 2, 4]);
+ // element.change.owner is similarly not included, because they don't need
+ // to vote. (In the overall logic owner is added as part of
+ // computeNewAttention)
+ assert.sameMembers(actualAccounts, [4]);
});
test('label picker', async () => {
