From 65eaf9300b0ebddcc764f828eb3b81ba30c00189 Mon Sep 17 00:00:00 2001 From: Brandon Keepers Date: Tue, 22 Nov 2016 22:44:24 -0600 Subject: [PATCH] Reimplement workflow with new plugin system --- lib/dispatcher.js | 23 +------- lib/plugins/issues.js | 116 +++++++++-------------------------------- lib/sandbox.js | 2 +- lib/workflow.js | 55 +++++++++++++------ scratchpad.js | 2 +- test/plugins/issues.js | 77 +++++++++++---------------- 6 files changed, 99 insertions(+), 176 deletions(-) diff --git a/lib/dispatcher.js b/lib/dispatcher.js index 98049fa8..290d3552 100644 --- a/lib/dispatcher.js +++ b/lib/dispatcher.js @@ -1,5 +1,4 @@ const Context = require('./context'); -const issues = require('./plugins/issues'); class Dispatcher { constructor(github, event) { @@ -11,28 +10,8 @@ class Dispatcher { // Get behaviors for the event const context = new Context(this.github, config, this.event); - // FIXME: have a better method to register evaluators - const evaluators = [ - issues.Evaluator - ]; - // Handle all behaviors - return Promise.all(config.workflows.map(w => { - if (w.matches(this.event)) { - return evaluators.map(E => { - const evaluator = new E(); - if (evaluator.checkIfEventApplies(context.event)) { - return evaluator.evaluate(w, context); - } else { - return []; - } - }); - } else { - return false; - } - }).reduce((a, b) => { - return a.concat(b); - }, [])); + return Promise.all(config.workflows.map(w => w.execute(context))); } } diff --git a/lib/plugins/issues.js b/lib/plugins/issues.js index 8d2ec437..fa209c5a 100644 --- a/lib/plugins/issues.js +++ b/lib/plugins/issues.js @@ -1,114 +1,48 @@ const handlebars = require('handlebars'); -const Evaluator = require('../evaluator'); -const IssuePlugin = superclass => class extends superclass { - comment(comment) { - this._setCommentData({comment}); - return this; +module.exports = class IssuesPlugin { + // checkIfEventApplies(event) { + // return event.issue !== undefined || event.pull_request !== undefined; + // } + + comment(context, content) { + const template = handlebars.compile(content)(context.payload); + return context.github.issues.createComment(context.payload.toIssue({body: template})); } - assign(...assignees) { - this._setCommentData({assign: assignees}); - return this; + assign(context, ...assignees) { + return context.github.issues.addAssigneesToIssue(context.payload.toIssue({assignees})); } - unassign(...assignees) { - this._setCommentData({unassign: assignees}); - return this; + unassign(context, ...assignees) { + return context.github.issues.removeAssigneesFromIssue(context.payload.toIssue({body: {assignees}})); } - label(...labels) { - this._setCommentData({label: labels}); - return this; + label(context, ...labels) { + return context.github.issues.addLabels(context.payload.toIssue({body: labels})); } - unlabel(...labels) { - this._setCommentData({unlabel: labels}); - return this; - } - - lock() { - this._setCommentData({lock: true}); - return this; - } - - unlock() { - this._setCommentData({unlock: true}); - return this; - } - - open() { - this._setCommentData({open: true}); - return this; - } - - close() { - this._setCommentData({close: true}); - return this; - } - - _setCommentData(obj) { - if (this.issueActions === undefined) { - this.issueActions = {}; - } - Object.assign(this.issueActions, obj); - } -}; - -// This is the function that implements all of the actions configured above. -class IssueEvaluator extends Evaluator { - - checkIfEventApplies(event) { - return event.issue !== undefined || event.pull_request !== undefined; - } - - comment(content) { - const template = handlebars.compile(content)(this.payload); - return this.github.issues.createComment(this.payload.toIssue({body: template})); - } - - assign(assignees) { - return this.github.issues.addAssigneesToIssue(this.payload.toIssue({assignees})); - } - - unassign(assignees) { - return this.github.issues.removeAssigneesFromIssue(this.payload.toIssue({body: {assignees}})); - } - - label(labels) { - return this.github.issues.addLabels(this.payload.toIssue({body: labels})); - } - - unlabel(labels) { + unlabel(context, ...labels) { return labels.map(label => { - return this.github.issues.removeLabel( - this.payload.toIssue({name: label}) + return context.github.issues.removeLabel( + context.payload.toIssue({name: label}) ); }); } - lock() { - return this.github.issues.lock(this.payload.toIssue({})); + lock(context) { + return context.github.issues.lock(context.payload.toIssue({})); } - unlock() { - return this.github.issues.unlock(this.payload.toIssue({})); + unlock(context) { + return context.github.issues.unlock(context.payload.toIssue({})); } - open() { - return this.github.issues.edit(this.payload.toIssue({state: 'open'})); + open(context) { + return context.github.issues.edit(context.payload.toIssue({state: 'open'})); } - close() { - return this.github.issues.edit(this.payload.toIssue({state: 'closed'})); + close(context) { + return context.github.issues.edit(context.payload.toIssue({state: 'closed'})); } - - pluginData(workflow) { - return workflow.issueActions; - } -} - -module.exports = { - Plugin: IssuePlugin, - Evaluator: IssueEvaluator }; diff --git a/lib/sandbox.js b/lib/sandbox.js index b7715741..d3a8602b 100644 --- a/lib/sandbox.js +++ b/lib/sandbox.js @@ -14,7 +14,7 @@ class Sandbox { on(...events) { const workflow = new Workflow(events); this.workflows.push(workflow); - return workflow; + return workflow.api; } execute() { diff --git a/lib/workflow.js b/lib/workflow.js index 4d2de511..de8ddcd7 100644 --- a/lib/workflow.js +++ b/lib/workflow.js @@ -1,14 +1,33 @@ -const issues = require('./plugins/issues'); +const Issues = require('./plugins/issues'); -class WorkflowCore { +const plugins = [ + new Issues() +]; + +module.exports = class Workflow { constructor(events) { + this.stack = []; this.events = events; this.filterFn = () => true; + this.api = {}; + + for (const plugin of plugins) { + // Get all the property names of the plugin + for (const method of Object.getOwnPropertyNames(plugin.constructor.prototype)) { + if (method !== 'constructor') { + // Define a new function in the API for this plugin method, forcing + // the binding to this to prevent any tampering. + this.api[method] = this.proxy(plugin, method).bind(this); + } + } + } + + this.api.filter = this.filter.bind(this); } filter(fn) { this.filterFn = fn; - return this; + return this.api; } matches(event) { @@ -17,18 +36,24 @@ class WorkflowCore { return name === event.event && (!action || action === event.payload.action); }) && this.filterFn(event); } -} -// FIXME: issues -const plugins = [ - issues.Plugin -]; + proxy(plugin, method) { + // This function is what gets exposed to the sandbox + return (...args) => { + // Push new function on the stack that calls the plugin method with a context. + this.stack.push(context => plugin[method](context, ...args)); -// Helper to combine an array of mixins into one class -function mix(superclass, ...mixins) { - return mixins.reduce((c, mixin) => mixin(c), superclass); -} + // Return the API to allow methods to be chained. + return this.api; + }; + } -class Workflow extends mix(WorkflowCore, ...plugins) {} - -module.exports = Workflow; + execute(context) { + if (this.matches(context.event)) { + // Reduce the stack to a chain of promises, each called with the given context + this.stack.reduce((promise, func) => { + return promise.then(func.bind(func, context)); + }, Promise.resolve()); + } + } +}; diff --git a/scratchpad.js b/scratchpad.js index 18c6a637..475416b1 100644 --- a/scratchpad.js +++ b/scratchpad.js @@ -40,7 +40,7 @@ class Workflow { this.stack = []; this.api = {}; - for(const plugin of PLUGINS) { + for (const plugin of PLUGINS) { // Get all the property names of the plugin for (const method of Object.getOwnPropertyNames(plugin.constructor.prototype)) { if (method !== 'constructor') { diff --git a/test/plugins/issues.js b/test/plugins/issues.js index a8ec0bd7..5247a5b5 100644 --- a/test/plugins/issues.js +++ b/test/plugins/issues.js @@ -1,36 +1,34 @@ const expect = require('expect'); -const issues = require('../../lib/plugins/issues'); -const Workflow = require('../../lib/workflow'); +const Issues = require('../../lib/plugins/issues'); const Context = require('../../lib/context'); const payload = require('../fixtures/webhook/comment.created.json'); const createSpy = expect.createSpy; -const github = { - issues: { - lock: createSpy(), - unlock: createSpy(), - edit: createSpy(), - addLabels: createSpy(), - createComment: createSpy(), - addAssigneesToIssue: createSpy(), - removeAssigneesFromIssue: createSpy(), - removeLabel: createSpy() - } -}; -const context = new Context(github, {}, {payload}); - describe('issues plugin', () => { + const github = { + issues: { + lock: createSpy().andReturn(Promise.resolve()), + unlock: createSpy().andReturn(Promise.resolve()), + edit: createSpy().andReturn(Promise.resolve()), + addLabels: createSpy().andReturn(Promise.resolve()), + createComment: createSpy().andReturn(Promise.resolve()), + addAssigneesToIssue: createSpy().andReturn(Promise.resolve()), + removeAssigneesFromIssue: createSpy().andReturn(Promise.resolve()), + removeLabel: createSpy().andReturn(Promise.resolve()) + } + }; + + const context = new Context(github, {}, {payload}); + before(() => { - this.w = new Workflow(); - this.evaluator = new issues.Evaluator(); + this.issues = new Issues(); }); describe('locking', () => { it('locks', () => { - this.w.lock(); + this.issues.lock(context); - Promise.all(this.evaluator.evaluate(this.w, context)); expect(github.issues.lock).toHaveBeenCalledWith({ owner: 'bkeepers-inc', repo: 'test', @@ -39,9 +37,8 @@ describe('issues plugin', () => { }); it('unlocks', () => { - this.w.unlock(); + this.issues.unlock(context); - Promise.all(this.evaluator.evaluate(this.w, context)); expect(github.issues.unlock).toHaveBeenCalledWith({ owner: 'bkeepers-inc', repo: 'test', @@ -52,9 +49,8 @@ describe('issues plugin', () => { describe('state', () => { it('opens an issue', () => { - this.w.open(); + this.issues.open(context); - Promise.all(this.evaluator.evaluate(this.w, context)); expect(github.issues.edit).toHaveBeenCalledWith({ owner: 'bkeepers-inc', repo: 'test', @@ -63,9 +59,8 @@ describe('issues plugin', () => { }); }); it('closes an issue', () => { - this.w.close(); + this.issues.close(context); - Promise.all(this.evaluator.evaluate(this.w, context)); expect(github.issues.edit).toHaveBeenCalledWith({ owner: 'bkeepers-inc', repo: 'test', @@ -77,9 +72,8 @@ describe('issues plugin', () => { describe('labels', () => { it('adds a label', () => { - this.w.label('hello'); + this.issues.label(context, 'hello'); - Promise.all(this.evaluator.evaluate(this.w, context)); expect(github.issues.addLabels).toHaveBeenCalledWith({ owner: 'bkeepers-inc', repo: 'test', @@ -89,9 +83,8 @@ describe('issues plugin', () => { }); it('adds multiple labels', () => { - this.w.label('hello', 'world'); + this.issues.label(context, 'hello', 'world'); - Promise.all(this.evaluator.evaluate(this.w, context)); expect(github.issues.addLabels).toHaveBeenCalledWith({ owner: 'bkeepers-inc', repo: 'test', @@ -101,9 +94,8 @@ describe('issues plugin', () => { }); it('removes a single label', () => { - this.w.unlabel('hello'); + this.issues.unlabel(context, 'hello'); - Promise.all(this.evaluator.evaluate(this.w, context)); expect(github.issues.removeLabel).toHaveBeenCalledWith({ owner: 'bkeepers-inc', repo: 'test', @@ -113,9 +105,8 @@ describe('issues plugin', () => { }); it('removes a multiple labels', () => { - this.w.unlabel('hello', 'goodbye'); + this.issues.unlabel(context, 'hello', 'goodbye'); - Promise.all(this.evaluator.evaluate(this.w, context)); expect(github.issues.removeLabel).toHaveBeenCalledWith({ owner: 'bkeepers-inc', repo: 'test', @@ -134,9 +125,8 @@ describe('issues plugin', () => { describe('comments', () => { it('creates a comment', () => { - this.w.comment('Hello world!'); + this.issues.comment(context, 'Hello world!'); - Promise.all(this.evaluator.evaluate(this.w, context)); expect(github.issues.createComment).toHaveBeenCalledWith({ owner: 'bkeepers-inc', repo: 'test', @@ -146,9 +136,8 @@ describe('issues plugin', () => { }); it('evaluates templates with handlebars', () => { - this.w.comment('Hello @{{ sender.login }}!'); + this.issues.comment(context, 'Hello @{{ sender.login }}!'); - Promise.all(this.evaluator.evaluate(this.w, context)); expect(github.issues.createComment).toHaveBeenCalledWith({ owner: 'bkeepers-inc', repo: 'test', @@ -160,9 +149,8 @@ describe('issues plugin', () => { describe('assignment', () => { it('assigns a user', () => { - this.w.assign('bkeepers'); + this.issues.assign(context, 'bkeepers'); - Promise.all(this.evaluator.evaluate(this.w, context)); expect(github.issues.addAssigneesToIssue).toHaveBeenCalledWith({ owner: 'bkeepers-inc', repo: 'test', @@ -172,9 +160,8 @@ describe('issues plugin', () => { }); it('assigns multiple users', () => { - this.w.assign('hello', 'world'); + this.issues.assign(context, 'hello', 'world'); - Promise.all(this.evaluator.evaluate(this.w, context)); expect(github.issues.addAssigneesToIssue).toHaveBeenCalledWith({ owner: 'bkeepers-inc', repo: 'test', @@ -184,9 +171,8 @@ describe('issues plugin', () => { }); it('unassigns a user', () => { - this.w.unassign('bkeepers'); + this.issues.unassign(context, 'bkeepers'); - Promise.all(this.evaluator.evaluate(this.w, context)); expect(github.issues.removeAssigneesFromIssue).toHaveBeenCalledWith({ owner: 'bkeepers-inc', repo: 'test', @@ -196,9 +182,8 @@ describe('issues plugin', () => { }); it('unassigns multiple users', () => { - this.w.unassign('hello', 'world'); + this.issues.unassign(context, 'hello', 'world'); - Promise.all(this.evaluator.evaluate(this.w, context)); expect(github.issues.removeAssigneesFromIssue).toHaveBeenCalledWith({ owner: 'bkeepers-inc', repo: 'test',