Update attention set when uploading a new patchset with comments
When uploading a new patchset with --publish-comments, attention
set should update the same way as in PostReview. This change
reformats the class in charge of those updates, and ensures that
both PostReview and ReceiveCommits use this same functionality.
Change-Id: I120dc69ebb1b638bdef2af229b1b92d1db4ec333
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index e9bdc25..8f20ac5 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -7430,24 +7430,24 @@
[options="header",cols="1,^1,5"]
|============================
-|Field Name ||Description
-|`message` |optional|
+|Field Name ||Description
+|`message` |optional|
The message to be added as review comment.
-|`tag` |optional|
+|`tag` |optional|
Apply this tag to the review comment message, votes, and inline
comments. Tags may be used by CI or other automated systems to
distinguish them from human reviews. Votes/comments that contain `tag` with
'autogenerated:' prefix can be filtered out in the web UI.
-|`labels` |optional|
+|`labels` |optional|
The votes that should be added to the revision as a map that maps the
label names to the voting values.
-|`comments` |optional|
+|`comments` |optional|
The comments that should be added as a map that maps a file path to a
list of link:#comment-input[CommentInput] entities.
-|`robot_comments` |optional|
+|`robot_comments` |optional|
The robot comments that should be added as a map that maps a file path
to a list of link:#robot-comment-input[RobotCommentInput] entities.
-|`drafts` |optional|
+|`drafts` |optional|
Draft handling that defines how draft comments are handled that are
already in the database but that were not also described in this
input. +
@@ -7456,37 +7456,37 @@
Only `KEEP` is allowed when used in conjunction with `on_behalf_of`. +
If not set, the default is `KEEP`. If `on_behalf_of` is set, then no other value
besides `KEEP` is allowed.
-|`notify` |optional|
+|`notify` |optional|
Notify handling that defines to whom email notifications should be sent
after the review is stored. +
Allowed values are `NONE`, `OWNER`, `OWNER_REVIEWERS` and `ALL`. +
If not set, the default is `ALL`.
-|`notify_details` |optional|
+|`notify_details` |optional|
Additional information about whom to notify about the update as a map
of recipient type to link:#notify-info[NotifyInfo] entity.
-|`omit_duplicate_comments` |optional|
+|`omit_duplicate_comments` |optional|
If `true`, comments with the same content at the same place will be omitted.
-|`on_behalf_of` |optional|
+|`on_behalf_of` |optional|
link:rest-api-accounts.html#account-id[\{account-id\}] the review
should be posted on behalf of. To use this option the caller must
have been granted `labelAs-NAME` permission for all keys of labels.
-|`reviewers` |optional|
+|`reviewers` |optional|
A list of link:rest-api-changes.html#reviewer-input[ReviewerInput]
representing reviewers that should be added to the change.
-|`ready` |optional|
+|`ready` |optional|
If true, and if the change is work in progress, then start review.
It is an error for both `ready` and `work_in_progress` to be true.
-|`work_in_progress` |optional|
+|`work_in_progress` |optional|
If true, mark the change as work in progress. It is an error for both
`ready` and `work_in_progress` to be true.
-|`add_to_attention_set` |optional|
+|`add_to_attention_set` |optional|
list of link:#attention-set-input[AttentionSetInput] entities to add
to the link:#attention-set[attention set].
-|`remove_from_attention_set` |optional|
+|`remove_from_attention_set` |optional|
list of link:#attention-set-input[AttentionSetInput] entities to remove
from the link:#attention-set[attention set].
-|`ignore_default_attention_set_rules`|optional|
-If set to true, ignore all default attention set rules described in the
+|`ignore_automatic_attention_set_rules`|optional|
+If set to true, ignore all automatic attention set rules described in the
link:#attention-set[attention set]. Updates in add_to_attention_set
and remove_from_attention_set are not ignored.
|============================
diff --git a/java/com/google/gerrit/extensions/api/changes/ReviewInput.java b/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
index 1f41c07..a213ed6 100644
--- a/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
+++ b/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
@@ -87,7 +87,7 @@
* occur. E.g, adding/removing reviewers, marking a change ready for review or work in progress,
* and replying on changes.
*/
- public boolean ignoreDefaultAttentionSetRules;
+ public boolean ignoreAutomaticAttentionSetRules;
public enum DraftHandling {
/** Leave pending drafts alone. */
@@ -175,8 +175,8 @@
return this;
}
- public ReviewInput blockDefaultAttentionSetRules() {
- ignoreDefaultAttentionSetRules = true;
+ public ReviewInput blockAutomaticAttentionSetRules() {
+ ignoreAutomaticAttentionSetRules = true;
return this;
}
diff --git a/java/com/google/gerrit/server/git/receive/BUILD b/java/com/google/gerrit/server/git/receive/BUILD
index 766a835..b59d431 100644
--- a/java/com/google/gerrit/server/git/receive/BUILD
+++ b/java/com/google/gerrit/server/git/receive/BUILD
@@ -19,6 +19,7 @@
"//java/com/google/gerrit/metrics",
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/server/logging",
+ "//java/com/google/gerrit/server/restapi",
"//java/com/google/gerrit/server/util/time",
"//java/com/google/gerrit/util/cli",
"//lib:args4j",
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index c3551cc..02747bb 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -161,6 +161,7 @@
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
+import com.google.gerrit.server.restapi.change.ReplyAttentionSetUpdates;
import com.google.gerrit.server.submit.MergeOp;
import com.google.gerrit.server.submit.MergeOpRepoManager;
import com.google.gerrit.server.submit.SubmoduleOp;
@@ -345,6 +346,7 @@
private final TagCache tagCache;
private final ProjectConfig.Factory projectConfigFactory;
private final SetPrivateOp.Factory setPrivateOpFactory;
+ private final ReplyAttentionSetUpdates replyAttentionSetUpdates;
// Assisted injected fields.
private final ProjectState projectState;
@@ -424,6 +426,7 @@
SubmoduleOp.Factory subOpFactory,
TagCache tagCache,
SetPrivateOp.Factory setPrivateOpFactory,
+ ReplyAttentionSetUpdates replyAttentionSetUpdates,
@Assisted ProjectState projectState,
@Assisted IdentifiedUser user,
@Assisted ReceivePack rp,
@@ -471,6 +474,7 @@
this.tagCache = tagCache;
this.projectConfigFactory = projectConfigFactory;
this.setPrivateOpFactory = setPrivateOpFactory;
+ this.replyAttentionSetUpdates = replyAttentionSetUpdates;
// Assisted injected fields.
this.projectState = projectState;
@@ -922,6 +926,28 @@
bu.addOp(
replace.notes.getChangeId(),
publishCommentsOp.create(replace.psId, project.getNameKey()));
+ Optional<ChangeNotes> changeNotes = getChangeNotes(replace.notes.getChangeId());
+ if (!changeNotes.isPresent()) {
+ // If not present, no need to update attention set here since this is a new change.
+ continue;
+ }
+ List<HumanComment> drafts =
+ commentsUtil.draftByChangeAuthor(changeNotes.get(), user.getAccountId());
+ if (drafts.isEmpty()) {
+ // If no comments, attention set shouldn't update since the user didn't reply.
+ continue;
+ }
+ // Reviewers can only be removed by becoming CCs.
+ Set<String> potentiallyRemovedReviewers =
+ magicBranch.getCombinedCcs(
+ getRecipientsFromFooters(
+ accountResolver, replace.revCommit.getFooterLines()));
+ replyAttentionSetUpdates.processAutomaticAttentionSetRulesOnReply(
+ bu,
+ changeNotes.get(),
+ isReadyForReview(changeNotes.get()),
+ potentiallyRemovedReviewers,
+ user.getAccountId());
}
}
}
@@ -976,7 +1002,11 @@
} catch (ResourceConflictException e) {
addError(e.getMessage());
reject(magicBranchCmd, "conflict");
- } catch (BadRequestException | UnprocessableEntityException | AuthException e) {
+ } catch (BadRequestException
+ | UnprocessableEntityException
+ | AuthException
+ | ConfigInvalidException
+ | PermissionBackendException e) {
logger.atFine().withCause(e).log("Rejecting due to client error");
reject(magicBranchCmd, e.getMessage());
} catch (RestApiException | IOException e) {
@@ -1002,6 +1032,11 @@
}
}
+ private boolean isReadyForReview(ChangeNotes changeNotes) {
+ return (!changeNotes.getChange().isWorkInProgress() && !magicBranch.workInProgress)
+ || magicBranch.ready;
+ }
+
private String buildError(String error, List<String> branches) {
StringBuilder sb = new StringBuilder();
if (branches.size() == 1) {
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index e9fd99d..3b986ea 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -178,7 +178,7 @@
private final ProjectCache projectCache;
private final PermissionBackend permissionBackend;
private final PluginSetContext<CommentValidator> commentValidators;
- private final PostReviewAttentionSet postReviewAttentionSet;
+ private final ReplyAttentionSetUpdates replyAttentionSetUpdates;
private final boolean strictLabels;
@Inject
@@ -203,7 +203,7 @@
ProjectCache projectCache,
PermissionBackend permissionBackend,
PluginSetContext<CommentValidator> commentValidators,
- PostReviewAttentionSet postReviewAttentionSet) {
+ ReplyAttentionSetUpdates replyAttentionSetUpdates) {
this.updateFactory = updateFactory;
this.changeResourceFactory = changeResourceFactory;
this.changeDataFactory = changeDataFactory;
@@ -223,7 +223,7 @@
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
this.commentValidators = commentValidators;
- this.postReviewAttentionSet = postReviewAttentionSet;
+ this.replyAttentionSetUpdates = replyAttentionSetUpdates;
this.strictLabels = gerritConfig.getBoolean("change", "strictLabels", false);
}
@@ -383,7 +383,8 @@
bu.setNotify(notify);
// Adjust the attention set based on the input
- postReviewAttentionSet.updateAttentionSet(bu, revision, input, reviewerResults);
+ replyAttentionSetUpdates.updateAttentionSet(
+ bu, revision.getNotes(), input, reviewerResults, revision.getAccountId());
bu.execute();
// Re-read change to take into account results of the update.
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewAttentionSet.java b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
similarity index 60%
rename from java/com/google/gerrit/server/restapi/change/PostReviewAttentionSet.java
rename to java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
index aeb2c2e..c7b5ad4 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewAttentionSet.java
+++ b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
@@ -15,12 +15,12 @@
package com.google.gerrit.server.restapi.change;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
-import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toSet;
import com.google.gerrit.entities.Account;
import com.google.gerrit.extensions.api.changes.AttentionSetInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
@@ -30,7 +30,7 @@
import com.google.gerrit.server.change.AttentionSetUnchangedOp;
import com.google.gerrit.server.change.RemoveFromAttentionSetOp;
import com.google.gerrit.server.change.ReviewerAdder;
-import com.google.gerrit.server.change.RevisionResource;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -42,12 +42,13 @@
import java.util.HashSet;
import java.util.List;
import java.util.Set;
+import java.util.stream.Collectors;
import org.eclipse.jgit.errors.ConfigInvalidException;
/**
- * This class is used by {@link PostReview} to update the attention set when performing a review.
+ * This class is used to update the attention set when performing a review or replying on a change.
*/
-public class PostReviewAttentionSet {
+public class ReplyAttentionSetUpdates {
private final PermissionBackend permissionBackend;
private final AddToAttentionSetOp.Factory addToAttentionSetOpFactory;
@@ -56,7 +57,7 @@
private final AccountResolver accountResolver;
@Inject
- PostReviewAttentionSet(
+ ReplyAttentionSetUpdates(
PermissionBackend permissionBackend,
AddToAttentionSetOp.Factory addToAttentionSetOpFactory,
RemoveFromAttentionSetOp.Factory removeFromAttentionSetOpFactory,
@@ -69,6 +70,28 @@
this.accountResolver = accountResolver;
}
+ /** Adjusts the attention set but only based on the automatic rules. */
+ public void processAutomaticAttentionSetRulesOnReply(
+ BatchUpdate bu,
+ ChangeNotes changeNotes,
+ boolean readyForReview,
+ Set<String> potentiallyRemovedReviewers,
+ Account.Id currentUser)
+ throws IOException, ConfigInvalidException, PermissionBackendException,
+ UnprocessableEntityException {
+
+ Set<Account.Id> potentiallyRemovedReviewerIds = new HashSet();
+ for (String reviewer : potentiallyRemovedReviewers) {
+ potentiallyRemovedReviewerIds.add(getAccountId(changeNotes, reviewer));
+ }
+ processRules(
+ bu,
+ changeNotes,
+ readyForReview,
+ getUpdatedReviewers(changeNotes, potentiallyRemovedReviewerIds),
+ currentUser);
+ }
+
/**
* Adjusts the attention set by adding and removing users. If the same user should be added and
* removed or added/removed twice, the user will only be added/removed once, based on first
@@ -76,68 +99,80 @@
*/
public void updateAttentionSet(
BatchUpdate bu,
- RevisionResource revision,
+ ChangeNotes changeNotes,
ReviewInput input,
- List<ReviewerAdder.ReviewerAddition> reviewerResults)
+ List<ReviewerAdder.ReviewerAddition> reviewerResults,
+ Account.Id currentUser)
throws BadRequestException, IOException, PermissionBackendException,
UnprocessableEntityException, ConfigInvalidException {
- processManualUpdates(bu, revision, input);
- if (input.ignoreDefaultAttentionSetRules) {
+ processManualUpdates(bu, changeNotes, input);
+ if (input.ignoreAutomaticAttentionSetRules) {
- // If We ignore default attention set rules it means we need to pass this information to
- // ChangeUpdate. Also, we should stop all other attention set update that are part of
- // this method (that happen in PostReview.
- bu.addOp(revision.getChange().getId(), new AttentionSetUnchangedOp());
+ // If we ignore automatic attention set rules it means we need to pass this information to
+ // ChangeUpdate. Also, we should stop all other attention set updates that are part of
+ // this method and happen in PostReview.
+ bu.addOp(changeNotes.getChangeId(), new AttentionSetUnchangedOp());
return;
}
- processRules(bu, revision, input, reviewerResults);
+ // Gets a set of all the CCs in this change. Updated reviewers will be defined as reviewers who
+ // didn't become CC (therefore this is a set of potentially removed reviewers - those that were
+ // reviewers but became cc).
+ Set<Account.Id> potentiallyRemovedReviewers =
+ reviewerResults.stream()
+ .filter(r -> r.state() == ReviewerState.CC)
+ .map(r -> r.reviewers)
+ .flatMap(x -> x.stream())
+ .collect(toSet());
+ processRules(
+ bu,
+ changeNotes,
+ isReadyForReview(changeNotes, input),
+ getUpdatedReviewers(changeNotes, potentiallyRemovedReviewers),
+ currentUser);
+ }
+
+ private Set<Account.Id> getUpdatedReviewers(
+ ChangeNotes changeNotes, Set<Account.Id> potentiallyRemovedReviewers) {
+ // Filter by users that are currently reviewers and remove CCs.
+ return approvalsUtil.getReviewers(changeNotes).byState(REVIEWER).stream()
+ .filter(r -> !potentiallyRemovedReviewers.contains(r))
+ .collect(Collectors.toSet());
}
/**
- * Process the default rules of the attention set. All of the default rules except adding/removing
- * reviewers and entering/exiting WIP state are done here, and the rest are done in {@link
- * ChangeUpdate}
+ * Process the automatic rules of the attention set. All of the automatic rules except
+ * adding/removing reviewers and entering/exiting WIP state are done here, and the rest are done
+ * in {@link ChangeUpdate}
*/
private void processRules(
BatchUpdate bu,
- RevisionResource revision,
- ReviewInput input,
- List<ReviewerAdder.ReviewerAddition> reviewerResults) {
+ ChangeNotes changeNotes,
+ boolean readyForReview,
+ Set<Account.Id> reviewers,
+ Account.Id currentUser) {
// Replying removes the publishing user from the attention set.
RemoveFromAttentionSetOp removeFromAttentionSetOp =
- removeFromAttentionSetOpFactory.create(revision.getAccountId(), "removed on reply");
- bu.addOp(revision.getChange().getId(), removeFromAttentionSetOp);
+ removeFromAttentionSetOpFactory.create(currentUser, "removed on reply");
+ bu.addOp(changeNotes.getChangeId(), removeFromAttentionSetOp);
// The rest of the conditions only apply if the change is ready for review
- if (!isReadyForReview(revision, input)) {
+ if (!readyForReview) {
return;
}
- Account.Id uploader = revision.getPatchSet().uploader();
- Account.Id owner = revision.getChange().getOwner();
- Account.Id currentUser = revision.getAccountId();
+ Account.Id uploader = changeNotes.getCurrentPatchSet().uploader();
+ Account.Id owner = changeNotes.getChange().getOwner();
if (currentUser.equals(uploader) && !uploader.equals(owner)) {
// When the uploader replies, add the owner to the attention set.
AddToAttentionSetOp addToAttentionSetOp =
addToAttentionSetOpFactory.create(owner, "uploader replied");
- bu.addOp(revision.getChange().getId(), addToAttentionSetOp);
+ bu.addOp(changeNotes.getChangeId(), addToAttentionSetOp);
}
if (currentUser.equals(uploader) || currentUser.equals(owner)) {
// When the owner or uploader replies, add the reviewers to the attention set.
- // Filter by users that are currently reviewers.
- Set<Account.Id> finalCCs =
- reviewerResults.stream()
- .filter(r -> r.result.ccs == null)
- .map(r -> r.reviewers)
- .flatMap(x -> x.stream())
- .collect(toSet());
- for (Account.Id reviewer :
- approvalsUtil.getReviewers(revision.getChangeResource().getNotes()).byState(REVIEWER)
- .stream()
- .filter(r -> !finalCCs.contains(r))
- .collect(toList())) {
+ for (Account.Id reviewer : reviewers) {
AddToAttentionSetOp addToAttentionSetOp =
addToAttentionSetOpFactory.create(reviewer, "owner or uploader replied");
- bu.addOp(revision.getChange().getId(), addToAttentionSetOp);
+ bu.addOp(changeNotes.getChangeId(), addToAttentionSetOp);
}
}
if (!currentUser.equals(uploader) && !currentUser.equals(owner)) {
@@ -145,24 +180,24 @@
// uploader to the attention set.
AddToAttentionSetOp addToAttentionSetOp =
addToAttentionSetOpFactory.create(owner, "reviewer or cc replied");
- bu.addOp(revision.getChange().getId(), addToAttentionSetOp);
+ bu.addOp(changeNotes.getChangeId(), addToAttentionSetOp);
if (owner.get() != uploader.get()) {
addToAttentionSetOp = addToAttentionSetOpFactory.create(uploader, "reviewer or cc replied");
- bu.addOp(revision.getChange().getId(), addToAttentionSetOp);
+ bu.addOp(changeNotes.getChangeId(), addToAttentionSetOp);
}
}
}
/** Process the manual updates of the attention set. */
- private void processManualUpdates(BatchUpdate bu, RevisionResource revision, ReviewInput input)
+ private void processManualUpdates(BatchUpdate bu, ChangeNotes changeNotes, ReviewInput input)
throws BadRequestException, IOException, PermissionBackendException,
UnprocessableEntityException, ConfigInvalidException {
Set<Account.Id> accountsChangedInCommit = new HashSet<>();
// If we specify a user to remove, and the user is in the attention set, we remove it.
if (input.removeFromAttentionSet != null) {
for (AttentionSetInput remove : input.removeFromAttentionSet) {
- removeFromAttentionSet(bu, revision, remove, accountsChangedInCommit);
+ removeFromAttentionSet(bu, changeNotes, remove, accountsChangedInCommit);
}
}
@@ -170,61 +205,68 @@
// added if they are not in the attention set yet.
if (input.addToAttentionSet != null) {
for (AttentionSetInput add : input.addToAttentionSet) {
- addToAttentionSet(bu, revision, add, accountsChangedInCommit);
+ addToAttentionSet(bu, changeNotes, add, accountsChangedInCommit);
}
}
}
- private static boolean isReadyForReview(RevisionResource revision, ReviewInput input) {
- return (!revision.getChange().isWorkInProgress() && !input.workInProgress) || input.ready;
+ private static boolean isReadyForReview(ChangeNotes changeNotes, ReviewInput input) {
+ return (!changeNotes.getChange().isWorkInProgress() && !input.workInProgress) || input.ready;
}
private void addToAttentionSet(
BatchUpdate bu,
- RevisionResource revision,
+ ChangeNotes changeNotes,
AttentionSetInput add,
- Set<Account.Id> accountsChangedInCommitv)
+ Set<Account.Id> accountsChangedInCommit)
throws BadRequestException, IOException, PermissionBackendException,
UnprocessableEntityException, ConfigInvalidException {
AttentionSetUtil.validateInput(add);
Account.Id attentionUserId =
- getAccountIdAndValidateUser(revision, add.user, accountsChangedInCommitv);
+ getAccountIdAndValidateUser(changeNotes, add.user, accountsChangedInCommit);
AddToAttentionSetOp addToAttentionSetOp =
addToAttentionSetOpFactory.create(attentionUserId, add.reason);
- bu.addOp(revision.getChange().getId(), addToAttentionSetOp);
+ bu.addOp(changeNotes.getChangeId(), addToAttentionSetOp);
}
private void removeFromAttentionSet(
BatchUpdate bu,
- RevisionResource revision,
+ ChangeNotes changeNotes,
AttentionSetInput remove,
Set<Account.Id> accountsChangedInCommit)
throws BadRequestException, IOException, PermissionBackendException,
UnprocessableEntityException, ConfigInvalidException {
AttentionSetUtil.validateInput(remove);
Account.Id attentionUserId =
- getAccountIdAndValidateUser(revision, remove.user, accountsChangedInCommit);
+ getAccountIdAndValidateUser(changeNotes, remove.user, accountsChangedInCommit);
RemoveFromAttentionSetOp removeFromAttentionSetOp =
removeFromAttentionSetOpFactory.create(attentionUserId, remove.reason);
- bu.addOp(revision.getChange().getId(), removeFromAttentionSetOp);
+ bu.addOp(changeNotes.getChangeId(), removeFromAttentionSetOp);
}
- private Account.Id getAccountIdAndValidateUser(
- RevisionResource revision, String user, Set<Account.Id> accountsChangedInCommit)
- throws ConfigInvalidException, IOException, PermissionBackendException,
- UnprocessableEntityException, BadRequestException {
+ private Account.Id getAccountId(ChangeNotes changeNotes, String user)
+ throws ConfigInvalidException, IOException, UnprocessableEntityException,
+ PermissionBackendException {
Account.Id attentionUserId = accountResolver.resolve(user).asUnique().account().id();
try {
permissionBackend
.absentUser(attentionUserId)
- .change(revision.getNotes())
+ .change(changeNotes)
.check(ChangePermission.READ);
} catch (AuthException e) {
throw new UnprocessableEntityException(
"Can't add to attention set: Read not permitted for " + attentionUserId, e);
}
+ return attentionUserId;
+ }
+
+ private Account.Id getAccountIdAndValidateUser(
+ ChangeNotes changeNotes, String user, Set<Account.Id> accountsChangedInCommit)
+ throws ConfigInvalidException, IOException, PermissionBackendException,
+ UnprocessableEntityException, BadRequestException {
+ Account.Id attentionUserId = getAccountId(changeNotes, user);
if (accountsChangedInCommit.contains(attentionUserId)) {
throw new BadRequestException(
String.format(
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index 0470af8..ddebd77 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -886,7 +886,7 @@
}
@Test
- public void attentionSetUnchangedWithIgnoreDefaultAttentionSetRules() throws Exception {
+ public void attentionSetUnchangedWithIgnoreAutomaticAttentionSetRules() throws Exception {
PushOneCommit.Result r = createChange();
change(r).addToAttentionSet(new AttentionSetInput(admin.email(), "reason"));
change(r)
@@ -894,7 +894,7 @@
.review(
ReviewInput.create()
.reviewer(admin.email(), ReviewerState.CC, false)
- .blockDefaultAttentionSetRules());
+ .blockAutomaticAttentionSetRules());
// admin is still in the attention set, although replies remove from attention set, and removing
// from reviewer also should remove from attention set.
@@ -905,7 +905,7 @@
}
@Test
- public void attentionSetStillChangesWithIgnoreDefaultAttentionSetRulesWithInputList()
+ public void attentionSetStillChangesWithIgnoreAutomaticAttentionSetRulesWithInputList()
throws Exception {
PushOneCommit.Result r = createChange();
change(r).addToAttentionSet(new AttentionSetInput(admin.email(), "reason"));
@@ -914,7 +914,7 @@
.review(
ReviewInput.create()
.removeUserFromAttentionSet(admin.email(), "removed")
- .blockDefaultAttentionSetRules());
+ .blockAutomaticAttentionSetRules());
// Admin is still removed although we block default attention set rules, since we remove
// the admin manually.
diff --git a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
index d68cada..075997b 100644
--- a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
@@ -22,11 +22,15 @@
import static org.mockito.MockitoAnnotations.initMocks;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.PushOneCommit.Result;
import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.extensions.api.changes.AttentionSetInput;
import com.google.gerrit.extensions.api.changes.DraftInput;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.config.FactoryModule;
@@ -101,6 +105,53 @@
}
@Test
+ public void attentionSetUpdatedReviewerAdded() throws Exception {
+ PushOneCommit.Result result = createChange();
+ String changeId = result.getChangeId();
+ gApi.changes().id(changeId).addReviewer(user.email());
+ gApi.changes().id(changeId).attention(user.email()).remove(new AttentionSetInput("removed"));
+ String revId = result.getCommit().getName();
+ DraftInput comment = testCommentHelper.newDraft(COMMENT_TEXT);
+ testCommentHelper.addDraft(changeId, revId, comment);
+ Result amendResult = amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo);
+ AttentionSetUpdate attentionSetUpdate =
+ Iterables.getOnlyElement(amendResult.getChange().attentionSet());
+ assertThat(attentionSetUpdate.account()).isEqualTo(user.id());
+ assertThat(attentionSetUpdate.reason()).isEqualTo("owner or uploader replied");
+ assertThat(attentionSetUpdate.operation()).isEqualTo(AttentionSetUpdate.Operation.ADD);
+ }
+
+ @Test
+ public void attentionSetUpdatedReviewerNotAddedWhenRemoved() throws Exception {
+ PushOneCommit.Result result = createChange();
+ String changeId = result.getChangeId();
+ gApi.changes().id(changeId).addReviewer(user.email());
+ gApi.changes().id(changeId).attention(user.email()).remove(new AttentionSetInput("removed"));
+ String revId = result.getCommit().getName();
+ DraftInput comment = testCommentHelper.newDraft(COMMENT_TEXT);
+ testCommentHelper.addDraft(changeId, revId, comment);
+ Result amendResult =
+ amendChange(
+ changeId, "refs/for/master%publish-comments,cc=" + user.email(), admin, testRepo);
+ AttentionSetUpdate attentionSetUpdate =
+ Iterables.getOnlyElement(amendResult.getChange().attentionSet());
+ assertThat(attentionSetUpdate.account()).isEqualTo(user.id());
+ assertThat(attentionSetUpdate.reason()).isEqualTo("removed");
+ assertThat(attentionSetUpdate.operation()).isEqualTo(AttentionSetUpdate.Operation.REMOVE);
+ }
+
+ @Test
+ public void attentionSetNotUpdatedWhenNoCommentsPublished() throws Exception {
+ PushOneCommit.Result result = createChange();
+ String changeId = result.getChangeId();
+ gApi.changes().id(changeId).addReviewer(user.email());
+ gApi.changes().id(changeId).attention(user.email()).remove(new AttentionSetInput("removed"));
+ ImmutableSet<AttentionSetUpdate> attentionSet = result.getChange().attentionSet();
+ Result amendResult = amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo);
+ assertThat(attentionSet).isEqualTo(amendResult.getChange().attentionSet());
+ }
+
+ @Test
public void validateComments_commentRejected() throws Exception {
PushOneCommit.Result result = createChange();
String changeId = result.getChangeId();
