From 78ef54baf2eb584008e2f5798cb0654b92bb9bcc Mon Sep 17 00:00:00 2001 From: Damian Bushong Date: Fri, 5 Jul 2013 15:55:58 -0500 Subject: [PATCH 1/4] Cleanup user.js for style guide compliance --- src/user.js | 74 ++++++++++++++++++++++++----------------------------- 1 file changed, 33 insertions(+), 41 deletions(-) diff --git a/src/user.js b/src/user.js index 1adb0c65a4..c2fec6851e 100644 --- a/src/user.js +++ b/src/user.js @@ -12,26 +12,25 @@ var utils = require('./../public/src/utils.js'), (function(User) { User.getUserField = function(uid, field, callback) { RDB.hget('user:' + uid, field, function(err, data) { - if(err === null) + if(err === null) { callback(data); - else + } else { console.log(err); + } }); } User.getUserFields = function(uid, fields, callback) { RDB.hmget('user:' + uid, fields, function(err, data) { - if(err === null) { - var returnData = {}; - - for(var i=0, ii=fields.length; i 150) { callback({error:'Signature can\'t be longer than 150 characters!'}); return; } - - - for(var i=0,ii=fields.length; i Date: Fri, 5 Jul 2013 16:02:59 -0500 Subject: [PATCH 2/4] Force a default gravatar if no email provided Gravatar provides the forceDefault option, let's use it. --- src/user.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/user.js b/src/user.js index c2fec6851e..d0ce0a7e9a 100644 --- a/src/user.js +++ b/src/user.js @@ -224,13 +224,14 @@ var utils = require('./../public/src/utils.js'), }; User.createGravatarURLFromEmail = function(email) { + var forceDefault = '' if (!email) { - email = utils.generateUUID(); + email = '0000', + forceDefault = '&forceDefault=y'; } - var md5sum = crypto.createHash('md5'); - md5sum.update(email.toLowerCase().trim()); - var gravatarURL = 'http://www.gravatar.com/avatar/' + md5sum.digest('hex') + '?default=identicon&s=128'; - return gravatarURL; + var emailHash = crypto.createHash('md5').update(email.toLowerCase().trim()).digest('hex'); + // @todo: https asset support? + return 'http://www.gravatar.com/avatar/' + emailHash + '?default=identicon&s=128' + forceDefault; } User.hashPassword = function(password, callback) { From f61e71729a70a8532fcac390750fda2d171cbc1a Mon Sep 17 00:00:00 2001 From: Damian Bushong Date: Fri, 5 Jul 2013 16:25:51 -0500 Subject: [PATCH 3/4] More cleanup of user.js See added comments for more information on problems discovered within the codebase. --- src/user.js | 280 +++++++++++++++++++++++++++++++++------------------- 1 file changed, 180 insertions(+), 100 deletions(-) diff --git a/src/user.js b/src/user.js index d0ce0a7e9a..e5db73a924 100644 --- a/src/user.js +++ b/src/user.js @@ -240,6 +240,9 @@ var utils = require('./../public/src/utils.js'), return; } + // round count should be variable somewhere instead of hardcoded here + // if an admin has the resources to up the round count, then making it easy for them to do so + // can't hurt bcrypt.genSalt(10, function(err, salt) { bcrypt.hash(password, salt, function(err, hash) { callback(hash); @@ -254,7 +257,7 @@ var utils = require('./../public/src/utils.js'), } RDB.keys('username:*'+ username + '*:uid', function(err, keys) { - if(err === null) { + if(!err) { if(keys && keys.length) { RDB.mget(keys, function(err, uids) { User.getDataForUsers(uids, function(userdata) { @@ -272,7 +275,7 @@ var utils = require('./../public/src/utils.js'), User.onNewPostMade = function(uid, tid, pid, timestamp) { User.addPostIdToUser(uid, pid) - + User.incrementUserFieldBy(uid, 'postcount', 1); User.setUserField(uid, 'lastposttime', timestamp); @@ -289,13 +292,12 @@ var utils = require('./../public/src/utils.js'), User.getPostIds = function(uid, start, end, callback) { RDB.lrange('uid:' + uid + ':posts', start, end, function(err, pids) { - if(err === null) { + if(!err) { if(pids && pids.length) callback(pids); else callback([]); - } - else { + } else { console.log(err); callback([]); } @@ -334,69 +336,78 @@ var utils = require('./../public/src/utils.js'), }); emailjsServer.send(message, function(err, success) { - if (err) + if (err) { console.log(err); + } }); } } User.follow = function(uid, followid, callback) { - RDB.sadd('following:'+uid, followid, function(err, data) { - if(err === null) { - RDB.sadd('followers:'+followid, uid, function(err, data) { + RDB.sadd('following:' + uid, followid, function(err, data) { + if(!err) { + RDB.sadd('followers:' + followid, uid, function(err, data) { callback(data); }); - } - else + } else { console.log(err); + } }); } User.unfollow = function(uid, unfollowid, callback) { - RDB.srem('following:'+uid, unfollowid, function(err, data){ - if(err === null) { - RDB.srem('followers:'+unfollowid, uid, function(err, data){ + RDB.srem('following:' + uid, unfollowid, function(err, data){ + if(!err) { + RDB.srem('followers:' + unfollowid, uid, function(err, data){ callback(data); }); - } - else + } else { console.log(err); + } }); } User.getFollowing = function(uid, callback) { - RDB.smembers('following:'+uid, function(err, userIds) { - if(err === null) + RDB.smembers('following:' + uid, function(err, userIds) { + if(!err) { User.getDataForUsers(userIds, callback); - else - console.log(err); + } else { + console.log(err); + } }); } User.getFollowers = function(uid, callback) { - RDB.smembers('followers:'+uid, function(err, userIds) { - if(err === null) + RDB.smembers('followers:' + uid, function(err, userIds) { + if(!err) { User.getDataForUsers(userIds, callback); - else + } else { console.log(err); + } }); } User.getFollowingCount = function(uid, callback) { - RDB.smembers('following:'+uid, function(err, userIds) { - if(err === null) + RDB.smembers('following:' + uid, function(err, userIds) { + if(!err) { callback(userIds.length); - else + } else { console.log(err); + } }); } User.getFollowerCount = function(uid, callback) { - RDB.smembers('followers:'+uid, function(err, userIds) { - if(err === null) + RDB.smembers('followers:' + uid, function(err, userIds) { + // @note why are error-handling styles being mixed? + // either go with not-error-dosomething-else-dosomethingelse, or + // go with if-error-dosomething-return + // also why is console.log(err) being used when below we're using RDB.handle()? + if(!err) { callback(userIds.length); - else - console.log(err); + } else { + console.log(err); + } }); } @@ -408,23 +419,21 @@ var utils = require('./../public/src/utils.js'), return; } - for(var i=0, ii=userIds.length; i= uids.length) + if(usernames.length >= uids.length) { callback(usernames); + } }); } } @@ -508,51 +527,65 @@ var utils = require('./../public/src/utils.js'), User.get_userslugs_by_uids = function(uids, callback) { var userslugs = []; - if (!Array.isArray(uids)) return callback([]); + if (!Array.isArray(uids)) { + return callback([]); + } + // @todo - rework this logic. it doesn't make much sense when you're going through + // each and then placing the check logic into the innermost callback. + // this is probably a situation where an async.method is ideal for(var i=0, ii=uids.length; i= uids.length) + if(userslugs.length >= uids.length) { callback(userslugs); + } }); } } User.get_uid_by_email = function(email, callback) { RDB.get('email:' + email + ':uid', function(err, data) { - RDB.handle(err); + if (err) { + RDB.handle(err); + } callback(data); }); }; User.get_uid_by_session = function(session, callback) { RDB.get('sess:' + session + ':uid', function(err, data) { - RDB.handle(err); + if (err) { + RDB.handle(err); + } callback(data); }); }; User.get_uid_by_twitter_id = function(twid, callback) { RDB.hget('twid:uid', twid, function(err, uid) { - RDB.handle(err); + if (err) { + RDB.handle(err); + } callback(uid); }); } User.get_uid_by_google_id = function(gplusid, callback) { RDB.hget('gplusid:uid', gplusid, function(err, uid) { - RDB.handle(err); + if (err) { + RDB.handle(err); + } callback(uid); }); } User.get_uid_by_fbid = function(fbid, callback) { RDB.hget('fbid:uid', fbid, function(err, uid) { - RDB.handle(err); + if (err) { + RDB.handle(err); + } callback(uid); }); } @@ -560,7 +593,9 @@ var utils = require('./../public/src/utils.js'), User.session_ping = function(sessionID, uid) { // Start, replace, or extend a session RDB.get('sess:' + sessionID, function(err, session) { - RDB.handle(err); + if (err) { + RDB.handle(err); + } var expiry = 60*60*24*14, // Login valid for two weeks sess_key = 'sess:' + sessionID + ':uid', @@ -575,61 +610,84 @@ var utils = require('./../public/src/utils.js'), User.isModerator = function(uid, cid, callback) { RDB.sismember('cid:' + cid + ':moderators', uid, function(err, exists) { + // @todo handle error callback(!!exists); }); } User.isAdministrator = function(uid, callback) { RDB.sismember('administrators', uid, function(err, exists) { + // @todo handle error callback(!!exists); }); } User.makeAdministrator = function(uid, callback) { RDB.sadd('administrators', uid, function(err, data){ - if(err === null) { + if(!err) { User.setUserField(uid, 'administrator', 1); } - if(callback) + + if(callback) { + // @todo address why we're only sending back a boolean in the callback and not an error if it occurred callback(err === null); + } }); } User.removeAdministrator = function(uid, callback) { RDB.srem('administrators', uid, function(err, data){ - if(err === null) { + if(!err) { User.setUserField(uid, 'administrator', 0); } - if(callback) + + if(callback) { + // @todo address why we're only sending back a boolean in the callback and not an error if it occurred callback(err === null); + } }); } User.reset = { validate: function(socket, code, callback) { - if (typeof callback !== 'function') callback = undefined; + if (typeof callback !== 'function') { + callback = null; + } RDB.get('reset:' + code + ':uid', function(err, uid) { - RDB.handle(err); + if (err) { + RDB.handle(err); + } if (uid !== null) { RDB.get('reset:' + code + ':expiry', function(err, expiry) { - RDB.handle(err); + if (err) { + RDB.handle(err); + } - if (expiry >= +new Date()/1000|0) { - if (!callback) socket.emit('user:reset.valid', { valid: true }); - else callback(true); + if (expiry >= +Date.now()/1000|0) { + if (!callback) { + socket.emit('user:reset.valid', { valid: true }); + } else { + callback(true); + } } else { // Expired, delete from db RDB.del('reset:' + code + ':uid'); RDB.del('reset:' + code + ':expiry'); - if (!callback) socket.emit('user:reset.valid', { valid: false }); - else callback(false); + if (!callback) { + socket.emit('user:reset.valid', { valid: false }); + } else { + callback(false); + } } }); } else { - if (!callback) socket.emit('user:reset.valid', { valid: false }); - else callback(false); + if (!callback) { + socket.emit('user:reset.valid', { valid: false }); + } else { + callback(false); + } } }); }, @@ -670,6 +728,7 @@ var utils = require('./../public/src/utils.js'), status: "error", message: "send-failed" }); + // @todo handle error properly throw new Error(err); } }); @@ -686,7 +745,9 @@ var utils = require('./../public/src/utils.js'), this.validate(code, function(validated) { if (validated) { RDB.get('reset:' + code + ':uid', function(err, uid) { - RDB.handle(err); + if (err) { + RDB.handle(err); + } User.setUserField(uid, 'password', password); RDB.del('reset:' + code + ':uid'); @@ -703,24 +764,25 @@ var utils = require('./../public/src/utils.js'), exists: function(socket, email, callback) { User.get_uid_by_email(email, function(exists) { exists = !!exists; - if (typeof callback !== 'function') socket.emit('user.email.exists', { exists: exists }); - else callback(exists); + if (typeof callback !== 'function') { + socket.emit('user.email.exists', { exists: exists }); + } else { + callback(exists); + } }); }, confirm: function(code, callback) { RDB.get('confirm:' + code + ':email', function(err, email) { - RDB.handle(err); + if (err) { + RDB.handle(err); + } if (email !== null) { RDB.set('email:' + email + ':confirm', true); RDB.del('confirm:' + code + ':email'); - callback({ - status: 'ok' - }); + callback({ status: 'ok' }); } else { - callback({ - status: 'not_ok' - }); + callback({ status: 'not_ok' }); } }); } @@ -728,19 +790,24 @@ var utils = require('./../public/src/utils.js'), User.get_online_users = function(socket, uids) { RDB.sismembers('users:online', uids, function(err, data) { + // @todo handle err socket.emit('api:user.get_online_users', data); }); }; User.go_online = function(uid) { RDB.sadd('users:online', uid, function(err) { - if (err) RDB.handle(err); + if (err) { + RDB.handle(err); + } }); }; User.go_offline = function(uid) { RDB.srem('users:online', uid, function(err) { - if (err) RDB.handle(err); + if (err) { + RDB.handle(err); + } }); }; @@ -749,7 +816,7 @@ var utils = require('./../public/src/utils.js'), get_record : function(socket) { RDB.mget(['global:active_user_record', 'global:active_user_record_date'], function(err, data) { RDB.handle(err); - socket.emit('api:user.active.get_record', {record: data[0], timestamp: data[1]}); + socket.emit('api:user.active.get_record', { record: data[0], timestamp: data[1] }); }); }, @@ -821,6 +888,7 @@ var utils = require('./../public/src/utils.js'), async.parallel({ unread: function(next) { RDB.zrevrangebyscore('uid:' + uid + ':notifications:unread', 10, 0, function(err, nids) { + // @todo handle err var unread = []; if (nids && nids.length > 0) { async.eachSeries(nids, function(nid, next) { @@ -831,11 +899,14 @@ var utils = require('./../public/src/utils.js'), }, function(err) { next(null, unread); }); - } else next(null, unread); + } else { + next(null, unread); + } }); }, read: function(next) { RDB.zrevrangebyscore('uid:' + uid + ':notifications:read', 10, 0, function(err, nids) { + // @todo handle err var read = []; if (nids && nids.length > 0) { async.eachSeries(nids, function(nid, next) { @@ -846,31 +917,40 @@ var utils = require('./../public/src/utils.js'), }, function(err) { next(null, read); }); - } else next(null, read); + } else { + next(null, read); + } }); } }, function(err, notifications) { // While maintaining score sorting, sort by time notifications.read.sort(function(a, b) { - if (a.score === b.score) return (a.datetime - b.datetime) > 0 ? -1 : 1; + if (a.score === b.score) { + return (a.datetime - b.datetime) > 0 ? -1 : 1; + } }); notifications.unread.sort(function(a, b) { - if (a.score === b.score) return (a.datetime - b.datetime) > 0 ? -1 : 1; + if (a.score === b.score) { + return (a.datetime - b.datetime) > 0 ? -1 : 1; + } }); callback(notifications); }); }, hasFlag: function(uid, callback) { RDB.get('uid:1:notifications:flag', function(err, flag) { - if (err) RDB.handle(err); + if (err) { + RDB.handle(err); + } - if (flag === '1') callback(true); - else callback(false); + callback(flag === 1); }); }, removeFlag: function(uid) { RDB.del('uid:' + uid + ':notifications:flag', function(err) { - if (err) RDB.handle(err); + if (err) { + RDB.handle(err); + } }); } } From e3b4a6029cd4e6d665ebce7419042849dc590752 Mon Sep 17 00:00:00 2001 From: Damian Bushong Date: Fri, 5 Jul 2013 16:37:45 -0500 Subject: [PATCH 4/4] Clean up excess end-of-line whitespace --- src/user.js | 76 ++++++++++++++++++++++++++--------------------------- 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/src/user.js b/src/user.js index e5db73a924..bdd85914c7 100644 --- a/src/user.js +++ b/src/user.js @@ -19,10 +19,10 @@ var utils = require('./../public/src/utils.js'), } }); } - + User.getUserFields = function(uid, fields, callback) { RDB.hmget('user:' + uid, fields, function(err, data) { - if(err === null) { + if(err === null) { for(var i = 0, returnData = {}, ii=fields.length; i