Refactor gravatarLookup, remove request dependency

no issue

- request is quite a heavy dependency
- we were only using request in 3 places: a test, storing contrib images in the gruntfile & the gravatar lookup
- all 3 are relatively simple to do with the http/https module
- refactored all 3, removed request

Gruntfile.js

@@ -8,11 +8,11 @@
 var _              = require('lodash'),
     chalk          = require('chalk'),
     fs             = require('fs-extra'),
+    https          = require('https'),
     moment         = require('moment'),
     getTopContribs = require('top-gh-contribs'),
     path           = require('path'),
     Promise        = require('bluebird'),
-    request        = require('request'),
 
     escapeChar     = process.platform.match(/^win/) ? '^' : '\\',
     cwd            = process.cwd().replace(/( |\(|\))/g, escapeChar + '$1'),
@@ -839,9 +839,11 @@ var _              = require('lodash'),
 
                     downloadImagePromise = function (url, name) {
                         return new Promise(function (resolve, reject) {
-                            request(url)
+                            https.get(url, function (res) {
-                            .pipe(fs.createWriteStream(imagePath + name))
+                                    fs.writeFile(imagePath + name, res, function () {
-                            .on('close', resolve)
+                                        resolve();
+                                    });
+                                })
                                 .on('error', reject);
                         });
                     };

core/server/models/user.js

@@ -2,13 +2,12 @@ var _              = require('lodash'),
     Promise        = require('bluebird'),
     errors         = require('../errors'),
     utils          = require('../utils'),
+    gravatar       = require('../utils/gravatar'),
     bcrypt         = require('bcryptjs'),
     ghostBookshelf = require('./base'),
     crypto         = require('crypto'),
     validator      = require('validator'),
-    request        = require('request'),
     validation     = require('../data/validation'),
-    config         = require('../config'),
     events         = require('../events'),
     i18n           = require('../i18n'),
 
