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

Add user profile at authentication #447

Merged
merged 2 commits into from Mar 3, 2018

Conversation

christophehenry
Copy link
Contributor

@christophehenry christophehenry commented Feb 27, 2018

/api-token-auth/ now returns limited profile infos in addition to token:

{
  "guid": "eba9d938-71a2-4570-96c2-99e6e6bb232f",
  "handle": "karl_marx@127.0.0.1:8000",
  "home_url": "http:\/\/127.0.0.1:8000\/p\/eba9d938-71a2-4570-96c2-99e6e6bb232f\/",
  "id": 2,
  "image_url_small": "http:\/\/127.0.0.1:8000\/static\/images\/pony50.png",
  "is_local": true,
  "name": "",
  "url": "http:\/\/127.0.0.1:8000\/p\/eba9d938-71a2-4570-96c2-99e6e6bb232f\/",
  "token": "0ab231f9f7ab63a6f78fd9746b9bdc9926a734fd"
}
  • Tests
  • Update the docs to explain how to recover token

Refs: #446

@codecov
Copy link

codecov bot commented Feb 27, 2018

Codecov Report

Merging #447 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #447      +/-   ##
==========================================
+ Coverage   92.43%   92.47%   +0.03%     
==========================================
  Files         107      107              
  Lines        3516     3533      +17     
==========================================
+ Hits         3250     3267      +17     
  Misses        266      266
Impacted Files Coverage Δ
config/urls.py 80% <100%> (-0.65%) ⬇️
socialhome/views.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f872152...b8dad13. Read the comment docs.

Copy link
Owner

@jaywink jaywink left a comment

Choose a reason for hiding this comment

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

Great! <3 Just needs a test since we're now owning the code from DRF and thus responsible for testing the view post doesn't crash and burn.

@jaywink
Copy link
Owner

jaywink commented Feb 27, 2018

Oh yeah, the docs should also have a little update where they currently say how to retrieve the token to indicate it now returns also some profile info.

@christophehenry
Copy link
Contributor Author

Right. Added to the tasks.

@christophehenry
Copy link
Contributor Author

@jaywink I have a problem with this test. What's the way to create the user in DB so that Django REST API is aware of it?

Also, how can I write the Swagger documentation for this endpoint?

self.assertEqual(response.data, {'non_field_errors': ['Unable to log in with provided credentials.']})

def test_authentication(self):
self.client.force_authenticate(self.user, token="0ab231f9f7ab63a6f78fd9746b9bdc9926a734fd")
Copy link
Owner

Choose a reason for hiding this comment

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

You're testing login so you shouldn't be forcing authentication in the test - this line shouldn't be here imho.


def test_authentication(self):
self.client.force_authenticate(self.user, token="0ab231f9f7ab63a6f78fd9746b9bdc9926a734fd")
response = self.client.post(reverse("api-token-auth"),
Copy link
Owner

Choose a reason for hiding this comment

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

Since this is a SocialhomeAPITestCase you don't need to use self.client. Our test classes use some handy helper stuff which means you can just do self.post("api-token-auth", {"username": self.user.username, "password": self.user.password}). (see here)

Does that work? If not I'm not sure what could be the problem. The password should be set by the factory so that shouldn't be an issue..

@jaywink
Copy link
Owner

jaywink commented Mar 2, 2018

Also, how can I write the Swagger documentation for this endpoint?

I mean this page - but Swagger docs would be nice as well! They're done by adding a docstring to the view itself. See this page. So basically, some generic title and a post: section should do it.

@christophehenry
Copy link
Contributor Author

Done. Ready for review.

@christophehenry christophehenry changed the title [WIP] Add user profile at authentication [Add user profile at authentication Mar 3, 2018
@christophehenry christophehenry changed the title [Add user profile at authentication Add user profile at authentication Mar 3, 2018
Copy link
Owner

@jaywink jaywink left a comment

Choose a reason for hiding this comment

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

A few minor comments, then good to go!

@@ -443,6 +443,7 @@ Changed

* Create and update content will now redirect to the content created or updated. Previous behaviour was user preferred landing page.
* Delete content will now redirect back to the page where the delete was triggered from. Previous behaviour was user preferred landing page. If the content delete is triggered from the content detail page, redirect will happen to user preferred landing page as before. (`#204 <https://github.com/jaywink/socialhome/issues/204>`_)
* ``/api-token-auth/`` endpoint now returns limited profile information in addition to token
Copy link
Owner

Choose a reason for hiding this comment

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

Changelog entry is in the wrong place, it should be in the latest upcoming release (top of file) not first release (bottom of file).

token, created = Token.objects.get_or_create(user=user)
data = LimitedProfileSerializer(user.profile, context={"request": self.request}).data
data.update({"token": token.key})
print(data)
Copy link
Owner

Choose a reason for hiding this comment

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

Print should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, missed this one after finishing to debug. Nice catch.

@christophehenry
Copy link
Contributor Author

Done.

Copy link
Owner

@jaywink jaywink left a comment

Choose a reason for hiding this comment

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

Great, thanks! <3

@jaywink jaywink merged commit c8a12ff into jaywink:master Mar 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants