Submit: Include ancestors into problem description

When the merge queue was killed, the submission model was changed to include the ancestors of the set of changes to be submitted, which consisted only of the changes in the same topic. Now the set to be submitted consists of changes in the same topic and all its ancestors. If those ancestors have a topic, they will aggregate further changes with their topic recursively. Currently the model for generating the description and title for the submit button still follow the old model, so it is easy to come up with a situation where the change screen makes a user believe a change can be submitted just fine, but it cannot, which is revealed during the merge process. Align the model for generating the submit button tooltip with the actual model used during merge. Change-Id: I41c3c4445ed8f4707b3aacb6745a23f435c7e943
2015-07-20 21:14:38 -07:00
parent e71ed6fcde
commit 0e655543a9
4 changed files with 142 additions and 68 deletions
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -871,6 +871,17 @@ abbreviated commit SHA-1 (`c9c0edb`).
 +
 Default is "Submit patch set ${patchSet} into ${branch}".

+[[change.submitTooltipAncestors]]change.submitTooltipAncestors::
+
+Tooltip for the submit button if there are ancestors which would
+also be submitted by submitting the change. Additionally to the variables
+as in link:#change.submitTooltip[change.submitTooltip], there is the
+variable `${submitSize}` indicating the number of changes which are
+submitted.
+
+Default is "Submit all ${topicSize} changes of the same topic (${submitSize}
+changes including ancestors and other changes related by topic)".
+
 [[change.submitWholeTopic]]change.submitWholeTopic::
 +
 Determines if the submit button submits the whole topic instead of
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
@@ -52,15 +52,14 @@ public class ActionsIT extends AbstractDaemonTest {
    commonActionsAssertions(actions);
    // We want to treat a single change in a topic not as a whole topic,
    // so regardless of how submitWholeTopic is configured:
-    noSubmitWholeTopicAssertions(actions);
+    noSubmitWholeTopicAssertions(actions, 1);
  }

  @Test
