heroku / identity

[DEPRECATED] Login and OAuth management service for Heroku
https://id.heroku.com/
MIT License
3 stars 0 forks source link

Convert change email confirm to use API's v3 PATCH /users/~ #179

Closed dmcinnes closed 9 years ago

dmcinnes commented 9 years ago

First part in an effort to move off of API v2.

Depends on https://github.com/heroku/api/pull/4929

dmcinnes commented 9 years ago

Not sure why it keeps insisting those are violations, it does not fail for me locally now that I've updated the .rubocop.yml file to a more sensible default. ¯(º_o)/¯

dmcinnes commented 9 years ago

Ready for review @heroku/api

rwz commented 9 years ago

LGTM except I'm not sure about URI.encode_www_form vs sending params as JSON in the API call.

Seems like it should be JSON since we're using v3, right?

dmcinnes commented 9 years ago

Ah you're right! Lazy copying from me. Fixed.

mikehale commented 9 years ago

Ah you're right! Lazy copying from me. Fixed.

Cool. I was curious about that too. Does that mean the api side of things should be updated to use v3_body_params instead of just params? Actually I can't remember which form it was using now...

mikehale commented 9 years ago

Cool. I was curious about that too. Does that mean the api side of things should be updated to use v3_body_params instead of just params? Actually I can't remember which form it was using now...

Actually it looks like most things are using v3_body_params, with the exception of getting the user id: https://github.com/heroku/api/blob/doug-v3-confirm-email-change/lib/api/endpoints/api_v3/users.rb#L41.

I'm not really sure what the difference between params and v3_body_params are.

dmcinnes commented 9 years ago

@mikehale The params should be just fields on the URL like user id in /users/:user_id (and I think query params as well) and body params are the key=value pairs in JSON in the post body.

mikehale commented 9 years ago

heh, it is right there in the name isn't. /me feels :sheep:ish

dmcinnes commented 9 years ago

@mikehale no worries! :grinning: