Conversation
2. Update Host filter 3. Update the calls to update() on the user model when logging in 4. Module updates
|
let me know when this is ready for re-review @cadecairos |
|
@gideonthomas will do. after many experiments I've now turned to downloading the travis-ci docker images (3.5 GB :/) so I can try to replicate the problem locally. |
|
oh dear! Ok good luck! |
|
Hmm looks like it fails because of mysql |
gideonthomas
left a comment
gideonthomas
left a comment
There was a problem hiding this comment.
tests pass, but jscs fails for me locally. Couple of comments about stuff that might break things in sequelize. Should be good to go after that
| used: false, | ||
| invalid: false, | ||
| createdAt: { | ||
| gte: moment(Date.now() - RESET_EXPIRY_TIME).utc().format("YYYY-MM-DD HH:mm:ss Z") |
There was a problem hiding this comment.
One thing I noticed is that sequelize recommends not using alias operators anymore and say that it will be removed soon according to this: http://docs.sequelizejs.com/manual/tutorial/querying.html#operators-security. Might be useful to switch to Sequelize.Op instead.
| id: id | ||
| } | ||
| }).complete(callback); | ||
| where: { |
There was a problem hiding this comment.
is the find method an alias for all the get one instance query methods? It's not really mentioned clearly in the docs http://docs.sequelizejs.com/manual/tutorial/models-usage.html#data-retrieval-finders nor in their changelog.
| .then(function (loginToken) { | ||
| .then(loginToken => { | ||
| if (!loginToken) { | ||
| return bPromise.reject({ |
There was a problem hiding this comment.
the problem with this is that sequelize uses an independent copy of bluebird so I'm not sure if rejecting a promise like this would conflict with that.
| client: clientId | ||
| }).then(function (client) { | ||
| return oauthLogin.create({ | ||
| client: clientId |
There was a problem hiding this comment.
according to http://docs.sequelizejs.com/manual/tutorial/models-usage.html#-findorcreate-search-for-a-specific-element-or-create-it-if-not-available, shouldn't you only be able to pass a single object into findOrCreate?
{
where: {
client: clientId
},
defaults: {
client: clientId
}
}| client: clientId | ||
| }, { | ||
| client: clientId | ||
| }).then(client => oauthLogin.create({ |
There was a problem hiding this comment.
does this work without spread? Won't findOrCreate return an array instead so this won't work?
| optimization: optimize ? 0 : 2 | ||
| })); | ||
|
|
||
| var optimize = env.get("NODE_ENV") !== "development", |
There was a problem hiding this comment.
duplicate of line 84/85
|
|
||
| // convert requests for ltr- or rtl-specific CSS back to the real filename, | ||
| // as the rtltr-for-less package was a hack that was never meant to hit production. | ||
| http.use(function rtltrRedirect(req, res, next) { |
There was a problem hiding this comment.
duplicate of line 89-96
| "MD5": "1.0.3", | ||
| "async": "0.2.9", | ||
| "badword": "0.0.1", | ||
| "basic-auth-connect": "^1.0.0", |
There was a problem hiding this comment.
package.json engines need to be updated
2 participants
This PR Fixes an issue with the original PR (#395) where the key that a related model was saved to got changed from older Sequelize versions.
It also contains many dependency updates to fix security issues, including changes to get the server running properly on Express 4.x