Fix submit rule evaluation for non blocking labels
Putting a negative score on a label configured as `NoBlock` causes the submit button to be disabled. However, a `NoBlock` label is supposed to be purely informational and its values not considered when determining whether a change is submittable. Fix the submit rule evaluation to prevent the button from being disabled. Also add an integration test. Bug: Issue 2453 Change-Id: I0825b3c2d4774c17d523ac5f7890b983fa07af39
This commit is contained in:
parent
882f1aae78
commit
5a32a6dc15
@ -1,8 +1,8 @@
|
||||
include_defs('//gerrit-acceptance-tests/tests.defs')
|
||||
|
||||
acceptance_tests(
|
||||
srcs = ['ChangeMessagesIT.java', 'DeleteDraftChangeIT.java',
|
||||
'DeleteDraftPatchSetIT.java'],
|
||||
srcs = ['CustomLabelIT.java', 'ChangeMessagesIT.java',
|
||||
'DeleteDraftChangeIT.java', 'DeleteDraftPatchSetIT.java'],
|
||||
deps = [
|
||||
':util',
|
||||
'//gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git:util',
|
||||
@ -37,6 +37,7 @@ java_library(
|
||||
deps = [
|
||||
'//lib:guava',
|
||||
'//gerrit-reviewdb:server',
|
||||
'//gerrit-acceptance-tests:lib',
|
||||
],
|
||||
visibility = ['//gerrit-acceptance-tests/...'],
|
||||
)
|
||||
|
@ -15,8 +15,10 @@
|
||||
package com.google.gerrit.acceptance.rest.change;
|
||||
|
||||
import com.google.gerrit.reviewdb.client.Change;
|
||||
import com.google.gerrit.server.change.ChangeJson.LabelInfo;
|
||||
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
|
||||
public class ChangeInfo {
|
||||
String id;
|
||||
@ -25,4 +27,5 @@ public class ChangeInfo {
|
||||
List<ChangeMessageInfo> messages;
|
||||
Change.Status status;
|
||||
public Boolean starred;
|
||||
Map<String, LabelInfo> labels;
|
||||
}
|
||||
|
@ -0,0 +1,207 @@
|
||||
// Copyright (C) 2014 The Android Open Source Project
|
||||
//
|
||||
// Licensed under the Apache License, Version 2.0 (the "License");
|
||||
// you may not use this file except in compliance with the License.
|
||||
// You may obtain a copy of the License at
|
||||
//
|
||||
// http://www.apache.org/licenses/LICENSE-2.0
|
||||
//
|
||||
// Unless required by applicable law or agreed to in writing, software
|
||||
// distributed under the License is distributed on an "AS IS" BASIS,
|
||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
// See the License for the specific language governing permissions and
|
||||
// limitations under the License.
|
||||
|
||||
package com.google.gerrit.acceptance.rest.change;
|
||||
|
||||
import static com.google.gerrit.acceptance.git.GitUtil.cloneProject;
|
||||
import static com.google.gerrit.acceptance.git.GitUtil.createProject;
|
||||
import static com.google.gerrit.acceptance.git.GitUtil.initSsh;
|
||||
import static com.google.gerrit.server.project.Util.REGISTERED;
|
||||
import static com.google.gerrit.server.project.Util.category;
|
||||
import static com.google.gerrit.server.project.Util.grant;
|
||||
import static com.google.gerrit.server.project.Util.value;
|
||||
import static org.junit.Assert.assertEquals;
|
||||
import static org.junit.Assert.assertNotNull;
|
||||
import static org.junit.Assert.assertNull;
|
||||
|
||||
import com.google.common.collect.Maps;
|
||||
import com.google.gerrit.acceptance.AbstractDaemonTest;
|
||||
import com.google.gerrit.acceptance.AccountCreator;
|
||||
import com.google.gerrit.acceptance.RestResponse;
|
||||
import com.google.gerrit.acceptance.RestSession;
|
||||
import com.google.gerrit.acceptance.SshSession;
|
||||
import com.google.gerrit.acceptance.TestAccount;
|
||||
import com.google.gerrit.acceptance.git.PushOneCommit;
|
||||
import com.google.gerrit.common.changes.ListChangesOption;
|
||||
import com.google.gerrit.common.data.LabelType;
|
||||
import com.google.gerrit.common.data.Permission;
|
||||
import com.google.gerrit.reviewdb.client.Project;
|
||||
import com.google.gerrit.reviewdb.server.ReviewDb;
|
||||
import com.google.gerrit.server.OutputFormat;
|
||||
import com.google.gerrit.server.change.ChangeJson.LabelInfo;
|
||||
import com.google.gerrit.server.config.AllProjectsName;
|
||||
import com.google.gerrit.server.git.MetaDataUpdate;
|
||||
import com.google.gerrit.server.git.ProjectConfig;
|
||||
import com.google.gerrit.server.project.ProjectCache;
|
||||
import com.google.gwtorm.server.SchemaFactory;
|
||||
import com.google.inject.Inject;
|
||||
|
||||
import org.apache.http.HttpStatus;
|
||||
import org.eclipse.jgit.api.Git;
|
||||
import org.eclipse.jgit.api.errors.GitAPIException;
|
||||
import org.junit.After;
|
||||
import org.junit.Before;
|
||||
import org.junit.Test;
|
||||
|
||||
import java.io.IOException;
|
||||
|
||||
public class CustomLabelIT extends AbstractDaemonTest {
|
||||
|
||||
@Inject
|
||||
private ProjectCache projectCache;
|
||||
|
||||
@Inject
|
||||
private AllProjectsName allProjects;
|
||||
|
||||
@Inject
|
||||
private MetaDataUpdate.Server metaDataUpdateFactory;
|
||||
|
||||
@Inject
|
||||
private AccountCreator accounts;
|
||||
|
||||
@Inject
|
||||
private SchemaFactory<ReviewDb> reviewDbProvider;
|
||||
|
||||
private TestAccount admin;
|
||||
|
||||
private RestSession session;
|
||||
private Git git;
|
||||
private ReviewDb db;
|
||||
|
||||
private final LabelType Q = category("CustomLabel",
|
||||
value(1, "Positive"),
|
||||
value(0, "No score"),
|
||||
value(-1, "Negative"));
|
||||
|
||||
@Before
|
||||
public void setUp() throws Exception {
|
||||
admin = accounts.admin();
|
||||
session = new RestSession(server, admin);
|
||||
initSsh(admin);
|
||||
Project.NameKey project = new Project.NameKey("p");
|
||||
SshSession sshSession = new SshSession(server, admin);
|
||||
createProject(sshSession, project.get());
|
||||
git = cloneProject(sshSession.getUrl() + "/" + project.get());
|
||||
sshSession.close();
|
||||
db = reviewDbProvider.open();
|
||||
ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig();
|
||||
grant(cfg, Permission.forLabel(Q.getName()), -1, +1, REGISTERED,
|
||||
"refs/heads/*");
|
||||
saveProjectConfig(cfg);
|
||||
}
|
||||
|
||||
@After
|
||||
public void cleanup() {
|
||||
db.close();
|
||||
}
|
||||
|
||||
@Test
|
||||
public void customLabelNoOp_NegativeVoteNotBlock() throws Exception {
|
||||
Q.setFunctionName("NoOp");
|
||||
saveLabelConfig();
|
||||
ChangeInfo c = get(disliked(change()));
|
||||
LabelInfo q = c.labels.get(Q.getName());
|
||||
assertEquals(1, q.all.size());
|
||||
assertNull(q.rejected);
|
||||
assertNotNull(q.disliked);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void customLabelNoBlock_NegativeVoteNotBlock() throws Exception {
|
||||
Q.setFunctionName("NoBlock");
|
||||
saveLabelConfig();
|
||||
ChangeInfo c = get(disliked(change()));
|
||||
LabelInfo q = c.labels.get(Q.getName());
|
||||
assertEquals(1, q.all.size());
|
||||
assertNull(q.rejected);
|
||||
assertNotNull(q.disliked);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void customLabelMaxNoBlock_NegativeVoteNotBlock() throws Exception {
|
||||
Q.setFunctionName("MaxNoBlock");
|
||||
saveLabelConfig();
|
||||
ChangeInfo c = get(disliked(change()));
|
||||
LabelInfo q = c.labels.get(Q.getName());
|
||||
assertEquals(1, q.all.size());
|
||||
assertNull(q.rejected);
|
||||
assertNotNull(q.disliked);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void customLabelAnyWithBlock_NegativeVoteBlock() throws Exception {
|
||||
Q.setFunctionName("AnyWithBlock");
|
||||
saveLabelConfig();
|
||||
ChangeInfo c = get(disliked(change()));
|
||||
LabelInfo q = c.labels.get(Q.getName());
|
||||
assertEquals(1, q.all.size());
|
||||
assertNull(q.disliked);
|
||||
assertNotNull(q.rejected);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void customLabelMaxWithBlock_NegativeVoteBlock() throws Exception {
|
||||
saveLabelConfig();
|
||||
ChangeInfo c = get(disliked(change()));
|
||||
LabelInfo q = c.labels.get(Q.getName());
|
||||
assertEquals(1, q.all.size());
|
||||
assertNull(q.disliked);
|
||||
assertNotNull(q.rejected);
|
||||
}
|
||||
|
||||
private String disliked(String changeId) throws IOException {
|
||||
ReviewInput in = new ReviewInput();
|
||||
in.labels = Maps.newHashMap();
|
||||
in.labels.put(Q.getName(), -1);
|
||||
RestResponse r =
|
||||
session.post("/changes/" + changeId + "/revisions/current/review",
|
||||
in);
|
||||
assertEquals(HttpStatus.SC_OK, r.getStatusCode());
|
||||
r.consume();
|
||||
return changeId;
|
||||
}
|
||||
|
||||
private ChangeInfo get(String changeId) throws IOException {
|
||||
RestResponse r = session.get("/changes/"
|
||||
+ changeId
|
||||
+ "?o="
|
||||
+ ListChangesOption.DETAILED_LABELS
|
||||
+ "&o="
|
||||
+ ListChangesOption.LABELS);
|
||||
return OutputFormat.JSON.newGson()
|
||||
.fromJson(r.getReader(), ChangeInfo.class);
|
||||
}
|
||||
|
||||
private String change() throws GitAPIException,
|
||||
IOException {
|
||||
PushOneCommit push = new PushOneCommit(db, admin.getIdent());
|
||||
return push.to(git, "refs/for/master").getChangeId();
|
||||
}
|
||||
|
||||
private void saveLabelConfig() throws Exception {
|
||||
ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig();
|
||||
cfg.getLabelSections().put(Q.getName(), Q);
|
||||
saveProjectConfig(cfg);
|
||||
}
|
||||
|
||||
private void saveProjectConfig(ProjectConfig cfg) throws Exception {
|
||||
MetaDataUpdate md = metaDataUpdateFactory.create(allProjects);
|
||||
try {
|
||||
cfg.commit(md);
|
||||
} finally {
|
||||
md.close();
|
||||
}
|
||||
projectCache.evict(allProjects);
|
||||
}
|
||||
}
|
@ -454,21 +454,13 @@ public class ChangeJson {
|
||||
return;
|
||||
}
|
||||
|
||||
if (score != 0) {
|
||||
if (score == type.getMin().getValue()) {
|
||||
label.rejected = accountLoader.get(accountId);
|
||||
} else if (score == type.getMax().getValue()) {
|
||||
label.approved = accountLoader.get(accountId);
|
||||
} else if (score < 0) {
|
||||
label.disliked = accountLoader.get(accountId);
|
||||
label.value = score;
|
||||
} else if (score > 0 && label.disliked == null) {
|
||||
label.recommended = accountLoader.get(accountId);
|
||||
label.value = score;
|
||||
}
|
||||
if (score < 0) {
|
||||
label.disliked = accountLoader.get(accountId);
|
||||
label.value = score;
|
||||
} else if (score > 0 && label.disliked == null) {
|
||||
label.recommended = accountLoader.get(accountId);
|
||||
label.value = score;
|
||||
}
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
private void setAllApprovals(ChangeData cd,
|
||||
@ -984,19 +976,19 @@ public class ChangeJson {
|
||||
String message;
|
||||
}
|
||||
|
||||
static class LabelInfo {
|
||||
public static class LabelInfo {
|
||||
transient SubmitRecord.Label.Status _status;
|
||||
|
||||
AccountInfo approved;
|
||||
AccountInfo rejected;
|
||||
AccountInfo recommended;
|
||||
AccountInfo disliked;
|
||||
List<ApprovalInfo> all;
|
||||
public AccountInfo approved;
|
||||
public AccountInfo rejected;
|
||||
public AccountInfo recommended;
|
||||
public AccountInfo disliked;
|
||||
public List<ApprovalInfo> all;
|
||||
|
||||
Map<String, String> values;
|
||||
public Map<String, String> values;
|
||||
|
||||
Short value;
|
||||
Boolean optional;
|
||||
public Short value;
|
||||
public Boolean optional;
|
||||
|
||||
void addApproval(ApprovalInfo ai) {
|
||||
if (all == null) {
|
||||
|
Loading…
Reference in New Issue
Block a user