trailsjs / trailpack-sequelize

:package: Sequelize.js Trailpack http://sequelizejs.com
MIT License
5 stars 9 forks source link

Footprints update is not calling individual hooks #26

Closed cesar2064 closed 8 years ago

cesar2064 commented 8 years ago

when I access : PUT: /api/v1/{model}/{id}, it doesn't call the individual hooks from the model : beforeCreate and beforeUpdate. But when I access POST: /api/v1/{model} for creating a new one, it calls the individual hooks related to create. I don't know if this happens with the other footprints.

scott-wyatt commented 8 years ago

@cesar2064 which web server are you using?

cesar2064 commented 8 years ago

Hi @scott-wyatt, I am using express.

scott-wyatt commented 8 years ago

I'll have to write some tests and try this when I get a few moments.

cesar2064 commented 8 years ago

Ok, thanks for your help :).

jaumard commented 8 years ago

@cesar2064 @scott-wyatt callback are tested and beforeUpdate is tested here https://github.com/trailsjs/trailpack-sequelize/blob/master/test/lib/sequelize.callbacks.test.js#L33

@cesar2064 maybe you can try to run those tests on your machine confirm they're green for you

cesar2064 commented 8 years ago

@jaumard How can I run it with trails?

jaumard commented 8 years ago

@cesar2064 you can start by just clone the repo and do npm i and npm test do be sure it's ok on your environment. Then you can implement same tests on your trails project and run it see if it pass.

cesar2064 commented 8 years ago

Ok I cloned the repo and tested it, it seems to be ok on my environment, but I am unable to run tests on the project.

jaumard commented 8 years ago

@cesar2064 what do you mean by you're not able to run tests on your project ? You didn't setup tests on it or test is failing ?

cesar2064 commented 8 years ago

Its weird because when I execute npm test. it tests each file and folder in the project

jaumard commented 8 years ago

@cesar2064 do you have this setup under your test folder ? https://github.com/trailsjs/trails/tree/master/archetype/test

cesar2064 commented 8 years ago

No, I am sorry I am very new with unit testing in nodejs. I will try to replicate this config under my test folder.

jaumard commented 8 years ago

No problem :) try this and let us know the result

cesar2064 commented 8 years ago

I have the following error:

\test\setup.js                                                                                                                                                    
   5:1  error  'before' is not defined                        no-undef                                                                                                                   
  10:1  error  'after' is not defined                         no-undef                                                                                                                   
  12:2  error  Newline required at end of file but not found  eol-last 
jaumard commented 8 years ago

You need to install mocha npm i --save-dev eslint mocha and have under your package.json

"scripts": {
    "test": "eslint . && mocha"
  },
cesar2064 commented 8 years ago

Ok I installed the dependencies, and I have the following configuration in the package.json;

  "devDependencies": {
    "eslint": "^2.13.1",
    "eslint-config-trails": "latest",
    "mocha": "^2.5.3",
    "supertest": "^1.2"
  },
  "scripts": {
    "start": "node server.js",
    "test": "eslint . && mocha"
  },

But I still have the same error:

\test\setup.js                                                                                                                                                    
   5:1  error  'before' is not defined                        no-undef                                                                                                                   
  10:1  error  'after' is not defined                         no-undef                                                                                                                   
  12:2  error  Newline required at end of file but not found  eol-last
jaumard commented 8 years ago

Ho yes this are eslint errors I didn't notice ^^ Also did you have this on your package.json ?

"eslintConfig": {
    "extends": "trails"
  },
cesar2064 commented 8 years ago

yes, I have it:

