updating profile upload with a new version of multer

[gustavodemari]

as described in #947

[ilanbiala]

@gustavodemari please add tests for any applicable route, controller, and model part of this.

[codydaig]

Needs a New Line

[gustavodemari]

My Travis-CI build is not passing due a timeout when getting all users in /api/users with an admin role.
I will try for the last time to reopen this PR to rebuild again.

[mleanos]

@gustavodemari What's the reason for upgrading Multer here? I understand your concerns for the inMemory option. However, it seems for the purposes of using this for the Profile Image upload, it may not be much of a concern.

Other than that, is there a good reason for this upgrade? I've spent quite a bit of time with Multer & angular-file-upload; I had my own implementation of a profile image upload when my project was still on the 0.3.3 version of MEANJS.

I'm not necessarily opposed to this, but I'd like to understand it a bit more & spend some time testing the newer version of Multer.

[mleanos]

@gustavodemari I've tested this and it doesn't appear to work with Firefox. The image is uploading correctly, and updating the user data with the new image url. The image isn't rendering. I think it may have to do with the URL encoding. I've tested in Chrome, and it's working fine.

[gustavodemari]

@mleanos I was having an issue to upload files in other routes and destinations, also, I was trying to upload larger files than pictures like .csv files in a instance of 512 MB of RAM, so I had a concern about inMemory.
Furthermore, I've tested in Firefox and it's working fine.

[mleanos]

@gustavodemari It's still not working for me in Firefox. The issue seems to be that profileImageURL is saved as modules\\users\\client\\img\\profile\\uploads\\78e540f60d2b73406420825e0d239ee3, which is fine but the browser will end up decoding this as modules%5Cusers%5Cclient%5Cimg%5Cprofile%5Cuploads%5C78e540f60d2b73406420825e0d239ee3

Chrome seems to be able to handle this, but not Firefox. Before this change, profileImageURL was saved in the database as modules/users/client/img/profile/testavatar.jpg. I'd suggest saving the path with this format. If not, then you'd have to URL decode it before you display it. However, that would break any existing profile images.

Can you share what the path looks like for you in Firefox? The above screen shot only had the name of the image. Not the full path. That might help us understand why it's working for you, and not for me.

Can anyone else confirm this works for them in Firefox?

[gustavodemari]

@mleanos Firefox 41.0 on Ubuntu 14.04.3 trusty

Also, I looked at /api/users/me and the profileImageURL was correct, see below.

{
    "_id": "560c52d70eea21c17224bfb1",
    "displayName": "Gustavo De Mari",
    "provider": "local",
    "username": "gustavodemari",
    "__v": 0,
    "updated": "2015-10-02T00:47:46.138Z",
    "created": "2015-09-30T21:23:35.014Z",
    "roles": ["user"],
    "profileImageURL": "modules/users/client/img/profile/uploads/cabdd49cda2eb8a0bbcb6f937986eaf8",
    "email": "guhdemari@gmail.com",
    "lastName": "De Mari",
    "firstName": "Gustavo"
}

Are you sure that you are using my PR version?

[mleanos]

@gustavodemari The data you provided isn't from the raw database is it? It looks like you've already queried it using mongoose; based on the date format.

This is how my user data looks in my database after I update the profile picture... As you can see the URL saved is not the same as yours. I actually used Chrome for this particular update.. Although, again, Chrome can handle this URL, but Firefox can't.

{
    "_id": {
        "$oid": "55d483767cc47ac40c01ab75"
    },
    "displayName": "Michael Leanos",
    "provider": "local",
    "username": "mleanos",
    "created": {
        "$date": "2015-08-19T13:24:06.484Z"
    },
    "roles": [
        "user"
    ],
    "profileImageURL": "modules\\users\\client\\img\\profile\\uploads\\7bd0a0ac2bd0e57527b31abc2e94936f",
    "lastName": "Leanos",
    "firstName": "Michael",
    "__v": 0,
    "updated": {
        "$date": "2015-09-25T07:19:37.151Z"
    }
}

I'm 100% certain I'm using your branch.
WIndows 7
Firefox 41.0.1
Mongodb: 3.0.6

[mleanos]

@gustavodemari My guess is that either Multer, or a package that it's using, is saving the path in format that is incompatible with either Windows and/or Firefox. Chrome being able to resolve it fine, doesn't surprise me. But we need to support Firefox on Windows.

I'm going to look into Multer's dependencies to see if I can determine where the issue is coming from. Do you have any way of testing this on Windows & Firefox?

[gustavodemari]

@mleanos The data came from the HTTP request on /api/users/me.
Now, take a look at the raw database:

