Skip to content

Commit c301510

Browse files
ErisDSErisDS
committed
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
1 parent 3c5c5ad commit c301510

File tree

8 files changed

+126
-88
lines changed

8 files changed

+126
-88
lines changed

‎Gruntfile.js

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,11 @@
88
var _ = require('lodash'),
99
chalk = require('chalk'),
1010
fs = require('fs-extra'),
11+
https = require('https'),
1112
moment = require('moment'),
1213
getTopContribs = require('top-gh-contribs'),
1314
path = require('path'),
1415
Promise = require('bluebird'),
15-
request = require('request'),
1616

1717
escapeChar = process.platform.match(/^win/) ? '^' : '\\',
1818
cwd = process.cwd().replace(/( |\(|\))/g, escapeChar + '$1'),
@@ -834,15 +834,17 @@ var _ = require('lodash'),
834834
).then(function (results) {
835835
var contributors = results[1],
836836
contributorTemplate = '<article>\n <a href="<%githubUrl%>" title="<%name%>">\n' +
837-
' <img src="{{gh-path "admin" "/img/contributors"}}/<%name%>" alt="<%name%>" />\n' +
838-
' </a>\n</article>',
837+
' <img src="{{gh-path "admin" "/img/contributors"}}/<%name%>" alt="<%name%>" />\n' +
838+
' </a>\n</article>',
839839

840840
downloadImagePromise = function (url, name) {
841841
return new Promise(function (resolve, reject) {
842-
request(url)
843-
.pipe(fs.createWriteStream(imagePath + name))
844-
.on('close', resolve)
845-
.on('error', reject);
842+
https.get(url, function (res) {
843+
fs.writeFile(imagePath + name, res, function () {
844+
resolve();
845+
});
846+
})
847+
.on('error', reject);
846848
});
847849
};
848850

‎core/server/models/user.js

Lines changed: 6 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,12 @@ var _ = require('lodash'),
22
Promise = require('bluebird'),
33
errors = require('../errors'),
44
utils = require('../utils'),
5+
gravatar = require('../utils/gravatar'),
56
bcrypt = require('bcryptjs'),
67
ghostBookshelf = require('./base'),
78
crypto = require('crypto'),
89
validator = require('validator'),
9-
request = require('request'),
1010
validation = require('../data/validation'),
11-
config = require('../config'),
1211
events = require('../events'),
1312
i18n = require('../i18n'),
1413

@@ -380,7 +379,7 @@ User = ghostBookshelf.Model.extend({
380379
// Assign the hashed password
381380
userData.password = hash;
382381
// LookupGravatar
383-
return self.gravatarLookup(userData);
382+
return gravatar.lookup(userData);
384383
}).then(function then(userData) {
385384
// Save the user with the hashed password
386385
return ghostBookshelf.Model.add.call(self, userData, options);
@@ -423,8 +422,10 @@ User = ghostBookshelf.Model.extend({
423422
// Assign the hashed password
424423
userData.password = hash;
425424

426-
return Promise.join(self.gravatarLookup(userData),
427-
ghostBookshelf.Model.generateSlug.call(this, User, userData.name, options));
425+
return Promise.join(
426+
gravatar.lookup(userData),
427+
ghostBookshelf.Model.generateSlug.call(this, User, userData.name, options)
428+
);
428429
}).then(function then(results) {
429430
userData = results[0];
430431
userData.slug = results[1];
@@ -776,31 +777,6 @@ User = ghostBookshelf.Model.extend({
776777
});
777778
},
778779

779-
gravatarLookup: function gravatarLookup(userData) {
780-
var gravatarUrl = '//www.gravatar.com/avatar/' +
781-
crypto.createHash('md5').update(userData.email.toLowerCase().trim()).digest('hex') +
782-
'?s=250';
783-
784-
return new Promise(function gravatarRequest(resolve) {
785-
if (config.isPrivacyDisabled('useGravatar')) {
786-
return resolve(userData);
787-
}
788-
789-
request({url: 'http:' + gravatarUrl + '&d=404&r=x', timeout: 2000}, function handler(err, response) {
790-
if (err) {
791-
// just resolve with no image url
792-
return resolve(userData);
793-
}
794-
795-
if (response.statusCode !== 404) {
796-
gravatarUrl += '&d=mm&r=x';
797-
userData.image = gravatarUrl;
798-
}
799-
800-
resolve(userData);
801-
});
802-
});
803-
},
804780
// Get the user by email address, enforces case insensitivity rejects if the user is not found
805781
// When multi-user support is added, email addresses must be deduplicated with case insensitivity, so that
806782
// joe@bloggs.com and JOE@BLOGGS.COM cannot be created as two separate users.

‎core/server/utils/gravatar.js

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
var Promise = require('bluebird'),
2+
config = require('../config'),
3+
crypto = require('crypto'),
4+
https = require('https');
5+
6+
module.exports.lookup = function lookup(userData, timeout) {
7+
var gravatarUrl = '//www.gravatar.com/avatar/' +
8+
crypto.createHash('md5').update(userData.email.toLowerCase().trim()).digest('hex') +
9+
'?s=250';
10+
11+
return new Promise(function gravatarRequest(resolve) {
12+
if (config.isPrivacyDisabled('useGravatar') || process.env.NODE_ENV.indexOf('testing') > -1) {
13+
return resolve(userData);
14+
}
15+
16+
var request, timer, timerEnded = false;
17+
18+
request = https.get('https:' + gravatarUrl + '&d=404&r=x', function (response) {
19+
clearTimeout(timer);
20+
if (response.statusCode !== 404 && !timerEnded) {
21+
gravatarUrl += '&d=mm&r=x';
22+
userData.image = gravatarUrl;
23+
}
24+
25+
resolve(userData);
26+
});
27+
28+
request.on('error', function () {
29+
clearTimeout(timer);
30+
// just resolve with no image url
31+
if (!timerEnded) {
32+
return resolve(userData);
33+
}
34+
});
35+
36+
timer = setTimeout(function () {
37+
timerEnded = true;
38+
request.abort();
39+
return resolve(userData);
40+
}, timeout || 2000);
41+
});
42+
};

‎core/test/integration/api/api_users_spec.js

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ var testUtils = require('../../utils'),
77
_ = require('lodash'),
88

99
// Stuff we are testing
10-
ModelUser = require('../../../server/models'),
10+
models = require('../../../server/models'),
1111
UserAPI = require('../../../server/api/users'),
1212
mail = require('../../../server/api/mail'),
1313

@@ -39,7 +39,7 @@ describe('Users API', function () {
3939
it('dateTime fields are returned as Date objects', function (done) {
4040
var userData = testUtils.DataGenerator.forModel.users[0];
4141

42-
ModelUser.User.check({email: userData.email, password: userData.password}).then(function (user) {
42+
models.User.check({email: userData.email, password: userData.password}).then(function (user) {
4343
return UserAPI.read({id: user.id});
4444
}).then(function (response) {
4545
response.users[0].created_at.should.be.an.instanceof(Date);
@@ -482,7 +482,7 @@ describe('Users API', function () {
482482
UserAPI.edit(
483483
{users: [{name: 'newname', password: 'newpassword'}]}, _.extend({}, context.author, {id: userIdFor.author})
484484
).then(function () {
485-
return ModelUser.User.findOne({id: userIdFor.author}).then(function (response) {
485+
return models.User.findOne({id: userIdFor.author}).then(function (response) {
486486
response.get('name').should.eql('newname');
487487
response.get('password').should.not.eql('newpassword');
488488
done();
@@ -497,10 +497,6 @@ describe('Users API', function () {
497497
beforeEach(function () {
498498
newUser = _.clone(testUtils.DataGenerator.forKnex.createUser(testUtils.DataGenerator.Content.users[4]));
499499

500-
sandbox.stub(ModelUser.User, 'gravatarLookup', function (userData) {
501-
return Promise.resolve(userData);
502-
});
503-
504500
sandbox.stub(mail, 'send', function () {
505501
return Promise.resolve();
506502
});

‎core/test/integration/model/model_users_spec.js

Lines changed: 3 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ var testUtils = require('../../utils'),
99

1010
// Stuff we are testing
1111
utils = require('../../../server/utils'),
12+
gravatar = require('../../../server/utils/gravatar'),
1213
UserModel = require('../../../server/models/user').User,
1314
RoleModel = require('../../../server/models/role').Role,
1415
events = require('../../../server/events'),
@@ -38,10 +39,6 @@ describe('User Model', function run() {
3839
it('can add first', function (done) {
3940
var userData = testUtils.DataGenerator.forModel.users[0];
4041

41-
sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
42-
return Promise.resolve(userData);
43-
});
44-
4542
UserModel.add(userData, context).then(function (createdUser) {
4643
should.exist(createdUser);
4744
createdUser.has('uuid').should.equal(true);
@@ -55,10 +52,6 @@ describe('User Model', function run() {
5552
it('shortens slug if possible', function (done) {
5653
var userData = testUtils.DataGenerator.forModel.users[2];
5754

58-
sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
59-
return Promise.resolve(userData);
60-
});
61-
6255
UserModel.add(userData, context).then(function (createdUser) {
6356
should.exist(createdUser);
6457
createdUser.has('slug').should.equal(true);
@@ -70,10 +63,6 @@ describe('User Model', function run() {
7063
it('does not short slug if not possible', function (done) {
7164
var userData = testUtils.DataGenerator.forModel.users[2];
7265

73-
sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
74-
return Promise.resolve(userData);
75-
});
76-
7766
UserModel.add(userData, context).then(function (createdUser) {
7867
should.exist(createdUser);
7968
createdUser.has('slug').should.equal(true);
@@ -99,10 +88,6 @@ describe('User Model', function run() {
9988
it('does NOT lowercase email', function (done) {
10089
var userData = testUtils.DataGenerator.forModel.users[2];
10190

102-
sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
103-
return Promise.resolve(userData);
104-
});
105-
10691
UserModel.add(userData, context).then(function (createdUser) {
10792
should.exist(createdUser);
10893
createdUser.has('uuid').should.equal(true);
@@ -114,7 +99,7 @@ describe('User Model', function run() {
11499
it('can find gravatar', function (done) {
115100
var userData = testUtils.DataGenerator.forModel.users[4];
116101

117-
sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
102+
sandbox.stub(gravatar, 'lookup', function (userData) {
118103
userData.image = 'http://www.gravatar.com/avatar/2fab21a4c4ed88e76add10650c73bae1?d=404';
119104
return Promise.resolve(userData);
120105
});
@@ -132,7 +117,7 @@ describe('User Model', function run() {
132117
it('can handle no gravatar', function (done) {
133118
var userData = testUtils.DataGenerator.forModel.users[0];
134119

135-
sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
120+
sandbox.stub(gravatar, 'lookup', function (userData) {
136121
return Promise.resolve(userData);
137122
});
138123

@@ -336,10 +321,6 @@ describe('User Model', function run() {
336321
it('can invite user', function (done) {
337322
var userData = testUtils.DataGenerator.forModel.users[4];
338323

339-
sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
340-
return Promise.resolve(userData);
341-
});
342-
343324
UserModel.add(_.extend({}, userData, {status: 'invited'}), context).then(function (createdUser) {
344325
should.exist(createdUser);
345326
createdUser.has('uuid').should.equal(true);
@@ -356,10 +337,6 @@ describe('User Model', function run() {
356337
it('can add active user', function (done) {
357338
var userData = testUtils.DataGenerator.forModel.users[4];
358339

359-
sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
360-
return Promise.resolve(userData);
361-
});
362-
363340
RoleModel.findOne().then(function (role) {
364341
userData.roles = [role.toJSON()];
365342

@@ -406,10 +383,6 @@ describe('User Model', function run() {
406383
var userData = testUtils.DataGenerator.forModel.users[4],
407384
userId;
408385

409-
sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
410-
return Promise.resolve(userData);
411-
});
412-
413386
UserModel.add(_.extend({}, userData, {status: 'invited'}), context).then(function (createdUser) {
414387
should.exist(createdUser);
415388
createdUser.has('uuid').should.equal(true);
@@ -436,10 +409,6 @@ describe('User Model', function run() {
436409
var userData = testUtils.DataGenerator.forModel.users[4],
437410
userId;
438411

439-
sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
440-
return Promise.resolve(userData);
441-
});
442-
443412
UserModel.add(_.extend({}, userData, {status: 'invited'}), context).then(function (createdUser) {
444413
should.exist(createdUser);
445414
createdUser.has('uuid').should.equal(true);
@@ -495,10 +464,6 @@ describe('User Model', function run() {
495464
var userData = testUtils.DataGenerator.forModel.users[4],
496465
userId;
497466

498-
sandbox.stub(UserModel, 'gravatarLookup', function (userData) {
499-
return Promise.resolve(userData);
500-
});
501-
502467
UserModel.add(_.extend({}, userData, {status: 'invited'}), context).then(function (createdUser) {
503468
should.exist(createdUser);
504469
createdUser.has('uuid').should.equal(true);

‎core/test/unit/server_spec.js

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/*globals describe, it*/
22
/*jshint expr:true*/
33
var should = require('should'),
4-
request = require('request'),
4+
http = require('http'),
55
config = require('../../../config');
66

77
describe('Server', function () {
@@ -10,11 +10,12 @@ describe('Server', function () {
1010
url = 'http://' + host + ':' + port;
1111

1212
it('should not start a connect server when required', function (done) {
13-
request(url, function (error, response, body) {
14-
should(response).equal(undefined);
15-
should(body).equal(undefined);
13+
http.get(url, function () {
14+
done('This request should not have worked');
15+
}).on('error', function (error) {
1616
should(error).not.equal(undefined);
1717
should(error.code).equal('ECONNREFUSED');
18+
1819
done();
1920
});
2021
});

0 commit comments

Comments
 (0)