From 7588f648a14bd6ec3c2573ede5327b0277ecccf0 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 26 Jul 2016 17:34:05 +0200 Subject: [PATCH 1/5] Generate the checks list in JS --- apps/workflowengine/js/admin.js | 74 ++++++++++--------- .../js/usergroupmembershipplugin.js | 10 +++ apps/workflowengine/templates/admin.php | 2 +- 3 files changed, 50 insertions(+), 36 deletions(-) diff --git a/apps/workflowengine/js/admin.js b/apps/workflowengine/js/admin.js index f51352c45e..590274b877 100644 --- a/apps/workflowengine/js/admin.js +++ b/apps/workflowengine/js/admin.js @@ -28,11 +28,27 @@ }); Handlebars.registerHelper('getOperators', function(classname) { - return OCA.WorkflowEngine.availableChecks - .getOperatorsByClassName(classname); + var check = OCA.WorkflowEngine.getCheckByClass(classname); + if (!_.isUndefined(check)) { + return check['operators']; + } + return []; }); - OCA.WorkflowEngine = OCA.WorkflowEngine || {}; + OCA.WorkflowEngine = OCA.WorkflowEngine || { + availablePlugins: [], + availableChecks: [], + + getCheckByClass: function(className) { + var length = OCA.WorkflowEngine.availableChecks.length; + for (var i = 0; i < length; i++) { + if (OCA.WorkflowEngine.availableChecks[i]['class'] === className) { + return OCA.WorkflowEngine.availableChecks[i]; + } + } + return undefined; + } + }; /** * 888b d888 888 888 @@ -86,22 +102,6 @@ url: OC.generateUrl('apps/workflowengine/operations') }); - /** - * @class OCA.WorkflowEngine.AvailableChecksCollection - * - * collection for all available checks - */ - OCA.WorkflowEngine.AvailableChecksCollection = - OC.Backbone.Collection.extend({ - model: OCA.WorkflowEngine.AvailableCheck, - url: OC.generateUrl('apps/workflowengine/checks'), - getOperatorsByClassName: function(classname) { - return OCA.WorkflowEngine.availableChecks - .findWhere({'class': classname}) - .get('operators'); - } - }); - /** * 888 888 d8b * 888 888 Y8P @@ -154,7 +154,6 @@ message: '', errorMessage: '', saving: false, - plugins: [], initialize: function() { // this creates a new copy of the object to definitely have a new reference and being able to reset the model this.originalModel = JSON.parse(JSON.stringify(this.model)); @@ -167,13 +166,6 @@ if (this.model.get('id') === undefined) { this.hasChanged = true; } - - this.plugins = OC.Plugins.getPlugins('OCA.WorkflowEngine.CheckPlugins'); - _.each(this.plugins, function(plugin) { - if (_.isFunction(plugin.initialize)) { - plugin.initialize(); - } - }); }, delete: function() { this.model.destroy(); @@ -209,9 +201,8 @@ }, add: function() { var checks = _.clone(this.model.get('checks')), - classname = OCA.WorkflowEngine.availableChecks.at(0).get('class'), - operators = OCA.WorkflowEngine.availableChecks - .getOperatorsByClassName(classname); + classname = OCA.WorkflowEngine.availableChecks[0]['class'], + operators = OCA.WorkflowEngine.availableChecks[0]['operators']; checks.push({ 'class': classname, @@ -249,9 +240,10 @@ // if the class is changed most likely also the operators have changed // with this we set the operator to the first possible operator if (key === 'class') { - var operators = OCA.WorkflowEngine.availableChecks - .getOperatorsByClassName(value); - checks[id]['operator'] = operators[0]; + var check = OCA.WorkflowEngine.getCheckByClass(value); + if (!_.isUndefined(check)) { + checks[id]['operator'] = check['operators'][0]; + } } // model change will trigger render this.model.set({'checks': checks}); @@ -294,7 +286,7 @@ render: function() { this.$el.html(this.template({ operation: this.model.toJSON(), - classes: OCA.WorkflowEngine.availableChecks.toJSON(), + classes: OCA.WorkflowEngine.availableChecks, hasChanged: this.hasChanged, message: this.message, errorMessage: this.errorMessage, @@ -308,7 +300,7 @@ check = checks[id], valueElement = $element.find('.check-value').first(); - _.each(this.plugins, function(plugin) { + _.each(OCA.WorkflowEngine.availablePlugins, function(plugin) { if (_.isFunction(plugin.render)) { plugin.render(valueElement, check['class'], check['value']); } @@ -334,6 +326,8 @@ OCA.WorkflowEngine.OperationsView = OCA.WorkflowEngine.TemplateView.extend({ templateId: '#operations-template', + collection: null, + $el: null, events: { 'click .button-add-operation': 'add' }, @@ -341,6 +335,16 @@ this._initialize('OCA\\WorkflowEngine\\Operation'); }, _initialize: function(classname) { + OCA.WorkflowEngine.availablePlugins = OC.Plugins.getPlugins('OCA.WorkflowEngine.CheckPlugins'); + _.each(OCA.WorkflowEngine.availablePlugins, function(plugin) { + if (_.isFunction(plugin.initialize)) { + plugin.initialize(); + } + if (_.isFunction(plugin.getCheck)) { + OCA.WorkflowEngine.availableChecks.push(plugin.getCheck()); + } + }); + this.collection.fetch({data: { 'class': classname }}); diff --git a/apps/workflowengine/js/usergroupmembershipplugin.js b/apps/workflowengine/js/usergroupmembershipplugin.js index 2a6068cda9..528a7bd3e3 100644 --- a/apps/workflowengine/js/usergroupmembershipplugin.js +++ b/apps/workflowengine/js/usergroupmembershipplugin.js @@ -24,6 +24,16 @@ OCA.WorkflowEngine.Plugins = OCA.WorkflowEngine.Plugins || {}; OCA.WorkflowEngine.Plugins.UserGroupMembershipPlugin = { + getCheck: function() { + return { + 'class': 'OCA\\WorkflowEngine\\Check\\UserGroupMembership', + 'name': t('workflowengine', 'User group membership'), + 'operators': [ + {'operator': 'is', 'name': t('workflowengine', 'is member of')}, + {'operator': '!is', 'name': t('workflowengine', 'is not member of')} + ] + }; + }, render: function(element, classname, value) { if (classname !== 'OCA\\WorkflowEngine\\Check\\UserGroupMembership') { return; diff --git a/apps/workflowengine/templates/admin.php b/apps/workflowengine/templates/admin.php index 86ecfe0855..cb6895f80c 100644 --- a/apps/workflowengine/templates/admin.php +++ b/apps/workflowengine/templates/admin.php @@ -48,7 +48,7 @@ From 681eebcfe613157e05919a3223fcd0bbbe324638 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 27 Jul 2016 08:54:34 +0200 Subject: [PATCH 2/5] Remove php side of check registration --- apps/workflowengine/appinfo/routes.php | 1 - .../lib/AppInfo/Application.php | 12 --- .../lib/Controller/FlowOperations.php | 21 +---- apps/workflowengine/lib/Manager.php | 14 ++-- .../WorkflowEngine/RegisterCheckEvent.php | 79 ------------------- 5 files changed, 9 insertions(+), 118 deletions(-) delete mode 100644 lib/public/WorkflowEngine/RegisterCheckEvent.php diff --git a/apps/workflowengine/appinfo/routes.php b/apps/workflowengine/appinfo/routes.php index 69478b1715..b8c9ae1c23 100644 --- a/apps/workflowengine/appinfo/routes.php +++ b/apps/workflowengine/appinfo/routes.php @@ -21,7 +21,6 @@ return [ 'routes' => [ - ['name' => 'flowOperations#getChecks', 'url' => '/checks', 'verb' => 'GET'], // TODO rm and do via js? ['name' => 'flowOperations#getOperations', 'url' => '/operations', 'verb' => 'GET'], ['name' => 'flowOperations#addOperation', 'url' => '/operations', 'verb' => 'POST'], ['name' => 'flowOperations#updateOperation', 'url' => '/operations/{id}', 'verb' => 'PUT'], diff --git a/apps/workflowengine/lib/AppInfo/Application.php b/apps/workflowengine/lib/AppInfo/Application.php index c196ecd955..8433950304 100644 --- a/apps/workflowengine/lib/AppInfo/Application.php +++ b/apps/workflowengine/lib/AppInfo/Application.php @@ -37,18 +37,6 @@ class Application extends \OCP\AppFramework\App { */ public function registerHooksAndListeners() { $dispatcher = $this->getContainer()->getServer()->getEventDispatcher(); - $dispatcher->addListener( - 'OCP\WorkflowEngine\RegisterCheckEvent', - function(RegisterCheckEvent $event) { - $event->addCheck( - 'OCA\WorkflowEngine\Check\UserGroupMembership', - 'User group membership', - ['is', '!is'] - ); - }, - -100 - ); - $dispatcher->addListener( 'OCP\WorkflowEngine::loadAdditionalSettingScripts', function() { diff --git a/apps/workflowengine/lib/Controller/FlowOperations.php b/apps/workflowengine/lib/Controller/FlowOperations.php index e0836c727a..94b8b9ddc7 100644 --- a/apps/workflowengine/lib/Controller/FlowOperations.php +++ b/apps/workflowengine/lib/Controller/FlowOperations.php @@ -26,38 +26,19 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\JSONResponse; use OCP\IRequest; -use OCP\WorkflowEngine\RegisterCheckEvent; -use Symfony\Component\EventDispatcher\EventDispatcherInterface; class FlowOperations extends Controller { /** @var Manager */ protected $manager; - /** @var EventDispatcherInterface */ - protected $dispatcher; - /** * @param IRequest $request * @param Manager $manager - * @param EventDispatcherInterface $dispatcher */ - public function __construct(IRequest $request, Manager $manager, EventDispatcherInterface $dispatcher) { + public function __construct(IRequest $request, Manager $manager) { parent::__construct('workflowengine', $request); $this->manager = $manager; - $this->dispatcher = $dispatcher; - } - - /** - * @NoCSRFRequired - * - * @return JSONResponse - */ - public function getChecks() { - $event = new RegisterCheckEvent(); - $this->dispatcher->dispatch('OCP\WorkflowEngine\RegisterCheckEvent', $event); - - return new JSONResponse($event->getChecks()); } /** diff --git a/apps/workflowengine/lib/Manager.php b/apps/workflowengine/lib/Manager.php index 98c34e894c..f2a04dfb0f 100644 --- a/apps/workflowengine/lib/Manager.php +++ b/apps/workflowengine/lib/Manager.php @@ -110,8 +110,8 @@ class Manager implements IManager { $checkInstance->setFileInfo($this->storage, $this->path); return $checkInstance->executeCheck($check['operator'], $check['value']); } else { - // Check is invalid, assume it matches. - return true; + // Check is invalid + throw new \RuntimeException('Check ' . htmlspecialchars($check['class']) . ' is invalid or does not exist'); } } @@ -258,10 +258,12 @@ class Manager implements IManager { } $result->closeCursor(); - // TODO What if a check is missing? Should we throw? - // As long as we only allow AND-concatenation of checks, a missing check - // is like a matching check, so it evaluates to true and therefor blocks - // access. So better save than sorry. + $checkIds = array_diff($checkIds, array_keys($checks)); + + if (!empty($checkIds)) { + $missingCheck = array_pop($checkIds); + throw new \RuntimeException('Check #' . htmlspecialchars($missingCheck) . ' is invalid or does not exist'); + } return $checks; } diff --git a/lib/public/WorkflowEngine/RegisterCheckEvent.php b/lib/public/WorkflowEngine/RegisterCheckEvent.php deleted file mode 100644 index e08aae5fbc..0000000000 --- a/lib/public/WorkflowEngine/RegisterCheckEvent.php +++ /dev/null @@ -1,79 +0,0 @@ - - * - * @license GNU AGPL version 3 or any later version - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as - * published by the Free Software Foundation, either version 3 of the - * License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - * - */ - -namespace OCP\WorkflowEngine; - - -use Symfony\Component\EventDispatcher\Event; - -/** - * Class RegisterCheckEvent - * - * @package OCP\WorkflowEngine - * @since 9.1 - */ -class RegisterCheckEvent extends Event { - - /** @var array[] */ - protected $checks = []; - - /** - * @param string $class - * @param string $name - * @param string[] $operators - * @throws \OutOfBoundsException when the check class is already registered - * @throws \OutOfBoundsException when the provided information is invalid - * @since 9.1 - */ - public function addCheck($class, $name, array $operators) { - if (!is_string($class)) { - throw new \OutOfBoundsException('Given class name is not a string'); - } - - if (isset($this->checks[$class])) { - throw new \OutOfBoundsException('Duplicate check class "' . $class . '"'); - } - - if (!is_string($name)) { - throw new \OutOfBoundsException('Given check name is not a string'); - } - - foreach ($operators as $operator) { - if (!is_string($operator)) { - throw new \OutOfBoundsException('At least one of the operators is not a string'); - } - } - - $this->checks[$class] = [ - 'class' => $class, - 'name' => $name, - 'operators' => $operators, - ]; - } - - /** - * @return array[] - * @since 9.1 - */ - public function getChecks() { - return array_values($this->checks); - } -} From a3d1cd4ad224059d3bc512b5591356d384d70f4c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 27 Jul 2016 10:38:03 +0200 Subject: [PATCH 3/5] Fix morris comments --- apps/workflowengine/js/admin.js | 7 ++----- apps/workflowengine/templates/admin.php | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/apps/workflowengine/js/admin.js b/apps/workflowengine/js/admin.js index 590274b877..394c580fbb 100644 --- a/apps/workflowengine/js/admin.js +++ b/apps/workflowengine/js/admin.js @@ -35,7 +35,7 @@ return []; }); - OCA.WorkflowEngine = OCA.WorkflowEngine || { + OCA.WorkflowEngine = _.extend(OCA.WorkflowEngine || {}, { availablePlugins: [], availableChecks: [], @@ -48,7 +48,7 @@ } return undefined; } - }; + }); /** * 888b d888 888 888 @@ -337,9 +337,6 @@ _initialize: function(classname) { OCA.WorkflowEngine.availablePlugins = OC.Plugins.getPlugins('OCA.WorkflowEngine.CheckPlugins'); _.each(OCA.WorkflowEngine.availablePlugins, function(plugin) { - if (_.isFunction(plugin.initialize)) { - plugin.initialize(); - } if (_.isFunction(plugin.getCheck)) { OCA.WorkflowEngine.availableChecks.push(plugin.getCheck()); } diff --git a/apps/workflowengine/templates/admin.php b/apps/workflowengine/templates/admin.php index cb6895f80c..c876f30c4c 100644 --- a/apps/workflowengine/templates/admin.php +++ b/apps/workflowengine/templates/admin.php @@ -48,7 +48,7 @@ From d47b7bd4b221bcc88515da07bf796f58c5d09807 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 27 Jul 2016 12:29:30 +0200 Subject: [PATCH 4/5] Fix default value of operator --- apps/workflowengine/js/admin.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/workflowengine/js/admin.js b/apps/workflowengine/js/admin.js index 394c580fbb..48d1592b45 100644 --- a/apps/workflowengine/js/admin.js +++ b/apps/workflowengine/js/admin.js @@ -206,7 +206,7 @@ checks.push({ 'class': classname, - 'operator': operators[0], + 'operator': operators[0]['operator'], 'value': '' }); this.model.set({'checks': checks}); @@ -242,7 +242,7 @@ if (key === 'class') { var check = OCA.WorkflowEngine.getCheckByClass(value); if (!_.isUndefined(check)) { - checks[id]['operator'] = check['operators'][0]; + checks[id]['operator'] = check['operators'][0]['operator']; } } // model change will trigger render From 29a9cb7f1a75c39a90635346c3fe9db852b8dab8 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 27 Jul 2016 12:29:49 +0200 Subject: [PATCH 5/5] Allow to reuse the template --- apps/workflowengine/templates/admin.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/workflowengine/templates/admin.php b/apps/workflowengine/templates/admin.php index c876f30c4c..9e01f8359f 100644 --- a/apps/workflowengine/templates/admin.php +++ b/apps/workflowengine/templates/admin.php @@ -22,7 +22,7 @@ /** @var array $_ */ /** @var OC_L10N $l */ ?> -
+