opendev/gerrit
Code Issues Proposed changes

Improve error message for grayed out submit buttons, 2nd edition

Browse Source 
When the submit button is grayed out because the changes would not
merge cleanly, let the user know in the tool tip. To do this properly
we need to first check for the current change and then for all other
changes in the same topic.

There is a fine distinction between BLOCKED_TOPIC_TOOLTIP ("Other
changes in this topic are not ready") and CLICK_FAILURE_OTHER_TOOLTIP
("Clicking the button would fail for other changes in the topic.").
Former indicates that a label is missing whereas the latter indicates
a dry run of a submission has failed (all labels ok, but the submission
is likely to fail)

This subtle behavioral difference is of course tested in ActionsIT,
where you'll have two approved changes, which are colliding with
upstream though.

Change-Id: I7a8389e98d9e20794b06245dab007d93fed7ccf3
This commit is contained in:
Stefan Beller
2015-04-15 10:19:56 -07:00
parent e2e8f4ffb0
commit f3614b0b8a
2 changed files with 78 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

63
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
View File

@@ -18,9 +18,13 @@ import static com.google.common.truth.Truth.assertThat;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ActionInfo;
import com.google.gerrit.testutil.ConfigSuite;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
import org.junit.Test;
@@ -34,7 +38,7 @@ public class ActionsIT extends AbstractDaemonTest {
@Test
public void revisionActionsOneChangePerTopicUnapproved() throws Exception {
String changeId = createChangeWithTopic(name("foo1")).getChangeId();
String changeId = createChangeWithTopic().getChangeId();
Map<String, ActionInfo> actions = getActions(changeId);
assertThat(actions).containsKey("cherrypick");
assertThat(actions).containsKey("rebase");
@@ -43,7 +47,7 @@ public class ActionsIT extends AbstractDaemonTest {
@Test
public void revisionActionsOneChangePerTopic() throws Exception {
String changeId = createChangeWithTopic(name("foo1")).getChangeId();
String changeId = createChangeWithTopic().getChangeId();
approve(changeId);
Map<String, ActionInfo> actions = getActions(changeId);
commonActionsAssertions(actions);
@@ -54,10 +58,10 @@ public class ActionsIT extends AbstractDaemonTest {
@Test
public void revisionActionsTwoChangeChangesInTopic() throws Exception {
String changeId = createChangeWithTopic(name("foo2")).getChangeId();
String changeId = createChangeWithTopic().getChangeId();
approve(changeId);
// create another change with the same topic
createChangeWithTopic(name("foo2")).getChangeId();
createChangeWithTopic().getChangeId();
Map<String, ActionInfo> actions = getActions(changeId);
commonActionsAssertions(actions);
if (isSubmitWholeTopicEnabled()) {
@@ -71,12 +75,43 @@ public class ActionsIT extends AbstractDaemonTest {
}
}
@Test
public void revisionActionsTwoChangeChangesInTopic_conflicting() throws Exception {
String changeId = createChangeWithTopic().getChangeId();
approve(changeId);
// create another change with the same topic
String changeId2 = createChangeWithTopic(testRepo, "foo2", "touching b",
"b.txt", "real content").getChangeId();
approve(changeId2);
// collide with the other change in the same topic
testRepo.reset("HEAD~2");
String collidingChange = createChangeWithTopic(testRepo, "off_topic",
"rewriting file b", "b.txt", "garbage\ngarbage\ngarbage").getChangeId();
gApi.changes().id(collidingChange).current().review(ReviewInput.approve());
gApi.changes().id(collidingChange).current().submit();
Map<String, ActionInfo> actions = getActions(changeId);
commonActionsAssertions(actions);
if (isSubmitWholeTopicEnabled()) {
ActionInfo info = actions.get("submit");
assertThat(info.enabled).isNull();
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.");
} else {
noSubmitWholeTopicAssertions(actions);
}
}
@Test
public void revisionActionsTwoChangeChangesInTopicReady() throws Exception {
String changeId = createChangeWithTopic(name("foo2")).getChangeId();
String changeId = createChangeWithTopic().getChangeId();
approve(changeId);
// create another change with the same topic
String changeId2 = createChangeWithTopic(name("foo2")).getChangeId();
String changeId2 = createChangeWithTopic().getChangeId();
approve(changeId2);
Map<String, ActionInfo> actions = getActions(changeId);
commonActionsAssertions(actions);
@@ -106,10 +141,18 @@ public class ActionsIT extends AbstractDaemonTest {
assertThat(actions).containsKey("rebase");
}
private PushOneCommit.Result createChangeWithTopic(String topic)
throws Exception {
PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo);
private PushOneCommit.Result createChangeWithTopic(
TestRepository<InMemoryRepository> repo, String topic,
String commitMsg, String fileName, String content) throws Exception {
PushOneCommit push = pushFactory.create(db, admin.getIdent(),
repo, commitMsg, fileName, content);
assertThat(topic).isNotEmpty();
return push.to("refs/for/master/" + topic);
return push.to("refs/for/master/" + name(topic));
}
private PushOneCommit.Result createChangeWithTopic()
throws Exception {
return createChangeWithTopic(testRepo, "foo2",
"a message", "a.txt", "content\n");
}
}

44
gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
View File

@@ -35,7 +35,6 @@ 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;
@@ -100,6 +99,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
"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";
public enum Status {
SUBMITTED, MERGED
@@ -134,7 +135,6 @@ 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,
@@ -151,8 +151,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
ChangeIndexer indexer,
LabelNormalizer labelNormalizer,
@GerritServerConfig Config cfg,
Provider<InternalChangeQuery> queryProvider,
Provider<Mergeable> mergeableProvider) {
Provider<InternalChangeQuery> queryProvider) {
this.serverIdent = serverIdent;
this.dbProvider = dbProvider;
this.repoManager = repoManager;
@@ -180,7 +179,6 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
cfg.getString("change", null, "submitTopicTooltip"),
DEFAULT_TOPIC_TOOLTIP));
this.queryProvider = queryProvider;
this.mergeableProvider = mergeableProvider;
}
@Override
@@ -245,14 +243,15 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
}
/**
* @param changes list of changes to be submitted at once
* @param changeList 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,
private String problemsForSubmittingChanges(
List<ChangeData> changeList,
IdentifiedUser identifiedUser) {
for (ChangeData c : changes) {
try {
try {
for (ChangeData c : changeList) {
ChangeControl changeControl = c.changeControl().forUser(
identifiedUser);
if (!changeControl.isVisible(dbProvider.get())) {
@@ -261,13 +260,16 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
if (!changeControl.canSubmit()) {
return BLOCKED_TOPIC_TOOLTIP;
}
if (!c.isMergeable()) {
return CLICK_FAILURE_OTHER_TOOLTIP;
}
checkSubmitRule(c, c.currentPatchSet(), false);
} catch (OrmException e) {
log.error("Error checking if change is submittable", e);
throw new OrmRuntimeException(e);
} catch (ResourceConflictException e) {
return BLOCKED_TOPIC_TOOLTIP;
}
} catch (ResourceConflictException e) {
return BLOCKED_TOPIC_TOOLTIP;
} catch (OrmException e) {
log.error("Error checking if change is submittable", e);
throw new OrmRuntimeException("Could not determine problems for the change", e);
}
return null;
}
@@ -280,12 +282,16 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
&& resource.getChange().getStatus().isOpen()
&& resource.getPatchSet().getId().equals(current)
&& resource.getControl().canSubmit();
ReviewDb db = dbProvider.get();
ChangeData cd = changeDataFactory.create(db, resource.getControl());
if (problemsForSubmittingChanges(Arrays.asList(cd), resource.getUser())
!= null) {
try {
checkSubmitRule(cd, cd.currentPatchSet(), false);
} catch (ResourceConflictException e) {
visible = false;
} catch (OrmException e) {
log.error("Error checking if change is submittable", e);
throw new OrmRuntimeException("Could not determine problems for the change", e);
}
if (!visible) {
@@ -297,8 +303,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
boolean enabled;
try {
enabled = mergeableProvider.get().apply(resource).mergeable;
} catch (RestApiException | OrmException | IOException e) {
enabled = cd.isMergeable();
} catch (OrmException e) {
throw new OrmRuntimeException("Could not determine mergeability", e);
}