From 8db1f0e927a2f906cf3b278dee6a978642844ebe Mon Sep 17 00:00:00 2001
From: Rajat Vig <rajatv@thoughtworks.com>
Date: Mon, 7 Dec 2015 09:06:07 -0800
Subject: [PATCH] Allow specifying item to use for actions in the actions
 directive

Currently, the item passed to the 'allowed-actions' is aware of
what the HTML referred to. This patch allows for passing in the
item to use in the actions directive. This removes
the need to pass in the item key as part of the
allowed actions list thus removing the implicit binding
of the controller/service/html.

This also changes the list type attribute for the directive
from the action-list-type to type for readability.

Co-Authored-By: Kyle Olivo<keolivo@thoughtworks.com>

Partially-Implements: blueprint angularize-images-table
Change-Id: Idc344153aa7582c485a5d9c6b3cccbf8ff788d7c
---
 .../action-list/actions.batch.mock.html       |   2 +-
 .../widgets/action-list/actions.directive.js  | 158 ++++++++++++------
 .../action-list/actions.directive.spec.js     | 126 +++++++-------
 .../widgets/action-list/actions.row.mock.html |   2 +-
 .../widgets/action-list/actions.service.js    |  10 +-
 5 files changed, 177 insertions(+), 121 deletions(-)

diff --git a/horizon/static/framework/widgets/action-list/actions.batch.mock.html b/horizon/static/framework/widgets/action-list/actions.batch.mock.html
index b2e7295cd8..b42918b3b3 100644
--- a/horizon/static/framework/widgets/action-list/actions.batch.mock.html
+++ b/horizon/static/framework/widgets/action-list/actions.batch.mock.html
@@ -1,2 +1,2 @@
-<actions allowed-actions="actions" action-list-type="batch">
+<actions allowed="actions" type="batch">
 </actions>
diff --git a/horizon/static/framework/widgets/action-list/actions.directive.js b/horizon/static/framework/widgets/action-list/actions.directive.js
index 566d5cc51e..326576100c 100644
--- a/horizon/static/framework/widgets/action-list/actions.directive.js
+++ b/horizon/static/framework/widgets/action-list/actions.directive.js
@@ -35,71 +35,111 @@
    *
    * Attributes:
    *
-   * allowedActions: actions allowed that can be displayed
-   * actionListType: allow the buttons to be shown as a list or doropdown
+   * @param {string} type
+   * Type can be only be 'row' or 'batch'.
+   * 'batch' actions are rendered as a button group, 'row' is rendered as a button dropdown menu.
+   * 'batch' actions are typically used for actions across multiple items while
+   * 'row' actions are used per item.
    *
-   * `allowedActions` is a list of allowed actions on the service.
-   * It's an array of objects of the form:
-   * { template: {}, permissions: <promise to determine permissions>, callback: 'callback'}
+   * @param {string=} item
+   * The item to pass to the callback when using 'row' type.
+   * The variable is evaluated and passed as an argument when evaluating 'allowed'.
+   * 'item' is not used when row type is 'batch'.
    *
-   * `template` is an object that can be
+   * @param {function} allowed
+   * Returns an array of actions that can be performed on the item(s).
+   * When using 'row' type, the current 'item' will be passed to the function.
+   * When using 'batch' type, no arguments are provided.
    *
-   * {url: 'template.html'} the location of the template for the action button.
-   * Use this for complete extensibility and control over what is rendered.
-   * The template will be responsible for binding the callback and styling.
+   * This is an array that should contain objects with the following properties:
+   * {
+   *   template: <template object - described below>,
+   *   permissions: <a promise to determine if action is allowed>,
+   *   callback: 'callback for the action'
+   * }
    *
-   * {type: 'type', item: 'item'} use a known action button type.
-   * Currently supported values are 'delete', 'delete-selected' and 'create'.
-   * `item` is optional and if provided will be used in the callback.
-   * The styling and binding of the callback is done by the template.
+   *   template: is an object that can be any of
+   *   1. url: <full_path_to_template.html>
+   *      This specifies the location of the template for the action button.
+   *      Use this for complete extensibility and control over what is rendered.
+   *      The template will be responsible for binding the callback and styling the button.
    *
