Refactor student views

[singingwolfboy]

Fix pylint/pep8 warnings, use JsonResponse instead of HttpResponse where useful, put in TODOs to change HTTP status codes to be more accurate, update tests to be less brittle.

[sarina]

What is the reasoning behind adding a TODO here instead of just making the jsonresponse actually return the status code as well?

[singingwolfboy]

Because that's changing functionality. This pull request is intended to be purely for cleanup, without changing functionality whatsoever.

[sarina]

while you're at it I'd fix a few more:

$ pep8 common/djangoapps/student/tests/test_login.py
common/djangoapps/student/tests/test_login.py:26:1: E302 expected 2 blank lines, found 1

and there's a handful of pep8/pylint in tests.py that look straightforward to fix.

The fixes and refactoring looks good overall but I am curious about the TODOs as mentioned above.

[cpennington]

👍

[flowerhack]

This seems to disable fixme warnings for the entire file—is this actually what we want? I'd kind of prefer disabling it on a case-by-case basis (even though it's kind of annoying to add it next to each line), since I think we'd like to prevent people from adding TODOs to this file

[singingwolfboy]

I believe that # pylint: disable= disables a warning for the entire file, regardless of where you put it. So repeating it multiple times, one on every line that I added, is more annoying and still ends up silencing all the TODOs anyway. Or am I wrong about that?

[sarina]

Yeah - you put the pragma after a line to just disable it on that one line. See https://github.com/edx/edx-platform/blob/master/CONTRIBUTING.rst#pythonjavascript-styling

[cpennington]

Also: http://docs.pylint.org/faq.html?highlight=disable#is-it-possible-to-locally-disable-a-particular-message

[sarina]

Although it's preferable to say disable=unused-argument over disable=W0511 as the latter obfuscates what warning you're actually disabling.

[sarina]

🚢 (once tests pass obvs)

[flowerhack]

besides that one suggestion, ⛵

[sarina]

👍 let's merge this!