@@ -380,7 +379,7 @@ User = ghostBookshelf.Model.extend({
             // Assign the hashed password
             userData.password = hash;
             // LookupGravatar
-            return self.gravatarLookup(userData);
+            return gravatar.lookup(userData);
         }).then(function then(userData) {
             // Save the user with the hashed password
             return ghostBookshelf.Model.add.call(self, userData, options);
@@ -423,8 +422,10 @@ User = ghostBookshelf.Model.extend({
             // Assign the hashed password
             userData.password = hash;
 
-            return Promise.join(self.gravatarLookup(userData),
+            return Promise.join(
-                                ghostBookshelf.Model.generateSlug.call(this, User, userData.name, options));
+                gravatar.lookup(userData),
+                ghostBookshelf.Model.generateSlug.call(this, User, userData.name, options)
+            );
         }).then(function then(results) {
             userData = results[0];
             userData.slug = results[1];
@@ -776,31 +777,6 @@ User = ghostBookshelf.Model.extend({
         });
     },
 
-    gravatarLookup: function gravatarLookup(userData) {
-        var gravatarUrl = '//www.gravatar.com/avatar/' +
-                crypto.createHash('md5').update(userData.email.toLowerCase().trim()).digest('hex') +
-                '?s=250';
-
-        return new Promise(function gravatarRequest(resolve) {
-            if (config.isPrivacyDisabled('useGravatar')) {
-                return resolve(userData);
-            }
-
-            request({url: 'http:' + gravatarUrl + '&d=404&r=x', timeout: 2000}, function handler(err, response) {
-                if (err) {
-                    // just resolve with no image url
-                    return resolve(userData);
-                }
-
-                if (response.statusCode !== 404) {
-                    gravatarUrl += '&d=mm&r=x';
-                    userData.image = gravatarUrl;
-                }
-
-                resolve(userData);
-            });
-        });
-    },
     // Get the user by email address, enforces case insensitivity rejects if the user is not found
     // When multi-user support is added, email addresses must be deduplicated with case insensitivity, so that
     // joe@bloggs.com and JOE@BLOGGS.COM cannot be created as two separate users.

core/server/utils/gravatar.js

@@ -0,0 +1,42 @@
+var Promise = require('bluebird'),
+    config = require('../config'),
+    crypto = require('crypto'),
+    https = require('https');
+
+module.exports.lookup = function lookup(userData, timeout) {
+    var gravatarUrl = '//www.gravatar.com/avatar/' +
+        crypto.createHash('md5').update(userData.email.toLowerCase().trim()).digest('hex') +
+        '?s=250';
+
+    return new Promise(function gravatarRequest(resolve) {
+        if (config.isPrivacyDisabled('useGravatar') || process.env.NODE_ENV.indexOf('testing') > -1) {
+            return resolve(userData);
+        }
+
+        var request, timer, timerEnded = false;
+
+        request = https.get('https:' + gravatarUrl + '&d=404&r=x', function (response) {
+            clearTimeout(timer);
+            if (response.statusCode !== 404 && !timerEnded) {
+                gravatarUrl += '&d=mm&r=x';
+                userData.image = gravatarUrl;
+            }
+
+            resolve(userData);
+        });
+
+        request.on('error', function () {
+            clearTimeout(timer);
+            // just resolve with no image url
+            if (!timerEnded) {
+                return resolve(userData);
+            }
+        });
+
+        timer = setTimeout(function () {
+            timerEnded = true;
+            request.abort();
+            return resolve(userData);
+        }, timeout || 2000);
+    });
+};

core/test/integration/api/api_users_spec.js

@@ -7,7 +7,7 @@ var testUtils       = require('../../utils'),
     _               = require('lodash'),
 
 // Stuff we are testing
-    ModelUser       = require('../../../server/models'),
+    models          = require('../../../server/models'),
     UserAPI         = require('../../../server/api/users'),
     mail            = require('../../../server/api/mail'),
 
@@ -39,7 +39,7 @@ describe('Users API', function () {
     it('dateTime fields are returned as Date objects', function (done) {
         var userData = testUtils.DataGenerator.forModel.users[0];
 
-        ModelUser.User.check({email: userData.email, password: userData.password}).then(function (user) {
+        models.User.check({email: userData.email, password: userData.password}).then(function (user) {
             return UserAPI.read({id: user.id});
         }).then(function (response) {
             response.users[0].created_at.should.be.an.instanceof(Date);
@@ -482,7 +482,7 @@ describe('Users API', function () {
             UserAPI.edit(
                 {users: [{name: 'newname', password: 'newpassword'}]}, _.extend({}, context.author, {id: userIdFor.author})
             ).then(function () {
-                return ModelUser.User.findOne({id: userIdFor.author}).then(function (response) {
+                return models.User.findOne({id: userIdFor.author}).then(function (response) {
                     response.get('name').should.eql('newname');
                     response.get('password').should.not.eql('newpassword');
                     done();
@@ -497,10 +497,6 @@ describe('Users API', function () {
         beforeEach(function () {
             newUser = _.clone(testUtils.DataGenerator.forKnex.createUser(testUtils.DataGenerator.Content.users[4]));
 
-            sandbox.stub(ModelUser.User, 'gravatarLookup', function (userData) {
-                return Promise.resolve(userData);
-            });
-
             sandbox.stub(mail, 'send', function () {
                 return Promise.resolve();
             });

core/test/integration/model/model_users_spec.js

@@ -9,6 +9,7 @@ var testUtils   = require('../../utils'),
 
     // Stuff we are testing
     utils       = require('../../../server/utils'),
+    gravatar    = require('../../../server/utils/gravatar'),
     UserModel   = require('../../../server/models/user').User,
     RoleModel   = require('../../../server/models/role').Role,
     events      = require('../../../server/events'),
@@ -38,10 +39,6 @@ describe('User Model', function run() {
         it('can add first', function (done) {
             var userData = testUtils.DataGenerator.forModel.users[0];
 
-            sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
-                return Promise.resolve(userData);
-            });
-
             UserModel.add(userData, context).then(function (createdUser) {
                 should.exist(createdUser);
                 createdUser.has('uuid').should.equal(true);
@@ -55,10 +52,6 @@ describe('User Model', function run() {
         it('shortens slug if possible', function (done) {
             var userData = testUtils.DataGenerator.forModel.users[2];
 
-            sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
-                return Promise.resolve(userData);
-            });
-
             UserModel.add(userData, context).then(function (createdUser) {
                 should.exist(createdUser);
                 createdUser.has('slug').should.equal(true);
@@ -70,10 +63,6 @@ describe('User Model', function run() {
         it('does not short slug if not possible', function (done) {
             var userData = testUtils.DataGenerator.forModel.users[2];
 
-            sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
-                return Promise.resolve(userData);
-            });
-
             UserModel.add(userData, context).then(function (createdUser) {
                 should.exist(createdUser);
                 createdUser.has('slug').should.equal(true);
@@ -99,10 +88,6 @@ describe('User Model', function run() {
         it('does NOT lowercase email', function (done) {
             var userData = testUtils.DataGenerator.forModel.users[2];
 
-            sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
-                return Promise.resolve(userData);
-            });
-
             UserModel.add(userData, context).then(function (createdUser) {
                 should.exist(createdUser);
                 createdUser.has('uuid').should.equal(true);
@@ -114,7 +99,7 @@ describe('User Model', function run() {
         it('can find gravatar', function (done) {
             var userData = testUtils.DataGenerator.forModel.users[4];
 
-            sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
+            sandbox.stub(gravatar, 'lookup', function (userData) {
                 userData.image = 'http://www.gravatar.com/avatar/2fab21a4c4ed88e76add10650c73bae1?d=404';
                 return Promise.resolve(userData);
             });
@@ -132,7 +117,7 @@ describe('User Model', function run() {
         it('can handle no gravatar', function (done) {
             var userData = testUtils.DataGenerator.forModel.users[0];
 
-            sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
+            sandbox.stub(gravatar, 'lookup', function (userData) {
                 return Promise.resolve(userData);
             });
 
@@ -336,10 +321,6 @@ describe('User Model', function run() {
         it('can invite user', function (done) {
             var userData = testUtils.DataGenerator.forModel.users[4];
 
-            sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
-                return Promise.resolve(userData);
-            });
-
             UserModel.add(_.extend({}, userData, {status: 'invited'}), context).then(function (createdUser) {
                 should.exist(createdUser);
                 createdUser.has('uuid').should.equal(true);
@@ -356,10 +337,6 @@ describe('User Model', function run() {
         it('can add active user', function (done) {
             var userData = testUtils.DataGenerator.forModel.users[4];
 
-            sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
-                return Promise.resolve(userData);
-            });
-
             RoleModel.findOne().then(function (role) {
                 userData.roles = [role.toJSON()];
 
@@ -406,10 +383,6 @@ describe('User Model', function run() {
             var userData = testUtils.DataGenerator.forModel.users[4],
                 userId;
 
-            sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
-                return Promise.resolve(userData);
-            });
-
             UserModel.add(_.extend({}, userData, {status: 'invited'}), context).then(function (createdUser) {
                 should.exist(createdUser);
                 createdUser.has('uuid').should.equal(true);
@@ -436,10 +409,6 @@ describe('User Model', function run() {
             var userData = testUtils.DataGenerator.forModel.users[4],
                 userId;
 
-            sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
-                return Promise.resolve(userData);
-            });
-
             UserModel.add(_.extend({}, userData, {status: 'invited'}), context).then(function (createdUser) {
                 should.exist(createdUser);
                 createdUser.has('uuid').should.equal(true);
@@ -495,10 +464,6 @@ describe('User Model', function run() {
             var userData = testUtils.DataGenerator.forModel.users[4],
                 userId;
 
-            sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
-                return Promise.resolve(userData);
-            });
-
             UserModel.add(_.extend({}, userData, {status: 'invited'}), context).then(function (createdUser) {
                 should.exist(createdUser);
                 createdUser.has('uuid').should.equal(true);

core/test/unit/server_spec.js

@@ -1,7 +1,7 @@
 /*globals describe, it*/
 /*jshint expr:true*/
 var should          = require('should'),
-    request         = require('request'),
+    http            = require('http'),
     config          = require('../../../config');
 
 describe('Server', function () {
@@ -10,11 +10,12 @@ describe('Server', function () {
         url = 'http://' + host + ':' + port;
 
     it('should not start a connect server when required', function (done) {
-        request(url, function (error, response, body) {
+        http.get(url, function () {
-            should(response).equal(undefined);
+            done('This request should not have worked');
-            should(body).equal(undefined);
+        }).on('error', function (error) {
             should(error).not.equal(undefined);
             should(error.code).equal('ECONNREFUSED');
+
             done();
         });
     });

core/test/unit/server_utils_spec.js

@@ -1,11 +1,13 @@
-/*globals describe, it*/
+/*globals describe, it, beforeEach, afterEach*/
 /*jshint expr:true*/
 var should          = require('should'),
     sinon           = require('sinon'),
+    nock            = require('nock'),
     parsePackageJson = require('../../server/utils/parse-package-json'),
     validateThemes  = require('../../server/utils/validate-themes'),
     readDirectory   = require('../../server/utils/read-directory'),
     readThemes      = require('../../server/utils/read-themes'),
+    gravatar        = require('../../server/utils/gravatar'),
     tempfile        = require('../utils/tempfile'),
     utils           = require('../../server/utils'),
     join            = require('path').join,
@@ -457,6 +459,61 @@ describe('Server Utilities', function () {
         });
     });
 
+    describe('gravatar-lookup', function () {
+        var currentEnv = process.env.NODE_ENV;
+
+        beforeEach(function () {
+            // give environment a value that will call gravatar
+            process.env.NODE_ENV = 'production';
+        });
+
+        afterEach(function () {
+            // reset the environment
+            process.env.NODE_ENV = currentEnv;
+        });
+
+        it('can successfully lookup a gravatar url', function (done) {
+            nock('https://www.gravatar.com')
+                .get('/avatar/ef6dcde5c99bb8f685dd451ccc3e050a?s=250&d=404&r=x')
+                .reply(200);
+
+            gravatar.lookup({email: 'exists@example.com'}).then(function (result) {
+                should.exist(result);
+                should.exist(result.image);
+                result.image.should.eql('//www.gravatar.com/avatar/ef6dcde5c99bb8f685dd451ccc3e050a?s=250&d=mm&r=x');
+
+                done();
+            }).catch(done);
+        });
+
+        it('can handle a non existant gravatar', function (done) {
+            nock('https://www.gravatar.com')
+                .get('/avatar/3a2963a39ebba98fb0724a1db2f13d63?s=250&d=404&r=x')
+                .reply(404);
+
+            gravatar.lookup({email: 'invalid@example.com'}).then(function (result) {
+                should.exist(result);
+                should.not.exist(result.image);
+
+                done();
+            }).catch(done);
+        });
+
+        it('will timeout', function (done) {
+            nock('https://www.gravatar.com')
+                .get('/avatar/ef6dcde5c99bb8f685dd451ccc3e050a?s=250&d=404&r=x')
+                .delay(11)
+                .reply(200);
+
+            gravatar.lookup({email: 'exists@example.com'}, 10).then(function (result) {
+                should.exist(result);
+                should.not.exist(result.image);
+
+                done();
+            }).catch(done);
+        });
+    });
+
     describe('redirect301', function () {
         it('performs a 301 correctly', function (done) {
             var res = {};

package.json

@@ -59,7 +59,6 @@
     "passport-http-bearer": "1.0.1",
     "passport-oauth2-client-password": "0.1.2",
     "path-match": "1.2.3",
-    "request": "2.69.0",
     "rss": "1.2.1",
     "semver": "5.1.0",
     "showdown-ghost": "0.3.6",