{
    "_id" : ObjectId("560c52d70eea21c17224bfb1"),
    "salt" : "xxxxxxxxxxxxxxxxx",
    "displayName" : "Gustavo De Mari",
    "provider" : "local",
    "username" : "gustavodemari",
    "created" : ISODate("2015-09-30T21:23:35.014Z"),
    "roles" : [
        "user"
    ],
    "profileImageURL" : "modules/users/client/img/profile/uploads/8a1ca188e4e0e4dcb7404976dfa9d6d0",
    "password" : "xxxxxxxxxxxxxxxxxxxxxx",
    "email" : "guhdemari@gmail.com",
    "lastName" : "De Mari",
    "firstName" : "Gustavo",
    "__v" : 0,
    "updated" : ISODate("2015-10-02T00:47:46.138Z")
}

Also, looking in your data, the field "updated" have an outdated date, which leads me to believe that you are using an outdated version:

"updated": {
        "$date": "2015-09-25T07:19:37.151Z"
    }

So, IHMO, isn't a browser problem, because the test are passing and I personally cloned my repo again, tested on Firefox and Chrome and everything is ok.

Can you try to git clone https://github.com/gustavodemari/mean.git and test again?

[mleanos]

Thanks to @bastianwegge for pointing this out..

This line will not work in Windows with the current implementation of Multer, because of how it is storing the path. It's not accommodating for Windows paths.

Can you update this line to use the following instead?
user.profileImageURL = 'modules/users/client/img/profile/uploads/' + req.file.filename;

I've tested this and it works in Firefox as well.

[mleanos]

@gustavodemari I just left a line comment. I think my suggestion is a safer way to calculate the path for the profile images.

https://github.com/meanjs/mean/pull/948/files#r41007858

[mleanos]

@gustavodemari The updated field isn't updated during the change profile image method. That's why my updated field was "outdated". If it makes sense to add it, then perhaps you can do so now since you're modifying that code.

[gustavodemari]

@mleanos I've tested on Chrome and Firefox on Windows, and it is working.

Also, in your case, the updated field has to be recent, because as you showed in your screenshot, the success message "Profile Picture Changed Successfully" makes clearly that the user.save method was executed with success.

[mleanos]

@gustavodemari Our user model .pre('save') hook doesn't update the updated field, neither does the changeProfilePicture method, so that field doesn't get updated when a user changes their profile picture. If you think otherwise, can you provide the reference?

I'm using the most up to date code base from master, and from your branch. I'm 100% confident about that. And it's simply not working for me in Firefox.

The reason this isn't working for me, is because the process that I'm running the application in. I'm running it directly from my Windows command prompt; I'm guessing you're running the app through bash or something similar, when you're testing on Windows.

None of our users, nor myself, should be required to run their app in an environment that can handle the req.file.path. We need to accommodate these other types of environments, when dealing with paths. The best way to handle this is to change this line
https://github.com/meanjs/mean/pull/948/files#diff-028054ee3eec2278e53157ccf4668fe4R67 to
user.profileImageURL = 'modules/users/client/img/profile/uploads/' + req.file.filename;

This is pretty close to how it was before it was changed with this PR. This solves the issue for me, and ensures this code works for all our users.

[gustavodemari]

@mleanos About the updated field, you're right, sorry.

So, about running through bash or something similar...no, I'm not, I am using the command prompt (check the video below).

https://www.youtube.com/watch?v=usT32SrzJck

[mleanos]

@gustavodemari In that video, you're using Chrome. I don't have any issues with Chrome. However, it does appear that the profileImageURL that is saved, is correct. If you say it's working for you in Firefox, I trust that.

The thing is, that it's genuinely not working for me. The profileImageURL is not saving in the correct format for me. For whatever reason. It's obviously caused by a difference in my environment. Since I'm having this issue, we can expect that others will also.

Let's just go ahead and change that line to what I suggested. It's the safest way to make sure we support all users. Is there a specific reason that you want to use req.file.path, and not my suggestion?

[bastianwegge]

