comment and refactor

controllers/avatar.js

@@ -1,3 +1,7 @@
+/**
+ * CRUD avatar image (middlewares)
+ */
+
 const sharp = require('sharp'),
       fs = require('fs-extra'),
       path = require('path'),
@@ -10,56 +14,44 @@ const upload = multer({ dest: 'uploads/' });
 
 const models = require(path.resolve('./models'));
 
+/**
+ * GET user's avatar.
+ * Will return either image uploaded by user (cropped and formatted to jpeg),
+ * or default svg
+ */
 async function get(req, res, next) {
   const { username } = req.params;
 
   const usernameExists = await models.user.exists(username);
 
+  // go to 404 if user was not found
+  // TODO perhaps return the error response here, with more specific message
   if (usernameExists !== true) {
     return next();
   }
 
-  const size = _.get(req, 'query.filter.size', 512);
+  // size of avatar (square side length in pixels)
+  const size = _.get(req, 'query.filter.size', 128);
 
   const avatarPath = path.resolve(`./files/avatars/${username}/${size}`);
 
+  // serve the avatar uploaded by user
   res.sendFile(avatarPath, { headers: { 'content-type': 'image/jpeg'} }, async (err) => {
 
     if (err) {
-      if (err.code === 'ENOENT') {
 
-        const defaultPath = path.resolve(`./files/avatars/${username}/default`);
-        const defaultFolderPath = path.resolve(`./files/avatars/${username}`);
+      // if uploaded avatar not found, serve the default avatar
+      if (err.code === 'ENOENT') {
 
-        // try to find default on disk
+        // if default avatar doesn't exist, save it on disk.
         try {
-          await fs.stat(defaultPath);
+          await createDefaultAvatarIfNotExist(username);
         } catch (e) {
-
-          /*
-           * default file not present
-           */
-          if (e.code !== 'ENOENT') {
-            return next(e);
-          }
-
-          const icon = identicon(username);
-
-          try {
-            // create folder if not exist
-            try {
-              await fs.mkdir(defaultFolderPath);
-            } catch (e) {
-              if (e.code !== 'EEXIST') throw e;
-            }
-
-            await fs.writeFile(defaultPath, icon);
-          } catch (e) {
-            return next(err);
-          }
+          return next(e);
         }
 
         // serve the default from disk
+        const defaultPath = getDefaultAvatarPath(username);
         return res.sendFile(defaultPath, { headers: { 'content-type': 'image/svg+xml' } }, err => { if (err) return next(err); });
       }
       return next(err);
@@ -67,6 +59,45 @@ async function get(req, res, next) {
   });
 }
 
+function getDefaultAvatarPath(username) {
+  return path.resolve(`./files/avatars/${username}/default`);
+}
+
+async function createDefaultAvatarIfNotExist(username) {
+  const defaultPath = getDefaultAvatarPath(username);
+  const defaultFolderPath = path.resolve(`./files/avatars/${username}`);
+
+  // try to find default on disk
+  try {
+    await fs.stat(defaultPath);
+  } catch (e) {
+
+    // throw unexpected error
+    if (e.code !== 'ENOENT') {
+      throw e;
+    }
+
+    // generate the default avatar, identicon
+    const icon = identicon(username);
+
+    // create folder for user's avatars if it doesn't exist already
+    try {
+      await fs.mkdir(defaultFolderPath);
+    } catch (e) {
+      if (e.code !== 'EEXIST') throw e;
+    }
+
+    // save the default user's avatar on disk.
+    await fs.writeFile(defaultPath, icon);
+  }
+}
+
+/**
+ * Generate identicon for a given username
+ *
+ * @param {string} username - who is the avatar for
+ * @returns {string} - a generated svg image
+ */
 function identicon(username) {
   const hash = crypto.createHash('sha256').update(username).digest('hex');
 
@@ -83,31 +114,40 @@ function identicon(username) {
   }
 }
 
+/**
+ * PATCH - update the avatar of a user
+ */
 async function patch(req, res, next) {
   const temporaryFilePath = path.resolve(`./${req.file.path}`);
   const { username } = req.auth;
+
   // sizes of the profile pictures
   const sizes = [512, 256, 128, 64, 32, 16];
 
   try {
-    // read the uploaded file
+    // read the uploaded file from a temporary folder
     const buffer = await fs.readFile(temporaryFilePath);
 
+    // resize the uploaded image to expected sizes and save
     const imgPromises = sizes.map(size => resizeSaveAvatar(buffer, size, username));
     await Promise.all(imgPromises);
 
-    // remove the temporary file
-    await fs.unlink(temporaryFilePath);
-
     return res.status(204).end();
   } catch (e) {
+    return next(e);
+  } finally {
     // remove the temporary file
     await fs.unlink(temporaryFilePath);
-
-    return next(e);
   }
 }
 
+/**
+ * Resize provided image and save it.
+ * @param {Buffer} buffer - the uploaded image
+ * @param {number} size - the final size of the image (square side) in pixels
+ * @param {string} username - user to save the avatar to
+ * @returns {Promise<info>} info returned by sharp library
+ */
 async function resizeSaveAvatar(buffer, size, username) {
   // create the user directory if not exist
   try {
@@ -126,8 +166,15 @@ async function resizeSaveAvatar(buffer, size, username) {
   return info;
 }
 
+/**
+ * Parse the uploaded multipart/form-data image (multer library)
+ */
 const parseAvatar = upload.single('avatar');
 
+/**
+ * If any error is encountered during validation etc., remove the temporary file
+ * from the temporary folder
+ */
 async function removeTemporaryFileOnError(err, req, res, next) {
   const temporaryFilePath = path.resolve(`./${req.file.path}`);
   await fs.unlink(temporaryFilePath);

controllers/deserialize.js

@@ -9,6 +9,10 @@ const router = express.Router();
 
 const serializers = require(path.resolve('./serializers'));
 
+/**
+ * Make sure we don't try to deserialize multipart/form-data requests
+ * Especially useful for avatar and other file uploads
+ */
 function notMultipart(req, res, next) {
   // if the request is multipart/form-data, don't deserialize.
   const isMultipart = /^multipart\/form-data/.test(req.headers['content-type']);

jobs/files.js

@@ -1,14 +1,7 @@
 'use strict';
 
 const path = require('path'),
-      { promisify } = require('util'),
-      rimraf = promisify(require('rimraf')),
-      fs = require('fs');
-
-// fs promisified
-const fsp = {
-  mkdir: promisify(fs.mkdir)
-};
+      fs = require('fs-extra');
 
 /**
  * clear the ./uploads folder (temporary files for uploading avatar)
@@ -19,9 +12,9 @@ async function clearTemporary() {
   const temp = path.resolve('./uploads');
 
   // rm -rf the temp folder
-  await rimraf(temp);
+  await fs.remove(temp);
   // recreate the temp folder
-  await fsp.mkdir(temp);
+  await fs.mkdir(temp);
 }
 
 module.exports = { clearTemporary };

test/avatar.js

@@ -58,10 +58,6 @@ describe('/users/:username/avatar', function () {
     [loggedUser, otherUser] = dbData.users;
   });
 
-  describe('create default image identicon on email confirmation', () => {
-    it('create the svg image');
-  });
-
   describe('GET', function () {
 
     context('logged', function () {
@@ -302,7 +298,7 @@ describe('/users/:username/avatar', function () {
 
     });
 
-    describe('job: regularly clear all temp files older than 1 minute', () => {
+    describe('job: regularly clear stale temporary upload files', () => {
       it('should leave the /uploads folder empty', async () => {
         const uploads = path.resolve('./uploads');
 
@@ -320,6 +316,8 @@ describe('/users/:username/avatar', function () {
         const filesAfter = await fs.readdir(uploads);
         should(filesAfter).Array().length(0);
       });
+
+      it('should leave the files less than 1 minute old');
     });
 
   });