Merge pull request #485 from CodeNow/notifications-cleanups

Notifications cleanups

Author: tjmehta

configs/.env

@@ -34,7 +34,7 @@ CAYLEY="http://localhost:64210"
 REDIS_NAMESPACE="runnable:api:"
 DNS_JOB_QUEUE_INTERVAL=200
 DNS_JOB_QUEUE_INTERVAL_DELETE=60000
-SLACK_BOT_IMAGE="https://avatars0.githubusercontent.com/u/2828361?v=3&s=200"
+SLACK_BOT_IMAGE="https://avatars0.githubusercontent.com/u/2335750?v=3&s=200"
 SLACK_BOT_USERNAME="runnabot"
 HIPCHAT_BOT_USERNAME="runnabot"
 ENABLE_BUILDS_ON_GIT_PUSH=false

lib/middlewares/notifications/index.js

@@ -5,7 +5,7 @@ var fs = require('fs');
 var inflect = require('i')();
 var createClassMiddleware = require('../create-class-middleware');
 
-var apiMiddlewares = module.exports = {};
+var notificationsMiddlewares = module.exports = {};
 
 // this file automatically generates middlewares from the models/notifications folder
 // if they donot exist in the middlewares/notifications folder
@@ -19,7 +19,7 @@ fs.readdirSync(notificationsMiddlewaresDir).forEach(function (filename) {
   var lower = filename.replace(/\.js$/, '').toLowerCase();
   var middleware = require(path.join(notificationsMiddlewaresDir, filename));
   var camel =  inflect.camelize(inflect.underscore(lower), false);
-  apiMiddlewares[camel] = middleware;
+  notificationsMiddlewares[camel] = middleware;
 });
 
 
@@ -29,9 +29,9 @@ var modelsIncludes = ['index.js'];
 fs.readdirSync(notificationsModelsDir).forEach(function (filename) {
   if (!~modelsIncludes.indexOf(filename)) { return; }
   var lower = filename.replace(/\.js$/, '').toLowerCase();
-  if (apiMiddlewares[lower]) { return; }
+  if (notificationsMiddlewares[lower]) { return; }
 
   var Model = require(path.join(notificationsModelsDir, filename));
   var camel = inflect.camelize(inflect.underscore(lower), false);
-  apiMiddlewares[camel] = createClassMiddleware(camel, Model);
+  notificationsMiddlewares[camel] = createClassMiddleware(camel, Model);
 });

lib/models/notifications/hipchat.js

@@ -3,24 +3,29 @@ var util  = require('util');
 var HipChatClient = require('hipchat-client');
 var Notifier = require('./notifier');
 
-
 // settings should have `api_token` and `room_id` properties.
-// NOTE: should we do validation here?
 function HipChat (settings) {
   Notifier.call(this, 'hipchat', settings);
   this.hipchatClient = new HipChatClient(settings.authToken);
 }
 
 util.inherits(HipChat, Notifier);
 
