From 7ef9ec838ff603baa0e096a34183bb3a1f3d00a3 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Thu, 16 Oct 2014 09:12:25 -0400 Subject: [PATCH] Added better docstrings and comments --- common/djangoapps/user_api/helpers.py | 70 +++++++++++--- common/djangoapps/user_api/views.py | 129 +++++++++++++++++--------- 2 files changed, 139 insertions(+), 60 deletions(-) diff --git a/common/djangoapps/user_api/helpers.py b/common/djangoapps/user_api/helpers.py index cd926983b9..bd9752eb15 100644 --- a/common/djangoapps/user_api/helpers.py +++ b/common/djangoapps/user_api/helpers.py @@ -5,6 +5,7 @@ This is NOT part of the public API. from functools import wraps import logging import json +from django.http import HttpResponseBadRequest LOGGER = logging.getLogger(__name__) @@ -58,6 +59,34 @@ def intercept_errors(api_error, ignore_errors=[]): return _decorator +def require_post_params(required_params): + """ + View decorator that ensures the required POST params are + present. If not, returns an HTTP response with status 400. + + Args: + required_params (list): The required parameter keys. + + Returns: + HttpResponse + + """ + def _decorator(func): + @wraps(func) + def _wrapped(*args, **kwargs): + request = args[0] + missing_params = set(required_params) - set(request.POST.keys()) + if len(missing_params) > 0: + msg = u"Missing POST parameters: {missing}".format( + missing=", ".join(missing_params) + ) + return HttpResponseBadRequest(msg) + else: + return func(request) + return _wrapped + return _decorator + + class InvalidFieldError(Exception): """The provided field definition is not valid. """ @@ -244,22 +273,30 @@ def shim_student_view(view_func, check_logged_in=False): function """ - @wraps(view_func) def _inner(request): - # Strip out enrollment action stuff, since we're handling that elsewhere + # The login and registration handlers in student view try to change + # the user's enrollment status if these parameters are present. + # Since we want the JavaScript client to communicate directly with + # the enrollment API, we want to prevent the student views from + # updating enrollments. if "enrollment_action" in request.POST: del request.POST["enrollment_action"] if "course_id" in request.POST: del request.POST["course_id"] - # Actually call the function! - # TODO ^^ + # Call the original view to generate a response. + # We can safely modify the status code or content + # of the response, but to be safe we won't mess + # with the headers. response = view_func(request) - # Most responses from this view are a JSON dict - # TODO -- explain this more + # Most responses from this view are JSON-encoded + # dictionaries with keys "success", "value", and + # (sometimes) "redirect_url". + # We want to communicate some of this information + # using HTTP status codes instead. try: response_dict = json.loads(response.content) msg = response_dict.get("value", u"") @@ -268,30 +305,35 @@ def shim_student_view(view_func, check_logged_in=False): msg = response.content redirect_url = None - # If the user could not be authenticated + # If the user is not authenticated, and we expect them to be + # send a status 403. if check_logged_in and not request.user.is_authenticated(): response.status_code = 403 response.content = msg - # Handle redirects - # TODO -- explain why this is safe + # If the view wants to redirect us, send a status 302 elif redirect_url is not None: response.status_code = 302 response.content = redirect_url - # Handle errors + # If an error condition occurs, send a status 400 elif response.status_code != 200 or not response_dict.get("success", False): - # TODO -- explain this + # The student views tend to send status 200 even when an error occurs + # If the JSON-serialized content has a value "success" set to False, + # then we know an error occurred. if response.status_code == 200: response.status_code = 400 response.content = msg - # Otherwise, return the response + # If the response is successful, then return the content + # of the response directly rather than including it + # in a JSON-serialized dictionary. else: response.content = msg - # Return the response. - # IMPORTANT: this NEEDS to preserve session variables / cookies! + # Return the response, preserving the original headers. + # This is really important, since the student views set cookies + # that are used elsewhere in the system (such as the marketing site). return response return _inner diff --git a/common/djangoapps/user_api/views.py b/common/djangoapps/user_api/views.py index af3b63b177..5d7254b330 100644 --- a/common/djangoapps/user_api/views.py +++ b/common/djangoapps/user_api/views.py @@ -1,9 +1,10 @@ -"""TODO""" +"""HTTP end-points for the User API. """ from django.conf import settings from django.contrib.auth.models import User -from django.http import HttpResponse, HttpResponseBadRequest +from django.http import HttpResponse from django.core.urlresolvers import reverse +from django.core.exceptions import ImproperlyConfigured from django.utils.translation import ugettext as _ from django.utils.decorators import method_decorator from django.views.decorators.csrf import ensure_csrf_cookie @@ -21,7 +22,7 @@ from django_comment_common.models import Role from opaque_keys.edx.locations import SlashSeparatedCourseKey from user_api.api import account as account_api, profile as profile_api -from user_api.helpers import FormDescription, shim_student_view +from user_api.helpers import FormDescription, shim_student_view, require_post_params class ApiKeyHeaderPermission(permissions.BasePermission): @@ -42,12 +43,24 @@ class ApiKeyHeaderPermission(permissions.BasePermission): class LoginSessionView(APIView): - """TODO""" + """HTTP end-points for logging in users. """ def get(self, request): - """Render a form for allowing a user to log in. + """Return a description of the login form. + + This decouples clients from the API definition: + if the API decides to modify the form, clients won't need + to be updated. + + See `user_api.helpers.FormDescription` for examples + of the JSON-encoded form description. + + Arguments: + request (HttpRequest) + + Returns: + HttpResponse - TODO """ form_desc = FormDescription("post", reverse("user_api_login_session")) @@ -76,32 +89,38 @@ class LoginSessionView(APIView): return HttpResponse(form_desc.to_json(), content_type="application/json") @method_decorator(ensure_csrf_cookie) + @method_decorator(require_post_params(["email", "password"])) def post(self, request): - """Authenticate a user and log them in. + """Log in a user. + + Arguments: + request (HttpRequest) + + Returns: + HttpResponse: 200 on success + HttpResponse: 400 if the request is not valid. + HttpResponse: 403 if authentication failed. + HttpResponse: 302 if redirecting to another page. + + Example Usage: + + POST /user_api/v1/login_session + with POST params `email` and `password` + + 200 OK - TODO """ - # Validate the parameters - # If either param is missing, it's a malformed request - email = request.POST.get("email") - password = request.POST.get("password") - if email is None or password is None: - return HttpResponseBadRequest() - - return self._login_shim(request) - - def _login_shim(self, request): - # Initially, this should be a shim to student views, - # since it will be too much work to re-implement everything there. - # Eventually, we'll want to pull out that functionality into this Django app. + # For the initial implementation, shim the existing login view + # from the student Django app. from student.views import login_user return shim_student_view(login_user, check_logged_in=True)(request) class RegistrationView(APIView): - """TODO""" + """HTTP end-points for creating a new user. """ DEFAULT_FIELDS = ["email", "name", "username", "password"] + EXTRA_FIELDS = [ "city", "country", "level_of_education", "gender", "year_of_birth", "mailing_address", "goals", @@ -110,15 +129,32 @@ class RegistrationView(APIView): def __init__(self, *args, **kwargs): super(RegistrationView, self).__init__(*args, **kwargs) + # Map field names to the instance method used to add the field to the form self.field_handlers = {} for field_name in (self.DEFAULT_FIELDS + self.EXTRA_FIELDS): handler = getattr(self, "_add_{field_name}_field".format(field_name=field_name)) self.field_handlers[field_name] = handler def get(self, request): - """Render a form for allowing the user to register. + """Return a description of the registration form. + + This decouples clients from the API definition: + if the API decides to modify the form, clients won't need + to be updated. + + This is especially important for the registration form, + since different edx-platform installations might + collect different demographic information. + + See `user_api.helpers.FormDescription` for examples + of the JSON-encoded form description. + + Arguments: + request (HttpRequest) + + Returns: + HttpResponse - TODO """ form_desc = FormDescription("post", reverse("user_api_registration")) @@ -126,37 +162,48 @@ class RegistrationView(APIView): for field_name in self.DEFAULT_FIELDS: self.field_handlers[field_name](form_desc, required=True) - # Extra fields from configuration may be required, optional, or hidden - # TODO -- explain error handling here + # Extra fields configured in Django settings + # may be required, optional, or hidden for field_name in self.EXTRA_FIELDS: - field_setting = settings.REGISTRATION_EXTRA_FIELDS.get(field_name) + field_setting = settings.REGISTRATION_EXTRA_FIELDS.get(field_name, "hidden") handler = self.field_handlers[field_name] if field_setting in ["required", "optional"]: handler(form_desc, required=(field_setting == "required")) elif field_setting != "hidden": - # TODO -- warning here - pass + msg = u"Setting REGISTRATION_EXTRA_FIELDS values must be either required, optional, or hidden." + raise ImproperlyConfigured(msg) return HttpResponse(form_desc.to_json(), content_type="application/json") + @method_decorator(ensure_csrf_cookie) def post(self, request): """Create the user's account. - TODO + Arguments: + request (HTTPRequest) + + Returns: + HttpResponse: 200 on success + HttpResponse: 400 if the request is not valid. + HttpResponse: 302 if redirecting to another page. + """ - # Backwards compat: - # TODO -- explain this + # Backwards compatability + # We used to validate that the users had checked + # "honor code" and "terms of service" + # on the registration form. Now we rely on the client + # to display this to users and validate that they + # agree before making the request to this service. request.POST["honor_code"] = "true" request.POST["terms_of_service"] = "true" - # Initially, this should be a shim to student views. - # Eventually, we'll want to pull that functionality into this API. + # For the initial implementation, shim the existing login view + # from the student Django app. from student.views import create_account return shim_student_view(create_account)(request) def _add_email_field(self, form_desc, required=True): - """TODO """ form_desc.add_field( "email", label=_(u"E-mail"), @@ -172,7 +219,6 @@ class RegistrationView(APIView): ) def _add_name_field(self, form_desc, required=True): - """TODO""" form_desc.add_field( "name", label=_(u"Full Name"), @@ -184,7 +230,6 @@ class RegistrationView(APIView): ) def _add_username_field(self, form_desc, required=True): - """TODO""" form_desc.add_field( "username", label=_(u"Public Username"), @@ -197,7 +242,6 @@ class RegistrationView(APIView): ) def _add_password_field(self, form_desc, required=True): - """TODO""" form_desc.add_field( "password", label=_(u"Password"), @@ -209,7 +253,6 @@ class RegistrationView(APIView): ) def _add_level_of_education_field(self, form_desc, required=True): - """ TODO """ form_desc.add_field( "level_of_education", label=_("Highest Level of Education Completed"), @@ -219,7 +262,6 @@ class RegistrationView(APIView): ) def _add_gender_field(self, form_desc, required=True): - """TODO """ form_desc.add_field( "gender", label=_("Gender"), @@ -229,7 +271,6 @@ class RegistrationView(APIView): ) def _add_year_of_birth_field(self, form_desc, required=True): - """TODO """ options = [(unicode(year), unicode(year)) for year in UserProfile.VALID_YEARS] form_desc.add_field( "year_of_birth", @@ -240,7 +281,6 @@ class RegistrationView(APIView): ) def _add_mailing_address_field(self, form_desc, required=True): - """TODO """ form_desc.add_field( "mailing_address", label=_("Mailing Address"), @@ -249,7 +289,6 @@ class RegistrationView(APIView): ) def _add_goals_field(self, form_desc, required=True): - """TODO """ form_desc.add_field( "goals", label=_("Please share with us your reasons for registering with edX"), @@ -258,7 +297,6 @@ class RegistrationView(APIView): ) def _add_city_field(self, form_desc, required=True): - """TODO """ form_desc.add_field( "city", label=_("City"), @@ -266,7 +304,6 @@ class RegistrationView(APIView): ) def _add_country_field(self, form_desc, required=True): - """TODO """ options = [ (country_code, unicode(country_name)) for country_code, country_name in COUNTRIES @@ -280,7 +317,7 @@ class RegistrationView(APIView): ) def _options_with_default(self, options): - """TODO """ + """Include a default option as the first option. """ return ( [("", "--")] + list(options) )