{
 ...
  "main": "index.js",
  ...
  "dependencies": {
    "accept-language": "^2.0.17",
    "bcryptjs": "^2.3.0",
    "eslint": "^2.13.1",
    "express": "^5.0.0-alpha.2",
    "geoip-lite": "^1.1.8",
    "jwt-simple": "^0.5.0",
    "lokijs": "^1.4.1",
    "pg": "^4.5.6",
    "trailpack-bootstrap": "^1.0.1",
    "trailpack-core": "^1.0.1",
    "trailpack-express": "^1.0.10",
    "trailpack-footprints": "^1.0.1",
    "trailpack-repl": "^1.0.8",
    "trailpack-router": "^1.0.7",
    "trailpack-sequelize": "^1.0.5",
    "trails": "^1.0.4",
    "trails-controller": "^1.0.0-beta-2",
    "trails-model": "^1.0.0-beta-2",
    "trails-policy": "^1.0.2",
    "trails-service": "^1.0.0-beta-2",
    "url-pattern": "^1.0.1",
    "winston": "^2.2.0"
  },
  "devDependencies": {
    "eslint": "^2.13.1",
    "eslint-config-trails": "latest",
    "mocha": "^2.5.3",
    "supertest": "^1.2"
  },
  "scripts": {
    "start": "node server.js",
    "test": "eslint . && mocha"
  },
  "engines": {
    "node": ">= 4.0.0"
  },
  "eslintConfig": {
    "extends": "trails"
  }
}
jaumard commented 8 years ago

Hum... maybe you miss this file under test ? https://github.com/trailsjs/trails/blob/master/archetype/test/.eslintrc.json If you want to disable eslint if you can't manage to make it work and use "test": "mocha"

cesar2064 commented 8 years ago

ok now I have a different error:

setup.js                                                                                                                                                    
  12:2  error  Newline required at end of file but not found  eol-last 
jaumard commented 8 years ago

You have to add a new empty line at the end of the file :) generally during dev I use this "test": "eslint --fix . && mocha" like this eslint fix errors when it can

cesar2064 commented 8 years ago

How can I tell to npm test that I only need the test from /test folder?

jaumard commented 8 years ago

Normally it test only folder with test/mocha.opts https://github.com/trailsjs/trails/blob/master/archetype/test/mocha.opts

cesar2064 commented 8 years ago

Because now I see the tests from each folder of the project except test folder.

cesar2064 commented 8 years ago

yes I have it in test/mocha.opts

--reporter spec
--recursive
--full-trace
--no-exit
--check-leaks
--globals app

test/setup.js
test/unit/
test/integration/
jaumard commented 8 years ago

You should be ok then... is your code hosted somewhere ?

cesar2064 commented 8 years ago

Yes in bitbucket.

jaumard commented 8 years ago

Give me the link lol I'll take a quick look :)

cesar2064 commented 8 years ago

I dont know if you can see it, may be you have to create an account there :S

jaumard commented 8 years ago

You can temporary add me on the project if it's a private project my username is the same jaumard

cesar2064 commented 8 years ago

Ok I added you :)

cesar2064 commented 8 years ago

The branch that have the new test config is develop

jaumard commented 8 years ago

Ok :) I just take a look ^^ you miss understand what is going on.

The npm test command do 2 things here, first it run eslint --fix . witch will return all your code quality errors based on Trails rules (the extends on package.json) but it does it for the all project.

If there no errors then it run mocha but here it does it only under /test. On your project you have a lot of eslint errors so tests are not run at all, like I said if you want to run only test just change the command to mocha only. But for now npm start doesn't work for me witch mean npm test will fail with same error.

cesar2064 commented 8 years ago

Umm its weird, because npm start works for me in different computers.

jaumard commented 8 years ago

Let's move to gitter :) it will be easier as it's not really related to Trails anymore. But it's not working for me because I use Node V4 witch is not compatible with sequelizeQueryFile(path,cb,params = {}) :)

cesar2064 commented 8 years ago

umm ok, first I'll try to correct eslint errors.

cesar2064 commented 8 years ago

You can change initialize to false in config/database.js for avoiding this error.

cesar2064 commented 8 years ago

Hi, I finished the changes and updated my code. I tested the User footprint in my environment and it didn't execute the beforeUpdate for encrypting the password.

cesar2064 commented 8 years ago

The file test path is: test/controllers/FootPrintController

jaumard commented 8 years ago

It's because you try to update multiple user at same time (no id on where clause) so yes beforeUpdate is not called, but beforeBulkUpdate is ;)

cesar2064 commented 8 years ago

@jaumard I changed the test and updated the user by using the id, and the test failed . After that, I added the beforeBulkUpdate hook and it didn't pass the test. Anyway, thanks for the help