-  public void revisionActionsTwoChangeChangesInTopic() throws Exception {
+  public void revisionActionsTwoChangesInTopic() throws Exception {
    String changeId = createChangeWithTopic().getChangeId();
    approve(changeId);
-    // create another change with the same topic
-    createChangeWithTopic().getChangeId();
+    String changeId2 = createChangeWithTopic().getChangeId();
    Map<String, ActionInfo> actions = getActions(changeId);
    commonActionsAssertions(actions);
    if (isSubmitWholeTopicEnabled()) {
@@ -68,14 +67,19 @@ public class ActionsIT extends AbstractDaemonTest {
      assertThat(info.enabled).isNull();
      assertThat(info.label).isEqualTo("Submit whole topic");
      assertThat(info.method).isEqualTo("POST");
-      assertThat(info.title).isEqualTo("Other changes in this topic are not ready");
+      assertThat(info.title).isEqualTo("This change depends on other " +
+          "changes which are not ready");
    } else {
-      noSubmitWholeTopicAssertions(actions);
+      noSubmitWholeTopicAssertions(actions, 1);
+
+      assertThat(getActions(changeId2).get("submit")).isNull();
+      approve(changeId2);
+      noSubmitWholeTopicAssertions(getActions(changeId2), 2);
    }
  }

  @Test
-  public void revisionActionsTwoChangeChangesInTopic_conflicting() throws Exception {
+  public void revisionActionsTwoChangesInTopic_conflicting() throws Exception {
    String changeId = createChangeWithTopic().getChangeId();
    approve(changeId);

@@ -99,38 +103,66 @@ public class ActionsIT extends AbstractDaemonTest {
      assertThat(info.label).isEqualTo("Submit whole topic");
      assertThat(info.method).isEqualTo("POST");
      assertThat(info.title).isEqualTo(
-          "Clicking the button would fail for other changes in the topic");
+          "Clicking the button would fail for other changes");
    } else {
-      noSubmitWholeTopicAssertions(actions);
+      noSubmitWholeTopicAssertions(actions, 1);
    }
  }

  @Test
-  public void revisionActionsTwoChangeChangesInTopicReady() throws Exception {
-    String changeId = createChangeWithTopic().getChangeId();
+  public void revisionActionsTwoChangesInTopicWithAncestorReady()
+      throws Exception {
+    String changeId = createChange().getChangeId();
    approve(changeId);
+    approve(changeId);
+    String changeId1 = createChangeWithTopic().getChangeId();
+    approve(changeId1);
    // create another change with the same topic
    String changeId2 = createChangeWithTopic().getChangeId();
    approve(changeId2);
-    Map<String, ActionInfo> actions = getActions(changeId);
+    Map<String, ActionInfo> actions = getActions(changeId1);
    commonActionsAssertions(actions);
    if (isSubmitWholeTopicEnabled()) {
      ActionInfo info = actions.get("submit");
      assertThat(info.enabled).isTrue();
      assertThat(info.label).isEqualTo("Submit whole topic");
      assertThat(info.method).isEqualTo("POST");
-      assertThat(info.title).isEqualTo("Submit all 2 changes of the same topic");
+      assertThat(info.title).isEqualTo("Submit all 2 changes of the same " +
+          "topic (3 changes including ancestors " +
+          "and other changes related by topic)");
    } else {
-      noSubmitWholeTopicAssertions(actions);
+      noSubmitWholeTopicAssertions(actions, 2);
    }
  }

-  private void noSubmitWholeTopicAssertions(Map<String, ActionInfo> actions) {
+  @Test
+  public void revisionActionsReadyWithAncestors() throws Exception {
+    String changeId = createChange().getChangeId();
+    approve(changeId);
+    approve(changeId);
+    String changeId1 = createChange().getChangeId();
+    approve(changeId1);
+    String changeId2 = createChangeWithTopic().getChangeId();
+    approve(changeId2);
+    Map<String, ActionInfo> actions = getActions(changeId2);
+    commonActionsAssertions(actions);
+    // The topic contains only one change, so standard text applies
+    noSubmitWholeTopicAssertions(actions, 3);
+  }
+
+  private void noSubmitWholeTopicAssertions(Map<String, ActionInfo> actions,
+      int nrChanges) {
    ActionInfo info = actions.get("submit");
    assertThat(info.enabled).isTrue();
    assertThat(info.label).isEqualTo("Submit");
    assertThat(info.method).isEqualTo("POST");
-    assertThat(info.title).isEqualTo("Submit patch set 1 into master");
+    if (nrChanges == 1) {
+      assertThat(info.title).isEqualTo("Submit patch set 1 into master");
+    } else {
+      assertThat(info.title).isEqualTo(String.format(
+          "Submit patch set 1 and ancestors (%d changes " +
+          "altogether) into master", nrChanges));
+    }
  }

  private void commonActionsAssertions(Map<String, ActionInfo> actions) {
--- 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
@@ -46,6 +46,7 @@ import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.ChangeSet;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MergeOp;
+import com.google.gerrit.server.git.MergeSuperSet;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.SubmitRuleEvaluator;
@@ -64,8 +65,6 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

 import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
@@ -77,16 +76,21 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,

  private static final String DEFAULT_TOOLTIP =
      "Submit patch set ${patchSet} into ${branch}";
+  private static final String DEFAULT_TOOLTIP_ANCESTORS =
+      "Submit patch set ${patchSet} and ancestors (${submitSize} changes " +
+      "altogether) 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";
-  private static final String CLICK_FAILURE_OTHER_TOOLTIP =
-      "Clicking the button would fail for other changes in the topic";
+      "Submit all ${topicSize} changes of the same topic " +
+      "(${submitSize} changes including ancestors and other " +
+      "changes related by topic)";
+  private static final String BLOCKED_SUBMIT_TOOLTIP =
+      "This change depends on other changes which are not ready";
+  private static final String BLOCKED_HIDDEN_SUBMIT_TOOLTIP =
+      "This change depends on other hidden changes which are not ready";
  private static final String CLICK_FAILURE_TOOLTIP =
-      "Clicking the button would fail.";
+      "Clicking the button would fail";
+  private static final String CLICK_FAILURE_OTHER_TOOLTIP =
+      "Clicking the button would fail for other changes";

  public static class Output {
    transient Change change;
@@ -100,11 +104,14 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
  private final GitRepositoryManager repoManager;
  private final ChangeData.Factory changeDataFactory;
  private final ChangeMessagesUtil cmUtil;
+  private final ChangeControl.GenericFactory changeControlFactory;
  private final Provider<MergeOp> mergeOpProvider;
+  private final MergeSuperSet mergeSuperSet;
  private final AccountsCollection accounts;
  private final ChangesCollection changes;
  private final String label;
  private final ParameterizedString titlePattern;
+  private final ParameterizedString titlePatternWithAncestors;
  private final String submitTopicLabel;
  private final ParameterizedString submitTopicTooltip;
  private final boolean submitWholeTopic;
@@ -115,7 +122,9 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
      GitRepositoryManager repoManager,
      ChangeData.Factory changeDataFactory,
      ChangeMessagesUtil cmUtil,
+      ChangeControl.GenericFactory changeControlFactory,
      Provider<MergeOp> mergeOpProvider,
+      MergeSuperSet mergeSuperSet,
      AccountsCollection accounts,
      ChangesCollection changes,
      @GerritServerConfig Config cfg,
@@ -124,7 +133,9 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
    this.repoManager = repoManager;
    this.changeDataFactory = changeDataFactory;
    this.cmUtil = cmUtil;
+    this.changeControlFactory = changeControlFactory;
    this.mergeOpProvider = mergeOpProvider;
+    this.mergeSuperSet = mergeSuperSet;
    this.accounts = accounts;
    this.changes = changes;
    this.label = MoreObjects.firstNonNull(
@@ -133,6 +144,10 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
    this.titlePattern = new ParameterizedString(MoreObjects.firstNonNull(
        cfg.getString("change", null, "submitTooltip"),
        DEFAULT_TOOLTIP));
+    this.titlePatternWithAncestors = new ParameterizedString(
+        MoreObjects.firstNonNull(
+            cfg.getString("change", null, "submitTooltipAncestors"),
+            DEFAULT_TOOLTIP_ANCESTORS));
    submitWholeTopic = wholeTopicEnabled(cfg);
    this.submitTopicLabel = MoreObjects.firstNonNull(
        Strings.emptyToNull(cfg.getString("change", null, "submitTopicLabel")),
@@ -170,16 +185,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
          rsrc.getPatchSet().getRevision().get()));
    }

-    List<Change> changes;
-    if (submitWholeTopic && !Strings.isNullOrEmpty(change.getTopic())) {
-      changes = new ArrayList<>();
-      for (ChangeData cd : getChangesByTopic(change.getTopic())) {
-        changes.add(cd.change());
-      }
-    } else {
-      changes = Arrays.asList(change);
-    }
-    ChangeSet submittedChanges = ChangeSet.create(changes);
+    ChangeSet submittedChanges = ChangeSet.create(change);

    try {
      ReviewDb db = dbProvider.get();
@@ -207,22 +213,24 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
  }

  /**
-   * @param changeList list of changes to be submitted at once
+   * @param cs set 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> changeList,
-      IdentifiedUser identifiedUser) {
+  private String problemsForSubmittingChangeset(
+      ChangeSet cs, IdentifiedUser identifiedUser) {
    try {
-      for (ChangeData c : changeList) {
-        ChangeControl changeControl = c.changeControl().forUser(
-            identifiedUser);
+      for (PatchSet.Id psId : cs.patchIds()) {
+        ReviewDb db = dbProvider.get();
+        ChangeControl changeControl = changeControlFactory
+            .controlFor(psId.getParentKey(), identifiedUser);
+        ChangeData c = changeDataFactory.create(db, changeControl);
+
        if (!changeControl.isVisible(dbProvider.get())) {
-          return BLOCKED_HIDDEN_TOPIC_TOOLTIP;
+          return BLOCKED_HIDDEN_SUBMIT_TOOLTIP;
        }
        if (!changeControl.canSubmit()) {
-          return BLOCKED_TOPIC_TOOLTIP;
+          return BLOCKED_SUBMIT_TOOLTIP;
        }
        // Recheck mergeability rather than using value stored in the index,
        // which may be stale.
@@ -240,8 +248,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
        checkSubmitRule(c, c.currentPatchSet(), false);
      }
    } catch (ResourceConflictException e) {
-      return BLOCKED_TOPIC_TOOLTIP;
-    } catch (OrmException e) {
+      return BLOCKED_SUBMIT_TOOLTIP;
+    } catch (NoSuchChangeException | OrmException e) {
      log.error("Error checking if change is submittable", e);
      throw new OrmRuntimeException("Could not determine problems for the change", e);
    }
@@ -282,40 +290,55 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
      throw new OrmRuntimeException("Could not determine mergeability", e);
    }

-    List<ChangeData> changesByTopic = null;
-    if (submitWholeTopic && !Strings.isNullOrEmpty(topic)) {
-      changesByTopic = getChangesByTopic(topic);
+    ChangeSet cs;
+    try {
+      cs = mergeSuperSet.completeChangeSet(db,
+          ChangeSet.create(cd.change()));
+    } catch (OrmException | IOException e) {
+      throw new OrmRuntimeException("Could not determine complete set of " +
+          "changes to be submitted", e);
    }
-    if (submitWholeTopic
+
+    int topicSize = 0;
+    if (!Strings.isNullOrEmpty(topic)) {
+      topicSize = getChangesByTopic(topic).size();
+    }
+    boolean treatWithTopic = submitWholeTopic
        && !Strings.isNullOrEmpty(topic)
-        && changesByTopic.size() > 1) {
+        && topicSize > 1;
+
+    String submitProblems = problemsForSubmittingChangeset(cs,
+        resource.getUser());
+    if (submitProblems != null) {
+      return new UiAction.Description()
+        .setLabel(treatWithTopic ? submitTopicLabel : label)
+        .setTitle(submitProblems)
+        .setVisible(true)
+        .setEnabled(false);
+    }
+
+    if (treatWithTopic) {
      Map<String, String> params = ImmutableMap.of(
-          "topicSize", String.valueOf(changesByTopic.size()));
-      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()
+          "topicSize", String.valueOf(topicSize),
+          "submitSize", String.valueOf(cs.size()));
+      return new UiAction.Description()
          .setLabel(submitTopicLabel)
          .setTitle(Strings.emptyToNull(
              submitTopicTooltip.replace(params)))
          .setVisible(true)
          .setEnabled(Boolean.TRUE.equals(enabled));
-      }
    } else {
      RevId revId = resource.getPatchSet().getRevision();
      Map<String, String> params = ImmutableMap.of(
          "patchSet", String.valueOf(resource.getPatchSet().getPatchSetId()),
          "branch", resource.getChange().getDest().getShortName(),
-          "commit", ObjectId.fromString(revId.get()).abbreviate(7).name());
+          "commit", ObjectId.fromString(revId.get()).abbreviate(7).name(),
+          "submitSize", String.valueOf(cs.size()));
+      ParameterizedString tp = cs.size() > 1 ? titlePatternWithAncestors :
+          titlePattern;
      return new UiAction.Description()
        .setLabel(label)
-        .setTitle(Strings.emptyToNull(titlePattern.replace(params)))
+        .setTitle(Strings.emptyToNull(tp.replace(params)))
        .setVisible(true)
        .setEnabled(Boolean.TRUE.equals(enabled));
    }
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java
@@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;

 /** A set of changes grouped together to be submitted atomically.*/
@@ -29,6 +30,7 @@ public abstract class ChangeSet {
    ImmutableSet.Builder<Project.NameKey> pb = ImmutableSet.builder();
    ImmutableSet.Builder<Branch.NameKey> bb = ImmutableSet.builder();
    ImmutableSet.Builder<Change.Id> ib = ImmutableSet.builder();
+    ImmutableSet.Builder<PatchSet.Id> psb = ImmutableSet.builder();
    ImmutableSetMultimap.Builder<Project.NameKey, Branch.NameKey> pbb =
        ImmutableSetMultimap.builder();
    ImmutableSetMultimap.Builder<Project.NameKey, Change.Id> pcb =
@@ -42,13 +44,14 @@ public abstract class ChangeSet {
      pb.add(project);
      bb.add(branch);
      ib.add(c.getId());
+      psb.add(c.currentPatchSetId());
      pbb.put(project, branch);
      pcb.put(project, c.getId());
      cbb.put(branch, c.getId());
    }

-    return new AutoValue_ChangeSet(pb.build(), bb.build(),
-        ib.build(), pbb.build(), pcb.build(), cbb.build());
+    return new AutoValue_ChangeSet(pb.build(), bb.build(), ib.build(),
+        psb.build(), pbb.build(), pcb.build(), cbb.build());
  }

  public static ChangeSet create(Change change) {
@@ -58,6 +61,7 @@ public abstract class ChangeSet {
  public abstract ImmutableSet<Project.NameKey> projects();
  public abstract ImmutableSet<Branch.NameKey> branches();
  public abstract ImmutableSet<Change.Id> ids();
+  public abstract ImmutableSet<PatchSet.Id> patchIds();
  public abstract ImmutableSetMultimap<Project.NameKey, Branch.NameKey>
      branchesByProject();
  public abstract ImmutableSetMultimap<Project.NameKey, Change.Id>
@@ -69,4 +73,8 @@ public abstract class ChangeSet {
  public int hashCode() {
    return ids().hashCode();
  }
+
+  public int size() {
+    return ids().size();
+  }
 }