From cf44a3802a4c85cf6d8b9cb37a81e12208f75fab Mon Sep 17 00:00:00 2001
From: Wyatt Allen <wyatta@google.com>
Date: Fri, 27 Jan 2017 13:53:58 -0800
Subject: [PATCH] Fix exception when deleting a change

Would previously throw an exception after attempting to set the loading
state on a change action that appears in the overflow menu (as opposed
to a top-level button). With this change it will skip setting the state
on actions that appear in the menu.

Added tests for the method and include notes for Issue 5366 to support
the loading state on menu actions.

Bug: Issue 5364
Change-Id: Ie7f5365249d00a93164377c0e59a716f80637554
---
 .../gr-change-actions/gr-change-actions.js    |  3 +++
 .../gr-change-actions_test.html               | 24 +++++++++++++++++++
 2 files changed, 27 insertions(+)

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 331c3c5af0..052aa23f04 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
@@ -562,6 +562,9 @@
     },
 
     _setLoadingOnButtonWithKey: function(key) {
+      // Return a NoOp for menu keys. @see Issue 5366
+      if (MENU_ACTION_KEYS.indexOf(key) !== -1) { return function() {}; }
+
       var buttonEl = this.$$('[data-action-key="' + key + '"]');
       buttonEl.setAttribute('loading', true);
       buttonEl.disabled = true;
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 dbdf00fa2e..cf3ab19aba 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
@@ -378,6 +378,30 @@ limitations under the License.
       });
     });
 
+    test('_setLoadingOnButtonWithKey top-level', function() {
+      var key = 'rebase';
+      var cleanup = element._setLoadingOnButtonWithKey(key);
+
+      var button = element.$$('[data-action-key="' + key + '"]');
+      assert.isTrue(button.hasAttribute('loading'));
+      assert.isTrue(button.disabled);
+
+      assert.isOk(cleanup);
+      assert.isFunction(cleanup);
+      cleanup();
+
+      assert.isFalse(button.hasAttribute('loading'));
+      assert.isFalse(button.disabled);
+    });
+
+    test('_setLoadingOnButtonWithKey overflow menu', function() {
+      // TODO(wyatta): Should not throw error when setting loading on an
+      // overflow action. @see Issue 5366
+      var key = 'cherrypick';
+      var cleanup = element._setLoadingOnButtonWithKey(key);
+      assert.isFunction(cleanup);
+    });
+
     suite('revert change', function() {
       var alertStub;
       var fireActionStub;