Skip to content

Commit

Permalink
Removed isNew usages in model layer
Browse files Browse the repository at this point in the history 
no issue

- `isNew` does not work in Ghost, because Ghost does not use auto increment id's
- see bookshelf/bookshelf#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
  • Loading branch information
@kirrg001
kirrg001 committed Feb 15, 2018
1 parent 0b5cfd9 commit 355ef54
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 29 deletions.
2 changes: 1 addition & 1 deletion core/server/models/user.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
14 changes: 0 additions & 14 deletions core/test/integration/model/model_users_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
85 changes: 71 additions & 14 deletions core/test/unit/models/user_spec.js
View file
Original file line number Diff line number Diff line change
@@ -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();
Expand All @@ -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);
Expand All @@ -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();
});
});
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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();
});
Expand All @@ -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();
});
Expand All @@ -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();
});
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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();
});
Expand All @@ -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();
});
Expand Down

0 comments on commit 355ef54

Please sign in to comment.