Fixed error handling in readContext and updateContext functions

readContext didn't check if storage returned error updateContext called callback without waiting for storage to finish
akinabot · Jun 23, 2017 · e9498bc · e9498bc
1 parent b524e49
commit e9498bc
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 27 deletions.
diff --git a/lib/middleware/utils.js b/lib/middleware/utils.js
@@ -17,6 +17,8 @@ var debug = require('debug')('watson-middleware:utils');
 
 var readContext = function(userId, storage, callback) {
   storage.users.get(userId, function(err, user_data) {
+    if (err) return callback(err);
+
     if (user_data && user_data.context) {
       debug('User: %s, Context: %s', userId, JSON.stringify(user_data.context, null, 2));
       callback(null, user_data.context);
@@ -28,8 +30,8 @@ var readContext = function(userId, storage, callback) {
 };
 
 var updateContext = function(userId, storage, watsonResponse, callback) {
-  readContext(userId, storage, function(error, user_data) {
-    if (error) return callback(error);
+  readContext(userId, storage, function(err, user_data) {
+    if (err) return callback(err);
 
     if (!user_data) {
       user_data = {};
@@ -39,21 +41,20 @@ var updateContext = function(userId, storage, watsonResponse, callback) {
 
     storage.users.save(user_data, function(err) {
       if (err) return callback(err);
+
+      debug('User: %s, Updated Context: %s', userId, JSON.stringify(watsonResponse.context, null, 2));
+      callback(null, watsonResponse);
     });
-    debug('User: %s, Updated Context: %s', userId, JSON.stringify(watsonResponse.context, null, 2));
-    callback(null, watsonResponse);
   });
 };
 
 var postMessage = function(conversation, payload, callback) {
   debug('Conversation Request: %s', JSON.stringify(payload, null, 2));
   conversation.message(payload, function(err, response) {
-    if (err) {
-      callback(err);
-    } else {
-      debug('Conversation Response: %s', JSON.stringify(response, null, 2));
-      callback(null, response);
-    }
+    if (err) return callback(err);
+
+    debug('Conversation Response: %s', JSON.stringify(response, null, 2));
+    callback(null, response);
   });
 };
 

diff --git a/package.json b/package.json
@@ -23,8 +23,9 @@
   "devDependencies": {
     "assert": "^1.4.1",
     "botkit": "^0.2.2",
+    "mocha": "^3.1.0",
     "nock": "^8.1.0",
-    "mocha": "^3.1.0"
+    "sinon": "^2.3.5"
   },
   "dependencies": {
     "bluebird": "^3.4.6",

diff --git a/test/test.context_store.js b/test/test.context_store.js
@@ -17,8 +17,9 @@
 var assert = require('assert');
 var utils = require('../lib/middleware/utils');
 var Botkit = require('botkit');
+var sinon = require('sinon');
 
-describe('context()', function() {
+describe('context', function() {
   var message = {
     "type": "message",
     "channel": "D2BQEJJ1X",
@@ -69,25 +70,47 @@ describe('context()', function() {
   });
   var storage = bot.botkit.storage;
 
-  it('should read context correctly', function() {
-    utils.readContext(message, storage, function(cb, context) {
-      assert.deepEqual(context, null, 'Actual context: ' + context + '\ndoes not match expected context: ' + null);
+  describe('readContext()', function() {
+    it('should read context correctly', function () {
+      utils.readContext(message, storage, function (cb, context) {
+        assert.deepEqual(context, null, 'Actual context: ' + context + '\ndoes not match expected context: ' + null);
+      });
+    });
+
+    it('should handle storage error correctly', function () {
+      var storageStub = sinon.stub(storage.users, 'get').yields('error message');
+
+      utils.readContext(message, storage, function (err, context) {
+        assert.equal(err, 'error message', 'Error was not passed to callback');
+      });
+      storageStub.restore();
     });
   });
 
-  it('should update context correctly', function() {
-    var expected_store = {
-      "U2BLZSKFG": {
-        "id": "U2BLZSKFG",
-        "context": conversation_response.context
-      }
-    };
-    utils.updateContext(message.user, storage, conversation_response, function(cb, response) {
-      storage.users.all(function(err, data) {
-        assert.equal(err, null);
-        assert.deepEqual(data, expected_store, 'Updated store: ' + data + '\ndoes not match expected store: ' + expected_store);
+  describe('updateContext()', function() {
+    it('should update context correctly', function () {
+      var expected_store = {
+        "U2BLZSKFG": {
+          "id": "U2BLZSKFG",
+          "context": conversation_response.context
+        }
+      };
+      utils.updateContext(message.user, storage, conversation_response, function (cb, response) {
+        storage.users.all(function (err, data) {
+          assert.equal(err, null);
+          assert.deepEqual(data, expected_store, 'Updated store: ' + data + '\ndoes not match expected store: ' + expected_store);
+        });
+      });
+    });
+
+    it('should handle storage error correctly', function () {
+      var storageStub = sinon.stub(storage.users, 'save').yields('error message');
+
+      utils.updateContext(message.user, storage, conversation_response, function (err, context) {
+        assert.equal(err, 'error message', 'Error was not passed to callback');
       });
+      storageStub.restore();
     });
   });
 
-});;
+});