Submit whole topic: disable submit button if not ready
The current implementation just disables the submit button if the current
user could not submit all changes in the topic. Additionally we want to
block the submit button if the changes do not pass the submit rule checks
individually. When the submission is disabled, show a tooltip explaining
why there is a problem.
This change is also still not what we want to have eventually:
We're having a cache invalidation problem, when other changes in
the same topic change and affect the submission of the whole topic here.
Change-Id: Ic242e738efa9b53cbf0dfd167e64b070c84dac29
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
index 6e9b3f2..cadd55c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
@@ -94,6 +94,10 @@
"Submit patch set ${patchSet} into ${branch}";
private static final String DEFAULT_TOPIC_TOOLTIP =
"Submit all ${topicSize} changes of the same topic";
+ private static final String BLOCKED_TOPIC_TOOLTIP =
+ "Other changes in this topic are not ready";
+ private static final String BLOCKED_HIDDEN_TOPIC_TOOLTIP =
+ "Other hidden changes in this topic are not ready";
public enum Status {
SUBMITTED, MERGED
@@ -233,22 +237,32 @@
}
}
- private boolean areChangesSubmittable(List<ChangeData> changes,
+ /**
+ * @param changes list of changes to be submitted at once
+ * @param identifiedUser the user who is checking to submit
+ * @return a reason why any of the changes is not submittable or null
+ */
+ private String problemsForSubmittingChanges(List<ChangeData> changes,
IdentifiedUser identifiedUser) {
for (ChangeData c : changes) {
try {
ChangeControl changeControl = c.changeControl().forUser(
identifiedUser);
- if (!changeControl.canSubmit()) {
- return false;
+ if (!changeControl.isVisible(dbProvider.get())) {
+ return BLOCKED_HIDDEN_TOPIC_TOOLTIP;
}
+ if (!changeControl.canSubmit()) {
+ return BLOCKED_TOPIC_TOOLTIP;
+ }
+ checkSubmitRule(c, c.currentPatchSet(), false);
} catch (OrmException e) {
- log.error("Failed to get a ChangeControl for Change.Id " +
- String.valueOf(c.getId()), e);
- return false;
+ log.error("Error checking if change is submittable", e);
+ throw new OrmRuntimeException(e);
+ } catch (ResourceConflictException e) {
+ return BLOCKED_TOPIC_TOOLTIP;
}
}
- return true;
+ return null;
}
@Override
@@ -274,17 +288,22 @@
}
Map<String, String> params = ImmutableMap.of(
"topicSize", String.valueOf(changesByTopic.size()));
- // TODO(sbeller):
- // If the button is visible but disabled the problem of submitting
- // is at another change in the same topic. Tell the user via
- // tooltip. Caution: Check access control for those changes.
- return new UiAction.Description()
- .setLabel(submitTopicLabel)
- .setTitle(Strings.emptyToNull(
- submitTopicTooltip.replace(params)))
- .setVisible(true)
- .setEnabled(areChangesSubmittable(
- changesByTopic, resource.getUser()));
+ String topicProblems = problemsForSubmittingChanges(changesByTopic,
+ resource.getUser());
+ if (topicProblems != null) {
+ return new UiAction.Description()
+ .setLabel(submitTopicLabel)
+ .setTitle(topicProblems)
+ .setVisible(true)
+ .setEnabled(false);
+ } else {
+ return new UiAction.Description()
+ .setLabel(submitTopicLabel)
+ .setTitle(Strings.emptyToNull(
+ submitTopicTooltip.replace(params)))
+ .setVisible(true)
+ .setEnabled(true);
+ }
} else {
RevId revId = resource.getPatchSet().getRevision();
Map<String, String> params = ImmutableMap.of(
@@ -336,13 +355,16 @@
private Change submitThisChange(RevisionResource rsrc, IdentifiedUser caller,
boolean force) throws ResourceConflictException, OrmException,
IOException {
- List<SubmitRecord> submitRecords = checkSubmitRule(rsrc, force);
+ ReviewDb db = dbProvider.get();
+ ChangeData cd = changeDataFactory.create(db, rsrc.getControl());
+ List<SubmitRecord> submitRecords = checkSubmitRule(cd,
+ rsrc.getPatchSet(), force);
+
final Timestamp timestamp = TimeUtil.nowTs();
Change change = rsrc.getChange();
ChangeUpdate update = updateFactory.create(rsrc.getControl(), timestamp);
update.submit(submitRecords);
- ReviewDb db = dbProvider.get();
db.changes().beginTransaction(change.getId());
try {
BatchMetaDataUpdate batch = approve(rsrc, update, caller, timestamp);
@@ -364,13 +386,18 @@
boolean force, String topic) throws ResourceConflictException, OrmException,
IOException {
Preconditions.checkNotNull(topic);
- List<SubmitRecord> submitRecords = checkSubmitRule(rsrc, force);
final Timestamp timestamp = TimeUtil.nowTs();
+
+ ReviewDb db = dbProvider.get();
+ ChangeData cd = changeDataFactory.create(db, rsrc.getControl());
+
+ List<SubmitRecord> submitRecords = checkSubmitRule(cd,
+ rsrc.getPatchSet(), force);
+
Change change = rsrc.getChange();
ChangeUpdate update = updateFactory.create(rsrc.getControl(), timestamp);
update.submit(submitRecords);
- ReviewDb db = dbProvider.get();
db.changes().beginTransaction(change.getId());
List<ChangeData> changesByTopic = queryProvider.get().byTopicOpen(topic);
@@ -498,12 +525,11 @@
}
}
- private List<SubmitRecord> checkSubmitRule(RevisionResource rsrc,
- boolean force) throws ResourceConflictException, OrmException {
- ChangeData cd =
- changeDataFactory.create(dbProvider.get(), rsrc.getControl());
+ private List<SubmitRecord> checkSubmitRule(ChangeData cd,
+ PatchSet patchSet, boolean force)
+ throws ResourceConflictException, OrmException {
List<SubmitRecord> results = new SubmitRuleEvaluator(cd)
- .setPatchSet(rsrc.getPatchSet())
+ .setPatchSet(patchSet)
.canSubmit();
Optional<SubmitRecord> ok = findOkRecord(results);
if (ok.isPresent()) {
@@ -514,8 +540,8 @@
} else if (results.isEmpty()) {
throw new IllegalStateException(String.format(
"ChangeControl.canSubmit returned empty list for %s in %s",
- rsrc.getPatchSet().getId(),
- rsrc.getChange().getProject().get()));
+ patchSet.getId(),
+ cd.change().getProject().get()));
}
for (SubmitRecord record : results) {
@@ -562,8 +588,8 @@
throw new IllegalStateException(String.format(
"Unsupported SubmitRecord.Label %s for %s in %s",
lbl.toString(),
- rsrc.getPatchSet().getId(),
- rsrc.getChange().getProject().get()));
+ patchSet.getId(),
+ cd.change().getProject().get()));
}
}
throw new ResourceConflictException(msg.toString());
@@ -572,8 +598,8 @@
throw new IllegalStateException(String.format(
"Unsupported SubmitRecord %s for %s in %s",
record,
- rsrc.getPatchSet().getId(),
- rsrc.getChange().getProject().get()));
+ patchSet.getId().getId(),
+ cd.change().getProject().get()));
}
}
throw new IllegalStateException();
