From 302b59c4915b7f3b6b03d6116cd4d99b55b50bc2 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Mon, 27 Feb 2017 14:22:35 +0100 Subject: [PATCH] Add change owner as reviewer if labels were applied during push If the owner is not added as a reviewer, deleting their label will not be possible. Change-Id: I1b8b5312b34a0bf9295b4bdacda7b97bce7d0708 --- .../acceptance/git/AbstractPushForReview.java | 44 +++++++++++++++++++ .../gerrit/server/change/ChangeInserter.java | 7 +++ .../google/gerrit/server/git/ReplaceOp.java | 9 ++++ 3 files changed, 60 insertions(+) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index a1bd747e3b..31ca9dfb85 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -44,6 +44,7 @@ import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.client.InheritableBoolean; import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.extensions.client.ProjectWatchInfo; +import com.google.gerrit.extensions.client.ReviewerState; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.ChangeMessageInfo; import com.google.gerrit.extensions.common.EditInfo; @@ -505,6 +506,8 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { cr = ci.labels.get("Code-Review"); assertThat(Iterables.getLast(ci.messages).message) .isEqualTo("Uploaded patch set 2: Code-Review+2."); + // Check that the user who pushed the change was added as a reviewer since they added a vote + assertThatUserIsOnlyReviewer(ci, admin); assertThat(cr.all).hasSize(1); assertThat(cr.all.get(0).name).isEqualTo("Administrator"); @@ -524,6 +527,36 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { assertThat(Iterables.getLast(ci.messages).message).isEqualTo("Uploaded patch set 3."); } + @Test + public void pushNewPatchSetForMasterWithApprovals() throws Exception { + PushOneCommit.Result r = pushTo("refs/for/master"); + r.assertOkStatus(); + + PushOneCommit push = + pushFactory.create( + db, + admin.getIdent(), + testRepo, + PushOneCommit.SUBJECT, + "b.txt", + "anotherContent", + r.getChangeId()); + r = push.to("refs/for/master/%l=Code-Review+2"); + + ChangeInfo ci = get(r.getChangeId()); + LabelInfo cr = ci.labels.get("Code-Review"); + assertThat(Iterables.getLast(ci.messages).message) + .isEqualTo("Uploaded patch set 2: Code-Review+2."); + + // Check that the user who pushed the new patch set was added as a reviewer since they added + // a vote + assertThatUserIsOnlyReviewer(ci, admin); + + assertThat(cr.all).hasSize(1); + assertThat(cr.all.get(0).name).isEqualTo("Administrator"); + assertThat(cr.all.get(0).value).isEqualTo(2); + } + /** * There was a bug that allowed a user with Forge Committer Identity access right to upload a * commit and put *votes on behalf of another user* on it. This test checks that this is not @@ -566,6 +599,8 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { assertThat(cr.all.get(indexUser).value.intValue()).isEqualTo(0); assertThat(Iterables.getLast(ci.messages).message) .isEqualTo("Uploaded patch set 1: Code-Review+1."); + // Check that the user who pushed the change was added as a reviewer since they added a vote + assertThatUserIsOnlyReviewer(ci, admin); } @Test @@ -594,6 +629,8 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { assertThat(cr.all).hasSize(1); cr = ci.labels.get("Custom-Label"); assertThat(cr.all).hasSize(1); + // Check that the user who pushed the change was added as a reviewer since they added a vote + assertThatUserIsOnlyReviewer(ci, admin); } @Test @@ -1366,6 +1403,13 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { assertThat(info.status).isEqualTo(ChangeStatus.NEW); } + private void assertThatUserIsOnlyReviewer(ChangeInfo ci, TestAccount reviewer) { + assertThat(ci.reviewers).isNotNull(); + assertThat(ci.reviewers.keySet()).containsExactly(ReviewerState.REVIEWER); + assertThat(ci.reviewers.get(ReviewerState.REVIEWER).iterator().next().email) + .isEqualTo(reviewer.email); + } + private void pushWithReviewerInFooter(String nameEmail, TestAccount expectedReviewer) throws Exception { int n = 5; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java index 9983928320..e9293821c2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.change; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.gerrit.reviewdb.client.Change.INITIAL_PATCH_SET_ID; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import static java.util.stream.Collectors.toSet; import com.google.common.base.MoreObjects; @@ -390,6 +391,12 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp { Collections.emptySet()); approvalsUtil.addApprovalsForNewPatchSet( db, update, labelTypes, patchSet, ctx.getControl(), approvals); + // Check if approvals are changing in with this update. If so, add current user to reviewers. + // Note that this is done separately as addReviewers is filtering out the change owner as + // reviewer which is needed in several other code paths. + if (!approvals.isEmpty()) { + update.putReviewer(ctx.getAccountId(), REVIEWER); + } if (message != null) { changeMessage = ChangeMessagesUtil.newMessage( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java index 3cd3c1edcc..a3bd97cf0c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.git; import static com.google.gerrit.common.FooterConstants.CHANGE_ID; import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters; import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromReviewers; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; @@ -280,6 +281,14 @@ public class ReplaceOp extends BatchUpdate.Op { info, recipients.getReviewers(), oldRecipients.getAll()); + + // Check if approvals are changing in with this update. If so, add current user to reviewers. + // Note that this is done separately as addReviewers is filtering out the change owner as + // reviewer which is needed in several other code paths. + if (magicBranch != null && !magicBranch.labels.isEmpty()) { + update.putReviewer(ctx.getAccountId(), REVIEWER); + } + recipients.add(oldRecipients); String approvalMessage =