Block submitting changes with unresolved comments for new sites by default
When initialising a new Gerrit site add a submit requirement to the
All-Projects project that blocks change submission if there are
unresolved comments.
User Stories:
* As a reviewer I want that changes can only be submitted when all my
comments have been resolved.
* As a change owner I want that submitting my change is blocked until I
have resolved all comments, so that I do not accidentally leave some
comments unaddressed.
For sites where this behaviour is not wanted, the site administrators
can simply remove the submit requirement.
Release-Notes: Submitting changes with unresolved comments is blocked for new sites by default
Bug: Issue 317351858
Change-Id: I1e5688701080d2ce7fe44970cfbf1215e285e80f
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/server/schema/AllProjectsCreator.java b/java/com/google/gerrit/server/schema/AllProjectsCreator.java
index 314faf2..f00d205 100644
--- a/java/com/google/gerrit/server/schema/AllProjectsCreator.java
+++ b/java/com/google/gerrit/server/schema/AllProjectsCreator.java
@@ -34,6 +34,8 @@
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.PermissionRule.Action;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.entities.SubmitRequirement;
+import com.google.gerrit.entities.SubmitRequirementExpression;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.Sequence;
import com.google.gerrit.server.config.AllProjectsName;
@@ -45,6 +47,7 @@
import com.google.gerrit.server.update.context.RefUpdateContext;
import com.google.inject.Inject;
import java.io.IOException;
+import java.util.Optional;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.BatchRefUpdate;
@@ -132,11 +135,16 @@
// init labels.
input.codeReviewLabel().ifPresent(codeReviewLabel -> config.upsertLabelType(codeReviewLabel));
+ // init access sections.
if (input.initDefaultAcls()) {
- // init access sections.
initDefaultAcls(config, input);
}
+ // init submit requirement sections.
+ if (input.initDefaultSubmitRequirements()) {
+ initDefaultSubmitRequirements(config);
+ }
+
// commit all the above configs as a commit in "refs/meta/config" branch of the All-Projects.
config.commitToNewRef(md, RefNames.REFS_CONFIG);
@@ -176,6 +184,18 @@
.ifPresent(adminsGroup -> initDefaultAclsForAdmins(config, codeReviewLabel, adminsGroup));
}
+ private void initDefaultSubmitRequirements(ProjectConfig config) {
+ config.upsertSubmitRequirement(
+ SubmitRequirement.builder()
+ .setName("No-Unresolved-Comments")
+ .setDescription(
+ Optional.of("Changes that have unresolved comments are not submittable."))
+ .setApplicabilityExpression(SubmitRequirementExpression.of("has:unresolved"))
+ .setSubmittabilityExpression(SubmitRequirementExpression.create("-has:unresolved"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+ }
+
private void initDefaultAclsForRegisteredUsers(
AccessSection.Builder heads, LabelType codeReviewLabel, ProjectConfig config) {
config.upsertAccessSection(
diff --git a/java/com/google/gerrit/server/schema/AllProjectsInput.java b/java/com/google/gerrit/server/schema/AllProjectsInput.java
index 4f1dd3a..f692691 100644
--- a/java/com/google/gerrit/server/schema/AllProjectsInput.java
+++ b/java/com/google/gerrit/server/schema/AllProjectsInput.java
@@ -92,6 +92,9 @@
/** Whether initializing default access sections in All-Projects. */
public abstract boolean initDefaultAcls();
+ /** Whether default submit requirements should be initialized in All-Projects. */
+ public abstract boolean initDefaultSubmitRequirements();
+
public abstract Builder toBuilder();
public static Builder builder() {
@@ -99,7 +102,8 @@
new AutoValue_AllProjectsInput.Builder()
.codeReviewLabel(getDefaultCodeReviewLabel())
.firstChangeIdForNoteDb(Sequences.FIRST_CHANGE_ID)
- .initDefaultAcls(true);
+ .initDefaultAcls(true)
+ .initDefaultSubmitRequirements(true);
DEFAULT_BOOLEAN_PROJECT_CONFIGS.forEach(builder::addBooleanProjectConfig);
return builder;
@@ -140,6 +144,8 @@
@UsedAt(UsedAt.Project.GOOGLE)
public abstract Builder initDefaultAcls(boolean initDefaultACLs);
+ public abstract Builder initDefaultSubmitRequirements(boolean initDefaultSubmitRequirements);
+
public abstract AllProjectsInput build();
}
}
diff --git a/java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java b/java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java
index 9d2c9fe..3c26d3f 100644
--- a/java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java
+++ b/java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java
@@ -109,6 +109,13 @@
" value = 0 No score",
" value = +1 Looks good to me, but someone else must approve",
" value = +2 Looks good to me, approved");
+ private static final ImmutableList<String> DEFAULT_ALL_PROJECTS_SUBMIT_REQUIREMENT_SECTION =
+ ImmutableList.of(
+ "[submit-requirement \"No-Unresolved-Comments\"]",
+ " description = Changes that have unresolved comments are not submittable.",
+ " applicableIf = has:unresolved",
+ " submittableIf = -has:unresolved",
+ " canOverrideInChildProjects = false");
public static String getDefaultAllProjectsWithAllDefaultSections() {
return Streams.stream(
@@ -118,7 +125,8 @@
DEFAULT_ALL_PROJECTS_SUBMIT_SECTION,
DEFAULT_ALL_PROJECTS_CAPABILITY_SECTION,
DEFAULT_ALL_PROJECTS_ACCESS_SECTION,
- DEFAULT_ALL_PROJECTS_LABEL_SECTION))
+ DEFAULT_ALL_PROJECTS_LABEL_SECTION,
+ DEFAULT_ALL_PROJECTS_SUBMIT_REQUIREMENT_SECTION))
.collect(Collectors.joining("\n"));
}
@@ -128,6 +136,19 @@
DEFAULT_ALL_PROJECTS_PROJECT_SECTION,
DEFAULT_ALL_PROJECTS_RECEIVE_SECTION,
DEFAULT_ALL_PROJECTS_SUBMIT_SECTION,
+ DEFAULT_ALL_PROJECTS_LABEL_SECTION,
+ DEFAULT_ALL_PROJECTS_SUBMIT_REQUIREMENT_SECTION))
+ .collect(Collectors.joining("\n"));
+ }
+
+ public static String getAllProjectsWithoutDefaultSubmitRequirements() {
+ return Streams.stream(
+ Iterables.concat(
+ DEFAULT_ALL_PROJECTS_PROJECT_SECTION,
+ DEFAULT_ALL_PROJECTS_RECEIVE_SECTION,
+ DEFAULT_ALL_PROJECTS_SUBMIT_SECTION,
+ DEFAULT_ALL_PROJECTS_CAPABILITY_SECTION,
+ DEFAULT_ALL_PROJECTS_ACCESS_SECTION,
DEFAULT_ALL_PROJECTS_LABEL_SECTION))
.collect(Collectors.joining("\n"));
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/DefaultSubmitRequirementsIT.java b/javatests/com/google/gerrit/acceptance/api/change/DefaultSubmitRequirementsIT.java
new file mode 100644
index 0000000..fc8eaed
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/api/change/DefaultSubmitRequirementsIT.java
@@ -0,0 +1,79 @@
+package com.google.gerrit.acceptance.api.change;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
+import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import java.util.Comparator;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
+import org.junit.Test;
+
+public class DefaultSubmitRequirementsIT extends AbstractDaemonTest {
+ /**
+ * Tests the "No-Unresolved-Comments" submit requirement that is created during the site
+ * initialization.
+ */
+ @Test
+ public void cannotSubmitChangeWithUnresolvedComment() throws Exception {
+ TestRepository<InMemoryRepository> repo = cloneProject(project);
+ PushOneCommit.Result r =
+ createChange(repo, "master", "Add a file", "foo", "content", /* topic= */ null);
+ String changeId = r.getChangeId();
+ CommentInfo commentInfo =
+ addComment(changeId, "foo", "message", /* unresolved= */ true, /* inReplyTo= */ null);
+ assertThat(commentInfo.unresolved).isTrue();
+ approve(changeId);
+ ResourceConflictException exception =
+ assertThrows(
+ ResourceConflictException.class, () -> gApi.changes().id(changeId).current().submit());
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "Failed to submit 1 change due to the following problems:\n"
+ + "Change %s: submit requirement 'No-Unresolved-Comments' is unsatisfied.",
+ r.getChange().getId().get()));
+
+ // Resolve the comment and check that the change can be submitted now.
+ CommentInfo commentInfo2 =
+ addComment(
+ changeId, "foo", "reply", /* unresolved= */ false, /* inReplyTo= */ commentInfo.id);
+ assertThat(commentInfo2.unresolved).isFalse();
+ gApi.changes().id(changeId).current().submit();
+ }
+
+ @CanIgnoreReturnValue
+ private CommentInfo addComment(
+ String changeId, String file, String message, boolean unresolved, @Nullable String inReplyTo)
+ throws Exception {
+ ReviewInput in = new ReviewInput();
+ CommentInput commentInput = new CommentInput();
+ commentInput.path = file;
+ commentInput.line = 1;
+ commentInput.message = message;
+ commentInput.unresolved = unresolved;
+ commentInput.inReplyTo = inReplyTo;
+ in.comments = ImmutableMap.of(file, ImmutableList.of(commentInput));
+ gApi.changes().id(changeId).current().review(in);
+
+ return gApi.changes().id(changeId).commentsRequest().getAsList().stream()
+ .filter(commentInfo -> commentInput.message.equals(commentInfo.message))
+ // if there are multiple comments with the same message, take the one was created last
+ .max(
+ Comparator.comparing(commentInfo1 -> commentInfo1.updated.toInstant().getEpochSecond()))
+ .orElseThrow(
+ () ->
+ new IllegalStateException(
+ String.format("comment '%s' not found", commentInput.message)));
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
index 42af666..a5d86ce 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
@@ -92,6 +92,7 @@
import org.eclipse.jgit.revwalk.RevObject;
import org.eclipse.jgit.transport.RefSpec;
import org.eclipse.jgit.util.RawParseUtils;
+import org.junit.Before;
import org.junit.Test;
@NoHttpd
@@ -103,6 +104,11 @@
@Inject private ExtensionRegistry extensionRegistry;
@Inject private IndexOperations.Change changeIndexOperations;
+ @Before
+ public void setup() throws RestApiException {
+ removeDefaultSubmitRequirements();
+ }
+
@Test
public void submitRecords() throws Exception {
PushOneCommit.Result r = createChange();
@@ -3160,4 +3166,8 @@
r.assertOkStatus();
return r;
}
+
+ private void removeDefaultSubmitRequirements() throws RestApiException {
+ gApi.projects().name(allProjects.get()).submitRequirement("No-Unresolved-Comments").delete();
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/project/SubmitRequirementsAPIIT.java b/javatests/com/google/gerrit/acceptance/api/project/SubmitRequirementsAPIIT.java
index e388dd1..6d4da66 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/SubmitRequirementsAPIIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/SubmitRequirementsAPIIT.java
@@ -582,7 +582,7 @@
infos = gApi.projects().name(project.get()).submitRequirements().withInherited(true).get();
- assertThat(names(infos)).containsExactly("base-sr", "sr-1", "sr-2");
+ assertThat(names(infos)).containsExactly("No-Unresolved-Comments", "base-sr", "sr-1", "sr-2");
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/server/project/OnStoreSubmitRequirementResultModifierIT.java b/javatests/com/google/gerrit/acceptance/server/project/OnStoreSubmitRequirementResultModifierIT.java
index 50fa3b2..3041744 100644
--- a/javatests/com/google/gerrit/acceptance/server/project/OnStoreSubmitRequirementResultModifierIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/project/OnStoreSubmitRequirementResultModifierIT.java
@@ -33,6 +33,7 @@
import com.google.gerrit.extensions.common.SubmitRequirementResultInfo;
import com.google.gerrit.extensions.common.SubmitRequirementResultInfo.Status;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.change.TestSubmitInput;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.OnStoreSubmitRequirementResultModifier;
@@ -64,6 +65,7 @@
@Before
public void setUp() throws Exception {
+ removeDefaultSubmitRequirements();
TEST_ON_STORE_SUBMIT_REQUIREMENT_RESULT_MODIFIER.hide(false);
configSubmitRequirement(
project,
@@ -242,4 +244,8 @@
.count())
.isEqualTo(1);
}
+
+ private void removeDefaultSubmitRequirements() throws RestApiException {
+ gApi.projects().name(allProjects.get()).submitRequirement("No-Unresolved-Comments").delete();
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsEvaluatorIT.java b/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsEvaluatorIT.java
index 0c24b14..b5a3b66 100644
--- a/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsEvaluatorIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsEvaluatorIT.java
@@ -75,6 +75,7 @@
@Before
public void setUp() throws Exception {
+ removeDefaultSubmitRequirements();
PushOneCommit.Result pushResult =
createChange(testRepo, "refs/heads/master", "Fix a bug", "file.txt", "content", "topic");
changeData = pushResult.getChange();
@@ -976,6 +977,10 @@
.build();
}
+ private void removeDefaultSubmitRequirements() throws RestApiException {
+ gApi.projects().name(allProjects.get()).submitRequirement("No-Unresolved-Comments").delete();
+ }
+
/** Submit requirement predicate that always throws an error on match. */
static class ThrowingSubmitRequirementPredicate extends SubmitRequirementPredicate
implements ChangeIsOperandFactory {
diff --git a/javatests/com/google/gerrit/server/schema/AllProjectsCreatorTest.java b/javatests/com/google/gerrit/server/schema/AllProjectsCreatorTest.java
index f73a250..3fae3ad 100644
--- a/javatests/com/google/gerrit/server/schema/AllProjectsCreatorTest.java
+++ b/javatests/com/google/gerrit/server/schema/AllProjectsCreatorTest.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.server.schema.testing.AllProjectsCreatorTestUtil.assertSectionEquivalent;
import static com.google.gerrit.server.schema.testing.AllProjectsCreatorTestUtil.assertTwoConfigsEquivalent;
import static com.google.gerrit.server.schema.testing.AllProjectsCreatorTestUtil.getAllProjectsWithoutDefaultAcls;
+import static com.google.gerrit.server.schema.testing.AllProjectsCreatorTestUtil.getAllProjectsWithoutDefaultSubmitRequirements;
import static com.google.gerrit.server.schema.testing.AllProjectsCreatorTestUtil.getDefaultAllProjectsWithAllDefaultSections;
import static com.google.gerrit.server.schema.testing.AllProjectsCreatorTestUtil.readAllProjectsConfig;
import static com.google.gerrit.truth.ConfigSubject.assertThat;
@@ -141,6 +142,7 @@
.addBooleanProjectConfig(
BooleanProjectConfig.REJECT_EMPTY_COMMIT, InheritableBoolean.TRUE)
.initDefaultAcls(true)
+ .initDefaultSubmitRequirements(true)
.build();
allProjectsCreator.create(allProjectsInput);
@@ -160,6 +162,26 @@
}
@Test
+ public void createAllProjectsWithoutInitializingDefaultSubmitRequirements() throws Exception {
+ GroupReference adminsGroup = createGroupReference("Administrators");
+ GroupReference serviceUsersGroup = createGroupReference(ServiceUserClassifier.SERVICE_USERS);
+ GroupReference blockedUsersGroup = createGroupReference("Blocked Users");
+ AllProjectsInput allProjectsInput =
+ AllProjectsInput.builder()
+ .administratorsGroup(adminsGroup)
+ .serviceUsersGroup(serviceUsersGroup)
+ .blockedUsersGroup(blockedUsersGroup)
+ .initDefaultSubmitRequirements(false)
+ .build();
+ allProjectsCreator.create(allProjectsInput);
+
+ Config expectedConfig = new Config();
+ expectedConfig.fromText(getAllProjectsWithoutDefaultSubmitRequirements());
+ Config config = readAllProjectsConfig(repoManager, allProjectsName);
+ assertTwoConfigsEquivalent(config, expectedConfig);
+ }
+
+ @Test
public void createAllProjectsOnlyInitializingProjectDescription() throws Exception {
String description = "a project.config with just a project description";
AllProjectsInput allProjectsInput =
@@ -167,6 +189,7 @@
.firstChangeIdForNoteDb(Sequences.FIRST_CHANGE_ID)
.projectDescription(description)
.initDefaultAcls(false)
+ .initDefaultSubmitRequirements(false)
.build();
allProjectsCreator.create(allProjectsInput);