-   * {text: 'text', item: 'item'} use an unstyled button with given text.
-   * `item` is optional and if provided will be used in the callback.
-   * The styling of the button will vary per the `actionListType` chosen.
-   * For custom styling of the button, `actionClasses` can be included in
-   * the template.
+   *   2. type: '<action_button_type>'
+   *      This uses a known action button type.
+   *      Currently supported values are
+   *      1. 'delete' - Delete a single row. Only for 'row' type.
+   *      2. 'delete-selected' - Delete multiple rows. Only for 'batch' type.
+   *      3. 'create' - Create a new entity. Only for 'batch' type.
    *
-   * `permissions` is expected to be a promise that resolves
-   * if permitted and is rejected if not.
-   * `callback` is the method to call when the button is clicked.
+   *      The styling and binding of the callback is done by the template.
    *
-   * `actionListType` can be only be `row` or `batch`
-   * `batch` is rendered as buttons, `row` is rendered as dropdown menu.
+   *   3. text: 'text', actionClasses: 'custom-classes'
+   *      This creates a button with the given text.
+   *      For custom styling of the button, `actionClasses` can be optionally included.
+   *
+   *   permissions: is expected to be a promise that resolves
+   *   if the action is permitted and is rejected if not. If there are multiple promises that
+   *   need to be resolved, you can $q.all to combine multiple promises into a single promise.
+   *
+   *   callback: is the method to call when the button is clicked.
+   *   When using 'row' type, the current 'item' is evaluated and passed to the function.
+   *   When using 'batch' type, 'item' is not passed.
+   *   When using 'delete-selected' for 'batch' type, all selected rows are passed.
    *
    * @restrict E
    * @scope
    * @example
    *
-   * $scope.actions = [{
-   *   template: {
-   *     text: gettext('Delete Image'),
-   *     type: 'delete',
-   *     item: 'image'
-   *   },
-   *   permissions: policy.ifAllowed({ rules: [['image', 'delete_image']] }),
-   *   callback: deleteModalService.open
-   *  }, {
-   *   template: {
-   *     text: gettext('Create Volume'),
-   *     item: 'image'
-   *   },
-   *   permissions: policy.ifAllowed({rules: [['volume', 'volume:create']]}),
-   *   callback: createVolumeModalService.open
-   *  }, {
-   *   template: {
-   *     url: basePath + 'actions/my-custom-action.html'
-   *   },
-   *   permissions: policy.ifAllowed({ rules: [['image', 'custom']] }),
-   *   callback: customModalService.open
-   * }]
+   * batch:
+   *
+   * function actions() {
+   *   return [{
+   *     callback: 'table.batchActions.delete.open',
+   *     template: {
+   *       type: 'delete-selected',
+   *       text: gettext('Delete Images')
+   *     },
+   *     permissions: policy.ifAllowed({ rules: [['image', 'delete_image']] })
+   *     }, {
+   *     callback: 'table.batchActions.create.open',
+   *     template: {
+   *       type: 'create',
+   *       text: gettext('Create Image')
+   *     },
+   *     permissions: policy.ifAllowed({ rules: [['image', 'add_image']] })
+   *   }];
+   * }
    *
    * ```
-   * <actions allowed-actions="actions" action-list-type="row">
-   * </actions>
-   *
-   * <actions allowed-actions="actions" action-list-type="batch">
+   * <actions allowed="actions" type="batch">
    * </actions>
    * ```
