Skip to content
This repository has been archived by the owner on May 26, 2020. It is now read-only.

Add support for Django 1.10 and Django REST Framework 3.4 #256

Merged
merged 9 commits into from
Sep 7, 2016

Conversation

mblayman
Copy link
Contributor

This branch takes a slightly different approach to 1.10/3.4 support than #252. It includes:

  • Updated documentation indicating change in support.
  • Add 1.10 and 3.4 to the Travis build matrix and tox file.
  • Consolidates the test views into tests/urls.py. 1.10 wasn't working with urlpatterns in the test_views and test_authentication files.
  • Tests the is_active behavior for the pre-1.10 ModelBackend and uses the new AllowAllUsersModelBackend to test the similar behavior for 1.10.
  • Turns on verbose for tox execution to show what is skipped for Travis builds.

There is one oddity in this branch that I can't really explain well. test_jwt_login_custom_response_json did a check for the username with:

self.assertEqual(response.data['user'], self.username)

That data did not exist under 1.10 and I don't understand why. Someone with a bit more history with the project might want to check if that's ok or a behavioral regression that needs to be fixed first.

@carltongibson
Copy link
Contributor

This looks great. +1

Not sure what's going on with the response.data['user'] check. Doesn't seem relevant. @jpadilla?

@jpadilla
Copy link
Owner

@mblayman great stuff! What do you mean "That data did not exist under 1.10..."? That assert is just to make sure what's expected after setting a custom JWT_RESPONSE_PAYLOAD_HANDLER in that test's setUp().

@carltongibson thanks for checking this out.

@jpadilla
Copy link
Owner

I see what you mean now with that assert failing after seeing this https://travis-ci.org/GetBlimp/django-rest-framework-jwt/jobs/156378076

@mblayman
Copy link
Contributor Author

@jpadilla I looked more closely based on your comments and can now see that deleting the assert for that custom payload handler test was the wrong thing to do. That user data should be there. I'll check if DRF is somehow ignoring the settings change that is done in the setUp and add the assert back.

I'm traveling tomorrow so I might not get to this for a couple of days.

@mblayman
Copy link
Contributor Author

I couldn't sleep so I found the problem. The custom handler function is stored in views.py at module level scope. Since the module is already imported, the jwt_response_payload_handler in views.py is basically locked to whatever the value of api_settings.JWT_RESPONSE_PAYLOAD_HANDLER was at module load time.

The setUp method changed api_settings and that had no effect on the value of jwt_response_payload_handler.

My guess is that APIClient changed something about how it loads the views so that it no longer worked when settings were changed (it smells like a caching change to me).

My fix works by hotswapping the module level variable in setUp and restoring it in tearDown. I chose this route because the project does not use mock, and I didn't want to pull that in as a dependency.

@carltongibson
Copy link
Contributor

carltongibson commented Aug 31, 2016

I think using override_settings is the right approach here.

@mblayman
Copy link
Contributor Author

mblayman commented Sep 1, 2016

@carltongibson I'm not clear how override_settings could work in this case. Here's the sequence I see:

  1. The views.py module is loaded when testing starts and sets the handler function to the function defined in api_settings (https://github.com/GetBlimp/django-rest-framework-jwt/blob/master/rest_framework_jwt/views.py#L11)
  2. The customer payload test setUp changes the settings value (https://github.com/GetBlimp/django-rest-framework-jwt/blob/master/tests/test_views.py#L56).
  3. The test executes a POST and hits the view's post method which invokes the handler (https://github.com/GetBlimp/django-rest-framework-jwt/blob/master/rest_framework_jwt/views.py#L59).

In that sequence, I can't see how changing the setting with override_settings would solve the problem. Am I missing something? Is there something inappropriate with hotswapping the module variable?

@carltongibson
Copy link
Contributor

Argh. You're right. It's setting the value at import time that does it.

It would need a bit of a jiggle round.

@mblayman
Copy link
Contributor Author

mblayman commented Sep 2, 2016

With the tests passing, I'm not sure there's anything left to do for this PR. Any chance of getting it merged and cutting a release? I'd love to get my projects upgraded to 1.10.

@jpadilla
Copy link
Owner

jpadilla commented Sep 4, 2016

@mblayman @carltongibson awesome, glad you figured it out! I'll do a final review on Tuesday to cut a release early in the week. Thanks again for the great work.

@jpadilla jpadilla merged commit 4e99990 into jpadilla:master Sep 7, 2016
@mblayman mblayman deleted the django-1.10 branch September 7, 2016 14:49
@jpadilla
Copy link
Owner

Reason why I haven't released this yet, Travis is reporting some errors. Any help debugging those would be amazing.

@mblayman
Copy link
Contributor Author

@jpadilla That's really odd considering the build was green before the merge. The broken builds seem to be related to Python 3.3 and a cryptography import error. Did you try a rebuild of master on Travis already? Seems to me like a busted tox environment.

@mblayman
Copy link
Contributor Author

@jpadilla Did you get a chance to try a rebuild of master on Travis?

@nicholashughes
Copy link

So I'd like a new release with 1.10 support as well. That being said, when I look at
https://travis-ci.org/GetBlimp/django-rest-framework-jwt/jobs/160398572
it says it fails because there are too many blank lines
When I look at the failing commit
4c8140e
I see there are 2 blank lines at line 30-31. Probably line 31 needs to be deleted. Line 33, which is also blank, could probably be dropped as well.

@mblayman
Copy link
Contributor Author

@nicholashughes the job you referenced is for a recent Pull Request, not for the latest master build. The last master build is failing for the reasons I documented earlier related to cryptography (see https://travis-ci.org/GetBlimp/django-rest-framework-jwt/builds/158195937 and https://travis-ci.org/GetBlimp/django-rest-framework-jwt/builds).

My guess is still that the problem is a bad virtualenv and would work with rebuild. Unfortunately, I do not have permissions to trigger that rebuild to confirm or deny my theory.

@nicholashughes
Copy link

Oh! Dang. Yeah I just clicked on the Build Failing link in the docs and that's what I saw.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants