Skip to content

Proof of concept for account profiles. #158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Proof of concept for account profiles. #158

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1a7a9b6
Proof of concept for account profiles.
jameswestnz Jun 2, 2016
5fca7ce
Merge branch 'master' into profile
jameswestnz Jun 14, 2016
cb223fa
restored options.includeProfile
jameswestnz Jun 14, 2016
940ff81
Removed profile fallback
jameswestnz Jun 14, 2016
b3f2a75
Allow profile update when updating account
jameswestnz Jun 15, 2016
b4d1f66
Fixed PATCH /session/account/profile
jameswestnz Jun 15, 2016
aa21170
Removed npmlog from account.js - was added for testing
jameswestnz Jun 15, 2016
e545911
Tests now pass
jameswestnz Jun 15, 2016
1a20c96
Reverted how the id was retrieved. HAd some inconsistent code which p…
jameswestnz Jun 15, 2016
95c58d2
And the tests to go with it
jameswestnz Jun 15, 2016
f69ffa9
Account add wasn't adding include to options, but instead properties.…
jameswestnz Jun 15, 2016
ed53fe9
Removed attributes modification in tests
jameswestnz Jun 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/accounts/add.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ function addAccount (state, properties, options) {
type: 'user',
name: properties.username,
password: properties.password,
profile: properties.profile || {},
roles: [
'id:' + accountId
].concat(properties.roles || [])
Expand Down
2 changes: 1 addition & 1 deletion api/utils/doc-to-account.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ function toAccount (doc, options) {
}

if (options.includeProfile) {
account.profile = doc.profile
account.profile = doc.profile || {}
}
Copy link
Member

@gr2m gr2m Jun 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave that in, but replace account.profile = doc.profile with account.profile = doc.profile || {}

Copy link
Author

@jameswestnz jameswestnz Jun 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restored here: cb223fa


return account
Expand Down
9 changes: 7 additions & 2 deletions routes/account.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,16 @@ function accountRoutes (server, options, next) {
handler: function (request, reply) {
var username = request.payload.data.attributes.username
var password = request.payload.data.attributes.password
var profile = request.payload.data.attributes.profile
var id = request.payload.data.id
var query = request.query
accounts.add({
username: username,
password: password,
Copy link
Author

@jameswestnz jameswestnz Jun 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gr2m is this correct? Or the below? My understanding is that this should be an option, not a property?

Copy link
Member

@gr2m gr2m Jun 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you got it right, good catch

include: query.include,
profile: profile,
id: id
}, {
include: query.include || null
})

.then(serialise)
Expand Down Expand Up @@ -126,6 +129,7 @@ function accountRoutes (server, options, next) {

var newUsername = request.payload.data.attributes.username
var newPassword = request.payload.data.attributes.password
var newProfile = request.payload.data.attributes.profile
var id = request.payload.data.id

admins.validateSession(sessionId)
Expand Down Expand Up @@ -154,7 +158,8 @@ function accountRoutes (server, options, next) {
}
return accounts.update(session.account, {
username: newUsername,
password: newPassword
password: newPassword,
profile: newProfile
}, {
include: request.query.include
})
Expand Down
33 changes: 32 additions & 1 deletion tests/integration/routes/account/put-account-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ var mockCouchDbPutUser = nock('http://localhost:5984')
derived_key: Joi.string().required(),
iterations: Joi.any().only(10).required(),
password_scheme: Joi.any().only('pbkdf2').required(),
roles: Joi.array().items(Joi.string().regex(/^id:[0-9a-f]{12}$/)).max(1).min(1)
roles: Joi.array().items(Joi.string().regex(/^id:[0-9a-f]{12}$/)).max(1).min(1),
profile: Joi.object().required()
}).validate(body.docs[0]).error === null
})
.query(true)
Expand Down Expand Up @@ -76,6 +77,36 @@ getServer(function (error, server) {
})
})

group.test('User added with profile', function (t) {
var couchdb = mockCouchDbPutUser
.reply(201, [{
id: 'org.couchdb.user:pat-doe',
rev: '1-234'
}])

var options = _.defaultsDeep({
url: '/session/account?include=profile'
}, routeOptions)

var accountWithProfileFixture = require('../../fixtures/account-with-profile.json')

server.inject(options, function (response) {
t.is(couchdb.pendingMocks()[0], undefined, 'CouchDB received request')
delete response.result.meta

t.is(response.statusCode, 201, 'returns 201 status')
response.result.data.id = 'userid123'
response.result.data.relationships.profile.data.id = 'userid123-profile'
response.result.included[0].id = 'userid123-profile'
response.result.included[0].attributes = {
fullName: 'pat Doe',
email: '[email protected]'
}
Copy link
Member

@gr2m gr2m Jun 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you shouldn’t need to set response.result.included[0].attributes in tests, we only set the id properties because they are random when coming from the server

Copy link
Author

@jameswestnz jameswestnz Jun 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out - It also identified that I was falsely passing the test by not passing the profile data to the accounts.add method, and instead only setting the attributes there - so the test was flawed before! Sorted now :)

t.deepEqual(response.result, accountWithProfileFixture, 'returns account in right format')
t.end()
})
})

group.test('CouchDB User already exists', function (t) {
var couchdb = mockCouchDbPutUser
.reply(201, [{
Expand Down
3 changes: 2 additions & 1 deletion tests/integration/routes/accounts/post-accounts-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ var mockCouchDbCreateUserDoc = nock('http://localhost:5984')
derived_key: Joi.string().required(),
iterations: Joi.any().only(10).required(),
password_scheme: Joi.any().only('pbkdf2').required(),
roles: Joi.array().items(Joi.string().regex(/^id:[0-9a-f]{12}$/)).max(1).min(1)
roles: Joi.array().items(Joi.string().regex(/^id:[0-9a-f]{12}$/)).max(1).min(1),
profile: Joi.object().required()
}).validate(body.docs[0]).error === null
})
.query(true)
Expand Down