Skip to content

Conversation

@peterbe
Copy link
Contributor

@peterbe peterbe commented Jun 9, 2017

No description provided.

@peterbe peterbe requested review from akatsoulas and johngian June 9, 2017 15:47
@codecov-io
Copy link

codecov-io commented Jun 9, 2017

Codecov Report

Merging #144 into master will increase coverage by 0.78%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
+ Coverage   96.65%   97.43%   +0.78%     
==========================================
  Files           6        6              
  Lines         239      234       -5     
==========================================
- Hits          231      228       -3     
+ Misses          8        6       -2
Impacted Files Coverage Δ
mozilla_django_oidc/views.py 100% <ø> (ø) ⬆️
mozilla_django_oidc/utils.py 94.11% <0%> (-1.34%) ⬇️
mozilla_django_oidc/middleware.py 95.74% <0%> (ø) ⬆️
mozilla_django_oidc/auth.py 96.59% <0%> (+2.27%) ⬆️

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 64a53df...7a6fd57. Read the comment docs.

@johngian
Copy link
Collaborator

johngian commented Jun 9, 2017

Please also add a test with a request factory sending a post to the logout view and check that logout is called as expected.

self.assertEqual(response.status_code, 302)
self.assertEqual(response.url, '/example-logout')
response = logout_view(request)
self.assertEqual(response.status_code, 405)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason we want to test get when we only implement post? I suggest we remove this test if that doesn't affect coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. It's simply part of Django. No point testing Django. Will remove.

return import_from_settings('LOGOUT_REDIRECT_URL', '/')

def dispatch(self, request, *args, **kwargs):
def post(self, request):
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about CSRF? Does it work by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests test that automatically. As long as the view doesn't have a csrf_exempt decorator it should be using CSRF.

@peterbe
Copy link
Contributor Author

peterbe commented Jun 12, 2017

Please also add a test with a request factory sending a post to the logout view and check that logout is called as expected.

This is already one.

@peterbe
Copy link
Contributor Author

peterbe commented Jun 13, 2017

I believe I've address the CSRF question.

@johngian johngian merged commit b0c85c6 into mozilla:master Jun 13, 2017
@peterbe peterbe deleted the only-logout-with-post-fixes-126 branch June 14, 2017 12:36
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.

3 participants