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

Don't use RequestsContext #4759

Merged
merged 3 commits into from
Oct 23, 2018

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Oct 15, 2018

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

@stsewd stsewd requested a review from a team October 15, 2018 22:16
Copy link
Member

@humitos humitos left a 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?

@stsewd
Copy link
Member Author

stsewd commented Oct 16, 2018

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

@stsewd
Copy link
Member Author

stsewd commented Oct 16, 2018

Well, we have only 20% coverage here p:

@stsewd
Copy link
Member Author

stsewd commented Oct 16, 2018

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

@davidfischer
Copy link
Contributor

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.

@stsewd
Copy link
Member Author

stsewd commented Oct 16, 2018

And we have 98% coverage on profiles/view.py now :)

I also deleted an unused view (create_profile), I tested with a new user and we don't use it, also it wasn't working before https://readthedocs.org/accounts/create/ anyway.

Copy link
Member Author

@stsewd stsewd left a 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'))
Copy link
Member Author

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

@humitos
Copy link
Member

humitos commented Oct 17, 2018

Let me know if we still need create_profile to put it back again and fix the bug

I grepped all the repositories and I didn't find that it's used in any place. I think we are safe removing it.

@humitos
Copy link
Member

humitos commented Oct 23, 2018

@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?

@ericholscher
Copy link
Member

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 django-profiles app.

@ericholscher ericholscher merged commit fa2e6f8 into readthedocs:master Oct 23, 2018
@stsewd stsewd deleted the dont-use-requestscontext branch October 23, 2018 12:32
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.

4 participants