opendev/gerrit
Code Issues Proposed changes

Merge changes I7c20112e,I16502b6a,I108d6671

Browse Source 
* changes:
  Don't create approvals for non-voting reviewers
  Don't implicitly vote for the user in reply dialog
  Don't return implicit approvals for CCed reviewers
This commit is contained in:
Dave Borowitz 2016-08-29 15:22:16 +00:00 committed by Gerrit Code Review
commit cc15c2bba9
4 changed files with 205 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

163
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
View File

@ -32,7 +32,9 @@ import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewResult;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ApprovalInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.common.ReviewerUpdateInfo;
import com.google.gerrit.server.change.PostReviewers;
import com.google.gerrit.server.mail.Address;
@ -43,8 +45,10 @@ import org.junit.Test;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
public class ChangeReviewersIT extends AbstractDaemonTest {
@Test
@ -259,6 +263,165 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
assertReviewers(c, CC);
}
@Test
public void driveByComment() throws Exception {
// Create change owned by admin.
PushOneCommit.Result r = createChange();
// Post drive-by message as user.
ReviewInput input = new ReviewInput().message("hello");
RestResponse resp = userRestSession.post(
"/changes/" + r.getChangeId() + "/revisions/" +
r.getCommit().getName() + "/review", input);
ReviewResult result = readContentFromJson(resp, 200, ReviewResult.class);
assertThat(result.labels).isNull();
assertThat(result.reviewers).isNull();
// Verify user is not added as reviewer.
ChangeInfo c = gApi.changes()
.id(r.getChangeId())
.get();
assertReviewers(c, REVIEWER);
assertReviewers(c, CC);
}
@Test
public void addSelfAsReviewer() throws Exception {
// Create change owned by admin.
PushOneCommit.Result r = createChange();
// user adds self as REVIEWER.
ReviewInput input = new ReviewInput().reviewer(user.username);
RestResponse resp = userRestSession.post(
"/changes/" + r.getChangeId() + "/revisions/" +
r.getCommit().getName() + "/review", input);
ReviewResult result = readContentFromJson(resp, 200, ReviewResult.class);
assertThat(result.labels).isNull();
assertThat(result.reviewers).isNotNull();
assertThat(result.reviewers).hasSize(1);
// Verify reviewer state.
ChangeInfo c = gApi.changes()
.id(r.getChangeId())
.get();
assertReviewers(c, REVIEWER, user);
assertReviewers(c, CC);
LabelInfo label = c.labels.get("Code-Review");
assertThat(label).isNotNull();
assertThat(label.all).isNotNull();
assertThat(label.all).hasSize(1);
ApprovalInfo approval = label.all.get(0);
assertThat(approval._accountId).isEqualTo(user.getId().get());
}
@Test
public void addSelfAsCc() throws Exception {
// Create change owned by admin.
PushOneCommit.Result r = createChange();
// user adds self as CC.
ReviewInput input = new ReviewInput().reviewer(user.username, CC, false);
RestResponse resp = userRestSession.post(
"/changes/" + r.getChangeId() + "/revisions/" +
r.getCommit().getName() + "/review", input);
ReviewResult result = readContentFromJson(resp, 200, ReviewResult.class);
assertThat(result.labels).isNull();
assertThat(result.reviewers).isNotNull();
assertThat(result.reviewers).hasSize(1);
// Verify reviewer state.
ChangeInfo c = gApi.changes()
.id(r.getChangeId())
.get();
if (notesMigration.readChanges()) {
assertReviewers(c, REVIEWER);
assertReviewers(c, CC, user);
// Verify no approvals were added.
assertThat(c.labels).isNotNull();
LabelInfo label = c.labels.get("Code-Review");
assertThat(label).isNotNull();
assertThat(label.all).isNull();
} else {
// When approvals are stored in ReviewDb, we still create a label for
// the reviewing user, and force them into the REVIEWER state.
assertReviewers(c, REVIEWER, user);
assertReviewers(c, CC);
LabelInfo label = c.labels.get("Code-Review");
assertThat(label).isNotNull();
assertThat(label.all).isNotNull();
assertThat(label.all).hasSize(1);
ApprovalInfo approval = label.all.get(0);
assertThat(approval._accountId).isEqualTo(user.getId().get());
}
}
@Test
public void reviewerReplyWithoutVote() throws Exception {
// Create change owned by admin.
PushOneCommit.Result r = createChange();
// Verify reviewer state.
ChangeInfo c = gApi.changes()
.id(r.getChangeId())
.get();
assertReviewers(c, REVIEWER);
assertReviewers(c, CC);
LabelInfo label = c.labels.get("Code-Review");
assertThat(label).isNotNull();
assertThat(label.all).isNull();
// Add user as REVIEWER.
ReviewInput input = new ReviewInput().reviewer(user.username);
ReviewResult result = review(r.getChangeId(), r.getCommit().name(), input);
assertThat(result.labels).isNull();
assertThat(result.reviewers).isNotNull();
assertThat(result.reviewers).hasSize(1);
// Verify reviewer state. Both admin and user should be REVIEWERs now,
// because admin gets forced into REVIEWER state by virtue of being owner.
c = gApi.changes()
.id(r.getChangeId())
.get();
assertReviewers(c, REVIEWER, admin, user);
assertReviewers(c, CC);
label = c.labels.get("Code-Review");
assertThat(label).isNotNull();
assertThat(label.all).isNotNull();
assertThat(label.all).hasSize(2);
Map<Integer, Integer> approvals = new HashMap<>();
for (ApprovalInfo approval : label.all) {
approvals.put(approval._accountId, approval.value);
}
assertThat(approvals).containsEntry(admin.getId().get(), 0);
assertThat(approvals).containsEntry(user.getId().get(), 0);
// Comment as user without voting. This should delete the approval and
// then replace it with the default value.
input = new ReviewInput().message("hello");
RestResponse resp = userRestSession.post(
"/changes/" + r.getChangeId() + "/revisions/" +
r.getCommit().getName() + "/review", input);
result = readContentFromJson(resp, 200, ReviewResult.class);
assertThat(result.labels).isNull();
// Verify reviewer state.
c = gApi.changes()
.id(r.getChangeId())
.get();
assertReviewers(c, REVIEWER, admin, user);
assertReviewers(c, CC);
label = c.labels.get("Code-Review");
assertThat(label).isNotNull();
assertThat(label.all).isNotNull();
assertThat(label.all).hasSize(2);
approvals.clear();
for (ApprovalInfo approval : label.all) {
approvals.put(approval._accountId, approval.value);
}
assertThat(approvals).containsEntry(admin.getId().get(), 0);
assertThat(approvals).containsEntry(user.getId().get(), 0);
}
@Test
public void reviewAndAddReviewers() throws Exception {
TestAccount observer = accounts.user2();

2
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
View File

@ -663,7 +663,7 @@ public class ChangeJson {
// - They are an explicit reviewer.
// - They ever voted on this change.
Set<Account.Id> allUsers = new HashSet<>();
allUsers.addAll(cd.reviewers().all());
allUsers.addAll(cd.reviewers().byState(ReviewerStateInternal.REVIEWER));
for (PatchSetApproval psa : cd.approvals().values()) {
allUsers.add(psa.getAccountId());
}

38
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
View File

@ -67,6 +67,7 @@ import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchLineCommentsUtil;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.account.AccountsCollection;
import com.google.gerrit.server.extensions.events.CommentAdded;
import com.google.gerrit.server.git.BatchUpdate;
@ -212,7 +213,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
bu.addOp(
revision.getChange().getId(),
new Op(revision.getPatchSet().getId(), input));
new Op(revision.getPatchSet().getId(), input, reviewerResults));
bu.execute();
for (PostReviewers.Addition reviewerResult : reviewerResults) {
@ -387,6 +388,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
private class Op extends BatchUpdate.Op {
private final PatchSet.Id psId;
private final ReviewInput in;
private final List<PostReviewers.Addition> reviewerResults;
private IdentifiedUser user;
private ChangeNotes notes;
@ -397,9 +399,11 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
private Map<String, Short> approvals = new HashMap<>();
private Map<String, Short> oldApprovals = new HashMap<>();
private Op(PatchSet.Id psId, ReviewInput in) {
private Op(PatchSet.Id psId, ReviewInput in,
List<PostReviewers.Addition> reviewerResults) {
this.psId = psId;
this.in = in;
this.reviewerResults = reviewerResults;
}
@Override
@ -615,6 +619,28 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
return previous;
}
private boolean isReviewer(ChangeContext ctx) throws OrmException {
if (ctx.getAccountId().equals(ctx.getChange().getOwner())) {
return true;
}
for (PostReviewers.Addition addition : reviewerResults) {
if (addition.op.addedReviewers == null) {
continue;
}
for (PatchSetApproval psa : addition.op.addedReviewers) {
if (psa.getAccountId().equals(ctx.getAccountId())) {
return true;
}
}
}
ChangeData cd = changeDataFactory.create(db.get(), ctx.getControl());
ReviewerSet reviewers = cd.reviewers();
if (reviewers.byState(REVIEWER).contains(ctx.getAccountId())) {
return true;
}
return false;
}
private boolean updateLabels(ChangeContext ctx)
throws OrmException, ResourceConflictException {
Map<String, Short> inLabels = MoreObjects.firstNonNull(in.labels,
@ -689,6 +715,14 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
&& ctx.getChange().getStatus().isClosed()) {
throw new ResourceConflictException("change is closed");
}
// Return early if user is not a reviewer and not posting any labels.
// This allows us to preserve their CC status.
if (current.isEmpty() && del.isEmpty() && ups.isEmpty() &&
!isReviewer(ctx)) {
return false;
}
forceCallerAsReviewer(ctx, current, ups, del);
ctx.getDb().patchSetApprovals().delete(del);
ctx.getDb().patchSetApprovals().upsert(ups);

6
polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
View File

@ -151,6 +151,10 @@
if (!this.permittedLabels.hasOwnProperty(label)) { continue; }
var selectorEl = this.$$('iron-selector[data-label="' + label + '"]');
// The user may have not voted on this label.
if (!selectorEl.selectedItem) { continue; }
var selectedVal = selectorEl.selectedItem.getAttribute('data-value');
selectedVal = parseInt(selectedVal, 10);
obj.labels[label] = selectedVal;
@ -280,7 +284,7 @@
labels, permittedLabels, labelName, account) {
var t = labels[labelName];
if (!t) { return null; }
var labelValue = t.default_value;
var labelValue = null;
// Is there an existing vote for the current user? If so, use that.
var votes = labels[labelName];