Re-organize change actions into an array

The gr-change-actions element organizes change actions into a single,
sorted array property for rendering. Formerly, the order was partially
established by keeping the actions in separate arrays and rendering
them in serial loops, whereas, with this change, the rendering and order
is simplified and less redundant.

In particular, the `_computeChangeActionValues` and
`_computeRevisionActionValues` methods are merged into a new
`_computeAllActions` method.

Change-Id: I4ff3d1458fe845e0a5932e4aeb6b91dcbb4c20ee

polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.html

@@ -63,29 +63,19 @@ limitations under the License.
       }
     </style>
     <div>
-      <section hidden$="[[_shouldHideActions(actions.*, _additionalActions.*, _loading)]]">
-        <template is="dom-repeat" items="[[_changeActionValues]]" as="action">
+      <section hidden$="[[_shouldHideActions(_allActionValues.*, _loading)]]">
+        <template
+            is="dom-repeat"
+            items="[[_allActionValues]]"
+            as="action">
           <gr-button title$="[[action.title]]"
-              hidden$="[[_computeActionHidden(action.__key, _hiddenChangeActions.*)]]"
+              hidden$="[[_computeActionHidden(action.__key, _hiddenActions.*)]]"
               primary$="[[action.__primary]]"
-              hidden$="[[!action.enabled]]"
               data-action-key$="[[action.__key]]"
               data-action-type$="[[action.__type]]"
               data-label$="[[action.label]]"
               data-loading-label$="[[_computeLoadingLabel(action.__key)]]"
-              on-tap="_handleActionTap"></gr-button>
-        </template>
-      </section>
-      <section hidden$="[[_shouldHideActions(revisionActions.*, _additionalActions.*, _loading)]]">
-        <template is="dom-repeat" items="[[_revisionActionValues]]" as="action">
-          <gr-button title$="[[action.title]]"
-              hidden$="[[_computeActionHidden(action.__key, _hiddenRevisionActions.*)]]"
-              primary$="[[action.__primary]]"
               disabled$="[[!action.enabled]]"
-              data-action-key$="[[action.__key]]"
-              data-action-type$="[[action.__type]]"
-              data-label$="[[action.label]]"
-              data-loading-label$="[[_computeLoadingLabel(action.__key)]]"
               on-tap="_handleActionTap"></gr-button>
         </template>
       </section>

polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js

@@ -96,6 +96,12 @@
      * @event reload-change
      */
 
+    /**
+     * Fired when an action is tapped.
+     *
+     * @event <action key>-tap
+     */
+
     properties: {
       change: Object,
       actions: {
@@ -129,25 +135,17 @@
         type: Boolean,
         value: true,
       },
-      _revisionActionValues: {
+
+      _allActionValues: {
         type: Array,
-        computed: '_computeRevisionActionValues(revisionActions.*, ' +
-            'primaryActionKeys.*, _additionalActions.*)',
-      },
-      _changeActionValues: {
-        type: Array,
-        computed: '_computeChangeActionValues(actions.*, ' +
+        computed: '_computeAllActions(actions.*, revisionActions.*,' +
             'primaryActionKeys.*, _additionalActions.*, change)',
       },
       _additionalActions: {
         type: Array,
         value: function() { return []; },
       },
-      _hiddenChangeActions: {
-        type: Array,
-        value: function() { return []; },
-      },
-      _hiddenRevisionActions: {
+      _hiddenActions: {
         type: Array,
         value: function() { return []; },
       },
@@ -220,20 +218,15 @@
     },
 
     setActionHidden: function(type, key, hidden) {
-      var path;
-      if (type === ActionType.CHANGE) {
-        path = '_hiddenChangeActions';
-      } else if (type === ActionType.REVISION) {
-        path = '_hiddenRevisionActions';
-      } else {
+      if (type !== ActionType.CHANGE && type !== ActionType.REVISION) {
         throw Error('Invalid action type given: ' + type);
       }
 
-      var idx = this.get(path).indexOf(key);
+      var idx = this._hiddenActions.indexOf(key);
       if (hidden && idx === -1) {
-        this.push(path, key);
+        this.push('_hiddenActions', key);
       } else if (!hidden && idx !== -1) {
-        this.splice(path, idx, 1);
+        this.splice('_hiddenActions', idx, 1);
       }
     },
 
@@ -251,16 +244,8 @@
           this.patchNum);
     },
 
-    _shouldHideActions: function(actionsRecord, additionalActionsRecord,
-        loading) {
-      if (loading) { return true; }
-      return !this._actionCount(actionsRecord, additionalActionsRecord);
-    },
-
-    _actionCount: function(actionsRecord, additionalActionsRecord) {
-      var additionalActions = (additionalActionsRecord &&
-          additionalActionsRecord.base) || [];
-      return this._keyCount(actionsRecord) + additionalActions.length;
+    _shouldHideActions: function(actions, loading) {
+      return loading || !actions || !actions.base || !actions.base.length;
     },
 
     _keyCount: function(changeRecord) {
@@ -282,24 +267,6 @@
       });
     },
 