-HipChat.prototype.send = function (text, cb) {
+HipChat.prototype.send = function (message, cb) {
   this.hipchatClient.api.rooms.message({
     room_id: this.settings.roomId,
     from: process.env.HIPCHAT_BOT_USERNAME,
     message_format: 'html',
-    message: text,
+    message: message,
     color: 'purple'
   }, cb);
 };
 
+HipChat.prototype.makeOnInstancesMessage = function(githubPushInfo, instances) {
+  githubPushInfo.instances = instances;
+  githubPushInfo.domain = process.env.DOMAIN;
+  return this.onInstancesTpl(githubPushInfo);
+};
+
+
 module.exports = HipChat;

lib/models/notifications/notifier.js

@@ -24,9 +24,21 @@ Handlebars.registerHelper('moreChangesSlack', function(repo, commitLog) {
 
 
 Handlebars.registerHelper('encode', function (str) {
-    return encodeURIComponent(str);
+  return encodeURIComponent(str);
 });
 
+/**
+ * Slack requires light escaping with just 3 rules:
+ * & replaced with &
+ * < replaced with &lt;
+ * > replaced with &gt;
+ */
+var ampRegExp = new RegExp('&', 'g');
+var ltRegExp = new RegExp('<', 'g');
+var gtRegExp = new RegExp('>', 'g');
+Handlebars.registerHelper('slackEscape', function (str) {
+  return str.replace(ampRegExp, '&amp;').replace(ltRegExp, '&lt;').replace(gtRegExp, '&gt;');
+});
 
 var onBuildTpls = {};
 var onInstanceTpls = {};
@@ -55,27 +67,34 @@ function createTpl (tplPath) {
 
 
 // should be implemented in the subclass
-Notifier.prototype.send = function (/* text, cb */) {
+Notifier.prototype.send = function (/* message, cb */) {
+  throw new Error('Not implemented');
+};
+
+Notifier.prototype.makeOnBuildMessage = function(githubPushInfo) {
+  githubPushInfo.domain = process.env.DOMAIN;
+  return this.onBuildTpl(githubPushInfo);
+};
+
+// should be implemented in the subclass
+Notifier.prototype.makeOnInstancesMessage = function(/* githubPushInfo, instances */) {
   throw new Error('Not implemented');
 };
 
 // Notify when image was build and ready to be run
 Notifier.prototype.notifyOnBuild = function (githubPushInfo, cb) {
   debug('fill context version for', githubPushInfo);
-  githubPushInfo.domain = process.env.DOMAIN;
-  var text = this.onBuildTpl(githubPushInfo);
-  this.send(text, cb);
+  var message = this.makeOnBuildMessage(githubPushInfo);
+  this.send(message, cb);
 };
 
 // Notify when image was build and deployed to instance
 Notifier.prototype.notifyOnInstances = function (githubPushInfo, instances, cb) {
   debug('fill context version for', githubPushInfo);
   if (instances && instances.length > 0) {
     debug('notify on instances', instances);
-    githubPushInfo.instances = instances;
-    githubPushInfo.domain = process.env.DOMAIN;
-    var text = this.onInstancesTpl(githubPushInfo);
-    this.send(text, cb);
+    var message = this.makeOnInstancesMessage(githubPushInfo, instances);
+    this.send(message, cb);
   }
   else {
     // do nothing

lib/models/notifications/slack.js

@@ -5,21 +5,52 @@ var Notifier = require('./notifier');
 
 
 // settings should have `webhookUrl` property.
-// NOTE: should we do validation here?
 function Slack (settings) {
   Notifier.call(this, 'slack', settings);
   this.slackClient = slack(settings.webhookUrl);
+  this.runnableNotify = this.slackClient.extend({
+    username: process.env.SLACK_BOT_USERNAME,
+    icon_url: process.env.SLACK_BOT_IMAGE
+  });
 }
 
 util.inherits(Slack, Notifier);
 
-Slack.prototype.send = function (text, cb) {
+
+Slack.prototype.send = function (message, cb) {
+  this.runnableNotify(message, cb);
+};
+
+Slack.prototype.makeOnInstancesMessage = function(githubPushInfo, instances) {
+  githubPushInfo.instances = instances;
+  githubPushInfo.domain = process.env.DOMAIN;
+  var text = this.onInstancesTpl(githubPushInfo);
+  var instancesLinks = instances.map(instanceToLink);
+  var fields = instancesLinks.map(istanceToAttachment);
   var opts = {
     text: text,
-    username: process.env.SLACK_BOT_USERNAME,
-    icon_url: process.env.SLACK_BOT_IMAGE
+    attachments: [
+      {
+        fallback: instancesLinks.join('\n'),
+        color: '#5b3777',
+        fields: fields
+      }
+    ]
   };
-  this.slackClient.send(opts, cb);
+  return opts;
 };
 
+
+
+function istanceToAttachment (link) {
+  return {
+    value: link
+  };
+}
+
+function instanceToLink(instance) {
+  return '<http://' + process.env.DOMAIN + '/' + instance.owner.username +
+         '/' + instance.name  + '|' + instance.name + '>';
+}
+
 module.exports = Slack;

lib/routes/actions/github.js

@@ -33,8 +33,7 @@ var pushSessionUser = {
   permissionLevel: 5,
   accounts: {
     github: {
-      id: 'githubPushInfo.user.id',
-      token: 'runnableToken'
+      id: 'githubPushInfo.user.id'
     }
   }
 };

lib/routes/instances/containers/index.js

@@ -33,10 +33,3 @@ app.get('/instances/:id/containers',
   findInstance,
   mw.res.json('instance.containers'));
 
-/**
- * Polling route used by frontend to determine if a new build was pushed
- *  @event GET rest/instances/:id/containers
- *  @memberof module:rest/instances/:id/containers */
-app.get('/instances/:id/build',
-  findInstance,
-  mw.res.json('instance.build')); // buildId as string

lib/routes/instances/index.js

@@ -242,7 +242,7 @@ app.get('/instances/',
   mw.query({or: [
     'owner', 'shortHash', 'name', '["contextVersion.appCodeVersions.repo"]'
   ]}).require(),
-  // Note: be careful pick does not work like the others, 
+  // Note: be careful pick does not work like the others,
   // pick only works with keys and not keypaths!
   mw.query('owner', 'shortHash', 'name' ,
     'owner.github', 'contextVersion.appCodeVersions.repo').pick(),
@@ -347,6 +347,18 @@ app.get('/instances/:id',
   instances.model.populateModels(),
   mw.res.json('instance'));
 
+
+/**
+ * Polling route used by frontend to determine if a new build was pushed
+ *  @event GET rest/instances/:id/containers
+ *  @memberof module:rest/instances/:id/containers */
+app.get('/instances/:id/build',
+  findInstance,
+  flow.or(
+    me.isOwnerOf('instance'),
+    me.isModerator),
+  mw.res.json('instance.build')); // buildId as string
+
 /**
  * acquire host locks for oldName and newName
  */

templates/hipchat_on_build.hbs

@@ -1,2 +1,2 @@
 {{user.login}}'s <a href="{{headCommit.url}}">changes</a> ({{headCommit.message}}{{{moreChangesHipchat repo commitLog}}}) to {{repo}} ({{branch}}) are ready.
-<a href="http://{{domain}}/{{owner.login}}/boxSelection/{{repoName}}/{{branch}}/{{encode headCommit.message}}/{{headCommit.id}}">Choose a server to run {{branch}}</a>.
+<a href="http://{{domain}}/{{owner.login}}/boxSelection/{{repoName}}/{{encode branch}}/{{encode headCommit.message}}/{{headCommit.id}}">Choose a server to run {{branch}}</a>.

templates/hipchat_on_instances.hbs

@@ -1,4 +1,4 @@
-{{user.login}}'s <a href="{{headCommit.url}}">changes</a> ({{headCommit.message}}{{{moreChangesHipchat repo commitLog}}}) to {{repo}} ({{branch}}) are deployed on servers:
+{{user.login}}'s <a href="{{headCommit.url}}">changes</a> ({{headCommit.message}}{{{moreChangesHipchat repo commitLog}}}) to {{repo}} ({{branch}}) are deployed on servers:<br/>
 {{#each instances}}
- <a href="http://{{../domain}}/{{this.owner.username}}/{{this.name}}">{{this.name}}</a></br>
-{{/each}}.
+ <a href="http://{{../domain}}/{{this.owner.username}}/{{this.name}}">{{this.name}}</a><br/>
+{{/each}}