opendev/gerrit
Code Issues Proposed changes

Change rebase action behavior

Browse Source 
Previously, if the server did not return enabled=true for the rebase
change action, the rebase button would be disabled on the UI. Really,
what disabled indicated in this case was that the rebase could not be
performed on the current branch, but could still perform a rebase on
a different parent revision.

This change updates revision actions in the change view always be
enabled, and introduces a new parameter to indicate whether or not
rebasing should be enabled on the current branch based on the initial
status of the enabled flag.

This change also disables the rebase button on the rebase dialog until
an option is explicitly chosen. Previously, if an option was not chosen,
if rebase was clicked, it would submit with a base branch of '', which
is the same thing checking the box accomplishes. Now, either the box
must be checked, or a base change must be entered, so it is more clear
what happens with the rebase and in the new scenario when you cannot
submit on the base branch, there won't be a possibility of a server
error.

Bug: Issue 5126
Change-Id: I47083ad328095aad4e04f2fc1586ad4d34412e4c
This commit is contained in:
Becky Siegel
2017-01-04 12:48:03 -08:00
parent 4dc0da639e
commit a5056224d1
10 changed files with 120 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html
View File

@@ -92,6 +92,7 @@ limitations under the License.
</div>
<gr-overlay id="overlay" with-backdrop>
<gr-confirm-rebase-dialog id="confirmRebase"
rebase-on-current="[[rebaseOnCurrent]]"
class="confirmDialog"
on-confirm="_handleRebaseConfirm"
on-cancel="_handleConfirmDialogCancel"

1
polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
View File

@@ -115,6 +115,7 @@
changeStatus: String,
commitNum: String,
patchNum: String,
rebaseOnCurrent: Boolean,
commitMessage: {
type: String,
value: '',

1
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
View File

@@ -267,6 +267,7 @@ limitations under the License.
<gr-change-actions id="actions"
change="[[_change]]"
actions="[[_change.actions]]"
rebase-on-current="[[_rebaseOnCurrent]]"
revision-actions="[[_currentRevisionActions]]"
change-num="[[_changeNum]]"
change-status="[[_change.status]]"

12
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
View File

@@ -96,6 +96,7 @@
},
_loading: Boolean,
_projectConfig: Object,
_rebaseOnCurrent: Boolean,
_replyButtonLabel: {
type: String,
value: 'Reply',
@@ -710,6 +711,14 @@
}.bind(this));
},
_updateRebaseAction: function(revisionActions) {
if (revisionActions && revisionActions.rebase){
this._rebaseOnCurrent = !!revisionActions.rebase.enabled;
revisionActions.rebase.enabled = true;
}
return revisionActions;
},
_getChangeDetail: function() {
return this.$.restAPI.getChangeDetail(this._changeNum,
this._handleGetChangeDetailError.bind(this)).then(
@@ -735,7 +744,8 @@
currentRevision.commit.commit = latestRevisionSha;
}
this._commitInfo = currentRevision.commit;
this._currentRevisionActions = currentRevision.actions;
this._currentRevisionActions =
this._updateRebaseAction(currentRevision.actions);
// TODO: Fetch and process files.
}
}.bind(this));

35
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
View File