+   *
+   * row:
+   *
+   * function actions(image) {
+   *   return [{
+   *     callback: 'table.rowActions.deleteImage.open',
+   *     template: {
+   *       text: gettext('Delete Image'),
+   *       type: 'delete'
+   *     },
+   *     permissions: imageDeletePermitted(image)
+   *   }, {
+   *     callback: 'table.rowActions.createVolume.open',
+   *     template: {
+   *       text: gettext('Create Volume')
+   *     },
+   *     permissions: createVolumeFromImagePermitted(image)
+   *   }];
+   * }
+   *
+   * ```
+   * <actions allowed="actions" type="row" item="image">
+   * </actions>
+   *
+   * ```
+   *
    */
   function actions(
     $parse,
@@ -114,11 +154,21 @@
     return directive;
 
     function link(scope, element, attrs) {
-      var listType = attrs.actionListType;
-      var allowedActions = $parse(attrs.allowedActions)(scope);
-
-      actionsService({scope: scope, element: element, listType: listType})
-        .renderActions(allowedActions);
+      var listType = attrs.type;
+      var item = attrs.item;
+      var service = actionsService({
+        scope: scope,
+        element: element,
+        listType: listType,
+        item: item
+      });
+      var allowedActions = $parse(attrs.allowed)(scope);
+      if (listType === 'row') {
+        var itemVal = $parse(item)(scope);
+        service.renderActions(allowedActions(itemVal));
+      } else {
+        service.renderActions(allowedActions());
+      }
     }
   }
 })();
diff --git a/horizon/static/framework/widgets/action-list/actions.directive.spec.js b/horizon/static/framework/widgets/action-list/actions.directive.spec.js
index f0a6c2a487..bdafcde48d 100644
--- a/horizon/static/framework/widgets/action-list/actions.directive.spec.js
+++ b/horizon/static/framework/widgets/action-list/actions.directive.spec.js
@@ -17,6 +17,8 @@
   describe('actions directive', function () {
     var $scope, $compile, $q, $templateCache, basePath;
 
+    var rowItem = {id: 1};
+
     beforeEach(module('templates'));
     beforeEach(module('horizon.framework'));
 
@@ -29,18 +31,12 @@
     }));
 
     it('should have no buttons if there are no actions', function () {
-      var element = angular.element(getTemplate('actions.batch'));
-      $scope.actions = [];
-      $compile(element)($scope);
-      $scope.$apply();
+      var element = batchElementFor([]);
       expect(element.children().length).toBe(0);
     });
 
     it('should allow for specifying action text', function () {
-      var element = angular.element(getTemplate('actions.batch'));
-      $scope.actions = [permittedActionWithText('Create Image', 'image')];
-      $compile(element)($scope);
-      $scope.$apply();
+      var element = batchElementFor([permittedActionWithText('Create Image')]);
 
       expect(element.children().length).toBe(1);
       var actionList = element.find('action-list');
@@ -52,10 +48,7 @@
     });
 
     it('should allow for specifying by template for create', function () {
-      var element = angular.element(getTemplate('actions.batch'));
-      $scope.actions = [permittedActionWithType('create', 'Create Image')];
-      $compile(element)($scope);
-      $scope.$apply();
+      var element = batchElementFor([permittedActionWithType('create', 'Create Image')]);
 
       expect(element.children().length).toBe(1);
       var actionList = element.find('action-list');
@@ -67,10 +60,7 @@
     });
 
     it('should allow for specifying by template for delete-selected', function () {
-      var element = angular.element(getTemplate('actions.batch'));
-      $scope.actions = [permittedActionWithType('delete-selected', 'Delete Images')];
-      $compile(element)($scope);
-      $scope.$apply();
+      var element = batchElementFor([permittedActionWithType('delete-selected', 'Delete Images')]);
 
       expect(element.children().length).toBe(1);
       var actionList = element.find('action-list');
@@ -83,10 +73,12 @@
     });
 
     it('should allow for specifying by template for delete', function () {
-      var element = angular.element(getTemplate('actions.row'));
-      $scope.actions = [permittedActionWithType('delete', 'Delete Image')];
-      $compile(element)($scope);
-      $scope.$apply();
+      $scope.callback = function(item) {
+        expect(item).toEqual(rowItem);
+      };
+      spyOn($scope, 'callback').and.callThrough();
+
+      var element = rowElementFor([permittedActionWithType('delete', 'Delete Image')]);
 
       expect(element.children().length).toBe(1);
       var actionList = element.find('action-list');
@@ -95,15 +87,14 @@
       expect(actionList.find('button').attr('class')).toEqual('text-danger');
       expect(actionList.find('button').attr('ng-click')).toEqual('disabled || callback(item)');
       expect(actionList.text().trim()).toEqual('Delete Image');
+
+      actionList.find('button').click();
+      expect($scope.callback).toHaveBeenCalled();
     });
 
     it('should have one button if there is one action', function () {
       var action = getTemplatePath('action-create', getTemplate());
-
-      var element = angular.element(getTemplate('actions.batch'));
-      $scope.actions = [permittedActionWithUrl(action)];
-      $compile(element)($scope);
-      $scope.$apply();
+      var element = batchElementFor([permittedActionWithUrl(action)]);
 
       expect(element.children().length).toBe(1);
       var actionList = element.find('action-list');
@@ -115,10 +106,7 @@
     });
 
     it('should have no buttons if not permitted', function () {
-      var element = angular.element(getTemplate('actions.batch'));
-      $scope.actions = [notPermittedAction()];
-      $compile(element)($scope);
-      $scope.$apply();
+      var element = batchElementFor([notPermittedAction()]);
 
       expect(element.children().length).toBe(0);
     });
