RevisionActions: Do not alter server response
Instead of setting values in the response from the server. Keep the
server response as-is and calculate necessary values from the response.
After 21f0ac88 - 'Removing Current Actions from Change load' server
response were altered twice:
* First in gr-rest-api-interface, were new attr rebaseOnCurrent was
added set to !!rebase.enabled after which rebase.enabled was set to
true since the rebase button should only be disabled on inital
commit.
* Later in gr-change-actions were the logic was duplicated with the
effect that rebaseOnCurrent was always true in the end regardless
of which response was sent from the server.
Parent not current tooltip:
rebase.enabled is used to calculate whether or not the "Not Current"
tooltip on the parent commit-info list in gr-change-metadata. But
since the server response was altered so that rebase.enable was always
true the tooltip was always shown.
Rebase dialog:
Since rebase.enable was always set to true and rebase.rebaseOnCurrent
was set to !!rebase.enable, rebase.rebaseOnCurrent was always set to
true. This meant that the option to rebasen the current target was
always enabled. But using it led to an error if the change was already
up-to-date.
Instead in gr-change-actions calculate wether the rebase action button
should be disabled or not and whether it rebase on current target is
neccessary based on the original, unaltered, server-response.
Alter the tests to test for this behavior.
Bug: Issue 12474
Change-Id: Id29ea8a0c59de0079a776e4ed7ea4122ae2d89c8
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html
index 000756f..e12f10d 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html
@@ -179,7 +179,7 @@
on-cancel="_handleConfirmDialogCancel"
branch="[[change.branch]]"
has-parent="[[hasParent]]"
- rebase-on-current="[[_revisionRebaseAction.rebaseOnCurrent]]"
+ rebase-on-current="[[_computeRebaseOnCurrent(_revisionRebaseAction)]]"
hidden></gr-confirm-rebase-dialog>
<gr-confirm-cherrypick-dialog id="confirmCherrypick"
class="confirmDialog"
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
index 89642e5..9c0c38f 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
@@ -433,9 +433,7 @@
},
_getRebaseAction(revisionActions) {
- return this._getRevisionAction(revisionActions, 'rebase',
- {rebaseOnCurrent: null}
- );
+ return this._getRevisionAction(revisionActions, 'rebase', null);
},
_getRevisionAction(revisionActions, actionName, emptyActionValue) {
@@ -459,7 +457,7 @@
return this._getRevisionActions().then(revisionActions => {
if (!revisionActions) { return; }
- this.revisionActions = this._updateRebaseAction(revisionActions);
+ this.revisionActions = revisionActions;
this._sendShowRevisionActions({
change: this.change,
revisionActions,
@@ -483,18 +481,6 @@
);
},
- _updateRebaseAction(revisionActions) {
- if (revisionActions && revisionActions.rebase) {
- revisionActions.rebase.rebaseOnCurrent =
- !!revisionActions.rebase.enabled;
- this._parentIsCurrent = !revisionActions.rebase.enabled;
- revisionActions.rebase.enabled = true;
- } else {
- this._parentIsCurrent = true;
- }
- return revisionActions;
- },
-
_changeChanged() {
this.reload();
},
@@ -1045,8 +1031,9 @@
},
_calculateDisabled(action, hasKnownChainState) {
- if (action.__key === 'rebase' && hasKnownChainState === false) {
- return true;
+ if (action.__key === 'rebase') {
+ // Rebase button is only disabled when change has no parent(s).
+ return hasKnownChainState === false;
}
return !action.enabled;
},
@@ -1483,6 +1470,13 @@
});
},
+ _computeRebaseOnCurrent(revisionRebaseAction) {
+ if (revisionRebaseAction) {
+ return !!revisionRebaseAction.enabled;
+ }
+ return null;
+ },
+
/**
* Occasionally, a change created by a change action is not yet knwon to the
* API for a brief time. Wait for the given change number to be recognized.
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
index 520e03f..74d262a 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html
@@ -363,7 +363,7 @@
action.enabled = false;
assert.equal(
- element._calculateDisabled(action, hasKnownChainState), true);
+ element._calculateDisabled(action, hasKnownChainState), false);
});
test('rebase change', done => {
@@ -385,7 +385,6 @@
};
assert.isTrue(fetchChangesStub.called);
element._handleRebaseConfirm({detail: {base: '1234'}});
- rebaseAction.rebaseOnCurrent = true;
assert.deepEqual(fireActionStub.lastCall.args,
['/rebase', rebaseAction, true, {base: '1234'}]);
done();
@@ -1569,57 +1568,23 @@
assert.strictEqual(element.$.confirmRebase.rebaseOnCurrent, null);
});
- test('_updateRebaseAction sets _parentIsCurrent on no rebase', () => {
- const currentRevisionActions = {
- cherrypick: {
- enabled: true,
- label: 'Cherry Pick',
- method: 'POST',
- title: 'cherrypick',
- },
+ test('_computeRebaseOnCurrent', () => {
+ const rebaseAction = {
+ enabled: true,
+ label: 'Rebase',
+ method: 'POST',
+ title: 'Rebase onto tip of branch or parent change',
};
- element._parentIsCurrent = undefined;
- element._updateRebaseAction(currentRevisionActions);
- assert.isTrue(element._parentIsCurrent);
- });
- test('_updateRebaseAction', () => {
- const 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',
- },
- };
- element._parentIsCurrent = undefined;
-
- // 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._computeRebaseOnCurrent(rebaseAction));
- assert.isTrue(currentRevisionActions.rebase.enabled);
- assert.isTrue(currentRevisionActions.rebase.rebaseOnCurrent);
- assert.isFalse(element._parentIsCurrent);
-
- delete currentRevisionActions.rebase.enabled;
+ delete rebaseAction.enabled;
// When rebase is not enabled initially, rebaseOnCurrent should be set to
// false.
- assert.equal(element._updateRebaseAction(currentRevisionActions),
- currentRevisionActions);
-
- assert.isTrue(currentRevisionActions.rebase.enabled);
- assert.isFalse(currentRevisionActions.rebase.rebaseOnCurrent);
- assert.isTrue(element._parentIsCurrent);
+ assert.isFalse(element._computeRebaseOnCurrent(rebaseAction));
});
});
</script>
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index 20c3d15..0c929b4 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -203,7 +203,6 @@
_loading: Boolean,
/** @type {?} */
_projectConfig: Object,
- _rebaseOnCurrent: Boolean,
_replyButtonLabel: {
type: String,
value: 'Reply',
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index 03735c9..8e4af15 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -1200,12 +1200,6 @@
reportEndpointAsIs: true,
};
return this._getChangeURLAndFetch(req).then(revisionActions => {
- // The rebase button on change screen is always enabled.
- if (revisionActions.rebase) {
- revisionActions.rebase.rebaseOnCurrent =
- !!revisionActions.rebase.enabled;
- revisionActions.rebase.enabled = true;
- }
return revisionActions;
});
},
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
index 1781ce7..635e0f5 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
@@ -321,37 +321,6 @@
]);
});
- suite('rebase action', () => {
- let resolve_fetchJSON;
- setup(() => {
- sandbox.stub(element._restApiHelper, 'fetchJSON').returns(
- new Promise(resolve => {
- resolve_fetchJSON = resolve;
- }));
- });
-
- test('no rebase on current', done => {
- element.getChangeRevisionActions('42', '1337').then(
- response => {
- assert.isTrue(response.rebase.enabled);
- assert.isFalse(response.rebase.rebaseOnCurrent);
- done();
- });
- resolve_fetchJSON({rebase: {}});
- });
-
- test('rebase on current', done => {
- element.getChangeRevisionActions('42', '1337').then(
- response => {
- assert.isTrue(response.rebase.enabled);
- assert.isTrue(response.rebase.rebaseOnCurrent);
- done();
- });
- resolve_fetchJSON({rebase: {enabled: true}});
- });
- });
-
-
test('server error', done => {
const getResponseObjectStub = sandbox.stub(element, 'getResponseObject');
window.fetch.returns(Promise.resolve({ok: false}));
