Submit: Disable submit button when change is unmergable.
A user reported internally: > It used to be the case that the submit button was greyed out if your > repo required fast-forward and your CL was out of date. Now there's > no obvious way to tell if a submit is likely to succeed without trying > it, which is a minor but daily annoyance. This may have been introduced with the `submitWholeTopic` feature recently. Looking at the code I suspect that there was a client side check for enabling the submit button as well as there is a client side class com.google.gerrit.client.changes.ChangeInfo.MergeableInfo. Part of the `submitWholeTopic` feature was moving responsibility more to the server and dumb down the client a bit. So now we also have to do this computation at the server side. Change-Id: I5dcd8c954446462660d45ead13eb38b6d8fa822a Signed-off-by: Stefan Beller <sbeller@google.com>
This commit is contained in:
		
				
					committed by
					
						
						David Pursehouse
					
				
			
			
				
	
			
			
			
						parent
						
							eecb2c598d
						
					
				
				
					commit
					ef00a864d1
				
			@@ -18,10 +18,13 @@ import static com.google.common.truth.Truth.assertThat;
 | 
			
		||||
 | 
			
		||||
import com.google.gerrit.acceptance.PushOneCommit;
 | 
			
		||||
import com.google.gerrit.extensions.client.SubmitType;
 | 
			
		||||
import com.google.gerrit.extensions.common.ActionInfo;
 | 
			
		||||
 | 
			
		||||
import org.eclipse.jgit.revwalk.RevCommit;
 | 
			
		||||
import org.junit.Test;
 | 
			
		||||
 | 
			
		||||
import java.util.Map;
 | 
			
		||||
 | 
			
		||||
public class SubmitByFastForwardIT extends AbstractSubmit {
 | 
			
		||||
 | 
			
		||||
  @Override
 | 
			
		||||
@@ -51,6 +54,14 @@ public class SubmitByFastForwardIT extends AbstractSubmit {
 | 
			
		||||
    testRepo.reset(initialHead);
 | 
			
		||||
    PushOneCommit.Result change2 =
 | 
			
		||||
        createChange("Change 2", "b.txt", "other content");
 | 
			
		||||
 | 
			
		||||
    approve(change2.getChangeId());
 | 
			
		||||
    Map<String, ActionInfo> actions = getActions(change2.getChangeId());
 | 
			
		||||
 | 
			
		||||
    assertThat(actions).containsKey("submit");
 | 
			
		||||
    ActionInfo info = actions.get("submit");
 | 
			
		||||
    assertThat(info.enabled).isNull();
 | 
			
		||||
 | 
			
		||||
    submitWithConflict(change2.getChangeId());
 | 
			
		||||
    assertThat(getRemoteHead()).isEqualTo(oldHead);
 | 
			
		||||
    assertSubmitter(change.getChangeId(), 1);
 | 
			
		||||
 
 | 
			
		||||
@@ -35,6 +35,7 @@ import com.google.gerrit.extensions.api.changes.SubmitInput;
 | 
			
		||||
import com.google.gerrit.extensions.common.ChangeInfo;
 | 
			
		||||
import com.google.gerrit.extensions.restapi.AuthException;
 | 
			
		||||
import com.google.gerrit.extensions.restapi.ResourceConflictException;
 | 
			
		||||
import com.google.gerrit.extensions.restapi.RestApiException;
 | 
			
		||||
import com.google.gerrit.extensions.restapi.RestModifyView;
 | 
			
		||||
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 | 
			
		||||
import com.google.gerrit.extensions.webui.UiAction;
 | 
			
		||||
@@ -133,6 +134,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
 | 
			
		||||
  private final ParameterizedString submitTopicTooltip;
 | 
			
		||||
  private final boolean submitWholeTopic;
 | 
			
		||||
  private final Provider<InternalChangeQuery> queryProvider;
 | 
			
		||||
  private final Provider<Mergeable> mergeableProvider;
 | 
			
		||||
 | 
			
		||||
  @Inject
 | 
			
		||||
  Submit(@GerritPersonIdent PersonIdent serverIdent,
 | 
			
		||||
@@ -149,7 +151,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
 | 
			
		||||
      ChangeIndexer indexer,
 | 
			
		||||
      LabelNormalizer labelNormalizer,
 | 
			
		||||
      @GerritServerConfig Config cfg,
 | 
			
		||||
      Provider<InternalChangeQuery> queryProvider) {
 | 
			
		||||
      Provider<InternalChangeQuery> queryProvider,
 | 
			
		||||
      Provider<Mergeable> mergeableProvider) {
 | 
			
		||||
    this.serverIdent = serverIdent;
 | 
			
		||||
    this.dbProvider = dbProvider;
 | 
			
		||||
    this.repoManager = repoManager;
 | 
			
		||||
@@ -177,6 +180,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
 | 
			
		||||
        cfg.getString("change", null, "submitTopicTooltip"),
 | 
			
		||||
        DEFAULT_TOPIC_TOOLTIP));
 | 
			
		||||
    this.queryProvider = queryProvider;
 | 
			
		||||
    this.mergeableProvider = mergeableProvider;
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  @Override
 | 
			
		||||
@@ -290,6 +294,14 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
 | 
			
		||||
        .setTitle("")
 | 
			
		||||
        .setVisible(false);
 | 
			
		||||
    }
 | 
			
		||||
 | 
			
		||||
    boolean enabled;
 | 
			
		||||
    try {
 | 
			
		||||
      enabled = mergeableProvider.get().apply(resource).mergeable;
 | 
			
		||||
    } catch (RestApiException | OrmException | IOException e) {
 | 
			
		||||
      throw new OrmRuntimeException("Could not determine mergeability", e);
 | 
			
		||||
    }
 | 
			
		||||
 | 
			
		||||
    List<ChangeData> changesByTopic = null;
 | 
			
		||||
    if (submitWholeTopic && !Strings.isNullOrEmpty(topic)) {
 | 
			
		||||
      changesByTopic = getChangesByTopic(topic);
 | 
			
		||||
@@ -313,7 +325,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
 | 
			
		||||
          .setTitle(Strings.emptyToNull(
 | 
			
		||||
              submitTopicTooltip.replace(params)))
 | 
			
		||||
          .setVisible(true)
 | 
			
		||||
          .setEnabled(true);
 | 
			
		||||
          .setEnabled(enabled);
 | 
			
		||||
      }
 | 
			
		||||
    } else {
 | 
			
		||||
      RevId revId = resource.getPatchSet().getRevision();
 | 
			
		||||
@@ -324,7 +336,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
 | 
			
		||||
      return new UiAction.Description()
 | 
			
		||||
        .setLabel(label)
 | 
			
		||||
        .setTitle(Strings.emptyToNull(titlePattern.replace(params)))
 | 
			
		||||
        .setVisible(true);
 | 
			
		||||
        .setVisible(true)
 | 
			
		||||
        .setEnabled(enabled);
 | 
			
		||||
    }
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
 
 | 
			
		||||
		Reference in New Issue
	
	Block a user