From 77049a226576c5cad8d2170ba46e3c00b3988051 Mon Sep 17 00:00:00 2001 From: Gabriel Birke Date: Fri, 9 Feb 2018 22:54:55 +0100 Subject: [PATCH 1/4] Improve code style Remove trailing whitespaces Add semicolons --- pinboard/pinboard.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/pinboard/pinboard.js b/pinboard/pinboard.js index 9d9a3309..739ecf4c 100644 --- a/pinboard/pinboard.js +++ b/pinboard/pinboard.js @@ -26,15 +26,15 @@ module.exports = function(RED) { token: { type:"password" } } }); - - + + function PinboardOutNode(n) { RED.nodes.createNode(this,n); var node = this; this.tags = n.tags; this.toread = n.toread; this.private = n.private; - + this.user = RED.nodes.getNode(n.user); if (this.user) { this.on("input", function(msg) { @@ -59,14 +59,14 @@ module.exports = function(RED) { "Accept":"application/json" } } - // TODO: allow tags to be added by the message + // TODO: allow tags to be added by the message if (node.tags) { - options.path += "&tags="+encodeURIComponent(node.tags) + options.path += "&tags="+encodeURIComponent(node.tags); } if (msg.description) { - options.path += "&extended="+encodeURIComponent(msg.description) + options.path += "&extended="+encodeURIComponent(msg.description); } - + node.status({fill:"blue",shape:"dot",text:"pinboard.status.saving"}); https.get(options, function(res) { @@ -87,12 +87,12 @@ module.exports = function(RED) { node.error(err,msg); node.status({fill:"red",shape:"ring",text:err.code}); }); - + }); } else { this.error(RED._("pinboard.error.no-apitoken")); } - + } RED.nodes.registerType("pinboard out",PinboardOutNode); -} +}; From 27ec0cdc158b79996a5609d15867ab7adc0623f9 Mon Sep 17 00:00:00 2001 From: Gabriel Birke Date: Fri, 9 Feb 2018 22:55:06 +0100 Subject: [PATCH 2/4] Improve pinboard error handling Use HTTPS. Check the HTTP status code to avoid trying to parse empty JSON messages. Catch JSON parsing errors. --- pinboard/pinboard.js | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/pinboard/pinboard.js b/pinboard/pinboard.js index 739ecf4c..df2ff8ad 100644 --- a/pinboard/pinboard.js +++ b/pinboard/pinboard.js @@ -47,6 +47,7 @@ module.exports = function(RED) { return; } var options = { + protocol: "https:", hostname: "api.pinboard.in", path: "/v1/posts/add?"+ "url="+encodeURIComponent(msg.payload)+ @@ -75,7 +76,20 @@ module.exports = function(RED) { m += chunk; }); res.on('end',function() { - var result = JSON.parse(m); + var result; + if (res.statusCode < 200 || res.statusCode > 299) { + node.error(res.statusMessage); + node.status({fill:"red",shape:"ring",text:res.statusMessage}); + return; + } + try { + result = JSON.parse(m); + } catch (e) { + node.error(e,msg); + node.status({fill:"red",shape:"ring",text:e}); + return; + } + if (result.result_code == "done") { node.status({}); } else { From 6591f37f614e65091a46c9f2ea8dc6888b11d663 Mon Sep 17 00:00:00 2001 From: Gabriel Birke Date: Wed, 14 Feb 2018 23:34:43 +0100 Subject: [PATCH 3/4] Speed up pinboard tests Instead of using timeouts, use sinon.js to stub the error functions. This also improves code quality as the test code no longer needs to know the logging internals. --- test/pinboard/pinboard_spec.js | 58 ++++++++++++++-------------------- 1 file changed, 23 insertions(+), 35 deletions(-) diff --git a/test/pinboard/pinboard_spec.js b/test/pinboard/pinboard_spec.js index cfacc258..7f5ca0e9 100644 --- a/test/pinboard/pinboard_spec.js +++ b/test/pinboard/pinboard_spec.js @@ -15,6 +15,7 @@ **/ var should = require("should"); +var sinon = require('sinon'); var pinboardNode = require("../../pinboard/pinboard.js"); var helper = require('../helper.js'); @@ -30,10 +31,10 @@ describe('pinboard nodes', function() { describe('out node', function() { it(' logs a warning if msg.payload is not set', function(done) { - helper.load(pinboardNode, + helper.load(pinboardNode, [ {id:"inject", type:"helper", wires:[["pinboard"]]}, {id:"del-user", type:"pinboard-user", username:"Bob Jones"}, - {id:"pinboard", type:"pinboard out", user:"del-user", private:true, tags:"testtag"}], + {id:"pinboard", type:"pinboard out", user:"del-user", private:true, tags:"testtag"}], { "del-user": { password:"abcd1234" @@ -43,30 +44,23 @@ describe('pinboard nodes', function() { var inject = helper.getNode("inject"); var pinboard = helper.getNode("pinboard"); pinboard.should.have.property('id','pinboard'); + + var stub = sinon.stub(pinboard, 'error').callsFake(function(msg){ + var expected = 'pinboard.error.no-url'; + should.deepEqual(msg, expected); + stub.restore(); + done(); + }); + inject.send({title:"test",description:"testdesc"}); - setTimeout(function() { - try { - helper.log().called.should.be.true; - var logEvents = helper.log().args.filter(function(evt) { - return evt[0].type == "pinboard out"; - }); - logEvents.should.have.length(1); - logEvents[0][0].should.have.a.property("id",pinboard.id); - logEvents[0][0].should.have.a.property("type",pinboard.type); - logEvents[0][0].should.have.a.property("msg","pinboard.error.no-url"); - done(); - } catch(err) { - done(err); - } - },200); }); }); - + it(' logs a warning if msg.title is not set', function(done) { - helper.load(pinboardNode, + helper.load(pinboardNode, [ {id:"inject", type:"helper", wires:[["pinboard"]]}, {id:"del-user", type:"pinboard-user", username:"Bob Jones"}, - {id:"pinboard", type:"pinboard out", user:"del-user", private:true, tags:"testtag"}], + {id:"pinboard", type:"pinboard out", user:"del-user", private:true, tags:"testtag"}], { "del-user": { password:"abcd1234" @@ -76,23 +70,17 @@ describe('pinboard nodes', function() { var inject = helper.getNode("inject"); var pinboard = helper.getNode("pinboard"); pinboard.should.have.property('id','pinboard'); + + var stub = sinon.stub(pinboard, 'error').callsFake(function(msg){ + var expected = 'pinboard.error.no-title'; + should.deepEqual(msg, expected); + stub.restore(); + done(); + }); + inject.send({payload:"foobar",description:"testdesc"}); - setTimeout(function() { - try { - helper.log().called.should.be.true; - var logEvents = helper.log().args.filter(function(evt) { - return evt[0].type == "pinboard out"; - }); - logEvents.should.have.length(1); - logEvents[0][0].should.have.a.property("id",pinboard.id); - logEvents[0][0].should.have.a.property("type",pinboard.type); - logEvents[0][0].should.have.a.property("msg","pinboard.error.no-title"); - done(); - } catch(err) { - done(err); - } - },200); }); }); + }); }); From e1887a23746c42fbf74066dfd94e16dda5c24050 Mon Sep 17 00:00:00 2001 From: Gabriel Birke Date: Wed, 14 Feb 2018 23:54:13 +0100 Subject: [PATCH 4/4] Test pinboard network requests use nock to intercept pinboard network requests. Change http.get to http.request to play nice with our 4-year old version of nock. If the server (or network mock) sends no HTTP status message, improvise. Show user-friendly, translatable status messages, log raw errors. --- pinboard/locales/en-US/pinboard.json | 4 +- pinboard/pinboard.js | 18 ++-- test/pinboard/pinboard_spec.js | 137 +++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 8 deletions(-) diff --git a/pinboard/locales/en-US/pinboard.json b/pinboard/locales/en-US/pinboard.json index 164d760b..7124343f 100644 --- a/pinboard/locales/en-US/pinboard.json +++ b/pinboard/locales/en-US/pinboard.json @@ -22,7 +22,9 @@ "error": { "no-url": "url must be provided in msg.payload", "no-title": "msg.title must be provided", - "no-apitoken": "missing api token" + "no-apitoken": "missing api token", + "server-error": "server returned error response", + "invalid-json": "server returned invalid json" } } } diff --git a/pinboard/pinboard.js b/pinboard/pinboard.js index df2ff8ad..f48f76ff 100644 --- a/pinboard/pinboard.js +++ b/pinboard/pinboard.js @@ -47,6 +47,7 @@ module.exports = function(RED) { return; } var options = { + method: 'GET', protocol: "https:", hostname: "api.pinboard.in", path: "/v1/posts/add?"+ @@ -70,23 +71,24 @@ module.exports = function(RED) { node.status({fill:"blue",shape:"dot",text:"pinboard.status.saving"}); - https.get(options, function(res) { + var req = https.request(options, function(res) { var m = ""; res.on('data',function(chunk) { m += chunk; }); res.on('end',function() { - var result; + var httpStatusMessage, result; if (res.statusCode < 200 || res.statusCode > 299) { - node.error(res.statusMessage); - node.status({fill:"red",shape:"ring",text:res.statusMessage}); + httpStatusMessage = res.statusMessage || ('Server Error, Status ' + res.statusCode ); + node.error(httpStatusMessage); + node.status({fill:"red",shape:"ring",text:RED._("pinboard.error.server-error")}); return; } try { result = JSON.parse(m); } catch (e) { - node.error(e,msg); - node.status({fill:"red",shape:"ring",text:e}); + node.error(e.message, msg); + node.status({fill:"red",shape:"ring",text:RED._("pinboard.error.invalid.json")}); return; } @@ -97,10 +99,12 @@ module.exports = function(RED) { node.status({fill:"red",shape:"ring",text:result.result_code}); } }); - }).on('error',function(err) { + }); + req.on('error',function(err) { node.error(err,msg); node.status({fill:"red",shape:"ring",text:err.code}); }); + req.end(); }); } else { diff --git a/test/pinboard/pinboard_spec.js b/test/pinboard/pinboard_spec.js index 7f5ca0e9..7f7a7adc 100644 --- a/test/pinboard/pinboard_spec.js +++ b/test/pinboard/pinboard_spec.js @@ -18,6 +18,7 @@ var should = require("should"); var sinon = require('sinon'); var pinboardNode = require("../../pinboard/pinboard.js"); var helper = require('../helper.js'); +var nock = helper.nock; describe('pinboard nodes', function() { @@ -82,5 +83,141 @@ describe('pinboard nodes', function() { }); }); + it(' logs a warning if server status is not ok', function(done) { + if (!nock) { return; } + helper.load(pinboardNode, + [ {id:"inject", type:"helper", wires:[["pinboard"]]}, + {id:"del-user", type:"pinboard-user", username:"Bob Jones"}, + {id:"pinboard", type:"pinboard out", user:"del-user", private:true, tags:"testtag"}], + { + "del-user": { + token:"bob:abcd1234" + }, + }, + function() { + + var scope = nock('https://api.pinboard.in:443') + .get('/v1/posts/add?url=http%3A%2F%2Fexample.com%2F&description=test%20link&auth_token=bob:abcd1234&format=json&shared=no&toread=no&tags=testtag') + .reply(401) + + var inject = helper.getNode("inject"); + var pinboard = helper.getNode("pinboard"); + pinboard.should.have.property('id','pinboard'); + + var stub = sinon.stub(pinboard, 'error').callsFake(function(msg){ + var expected = 'Server Error, Status 401'; + should.deepEqual(msg, expected); + stub.restore(); + done(); + }); + + inject.send({payload:"http://example.com/",title:"test link"}); + + }); + }); + + it(' logs a warning if server returns broken JSON', function(done) { + if (!nock) { return; } + helper.load(pinboardNode, + [ {id:"inject", type:"helper", wires:[["pinboard"]]}, + {id:"del-user", type:"pinboard-user", username:"Bob Jones"}, + {id:"pinboard", type:"pinboard out", user:"del-user", private:true, tags:"testtag"}], + { + "del-user": { + token:"bob:abcd1234" + }, + }, + function() { + + var scope = nock('https://api.pinboard.in:443') + .get('/v1/posts/add?url=http%3A%2F%2Fexample.com%2F&description=test%20link&auth_token=bob:abcd1234&format=json&shared=no&toread=no&tags=testtag') + .reply(200, "}This is not Json{") + + var inject = helper.getNode("inject"); + var pinboard = helper.getNode("pinboard"); + pinboard.should.have.property('id','pinboard'); + + var stub = sinon.stub(pinboard, 'error').callsFake(function(msg){ + msg.should.containEql('Unexpected token }'); + stub.restore(); + done(); + }); + + inject.send({payload:"http://example.com/",title:"test link"}); + + }); + }); + + it(' logs a warning if server returns an error code', function(done) { + if (!nock) { return; } + helper.load(pinboardNode, + [ {id:"inject", type:"helper", wires:[["pinboard"]]}, + {id:"del-user", type:"pinboard-user", username:"Bob Jones"}, + {id:"pinboard", type:"pinboard out", user:"del-user", private:true, tags:"testtag"}], + { + "del-user": { + token:"bob:abcd1234" + }, + }, + function() { + + var scope = nock('https://api.pinboard.in:443') + .get('/v1/posts/add?url=http%3A%2F%2Fexample.com%2F&description=test%20link&auth_token=bob:abcd1234&format=json&shared=no&toread=no&tags=testtag') + .reply(200, {result_code: 'internal error (testing)'}) + + var inject = helper.getNode("inject"); + var pinboard = helper.getNode("pinboard"); + pinboard.should.have.property('id','pinboard'); + + var stub = sinon.stub(pinboard, 'error').callsFake(function(msg){ + var expected = 'internal error (testing)'; + should.deepEqual(msg, expected); + stub.restore(); + done(); + }); + + inject.send({payload:"http://example.com/",title:"test link"}); + + }); + }); + + it(' changes the status on successful request', function(done) { + if (!nock) { return; } + helper.load(pinboardNode, + [ {id:"inject", type:"helper", wires:[["pinboard"]]}, + {id:"del-user", type:"pinboard-user", username:"Bob Jones"}, + {id:"pinboard", type:"pinboard out", user:"del-user", private:true, tags:"testtag"}], + { + "del-user": { + token:"bob:abcd1234" + }, + }, + function() { + + var scope = nock('https://api.pinboard.in:443') + .get('/v1/posts/add?url=http%3A%2F%2Fexample.com%2F&description=test%20link&auth_token=bob:abcd1234&format=json&shared=no&toread=no&tags=testtag') + .reply(200, {result_code: 'done'}) + + var inject = helper.getNode("inject"); + var pinboard = helper.getNode("pinboard"); + pinboard.should.have.property('id','pinboard'); + + var stub = sinon.stub(pinboard, 'status').callsFake(function(status){ + switch (stub.callCount) { + case 1: + should.deepEqual(status, {fill:"blue",shape:"dot",text:"pinboard.status.saving"}); + break; + case 2: + should.deepEqual(status, {}); + stub.restore(); + done(); + } + }); + + inject.send({payload:"http://example.com/",title:"test link"}); + + }); + }); + }); });