-    _computeRevisionActionValues: function(actionsChangeRecord,
-        primariesChangeRecord, additionalActionsChangeRecord) {
-      return this._getActionValues(actionsChangeRecord, primariesChangeRecord,
-          additionalActionsChangeRecord, ActionType.REVISION);
-    },
-
-    _computeChangeActionValues: function(actionsChangeRecord,
-        primariesChangeRecord, additionalActionsChangeRecord, change) {
-      var actions = this._getActionValues(
-          actionsChangeRecord, primariesChangeRecord,
-          additionalActionsChangeRecord, ActionType.CHANGE, change);
-      var quickApprove = this._getQuickApproveAction();
-      if (quickApprove) {
-        actions.unshift(quickApprove);
-      }
-      return actions;
-    },
-
     _getLabelStatus: function(label) {
       if (label.approved) {
         return LabelStatus.OK;
@@ -473,7 +440,7 @@
       } else if (key === ChangeActions.ABANDON) {
         this._showActionDialog(this.$.confirmAbandonDialog);
       } else if (key === QUICK_APPROVE_ACTION.key) {
-        var action = this._changeActionValues.find(function(o) {
+        var action = this._allActionValues.find(function(o) {
           return o.key === key;
         });
         this._fireAction(
@@ -664,5 +631,59 @@
         return response;
       }.bind(this)).then(this._handleResponseError.bind(this));
     },
+
+    /**
+     * Merge sources of change actions into a single ordered array of action
+     * values.
+     * @param {splices} changeActionsRecord
+     * @param {splices} revisionActionsRecord
+     * @param {splices} primariesRecord
+     * @param {splices} additionalActionsRecord
+     * @param {Object} change The change object.
+     * @return {Array}
+     */
+    _computeAllActions: function(changeActionsRecord, revisionActionsRecord,
+        primariesRecord, additionalActionsRecord, change) {
+      var revisionActionValues = this._getActionValues(revisionActionsRecord,
+          primariesRecord, additionalActionsRecord, ActionType.REVISION);
+      var changeActionValues = this._getActionValues(changeActionsRecord,
+          primariesRecord, additionalActionsRecord, ActionType.CHANGE, change);
+      var quickApprove = this._getQuickApproveAction();
+      if (quickApprove) {
+        changeActionValues.unshift(quickApprove);
+      }
+      return revisionActionValues
+          .concat(changeActionValues)
+          .sort(this._actionComparator);
+    },
+
+    /**
+     * Sort comparator to define the order of change actions.
+     */
+    _actionComparator: function(actionA, actionB) {
+      // The code review action always appears first.
+      if (actionA.__key === 'review') {
+        return -1;
+      } else if (actionB.__key === 'review') {
+        return 1;
+      }
+
+      // Primary actions always appear last.
+      if (actionA.__primary) {
+        return 1;
+      } else if (actionB.__primary) {
+        return -1;
+      }
+
+      // Change actions appear before revision actions.
+      if (actionA.__type === 'change' && actionB.__type === 'revision') {
+        return -1;
+      } else if (actionA.__type === 'revision' && actionB.__type === 'change') {
+        return 1;
+      }
+
+      // Otherwise, sort by the button label.
+      return actionA.label > actionB.label ? 1 : -1;
+    },
   });
 })();

polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.html

@@ -99,11 +99,9 @@ limitations under the License.
     });
 
     test('_shouldHideActions', function() {
-      assert.isTrue(element._shouldHideActions(undefined, undefined, true));
-      assert.isTrue(element._shouldHideActions({base: {}},
-          {base: []}, false));
-      assert.isFalse(element._shouldHideActions({base: { test: 'test' }},
-          {base: ['test']}, false));
+      assert.isTrue(element._shouldHideActions(undefined, true));
+      assert.isTrue(element._shouldHideActions({base: {}}, false));
+      assert.isFalse(element._shouldHideActions({base: ['test']}, false));
     });
 
     test('hide revision action', function(done) {
@@ -114,10 +112,10 @@ limitations under the License.
         assert.throws(element.setActionHidden.bind(element, 'invalid type'));
         element.setActionHidden(element.ActionType.REVISION,
             element.RevisionActions.SUBMIT, true);
-        assert.lengthOf(element._hiddenRevisionActions, 1);
+        assert.lengthOf(element._hiddenActions, 1);
         element.setActionHidden(element.ActionType.REVISION,
             element.RevisionActions.SUBMIT, true);
-        assert.lengthOf(element._hiddenRevisionActions, 1);
+        assert.lengthOf(element._hiddenActions, 1);
         flush(function() {
           var buttonEl = element.$$('[data-action-key="submit"]');
           assert.isOk(buttonEl);
@@ -143,10 +141,10 @@ limitations under the License.
         assert.throws(element.setActionHidden.bind(element, 'invalid type'));
         element.setActionHidden(element.ActionType.CHANGE,
             element.ChangeActions.DELETE, true);
-        assert.lengthOf(element._hiddenChangeActions, 1);
+        assert.lengthOf(element._hiddenActions, 1);
         element.setActionHidden(element.ActionType.CHANGE,
             element.ChangeActions.DELETE, true);
-        assert.lengthOf(element._hiddenChangeActions, 1);
+        assert.lengthOf(element._hiddenActions, 1);
         flush(function() {
           var buttonEl = element.$$('[data-action-key="/"]');
           assert.isOk(buttonEl);
@@ -205,6 +203,18 @@ limitations under the License.
       assert.deepEqual(element._getRevision(change, '2'), revObj);
     });
 
+    test('_actionComparator sort order', function() {
+      var actions = [
+        {label: '123', type: 'change', __key: 'review'},
+        {label: 'abc',type: 'revision'},
+        {label: 'abc',type: 'change'},
+        {label: 'def', type: 'change'},
+        {label: 'def', type: 'change',__primary: true},
+      ];
+      var result = actions.slice().sort(element._actionComparator);
+      assert.deepEqual(result, actions);
+    });
+
     test('submit change', function(done) {
       element.change = {
         revisions: {

polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-change-actions-js-api_test.html

@@ -49,6 +49,7 @@ breaking changes to gr-change-actions won’t be noticed.
 
     setup(function() {
       element = fixture('basic');
+      element.change = {};
       var plugin;
       Gerrit.install(function(p) { plugin = p; }, '0.1',
           'http://test.com/plugins/testplugin/static/test.js');