-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Don't use RequestsContext #4759
Don't use RequestsContext #4759
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
This is ready to merge.
This is something that a simple test case for any of this views would have caught. Don't we have a test case for this? If no, do you want to write tests for creating/editing the profiles?
I think we don't have tests for this, we only have a couple of tests for the forms, not the views. I'll check the coverage |
Well, we have only 20% coverage here p: |
I'm writing some tests for this, but @davidfischer feel free to merge this if you need to keep going with the django upgrade. I think there are some bugs on the views and it will take some time |
I don't think there's a huge rush although the Django upgrade is pretty high up my priority list. You have plenty of time to write tests. |
And we have 98% coverage on I also deleted an unused view ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if we still need create_profile
to put it back again and fix the bug
""" | ||
try: | ||
profile_obj = request.user.profile | ||
return HttpResponseRedirect(reverse('profiles_edit_profile')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here was a bug, it should be profiles_profile_edit
I grepped all the repositories and I didn't find that it's used in any place. I think we are safe removing it. |
@ericholscher I think we can merge this one, can you confirm that the method removed is not needed and it's OK by removing it? |
Yep. I think we should probably just get rid of profiles here at some point, since they are barely used. It's actually a vendoring of the old |
This is kind of deprecated on django 1.11 https://docs.djangoproject.com/en/2.1/releases/1.11/#django-template-backends-django-template-render-prohibits-non-dict-context
The solution s compatible with older versions
Ref #4750