Ensure that 'Verified' label is handled by default Add the integration tests that check handling of the 'Verified' label. Assumptions: * having 'CR+2' casted by the file owner is not sufficient if 'V+1' is missing * having 'V+1' casted by the file owner is not sufficient if 'CR+2' is missing Note: In the setup where more than one label is votable (e.g. where 'Code Review' and 'Verified' are defined) one needs to configure the owners plugin which label is binding. This change introduce a method that will be responsible for getting it from the configuration but for the time being it is stubbed to always return 'Code Review' (or nothing in case the 'Code Review' is not present in the setup). Bug: Issue 15556 Change-Id: I85b989e74f7837365f3c26266bbcfac38e1d5381
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java index 3a7334b..2fe4952 100644 --- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java +++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java
@@ -25,6 +25,7 @@ import com.google.gerrit.entities.Account.Id; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.LabelFunction; +import com.google.gerrit.entities.LabelId; import com.google.gerrit.entities.LabelType; import com.google.gerrit.entities.LabelTypes; import com.google.gerrit.entities.LegacySubmitRequirement; @@ -140,7 +141,7 @@ ChangeNotes notes = cd.notes(); requireNonNull(notes, "notes"); - LabelTypes labelTypes = projectState.getLabelTypes(notes); + LabelTypes labelTypes = ownersLabel(projectState.getLabelTypes(notes)); Account.Id uploader = notes.getCurrentPatchSet().uploader(); Map<Account.Id, List<PatchSetApproval>> approvalsByAccount = Streams.stream(approvalsUtil.byPatchSet(notes, cd.currentPatchSet().id())) @@ -171,6 +172,19 @@ } } + /** the idea is to select the label type that is configured for owner to cast the vote */ + private LabelTypes ownersLabel(LabelTypes labelTypes) { + + // TODO: there is no specific label configuration introduced to the OWNERS file therefore for + // the time being set it to Code Review explicitly or nothing if it doesn't exist for the + // project in question + return new LabelTypes( + labelTypes + .byLabel(LabelId.CODE_REVIEW) + .map(Collections::singletonList) + .orElseGet(Collections::emptyList)); + } + static boolean isApprovalMissing( Map.Entry<String, Set<Account.Id>> requiredApproval, Account.Id uploader,
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java index 9c507be..b611cde 100644 --- a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java +++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java
@@ -16,6 +16,11 @@ package com.googlesource.gerrit.owners; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth8.assertThat; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel; +import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; +import static com.google.gerrit.server.project.testing.TestLabels.labelBuilder; +import static com.google.gerrit.server.project.testing.TestLabels.value; import static java.util.stream.Collectors.joining; import com.google.gerrit.acceptance.LightweightPluginDaemonTest; @@ -24,14 +29,26 @@ import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.UseLocalDisk; import com.google.gerrit.acceptance.config.GlobalPluginConfig; +import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; +import com.google.gerrit.entities.LabelFunction; +import com.google.gerrit.entities.LabelId; +import com.google.gerrit.entities.LabelType; +import com.google.gerrit.entities.RefNames; import com.google.gerrit.extensions.api.changes.ChangeApi; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.LegacySubmitRequirementInfo; +import com.google.gerrit.extensions.common.SubmitRecordInfo; import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.server.git.meta.MetaDataUpdate; +import com.google.gerrit.server.project.ProjectConfig; import com.google.inject.Inject; +import java.io.IOException; +import java.util.Collection; import java.util.stream.Stream; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.junit.Test; @TestPlugin(name = "owners", sysModule = "com.googlesource.gerrit.owners.OwnersModule") @@ -43,6 +60,7 @@ new LegacySubmitRequirementInfo("OK", "Owners", "owners"); @Inject private RequestScopeOperations requestScopeOperations; + @Inject private ProjectOperations projectOperations; @Test @GlobalPluginConfig( @@ -145,6 +163,98 @@ assertThat(forChange(r).get().submittable).isFalse(); } + @Test + @GlobalPluginConfig( + pluginName = "owners", + name = "owners.enableSubmitRequirement", + value = "true") + public void shouldRequireVerifiedApprovalEvenIfCodeOwnerApproved() throws Exception { + TestAccount admin2 = accountCreator.admin2(); + addOwnerFileToRoot(true, admin2); + + installVerifiedLabel(); + + PushOneCommit.Result r = createChange("Add a file", "foo", "bar"); + ChangeApi changeApi = forChange(r); + assertThat(changeApi.get().submittable).isFalse(); + assertThat(changeApi.get().requirements).containsExactly(NOT_READY); + + requestScopeOperations.setApiUser(admin2.id()); + forChange(r).current().review(ReviewInput.approve()); + assertThat(forChange(r).get().submittable).isFalse(); + assertThat(forChange(r).get().requirements).containsExactly(READY); + verifyHasSubmitRecord( + forChange(r).get().submitRecords, LabelId.VERIFIED, SubmitRecordInfo.Label.Status.NEED); + + changeApi.current().review(new ReviewInput().label(LabelId.VERIFIED, 1)); + assertThat(changeApi.get().submittable).isTrue(); + verifyHasSubmitRecord( + changeApi.get().submitRecords, LabelId.VERIFIED, SubmitRecordInfo.Label.Status.OK); + } + + @Test + @GlobalPluginConfig( + pluginName = "owners", + name = "owners.enableSubmitRequirement", + value = "true") + public void shouldRequireCodeOwnerApprovalEvenIfVerifiedWasApproved() throws Exception { + TestAccount admin2 = accountCreator.admin2(); + addOwnerFileToRoot(true, admin2); + + installVerifiedLabel(); + + PushOneCommit.Result r = createChange("Add a file", "foo", "bar"); + ChangeApi changeApi = forChange(r); + assertThat(changeApi.get().submittable).isFalse(); + assertThat(changeApi.get().requirements).containsExactly(NOT_READY); + + requestScopeOperations.setApiUser(admin2.id()); + forChange(r).current().review(new ReviewInput().label(LabelId.VERIFIED, 1)); + ChangeInfo changeNotReady = forChange(r).get(); + assertThat(changeNotReady.submittable).isFalse(); + assertThat(changeNotReady.requirements).containsExactly(NOT_READY); + verifyHasSubmitRecord( + changeNotReady.submitRecords, LabelId.VERIFIED, SubmitRecordInfo.Label.Status.OK); + + forChange(r).current().review(ReviewInput.approve()); + ChangeInfo changeReady = forChange(r).get(); + assertThat(changeReady.submittable).isTrue(); + assertThat(changeReady.requirements).containsExactly(READY); + } + + private void verifyHasSubmitRecord( + Collection<SubmitRecordInfo> records, String label, SubmitRecordInfo.Label.Status status) { + assertThat( + records.stream() + .flatMap(record -> record.labels.stream()) + .filter(l -> l.label.equals(label) && l.status == status) + .findAny()) + .isPresent(); + } + + private void installVerifiedLabel() + throws IOException, ConfigInvalidException, RepositoryNotFoundException { + LabelType verified = + labelBuilder( + LabelId.VERIFIED, value(1, "Verified"), value(0, "No score"), value(-1, "Fails")) + .setFunction(LabelFunction.MAX_WITH_BLOCK) + .build(); + try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) { + ProjectConfig cfg = projectConfigFactory.create(project); + cfg.load(md); + cfg.upsertLabelType(verified); + cfg.commit(md); + } + projectCache.evictAndReindex(project); + + String heads = RefNames.REFS_HEADS + "*"; + projectOperations + .project(project) + .forUpdate() + .add(allowLabel(verified.getName()).ref(heads).group(REGISTERED_USERS).range(-1, 1)) + .update(); + } + private ChangeApi forChange(PushOneCommit.Result r) throws RestApiException { return gApi.changes().id(r.getChangeId()); }