Update gr-confirm-rebase-dialog with tailored options

The three rebase options are 1. Rebase on parent change 2. Rebase on top of the <branch name> branch 3. Rebase on a specific change or ref Scenarios: Has parent, can rebase on parent - Show all 3 options. Second two options show "breaks relation chain" message. Has parent, can't rebase on parent - Instead of option 1, show message that is up to date with parent. - Show second two options with "breaks relation chain" message. Has no parent, can rebase on parent (tip of branch) - Do not show option 1, nor dipslay message in its place. - Show second two options without "breaks relation chain" message. Has no parent, can't rebase on parent (tip of branch) - Do not show option 1, nor dipslay message in its place. - Instead of option, show message that change is up to date with tip of branch already - Show option 3 without "breaks relation chain" message. Because 'hasParent' isn't immediately known when change actions are displayed, the rebase button will also remain disabled until this becomes known. Bug: Issue 5447 Change-Id: I45ef8241a75c1f5d197ed64e51b19c5ce74d0227
2017-02-15 11:32:21 -08:00
parent 2705a51dba
commit 61ecc0cf10
8 changed files with 228 additions and 125 deletions
--- 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
@@ -96,7 +96,7 @@ limitations under the License.
              data-action-key$="[[action.__key]]"
              data-action-type$="[[action.__type]]"
              data-label$="[[action.label]]"
-              disabled$="[[!action.enabled]]"
+              disabled$="[[_calculateDisabled(action, _hasKnownChainState)]]"
              on-tap="_handleActionTap"></gr-button>
        </template>
      </section>
--- 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
@@ -126,10 +126,17 @@
          ];
        },
      },
+      _hasKnownChainState: {
+        type: Boolean,
+        value: false,
+      },
      changeNum: String,
      changeStatus: String,
      commitNum: String,
-      hasParent: Boolean,
+      hasParent: {
+        type: Boolean,
+        observer: '_computeChainState',
+      },
      patchNum: String,
      commitMessage: {
        type: String,
@@ -489,6 +496,22 @@
      return key === '/' ? key : '/' + key;
    },

+    /**
+     * Returns true if hasParent is defined (can be either true or false).
+     * returns false otherwise.
+     * @return {boolean} hasParent
+     */
+    _computeChainState: function(hasParent) {
+      this._hasKnownChainState = true;
+    },
+
+    _calculateDisabled: function(action, hasKnownChainState) {
+      if (action.__key === 'rebase' && hasKnownChainState === false) {
+        return true;
+      }
+      return !action.enabled;
+    },
+
    _handleConfirmDialogCancel: function() {
      this._hideAllDialogs();
    },
@@ -503,19 +526,8 @@
    },

    _handleRebaseConfirm: function() {
-      var payload = {};
      var el = this.$.confirmRebase;
-      if (el.clearParent) {
-        // There is a subtle but important difference between setting the base
-        // to an empty string and omitting it entirely from the payload. An
-        // empty string implies that the parent should be cleared and the
-        // change should be rebased on top of the target branch. Leaving out
-        // the base implies that it should be rebased on top of its current
-        // parent.
-        payload.base = '';
-      } else if (el.base && el.base.length > 0) {
-        payload.base = el.base;
-      }
+      var payload = {base: el.base};
      this.$.overlay.close();
      el.hidden = true;
      this._fireAction('/rebase', this.revisionActions.rebase, true, payload);
@@ -617,47 +629,50 @@
    },

    _handleResponse: function(action, response) {
+      if (!response) { return; }
      return this.$.restAPI.getResponseObject(response).then(function(obj) {
-        switch (action.__key) {
-          case ChangeActions.REVERT:
-            this._setLabelValuesOnRevert(obj.change_id);
-            /* falls through */
-          case RevisionActions.CHERRYPICK:
-            page.show(this.changePath(obj._number));
-            break;
-          case ChangeActions.DELETE:
-          case RevisionActions.DELETE:
-            if (action.__type === ActionType.CHANGE) {
-              page.show('/');
-            } else {
-              page.show(this.changePath(this.changeNum));
-            }
-            break;
-          default:
-            this.dispatchEvent(new CustomEvent('reload-change',
-                {detail: {action: action.__key}, bubbles: false}));
-            break;
-        }
+          switch (action.__key) {
+            case ChangeActions.REVERT:
+              this._setLabelValuesOnRevert(obj.change_id);
+              /* falls through */
+            case RevisionActions.CHERRYPICK:
+              page.show(this.changePath(obj._number));
+              break;
+            case ChangeActions.DELETE:
+            case RevisionActions.DELETE:
+              if (action.__type === ActionType.CHANGE) {
+                page.show('/');
+              } else {
+                page.show(this.changePath(this.changeNum));
+              }
+              break;
+            default:
+              this.dispatchEvent(new CustomEvent('reload-change',
+                  {detail: {action: action.__key}, bubbles: false}));
+              break;
+          }
      }.bind(this));
    },

    _handleResponseError: function(response) {
-      if (response.ok) { return response; }
-
      return response.text().then(function(errText) {
-        alert('Could not perform action: ' + errText);
-        throw Error(errText);
-      });
+        this.fire('show-alert',
+            { message: 'Could not perform action: ' + errText });
+        if (errText.indexOf('Change is already up to date') !== 0) {
+          throw Error(errText);
+        }
+      }.bind(this));
    },

    _send: function(method, payload, actionEndpoint, revisionAction,
-        cleanupFn) {
+        cleanupFn, opt_errorFn) {
      var url = this.$.restAPI.getChangeActionURL(this.changeNum,
          revisionAction ? this.patchNum : null, actionEndpoint);
-      return this.$.restAPI.send(method, url, payload).then(function(response) {
-        cleanupFn.call(this);
-        return response;
-      }.bind(this)).then(this._handleResponseError.bind(this));
+      return this.$.restAPI.send(method, url, payload,
+          this._handleResponseError, this).then(function(response) {
+            cleanupFn.call(this);
+            return response;
+      }.bind(this));
    },

    _handleAbandonTap: function() {
--- 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
@@ -252,6 +252,33 @@ limitations under the License.
      });
    });

