Replace Set<BlockingCondition> with "required" boolean
BlockingCondition was originally implemented as a checker field used to
describe conditions that block a submission. While this is useful for
Checkers, it is not useful for Checks since the frontend needs to
interpret them correctly.
This change creates an alternative field that directly indicates whether
this specific check is relevant for submit of a change. Also, this helps
the backend more accurately reflect the submit impact as considered within
the submit rule.
No storage changes are necessary and hense no mgiration is required.
Follow-up changes will have the "required" field entering another Info
entity for the purpose of having it easier to add more fields which are
in direct relation to whether a change can be submitted, by having all
those fields grouped under the same entity.
Another follow-up change will have documentation updates.
This change only adds an alternative to BlockingCondition without
removing support for it. Thus, it doesn't break the frontend.
However, the frontend should still adjust by using the new field. Once
the frontend is done, I can clean the backend by removing the legacy
BlockingCondition, as it is used only by the frontend.
Change-Id: I1790c62a6ee048fb7171b8a43ca95ca370dda210
diff --git a/java/com/google/gerrit/plugins/checks/CheckJson.java b/java/com/google/gerrit/plugins/checks/CheckJson.java
index 535fbc4..2f5b3a2 100644
--- a/java/com/google/gerrit/plugins/checks/CheckJson.java
+++ b/java/com/google/gerrit/plugins/checks/CheckJson.java
@@ -89,6 +89,7 @@
info.checkerName = checker.getName();
info.checkerStatus = checker.getStatus();
info.blocking = checker.getBlockingConditions();
+ info.required = checker.isRequired() ? true : null;
info.checkerDescription = checker.getDescription().orElse(null);
});
} catch (ConfigInvalidException e) {
diff --git a/java/com/google/gerrit/plugins/checks/api/CheckInfo.java b/java/com/google/gerrit/plugins/checks/api/CheckInfo.java
index d0d5c38..4fc22c5 100644
--- a/java/com/google/gerrit/plugins/checks/api/CheckInfo.java
+++ b/java/com/google/gerrit/plugins/checks/api/CheckInfo.java
@@ -56,6 +56,9 @@
/** Blocking conditions that apply to this check. */
public Set<BlockingCondition> blocking;
+ /** True if the check is required for submission, unset otherwise */
+ public Boolean required;
+
/** Description of the checker that produced this check */
public String checkerDescription;
@@ -79,6 +82,7 @@
&& Objects.equals(other.checkerName, checkerName)
&& Objects.equals(other.checkerStatus, checkerStatus)
&& Objects.equals(other.blocking, blocking)
+ && Objects.equals(other.required, required)
&& Objects.equals(other.checkerDescription, checkerDescription);
}
@@ -99,6 +103,7 @@
checkerName,
checkerStatus,
blocking,
+ required,
checkerDescription);
}
@@ -119,6 +124,7 @@
.add("checkerName", checkerName)
.add("checkerStatus", checkerStatus)
.add("blocking", blocking)
+ .add("required", required)
.add("checkerDescription", checkerDescription)
.toString();
}
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 9446f9d..c14424a 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckIT.java
@@ -95,6 +95,7 @@
expectedCheckInfo.checkerName = "My Checker";
expectedCheckInfo.checkerStatus = CheckerStatus.ENABLED;
expectedCheckInfo.blocking = ImmutableSortedSet.of();
+ expectedCheckInfo.required = null;
expectedCheckInfo.checkerDescription = "Description";
assertThat(getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER))
.isEqualTo(expectedCheckInfo);
@@ -113,6 +114,7 @@
expectedCheckInfo.checkerName = "My Checker";
expectedCheckInfo.checkerStatus = CheckerStatus.ENABLED;
expectedCheckInfo.blocking = ImmutableSortedSet.of();
+ expectedCheckInfo.required = null;
RestResponse r =
adminRestSession.get(
@@ -261,7 +263,7 @@
}
@Test
- public void getCheckReturnsBlockingConditionsOnlyForCheckerOption() throws Exception {
+ public void getCheckReturnsRequiredOnlyForCheckerOption() throws Exception {
CheckerUuid checkerUuid =
checkerOperations.newChecker().repository(project).required().create();
@@ -271,6 +273,8 @@
assertThat(getCheckInfo(patchSetId, checkerUuid).blocking).isNull();
assertThat(getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER).blocking)
.isNotEmpty();
+ assertThat(getCheckInfo(patchSetId, checkerUuid).required).isNull();
+ assertThat(getCheckInfo(patchSetId, checkerUuid, ListChecksOption.CHECKER).required).isTrue();
}
@Test
@@ -289,6 +293,7 @@
// Checker fields are not set.
assertThat(check.checkerName).isNull();
assertThat(check.blocking).isNull();
+ assertThat(check.required).isNull();
assertThat(check.checkerStatus).isNull();
// Check that at least some non-checker fields are set to ensure that we didn't get a completely
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
index f1f7e52..6ea438e 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListChecksIT.java
@@ -92,12 +92,14 @@
expectedCheckInfo1.repository = project.get();
expectedCheckInfo1.checkerName = checkerName1;
expectedCheckInfo1.blocking = ImmutableSet.of();
+ expectedCheckInfo1.required = null;
expectedCheckInfo1.checkerStatus = CheckerStatus.ENABLED;
CheckInfo expectedCheckInfo2 = checkOperations.check(checkKey2).asInfo();
expectedCheckInfo2.repository = project.get();
expectedCheckInfo2.checkerName = checkerName2;
expectedCheckInfo2.blocking = ImmutableSet.of();
+ expectedCheckInfo2.required = null;
expectedCheckInfo2.checkerStatus = CheckerStatus.ENABLED;
assertThat(checksApiFactory.revision(patchSetId).list(ListChecksOption.CHECKER))
@@ -124,12 +126,14 @@
expectedCheckInfo1.repository = project.get();
expectedCheckInfo1.checkerName = checkerName1;
expectedCheckInfo1.blocking = ImmutableSet.of();
+ expectedCheckInfo1.required = null;
expectedCheckInfo1.checkerStatus = CheckerStatus.ENABLED;
CheckInfo expectedCheckInfo2 = checkOperations.check(checkKey2).asInfo();
expectedCheckInfo2.repository = project.get();
expectedCheckInfo2.checkerName = checkerName2;
expectedCheckInfo2.blocking = ImmutableSet.of();
+ expectedCheckInfo2.required = null;
expectedCheckInfo2.checkerStatus = CheckerStatus.ENABLED;
RestResponse r =
@@ -174,6 +178,7 @@
CheckInfo check1 = maybeCheck1.get();
assertThat(check1.checkerName).isEqualTo(checkerName1);
assertThat(check1.blocking).isEmpty();
+ assertThat(check1.required).isNull();
assertThat(check1.checkerStatus).isEqualTo(CheckerStatus.ENABLED);
Optional<CheckInfo> maybeCheck2 =
@@ -182,6 +187,7 @@
CheckInfo check2 = maybeCheck2.get();
assertThat(check2.checkerName).isNull();
assertThat(check2.blocking).isNull();
+ assertThat(check2.required).isNull();
assertThat(check2.checkerStatus).isNull();
}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java b/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java
index b545f99..02b187d 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java
@@ -81,6 +81,7 @@
assertThat(foundCheck.checkerName).isNull();
assertThat(foundCheck.checkerStatus).isNull();
assertThat(foundCheck.blocking).isNull();
+ assertThat(foundCheck.required).isNull();
}
@Test
