opendev/gerrit
Code Issues Proposed changes

Always keep Submit button enabled even if change is not mergeable

Browse Source 
If 'changeMerge.test' is set to true Gerrit checks if a change is
mergeable and shows this on the ChangeScreen. If a change is not
mergeable the submit button is hidden. There are two problems with
hiding the submit button:

1. Users get confused why the change cannot be submitted. They are
searching for the submit button and do not understand that they have
to rebase the change. Gerrit administrators are annoyed to answer
questions about this. If trying to submit the change fails there is a
good error message that tells the user that he needs to rebase the
change. This is mostly a problem with the old change screen. On the
new change screen the red "Cannot Merge" and the "Merge Conflict"
status can hardly be overlooked.

2. If a change has a conflict it is possible to resolve the conflict
in a merge commit which is a successor of the conflicting change. If
this merge commit is submitted it is in state 'Submitted, Merge
Pending'. If now the conflicting change is submitted the merge
succeeds. This way of resolving conflicts is not possible if the
submit button is not available on conflicting changes.

Since we are now always showing the submit button (if the approvals
allow submit and the user has the permission to submit) and we are
also always computing the mergeability flag for each open change,
there is no need for the 'changeMerge.test' configuration parameter
anymore. This feature is now enabled by default and the
'changeMerge.test' configuration parameter is removed.

Change-Id: I37e69e67992b3aea153e134a5f21fe76ba7e2b91
Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>
This commit is contained in:
Edwin Kempin
2013-11-25 14:04:15 +01:00
parent b376b5bfc8
commit 475b241f4d
11 changed files with 20 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
Documentation/config-gerrit.txt
View File

@@ -813,15 +813,6 @@ master node startup.
+
Default is 300 seconds (5 minutes).
[[changeMerge.test]]changeMerge.test::
+
Controls whether or not the mergeability test of changes is
enabled. If enabled, when the change page is loaded, the test is
triggered. The submit button will be enabled or disabled according to
the result.
+
By default this is false (test is not enabled).
[[changeMerge.threadPoolSize]]changeMerge.threadPoolSize::
+
Maximum size of the thread pool in which the mergeability flag of open

9
gerrit-common/src/main/java/com/google/gerrit/common/data/GerritConfig.java
View File

