Check "required" also depends on checker's status and whether it applies
When introducing the required field, this field shouldn't depend
only on the checker's blocking conditions. The required field should
accurately reflect whether or not this check is required for submission.
Change-Id: I7bb3ee31118bb23c7b4d66643c91b37df9accf17
diff --git a/java/com/google/gerrit/plugins/checks/CheckJson.java b/java/com/google/gerrit/plugins/checks/CheckJson.java
index 2f5b3a2..7fa17ca 100644
--- a/java/com/google/gerrit/plugins/checks/CheckJson.java
+++ b/java/com/google/gerrit/plugins/checks/CheckJson.java
@@ -16,6 +16,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.Change;
import com.google.gerrit.plugins.checks.api.CheckInfo;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -50,11 +51,13 @@
}
private final Checkers checkers;
+ private final Checks checks;
private final ImmutableSet<ListChecksOption> options;
@Inject
- CheckJson(Checkers checkers, @Assisted Iterable<ListChecksOption> options) {
+ CheckJson(Checkers checkers, Checks checks, @Assisted Iterable<ListChecksOption> options) {
this.checkers = checkers;
+ this.checks = checks;
this.options = ImmutableSet.copyOf(options);
}
@@ -75,12 +78,14 @@
info.updated = check.updated();
if (options.contains(ListChecksOption.CHECKER)) {
- populateCheckerFields(check.key().checkerUuid(), info);
+ populateCheckerFields(check.key().checkerUuid(), info, check.key().patchSet().changeId());
}
return info;
}
- private void populateCheckerFields(CheckerUuid checkerUuid, CheckInfo info) throws IOException {
+ private void populateCheckerFields(CheckerUuid checkerUuid, CheckInfo info, Change.Id changeId)
+ throws IOException {
+
try {
checkers
.getChecker(checkerUuid)
@@ -89,7 +94,7 @@
info.checkerName = checker.getName();
info.checkerStatus = checker.getStatus();
info.blocking = checker.getBlockingConditions();
- info.required = checker.isRequired() ? true : null;
+ info.required = checks.isRequiredForSubmit(checker, changeId) ? true : null;
info.checkerDescription = checker.getDescription().orElse(null);
});
} catch (ConfigInvalidException e) {
diff --git a/java/com/google/gerrit/plugins/checks/Checks.java b/java/com/google/gerrit/plugins/checks/Checks.java
index cf9d965..9ae249c 100644
--- a/java/com/google/gerrit/plugins/checks/Checks.java
+++ b/java/com/google/gerrit/plugins/checks/Checks.java
@@ -101,6 +101,15 @@
*/
String getETag(Project.NameKey projectName, Change.Id changeId) throws IOException;
+ /**
+ * Returns whether the checker is required for submission for this change.
+ *
+ * @param checker The checker that is being checked whether it's required for submission.
+ * @param changeId The changeId of the change which submission requirement is in question.
+ * @return True if the checker is required for submit, false otherwise.
+ */
+ boolean isRequiredForSubmit(Checker checker, Change.Id changeId);
+
@AutoValue
abstract class GetCheckOptions {
public static GetCheckOptions defaults() {
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
index af8cabc..44f3d70 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
@@ -167,7 +167,6 @@
private ImmutableListMultimap<CheckState, Boolean> getStatesAndRequiredMap(
Project.NameKey projectName, PatchSet.Id patchSetId) throws IOException, StorageException {
- ChangeData changeData = changeDataFactory.create(projectName, patchSetId.changeId());
ImmutableMap<String, Checker> allCheckersOfProject =
checkers.checkersOf(projectName).stream()
.collect(ImmutableMap.toImmutableMap(c -> c.getUuid().get(), c -> c));
@@ -192,16 +191,22 @@
continue;
}
- boolean isRequired =
- checker.getStatus() == CheckerStatus.ENABLED
- && checker.isRequired()
- && checkerQueryProvider.get().isCheckerRelevant(checker, changeData);
+ boolean isRequired = isRequiredForSubmit(checker, patchSetId.changeId());
statesAndRequired.put(check.state(), isRequired);
}
return statesAndRequired.build();
}
+ @Override
+ public boolean isRequiredForSubmit(Checker checker, Change.Id changeId) {
+ ChangeData changeData = changeDataFactory.create(checker.getRepository(), changeId);
+
+ return checker.getStatus() == CheckerStatus.ENABLED
+ && checker.isRequired()
+ && checkerQueryProvider.get().isCheckerRelevant(checker, changeData);
+ }
+
private ImmutableList<Checker> getCheckersForBackfiller(
Project.NameKey projectName, List<Check> existingChecks) throws IOException {
ImmutableSet<CheckerUuid> checkersWithExistingChecks =
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
index c14424a..638ce4c 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
@@ -278,6 +278,29 @@
}
@Test
+ public void notApplyingButRequiredCheckerIsNotRequiredForSubmission() throws Exception {
+ CheckerUuid checkerUuid =
+ checkerOperations
+ .newChecker()
+ .repository(project)
+ .query("status:merged")
+ .required()
+ .create();
+ CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
+ checkOperations.newCheck(checkKey).upsert();
+ assertThat(getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER).required).isNull();
+ }
+
+ @Test
+ public void disabledButRequiredCheckerIsNotRequiredForSubmission() throws Exception {
+ CheckerUuid checkerUuid =
+ checkerOperations.newChecker().repository(project).disable().required().create();
+ CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
+ checkOperations.newCheck(checkKey).upsert();
+ assertThat(getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER).required).isNull();
+ }
+
+ @Test
public void getCheckWithCheckerOptionReturnsCheckEvenIfCheckerIsInvalid() throws Exception {
CheckerUuid checkerUuid =
checkerOperations.newChecker().repository(project).name("My Checker").create();
