opendev/gerrit
Code Issues Proposed changes

Fail when attempting to change labels on closed changes

Browse Source 
In 2.5 this was an error. In the transition to the PostReview
implementation the error started being ignored. Return it to an
error so that scripts and users don't have to query Gerrit to see
if their vote was successful.

Change-Id: Iac640e5daf9557dd275b345a50da852c0f68d838
This commit is contained in:
Nasser Grainawi
2015-11-07 21:47:45 -08:00
committed by Nasser Grainawi
parent 33422d0249
commit fae23d589c
2 changed files with 19 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java
View File

@@ -24,6 +24,7 @@ import com.google.gerrit.extensions.api.changes.CherryPickInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.project.Util;
@@ -45,6 +46,15 @@ public class LabelTypeIT extends AbstractDaemonTest {
saveProjectConfig(cfg);
}
@Test
public void failChangedLabelValueOnClosedChange() throws Exception {
PushOneCommit.Result r = createChange();
merge(r);
exception.expect(ResourceConflictException.class);
exception.expectMessage("change is closed");
revision(r).review(ReviewInput.reject());
}
@Test
public void noCopyMinScoreOnRework() throws Exception {
codeReview.setCopyMinScore(false);

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

@@ -349,7 +349,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
@Override
public void updateChange(ChangeContext ctx) throws OrmException {
public void updateChange(ChangeContext ctx)
throws OrmException, ResourceConflictException {
user = ctx.getUser().asIdentifiedUser();
change = ctx.getChange();
if (change.getLastUpdatedOn().before(ctx.getWhen())) {
@@ -500,7 +501,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
return drafts;
}
private boolean updateLabels(ChangeContext ctx) throws OrmException {
private boolean updateLabels(ChangeContext ctx)
throws OrmException, ResourceConflictException {
Map<String, Short> labels = in.labels;
if (labels == null) {
labels = Collections.emptyMap();
@@ -515,10 +517,6 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
for (Map.Entry<String, Short> ent : labels.entrySet()) {
String name = ent.getKey();
LabelType lt = checkNotNull(labelTypes.byLabel(name), name);
if (ctx.getChange().getStatus().isClosed()) {
// TODO Allow updating some labels even when closed.
continue;
}
PatchSetApproval c = current.remove(lt.getName());
String normName = lt.getName();
@@ -554,6 +552,11 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
if (!del.isEmpty() || !ups.isEmpty()) {
if (ctx.getChange().getStatus().isClosed()) {
throw new ResourceConflictException("change is closed");
}
}
forceCallerAsReviewer(ctx, current, ups, del);
ctx.getDb().patchSetApprovals().delete(del);
ctx.getDb().patchSetApprovals().upsert(ups);