meanjs / mean

MEAN.JS - Full-Stack JavaScript Using MongoDB, Express, AngularJS, and Node.js -
http://meanjs.org
MIT License
4.87k stars 1.98k forks source link

Bad example of file type valiator for multer.fileFilter #1270

Open incNick opened 8 years ago

incNick commented 8 years ago

Source in file modules/users/server/controllers/users/users.profile.server.controller.js, some content like:

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

  // Filtering to upload only images
  upload.fileFilter = profileUploadFileFilter;

I just create one other controller use for save upload files, and reference above code, just found the fileFilter always not work. Correct way should be:

  var multerConfig = config.uploads.profileUpload;
  multerConfig.fileFilter = require(path.resolve('./config/lib/multer')).profileUploadFileFilter;
  var upload = multer(multerConfig).single('newProfilePicture');
lirantal commented 8 years ago

@incNick would you like to submit a PR to propose a fix and explain it?

incNick commented 8 years ago

aHa, in fact I'm first time use github and also one new player of Node & Express so I don't know how to submit a PR. Bur after read the source of multer, looks like the filter option only can assign when construct instance, so run multerInstance.fileFilter = something nothing change.

I'm sorry of can't provide you more help :(

simison commented 8 years ago

@incNick, that's no problemo! This is a great change to get you started with Open Source contributions, Git and GitHub! :-) Up for a challenge? I'll spare some 5 minutes to write quick instructions. Probably easier if I write it for an app, since it's easier to get started with than command line (but lemme know if you'd like to try command line instead):

  1. Click Fork button on top-right corner at GitHub
  2. Open your fork https://github.com/incNick/mean
  3. Install https://desktop.github.com/
  4. Click image from your forked repository on GitHub. It opens the app and asks where to save your copy.
  5. At the app, click image to create a new branch
  6. Name branch something like "fix-multer-filetype-valiator"
  7. Open files, do your changes, make sure tests work ('npm test')
  8. When you're done, then again in the app, prepare the commit: image
      1. Choose "Uncommited Changes"
      1. Choose files (or click blue areas in files to commit only some lines)
      1. Write title, e.g. Fix(users): changes to multer filetype validator
      1. Write description, see guidelines (might look scary but don't worry if something isn't exactly as it should). Remember to reference this issue in your description by writing "#1270"
      1. Commit changes to your branch
      1. Admire your first commit :-)
      1. Create a Pull request:

image

Volá! :-)

If you need to do changes, just commit and push to the same branch and they'll appear at the Pull Request here.

This was with OSX, should be quite similar with Windows or Linux clients.

Feel free to ask any questions here or just add me on any chat app. I'm always eager to get new people into open source community.

incNick commented 8 years ago

@simison sure, I think you are right, go further then need learn more :) I just public my branch fix-multer-filetype-validator there, but I'm not sure you can see it. Hope you have one very nice weekend.

trendzetter commented 8 years ago

@incNick I have created the PR for you #1272

trendzetter commented 8 years ago

If only I could reproduce the problem. The filetype-validator seems to work here, anyone else seeing the issue?

trendzetter commented 8 years ago

I was able to reproduce the issue, see updates in pr #1272

jamviking commented 8 years ago

@simison thanks for the lesson...am a very newbie myself. Wish I had seen it before I attempted my first PR for the generator.

lirantal commented 8 years ago

@simison kudos on that lovely invite for contributing on GitHub! @trendzetter let's talk on your open PR for status

mleanos commented 8 years ago

1465