@gustavodemari seriously, there is someone having a serious issue with the code you submitted (since it's not working for him) and you're arguing with him altough he gave you a detailed description what to do to make it work for you and for him. What's the point? Just go ahead and use a variable for the path in that controller and use that in Line 59 and 65. You both invested about 5 days of your sparetime in this PR ... isn't that enough ?

[gustavodemari]

@mleanos Jump to 1:23 forward, I'm using Firefox.

My reason is just to understand what is happening, and now we know that isn't a browser problem, so, relax, I will update the code using your suggestion soon.

@bastianwegge As I said, I will update the code using @mleanos suggestion, relax, lol.

[ilanbiala]

Can we also add a test for not being able to change profile picture if not signed in.

[ilanbiala]

@gustavodemari LGTM except for 1 test that we should definitely add. Also, please add a couple tests to make sure the error handling works as expected. Feel free to check coveralls to see what lines aren't covered to know what specific test cases you may have missed. Also, we just need to get this rebased and passing before we can merge.

[gustavodemari]

@ilanbiala Added the test, now I will check the conflicts.

[gustavodemari]

I've made some change on the uploads config to add some other necessary things, like file size limit and filetype filtering

[lirantal]

I like it, good stuff.
The function callback looks a bit odd in the configuration object, but can you think of another alternative?

[gustavodemari]

Yes, I agree that looks a bit odd.
But, the another alternative that I think is possible, is to define the file filter in the controller, but, seems to me that is the worst way, because doesn't scale when we've a scenario where we need to reuse the same filter in multiple controllers

[mleanos]

@gustavodemari What about adding this filter as an angular filter? I also think the filter could be refactored to be generic. Perhaps, add a parameter for the accepted file types? The list of file types seem more appropriate as a env config.

Another option

[simison]

Whoop, please no app logic at config files.

@gustavodemari you can keep the codebase DRY by putting it somewhere else and exporting/importing it when needed, e.g. as a middleware.

[mleanos]

I misunderstood what this fileFilter is being used for. Is this a requirement of Multer?

Since this is on the server side, can it be a reusable function?

[lirantal]

First of all, this is indeed a server-side config thing.
Secondly, there's another down-side to put it like that which is that we now force that filefilter everywhere else multer is used and that is wrong because for uploading some other kind of assets you wouldn't want to force only images.

@gustavodemari please recommend another alternative.

[gustavodemari]

@mleanos I added this filter in the backend because the previous version allowed to upload any type of file using a POST in /api/users/picture.
Also, filefilter is an option of multer and I believe that this code can't be used on Angular.

@lirantal We don't force to use filefilter everywhere, because after these changes, multer will operate in a independent way on the controller.

@simison Any suggestion where to put?

Some alternatives (feel free to give your opinion or to add suggestions):

Alternative 1:
Remove fileFilter function from config/env/default.js and define in the controller

config/env/default.js

uploads: {
    profileUpload: {
      dest: './modules/users/client/img/profile/uploads/', // Profile upload destination path
      limits: {
        fileSize: 1*1024*1024 // Max file size in bytes (1 MB)
      }
    }
  }

modules/users/server/controllers/users/users.profile.server.controller.js

var upload = multer(config.uploads.profileUpload).single('newProfilePicture');
upload.fileFilter = function (req, file, cb) {
  if (file.mimetype !== 'image/png' && file.mimetype !== 'image/jpg' && file.mimetype !== 'image/jpeg' && file.mimetype !== 'image/gif') {
        return cb(new Error('Only image files are allowed!'), false);
  }
  cb(null, true);
};

---

Alternative 2:
Remove fileFilter function from config/env/default.js, create config/lib/multer.js, define a profileUploadFilter in config/lib/multer.js and import config/lib/multer.js on user profile controller

config/env/default.js

uploads: {
    profileUpload: {
      dest: './modules/users/client/img/profile/uploads/', // Profile upload destination path
      limits: {
        fileSize: 1*1024*1024 // Max file size in bytes (1 MB)
      }
    }
  }

config/lib/multer.js

module.exports.profileUploadFileFilter = function (req, file, cb) {
    if (file.mimetype !== 'image/png' && file.mimetype !== 'image/jpg' && file.mimetype !== 'image/jpeg' && file.mimetype !== 'image/gif') {
        return cb(new Error('Only image files are allowed!'), false);
    }
    cb(null, true);
  };

modules/users/server/controllers/users/users.profile.server.controller.js

var profileUploadFileFilter = require(path.resolve('./config/lib/multer')).profileUploadFileFilter;
...
var upload = multer(config.uploads.profileUpload).single('newProfilePicture');
upload.fileFilter = profileUploadFileFilter;

[mleanos]

@gustavodemari I like the 2nd option.

What do you think about adding a env config setting for a list of accepted file types for profileUpload? In the Multer configuration file, you can retrieve the list of accepted file types from the env config.

Also, I think using lodash in your fileFilter function to check the current file's mimetype against the list. WDYT?

[lirantal]

@gustavodemari 2nd option is definitely better. I'd even move the file upload limit to that specific multer function in config/lib/multer because it is very specific.

[ilanbiala]

space after //

[ilanbiala]

@gustavodemari last two things, then this is ready to go.

[ilanbiala]

@gustavodemari can you fix up the last couple things and rebase so we can merge?

[gustavodemari]

@ilanbiala I think it's ready. Thanks.

[codydaig]

LGTM