@@ -126,11 +114,10 @@
     it('should have multiple buttons for multiple actions as a list', function () {
       var action1 = getTemplatePath('action-create');
       var action2 = getTemplatePath('action-delete');
-
-      var element = angular.element(getTemplate('actions.batch'));
-      $scope.actions = [permittedActionWithUrl(action1), permittedActionWithUrl(action2)];
-      $compile(element)($scope);
-      $scope.$apply();
+      var element = batchElementFor([
+        permittedActionWithUrl(action1),
+        permittedActionWithUrl(action2)
+      ]);
 
       expect(element.children().length).toBe(2);
       var actionList = element.find('action-list');
@@ -142,11 +129,10 @@
 
     it('should have as many buttons as permitted', function () {
       var actionTemplate1 = getTemplatePath('action-create');
-
-      var element = angular.element(getTemplate('actions.batch'));
-      $scope.actions = [permittedActionWithUrl(actionTemplate1), notPermittedAction()];
-      $compile(element)($scope);
-      $scope.$apply();
+      var element = batchElementFor([
+        permittedActionWithUrl(actionTemplate1),
+        notPermittedAction()
+      ]);
 
       expect(element.children().length).toBe(1);
       var actionList = element.find('action-list');
@@ -159,10 +145,10 @@
       var action1 = getTemplatePath('action-create');
       var action2 = getTemplatePath('action-delete');
 
-      var element = angular.element(getTemplate('actions.row'));
-      $scope.actions = [permittedActionWithUrl(action1), permittedActionWithUrl(action2)];
-      $compile(element)($scope);
-      $scope.$apply();
+      var element = rowElementFor([
+        permittedActionWithUrl(action1),
+        permittedActionWithUrl(action2)
+      ]);
 
       expect(element.children().length).toBe(1);
       var actionList = element.find('action-list');
@@ -173,13 +159,10 @@
     });
 
     it('should have multiple buttons as a dropdown for actions text', function () {
-      var element = angular.element(getTemplate('actions.row'));
-      $scope.actions = [
-        permittedActionWithText('Create Image', 'image'),
-        permittedActionWithText('Delete Image', 'image', 'text-danger')
-      ];
-      $compile(element)($scope);
-      $scope.$apply();
+      var element = rowElementFor([
+        permittedActionWithText('Create Image'),
+        permittedActionWithText('Delete Image', 'text-danger')
+      ]);
 
       expect(element.children().length).toBe(1);
       var actionList = element.find('action-list');
@@ -190,13 +173,10 @@
     });
 
     it('should have one button if only one permitted for dropdown', function () {
-      var element = angular.element(getTemplate('actions.row'));
-      $scope.actions = [
+      var element = rowElementFor([
         permittedActionWithUrl(getTemplatePath('action-create')),
         notPermittedAction()
-      ];
-      $compile(element)($scope);
-      $scope.$apply();
+      ]);
 
       expect(element.children().length).toBe(1);
       var actionList = element.find('action-list');
@@ -213,11 +193,10 @@
       };
     }
 
-    function permittedActionWithText(text, item, actionClasses) {
+    function permittedActionWithText(text, actionClasses) {
       return {
         template: {
           text: text,
-          item: item,
           actionClasses: actionClasses
         },
         permissions: getPermission(true),
@@ -225,12 +204,11 @@
       };
     }
 
-    function permittedActionWithType(templateType, text, item) {
+    function permittedActionWithType(templateType, text) {
       return {
         template: {
           type: templateType,
-          text: text,
-          item: item
+          text: text
         },
         permissions: getPermission(true),
         callback: 'callback'
@@ -261,5 +239,33 @@
       return deferred.promise;
     }
 
+    function batchElementFor(actions) {
+      $scope.actions = function() {
+        return actions;
+      };
+
+      var element = angular.element(getTemplate('actions.batch'));
+
+      $compile(element)($scope);
+      $scope.$apply();
+
+      return element;
+    }
+
+    function rowElementFor(actions) {
+      $scope.rowItem = rowItem;
+      $scope.actions = function(item) {
+        expect(item).toEqual(rowItem);
+        return actions;
+      };
+
+      var element = angular.element(getTemplate('actions.row'));
+
+      $compile(element)($scope);
+      $scope.$apply();
+
+      return element;
+    }
+
   });
 })();
diff --git a/horizon/static/framework/widgets/action-list/actions.row.mock.html b/horizon/static/framework/widgets/action-list/actions.row.mock.html
index a4366a5542..4faf047873 100644
--- a/horizon/static/framework/widgets/action-list/actions.row.mock.html
+++ b/horizon/static/framework/widgets/action-list/actions.row.mock.html
@@ -1,2 +1,2 @@
-<actions allowed-actions="actions" action-list-type="row">
+<actions allowed="actions" type="row" item="rowItem">
 </actions>
diff --git a/horizon/static/framework/widgets/action-list/actions.service.js b/horizon/static/framework/widgets/action-list/actions.service.js
index 38dbead355..529f0b3e9c 100644
--- a/horizon/static/framework/widgets/action-list/actions.service.js
+++ b/horizon/static/framework/widgets/action-list/actions.service.js
@@ -30,12 +30,12 @@
 
   function actionsService($compile, $http, $q, $templateCache, basePath, $qExtensions) {
     return function(spec) {
-      return createService(spec.scope, spec.element, spec.listType);
+      return createService(spec.scope, spec.element, spec.listType, spec.item);
     };
 
     ///////////////
 
-    function createService(scope, element, listType) {
+    function createService(scope, element, listType, item) {
       var service = {
         renderActions: renderActions
       };
@@ -157,9 +157,9 @@
       /**
        * Fetch the HTML Template for the Action
        */
-      function getTemplate(permittedActionResponse) {
+      function getTemplate(permittedAction) {
         var defered = $q.defer();
-        var action = permittedActionResponse.context;
+        var action = permittedAction.context;
         $http.get(getTemplateUrl(action), {cache: $templateCache}).then(onTemplateGet);
         return defered.promise;
 
@@ -167,7 +167,7 @@
           var template = response.data
                 .replace('$action-classes$', action.template.actionClasses || '')
                 .replace('$text$', action.template.text)
-                .replace('$item$', action.template.item);
+                .replace('$item$', item);
           defered.resolve({template: template, callback: action.callback});
         }
       }