TryGhost/Ghost
1
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2024-12-29 13:52:10 +03:00
Code Issues Projects Releases Wiki Activity

Removed isNew usages in model layer

Browse Source 
no issue

- `isNew` does not work in Ghost, because Ghost does not use auto increment id's
- see https://github.com/bookshelf/bookshelf/issues/1265
- see https://github.com/bookshelf/bookshelf/blob/0.10.3/src/base/model.js#L211
- we only had one occurance, which was anyway redundant
  - if you add a user, `hasChanged('password') is true
  - if you edit a user and the password has changed, `hasChanged('password')` is true as well

NOTE #1:

1. We can't override `isNew` and throw an error, because bookshelf makes use of `isNew` as well, but it's a fallback if `options.method` is not set.
2. It's hard to re-implement `isNew` based on `options.method`, because then we need to ensure that this value is always set (requires a couple of changes)

NOTE #2:
If we need to differentiate if a model is new or edited, we should manually check for `options.method === insert`.

NOTE #3:
The unit tests are much faster compared to the model integration tests.
I did a comparision with the same test assertion:
  - unit test takes 70ms
  - integration test takes 190ms
This commit is contained in:
kirrg001 2018-02-15 20:15:43 +01:00 committed by Katharina Irrgang
parent 0b5cfd933f
commit 355ef54702
3 changed files with 72 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
core/server/models/user.js
View File

@ -131,7 +131,7 @@ User = ghostBookshelf.Model.extend({
* normal behaviour if not (set random password, lock user, and hash password)
* - no validations should run, when importing
*/
if (self.isNew() || self.hasChanged('password')) {
if (self.hasChanged('password')) {
this.set('password', String(this.get('password')));
// CASE: import with `importPersistUser` should always be an bcrypt password already,

14
core/test/integration/model/model_users_spec.js
View File

@ -117,20 +117,6 @@ describe('User Model', function run() {
}).catch(done);
});
it('can set password of only numbers', function () {
var userData = testUtils.DataGenerator.forModel.users[0];
// avoid side-effects!
userData = _.cloneDeep(userData);
userData.password = 109674836589;
// mocha supports promises
return UserModel.add(userData, context).then(function (createdUser) {
should.exist(createdUser);
// cannot validate password
});
});
it('can find by email and is case insensitive', function (done) {
var userData = testUtils.DataGenerator.forModel.users[2],
email = testUtils.DataGenerator.forModel.users[2].email;

85
core/test/unit/models/user_spec.js
View File

@ -1,16 +1,73 @@
var should = require('should'),
'use strict';
const should = require('should'),
sinon = require('sinon'),
models = require('../../../server/models'),
common = require('../../../server/lib/common'),
utils = require('../../utils'),
security = require('../../../server/lib/security'),
testUtils = require('../../utils'),
sandbox = sinon.sandbox.create();
describe('Models: User', function () {
let knexMock;
before(function () {
models.init();
});
afterEach(function () {
sandbox.restore();
});
describe('validation', function () {
before(function () {
knexMock = new testUtils.mocks.knex();
knexMock.mock();
});
beforeEach(function () {
sandbox.stub(security.password, 'hash').resolves('$2a$10$we16f8rpbrFZ34xWj0/ZC.LTPUux8ler7bcdTs5qIleN6srRHhilG');
});
after(function () {
knexMock.unmock();
});
describe('password', function () {
it('no password', function () {
return models.User.add({email: 'test1@ghost.org', name: 'Ghosty'})
.then(function (user) {
user.get('name').should.eql('Ghosty');
should.exist(user.get('password'));
});
});
it('only numbers', function () {
return models.User.add({email: 'test2@ghost.org', name: 'Wursti', password: 109674836589})
.then(function (user) {
user.get('name').should.eql('Wursti');
should.exist(user.get('password'));
});
});
it('can change password', function () {
let oldPassword;
return models.User.findOne({slug: 'joe-bloggs'})
.then(function (user) {
user.get('slug').should.eql('joe-bloggs');
oldPassword = user.get('password');
user.set('password', '12734!!332');
return user.save();
})
.then(function (user) {
user.get('slug').should.eql('joe-bloggs');
user.get('password').should.not.eql(oldPassword);
});
});
});
});
describe('Permissible', function () {
function getUserModel(id, role) {
var hasRole = sandbox.stub();
@ -28,7 +85,7 @@ describe('Models: User', function () {
var mockUser = getUserModel(1, 'Owner'),
context = {user: 1};
models.User.permissible(mockUser, 'destroy', context, {}, utils.permissions.owner, true, true).then(() => {
models.User.permissible(mockUser, 'destroy', context, {}, testUtils.permissions.owner, true, true).then(() => {
done(new Error('Permissible function should have errored'));
}).catch((error) => {
error.should.be.an.instanceof(common.errors.NoPermissionError);
@ -41,7 +98,7 @@ describe('Models: User', function () {
var mockUser = getUserModel(3, 'Contributor'),
context = {user: 3};
return models.User.permissible(mockUser, 'edit', context, {}, utils.permissions.contributor, false, true).then(() => {
return models.User.permissible(mockUser, 'edit', context, {}, testUtils.permissions.contributor, false, true).then(() => {
should(mockUser.get.calledOnce).be.true();
});
});
@ -51,7 +108,7 @@ describe('Models: User', function () {
var mockUser = getUserModel(3, 'Editor'),
context = {user: 2};
models.User.permissible(mockUser, 'edit', context, {}, utils.permissions.editor, true, true).then(() => {
models.User.permissible(mockUser, 'edit', context, {}, testUtils.permissions.editor, true, true).then(() => {
done(new Error('Permissible function should have errored'));
}).catch((error) => {
error.should.be.an.instanceof(common.errors.NoPermissionError);
@ -65,7 +122,7 @@ describe('Models: User', function () {
var mockUser = getUserModel(3, 'Administrator'),
context = {user: 2};
models.User.permissible(mockUser, 'edit', context, {}, utils.permissions.editor, true, true).then(() => {
models.User.permissible(mockUser, 'edit', context, {}, testUtils.permissions.editor, true, true).then(() => {
done(new Error('Permissible function should have errored'));
}).catch((error) => {
error.should.be.an.instanceof(common.errors.NoPermissionError);
@ -79,7 +136,7 @@ describe('Models: User', function () {
var mockUser = getUserModel(3, 'Author'),
context = {user: 2};
return models.User.permissible(mockUser, 'edit', context, {}, utils.permissions.editor, true, true).then(() => {
return models.User.permissible(mockUser, 'edit', context, {}, testUtils.permissions.editor, true, true).then(() => {
should(mockUser.hasRole.called).be.true();
should(mockUser.get.calledOnce).be.true();
});
@ -89,7 +146,7 @@ describe('Models: User', function () {
var mockUser = getUserModel(3, 'Contributor'),
context = {user: 2};
return models.User.permissible(mockUser, 'edit', context, {}, utils.permissions.editor, true, true).then(() => {
return models.User.permissible(mockUser, 'edit', context, {}, testUtils.permissions.editor, true, true).then(() => {
should(mockUser.hasRole.called).be.true();
should(mockUser.get.calledOnce).be.true();
});
@ -99,7 +156,7 @@ describe('Models: User', function () {
var mockUser = getUserModel(3, 'Editor'),
context = {user: 3};
return models.User.permissible(mockUser, 'destroy', context, {}, utils.permissions.editor, true, true).then(() => {
return models.User.permissible(mockUser, 'destroy', context, {}, testUtils.permissions.editor, true, true).then(() => {
should(mockUser.hasRole.called).be.true();
should(mockUser.get.calledOnce).be.true();
});
@ -109,7 +166,7 @@ describe('Models: User', function () {
var mockUser = getUserModel(3, 'Editor'),
context = {user: 2};
models.User.permissible(mockUser, 'destroy', context, {}, utils.permissions.editor, true, true).then(() => {
models.User.permissible(mockUser, 'destroy', context, {}, testUtils.permissions.editor, true, true).then(() => {
done(new Error('Permissible function should have errored'));
}).catch((error) => {
error.should.be.an.instanceof(common.errors.NoPermissionError);
@ -123,7 +180,7 @@ describe('Models: User', function () {
var mockUser = getUserModel(3, 'Administrator'),
context = {user: 2};
models.User.permissible(mockUser, 'destroy', context, {}, utils.permissions.editor, true, true).then(() => {
models.User.permissible(mockUser, 'destroy', context, {}, testUtils.permissions.editor, true, true).then(() => {
done(new Error('Permissible function should have errored'));
}).catch((error) => {
error.should.be.an.instanceof(common.errors.NoPermissionError);
@ -137,7 +194,7 @@ describe('Models: User', function () {
var mockUser = getUserModel(3, 'Author'),
context = {user: 2};
return models.User.permissible(mockUser, 'destroy', context, {}, utils.permissions.editor, true, true).then(() => {
return models.User.permissible(mockUser, 'destroy', context, {}, testUtils.permissions.editor, true, true).then(() => {
should(mockUser.hasRole.called).be.true();
should(mockUser.get.calledOnce).be.true();
});
@ -147,7 +204,7 @@ describe('Models: User', function () {
var mockUser = getUserModel(3, 'Contributor'),
context = {user: 2};
return models.User.permissible(mockUser, 'destroy', context, {}, utils.permissions.editor, true, true).then(() => {
return models.User.permissible(mockUser, 'destroy', context, {}, testUtils.permissions.editor, true, true).then(() => {
should(mockUser.hasRole.called).be.true();
should(mockUser.get.calledOnce).be.true();
});