From 2c12062ce05485cd180883c567e5bbb5726a74bd Mon Sep 17 00:00:00 2001 From: Damian Bushong Date: Fri, 5 Jul 2013 12:51:56 -0500 Subject: [PATCH 1/6] Update user.js - Do not use a temp var here, that's just wasting memory since we're only using it once (and it isn't helping readability all that much) - Add notes about console.error use, indicate it's temporary and needs replaced with proper logging - Indicate that resizing code should be split out into another process (perhaps with node's built in cluster module? or child_process.spawn?) - Do not use `res.send` with JSON data; use res.json - Use consistent spacing and whitespace usage --- src/routes/user.js | 83 +++++++++++++++++++--------------------------- 1 file changed, 35 insertions(+), 48 deletions(-) diff --git a/src/routes/user.js b/src/routes/user.js index d4111570de..9576ef39e1 100644 --- a/src/routes/user.js +++ b/src/routes/user.js @@ -1,4 +1,3 @@ - var user = require('./../user.js'), posts = require('./../posts.js'), fs = require('fs'), @@ -15,10 +14,13 @@ var user = require('./../user.js'), return res.redirect('/404'); user.getUserData(req.params.uid, function(data){ - if(data) + if(data) { res.send(data); - else + } else { res.send("User doesn't exist!"); + } + + res.send(data); }); }); @@ -44,7 +46,6 @@ var user = require('./../user.js'), }); app.get('/users/:userslug', function(req, res) { - if(!req.params.userslug) { res.send("User doesn't exist!"); return; @@ -59,8 +60,7 @@ var user = require('./../user.js'), user.getUserData(uid, function(userdata) { if(userdata) { res.send(app.build_header(res) + app.create_route('users/'+userdata.userslug, 'account') + templates['footer']); - } - else { + } else { res.redirect('/404'); } }); @@ -68,21 +68,20 @@ var user = require('./../user.js'), }); app.get('/users/:userslug/edit', function(req, res){ - if(!req.user) return res.redirect('/403'); user.getUserField(req.user.uid, 'userslug', function(userslug) { - if(req.params.userslug && userslug === req.params.userslug) + if(req.params.userslug && userslug === req.params.userslug) { res.send(app.build_header(res) + app.create_route('users/'+req.params.userslug+'/edit','accountedit') + templates['footer']); - else + } else { return res.redirect('/404'); + } }); }); app.post('/users/doedit', function(req, res){ - if(!req.user) return res.redirect('/403'); @@ -96,7 +95,6 @@ var user = require('./../user.js'), }); app.post('/users/uploadpicture', function(req, res) { - if(!req.user) return res.redirect('/403'); @@ -108,9 +106,8 @@ var user = require('./../user.js'), } var allowedTypes = ['image/png', 'image/jpeg', 'image/jpg', 'image/gif']; - var type = req.files.userPhoto.type; - if(allowedTypes.indexOf(type) === -1) { + if(allowedTypes.indexOf(req.files.userPhoto.type) === -1) { res.send({ error: 'Allowed image types are png, jpg and gif!' }); @@ -128,7 +125,7 @@ var user = require('./../user.js'), fs.unlink(absolutePath, function(err) { if(err) { - console.log(err); + console.error('[%d] %s', Date.now(), + err); } uploadUserPicture(req.user.uid, path.extname(req.files.userPhoto.name), req.files.userPhoto.path, res); @@ -138,7 +135,6 @@ var user = require('./../user.js'), }); function uploadUserPicture(uid, extension, tempPath, res) { - if(!extension) { res.send({ error: 'Error uploading file! Error : Invalid extension!' @@ -149,6 +145,7 @@ var user = require('./../user.js'), var filename = uid + '-profileimg' + extension; var uploadPath = path.join(global.configuration['ROOT_DIRECTORY'], config.upload_path, filename); + // @todo move to proper logging code - this should only be temporary console.log('Info: Attempting upload to: '+ uploadPath); var is = fs.createReadStream(tempPath); @@ -159,9 +156,7 @@ var user = require('./../user.js'), var imageUrl = config.upload_url + filename; - res.send({ - path: imageUrl - }); + res.json({ path: imageUrl }); user.setUserField(uid, 'uploadedpicture', imageUrl); user.setUserField(uid, 'picture', imageUrl); @@ -174,14 +169,18 @@ var user = require('./../user.js'), width: 128 }, function(err, stdout, stderr) { if (err) { - console.log(err); + // @todo: better logging method; for now, send to stderr. + // ideally, this should be happening in another process + // to avoid poisoning the main process on error or allowing a significant problem + // to crash the main process + console.error('[%d] %s', Date.now(), + err); } }); }); os.on('error', function(err) { - console.log(err); + console.error('[%d] %s', Date.now(), + err); }); is.pipe(os); @@ -217,7 +216,7 @@ var user = require('./../user.js'), return res.redirect('/'); user.follow(req.user.uid, req.body.uid, function(data) { - res.send({data:data}); + res.json({ data:data }); }); }); @@ -229,7 +228,7 @@ var user = require('./../user.js'), return res.redirect('/'); user.unfollow(req.user.uid, req.body.uid, function(data) { - res.send({data:data}); + res.json({ data:data }); }); }); @@ -251,20 +250,18 @@ var user = require('./../user.js'), function api_method(req, res) { - var callerUID = req.user?req.user.uid : 0; + var callerUID = req.user ? req.user.uid : 0; if (!req.params.section && !req.params.userslug) { user.getUserList(function(data) { data = data.sort(function(a, b) { return b.joindate - a.joindate; }); - res.json({search_display: 'none', users:data}); + res.json({ search_display: 'none', users: data }); }); } else if(String(req.params.section).toLowerCase() === 'following') { - getUserDataByUserSlug(req.params.userslug, callerUID, function(userData) { - user.getFollowing(userData.uid, function(followingData){ userData.following = followingData; userData.followingCount = followingData.length; @@ -273,9 +270,7 @@ var user = require('./../user.js'), }); } else if(String(req.params.section).toLowerCase() === 'followers') { - getUserDataByUserSlug(req.params.userslug, callerUID, function(userData) { - user.getFollowers(userData.uid, function(followersData){ userData.followers = followersData; userData.followersCount = followersData.length; @@ -289,22 +284,17 @@ var user = require('./../user.js'), }); } else { getUserDataByUserSlug(req.params.userslug, callerUID, function(userData) { - user.isFollowing(callerUID, userData.theirid, function(isFollowing) { - posts.getPostsByUid(userData.theirid, 0, 9, function(posts) { userData.posts = posts; userData.isFollowing = isFollowing; - userData.signature = marked(userData.signature || ''); - res.json(userData); }); }); }); } - } app.get('/api/users/:userslug?/:section?', api_method); @@ -318,16 +308,16 @@ var user = require('./../user.js'), data = data.sort(function(a, b) { return b.postcount - a.postcount; }); - res.json({search_display: 'none', users:data}); + res.json({ search_display: 'none', users:data }); }); } - + function getUsersSortedByReputation(req, res) { user.getUserList(function(data) { data = data.sort(function(a, b) { return b.reputation - a.reputation; }); - res.json({search_display: 'none', users:data}); + res.json({ search_display: 'none', users:data }); }); } @@ -336,28 +326,26 @@ var user = require('./../user.js'), data = data.sort(function(a, b) { return b.joindate - a.joindate; }); - res.json({search_display: 'none', users:data}); + res.json({ search_display: 'none', users:data }); }); } function getUsersForSearch(req, res) { - res.json({search_display: 'block', users: []}); + res.json({ search_display: 'block', users: [] }); } function getUserDataByUserSlug(userslug, callerUID, callback) { - user.get_uid_by_userslug(userslug, function(uid) { - user.getUserData(uid, function(data) { if(data) { - data.joindate = utils.relativeTime(data.joindate); - if(!data.birthday) + if(!data.birthday) { data.age = ''; - else + } else { data.age = new Date().getFullYear() - new Date(data.birthday).getFullYear(); + } data.uid = uid; data.yourid = callerUID; @@ -367,14 +355,13 @@ var user = require('./../user.js'), user.getFollowerCount(uid, function(followerCount) { data.followingCount = followingCount; data.followerCount = followerCount; - callback(data); - }); }); - } - else + } else { + // @todo why is this an empty object? perhaps the callback should expect NULL or undefined instead. callback({}); + } }); }); @@ -385,4 +372,4 @@ var user = require('./../user.js'), }; -}(exports)); \ No newline at end of file +}(exports)); From 8867a4d666cb4118e68ae2e37690717cbd42760a Mon Sep 17 00:00:00 2001 From: Damian Bushong Date: Fri, 5 Jul 2013 13:01:33 -0500 Subject: [PATCH 2/6] Update user.js Fix minor copy-paste mistake --- src/routes/user.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/routes/user.js b/src/routes/user.js index 9576ef39e1..34884afe86 100644 --- a/src/routes/user.js +++ b/src/routes/user.js @@ -19,8 +19,6 @@ var user = require('./../user.js'), } else { res.send("User doesn't exist!"); } - - res.send(data); }); }); From b1e4aecdb05d169f1eeb73917a0ec75bfee7213a Mon Sep 17 00:00:00 2001 From: Julian Lam Date: Fri, 5 Jul 2013 14:06:36 -0400 Subject: [PATCH 3/6] addressed issue #50 with taskbar weirdness --- public/src/modules/taskbar.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/public/src/modules/taskbar.js b/public/src/modules/taskbar.js index dfe6dfc91a..25c828b0cd 100644 --- a/public/src/modules/taskbar.js +++ b/public/src/modules/taskbar.js @@ -22,6 +22,7 @@ define(function() { require([module], function(module) { if (_btn.className.indexOf('active') === -1) { + taskbar.minimizeAll(); module.load(uuid); // Highlight the button @@ -64,6 +65,8 @@ define(function() { btnEl.setAttribute('data-module', module); btnEl.setAttribute('data-uuid', uuid); btnEl.className = options.state || 'active'; + + if (!options.state || options.state === 'active') taskbar.minimizeAll(); taskbar.tasklist.appendChild(btnEl); taskbar.update(); @@ -71,6 +74,9 @@ define(function() { minimize: function(module, uuid) { var btnEl = taskbar.tasklist.querySelector('[data-module="' + module + '"][data-uuid="' + uuid + '"]'); $(btnEl).removeClass('active'); + }, + minimizeAll: function() { + $(taskbar.tasklist.querySelectorAll('.active')).removeClass('active'); } } From 37334e291018fe66c10643aa81437455a67b93f0 Mon Sep 17 00:00:00 2001 From: Julian Lam Date: Fri, 5 Jul 2013 14:33:06 -0400 Subject: [PATCH 4/6] fixing bug where clicking on a taskbar item would cause the page to re-ajaxify (due to href="#"). closed #50 --- public/src/modules/taskbar.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/public/src/modules/taskbar.js b/public/src/modules/taskbar.js index 25c828b0cd..02ba6e7a8c 100644 --- a/public/src/modules/taskbar.js +++ b/public/src/modules/taskbar.js @@ -7,6 +7,7 @@ define(function() { var footerEl = document.getElementById('footer'); taskbar.taskbar = document.createElement('div'); + var jTaskbar = $(taskbar.taskbar); taskbar.taskbar.innerHTML = '
    '; taskbar.taskbar.className = 'taskbar navbar navbar-fixed-bottom'; taskbar.taskbar.id = 'taskbar'; @@ -15,7 +16,7 @@ define(function() { document.body.insertBefore(taskbar.taskbar, footerEl); // Posts bar events - $(taskbar.taskbar).on('click', 'li', function() { + jTaskbar.on('click', 'li', function() { var _btn = this, module = this.getAttribute('data-module'), uuid = this.getAttribute('data-uuid'); @@ -34,6 +35,10 @@ define(function() { }); }); + jTaskbar.on('click', 'li a', function(e) { + e.preventDefault(); + }); + taskbar.initialized = true; }, update: function() { From 651c652df7e321053de2db08a29f32c7d4545759 Mon Sep 17 00:00:00 2001 From: Julian Lam Date: Fri, 5 Jul 2013 14:38:02 -0400 Subject: [PATCH 5/6] closed #65 --- package.json | 1 - 1 file changed, 1 deletion(-) diff --git a/package.json b/package.json index f1a3534c9d..fad6292664 100644 --- a/package.json +++ b/package.json @@ -48,7 +48,6 @@ "less-middleware": "0.1.12", "marked": "0.2.8", "bcrypt": "0.7.5", - "node-gyp": "0.9.5", "async": "0.2.8", "node-imagemagick": "0.1.8", "node-rss": "1.0.1" From ab6639e1304bc8ebb03479dc05d1206fb1b4c62d Mon Sep 17 00:00:00 2001 From: Julian Lam Date: Fri, 5 Jul 2013 14:40:49 -0400 Subject: [PATCH 6/6] closed #64 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index fad6292664..6ec3aa08cb 100644 --- a/package.json +++ b/package.json @@ -56,6 +56,6 @@ "url": "https://github.com/designcreateplay/NodeBB/issues" }, "engines": { - "node": "*" + "node": ">=0.8" } }