From 927bf14795052b710e45a5887a51695e4eb9e9e8 Mon Sep 17 00:00:00 2001
From: Stokes <stokes@casetext.com>
Date: Thu, 7 Mar 2019 12:32:11 -0600
Subject: [PATCH 1/2] Demonstrate problem where item.save() encounters an error
 [tests RED]

When item.save() gets a 400 Bad Request response, it chokes parsing the null `data` value without ever calling the save() callback.
---
 test/unit/CollectionItem.js | 47 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/test/unit/CollectionItem.js b/test/unit/CollectionItem.js
index 2ed63dd7..4a1ddde1 100644
--- a/test/unit/CollectionItem.js
+++ b/test/unit/CollectionItem.js
@@ -10,7 +10,7 @@ describe('CollectionItem', function () {
 	var restHandlersPath = path.normalize(__dirname + '/../../lib/restHandlers');
 
 	var restHandlerMock = {
-		editItem: sinon.spy(),
+		editItem: sinon.stub(),
 		listItems: sinon.spy()
 	};
 
@@ -23,6 +23,7 @@ describe('CollectionItem', function () {
 		var CollectionItem = proxyquire('./../../lib/CollectionItem', stubs);
 
 		item = new CollectionItem('deals', {
+			id: 1,
 			title: 'Deal title'
 		}, 1);
 
@@ -64,4 +65,48 @@ describe('CollectionItem', function () {
 			sinon.assert.calledWith(restHandlerMock.editItem, 2, 'deals/1/products', {id: 2});
 		});
 	});
+
+	describe('item.save()', function() {
+		it('should call restHandler.editItem()', function () {
+			// In the normal case, we can set a field value and call save().  Internally, the restHandler.editItem()
+			// method will be called with our update.
+			item.set('title', 'Other deal title');
+			item.save();
+
+			sinon.assert.calledWith(restHandlerMock.editItem, 1, 'deals', {title: 'Other deal title'});
+		});
+
+		it('should handle error with null data from REST call', function () {
+			// Set a field to a field ID that doesn't exist (maybe the custom field key is bad).
+			item.set('badFieldId', 'Ignored value');
+
+			// When we save the item in this state, the real RestHandler.editItem() is given bad input, so it receives
+			// back an error response, such as a 400 with body:
+			//    {
+			//      "success":false,
+			//      "error":"Bad request",
+			//      "error_info":"Please check developers.pipedrive.com for more information about Pipedrive API.",
+			//      "data":null,
+			//      "additional_data":null
+			//    }
+			//
+			// We want to emulate this behavior to make sure our CollectionItem callback handles it properly.  When a
+			// item.save(callbackFn) call is made, the callbackFn should have the opportunity to handle this error.
+			var restError = new Error('Pipedrive API error:Bad request');
+			restHandlerMock.editItem.callsFake(function(id, kind, changedData, callbackFn) {
+				callbackFn(restError, null, null);
+			});
+
+			// For purposes of this test, we don't need to do anything in our callback, but we MUST ensure that we get
+			// called back.  Without getting the error object passed to it, we can't respond to the rejected input.
+			var saveCallback = sinon.spy();
+			item.save(saveCallback);
+
+			sinon.assert.calledWith(restHandlerMock.editItem, 1, 'deals', {'badFieldId': 'Ignored value'});
+			sinon.assert.calledOnce(saveCallback);
+			sinon.assert.calledWith(saveCallback, restError);
+
+			restHandlerMock.editItem.resetBehavior();
+		});
+	});
 });
\ No newline at end of file

From 984b637fd24f3bbfe6492c4c14617ee14b97f152 Mon Sep 17 00:00:00 2001
From: Stokes <stokes@casetext.com>
Date: Thu, 7 Mar 2019 12:33:27 -0600
Subject: [PATCH 2/2] fix error handling in item.save().

When item.save(callbackFn) is called with invalid input, the server rejects the request with a 400 response, as expected. The client, however, does not call the given callbackFn to provide the error to the caller. Instead, an uncaught error is thrown trying to wrap the `null` data into a collection.

This fixes the problem by only wrapping non-`null` response `data`.
---
 lib/CollectionItem.js | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/CollectionItem.js b/lib/CollectionItem.js
index a8f1a25c..690f4505 100644
--- a/lib/CollectionItem.js
+++ b/lib/CollectionItem.js
@@ -23,7 +23,7 @@
 
 		this.save = function(saveCallback) {
 			restHandlers.editItem(data.id, kind, changedData, function (error, data, additionalData, req, res) {
-				var collectionItems = wrapCollectionItems([data], kind, options);
+				var collectionItems = wrapCollectionItems(data ? [data] : data, kind, options);
 				saveCallback(error, collectionItems[0], additionalData, req, res);
 			});
 
@@ -41,14 +41,14 @@
 
 		this.merge = function (withId, callback) {
 			return restHandlers.mergeItem(data.id, withId, kind, function (error, data, additionalData, req, res) {
-				var collectionItems = wrapCollectionItems([data], kind, options);
+				var collectionItems = wrapCollectionItems(data ? [data] : data, kind, options);
 				callback(error, collectionItems[0], additionalData, req, res);
 			});
 		};
 
 		this.duplicate = function (callback) {
 			return restHandlers.duplicateItem(data.id, kind, function (error, data, additionalData, req, res) {
-				var collectionItems = wrapCollectionItems([data], kind, options);
+				var collectionItems = wrapCollectionItems(data ? [data] : data, kind, options);
 				callback(error, collectionItems[0], additionalData, req, res);
 			});
 		};