Skip to content

Conversation

@jameswestnz
Copy link

@jameswestnz jameswestnz commented Jun 2, 2016

Some notes on this PR:

  • Tests have not yet been completed.
  • I have assumed that profiles will be a permanent part of all account requests. I.e. this information is always included in get requests.
  • Have only focussed on initial sign-up and session/account retrieval. So currently no profile requests have been dealt to.

To do:

  • Code tests
  • Docs for hoodie-account-client
  • PR hoodie-account

More than happy for someone to add to this PR ;)

closes #11

@gr2m
Copy link
Member

gr2m commented Jun 5, 2016

I have assumed that profiles will be a permanent part of all account requests. I.e. this information is always included in get requests.

No I wouldn’t do that. I’d agree with account.signUp, but for account.signIn I’d suggest to add an include argument which can be set to profile to include it in the response.

I think the account.profile methods should be fine. Only thing missing in your TODOs is updating docs

@gr2m
Copy link
Member

gr2m commented Jun 15, 2016

can you add a test to hoodie-account-server/tests/integration/routes/account/put-account-test.js with payload.data.attributes.profile set to something like fullname: 'Pat Doe'? Ideally to post-accounts-test.js, too. It would be great to test that at the end the _users doc will have profile.fullname set to Pat Doe

index.js Outdated
update: require('./lib/update').bind(null, state),
profile: {
get: require('./lib/profile-get').bind(null, state),
fetch: require('./lib/profile-fetch').bind(null, state, 'account.profile'),
Copy link
Member

Choose a reason for hiding this comment

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

good catch 👍

@gr2m
Copy link
Member

gr2m commented Jun 15, 2016

I’d maybe add another integration test: sign-up-with-profile-test.js to make sure the behavior works and won’t break in future. Otherwise looking good 👍

@jameswestnz
Copy link
Author

@gr2m have added (server) a test here: hoodiehq/hoodie-account-server@f69ffa9

Keen on feedback before I do the PATCH test just in case :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement account.signUp with profile: {...} option

2 participants