Skip to content
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.

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

Proof of concept for account profiles. #98

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

Proof of concept for account profiles. #98

wants to merge 8 commits into from

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

@@ -31,7 +31,7 @@ function Account (options) {
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