@@ -48,7 +48,6 @@ public class GerritConfig implements Cloneable {
protected Project.NameKey wildProject;
protected Set<Account.FieldName> editableAccountFields;
protected boolean documentationAvailable;
protected boolean testChangeMerge;
protected String anonymousCowardName;
protected int suggestFrom;
protected int changeUpdateDelay;
@@ -234,14 +233,6 @@ public class GerritConfig implements Cloneable {
documentationAvailable = available;
}
public boolean testChangeMerge() {
return testChangeMerge;
}
public void setTestChangeMerge(final boolean test) {
testChangeMerge = test;
}
public String getAnonymousCowardName() {
return anonymousCowardName;
}

4
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Actions.java
View File

@@ -137,8 +137,8 @@ class Actions extends Composite {
return ids;
}
void setSubmitEnabled(boolean ok) {
submit.setVisible(ok && canSubmit);
void setSubmitEnabled() {
submit.setVisible(canSubmit);
}
boolean isSubmitEnabled() {

17
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen2.java
View File

@@ -648,7 +648,7 @@ public class ChangeScreen2 extends Screen {
private void loadSubmitType(final Change.Status status, final boolean canSubmit) {
if (canSubmit) {
actions.setSubmitEnabled(true);
actions.setSubmitEnabled();
if (status == Change.Status.NEW) {
statusText.setInnerText(Util.C.readyToSubmit());
}
@@ -658,17 +658,14 @@ public class ChangeScreen2 extends Screen {
.get(new AsyncCallback<NativeString>() {
@Override
public void onSuccess(NativeString result) {
if (Gerrit.getConfig().testChangeMerge()) {
if (canSubmit) {
actions.setSubmitEnabled(changeInfo.mergeable());
if (status == Change.Status.NEW) {
statusText.setInnerText(changeInfo.mergeable()
? Util.C.readyToSubmit()
: Util.C.mergeConflict());
}
if (canSubmit) {
if (status == Change.Status.NEW) {
statusText.setInnerText(changeInfo.mergeable()
? Util.C.readyToSubmit()
: Util.C.mergeConflict());
}
setVisible(notMergeable, !changeInfo.mergeable());
}
setVisible(notMergeable, !changeInfo.mergeable());
renderSubmitType(result.asString());
}

3
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java
View File

@@ -164,8 +164,7 @@ public class ApprovalTable extends Composite {
table.setVisible(true);
}
if (Gerrit.getConfig().testChangeMerge()
&& change.status() != Change.Status.MERGED
if (change.status() != Change.Status.MERGED
&& !change.mergeable()) {
addMissingLabel(Util.C.messageNeedsRebaseOrHasDependency());
}

24
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfoBlock.java
View File

@@ -60,11 +60,7 @@ public class ChangeInfoBlock extends Composite {
private final Grid table;
public ChangeInfoBlock() {
if (Gerrit.getConfig().testChangeMerge()) {
table = new Grid(R_CNT, 2);
} else {
table = new Grid(R_CNT - 1, 2);
}
table = new Grid(R_CNT, 2);
table.setStyleName(Gerrit.RESOURCES.css().infoBlock());
table.addStyleName(Gerrit.RESOURCES.css().changeInfoBlock());
@@ -77,9 +73,7 @@ public class ChangeInfoBlock extends Composite {
initRow(R_UPDATED, Util.C.changeInfoBlockUpdated());
initRow(R_STATUS, Util.C.changeInfoBlockStatus());
initRow(R_SUBMIT_TYPE, Util.C.changeInfoBlockSubmitType());
if (Gerrit.getConfig().testChangeMerge()) {
initRow(R_MERGE_TEST, Util.C.changeInfoBlockCanMerge());
}
initRow(R_MERGE_TEST, Util.C.changeInfoBlockCanMerge());
final CellFormatter fmt = table.getCellFormatter();
fmt.addStyleName(0, 0, Gerrit.RESOURCES.css().topmost());
@@ -128,14 +122,12 @@ public class ChangeInfoBlock extends Composite {
}
table.setText(R_SUBMIT_TYPE, 1, submitType);
final Change.Status status = chg.getStatus();
if (Gerrit.getConfig().testChangeMerge()) {
if (status.equals(Change.Status.NEW) || status.equals(Change.Status.DRAFT)) {
table.getRowFormatter().setVisible(R_MERGE_TEST, true);
table.setText(R_MERGE_TEST, 1, chg.isMergeable() ? Util.C
.changeInfoBlockCanMergeYes() : Util.C.changeInfoBlockCanMergeNo());
} else {
table.getRowFormatter().setVisible(R_MERGE_TEST, false);
}
if (status.equals(Change.Status.NEW) || status.equals(Change.Status.DRAFT)) {
table.getRowFormatter().setVisible(R_MERGE_TEST, true);
table.setText(R_MERGE_TEST, 1, chg.isMergeable() ? Util.C
.changeInfoBlockCanMergeYes() : Util.C.changeInfoBlockCanMergeNo());
} else {
table.getRowFormatter().setVisible(R_MERGE_TEST, false);
}
if (status.isClosed()) {

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeTable2.java
View File

@@ -206,7 +206,7 @@ public class ChangeTable2 extends NavigationTable<ChangeInfo> {
Change.Status status = c.status();
if (status != Change.Status.NEW) {
table.setText(row, C_STATUS, Util.toLongString(status));
} else if (Gerrit.getConfig().testChangeMerge() && !c.mergeable()) {
} else if (!c.mergeable()) {
table.setText(row, C_STATUS, Util.C.changeTableNotMergeable());
table.getCellFormatter().addStyleName(row, C_STATUS, Gerrit.RESOURCES.css().notMergeable());
}

4
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java
View File

@@ -312,10 +312,6 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel
final Button b =
new Button(Util.M
.submitPatchSet(detail.getPatchSet().getPatchSetId()));
if (Gerrit.getConfig().testChangeMerge()) {
b.setEnabled(changeDetail.getChange().isMergeable());
}
b.addClickHandler(new ClickHandler() {
@Override
public void onClick(final ClickEvent event) {

3
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java
View File

@@ -376,9 +376,6 @@ public class PublishCommentScreen extends AccountScreen implements
}
submit.setVisible(r.canSubmit());
if (Gerrit.getConfig().testChangeMerge()) {
submit.setEnabled(r.getChange().isMergeable());
}
}
private void onSend(final boolean submit) {

2
gerrit-httpd/src/main/java/com/google/gerrit/httpd/GerritConfigProvider.java
View File

@@ -123,8 +123,6 @@ class GerritConfigProvider implements Provider<GerritConfig> {
config.setWildProject(wildProject);
config.setDocumentationAvailable(servletContext
.getResource("/Documentation/index.html") != null);
config.setTestChangeMerge(cfg.getBoolean("changeMerge",
"test", false));
config.setAnonymousCowardName(anonymousCowardName);
config.setSuggestFrom(cfg.getInt("suggest", "from", 0));
config.setChangeUpdateDelay((int) ConfigUtil.getTimeUnit(

5
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
View File

@@ -88,7 +88,6 @@ public class ChangeDetailFactory extends Handler<ChangeDetail> {
private Map<PatchSet.Id, PatchSet> patchsetsById;
private final Mergeable mergeable;
private boolean testMerge;
private List<PatchSetAncestor> currentPatchSetAncestors;
private List<PatchSet> currentDepPatchSets;
@@ -112,7 +111,6 @@ public class ChangeDetailFactory extends Handler<ChangeDetail> {
this.aic = accountInfoCacheFactory.create();
this.mergeable = mergeable;
this.testMerge = cfg.getBoolean("changeMerge", "test", false);
this.changeId = id;
}
@@ -231,8 +229,7 @@ public class ChangeDetailFactory extends Handler<ChangeDetail> {
private void load() throws OrmException, NoSuchChangeException,
NoSuchProjectException {
final Change.Status status = detail.getChange().getStatus();
if ((status.equals(Change.Status.NEW) || status.equals(Change.Status.DRAFT)) &&
testMerge) {
if ((status.equals(Change.Status.NEW) || status.equals(Change.Status.DRAFT))) {
try {
detail.getChange().setMergeable(mergeable.apply(new RevisionResource(
new ChangeResource(control),