@@ -180,6 +180,41 @@ limitations under the License.
assert.equal(putDescStub.args[0][2], 'test2');
});
test('_updateRebaseAction', function(){
var currentRevisionActions = {
cherrypick: {
enabled: true,
label: 'Cherry Pick',
method: 'POST',
title: 'cherrypick'
},
rebase: {
enabled: true,
label: 'Rebase',
method: 'POST',
title: 'Rebase onto tip of branch or parent change'
},
};
// Rebase enabled should always end up true.
// When rebase is enabled initially, rebaseOnCurrent should be set to
// true.
assert.equal(element._updateRebaseAction(currentRevisionActions),
currentRevisionActions);
assert.isTrue(element._rebaseOnCurrent);
var newRevisionActions = currentRevisionActions
delete newRevisionActions.rebase.enabled;
// When rebase is not enabled initially, rebaseOnCurrent should be set to
// false.
assert.equal(element._updateRebaseAction(newRevisionActions),
currentRevisionActions);
assert.isFalse(element._rebaseOnCurrent);
});
test('_reload is called when an approved label is removed', function() {
var vote = {_account_id: 1, name: 'bojack', value: 1};
element._changeNum = '42';

22
polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.html
View File

@@ -31,6 +31,9 @@ limitations under the License.
label {
cursor: pointer;
}
.info {
font-style: italic;
}
.parentRevisionContainer label,
.parentRevisionContainer input[type="text"] {
display: block;
@@ -47,12 +50,16 @@ limitations under the License.
<gr-confirm-dialog
confirm-label="Rebase"
on-confirm="_handleConfirmTap"
on-cancel="_handleCancelTap">
on-cancel="_handleCancelTap"
disabled="[[!valueSelected]]">
<div class="header">Confirm rebase</div>
<div class="main">
<div class="parentRevisionContainer">
<label for="parentInput">
Parent revision (optional)
Parent revision
<span id="optionalText" hidden$="[[!rebaseOnCurrent]]"> (optional)
</span>
<span hidden$="[[rebaseOnCurrent]]"> (not current branch)</span>
</label>
<input is="iron-input"
type="text"
@@ -62,11 +69,18 @@ limitations under the License.
</div>
<div class="clearParentContainer">
<input id="clearParent"
hidden$="[[!rebaseOnCurrent]]"
type="checkbox"
on-tap="_handleClearParentTap">
<label for="clearParent">
on-tap="_handleClearParentTap"
disabled="[[!rebaseOnCurrent]]">
<label id="clearParentLabel" for="clearParent"
hidden$="[[!rebaseOnCurrent]]">
Rebase on top of current branch (clear parent revision).
</label>
<p id="rebaseUpToDateInfo" class="info" for="clearParent"
hidden$="[[rebaseOnCurrent]]">
Already up to date with current branch.
</p>
</div>
</div>
</gr-confirm-dialog>

14
polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.js
View File

@@ -31,7 +31,15 @@
properties: {
base: String,
clearParent: Boolean,
clearParent: {
type: Boolean,
value: false,
},
rebaseOnCurrent: Boolean,
valueSelected: {
type: Boolean,
computed: '_updateValueSelected(base, clearParent)',
},
},
_handleConfirmTap: function(e) {
@@ -52,5 +60,9 @@
this.$.parentInput.disabled = clear;
this.clearParent = clear;
},
_updateValueSelected: function(base, clearParent) {
return base.length || clearParent;
},
});
})();

35
polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.html
View File

@@ -38,14 +38,47 @@ limitations under the License.
element = fixture('basic');
});
test('controls', function() {
test('controls with rebase on current available', function() {
element.rebaseOnCurrent = true;
flushAsynchronousOperations();
// The correct content is hidden/displayed regarding the ability to rebase
// on top of the current branch.
assert.isFalse(element.$.clearParentLabel.hasAttribute('hidden'));
assert.isTrue(element.$.rebaseUpToDateInfo.hasAttribute('hidden'));
assert.isFalse(element.$.optionalText.hasAttribute('hidden'));
assert.isFalse(!!element.valueSelected);
assert.isFalse(element.$.parentInput.hasAttribute('disabled'));
assert.isFalse(element.$.clearParent.hasAttribute('disabled'));
assert.isFalse(element.$.clearParent.checked);
element.base = 'something great';
assert.isTrue(!!element.valueSelected);
MockInteractions.tap(element.$.clearParent);
assert.isTrue(!!element.valueSelected);
assert.isTrue(element.$.parentInput.hasAttribute('disabled'));
assert.isTrue(element.$.clearParent.checked);
assert.equal(element.base, '');
MockInteractions.tap(element.$.clearParent);
assert.isFalse(!!element.valueSelected);
});
test('controls without rebase on current available', function() {
element.rebaseOnCurrent = false;
flushAsynchronousOperations();
// The correct content is hidden/displayed regarding the ability to rebase
// on top of the current branch.
assert.isTrue(element.$.clearParentLabel.hasAttribute('hidden'));
assert.isFalse(element.$.rebaseUpToDateInfo.hasAttribute('hidden'));
assert.isTrue(element.$.optionalText.hasAttribute('hidden'));
assert.isFalse(!!element.valueSelected);
assert.isFalse(element.$.parentInput.hasAttribute('disabled'));
assert.isTrue(element.$.clearParent.hasAttribute('disabled'));
assert.isTrue(element.$.clearParentLabel.hasAttribute('hidden'));
assert.isFalse(element.$.rebaseUpToDateInfo.hasAttribute('hidden'));
element.base = 'something great';
assert.isTrue(!!element.valueSelected);
});
});
</script>

2
polygerrit-ui/app/elements/shared/gr-confirm-dialog/gr-confirm-dialog.html
View File

@@ -54,7 +54,7 @@ limitations under the License.
<header><content select=".header"></content></header>
<main><content select=".main"></content></main>
<footer>
<gr-button primary on-tap="_handleConfirmTap">
<gr-button primary on-tap="_handleConfirmTap" disabled="[[disabled]]">
[[confirmLabel]]
</gr-button>
<gr-button on-tap="_handleCancelTap">Cancel</gr-button>

6
polygerrit-ui/app/elements/shared/gr-confirm-dialog/gr-confirm-dialog.js
View File

@@ -33,7 +33,11 @@
confirmLabel: {
type: String,
value: 'Confirm',
}
},
disabled: {
type: Boolean,
value: false,
},
},
hostAttributes: {