+    test('chain state', function() {
+      assert.equal(element._hasKnownChainState, false);
+      element.hasParent = true;
+      assert.equal(element._hasKnownChainState, true);
+      element.hasParent = false;
+    });
+
+    test('_calculateDisabled', function() {
+      var hasKnownChainState = false;
+      var action = {__key: 'rebase', enabled: true};
+      assert.equal(
+          element._calculateDisabled(action, hasKnownChainState), true);
+
+      action.__key = 'delete';
+      assert.equal(
+          element._calculateDisabled(action, hasKnownChainState), false);
+
+      action.__key = 'rebase';
+      hasKnownChainState = true;
+      assert.equal(
+          element._calculateDisabled(action, hasKnownChainState), false);
+
+      action.enabled = false;
+      assert.equal(
+          element._calculateDisabled(action, hasKnownChainState), true);
+    });
+
    test('rebase change', function(done) {
      var fireActionStub = sinon.stub(element, '_fireAction');
      flush(function() {
@@ -266,18 +293,20 @@ limitations under the License.
          method: 'POST',
          title: 'Rebase onto tip of branch or parent change',
        };
+        // rebase on other
        element.$.confirmRebase.base = '1234';
        element._handleRebaseConfirm();
        assert.deepEqual(fireActionStub.lastCall.args,
          ['/rebase', rebaseAction, true, {base: '1234'}]);

-        element.$.confirmRebase.base = '';
+        // rebase on parent
+        element.$.confirmRebase.base = null;
        element._handleRebaseConfirm();
        assert.deepEqual(fireActionStub.lastCall.args,
-          ['/rebase', rebaseAction, true, {}]);
+          ['/rebase', rebaseAction, true, {base: null}]);

-        element.$.confirmRebase.base = 'does not matter';
-        element.$.confirmRebase.clearParent = true;
+        // rebase on tip
+        element.$.confirmRebase.base = '';
        element._handleRebaseConfirm();
        assert.deepEqual(fireActionStub.lastCall.args,
          ['/rebase', rebaseAction, true, {base: ''}]);
@@ -288,6 +317,7 @@ limitations under the License.
    });

    test('two dialogs are not shown at the same time', function(done) {
+      element._hasKnownChainState = true;
      flush(function() {
        var rebaseButton = element.$$('gr-button[data-action-key="rebase"]');
        assert.ok(rebaseButton);
--- a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.html
+++ b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.html
@@ -31,7 +31,7 @@ limitations under the License.
      label {
        cursor: pointer;
      }
-      .info {
+      .message {
        font-style: italic;
      }
      .parentRevisionContainer label,
@@ -43,45 +43,66 @@ limitations under the License.
      .parentRevisionContainer label {
        margin-bottom: .2em;
      }
-      .clearParentContainer {
+      .rebaseOption {
        margin: .5em 0;
      }
    </style>
    <gr-confirm-dialog
        confirm-label="Rebase"
        on-confirm="_handleConfirmTap"
-        on-cancel="_handleCancelTap"
-        disabled="[[!valueSelected]]">
+        on-cancel="_handleCancelTap">
      <div class="header">Confirm rebase</div>
      <div class="main">
-        <div class="parentRevisionContainer">
-          <label for="parentInput">
-            Parent revision
-            <span id="optionalText" hidden$="[[!rebaseOnCurrent]]"> (optional)
-            </span>
-            <span hidden$="[[rebaseOnCurrent]]"> (not current branch)</span>
+        <div id="rebaseOnParentContainer" class="rebaseOption"
+            hidden$="[[!_displayParentOption(rebaseOnCurrent, hasParent)]]">
+          <input id="rebaseOnParent"
+              name="rebaseOptions"
+              type="radio"
+              on-tap="_handleRebaseOnParent">
+          <label id="rebaseOnParentLabel" for="rebaseOnParent">
+            Rebase on parent change
          </label>
+        </div>
+        <div id="parentUpToDateMsg" class="message"
+            hidden$="[[!_displayParentUpToDateMsg(rebaseOnCurrent, hasParent)]]">
+          This change is up to date with its parent.
+        </div>
+        <div id="rebaseOnTip" class="rebaseOption"
+            hidden$="[[!_displayTipOption(rebaseOnCurrent, hasParent)]]">
+          <input id="rebaseOnTip"
+              name="rebaseOptions"
+              type="radio"
+              disabled$="[[!_displayTipOption(rebaseOnCurrent, hasParent)]]"
+              on-tap="_handleRebaseOnTip">
+          <label id="rebaseOnTipLabel" for="rebaseOnTip">
+            Rebase on top of the [[branch]] branch<span hidden="[[!hasParent]]">
+              (breaks relation chain)
+            </span>
+          </label>
+        </div>
+        <div id="tipUpToDateMsg" class="message"
+            hidden$="[[_displayTipOption(rebaseOnCurrent, hasParent)]]">
+          Change is up to date with the target branch already ([[branch]])
+        </div>
+        <div id="rebaseOnOther" class="rebaseOption">
+          <input id="rebaseOnOtherInput"
+              name="rebaseOptions"
+              type="radio"
+              on-tap="_handleRebaseOnOther">
+          <label id="rebaseOnOtherLabel" for="rebaseOnOtherInput">
+            Rebase on a specific change or ref <span hidden="[[!hasParent]]">
+              (breaks relation chain)
+            </span>
+          </label>
+        </div>
+        <div class="parentRevisionContainer">
          <input is="iron-input"
              type="text"
              id="parentInput"
              bind-value="{{base}}"
+              on-tap="_handleEnterChangeNumberTap"
              placeholder="Change number">
        </div>
-        <div class="clearParentContainer">
-          <input id="clearParent"
-              hidden$="[[!rebaseOnCurrent]]"
-              type="checkbox"
-              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>
  </template>
--- a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.js
+++ b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.js
@@ -33,15 +33,23 @@
      base: String,
      branch: String,
      hasParent: Boolean,
-      clearParent: {
-        type: Boolean,
-        value: false,
-      },
      rebaseOnCurrent: Boolean,
-      valueSelected: {
-        type: Boolean,
-        computed: '_updateValueSelected(base, clearParent)',
-      },
+    },
+
+    observers: [
+      '_updateSelectedOption(rebaseOnCurrent, hasParent)',
+    ],
+
+    _displayParentOption: function(rebaseOnCurrent, hasParent) {
+      return hasParent && rebaseOnCurrent;
+    },
+
+    _displayParentUpToDateMsg: function(rebaseOnCurrent, hasParent) {
+      return hasParent && !rebaseOnCurrent;
+    },
+
+    _displayTipOption: function(rebaseOnCurrent, hasParent) {
+      return !(!rebaseOnCurrent && !hasParent);
    },

    _handleConfirmTap: function(e) {
@@ -54,17 +62,44 @@
      this.fire('cancel', null, {bubbles: false});
    },

-    _handleClearParentTap: function(e) {
-      var clear = Polymer.dom(e).rootTarget.checked;
-      if (clear) {
-        this.base = '';
-      }
-      this.$.parentInput.disabled = clear;
-      this.clearParent = clear;
+    _handleRebaseOnOther: function(e) {
+      this.$.parentInput.focus();
    },

-    _updateValueSelected: function(base, clearParent) {
-      return base.length || clearParent;
+    /**
+     * There is a subtle but important difference between setting the base to an
+     * empty string and omitting it entirely from the payload. An empty string
+     * implies that the parent should be cleared and the change should be
+     * rebased on top of the target branch. Leaving out the base implies that it
+     * should be rebased on top of its current parent.
+     */
+    _handleRebaseOnTip: function(e) {
+      this.base = '';
+    },
+
+    _handleRebaseOnParent: function(e) {
+      this.base = null;
+    },
+
+    _handleEnterChangeNumberTap: function(e) {
+      this.$.rebaseOnOtherInput.checked = true;
+    },
+
+    /**
+     * Sets the default radio button based on the state of the app and
+     * the corresponding value to be submitted.
+     */
+    _updateSelectedOption: function(rebaseOnCurrent, hasParent) {
+      if (this._displayParentOption(rebaseOnCurrent, hasParent)) {
+        this.$.rebaseOnParent.checked = true;
+        this._handleRebaseOnParent();
+      } else if (this._displayTipOption(rebaseOnCurrent, hasParent)) {
+        this.$.rebaseOnTip.checked = true;
+        this._handleRebaseOnTip();
+      } else {
+        this.$.rebaseOnOtherInput.checked = true;
+        this._handleRebaseOnOther();
+      }
    },
  });
 })();
--- a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.html
+++ b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.html
@@ -38,47 +38,48 @@ limitations under the License.
      element = fixture('basic');
    });

-    test('controls with rebase on current available', function() {
+    test('controls with parent and rebase on current available', function() {
      element.rebaseOnCurrent = true;
+      element.hasParent = 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);
+      assert.isTrue(element.$.rebaseOnParent.checked);
+      assert.isFalse(element.$.rebaseOnParentContainer.hasAttribute('hidden'));
+      assert.isTrue(element.$.parentUpToDateMsg.hasAttribute('hidden'));
+      assert.isFalse(element.$.rebaseOnTip.hasAttribute('hidden'));
+      assert.isTrue(element.$.tipUpToDateMsg.hasAttribute('hidden'));
    });

-    test('controls without rebase on current available', function() {
+    test('controls with parent rebase on current not available', function() {
      element.rebaseOnCurrent = false;
+      element.hasParent = true;
      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.isTrue(element.$.rebaseOnTip.checked);
+      assert.isTrue(element.$.rebaseOnParentContainer.hasAttribute('hidden'));
+      assert.isFalse(element.$.parentUpToDateMsg.hasAttribute('hidden'));
+      assert.isFalse(element.$.rebaseOnTip.hasAttribute('hidden'));
+      assert.isTrue(element.$.tipUpToDateMsg.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'));
+    test('controls without parent and rebase on current available', function() {
+      element.rebaseOnCurrent = true;
+      element.hasParent = false;
+      flushAsynchronousOperations();
+      assert.isTrue(element.$.rebaseOnTip.checked);
+      assert.isTrue(element.$.rebaseOnParentContainer.hasAttribute('hidden'));
+      assert.isTrue(element.$.parentUpToDateMsg.hasAttribute('hidden'));
+      assert.isFalse(element.$.rebaseOnTip.hasAttribute('hidden'));
+      assert.isTrue(element.$.tipUpToDateMsg.hasAttribute('hidden'));
+    });

-      element.base = 'something great';
-      assert.isTrue(!!element.valueSelected);
+    test('controls without parent rebase on current not available', function() {
+      element.rebaseOnCurrent = false;
+      element.hasParent = false;
+      flushAsynchronousOperations();
+      assert.isTrue(element.$.rebaseOnOtherInput.checked);
+      assert.isTrue(element.$.rebaseOnParentContainer.hasAttribute('hidden'));
+      assert.isTrue(element.$.parentUpToDateMsg.hasAttribute('hidden'));
+      assert.isTrue(element.$.rebaseOnTip.hasAttribute('hidden'));
+      assert.isFalse(element.$.tipUpToDateMsg.hasAttribute('hidden'));
    });
  });
 </script>
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-change-actions-js-api_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-change-actions-js-api_test.html
@@ -50,6 +50,7 @@ breaking changes to gr-change-actions won’t be noticed.
    setup(function() {
      element = fixture('basic');
      element.change = {};
+      element._hasKnownChainState = false;
      var plugin;
      Gerrit.install(function(p) { plugin = p; }, '0.1',
          'http://test.com/plugins/testplugin/static/test.js');
@@ -110,7 +111,7 @@ breaking changes to gr-change-actions won’t be noticed.
        var button = element.$$('[data-action-key="' + key + '"]');
        assert.isOk(button);
        assert.equal(button.getAttribute('data-label'), 'Bork!');
-        assert.isFalse(button.disabled);
+        assert.isNotOk(button.disabled);
        changeActions.setLabel(key, 'Yo');
        changeActions.setEnabled(key, false);
        flush(function() {
--- 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
@@ -693,7 +693,7 @@
      return fetch(url, options).then(function(response) {
        if (!response.ok) {
          if (opt_errFn) {
-            opt_errFn.call(null, response);
+            opt_errFn.call(opt_ctx || null, response);
            return undefined;
          }
          this.fire('server-error', {response: response});