From 8061336102aa37476c728bdd0325acdc572a1205 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 14 Oct 2014 07:38:10 -0400 Subject: [PATCH 01/97] Move user_service into user_api/api --- .../{user_service.py => api/course_tag.py} | 0 ...user_service.py => test_course_tag_api.py} | 14 ++++++------- lms/lib/xblock/runtime.py | 20 +++++++++++-------- 3 files changed, 19 insertions(+), 15 deletions(-) rename common/djangoapps/user_api/{user_service.py => api/course_tag.py} (100%) rename common/djangoapps/user_api/tests/{test_user_service.py => test_course_tag_api.py} (58%) diff --git a/common/djangoapps/user_api/user_service.py b/common/djangoapps/user_api/api/course_tag.py similarity index 100% rename from common/djangoapps/user_api/user_service.py rename to common/djangoapps/user_api/api/course_tag.py diff --git a/common/djangoapps/user_api/tests/test_user_service.py b/common/djangoapps/user_api/tests/test_course_tag_api.py similarity index 58% rename from common/djangoapps/user_api/tests/test_user_service.py rename to common/djangoapps/user_api/tests/test_course_tag_api.py index 366a2cb27b..51b48a56a4 100644 --- a/common/djangoapps/user_api/tests/test_user_service.py +++ b/common/djangoapps/user_api/tests/test_course_tag_api.py @@ -1,10 +1,10 @@ """ -Test the user service +Test the user course tag API. """ from django.test import TestCase from student.tests.factories import UserFactory -from user_api import user_service +from user_api.api import course_tag as course_tag_api from opaque_keys.edx.locations import SlashSeparatedCourseKey @@ -19,17 +19,17 @@ class TestUserService(TestCase): def test_get_set_course_tag(self): # get a tag that doesn't exist - tag = user_service.get_course_tag(self.user, self.course_id, self.test_key) + tag = course_tag_api.get_course_tag(self.user, self.course_id, self.test_key) self.assertIsNone(tag) # test setting a new key test_value = 'value' - user_service.set_course_tag(self.user, self.course_id, self.test_key, test_value) - tag = user_service.get_course_tag(self.user, self.course_id, self.test_key) + course_tag_api.set_course_tag(self.user, self.course_id, self.test_key, test_value) + tag = course_tag_api.get_course_tag(self.user, self.course_id, self.test_key) self.assertEqual(tag, test_value) #test overwriting an existing key test_value = 'value2' - user_service.set_course_tag(self.user, self.course_id, self.test_key, test_value) - tag = user_service.get_course_tag(self.user, self.course_id, self.test_key) + course_tag_api.set_course_tag(self.user, self.course_id, self.test_key, test_value) + tag = course_tag_api.get_course_tag(self.user, self.course_id, self.test_key) self.assertEqual(tag, test_value) diff --git a/lms/lib/xblock/runtime.py b/lms/lib/xblock/runtime.py index f02657c511..2c1729ec49 100644 --- a/lms/lib/xblock/runtime.py +++ b/lms/lib/xblock/runtime.py @@ -7,7 +7,7 @@ import xblock.reference.plugins from django.core.urlresolvers import reverse from django.conf import settings -from user_api import user_service +from user_api.api import course_tag as user_course_tag_api from xmodule.modulestore.django import modulestore from xmodule.x_module import ModuleSystem from xmodule.partitions.partitions_service import PartitionService @@ -144,7 +144,7 @@ class UserTagsService(object): the current course id and current user. """ - COURSE_SCOPE = user_service.COURSE_SCOPE + COURSE_SCOPE = user_course_tag_api.COURSE_SCOPE def __init__(self, runtime): self.runtime = runtime @@ -161,11 +161,13 @@ class UserTagsService(object): scope: the current scope of the runtime key: the key for the value we want """ - if scope != user_service.COURSE_SCOPE: + if scope != user_course_tag_api.COURSE_SCOPE: raise ValueError("unexpected scope {0}".format(scope)) - return user_service.get_course_tag(self._get_current_user(), - self.runtime.course_id, key) + return user_course_tag_api.get_course_tag( + self._get_current_user(), + self.runtime.course_id, key + ) def set_tag(self, scope, key, value): """ @@ -175,11 +177,13 @@ class UserTagsService(object): key: the key that to the value to be set value: the value to set """ - if scope != user_service.COURSE_SCOPE: + if scope != user_course_tag_api.COURSE_SCOPE: raise ValueError("unexpected scope {0}".format(scope)) - return user_service.set_course_tag(self._get_current_user(), - self.runtime.course_id, key, value) + return user_course_tag_api.set_course_tag( + self._get_current_user(), + self.runtime.course_id, key, value + ) class LmsModuleSystem(LmsHandlerUrls, ModuleSystem): # pylint: disable=abstract-method From e89afa93c018cfe8d1d19cf2fc56f2d564925908 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 14 Oct 2014 08:23:38 -0400 Subject: [PATCH 02/97] WIP: add login and registration end-points to the user API. --- common/djangoapps/user_api/api/profile.py | 12 +- common/djangoapps/user_api/helpers.py | 241 ++++++++ .../djangoapps/user_api/tests/test_views.py | 571 +++++++++++++++++- common/djangoapps/user_api/urls.py | 2 + common/djangoapps/user_api/views.py | 261 +++++++- .../student_account/test/test_views.py | 32 + lms/djangoapps/student_account/urls.py | 15 +- lms/djangoapps/student_account/views.py | 37 +- lms/djangoapps/student_profile/urls.py | 18 +- lms/envs/test.py | 11 + .../student_account/login_and_register.html | 52 ++ lms/urls.py | 10 +- 12 files changed, 1234 insertions(+), 28 deletions(-) create mode 100644 lms/templates/student_account/login_and_register.html diff --git a/common/djangoapps/user_api/api/profile.py b/common/djangoapps/user_api/api/profile.py index fc14e47273..4b18b6c1e6 100644 --- a/common/djangoapps/user_api/api/profile.py +++ b/common/djangoapps/user_api/api/profile.py @@ -65,9 +65,15 @@ def profile_info(username): return None profile_dict = { - u'username': profile.user.username, - u'email': profile.user.email, - u'full_name': profile.name, + "username": profile.user.username, + "email": profile.user.email, + "full_name": profile.name, + "level_of_education": profile.level_of_education, + "mailing_address": profile.mailing_address, + "year_of_birth": profile.year_of_birth, + "goals": profile.goals, + "city": profile.city, + "country": profile.country, } return profile_dict diff --git a/common/djangoapps/user_api/helpers.py b/common/djangoapps/user_api/helpers.py index 1cae378786..cd926983b9 100644 --- a/common/djangoapps/user_api/helpers.py +++ b/common/djangoapps/user_api/helpers.py @@ -4,6 +4,8 @@ This is NOT part of the public API. """ from functools import wraps import logging +import json + LOGGER = logging.getLogger(__name__) @@ -54,3 +56,242 @@ def intercept_errors(api_error, ignore_errors=[]): raise api_error(msg) return _wrapped return _decorator + + +class InvalidFieldError(Exception): + """The provided field definition is not valid. """ + + +class FormDescription(object): + """Generate a JSON representation of a form. """ + + ALLOWED_TYPES = ["text", "select", "textarea"] + + ALLOWED_RESTRICTIONS = { + "text": ["min_length", "max_length"], + } + + def __init__(self, method, submit_url): + """Configure how the form should be submitted. + + Args: + method (unicode): The HTTP method used to submit the form. + submit_url (unicode): The URL where the form should be submitted. + + """ + self.method = method + self.submit_url = submit_url + self.fields = [] + + def add_field( + self, name, label=u"", field_type=u"text", default=u"", + placeholder=u"", instructions=u"", required=True, restrictions=None, + options=None + ): + """Add a field to the form description. + + Args: + name (unicode): The name of the field, which is the key for the value + to send back to the server. + + Keyword Arguments: + label (unicode): The label for the field (e.g. "E-mail" or "Username") + + field_type (unicode): The type of the field. See `ALLOWED_TYPES` for + acceptable values. + + default (unicode): The default value for the field. + + placeholder (unicode): Placeholder text in the field + (e.g. "user@example.com" for an email field) + + instructions (unicode): Short instructions for using the field + (e.g. "This is the email address you used when you registered.") + + required (boolean): Whether the field is required or optional. + + restrictions (dict): Validation restrictions for the field. + See `ALLOWED_RESTRICTIONS` for acceptable values. + + options (list): For "select" fields, a list of tuples + (value, display_name) representing the options available to + the user. `value` is the value of the field to send to the server, + and `display_name` is the name to display to the user. + If the field type is "select", you *must* provide this kwarg. + + Raises: + InvalidFieldError + + """ + if field_type not in self.ALLOWED_TYPES: + msg = u"Field type '{field_type}' is not a valid type. Allowed types are: {allowed}.".format( + field_type=field_type, + allowed=", ".join(self.ALLOWED_TYPES) + ) + raise InvalidFieldError(msg) + + field_dict = { + "label": label, + "name": name, + "type": field_type, + "default": default, + "placeholder": placeholder, + "instructions": instructions, + "required": required, + "restrictions": {} + } + + if field_type == "select": + if options is not None: + field_dict["options"] = [ + {"value": option_value, "name": option_name} + for option_value, option_name in options + ] + else: + raise InvalidFieldError("You must provide options for a select field.") + + if restrictions is not None: + allowed_restrictions = self.ALLOWED_RESTRICTIONS.get(field_type, []) + for key, val in restrictions.iteritems(): + if key in allowed_restrictions: + field_dict["restrictions"][key] = val + else: + msg = "Restriction '{restriction}' is not allowed for field type '{field_type}'".format( + restriction=key, + field_type=field_type + ) + raise InvalidFieldError(msg) + + self.fields.append(field_dict) + + def to_json(self): + """Create a JSON representation of the form description. + + Here's an example of the output: + { + "method": "post", + "submit_url": "/submit", + "fields": [ + { + "name": "cheese_or_wine", + "label": "Cheese or Wine?", + "default": "cheese", + "type": "select", + "required": True, + "placeholder": "", + "instructions": "", + "options": [ + {"value": "cheese", "name": "Cheese"}, + {"value": "wine", "name": "Wine"} + ] + "restrictions": {}, + }, + { + "name": "comments", + "label": "comments", + "default": "", + "type": "text", + "required": False, + "placeholder": "Any comments?", + "instructions": "Please enter additional comments here." + "restrictions": { + "max_length": 200 + } + }, + ... + ] + } + + If the field is NOT a "select" type, then the "options" + key will be omitted. + + Returns: + unicode + """ + return json.dumps({ + "method": self.method, + "submit_url": self.submit_url, + "fields": self.fields + }) + + +def shim_student_view(view_func, check_logged_in=False): + """Create a "shim" view for a view function from the student Django app. + + Specifically, we need to: + * Strip out enrollment params, since the client for the new registration/login + page will communicate with the enrollment API to update enrollments. + + * Return responses with HTTP status codes indicating success/failure + (instead of always using status 200, but setting "success" to False in + the JSON-serialized content of the response) + + * Use status code 302 for redirects instead of + "redirect_url" in the JSON-serialized content of the response. + + * Use status code 403 to indicate a login failure. + + The shim will preserve any cookies set by the view. + + Arguments: + view_func (function): The view function from the student Django app. + + Keyword Args: + check_logged_in (boolean): If true, check whether the user successfully + authenticated and if not set the status to 403. + + Returns: + function + + """ + + @wraps(view_func) + def _inner(request): + + # Strip out enrollment action stuff, since we're handling that elsewhere + 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 ^^ + response = view_func(request) + + # Most responses from this view are a JSON dict + # TODO -- explain this more + try: + response_dict = json.loads(response.content) + msg = response_dict.get("value", u"") + redirect_url = response_dict.get("redirect_url") + except (ValueError, TypeError): + msg = response.content + redirect_url = None + + # If the user could not be authenticated + 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 + elif redirect_url is not None: + response.status_code = 302 + response.content = redirect_url + + # Handle errors + elif response.status_code != 200 or not response_dict.get("success", False): + # TODO -- explain this + if response.status_code == 200: + response.status_code = 400 + response.content = msg + + # Otherwise, return the response + else: + response.content = msg + + # Return the response. + # IMPORTANT: this NEEDS to preserve session variables / cookies! + return response + + return _inner diff --git a/common/djangoapps/user_api/tests/test_views.py b/common/djangoapps/user_api/tests/test_views.py index ddebd1e5f4..a8f25b5085 100644 --- a/common/djangoapps/user_api/tests/test_views.py +++ b/common/djangoapps/user_api/tests/test_views.py @@ -1,12 +1,22 @@ -import base64 +"""Tests for the user API at the HTTP request level. """ -from django.test import TestCase -from django.test.utils import override_settings +import datetime +import base64 import json import re -from student.tests.factories import UserFactory + +from django.core.urlresolvers import reverse +from django.core import mail +from django.test import TestCase +from django.test.utils import override_settings from unittest import SkipTest -from user_api.models import UserPreference +import ddt +from pytz import UTC +from django_countries.countries import COUNTRIES + +from user_api.api import account as account_api, profile as profile_api + +from student.tests.factories import UserFactory from user_api.tests.factories import UserPreferenceFactory from django_comment_common import models from opaque_keys.edx.locations import SlashSeparatedCourseKey @@ -532,3 +542,554 @@ class PreferenceUsersListViewTest(UserApiTestCase): self.assertUserIsValid(user) all_user_uris = [user["url"] for user in first_page_users + second_page_users] self.assertEqual(len(set(all_user_uris)), 2) + + +class LoginSessionViewTest(ApiTestCase): + """Tests for the login end-points of the user API. """ + + USERNAME = "bob" + EMAIL = "bob@example.com" + PASSWORD = "password" + + def setUp(self): + super(LoginSessionViewTest, self).setUp() + self.url = reverse("user_api_login_session") + + def test_allowed_methods(self): + self.assertAllowedMethods(self.url, ["GET", "POST", "HEAD", "OPTIONS"]) + + def test_put_not_allowed(self): + response = self.client.put(self.url) + self.assertHttpMethodNotAllowed(response) + + def test_delete_not_allowed(self): + response = self.client.delete(self.url) + self.assertHttpMethodNotAllowed(response) + + def test_patch_not_allowed(self): + raise SkipTest("Django 1.4's test client does not support patch") + + def test_login_form(self): + # Retrieve the login form + response = self.client.get(self.url, content_type="application/json") + self.assertHttpOK(response) + + # Verify that the form description matches what we expect + form_desc = json.loads(response.content) + self.assertEqual(form_desc["method"], "post") + self.assertEqual(form_desc["submit_url"], self.url) + self.assertEqual(form_desc["fields"], [ + { + u"name": u"email", + u"default": u"", + u"type": u"text", + u"required": True, + u"label": u"E-mail", + u"placeholder": u"example: username@domain.com", + u"instructions": u"This is the e-mail address you used to register with edX", + u"restrictions": { + u"min_length": 3, + u"max_length": 254 + }, + }, + { + u"name": u"password", + u"default": u"", + u"type": u"text", + u"required": True, + u"label": u"Password", + u"placeholder": u"", + u"instructions": u"", + u"restrictions": { + u"min_length": 2, + u"max_length": 75 + }, + }, + ]) + + def test_login(self): + # Create a test user + UserFactory.create(username=self.USERNAME, email=self.EMAIL, password=self.PASSWORD) + + # Login + response = self.client.post(self.url, { + "email": self.EMAIL, + "password": self.PASSWORD, + }) + self.assertHttpOK(response) + + # Verify that we logged in successfully by accessing + # a page that requires authentication. + response = self.client.get(reverse("dashboard")) + self.assertHttpOK(response) + + def test_invalid_credentials(self): + # Create a test user + UserFactory.create(username=self.USERNAME, email=self.EMAIL, password=self.PASSWORD) + + # Invalid password + response = self.client.post(self.url, { + "email": self.EMAIL, + "password": "invalid" + }) + self.assertHttpForbidden(response) + + # Invalid email address + response = self.client.post(self.url, { + "email": "invalid@example.com", + "password": self.PASSWORD, + }) + self.assertHttpForbidden(response) + + def test_missing_login_params(self): + # Create a test user + UserFactory.create(username=self.USERNAME, email=self.EMAIL, password=self.PASSWORD) + + # Missing password + response = self.client.post(self.url, { + "email": self.EMAIL, + }) + self.assertHttpBadRequest(response) + + # Missing email + response = self.client.post(self.url, { + "password": self.PASSWORD, + }) + self.assertHttpBadRequest(response) + + # Missing both email and password + response = self.client.post(self.url, {}) + + +@ddt.ddt +class RegistrationViewTest(ApiTestCase): + """Tests for the registration end-points of the User API. """ + + USERNAME = "bob" + EMAIL = "bob@example.com" + PASSWORD = "password" + NAME = "Bob Smith" + EDUCATION = "m" + YEAR_OF_BIRTH = "1998" + ADDRESS = "123 Fake Street" + CITY = "Springfield" + COUNTRY = "us" + GOALS = "Learn all the things!" + + def setUp(self): + super(RegistrationViewTest, self).setUp() + self.url = reverse("user_api_registration") + + def test_allowed_methods(self): + self.assertAllowedMethods(self.url, ["GET", "POST", "HEAD", "OPTIONS"]) + + def test_put_not_allowed(self): + response = self.client.put(self.url) + self.assertHttpMethodNotAllowed(response) + + def test_delete_not_allowed(self): + response = self.client.delete(self.url) + self.assertHttpMethodNotAllowed(response) + + def test_patch_not_allowed(self): + raise SkipTest("Django 1.4's test client does not support patch") + + def test_register_form_default_fields(self): + no_extra_fields_setting = {} + + self._assert_reg_field( + no_extra_fields_setting, + { + u"name": u"email", + u"default": u"", + u"type": u"text", + u"required": True, + u"label": u"E-mail", + u"placeholder": u"example: username@domain.com", + u"instructions": u"This is the e-mail address you used to register with edX", + u"restrictions": { + u"min_length": 3, + u"max_length": 254 + }, + } + ) + + self._assert_reg_field( + no_extra_fields_setting, + { + u"name": u"name", + u"default": u"", + u"type": u"text", + u"required": True, + u"label": u"Full Name", + u"placeholder": u"", + u"instructions": u"Needed for any certificates you may earn", + u"restrictions": { + "max_length": 255, + } + } + ) + + self._assert_reg_field( + no_extra_fields_setting, + { + u"name": u"username", + u"default": u"", + u"type": u"text", + u"required": True, + u"label": u"Public Username", + u"placeholder": u"", + u"instructions": u"Will be shown in any discussions or forums you participate in (cannot be changed)", + u"restrictions": { + u"min_length": 2, + u"max_length": 30, + } + } + ) + + self._assert_reg_field( + no_extra_fields_setting, + { + u"name": u"password", + u"default": u"", + u"type": u"text", + u"required": True, + u"label": u"Password", + u"placeholder": u"", + u"instructions": u"", + u"restrictions": { + u"min_length": 2, + u"max_length": 75 + }, + } + ) + + def test_register_form_level_of_education(self): + self._assert_reg_field( + {"level_of_education": "optional"}, + { + "name": "level_of_education", + "default": "", + "type": "select", + "required": False, + "label": "Highest Level of Education Completed", + "placeholder": "", + "instructions": "", + "options": [ + {"value": "", "name": "--"}, + {"value": "p", "name": "Doctorate"}, + {"value": "m", "name": "Master's or professional degree"}, + {"value": "b", "name": "Bachelor's degree"}, + {"value": "a", "name": "Associate's degree"}, + {"value": "hs", "name": "Secondary/high school"}, + {"value": "jhs", "name": "Junior secondary/junior high/middle school"}, + {"value": "el", "name": "Elementary/primary school"}, + {"value": "none", "name": "None"}, + {"value": "other", "name": "Other"}, + ], + "restrictions": {}, + } + ) + + def test_register_form_gender(self): + self._assert_reg_field( + {"gender": "optional"}, + { + "name": "gender", + "default": "", + "type": "select", + "required": False, + "label": "Gender", + "placeholder": "", + "instructions": "", + "options": [ + {"value": "", "name": "--"}, + {"value": "m", "name": "Male"}, + {"value": "f", "name": "Female"}, + {"value": "o", "name": "Other"}, + ], + "restrictions": {}, + } + ) + + def test_register_form_year_of_birth(self): + this_year = datetime.datetime.now(UTC).year + year_options = ( + [{"value": "", "name": "--"}] + [ + {"value": unicode(year), "name": unicode(year)} + for year in range(this_year, this_year - 120, -1) + ] + ) + self._assert_reg_field( + {"year_of_birth": "optional"}, + { + "name": "year_of_birth", + "default": "", + "type": "select", + "required": False, + "label": "Year of Birth", + "placeholder": "", + "instructions": "", + "options": year_options, + "restrictions": {}, + } + ) + + def test_registration_form_mailing_address(self): + self._assert_reg_field( + {"mailing_address": "optional"}, + { + "name": "mailing_address", + "default": "", + "type": "textarea", + "required": False, + "label": "Mailing Address", + "placeholder": "", + "instructions": "", + "restrictions": {}, + } + ) + + def test_registration_form_goals(self): + self._assert_reg_field( + {"goals": "optional"}, + { + "name": "goals", + "default": "", + "type": "textarea", + "required": False, + "label": "Please share with us your reasons for registering with edX", + "placeholder": "", + "instructions": "", + "restrictions": {}, + } + ) + + def test_registration_form_city(self): + self._assert_reg_field( + {"city": "optional"}, + { + "name": "city", + "default": "", + "type": "text", + "required": False, + "label": "City", + "placeholder": "", + "instructions": "", + "restrictions": {}, + } + ) + + def test_registration_form_country(self): + country_options = ( + [{"name": "--", "value": ""}] + + [ + {"value": country_code, "name": unicode(country_name)} + for country_code, country_name in COUNTRIES + ] + ) + self._assert_reg_field( + {"country": "required"}, + { + "label": "Country", + "name": "country", + "default": "", + "type": "select", + "required": True, + "placeholder": "", + "instructions": "", + "options": country_options, + "restrictions": {}, + } + ) + + @override_settings(REGISTRATION_EXTRA_FIELDS={ + "level_of_education": "optional", + "gender": "optional", + "year_of_birth": "optional", + "mailing_address": "optional", + "goals": "optional", + "city": "optional", + "country": "required", + }) + def test_field_order(self): + response = self.client.get(self.url) + self.assertHttpOK(response) + + # Verify that all fields render in the correct order + form_desc = json.loads(response.content) + field_names = [field["name"] for field in form_desc["fields"]] + self.assertEqual(field_names, [ + "email", + "name", + "username", + "password", + "city", + "country", + "level_of_education", + "gender", + "year_of_birth", + "mailing_address", + "goals", + ]) + + def test_register(self): + # Create a new registration + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": self.NAME, + "username": self.USERNAME, + "password": self.PASSWORD, + }) + self.assertHttpOK(response) + + # Verify that the user exists + self.assertEqual( + account_api.account_info(self.USERNAME), + { + "username": self.USERNAME, + "email": self.EMAIL, + "is_active": False + } + ) + + # Verify that the user's full name is set + profile_info = profile_api.profile_info(self.USERNAME) + self.assertEqual(profile_info["full_name"], self.NAME) + + # Verify that we've been logged in + # by trying to access a page that requires authentication + response = self.client.get(reverse("dashboard")) + self.assertHttpOK(response) + + @override_settings(REGISTRATION_EXTRA_FIELDS={ + "level_of_education": "optional", + "gender": "optional", + "year_of_birth": "optional", + "mailing_address": "optional", + "goals": "optional", + "city": "optional", + "country": "required", + }) + def test_register_with_profile_info(self): + # Register, providing lots of demographic info + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": self.NAME, + "username": self.USERNAME, + "password": self.PASSWORD, + "level_of_education": self.EDUCATION, + "mailing_address": self.ADDRESS, + "year_of_birth": self.YEAR_OF_BIRTH, + "goals": self.GOALS, + "city": self.CITY, + "country": self.COUNTRY + }) + self.assertHttpOK(response) + + # Verify the profile information + profile_info = profile_api.profile_info(self.USERNAME) + self.assertEqual(profile_info["level_of_education"], self.EDUCATION) + self.assertEqual(profile_info["mailing_address"], self.ADDRESS) + self.assertEqual(profile_info["year_of_birth"], int(self.YEAR_OF_BIRTH)) + self.assertEqual(profile_info["goals"], self.GOALS) + self.assertEqual(profile_info["city"], self.CITY) + self.assertEqual(profile_info["country"], self.COUNTRY) + + def test_activation_email(self): + # Register, which should trigger an activation email + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": self.NAME, + "username": self.USERNAME, + "password": self.PASSWORD, + }) + self.assertHttpOK(response) + + # Verify that the activation email was sent + self.assertEqual(len(mail.outbox), 1) + sent_email = mail.outbox[0] + self.assertEqual(sent_email.to, [self.EMAIL]) + self.assertEqual(sent_email.subject, "Activate Your edX Account") + self.assertIn("activate your account", sent_email.body) + + @ddt.data( + {"email": ""}, + {"email": "invalid"}, + {"name": ""}, + {"username": ""}, + {"username": "a"}, + {"password": ""}, + ) + def test_register_invalid_input(self, invalid_fields): + # Initially, the field values are all valid + data = { + "email": self.EMAIL, + "name": self.NAME, + "username": self.USERNAME, + "password": self.PASSWORD, + } + + # Override the valid fields, making the input invalid + data.update(invalid_fields) + + # Attempt to create the account, expecting an error response + response = self.client.post(self.url, data) + self.assertHttpBadRequest(response) + + + @override_settings(REGISTRATION_EXTRA_FIELDS={"country": "required"}) + @ddt.data("email", "name", "username", "password", "country") + def test_register_missing_required_field(self, missing_field): + data = { + "email": self.EMAIL, + "name": self.NAME, + "username": self.USERNAME, + "password": self.PASSWORD, + "country": self.COUNTRY, + } + + del data[missing_field] + + # Send a request missing a field + response = self.client.post(self.url, data) + self.assertHttpBadRequest(response) + + def test_register_already_authenticated(self): + data = { + "email": self.EMAIL, + "name": self.NAME, + "username": self.USERNAME, + "password": self.PASSWORD, + } + + # Register once, which will also log us in + response = self.client.post(self.url, data) + self.assertHttpOK(response) + + # Try to register again + response = self.client.post(self.url, data) + self.assertHttpBadRequest(response) + + def _assert_reg_field(self, extra_fields_setting, expected_field): + """Retrieve the registration form description from the server and + verify that it contains the expected field. + + Args: + extra_fields_setting (dict): Override the Django setting controlling + which extra fields are displayed in the form. + + expected_field (dict): The field definition we expect to find in the form. + + Raises: + AssertionError + + """ + # Retrieve the registration form description + with override_settings(REGISTRATION_EXTRA_FIELDS=extra_fields_setting): + response = self.client.get(self.url) + self.assertHttpOK(response) + + # Verify that the form description matches what we'd expect + form_desc = json.loads(response.content) + self.assertIn(expected_field, form_desc["fields"]) diff --git a/common/djangoapps/user_api/urls.py b/common/djangoapps/user_api/urls.py index 9fd20194ea..f73c4969fa 100644 --- a/common/djangoapps/user_api/urls.py +++ b/common/djangoapps/user_api/urls.py @@ -10,6 +10,8 @@ user_api_router.register(r'user_prefs', user_api_views.UserPreferenceViewSet) urlpatterns = patterns( '', url(r'^v1/', include(user_api_router.urls)), + url(r'^v1/account/login_session/$', user_api_views.LoginSessionView.as_view(), name="user_api_login_session"), + url(r'^v1/account/registration/$', user_api_views.RegistrationView.as_view(), name="user_api_registration"), url( r'^v1/preferences/(?P{})/users/$'.format(UserPreference.KEY_REGEX), user_api_views.PreferenceUsersListView.as_view() diff --git a/common/djangoapps/user_api/views.py b/common/djangoapps/user_api/views.py index d007d7a9aa..af3b63b177 100644 --- a/common/djangoapps/user_api/views.py +++ b/common/djangoapps/user_api/views.py @@ -1,18 +1,28 @@ +"""TODO""" + from django.conf import settings from django.contrib.auth.models import User +from django.http import HttpResponse, HttpResponseBadRequest +from django.core.urlresolvers import reverse +from django.utils.translation import ugettext as _ +from django.utils.decorators import method_decorator +from django.views.decorators.csrf import ensure_csrf_cookie from rest_framework import authentication from rest_framework import filters from rest_framework import generics from rest_framework import permissions -from rest_framework import status from rest_framework import viewsets +from rest_framework.views import APIView from rest_framework.exceptions import ParseError -from rest_framework.response import Response +from django_countries.countries import COUNTRIES from user_api.serializers import UserSerializer, UserPreferenceSerializer -from user_api.models import UserPreference +from user_api.models import UserPreference, UserProfile 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 + class ApiKeyHeaderPermission(permissions.BasePermission): def has_permission(self, request, view): @@ -31,6 +41,251 @@ class ApiKeyHeaderPermission(permissions.BasePermission): ) +class LoginSessionView(APIView): + """TODO""" + + def get(self, request): + """Render a form for allowing a user to log in. + + TODO + """ + form_desc = FormDescription("post", reverse("user_api_login_session")) + + form_desc.add_field( + "email", + label=_(u"E-mail"), + placeholder=_(u"example: username@domain.com"), + instructions=_( + u"This is the e-mail address you used to register with {platform}" + ).format(platform=settings.PLATFORM_NAME), + restrictions={ + "min_length": account_api.EMAIL_MIN_LENGTH, + "max_length": account_api.EMAIL_MAX_LENGTH, + } + ) + + form_desc.add_field( + "password", + label=_(u"Password"), + restrictions={ + "min_length": account_api.PASSWORD_MIN_LENGTH, + "max_length": account_api.PASSWORD_MAX_LENGTH, + } + ) + + return HttpResponse(form_desc.to_json(), content_type="application/json") + + @method_decorator(ensure_csrf_cookie) + def post(self, request): + """Authenticate a user and log them in. + + 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. + from student.views import login_user + return shim_student_view(login_user, check_logged_in=True)(request) + + +class RegistrationView(APIView): + """TODO""" + + DEFAULT_FIELDS = ["email", "name", "username", "password"] + EXTRA_FIELDS = [ + "city", "country", "level_of_education", "gender", + "year_of_birth", "mailing_address", "goals", + ] + + def __init__(self, *args, **kwargs): + super(RegistrationView, self).__init__(*args, **kwargs) + + 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. + + TODO + """ + form_desc = FormDescription("post", reverse("user_api_registration")) + + # Default fields are always required + 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 + for field_name in self.EXTRA_FIELDS: + field_setting = settings.REGISTRATION_EXTRA_FIELDS.get(field_name) + 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 + + return HttpResponse(form_desc.to_json(), content_type="application/json") + + def post(self, request): + """Create the user's account. + + TODO + """ + # Backwards compat: + # TODO -- explain this + 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. + 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"), + placeholder=_(u"example: username@domain.com"), + instructions=_( + u"This is the e-mail address you used to register with {platform}" + ).format(platform=settings.PLATFORM_NAME), + restrictions={ + "min_length": account_api.EMAIL_MIN_LENGTH, + "max_length": account_api.EMAIL_MAX_LENGTH, + }, + required=required + ) + + def _add_name_field(self, form_desc, required=True): + """TODO""" + form_desc.add_field( + "name", + label=_(u"Full Name"), + instructions=_(u"Needed for any certificates you may earn"), + restrictions={ + "max_length": profile_api.FULL_NAME_MAX_LENGTH, + }, + required=required + ) + + def _add_username_field(self, form_desc, required=True): + """TODO""" + form_desc.add_field( + "username", + label=_(u"Public Username"), + instructions=_(u"Will be shown in any discussions or forums you participate in (cannot be changed)"), + restrictions={ + "min_length": account_api.USERNAME_MIN_LENGTH, + "max_length": account_api.USERNAME_MAX_LENGTH, + }, + required=required + ) + + def _add_password_field(self, form_desc, required=True): + """TODO""" + form_desc.add_field( + "password", + label=_(u"Password"), + restrictions={ + "min_length": account_api.PASSWORD_MIN_LENGTH, + "max_length": account_api.PASSWORD_MAX_LENGTH, + }, + required=required + ) + + 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"), + field_type="select", + options=self._options_with_default(UserProfile.LEVEL_OF_EDUCATION_CHOICES), + required=required + ) + + def _add_gender_field(self, form_desc, required=True): + """TODO """ + form_desc.add_field( + "gender", + label=_("Gender"), + field_type="select", + options=self._options_with_default(UserProfile.GENDER_CHOICES), + required=required + ) + + 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", + label=_("Year of Birth"), + field_type="select", + options=self._options_with_default(options), + required=required + ) + + def _add_mailing_address_field(self, form_desc, required=True): + """TODO """ + form_desc.add_field( + "mailing_address", + label=_("Mailing Address"), + field_type="textarea", + required=required + ) + + 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"), + field_type="textarea", + required=required + ) + + def _add_city_field(self, form_desc, required=True): + """TODO """ + form_desc.add_field( + "city", + label=_("City"), + required=required + ) + + def _add_country_field(self, form_desc, required=True): + """TODO """ + options = [ + (country_code, unicode(country_name)) + for country_code, country_name in COUNTRIES + ] + form_desc.add_field( + "country", + label=_("Country"), + field_type="select", + options=self._options_with_default(options), + required=required + ) + + def _options_with_default(self, options): + """TODO """ + return ( + [("", "--")] + list(options) + ) + + class UserViewSet(viewsets.ReadOnlyModelViewSet): authentication_classes = (authentication.SessionAuthentication,) permission_classes = (ApiKeyHeaderPermission,) diff --git a/lms/djangoapps/student_account/test/test_views.py b/lms/djangoapps/student_account/test/test_views.py index 0b2447c3ea..0f6900adad 100644 --- a/lms/djangoapps/student_account/test/test_views.py +++ b/lms/djangoapps/student_account/test/test_views.py @@ -3,6 +3,7 @@ import re from urllib import urlencode +import json from mock import patch import ddt from django.test import TestCase @@ -60,6 +61,37 @@ class StudentAccountViewTest(UrlResetMixin, TestCase): response = self.client.get(reverse('account_index')) self.assertContains(response, "Student Account") + @ddt.data( + ("login", "login"), + ("register", "register"), + ) + @ddt.unpack + def test_login_and_registration_form(self, url_name, initial_mode): + response = self.client.get(reverse(url_name)) + expected_data = u"data-initial-mode=\"{mode}\"".format(mode=initial_mode) + self.assertContains(response, expected_data) + + @ddt.data("login", "register") + def test_login_and_registration_third_party_auth_urls(self, url_name): + response = self.client.get(reverse(url_name)) + + # This relies on the THIRD_PARTY_AUTH configuration in the test settings + expected_data = u"data-third-party-auth-providers=\"{providers}\"".format( + providers=json.dumps([ + { + u'icon_class': u'icon-facebook', + u'login_url': u'/auth/login/facebook/?auth_entry=login', + u'name': u'Facebook' + }, + { + u'icon_class': u'icon-google-plus', + u'login_url': u'/auth/login/google-oauth2/?auth_entry=login', + u'name': u'Google' + } + ]) + ) + self.assertContains(response, expected_data) + def test_change_email(self): response = self._change_email(self.NEW_EMAIL, self.PASSWORD) self.assertEquals(response.status_code, 200) diff --git a/lms/djangoapps/student_account/urls.py b/lms/djangoapps/student_account/urls.py index bb5a0d5690..d6e3b71233 100644 --- a/lms/djangoapps/student_account/urls.py +++ b/lms/djangoapps/student_account/urls.py @@ -1,8 +1,17 @@ from django.conf.urls import patterns, url +from django.conf import settings + urlpatterns = patterns( 'student_account.views', - url(r'^$', 'index', name='account_index'), - url(r'^email$', 'email_change_request_handler', name='email_change_request'), - url(r'^email/confirmation/(?P[^/]*)$', 'email_change_confirmation_handler', name='email_change_confirm'), + url(r'^login/$', 'login_and_registration_form', {'initial_mode': 'login'}, name='login'), + url(r'^register/$', 'login_and_registration_form', {'initial_mode': 'register'}, name='register'), ) + +if settings.FEATURES.get('ENABLE_NEW_DASHBOARD'): + urlpatterns += patterns( + 'student_account.views', + url(r'^$', 'index', name='account_index'), + url(r'^email$', 'email_change_request_handler', name='email_change_request'), + url(r'^email/confirmation/(?P[^/]*)$', 'email_change_confirmation_handler', name='email_change_confirm'), + ) \ No newline at end of file diff --git a/lms/djangoapps/student_account/views.py b/lms/djangoapps/student_account/views.py index b20e19d051..c64818cf21 100644 --- a/lms/djangoapps/student_account/views.py +++ b/lms/djangoapps/student_account/views.py @@ -1,15 +1,16 @@ """ Views for a student's account information. """ +import json from django.conf import settings from django.http import ( - QueryDict, HttpResponse, - HttpResponseBadRequest, HttpResponseServerError + HttpResponse, HttpResponseBadRequest, HttpResponseServerError ) from django.core.mail import send_mail from django_future.csrf import ensure_csrf_cookie from django.contrib.auth.decorators import login_required from django.views.decorators.http import require_http_methods from edxmako.shortcuts import render_to_response, render_to_string +import third_party_auth from microsite_configuration import microsite from user_api.api import account as account_api @@ -41,6 +42,38 @@ def index(request): ) +@require_http_methods(['GET']) +def login_and_registration_form(request, initial_mode="login"): + """Render the combined login/registration form, defaulting to login + + This relies on the JS to asynchronously load the actual form from + the user_api. + + Keyword Args: + initial_mode (string): Either "login" or "registration". + + """ + context = { + 'disable_courseware_js': True, + 'initial_mode': initial_mode, + 'third_party_auth_providers': json.dumps([]) + } + + if microsite.get_value("ENABLE_THIRD_PARTY_AUTH", settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH")): + context["third_party_auth_providers"] = json.dumps([ + { + "name": enabled.NAME, + "icon_class": enabled.ICON_CLASS, + "login_url": third_party_auth.pipeline.get_login_url( + enabled.NAME, third_party_auth.pipeline.AUTH_ENTRY_LOGIN + ), + } + for enabled in third_party_auth.provider.Registry.enabled() + ]) + + return render_to_response('student_account/login_and_register.html', context) + + @login_required @require_http_methods(['POST']) @ensure_csrf_cookie diff --git a/lms/djangoapps/student_profile/urls.py b/lms/djangoapps/student_profile/urls.py index ba4cebeba1..48d20711d9 100644 --- a/lms/djangoapps/student_profile/urls.py +++ b/lms/djangoapps/student_profile/urls.py @@ -1,8 +1,14 @@ from django.conf.urls import patterns, url +from django.conf import settings -urlpatterns = patterns( - 'student_profile.views', - url(r'^$', 'index', name='profile_index'), - url(r'^preferences$', 'preference_handler', name='preference_handler'), - url(r'^preferences/languages$', 'language_info', name='language_info'), -) + +urlpatterns = [] + + +if settings.FEATURES.get('ENABLE_NEW_DASHBOARD'): + urlpatterns = patterns( + 'student_profile.views', + url(r'^$', 'index', name='profile_index'), + url(r'^preferences$', 'preference_handler', name='preference_handler'), + url(r'^preferences/languages$', 'language_info', name='language_info'), + ) diff --git a/lms/envs/test.py b/lms/envs/test.py index a2587ee7c5..c807101b9a 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -203,6 +203,17 @@ simplefilter('ignore') # Change to "default" to see the first instance of each ######### Third-party auth ########## FEATURES['ENABLE_THIRD_PARTY_AUTH'] = True +THIRD_PARTY_AUTH = { + "Google": { + "SOCIAL_AUTH_GOOGLE_OAUTH2_KEY": "test", + "SOCIAL_AUTH_GOOGLE_OAUTH2_SECRET": "test", + }, + "Facebook": { + "SOCIAL_AUTH_GOOGLE_OAUTH2_KEY": "test", + "SOCIAL_AUTH_GOOGLE_OAUTH2_SECRET": "test", + }, +} + ################################## OPENID ##################################### FEATURES['AUTH_USE_OPENID'] = True FEATURES['AUTH_USE_OPENID_PROVIDER'] = True diff --git a/lms/templates/student_account/login_and_register.html b/lms/templates/student_account/login_and_register.html new file mode 100644 index 0000000000..19ed498fd8 --- /dev/null +++ b/lms/templates/student_account/login_and_register.html @@ -0,0 +1,52 @@ +<%! from django.utils.translation import ugettext as _ %> +<%namespace name='static' file='/static_content.html'/> + +<%inherit file="../main.html" /> + +<%block name="pagetitle">${_("Login and Register")} + +<%block name="js_extra"> + + + <%static:js group='student_account'/> + + +<%block name="header_extras"> +% for template_name in ["account"]: + +% endfor + + +

Login and Registration!

+ +

This is a placeholder for the combined login and registration form

+ +## TODO: Use JavaScript to populate this div with +## the actual registration/login forms (loaded asynchronously from the user API) +## The URLS for the forms are: +## - GET /user_api/v1/registration/ +## - GET /user_api/v1/login_session/ +## +## You can post back to those URLs with JSON-serialized +## data from the form fields in order to complete the registration +## or login. +## +## Also TODO: we need to figure out how to enroll students in +## a course if they got here from a course about page. +## +## third_party_auth_providers is a JSON-serialized list of +## dictionaries of the form: +## { +## "name": "Facebook", +## "icon_class": "facebook-icon", +## "login_url": "http://api.facebook.com/auth" +## } +## +## Note that this list may be empty. +## +
diff --git a/lms/urls.py b/lms/urls.py index 4f746e6fe6..5445900c65 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -375,6 +375,10 @@ if settings.COURSEWARE_ENABLED: # LTI endpoints listing url(r'^courses/{}/lti_rest_endpoints/'.format(settings.COURSE_ID_PATTERN), 'courseware.views.get_course_lti_endpoints', name='lti_rest_endpoints'), + + # Student account and profile + url(r'^account/', include('student_account.urls')), + url(r'^profile/', include('student_profile.urls')), ) # allow course staff to change to student view of courseware @@ -537,12 +541,6 @@ if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH'): url(r'', include('third_party_auth.urls')), ) -# If enabled, expose the URLs for the new dashboard, account, and profile pages -if settings.FEATURES.get('ENABLE_NEW_DASHBOARD'): - urlpatterns += ( - url(r'^profile/', include('student_profile.urls')), - url(r'^account/', include('student_account.urls')), - ) urlpatterns = patterns(*urlpatterns) From 7ef9ec838ff603baa0e096a34183bb3a1f3d00a3 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Thu, 16 Oct 2014 09:12:25 -0400 Subject: [PATCH 03/97] 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) ) From 8761eaf3dd020ae2b5b5371cf9f44fb158bd56b4 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Thu, 16 Oct 2014 10:20:21 -0400 Subject: [PATCH 04/97] Added check for duplicate email and username --- common/djangoapps/user_api/api/account.py | 30 +++++++++ .../djangoapps/user_api/tests/test_views.py | 63 ++++++++++++++++--- common/djangoapps/user_api/views.py | 14 +++++ 3 files changed, 97 insertions(+), 10 deletions(-) diff --git a/common/djangoapps/user_api/api/account.py b/common/djangoapps/user_api/api/account.py index 4281326860..0bc66fb267 100644 --- a/common/djangoapps/user_api/api/account.py +++ b/common/djangoapps/user_api/api/account.py @@ -1,11 +1,13 @@ """Python API for user accounts. + Account information includes a student's username, password, and email address, but does NOT include user profile information (i.e., demographic information and preferences). """ from django.db import transaction, IntegrityError +from django.db.models import Q from django.core.validators import validate_email, validate_slug, ValidationError from user_api.models import User, UserProfile, Registration, PendingEmailChange from user_api.helpers import intercept_errors @@ -138,6 +140,34 @@ def create_account(username, password, email): return registration.activation_key +def check_account_exists(username=None, email=None): + """Check whether an account with a particular username or email already exists. + + Keyword Arguments: + username (unicode) + email (unicode) + + Returns: + list of conflicting fields + + Example Usage: + >>> account_api.check_account_exists(username="bob") + [] + >>> account_api.check_account_exists(username="ted", email="ted@example.com") + ["email", "username"] + + """ + conflicts = [] + + if email is not None and User.objects.filter(email=email).exists(): + conflicts.append("email") + + if username is not None and User.objects.filter(username=username).exists(): + conflicts.append("username") + + return conflicts + + @intercept_errors(AccountInternalError, ignore_errors=[AccountRequestError]) def account_info(username): """Retrieve information about a user's account. diff --git a/common/djangoapps/user_api/tests/test_views.py b/common/djangoapps/user_api/tests/test_views.py index a8f25b5085..5677c0b99f 100644 --- a/common/djangoapps/user_api/tests/test_views.py +++ b/common/djangoapps/user_api/tests/test_views.py @@ -1037,7 +1037,6 @@ class RegistrationViewTest(ApiTestCase): response = self.client.post(self.url, data) self.assertHttpBadRequest(response) - @override_settings(REGISTRATION_EXTRA_FIELDS={"country": "required"}) @ddt.data("email", "name", "username", "password", "country") def test_register_missing_required_field(self, missing_field): @@ -1055,21 +1054,65 @@ class RegistrationViewTest(ApiTestCase): response = self.client.post(self.url, data) self.assertHttpBadRequest(response) - def test_register_already_authenticated(self): - data = { + def test_register_duplicate_email(self): + # Register the first user + response = self.client.post(self.url, { "email": self.EMAIL, "name": self.NAME, "username": self.USERNAME, "password": self.PASSWORD, - } - - # Register once, which will also log us in - response = self.client.post(self.url, data) + }) self.assertHttpOK(response) - # Try to register again - response = self.client.post(self.url, data) - self.assertHttpBadRequest(response) + # Try to create a second user with the same email address + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": "Someone Else", + "username": "someone_else", + "password": self.PASSWORD, + }) + self.assertEqual(response.status_code, 409) + self.assertEqual(response.content, json.dumps(["email"])) + + def test_register_duplicate_username(self): + # Register the first user + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": self.NAME, + "username": self.USERNAME, + "password": self.PASSWORD, + }) + self.assertHttpOK(response) + + # Try to create a second user with the same username + response = self.client.post(self.url, { + "email": "someone+else@example.com", + "name": "Someone Else", + "username": self.USERNAME, + "password": self.PASSWORD, + }) + self.assertEqual(response.status_code, 409) + self.assertEqual(response.content, json.dumps(["username"])) + + def test_register_duplicate_username_and_email(self): + # Register the first user + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": self.NAME, + "username": self.USERNAME, + "password": self.PASSWORD, + }) + self.assertHttpOK(response) + + # Try to create a second user with the same username + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": "Someone Else", + "username": self.USERNAME, + "password": self.PASSWORD, + }) + self.assertEqual(response.status_code, 409) + self.assertEqual(response.content, json.dumps(["email", "username"])) def _assert_reg_field(self, extra_fields_setting, expected_field): """Retrieve the registration form description from the server and diff --git a/common/djangoapps/user_api/views.py b/common/djangoapps/user_api/views.py index 5d7254b330..b25814a748 100644 --- a/common/djangoapps/user_api/views.py +++ b/common/djangoapps/user_api/views.py @@ -1,4 +1,5 @@ """HTTP end-points for the User API. """ +import json from django.conf import settings from django.contrib.auth.models import User @@ -177,6 +178,7 @@ class RegistrationView(APIView): return HttpResponse(form_desc.to_json(), content_type="application/json") @method_decorator(ensure_csrf_cookie) + @method_decorator(require_post_params(DEFAULT_FIELDS)) def post(self, request): """Create the user's account. @@ -198,6 +200,18 @@ class RegistrationView(APIView): request.POST["honor_code"] = "true" request.POST["terms_of_service"] = "true" + # Handle duplicate username/email + conflicts = account_api.check_account_exists( + username=request.POST.get('username'), + email=request.POST.get('email') + ) + if conflicts: + return HttpResponse( + status=409, + content=json.dumps(conflicts), + content_type="application/json" + ) + # For the initial implementation, shim the existing login view # from the student Django app. from student.views import create_account From c0582cc4376fda4f09ffc2a488d28db9767dfb9c Mon Sep 17 00:00:00 2001 From: Will Daly Date: Thu, 16 Oct 2014 10:24:00 -0400 Subject: [PATCH 05/97] Fix broken profile API test --- common/djangoapps/user_api/api/profile.py | 2 +- common/djangoapps/user_api/tests/test_profile_api.py | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/user_api/api/profile.py b/common/djangoapps/user_api/api/profile.py index 4b18b6c1e6..35356b6256 100644 --- a/common/djangoapps/user_api/api/profile.py +++ b/common/djangoapps/user_api/api/profile.py @@ -73,7 +73,7 @@ def profile_info(username): "year_of_birth": profile.year_of_birth, "goals": profile.goals, "city": profile.city, - "country": profile.country, + "country": unicode(profile.country), } return profile_dict diff --git a/common/djangoapps/user_api/tests/test_profile_api.py b/common/djangoapps/user_api/tests/test_profile_api.py index 823985588c..5f78c0a832 100644 --- a/common/djangoapps/user_api/tests/test_profile_api.py +++ b/common/djangoapps/user_api/tests/test_profile_api.py @@ -28,6 +28,12 @@ class ProfileApiTest(TestCase): 'username': self.USERNAME, 'email': self.EMAIL, 'full_name': u'', + 'goals': None, + 'level_of_education': None, + 'mailing_address': None, + 'year_of_birth': None, + 'country': '', + 'city': None, }) def test_update_full_name(self): From 52051df4d44b72b5ba1ce61f9434b4135c9b52a7 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Thu, 16 Oct 2014 14:03:41 -0400 Subject: [PATCH 06/97] Fix some bugs in the student view shim, add tests --- common/djangoapps/user_api/helpers.py | 6 +- .../djangoapps/user_api/tests/test_helpers.py | 123 +++++++++++++++++- 2 files changed, 126 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/user_api/helpers.py b/common/djangoapps/user_api/helpers.py index bd9752eb15..0621278074 100644 --- a/common/djangoapps/user_api/helpers.py +++ b/common/djangoapps/user_api/helpers.py @@ -300,9 +300,11 @@ def shim_student_view(view_func, check_logged_in=False): try: response_dict = json.loads(response.content) msg = response_dict.get("value", u"") - redirect_url = response_dict.get("redirect_url") + redirect_url = response_dict.get("redirect_url") or response_dict.get("redirect") + success = response_dict.get("success") except (ValueError, TypeError): msg = response.content + success = True redirect_url = None # If the user is not authenticated, and we expect them to be @@ -317,7 +319,7 @@ def shim_student_view(view_func, check_logged_in=False): response.content = redirect_url # If an error condition occurs, send a status 400 - elif response.status_code != 200 or not response_dict.get("success", False): + elif response.status_code != 200 or not success: # 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. diff --git a/common/djangoapps/user_api/tests/test_helpers.py b/common/djangoapps/user_api/tests/test_helpers.py index 7daf5c2a4b..49ddff9dfa 100644 --- a/common/djangoapps/user_api/tests/test_helpers.py +++ b/common/djangoapps/user_api/tests/test_helpers.py @@ -1,10 +1,16 @@ """ Tests for helper functions. """ +import json import mock +import ddt from django.test import TestCase from nose.tools import raises -from user_api.helpers import intercept_errors +from django.http import HttpRequest, HttpResponse +from user_api.helpers import ( + intercept_errors, shim_student_view, + FormDescription, InvalidFieldError +) class FakeInputException(Exception): @@ -64,3 +70,118 @@ class InterceptErrorsTest(TestCase): # This will include the stack trace for the original exception # because it's called with log level "ERROR" mock_logger.exception.assert_called_once_with(expected_log_msg) + + +class FormDescriptionTest(TestCase): + + def test_to_json(self): + desc = FormDescription("post", "/submit") + desc.add_field( + "name", + label="label", + field_type="text", + default="default", + placeholder="placeholder", + instructions="instructions", + required=True, + restrictions={ + "min_length": 2, + "max_length": 10 + } + ) + + self.assertEqual(desc.to_json(), json.dumps({ + "method": "post", + "submit_url": "/submit", + "fields": [ + { + "name": "name", + "label": "label", + "type": "text", + "default": "default", + "placeholder": "placeholder", + "instructions": "instructions", + "required": True, + "restrictions": { + "min_length": 2, + "max_length": 10, + } + } + ] + })) + + def test_invalid_field_type(self): + desc = FormDescription("post", "/submit") + with self.assertRaises(InvalidFieldError): + desc.add_field("invalid", field_type="invalid") + + def test_missing_options(self): + desc = FormDescription("post", "/submit") + with self.assertRaises(InvalidFieldError): + desc.add_field("name", field_type="select") + + def test_invalid_restriction(self): + desc = FormDescription("post", "/submit") + with self.assertRaises(InvalidFieldError): + desc.add_field("name", field_type="text", restrictions={"invalid": 0}) + + +@ddt.ddt +class StudentViewShimTest(TestCase): + + def setUp(self): + self.captured_request = None + + def test_strip_enrollment_action(self): + view = self._shimmed_view(HttpResponse()) + request = HttpRequest() + request.POST["enrollment_action"] = "enroll" + request.POST["course_id"] = "edx/101/demo" + view(request) + + # Expect that the enrollment action and course ID + # were stripped out before reaching the wrapped view. + self.assertNotIn("enrollment_action", self.captured_request.POST) + self.assertNotIn("course_id", self.captured_request.POST) + + def test_non_json_response(self): + view = self._shimmed_view(HttpResponse(content="Not a JSON dict")) + response = view(HttpRequest()) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.content, "Not a JSON dict") + + @ddt.data("redirect", "redirect_url") + def test_redirect_from_json(self, redirect_key): + view = self._shimmed_view( + HttpResponse(content=json.dumps({ + "success": True, + redirect_key: "/redirect" + })) + ) + response = view(HttpRequest()) + self.assertEqual(response.status_code, 302) + self.assertEqual(response.content, "/redirect") + + def test_error_from_json(self): + view = self._shimmed_view( + HttpResponse(content=json.dumps({ + "success": False, + "value": "Error!" + })) + ) + response = view(HttpRequest()) + self.assertEqual(response.status_code, 400) + self.assertEqual(response.content, "Error!") + + def test_preserve_headers(self): + view_response = HttpResponse() + view_response["test-header"] = "test" + view = self._shimmed_view(view_response) + response = view(HttpRequest()) + self.assertEqual(response["test-header"], "test") + + def _shimmed_view(self, response): + def stub_view(request): + self.captured_request = request + return response + return shim_student_view(stub_view) From e65fe4ed035bdea0bcd5ef26254af54c0fd5920d Mon Sep 17 00:00:00 2001 From: Will Daly Date: Fri, 17 Oct 2014 13:17:13 -0400 Subject: [PATCH 07/97] Add remember me checkbox to the login page --- common/djangoapps/user_api/helpers.py | 2 +- .../djangoapps/user_api/tests/test_views.py | 79 ++++++++++++++----- common/djangoapps/user_api/views.py | 8 ++ 3 files changed, 68 insertions(+), 21 deletions(-) diff --git a/common/djangoapps/user_api/helpers.py b/common/djangoapps/user_api/helpers.py index 0621278074..b591e41783 100644 --- a/common/djangoapps/user_api/helpers.py +++ b/common/djangoapps/user_api/helpers.py @@ -94,7 +94,7 @@ class InvalidFieldError(Exception): class FormDescription(object): """Generate a JSON representation of a form. """ - ALLOWED_TYPES = ["text", "select", "textarea"] + ALLOWED_TYPES = ["text", "select", "textarea", "checkbox"] ALLOWED_RESTRICTIONS = { "text": ["min_length", "max_length"], diff --git a/common/djangoapps/user_api/tests/test_views.py b/common/djangoapps/user_api/tests/test_views.py index 5677c0b99f..3954505428 100644 --- a/common/djangoapps/user_api/tests/test_views.py +++ b/common/djangoapps/user_api/tests/test_views.py @@ -544,6 +544,7 @@ class PreferenceUsersListViewTest(UserApiTestCase): self.assertEqual(len(set(all_user_uris)), 2) +@ddt.ddt class LoginSessionViewTest(ApiTestCase): """Tests for the login end-points of the user API. """ @@ -580,31 +581,41 @@ class LoginSessionViewTest(ApiTestCase): self.assertEqual(form_desc["submit_url"], self.url) self.assertEqual(form_desc["fields"], [ { - u"name": u"email", - u"default": u"", - u"type": u"text", - u"required": True, - u"label": u"E-mail", - u"placeholder": u"example: username@domain.com", - u"instructions": u"This is the e-mail address you used to register with edX", - u"restrictions": { - u"min_length": 3, - u"max_length": 254 + "name": "email", + "default": "", + "type": "text", + "required": True, + "label": "E-mail", + "placeholder": "example: username@domain.com", + "instructions": "This is the e-mail address you used to register with edX", + "restrictions": { + "min_length": 3, + "max_length": 254 }, }, { - u"name": u"password", - u"default": u"", - u"type": u"text", - u"required": True, - u"label": u"Password", - u"placeholder": u"", - u"instructions": u"", - u"restrictions": { - u"min_length": 2, - u"max_length": 75 + "name": "password", + "default": "", + "type": "text", + "required": True, + "label": "Password", + "placeholder": "", + "instructions": "", + "restrictions": { + "min_length": 2, + "max_length": 75 }, }, + { + "name": "remember", + "default": False, + "type": "checkbox", + "required": False, + "label": "Remember me", + "placeholder": "", + "instructions": "", + "restrictions": {}, + } ]) def test_login(self): @@ -623,6 +634,34 @@ class LoginSessionViewTest(ApiTestCase): response = self.client.get(reverse("dashboard")) self.assertHttpOK(response) + @ddt.data( + (json.dumps(True), False), + (json.dumps(False), True), + (None, True), + ) + @ddt.unpack + def test_login_remember_me(self, remember_value, expire_at_browser_close): + # Create a test user + UserFactory.create(username=self.USERNAME, email=self.EMAIL, password=self.PASSWORD) + + # Login and remember me + data = { + "email": self.EMAIL, + "password": self.PASSWORD, + } + + if remember_value is not None: + data["remember"] = remember_value + + response = self.client.post(self.url, data) + self.assertHttpOK(response) + + # Verify that the session expiration was set correctly + self.assertEqual( + self.client.session.get_expire_at_browser_close(), + expire_at_browser_close + ) + def test_invalid_credentials(self): # Create a test user UserFactory.create(username=self.USERNAME, email=self.EMAIL, password=self.PASSWORD) diff --git a/common/djangoapps/user_api/views.py b/common/djangoapps/user_api/views.py index b25814a748..dde98e092e 100644 --- a/common/djangoapps/user_api/views.py +++ b/common/djangoapps/user_api/views.py @@ -87,6 +87,14 @@ class LoginSessionView(APIView): } ) + form_desc.add_field( + "remember", + field_type="checkbox", + label=_("Remember me"), + default=False, + required=False, + ) + return HttpResponse(form_desc.to_json(), content_type="application/json") @method_decorator(ensure_csrf_cookie) From be0033eefec28c2000571c743b430ace55062d5b Mon Sep 17 00:00:00 2001 From: Will Daly Date: Fri, 17 Oct 2014 13:41:10 -0400 Subject: [PATCH 08/97] Use password field type for the login form password --- common/djangoapps/user_api/helpers.py | 3 ++- common/djangoapps/user_api/tests/test_views.py | 2 +- common/djangoapps/user_api/views.py | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/user_api/helpers.py b/common/djangoapps/user_api/helpers.py index b591e41783..9bc296fe13 100644 --- a/common/djangoapps/user_api/helpers.py +++ b/common/djangoapps/user_api/helpers.py @@ -94,10 +94,11 @@ class InvalidFieldError(Exception): class FormDescription(object): """Generate a JSON representation of a form. """ - ALLOWED_TYPES = ["text", "select", "textarea", "checkbox"] + ALLOWED_TYPES = ["text", "select", "textarea", "checkbox", "password"] ALLOWED_RESTRICTIONS = { "text": ["min_length", "max_length"], + "password": ["min_length", "max_length"], } def __init__(self, method, submit_url): diff --git a/common/djangoapps/user_api/tests/test_views.py b/common/djangoapps/user_api/tests/test_views.py index 3954505428..8d16e02d0f 100644 --- a/common/djangoapps/user_api/tests/test_views.py +++ b/common/djangoapps/user_api/tests/test_views.py @@ -596,7 +596,7 @@ class LoginSessionViewTest(ApiTestCase): { "name": "password", "default": "", - "type": "text", + "type": "password", "required": True, "label": "Password", "placeholder": "", diff --git a/common/djangoapps/user_api/views.py b/common/djangoapps/user_api/views.py index dde98e092e..a35e4cf259 100644 --- a/common/djangoapps/user_api/views.py +++ b/common/djangoapps/user_api/views.py @@ -81,6 +81,7 @@ class LoginSessionView(APIView): form_desc.add_field( "password", label=_(u"Password"), + field_type="password", restrictions={ "min_length": account_api.PASSWORD_MIN_LENGTH, "max_length": account_api.PASSWORD_MAX_LENGTH, From efec188b69c80498c61f0639b376c371b7e004a6 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Fri, 17 Oct 2014 15:01:21 -0400 Subject: [PATCH 09/97] Fix unit tests --- common/djangoapps/user_api/tests/test_views.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/user_api/tests/test_views.py b/common/djangoapps/user_api/tests/test_views.py index 8d16e02d0f..a84fa7bd1e 100644 --- a/common/djangoapps/user_api/tests/test_views.py +++ b/common/djangoapps/user_api/tests/test_views.py @@ -5,11 +5,12 @@ import base64 import json import re +from django.conf import settings from django.core.urlresolvers import reverse from django.core import mail from django.test import TestCase from django.test.utils import override_settings -from unittest import SkipTest +from unittest import SkipTest, skipUnless import ddt from pytz import UTC from django_countries.countries import COUNTRIES @@ -545,6 +546,7 @@ class PreferenceUsersListViewTest(UserApiTestCase): @ddt.ddt +@skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') class LoginSessionViewTest(ApiTestCase): """Tests for the login end-points of the user API. """ @@ -701,6 +703,7 @@ class LoginSessionViewTest(ApiTestCase): @ddt.ddt +@skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') class RegistrationViewTest(ApiTestCase): """Tests for the registration end-points of the User API. """ From 6250c95d2e31ed264f38bd940080c55c2d316dfb Mon Sep 17 00:00:00 2001 From: Will Daly Date: Fri, 17 Oct 2014 16:06:40 -0400 Subject: [PATCH 10/97] Fix immutable querydict error --- common/djangoapps/user_api/views.py | 1 + 1 file changed, 1 insertion(+) diff --git a/common/djangoapps/user_api/views.py b/common/djangoapps/user_api/views.py index a35e4cf259..9a50a1966e 100644 --- a/common/djangoapps/user_api/views.py +++ b/common/djangoapps/user_api/views.py @@ -206,6 +206,7 @@ class RegistrationView(APIView): # 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 = request.POST.copy() request.POST["honor_code"] = "true" request.POST["terms_of_service"] = "true" From d558a36127d3810d8d33b0069453ad0089f23407 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Fri, 17 Oct 2014 16:16:30 -0400 Subject: [PATCH 11/97] Fix unit test failures due to duplicate url names Reset the correct URLs module to fix test failures. Fix third party auth to reconfigure the registry on test cleanup. --- .../third_party_auth/tests/testutil.py | 2 + common/djangoapps/util/testing.py | 39 ++++++++++++++----- .../student_account/test/test_views.py | 8 ++-- lms/djangoapps/student_account/urls.py | 4 +- .../student_profile/test/test_views.py | 2 +- 5 files changed, 38 insertions(+), 17 deletions(-) diff --git a/common/djangoapps/third_party_auth/tests/testutil.py b/common/djangoapps/third_party_auth/tests/testutil.py index 6907cea26e..f4f3600df7 100644 --- a/common/djangoapps/third_party_auth/tests/testutil.py +++ b/common/djangoapps/third_party_auth/tests/testutil.py @@ -30,8 +30,10 @@ class TestCase(unittest.TestCase): def setUp(self): super(TestCase, self).setUp() + self._original_providers = provider.Registry._get_all() provider.Registry._reset() def tearDown(self): provider.Registry._reset() + provider.Registry.configure_once(self._original_providers) super(TestCase, self).tearDown() diff --git a/common/djangoapps/util/testing.py b/common/djangoapps/util/testing.py index fdd61a2a6a..311ed89244 100644 --- a/common/djangoapps/util/testing.py +++ b/common/djangoapps/util/testing.py @@ -19,19 +19,38 @@ class UrlResetMixin(object): that affect the contents of urls.py """ - def _reset_urls(self, urlconf=None): - if urlconf is None: - urlconf = settings.ROOT_URLCONF - - if urlconf in sys.modules: - reload(sys.modules[urlconf]) + def _reset_urls(self, urlconf_modules): + for urlconf in urlconf_modules: + if urlconf in sys.modules: + reload(sys.modules[urlconf]) clear_url_caches() # Resolve a URL so that the new urlconf gets loaded resolve('/') - def setUp(self, **kwargs): - """Reset django default urlconf before tests and after tests""" + def setUp(self, *args, **kwargs): + """Reset Django urls before tests and after tests + + If you need to reset `urls.py` from a particular Django app (or apps), + specify these modules in *args. + + Examples: + + # Reload only the root urls.py + super(MyTestCase, self).setUp() + + # Reload urls from my_app + super(MyTestCase, self).setUp("my_app.urls") + + # Reload urls from my_app and another_app + super(MyTestCase, self).setUp("my_app.urls", "another_app.urls") + + """ super(UrlResetMixin, self).setUp(**kwargs) - self._reset_urls() - self.addCleanup(self._reset_urls) + + urlconf_modules = [settings.ROOT_URLCONF] + if args: + urlconf_modules.extend(args) + + self._reset_urls(urlconf_modules) + self.addCleanup(lambda: self._reset_urls(urlconf_modules)) diff --git a/lms/djangoapps/student_account/test/test_views.py b/lms/djangoapps/student_account/test/test_views.py index 0f6900adad..198b438a1d 100644 --- a/lms/djangoapps/student_account/test/test_views.py +++ b/lms/djangoapps/student_account/test/test_views.py @@ -47,7 +47,7 @@ class StudentAccountViewTest(UrlResetMixin, TestCase): @patch.dict(settings.FEATURES, {'ENABLE_NEW_DASHBOARD': True}) def setUp(self): - super(StudentAccountViewTest, self).setUp() + super(StudentAccountViewTest, self).setUp("student_account.urls") # Create/activate a new account activation_key = account_api.create_account(self.USERNAME, self.PASSWORD, self.OLD_EMAIL) @@ -62,8 +62,8 @@ class StudentAccountViewTest(UrlResetMixin, TestCase): self.assertContains(response, "Student Account") @ddt.data( - ("login", "login"), - ("register", "register"), + ("account_login", "login"), + ("account_register", "register"), ) @ddt.unpack def test_login_and_registration_form(self, url_name, initial_mode): @@ -71,7 +71,7 @@ class StudentAccountViewTest(UrlResetMixin, TestCase): expected_data = u"data-initial-mode=\"{mode}\"".format(mode=initial_mode) self.assertContains(response, expected_data) - @ddt.data("login", "register") + @ddt.data("account_login", "account_register") def test_login_and_registration_third_party_auth_urls(self, url_name): response = self.client.get(reverse(url_name)) diff --git a/lms/djangoapps/student_account/urls.py b/lms/djangoapps/student_account/urls.py index d6e3b71233..0c8908ec5a 100644 --- a/lms/djangoapps/student_account/urls.py +++ b/lms/djangoapps/student_account/urls.py @@ -4,8 +4,8 @@ from django.conf import settings urlpatterns = patterns( 'student_account.views', - url(r'^login/$', 'login_and_registration_form', {'initial_mode': 'login'}, name='login'), - url(r'^register/$', 'login_and_registration_form', {'initial_mode': 'register'}, name='register'), + url(r'^login/$', 'login_and_registration_form', {'initial_mode': 'login'}, name='account_login'), + url(r'^register/$', 'login_and_registration_form', {'initial_mode': 'register'}, name='account_register'), ) if settings.FEATURES.get('ENABLE_NEW_DASHBOARD'): diff --git a/lms/djangoapps/student_profile/test/test_views.py b/lms/djangoapps/student_profile/test/test_views.py index db30146986..c24bd0ea0a 100644 --- a/lms/djangoapps/student_profile/test/test_views.py +++ b/lms/djangoapps/student_profile/test/test_views.py @@ -35,7 +35,7 @@ class StudentProfileViewTest(UrlResetMixin, TestCase): @patch.dict(settings.FEATURES, {'ENABLE_NEW_DASHBOARD': True}) def setUp(self): - super(StudentProfileViewTest, self).setUp() + super(StudentProfileViewTest, self).setUp("student_profile.urls") # Create/activate a new account activation_key = account_api.create_account(self.USERNAME, self.PASSWORD, self.EMAIL) From 97f3f09009296a645dd0534e9bc80858089120b5 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Mon, 20 Oct 2014 08:59:23 -0400 Subject: [PATCH 12/97] Add honor code to registration dialog. --- common/djangoapps/user_api/helpers.py | 12 +++++ .../djangoapps/user_api/tests/test_views.py | 50 ++++++++++++++++++- common/djangoapps/user_api/views.py | 46 +++++++++++++---- 3 files changed, 96 insertions(+), 12 deletions(-) diff --git a/common/djangoapps/user_api/helpers.py b/common/djangoapps/user_api/helpers.py index 9bc296fe13..3be67abea6 100644 --- a/common/djangoapps/user_api/helpers.py +++ b/common/djangoapps/user_api/helpers.py @@ -276,6 +276,8 @@ def shim_student_view(view_func, check_logged_in=False): """ @wraps(view_func) def _inner(request): + # Ensure that the POST querydict is mutable + request.POST = request.POST.copy() # The login and registration handlers in student view try to change # the user's enrollment status if these parameters are present. @@ -287,6 +289,16 @@ def shim_student_view(view_func, check_logged_in=False): if "course_id" in request.POST: del request.POST["course_id"] + # Backwards compatibility: the student view expects both + # terms of service and honor code values. Since we're combining + # these into a single checkbox, the only value we may get + # from the new view is "honor_code". + # Longer term, we will need to make this more flexible to support + # open source installations that may have separate checkboxes + # for TOS, privacy policy, etc. + if request.POST.get("honor_code") is not None and request.POST.get("terms_of_service") is None: + request.POST["terms_of_service"] = request.POST.get("honor_code") + # 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 diff --git a/common/djangoapps/user_api/tests/test_views.py b/common/djangoapps/user_api/tests/test_views.py index cd4af1388e..1edc3e8a65 100644 --- a/common/djangoapps/user_api/tests/test_views.py +++ b/common/djangoapps/user_api/tests/test_views.py @@ -13,6 +13,7 @@ from django.test.utils import override_settings from unittest import SkipTest, skipUnless import ddt from pytz import UTC +from mock import patch from user_api.api import account as account_api, profile as profile_api @@ -946,6 +947,42 @@ class RegistrationViewTest(ApiTestCase): } ) + @override_settings( + MKTG_URLS={"ROOT": "https://www.test.com/", "HONOR": "honor"}, + ) + @patch.dict(settings.FEATURES, {"ENABLE_MKTG_SITE": True}) + def test_registration_honor_code_mktg_site_enabled(self): + self._assert_reg_field( + {"honor_code": "required"}, + { + "label": "I agree to the Terms of Service and Honor Code", + "name": "honor_code", + "default": False, + "type": "checkbox", + "required": True, + "placeholder": "", + "instructions": "", + "restrictions": {}, + } + ) + + @override_settings(MKTG_URLS_LINK_MAP={"HONOR": "honor"}) + @patch.dict(settings.FEATURES, {"ENABLE_MKTG_SITE": False}) + def test_registration_honor_code_mktg_site_disabled(self): + self._assert_reg_field( + {"honor_code": "required"}, + { + "label": "I agree to the Terms of Service and Honor Code", + "name": "honor_code", + "default": False, + "type": "checkbox", + "required": True, + "placeholder": "", + "instructions": "", + "restrictions": {}, + } + ) + @override_settings(REGISTRATION_EXTRA_FIELDS={ "level_of_education": "optional", "gender": "optional", @@ -954,6 +991,7 @@ class RegistrationViewTest(ApiTestCase): "goals": "optional", "city": "optional", "country": "required", + "honor_code": "required", }) def test_field_order(self): response = self.client.get(self.url) @@ -974,6 +1012,7 @@ class RegistrationViewTest(ApiTestCase): "year_of_birth", "mailing_address", "goals", + "honor_code", ]) def test_register(self): @@ -983,6 +1022,7 @@ class RegistrationViewTest(ApiTestCase): "name": self.NAME, "username": self.USERNAME, "password": self.PASSWORD, + "honor_code": "true", }) self.assertHttpOK(response) @@ -1026,7 +1066,8 @@ class RegistrationViewTest(ApiTestCase): "year_of_birth": self.YEAR_OF_BIRTH, "goals": self.GOALS, "city": self.CITY, - "country": self.COUNTRY + "country": self.COUNTRY, + "honor_code": "true", }) self.assertHttpOK(response) @@ -1046,6 +1087,7 @@ class RegistrationViewTest(ApiTestCase): "name": self.NAME, "username": self.USERNAME, "password": self.PASSWORD, + "honor_code": "true", }) self.assertHttpOK(response) @@ -1104,6 +1146,7 @@ class RegistrationViewTest(ApiTestCase): "name": self.NAME, "username": self.USERNAME, "password": self.PASSWORD, + "honor_code": "true", }) self.assertHttpOK(response) @@ -1113,6 +1156,7 @@ class RegistrationViewTest(ApiTestCase): "name": "Someone Else", "username": "someone_else", "password": self.PASSWORD, + "honor_code": "true", }) self.assertEqual(response.status_code, 409) self.assertEqual(response.content, json.dumps(["email"])) @@ -1124,6 +1168,7 @@ class RegistrationViewTest(ApiTestCase): "name": self.NAME, "username": self.USERNAME, "password": self.PASSWORD, + "honor_code": "true", }) self.assertHttpOK(response) @@ -1133,6 +1178,7 @@ class RegistrationViewTest(ApiTestCase): "name": "Someone Else", "username": self.USERNAME, "password": self.PASSWORD, + "honor_code": "true", }) self.assertEqual(response.status_code, 409) self.assertEqual(response.content, json.dumps(["username"])) @@ -1144,6 +1190,7 @@ class RegistrationViewTest(ApiTestCase): "name": self.NAME, "username": self.USERNAME, "password": self.PASSWORD, + "honor_code": "true", }) self.assertHttpOK(response) @@ -1153,6 +1200,7 @@ class RegistrationViewTest(ApiTestCase): "name": "Someone Else", "username": self.USERNAME, "password": self.PASSWORD, + "honor_code": "true", }) self.assertEqual(response.status_code, 409) self.assertEqual(response.content, json.dumps(["email", "username"])) diff --git a/common/djangoapps/user_api/views.py b/common/djangoapps/user_api/views.py index e964cdff99..3059c08168 100644 --- a/common/djangoapps/user_api/views.py +++ b/common/djangoapps/user_api/views.py @@ -1,4 +1,5 @@ """HTTP end-points for the User API. """ +import copy import json from django.conf import settings @@ -21,6 +22,7 @@ from user_api.serializers import UserSerializer, UserPreferenceSerializer from user_api.models import UserPreference, UserProfile from django_comment_common.models import Role from opaque_keys.edx.locations import SlashSeparatedCourseKey +from edxmako.shortcuts import marketing_link from user_api.api import account as account_api, profile as profile_api from user_api.helpers import FormDescription, shim_student_view, require_post_params @@ -134,6 +136,7 @@ class RegistrationView(APIView): EXTRA_FIELDS = [ "city", "country", "level_of_education", "gender", "year_of_birth", "mailing_address", "goals", + "honor_code", ] def __init__(self, *args, **kwargs): @@ -172,10 +175,15 @@ class RegistrationView(APIView): for field_name in self.DEFAULT_FIELDS: self.field_handlers[field_name](form_desc, required=True) + # Backwards compatibility: Honor code is required by default, unless + # explicitly set to "optional" in Django settings. + extra_fields_setting = copy.deepcopy(settings.REGISTRATION_EXTRA_FIELDS) + extra_fields_setting["honor_code"] = extra_fields_setting.get("honor_code", "required") + # 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, "hidden") + field_setting = extra_fields_setting.get(field_name, "hidden") handler = self.field_handlers[field_name] if field_setting in ["required", "optional"]: @@ -200,16 +208,6 @@ class RegistrationView(APIView): HttpResponse: 302 if redirecting to another page. """ - # 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 = request.POST.copy() - request.POST["honor_code"] = "true" - request.POST["terms_of_service"] = "true" - # Handle duplicate username/email conflicts = account_api.check_account_exists( username=request.POST.get('username'), @@ -343,6 +341,32 @@ class RegistrationView(APIView): required=required ) + def _add_honor_code_field(self, form_desc, required=True): + # Translators: This is a legal document users must agree to in order to register a new account. + terms_text = _(u"Terms of Service and Honor Code") + + # Translators: "Terms of service" is a legal document users must agree to in order to register a new account. + label = _( + u"I agree to the {terms_of_service}" + ).format( + terms_of_service=u"{terms_text}".format( + url=marketing_link("HONOR"), + terms_text=terms_text + ) + ) + + # TODO: Open source installations may need + # separate checkboxes for terms or service or privacy + # policies. Instead of hard-coding this, we should create a more flexible + # mechanism for configuring which fields appear on the registration form. + form_desc.add_field( + "honor_code", + label=label, + field_type="checkbox", + default=False, + required=required, + ) + def _options_with_default(self, options): """Include a default option as the first option. """ return ( From d774bcf36e1bbdecc14e92c2b2447608f56d1c9e Mon Sep 17 00:00:00 2001 From: Will Daly Date: Mon, 20 Oct 2014 10:42:23 -0400 Subject: [PATCH 13/97] Support separate terms of service and honor code checkboxes. --- .../djangoapps/user_api/tests/test_views.py | 72 ++++++++++++++++++ common/djangoapps/user_api/views.py | 75 ++++++++++++++----- lms/envs/common.py | 1 + 3 files changed, 128 insertions(+), 20 deletions(-) diff --git a/common/djangoapps/user_api/tests/test_views.py b/common/djangoapps/user_api/tests/test_views.py index 1edc3e8a65..350a5e4e44 100644 --- a/common/djangoapps/user_api/tests/test_views.py +++ b/common/djangoapps/user_api/tests/test_views.py @@ -983,6 +983,78 @@ class RegistrationViewTest(ApiTestCase): } ) + @override_settings(MKTG_URLS={ + "ROOT": "https://www.test.com/", + "HONOR": "honor", + "TOS": "tos", + }) + @patch.dict(settings.FEATURES, {"ENABLE_MKTG_SITE": True}) + def test_registration_separate_terms_of_service_mktg_site_enabled(self): + # Honor code field should say ONLY honor code, + # not "terms of service and honor code" + self._assert_reg_field( + {"honor_code": "required", "terms_of_service": "required"}, + { + "label": "I agree to the Honor Code", + "name": "honor_code", + "default": False, + "type": "checkbox", + "required": True, + "placeholder": "", + "instructions": "", + "restrictions": {}, + } + ) + + # Terms of service field should also be present + self._assert_reg_field( + {"honor_code": "required", "terms_of_service": "required"}, + { + "label": "I agree to the Terms of Service", + "name": "terms_of_service", + "default": False, + "type": "checkbox", + "required": True, + "placeholder": "", + "instructions": "", + "restrictions": {}, + } + ) + + @override_settings(MKTG_URLS_LINK_MAP={"HONOR": "honor", "TOS": "tos"}) + @patch.dict(settings.FEATURES, {"ENABLE_MKTG_SITE": False}) + def test_registration_separate_terms_of_service_mktg_site_disabled(self): + # Honor code field should say ONLY honor code, + # not "terms of service and honor code" + self._assert_reg_field( + {"honor_code": "required", "terms_of_service": "required"}, + { + "label": "I agree to the Honor Code", + "name": "honor_code", + "default": False, + "type": "checkbox", + "required": True, + "placeholder": "", + "instructions": "", + "restrictions": {}, + } + ) + + # Terms of service field should also be present + self._assert_reg_field( + {"honor_code": "required", "terms_of_service": "required"}, + { + "label": "I agree to the Terms of Service", + "name": "terms_of_service", + "default": False, + "type": "checkbox", + "required": True, + "placeholder": "", + "instructions": "", + "restrictions": {}, + } + ) + @override_settings(REGISTRATION_EXTRA_FIELDS={ "level_of_education": "optional", "gender": "optional", diff --git a/common/djangoapps/user_api/views.py b/common/djangoapps/user_api/views.py index 3059c08168..7d891fae82 100644 --- a/common/djangoapps/user_api/views.py +++ b/common/djangoapps/user_api/views.py @@ -136,12 +136,31 @@ class RegistrationView(APIView): EXTRA_FIELDS = [ "city", "country", "level_of_education", "gender", "year_of_birth", "mailing_address", "goals", - "honor_code", + "honor_code", "terms_of_service", ] + def _is_field_visible(self, field_name): + """Check whether a field is visible based on Django settings. """ + return self._extra_fields_setting.get(field_name) in ["required", "optional"] + + def _is_field_required(self, field_name): + """Check whether a field is required based on Django settings. """ + return self._extra_fields_setting.get(field_name) == "required" + def __init__(self, *args, **kwargs): super(RegistrationView, self).__init__(*args, **kwargs) + # Backwards compatibility: Honor code is required by default, unless + # explicitly set to "optional" in Django settings. + self._extra_fields_setting = copy.deepcopy(settings.REGISTRATION_EXTRA_FIELDS) + self._extra_fields_setting["honor_code"] = self._extra_fields_setting.get("honor_code", "required") + + # Check that the setting is configured correctly + for field_name in self.EXTRA_FIELDS: + if self._extra_fields_setting.get(field_name, "hidden") not in ["required", "optional", "hidden"]: + msg = u"Setting REGISTRATION_EXTRA_FIELDS values must be either required, optional, or hidden." + raise ImproperlyConfigured(msg) + # 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): @@ -175,22 +194,14 @@ class RegistrationView(APIView): for field_name in self.DEFAULT_FIELDS: self.field_handlers[field_name](form_desc, required=True) - # Backwards compatibility: Honor code is required by default, unless - # explicitly set to "optional" in Django settings. - extra_fields_setting = copy.deepcopy(settings.REGISTRATION_EXTRA_FIELDS) - extra_fields_setting["honor_code"] = extra_fields_setting.get("honor_code", "required") - # Extra fields configured in Django settings # may be required, optional, or hidden for field_name in self.EXTRA_FIELDS: - field_setting = extra_fields_setting.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": - msg = u"Setting REGISTRATION_EXTRA_FIELDS values must be either required, optional, or hidden." - raise ImproperlyConfigured(msg) + if self._is_field_visible(field_name): + self.field_handlers[field_name]( + form_desc, + required=self._is_field_required(field_name) + ) return HttpResponse(form_desc.to_json(), content_type="application/json") @@ -342,8 +353,14 @@ class RegistrationView(APIView): ) def _add_honor_code_field(self, form_desc, required=True): - # Translators: This is a legal document users must agree to in order to register a new account. - terms_text = _(u"Terms of Service and Honor Code") + # Separate terms of service and honor code checkboxes + if self._is_field_visible("terms_of_service"): + terms_text = _(u"Honor Code") + + # Combine terms of service and honor code checkboxes + else: + # Translators: This is a legal document users must agree to in order to register a new account. + terms_text = _(u"Terms of Service and Honor Code") # Translators: "Terms of service" is a legal document users must agree to in order to register a new account. label = _( @@ -355,10 +372,6 @@ class RegistrationView(APIView): ) ) - # TODO: Open source installations may need - # separate checkboxes for terms or service or privacy - # policies. Instead of hard-coding this, we should create a more flexible - # mechanism for configuring which fields appear on the registration form. form_desc.add_field( "honor_code", label=label, @@ -367,6 +380,28 @@ class RegistrationView(APIView): required=required, ) + def _add_terms_of_service_field(self, form_desc, required=True): + # Translators: This is a legal document users must agree to in order to register a new account. + terms_text = _(u"Terms of Service") + + # Translators: "Terms of service" is a legal document users must agree to in order to register a new account. + label = _( + u"I agree to the {terms_of_service}" + ).format( + terms_of_service=u"{terms_text}".format( + url=marketing_link("TOS"), + terms_text=terms_text + ) + ) + + form_desc.add_field( + "terms_of_service", + label=label, + field_type="checkbox", + default=False, + required=required, + ) + def _options_with_default(self, options): """Include a default option as the first option. """ return ( diff --git a/lms/envs/common.py b/lms/envs/common.py index c57ff34565..8ccc48ad0b 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1536,6 +1536,7 @@ REGISTRATION_EXTRA_FIELDS = { 'mailing_address': 'optional', 'goals': 'optional', 'honor_code': 'required', + 'terms_of_service': 'hidden', 'city': 'hidden', 'country': 'hidden', } From bd0c5f227c7a9c1d2c2c98d70a7e0c87a702bb31 Mon Sep 17 00:00:00 2001 From: AlasdairSwan Date: Fri, 17 Oct 2014 14:09:14 -0400 Subject: [PATCH 14/97] ECOM-369 Added initial Backbone code which allows a user to login to lms --- lms/envs/common.py | 7 +- lms/static/js/student_account/accessApp.js | 13 ++ .../js/student_account/models/LoginModel.js | 52 ++++++ .../js/student_account/views/AccessView.js | 84 +++++++++ .../js/student_account/views/LoginView.js | 160 ++++++++++++++++++ .../student_account/access.underscore | 15 ++ .../student_account/form_field.underscore | 27 +++ .../student_account/login.underscore | 6 + .../student_account/login_and_register.html | 4 +- 9 files changed, 365 insertions(+), 3 deletions(-) create mode 100644 lms/static/js/student_account/accessApp.js create mode 100644 lms/static/js/student_account/models/LoginModel.js create mode 100644 lms/static/js/student_account/views/AccessView.js create mode 100644 lms/static/js/student_account/views/LoginView.js create mode 100644 lms/templates/student_account/access.underscore create mode 100644 lms/templates/student_account/form_field.underscore create mode 100644 lms/templates/student_account/login.underscore diff --git a/lms/envs/common.py b/lms/envs/common.py index c57ff34565..595edf30e4 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1025,7 +1025,12 @@ instructor_dash_js = sorted(rooted_glob(PROJECT_ROOT / 'static', 'coffee/src/ins # JavaScript used by the student account and profile pages # These are not courseware, so they do not need many of the courseware-specific # JavaScript modules. -student_account_js = sorted(rooted_glob(PROJECT_ROOT / 'static', 'js/student_account/**/*.js')) +student_account_js = [ + 'js/student_account/models/LoginModel.js', + 'js/student_account/views/LoginView.js', + 'js/student_account/views/AccessView.js', + 'js/student_account/accessApp.js', +] student_profile_js = sorted(rooted_glob(PROJECT_ROOT / 'static', 'js/student_profile/**/*.js')) PIPELINE_CSS = { diff --git a/lms/static/js/student_account/accessApp.js b/lms/static/js/student_account/accessApp.js new file mode 100644 index 0000000000..b3e3a893b9 --- /dev/null +++ b/lms/static/js/student_account/accessApp.js @@ -0,0 +1,13 @@ +var edx = edx || {}; + +(function($) { + 'use strict'; + + edx.student = edx.student || {}; + edx.student.account = edx.student.account || {}; + + return new edx.student.account.AccessView({ + mode: $('#login-and-registration-container').data('initial-mode') || 'login', + thirdPartyAuth: $('#login-and-registration-container').data('third-party-auth-providers') || false + }); +})(jQuery); \ No newline at end of file diff --git a/lms/static/js/student_account/models/LoginModel.js b/lms/static/js/student_account/models/LoginModel.js new file mode 100644 index 0000000000..7930d281ca --- /dev/null +++ b/lms/static/js/student_account/models/LoginModel.js @@ -0,0 +1,52 @@ +var edx = edx || {}; + +(function($, _, Backbone, gettext) { + 'use strict'; + + edx.student = edx.student || {}; + edx.student.account = edx.student.account || {}; + + edx.student.account.LoginModel = Backbone.Model.extend({ + + defaults: { + email: '', + password: '', + remember: false + }, + + urlRoot: '', + + initialize: function( obj ) { + this.urlRoot = obj.url; + }, + + sync: function(method, model) { + var headers = { + 'X-CSRFToken': $.cookie('csrftoken') + }; + + $.ajax({ + url: model.urlRoot, + type: 'POST', + data: model.attributes, + headers: headers + }) + .done(function() { + var query = window.location.search, + url = '/dashboard'; + + model.trigger('sync'); + + // If query string in url go back to that page + if ( query.length > 1 ) { + url = query.substring( query.indexOf('=') + 1 ); + } + + window.location.href = url; + }) + .fail( function( error ) { + model.trigger('error', error); + }); + } + }); +})(jQuery, _, Backbone, gettext); \ No newline at end of file diff --git a/lms/static/js/student_account/views/AccessView.js b/lms/static/js/student_account/views/AccessView.js new file mode 100644 index 0000000000..432552fbba --- /dev/null +++ b/lms/static/js/student_account/views/AccessView.js @@ -0,0 +1,84 @@ +var edx = edx || {}; + +(function($, _, Backbone, gettext) { + 'use strict'; + + edx.student = edx.student || {}; + edx.student.account = edx.student.account || {}; + + edx.student.account.AccessView = Backbone.View.extend({ + el: '#login-and-registration-container', + + tpl: $('#access-tpl').html(), + + events: { + 'change .form-toggle': 'toggleForm' + }, + + // The form currently loaded + activeForm: '', + + initialize: function( obj ) { + this.activeForm = obj.mode; + console.log(obj); + + this.render(); + }, + + render: function() { + $(this.el).html( _.template( this.tpl, { + mode: this.activeForm + })); + + this.postRender(); + + return this; + }, + + postRender: function() { + // Load the default form + this.loadForm( this.activeForm ); + }, + + loadForm: function( type ) { + if ( type === 'login' ) { + console.log('load login'); + return new edx.student.account.LoginView(); + } + + // return new app.LoginView({ + // el: $('#' + type + '-form'), + // model: this.getModel( type ), + // tpl: $('#' + type + '-form-tpl').html() + // }); + }, + + toggleForm: function( e ) { + var type = $(e.currentTarget).val(), + $form = $('#' + type + '-form'); + + if ( !this.form.isLoaded( $form ) ) { + this.loadForm( type ); + } + + $(this.el).find('.form-wrapper').addClass('hidden'); + $form.removeClass('hidden'); + }, + + getModel: function( type ) { + var models = { + join: app.JoinModel, + login: app.JoinModel + }; + + return models[type] ? new models[type]() : false; + }, + + form: { + isLoaded: function( $form ) { + return $form.html().length > 0; + } + } + }); + +})(jQuery, _, Backbone, gettext); \ No newline at end of file diff --git a/lms/static/js/student_account/views/LoginView.js b/lms/static/js/student_account/views/LoginView.js new file mode 100644 index 0000000000..10fd470566 --- /dev/null +++ b/lms/static/js/student_account/views/LoginView.js @@ -0,0 +1,160 @@ +var edx = edx || {}; + +(function($, _, Backbone, gettext) { + 'use strict'; + + edx.student = edx.student || {}; + edx.student.account = edx.student.account || {}; + + edx.student.account.LoginView = Backbone.View.extend({ + tagName: 'form', + + el: '#login-form', + + tpl: $('#login-tpl').html(), + + fieldTpl: $('#form_field-tpl').html(), + + events: { + 'click .js-login': 'submitForm', + 'click .forgot-password': 'forgotPassword' + }, + + errors: [], + + $form: {}, + + initialize: function( obj ) { + this.getInitialData(); + }, + + // Renders the form. + render: function( html ) { + var fields = html || ''; + + $(this.el).html( _.template( this.tpl, { + fields: fields + })); + + this.postRender(); + + return this; + }, + + postRender: function() { + + this.$form = $(this.el).find('form'); + }, + + getInitialData: function() { + var that = this; + + $.ajax({ + type: 'GET', + dataType: 'json', + url: '/user_api/v1/account/login_session/', + success: function( data ) { + console.log(data); + that.buildForm( data.fields ); + that.initModel( data.submit_url, data.method ); + }, + error: function( jqXHR, textStatus, errorThrown ) { + console.log('fail ', errorThrown); + } + }); + }, + + initModel: function( url, method ) { + this.model = new edx.student.account.LoginModel({ + url: url + }); + + this.listenTo( this.model, 'error', function( error ) { + console.log(error.status, ' error: ', error.responseText); + }); + }, + + buildForm: function( data ) { + var html = [], + i, + len = data.length, + fieldTpl = this.fieldTpl; + + for ( i=0; i +

+ checked<% } %> > + +

+
+ + +
+

+ checked<% } %>> + +

+
+
diff --git a/lms/templates/student_account/form_field.underscore b/lms/templates/student_account/form_field.underscore new file mode 100644 index 0000000000..b5787baf5a --- /dev/null +++ b/lms/templates/student_account/form_field.underscore @@ -0,0 +1,27 @@ +

+ <% if ( type !== 'checkbox' ) { %> + <% } %> + + <% } %> + + <% if( form === 'login' && name === 'password' ) { %> + Forgot password? + <% } %> + + minlength="<%= restrictions.min_length %>"<% } %> + <% if ( restrictions.max_length ) { %> maxlength="<%= restrictions.max_length %>"<% } %> + <% if ( required ) { %> required<% } %> + /> + + <% if ( type === 'checkbox' ) { %> + <% } %> + + <% } %> + + <%= instructions %> +

\ No newline at end of file diff --git a/lms/templates/student_account/login.underscore b/lms/templates/student_account/login.underscore new file mode 100644 index 0000000000..c1f85c3088 --- /dev/null +++ b/lms/templates/student_account/login.underscore @@ -0,0 +1,6 @@ +
+ <%= fields %> + + + +
\ No newline at end of file diff --git a/lms/templates/student_account/login_and_register.html b/lms/templates/student_account/login_and_register.html index 19ed498fd8..ee2b49c503 100644 --- a/lms/templates/student_account/login_and_register.html +++ b/lms/templates/student_account/login_and_register.html @@ -12,7 +12,7 @@ <%block name="header_extras"> -% for template_name in ["account"]: +% for template_name in ["account", "access", "form_field", "login"]: @@ -21,7 +21,7 @@

Login and Registration!

-

This is a placeholder for the combined login and registration form

+

This is a placeholder for the combined login and registration form.

## TODO: Use JavaScript to populate this div with ## the actual registration/login forms (loaded asynchronously from the user API) From b70dfb07809ae898b8174b82f82bd9dd3d8efd1f Mon Sep 17 00:00:00 2001 From: AlasdairSwan Date: Fri, 17 Oct 2014 15:57:05 -0400 Subject: [PATCH 15/97] ECOM-369 added registration view and front end validation --- .../js/spec_helpers/edx.utils.validate.js | 71 ++++++++ lms/envs/common.py | 3 + .../student_account/models/RegisterModel.js | 60 +++++++ .../js/student_account/views/AccessView.js | 3 + .../js/student_account/views/LoginView.js | 30 ++-- .../js/student_account/views/RegisterView.js | 157 ++++++++++++++++++ .../student_account/access.underscore | 2 +- .../student_account/form_field.underscore | 24 ++- .../student_account/login.underscore | 6 + .../student_account/login_and_register.html | 6 +- .../student_account/register.underscore | 15 ++ 11 files changed, 354 insertions(+), 23 deletions(-) create mode 100644 common/static/js/spec_helpers/edx.utils.validate.js create mode 100644 lms/static/js/student_account/models/RegisterModel.js create mode 100644 lms/static/js/student_account/views/RegisterView.js create mode 100644 lms/templates/student_account/register.underscore diff --git a/common/static/js/spec_helpers/edx.utils.validate.js b/common/static/js/spec_helpers/edx.utils.validate.js new file mode 100644 index 0000000000..293d881ee7 --- /dev/null +++ b/common/static/js/spec_helpers/edx.utils.validate.js @@ -0,0 +1,71 @@ +var edx = edx || {}; + +(function( $, _ ) { + 'use strict'; + + edx.utils = edx.utils || {}; + + var utils = (function(){ + var _fn = { + validate: { + + field: function( el ) { + var $el = $(el); + + return _fn.validate.required( $el ) && + _fn.validate.charLength( $el ) && + _fn.validate.email.valid( $el ); + }, + + charLength: function( $el ) { + // Cannot assume there will be both min and max + var min = $el.attr('minlength') || 0, + max = $el.attr('maxlength') || false, + chars = $el.val().length, + within = false; + + // if max && min && within the range + if ( min <= chars && ( max && chars <= max ) ) { + within = true; + } else if ( min <= chars && !max ) { + within = true; + } + + return within; + }, + + required: function( $el ) { + return $el.attr('required') ? $el.val() : true; + }, + + email: { + // This is the same regex used to validate email addresses in Django 1.4 + regex: new RegExp( + [ + '(^[-!#$%&\'*+/=?^_`{}|~0-9A-Z]+(\\.[-!#$%&\'*+/=?^_`{}|~0-9A-Z]+)*', + '|^"([\\001-\\010\\013\\014\\016-\\037!#-\\[\\]-\\177]|\\\\[\\001-\\011\\013\\014\\016-\\177])*"', + ')@((?:[A-Z0-9](?:[A-Z0-9-]{0,61}[A-Z0-9])?\\.)+[A-Z]{2,6}\\.?$)', + '|\\[(25[0-5]|2[0-4]\\d|[0-1]?\\d?\\d)(\\.(25[0-5]|2[0-4]\\d|[0-1]?\\d?\\d)){3}\\]$' + ].join(''), 'i' + ), + + valid: function( $el ) { + return $el.data('email') ? _fn.validate.email.format( $el.val() ) : true; + }, + + format: function( str ) { + return _fn.validate.email.regex.test( str ); + } + } + } + }; + + return { + validate: _fn.validate.field + }; + + })(); + + edx.utils.validate = utils.validate + +})( jQuery, _ ); \ No newline at end of file diff --git a/lms/envs/common.py b/lms/envs/common.py index 595edf30e4..dc1ec56026 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1026,8 +1026,11 @@ instructor_dash_js = sorted(rooted_glob(PROJECT_ROOT / 'static', 'coffee/src/ins # These are not courseware, so they do not need many of the courseware-specific # JavaScript modules. student_account_js = [ + 'js/common_helpers/edx.utils.validate.js', 'js/student_account/models/LoginModel.js', + 'js/student_account/models/RegisterModel.js', 'js/student_account/views/LoginView.js', + 'js/student_account/views/RegisterView.js', 'js/student_account/views/AccessView.js', 'js/student_account/accessApp.js', ] diff --git a/lms/static/js/student_account/models/RegisterModel.js b/lms/static/js/student_account/models/RegisterModel.js new file mode 100644 index 0000000000..08ca2ec6d8 --- /dev/null +++ b/lms/static/js/student_account/models/RegisterModel.js @@ -0,0 +1,60 @@ +var edx = edx || {}; + +(function($, _, Backbone, gettext) { + 'use strict'; + + edx.student = edx.student || {}; + edx.student.account = edx.student.account || {}; + + edx.student.account.RegisterModel = Backbone.Model.extend({ + + defaults: { + email: '', + name: '', + username: '', + password: '', + level_of_education: '', + gender: '', + year_of_birth: '', + mailing_address: '', + goals: '', + termsofservice: false + }, + + urlRoot: '', + + initialize: function( obj ) { + this.urlRoot = obj.url; + }, + + sync: function(method, model) { + var headers = { + 'X-CSRFToken': $.cookie('csrftoken') + }; + + $.ajax({ + url: model.urlRoot, + type: 'POST', + data: model.attributes, + headers: headers + }) + .done(function() { + var query = window.location.search, + url = '/dashboard'; + + // model.trigger('sync'); + + // If query string in url go back to that page + if ( query.length > 1 ) { + url = query.substring( query.indexOf('=') + 1 ); + } + + window.location.href = url; + }) + .fail( function( error ) { + console.log('RegisterModel.save() FAILURE!!!!!'); + model.trigger('error', error); + }); + } + }); +})(jQuery, _, Backbone, gettext); \ No newline at end of file diff --git a/lms/static/js/student_account/views/AccessView.js b/lms/static/js/student_account/views/AccessView.js index 432552fbba..40486b3fc4 100644 --- a/lms/static/js/student_account/views/AccessView.js +++ b/lms/static/js/student_account/views/AccessView.js @@ -44,6 +44,9 @@ var edx = edx || {}; if ( type === 'login' ) { console.log('load login'); return new edx.student.account.LoginView(); + } else if ( type === 'register' ) { + console.log('load register'); + return new edx.student.account.RegisterView(); } // return new app.LoginView({ diff --git a/lms/static/js/student_account/views/LoginView.js b/lms/static/js/student_account/views/LoginView.js index 10fd470566..1e26bf8727 100644 --- a/lms/static/js/student_account/views/LoginView.js +++ b/lms/static/js/student_account/views/LoginView.js @@ -42,8 +42,10 @@ var edx = edx || {}; }, postRender: function() { + var $container = $(this.el); - this.$form = $(this.el).find('form'); + this.$form = $container.find('form'); + this.$errors = $container.find('.error-msg'); }, getInitialData: function() { @@ -81,10 +83,6 @@ var edx = edx || {}; fieldTpl = this.fieldTpl; for ( i=0; ichecked<% } %> > -
+
diff --git a/lms/templates/student_account/form_field.underscore b/lms/templates/student_account/form_field.underscore index b5787baf5a..13dd53d787 100644 --- a/lms/templates/student_account/form_field.underscore +++ b/lms/templates/student_account/form_field.underscore @@ -10,11 +10,25 @@ Forgot password? <% } %> - minlength="<%= restrictions.min_length %>"<% } %> - <% if ( restrictions.max_length ) { %> maxlength="<%= restrictions.max_length %>"<% } %> - <% if ( required ) { %> required<% } %> - /> + <% if ( type === 'select' ) { %> + + <% } else if ( type === 'textarea' ) { %> + + <% } else { %> + minlength="<%= restrictions.min_length %>"<% } %> + <% if ( restrictions.max_length ) { %> maxlength="<%= restrictions.max_length %>"<% } %> + <% if ( required ) { %> required<% } %> + /> + <% } %> <% if ( type === 'checkbox' ) { %>
\ No newline at end of file diff --git a/lms/templates/student_account/register.underscore b/lms/templates/student_account/register.underscore index dc620f790f..874d72530d 100644 --- a/lms/templates/student_account/register.underscore +++ b/lms/templates/student_account/register.underscore @@ -1,4 +1,4 @@ -
+
- - - <%= fields %> - - \ No newline at end of file From 93246db6567d77eba9dfffcf934ba87feaf22105 Mon Sep 17 00:00:00 2001 From: AlasdairSwan Date: Tue, 21 Oct 2014 09:11:59 -0400 Subject: [PATCH 21/97] ECOM-408 Added grid-settings to override the default global box-sizing style --- lms/static/sass/application-extend1.scss.mako | 1 + lms/static/sass/application-extend2.scss.mako | 1 + lms/static/sass/application.scss.mako | 1 + lms/static/sass/base/_grid-settings.scss | 2 + lms/static/sass/views/_login-register.scss | 156 +++++++++++++++++- 5 files changed, 158 insertions(+), 3 deletions(-) create mode 100644 lms/static/sass/base/_grid-settings.scss diff --git a/lms/static/sass/application-extend1.scss.mako b/lms/static/sass/application-extend1.scss.mako index 8aa2db8e47..91e56c904e 100644 --- a/lms/static/sass/application-extend1.scss.mako +++ b/lms/static/sass/application-extend1.scss.mako @@ -5,6 +5,7 @@ // libs and resets *do not edit* @import 'bourbon/bourbon'; // lib - bourbon +@import 'base/grid-settings'; @import "neat/neat"; // lib - Neat @import 'vendor/bi-app/bi-app-ltr'; // set the layout for left to right languages diff --git a/lms/static/sass/application-extend2.scss.mako b/lms/static/sass/application-extend2.scss.mako index 57aee6c7a5..2acf42d930 100644 --- a/lms/static/sass/application-extend2.scss.mako +++ b/lms/static/sass/application-extend2.scss.mako @@ -5,6 +5,7 @@ // libs and resets *do not edit* @import 'bourbon/bourbon'; // lib - bourbon +@import 'base/grid-settings'; @import "neat/neat"; // lib - Neat @import 'vendor/bi-app/bi-app-ltr'; // set the layout for left to right languages diff --git a/lms/static/sass/application.scss.mako b/lms/static/sass/application.scss.mako index 69138290e0..23adcf4cff 100644 --- a/lms/static/sass/application.scss.mako +++ b/lms/static/sass/application.scss.mako @@ -5,6 +5,7 @@ // libs and resets *do not edit* @import 'bourbon/bourbon'; // lib - bourbon +@import 'base/grid-settings'; @import "neat/neat"; // lib - Neat @import 'vendor/bi-app/bi-app-ltr'; // set the layout for left to right languages diff --git a/lms/static/sass/base/_grid-settings.scss b/lms/static/sass/base/_grid-settings.scss new file mode 100644 index 0000000000..54bb349d8c --- /dev/null +++ b/lms/static/sass/base/_grid-settings.scss @@ -0,0 +1,2 @@ +/* Override the default global box-sizing */ +$border-box-sizing: false; \ No newline at end of file diff --git a/lms/static/sass/views/_login-register.scss b/lms/static/sass/views/_login-register.scss index 6065ad8e37..d077947115 100644 --- a/lms/static/sass/views/_login-register.scss +++ b/lms/static/sass/views/_login-register.scss @@ -1,10 +1,16 @@ // lms - views - login/register view // ==================== -.login-register{ +.login-register { + @include box-sizing(border-box); @include span-columns(6); @include shift(3); + /* Temp. fix until applied globally */ + & > { + @include box-sizing(border-box); + } + input:-webkit-autofill { -webkit-box-shadow:0 0 0 50px white inset; -webkit-text-fill-color: #333; @@ -17,15 +23,159 @@ .form-field { width: 100%; + margin: 0 0 $baseline 0; + + /** FROM _accounts.scss - start **/ + label, input, textarea { + border-radius: 0; + display: block; + height: auto; + font-family: $sans-serif; + font-style: normal; + font-weight: 500; + color: $base-font-color; + } + + label { + @include transition(color 0.15s ease-in-out 0s); + margin: 0 0 ($baseline/4) 0; + color: tint($black, 20%); + } + + input, + textarea { + width: 100%; + margin: 0; + padding: ($baseline/2) ($baseline*.75); + + &.long { + width: 100%; + } + + &.short { + width: 25%; + } + } + + textarea.long { + height: ($baseline*5); + } + + select { + width: 100%; + } + /** FROM _accounts.scss - end **/ + } + + .input-block { + width: 100%; } - .input-block, .desc { display: block; + /*@extend %body-text-emphasized;*/ + margin-bottom: $baseline; } .input-inline { display: inline; } -} \ No newline at end of file + + +} + + /*// individual fields + .field { + + + + &:last-child { + margin-bottom: 0; + } + + // types - password + + // types - select + + + // types - checkboxes/radio buttons + &.checkbox { + + input[type="checkbox"] { + display: inline-block; + width: auto; + @include margin-right($baseline/4); + } + + label { + display: inline-block; + } + } + + // states - all + &.disabled, + &.submitted { + color: rgba(0,0,0,.25); + + label { + cursor: text; + + &:after { + margin-left: ($baseline/4); + } + } + + textarea, input { + background: $container-bg; + color: rgba(0,0,0,.25); + } + } + + // states - focused + &.is-focused { + + label { + color: saturate($link-color-d1,15%); + } + + .tip { + color: saturate($link-color-d1,15%); + } + } + + // states - disabled + &.disabled { + label:after { + color: rgba(0,0,0,.35); + content: "(Disabled Currently)"; + } + } + + &.error { + + label { + color: $red; + } + + input, textarea { + border-color: tint($red,50%); + } + } + + &.required { + + label { + font-weight: 600; + + a { + font-weight: 600 !important; + } + } + + label:after { + margin-left: ($baseline/4); + content: "*"; + } + } + }*/ From a5da61b4abe78d68c7491c6cfce58ab7d3f5d73f Mon Sep 17 00:00:00 2001 From: AlasdairSwan Date: Tue, 21 Oct 2014 09:39:32 -0400 Subject: [PATCH 22/97] ECOM-408 Added breakpoint variables to _grid-settings.scss --- lms/static/sass/base/_grid-settings.scss | 9 ++++++++- lms/static/sass/views/_login-register.scss | 15 ++++++++++++--- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/lms/static/sass/base/_grid-settings.scss b/lms/static/sass/base/_grid-settings.scss index 54bb349d8c..6fdfaf9174 100644 --- a/lms/static/sass/base/_grid-settings.scss +++ b/lms/static/sass/base/_grid-settings.scss @@ -1,2 +1,9 @@ /* Override the default global box-sizing */ -$border-box-sizing: false; \ No newline at end of file +@import "neat/neat-helpers"; // or "neat-helpers" when in Rails + +$border-box-sizing: false; + +/* Breakpoints */ +$mobile: new-breakpoint(max-width 320px 4); +$tablet: new-breakpoint(min-width 321px max-width 768px, 8); +$desktop: new-breakpoint(min-width 769px 12); \ No newline at end of file diff --git a/lms/static/sass/views/_login-register.scss b/lms/static/sass/views/_login-register.scss index d077947115..2dd91b4871 100644 --- a/lms/static/sass/views/_login-register.scss +++ b/lms/static/sass/views/_login-register.scss @@ -3,8 +3,9 @@ .login-register { @include box-sizing(border-box); - @include span-columns(6); - @include shift(3); + @include span-columns(4); + width: 100%; + max-width: 1200px; /* Temp. fix until applied globally */ & > { @@ -81,10 +82,18 @@ display: inline; } + @include media( $tablet ) { + @include span-columns(6); + @include shift(1); + } - + @include media( $desktop ) { + @include span-columns(6); + @include shift(3); + } } + /*// individual fields .field { From 00ff10820a91773c6ad0b9c06f5afb797c880fdd Mon Sep 17 00:00:00 2001 From: AlasdairSwan Date: Tue, 21 Oct 2014 16:03:58 -0400 Subject: [PATCH 23/97] ECOM-531 Styling the forms --- lms/static/sass/base/_grid-settings.scss | 5 +- lms/static/sass/base/_variables.scss | 7 + lms/static/sass/shared/_footer.scss | 1 + lms/static/sass/views/_login-register.scss | 130 ++++++++++++++++-- .../student_account/access.underscore | 8 +- .../student_account/form_field.underscore | 6 +- .../student_account/login_and_register.html | 12 +- .../student_account/password_reset.underscore | 2 +- 8 files changed, 142 insertions(+), 29 deletions(-) diff --git a/lms/static/sass/base/_grid-settings.scss b/lms/static/sass/base/_grid-settings.scss index 6fdfaf9174..d3c8a75802 100644 --- a/lms/static/sass/base/_grid-settings.scss +++ b/lms/static/sass/base/_grid-settings.scss @@ -1,8 +1,11 @@ -/* Override the default global box-sizing */ @import "neat/neat-helpers"; // or "neat-helpers" when in Rails +/* Change the grid settings */ +$max-width: 1200px; +/* Override the default global box-sizing */ $border-box-sizing: false; + /* Breakpoints */ $mobile: new-breakpoint(max-width 320px 4); $tablet: new-breakpoint(min-width 321px max-width 768px, 8); diff --git a/lms/static/sass/base/_variables.scss b/lms/static/sass/base/_variables.scss index a54a4687c7..8b5649f825 100644 --- a/lms/static/sass/base/_variables.scss +++ b/lms/static/sass/base/_variables.scss @@ -22,6 +22,12 @@ $monospace: Monaco, 'Bitstream Vera Sans Mono', 'Lucida Console', monospace; $body-font-family: $sans-serif; $serif: $georgia; +// FONT-WEIGHTS +$font-light: 300; +$font-regular: 400; +$font-semibold: 600; +$font-bold: 700; + // ==================== // MISC: base fonts/colors @@ -172,6 +178,7 @@ $m-blue-d1: #1790C7; $m-blue-d2: #1580B0; $m-blue-d3: #126F9A; $m-blue-d4: #0A4A67; +$m-blue-d5: #009EE7; $m-blue-t0: rgba($m-blue,0.125); $m-blue-t1: rgba($m-blue,0.25); $m-blue-t2: rgba($m-blue,0.50); diff --git a/lms/static/sass/shared/_footer.scss b/lms/static/sass/shared/_footer.scss index 1420370be9..3255716e78 100644 --- a/lms/static/sass/shared/_footer.scss +++ b/lms/static/sass/shared/_footer.scss @@ -5,6 +5,7 @@ border-top: 1px solid tint($m-gray,50%); padding: 25px ($baseline/2) ($baseline*1.5) ($baseline/2); background: $footer-bg; + clear: both; footer { @include clearfix(); diff --git a/lms/static/sass/views/_login-register.scss b/lms/static/sass/views/_login-register.scss index 2dd91b4871..bdb445bc3f 100644 --- a/lms/static/sass/views/_login-register.scss +++ b/lms/static/sass/views/_login-register.scss @@ -1,17 +1,29 @@ // lms - views - login/register view // ==================== +.section-bkg-wrapper { + background: $m-gray-l4; +} .login-register { @include box-sizing(border-box); - @include span-columns(4); + @include outer-container; + background: white; + width: 100%; - max-width: 1200px; + + h2 { + line-height: 16px; + margin: 0; + font-family: $sans-serif; + } /* Temp. fix until applied globally */ - & > { + > { @include box-sizing(border-box); } + + /* Remove autocomplete yellow background */ input:-webkit-autofill { -webkit-box-shadow:0 0 0 50px white inset; -webkit-text-fill-color: #333; @@ -22,14 +34,73 @@ -webkit-text-fill-color: #333; } - .form-field { + header { + @include outer-container; + border-bottom: 1px solid $gray-l4; width: 100%; + padding-top: 35px; + padding-bottom: 35px; + + .headline { + @include box-sizing(border-box); + @include span-columns(4); + @include font-size(35); + font-family: $sans-serif; + font-weight: $font-semibold; + text-align: left; + margin-bottom: 0; + color: $m-blue-d5; + } + + .tagline { + @include box-sizing(border-box); + @include span-columns(4); + @include font-size(24); + font-family: $sans-serif; + font-weight: $font-regular; + } + } + + .form-toggle { + margin: 0 10px; + } + + .form-type { + padding-top: 25px; + padding-bottom: 25px; + @include box-sizing(border-box); + @include span-columns(4); + + &:first-of-type { + border-bottom: 1px solid $gray-l4; + } + } + + + + /** The forms **/ + .form-wrapper { + padding-top: 25px; + } + + .form-label { + @include font-size(16); + font-family: $sans-serif; + font-weight: $font-semibold; + font-style: normal; + text-transform: none; + } + + .form-field { + width: calc( 100% - 20px ); margin: 0 0 $baseline 0; + padding-left: 10px; /** FROM _accounts.scss - start **/ - label, input, textarea { + label, + input, + textarea { border-radius: 0; - display: block; height: auto; font-family: $sans-serif; font-style: normal; @@ -41,10 +112,22 @@ @include transition(color 0.15s ease-in-out 0s); margin: 0 0 ($baseline/4) 0; color: tint($black, 20%); + font-weight: $font-semibold; + + &.inline { + display: inline; + } + + } + + .field-link { + position: relative; + float: right } input, textarea { + display: block; width: 100%; margin: 0; padding: ($baseline/2) ($baseline*.75); @@ -56,6 +139,12 @@ &.short { width: 25%; } + + &.checkbox { + display: inline; + width: auto; + margin-right: 5px; + } } textarea.long { @@ -72,24 +161,35 @@ width: 100%; } - .desc { - display: block; - /*@extend %body-text-emphasized;*/ - margin-bottom: $baseline; - } .input-inline { display: inline; } + .desc { + @include transition(color 0.15s ease-in-out 0s); + display: block; + margin-top: ($baseline/4); + color: $lighter-base-font-color; + font-size: em(13); + } + @include media( $tablet ) { - @include span-columns(6); - @include shift(1); + header h1, + header p, + .form-type { + @include span-columns(6); + @include shift(1); + } } @include media( $desktop ) { - @include span-columns(6); - @include shift(3); + header h1, + header p, + .form-type { + @include span-columns(6); + @include shift(3); + } } } diff --git a/lms/templates/student_account/access.underscore b/lms/templates/student_account/access.underscore index e32f587c6b..981a1dc5fa 100644 --- a/lms/templates/student_account/access.underscore +++ b/lms/templates/student_account/access.underscore @@ -1,12 +1,12 @@

checked<% } %> > - +

@@ -14,7 +14,7 @@

checked<% } %>> - +

diff --git a/lms/templates/student_account/form_field.underscore b/lms/templates/student_account/form_field.underscore index 3d6c2b0851..8cb463ef55 100644 --- a/lms/templates/student_account/form_field.underscore +++ b/lms/templates/student_account/form_field.underscore @@ -7,7 +7,7 @@ <% } %> <% if( form === 'login' && name === 'password' ) { %> - Forgot password? + Forgot password? <% } %> <% if ( type === 'select' ) { %> @@ -23,7 +23,7 @@ <% if ( required ) { %> required<% } %> > <% } else { %> - minlength="<%= restrictions.min_length %>"<% } %> <% if ( restrictions.max_length ) { %> maxlength="<%= restrictions.max_length %>"<% } %> <% if ( required ) { %> required<% } %> @@ -31,7 +31,7 @@ <% } %> <% if ( type === 'checkbox' ) { %> - diff --git a/lms/templates/student_account/login_and_register.html b/lms/templates/student_account/login_and_register.html index 629d0d1297..fb8dcd0e8e 100644 --- a/lms/templates/student_account/login_and_register.html +++ b/lms/templates/student_account/login_and_register.html @@ -43,8 +43,10 @@ ## ## Note that this list may be empty. ## - diff --git a/lms/templates/register.html b/lms/templates/register.html index 8cf3ba8f1f..9942f807c1 100644 --- a/lms/templates/register.html +++ b/lms/templates/register.html @@ -125,7 +125,7 @@ % for enabled in provider.Registry.enabled(): ## Translators: provider_name is the name of an external, third-party user authentication service (like Google or LinkedIn). - + % endfor @@ -362,7 +362,7 @@ % endif
- +
diff --git a/lms/templates/student_account/login.underscore b/lms/templates/student_account/login.underscore index b01e4432d0..6cb07529ea 100644 --- a/lms/templates/student_account/login.underscore +++ b/lms/templates/student_account/login.underscore @@ -14,12 +14,12 @@ <%= context.fields %> - + <% _.each( context.providers, function( provider ) { if ( provider.loginUrl ) { %> - diff --git a/lms/templates/student_account/register.underscore b/lms/templates/student_account/register.underscore index d588940666..01ffe07f47 100644 --- a/lms/templates/student_account/register.underscore +++ b/lms/templates/student_account/register.underscore @@ -8,7 +8,7 @@ <% } else { _.each( context.providers, function( provider) { if ( provider.registerUrl ) { %> - @@ -24,5 +24,5 @@ <%= context.fields %> - + From 6defcaa0afcdb85d6276c4331930cec93b20dde5 Mon Sep 17 00:00:00 2001 From: Renzo Lucioni Date: Mon, 10 Nov 2014 15:51:26 -0500 Subject: [PATCH 81/97] Final text review --- .../djangoapps/user_api/tests/test_views.py | 10 ++++-- common/djangoapps/user_api/views.py | 34 ++++++++++++++----- .../static/js/spec/edx.utils.validate_spec.js | 7 ++-- common/static/js/utils/edx.utils.validate.js | 25 +++++++------- .../student_account/access.underscore | 2 +- .../student_account/register.underscore | 2 +- 6 files changed, 50 insertions(+), 30 deletions(-) diff --git a/common/djangoapps/user_api/tests/test_views.py b/common/djangoapps/user_api/tests/test_views.py index d6319c549b..49d3741875 100644 --- a/common/djangoapps/user_api/tests/test_views.py +++ b/common/djangoapps/user_api/tests/test_views.py @@ -1345,7 +1345,9 @@ class RegistrationViewTest(ApiTestCase): self.assertEqual(response.status_code, 409) self.assertEqual( response.content, - "It looks like {} belongs to an existing account.".format(self.EMAIL) + "It looks like {} belongs to an existing account. Try again with a different email address.".format( + self.EMAIL + ) ) def test_register_duplicate_username(self): @@ -1370,7 +1372,9 @@ class RegistrationViewTest(ApiTestCase): self.assertEqual(response.status_code, 409) self.assertEqual( response.content, - "It looks like {} belongs to an existing account.".format(self.USERNAME) + "It looks like {} belongs to an existing account. Try again with a different email address and username.".format( + self.USERNAME + ) ) def test_register_duplicate_username_and_email(self): @@ -1395,7 +1399,7 @@ class RegistrationViewTest(ApiTestCase): self.assertEqual(response.status_code, 409) self.assertEqual( response.content, - "It looks like {} and {} belong to an existing account.".format( + "It looks like {} and {} belong to an existing account. Try again with a different email address and username.".format( self.EMAIL, self.USERNAME ) ) diff --git a/common/djangoapps/user_api/views.py b/common/djangoapps/user_api/views.py index 58c18c3097..4791f15d2d 100644 --- a/common/djangoapps/user_api/views.py +++ b/common/djangoapps/user_api/views.py @@ -280,19 +280,19 @@ class RegistrationView(APIView): # account using both an email address and a username associated with an # existing account. error_msg = _( - u"It looks like {email_address} and {username} belong to an existing account." + u"It looks like {email_address} and {username} belong to an existing account. Try again with a different email address and username." ).format(email_address=email, username=username) elif 'email' in conflicts: # Translators: This message is shown to users who attempt to create a new # account using an email address associated with an existing account. error_msg = _( - u"It looks like {email_address} belongs to an existing account." + u"It looks like {email_address} belongs to an existing account. Try again with a different email address." ).format(email_address=email) else: # Translators: This message is shown to users who attempt to create a new # account using a username associated with an existing account. error_msg = _( - u"It looks like {username} belongs to an existing account." + u"It looks like {username} belongs to an existing account. Try again with a different username." ).format(username=username) return HttpResponse( @@ -504,14 +504,20 @@ class RegistrationView(APIView): # Translators: "Terms of Service" is a legal document users must agree to # in order to register a new account. label = _( - u"I agree to the {terms_of_service}" - ).format(terms_of_service=terms_link) + u"I agree to the {platform_name} {terms_of_service}." + ).format( + platform_name=settings.PLATFORM_NAME, + terms_of_service=terms_link + ) # Translators: "Terms of Service" is a legal document users must agree to # in order to register a new account. error_msg = _( - u"You must agree to the {terms_of_service}" - ).format(terms_of_service=terms_link) + u"You must agree to the {platform_name} {terms_of_service}." + ).format( + platform_name=settings.PLATFORM_NAME, + terms_of_service=terms_link + ) form_desc.add_field( "honor_code", @@ -535,11 +541,21 @@ class RegistrationView(APIView): # Translators: "Terms of service" is a legal document users must agree to # in order to register a new account. - label = _(u"I agree to the {terms_of_service}").format(terms_of_service=terms_link) + label = _( + u"I agree to the {platform_name} {terms_of_service}." + ).format( + platform_name=settings.PLATFORM_NAME, + terms_of_service=terms_link + ) # Translators: "Terms of service" is a legal document users must agree to # in order to register a new account. - error_msg = _("You must agree to the {terms_of_service}").format(terms_of_service=terms_link) + error_msg = _( + u"You must agree to the {platform_name} {terms_of_service}." + ).format( + platform_name=settings.PLATFORM_NAME, + terms_of_service=terms_link + ) form_desc.add_field( "terms_of_service", diff --git a/common/static/js/spec/edx.utils.validate_spec.js b/common/static/js/spec/edx.utils.validate_spec.js index 54de2f03ff..2983f421b2 100644 --- a/common/static/js/spec/edx.utils.validate_spec.js +++ b/common/static/js/spec/edx.utils.validate_spec.js @@ -9,9 +9,10 @@ describe('edx.utils.validate', function () { VALID_STRING = 'xsy_is_awesome', SHORT_STRING = 'x', LONG_STRING = 'xsy_is_way_too_awesome', - REQUIRED_ERROR_FRAGMENT = 'required', + EMAIL_ERROR_FRAGMENT = 'formatted', MIN_ERROR_FRAGMENT = 'least', MAX_ERROR_FRAGMENT = 'up to', + REQUIRED_ERROR_FRAGMENT = 'empty', CUSTOM_MESSAGE = 'custom message'; var createFixture = function( type, name, required, minlength, maxlength, value ) { @@ -117,11 +118,11 @@ describe('edx.utils.validate', function () { createFixture('email', 'email', false, MIN_LENGTH, MAX_LENGTH, 'localpart'); // Verify optional field behavior - expectInvalid('invalid'); + expectInvalid(EMAIL_ERROR_FRAGMENT); // Verify required field behavior field.prop('required', false); - expectInvalid('invalid'); + expectInvalid(EMAIL_ERROR_FRAGMENT); }); it('succeeds if an email field is provided a valid address', function () { diff --git a/common/static/js/utils/edx.utils.validate.js b/common/static/js/utils/edx.utils.validate.js index b20a10016e..60366c6630 100644 --- a/common/static/js/utils/edx.utils.validate.js +++ b/common/static/js/utils/edx.utils.validate.js @@ -18,10 +18,10 @@ var edx = edx || {}; validate: { msg: { - email: '
  • <%- gettext("The email address you\'ve provided is invalid.") %>
    • ', - min: '
  • <%- _.sprintf(gettext("%(field)s must have at least %(count)d characters"), context) %>
    • ', - max: '
  • <%- _.sprintf(gettext("%(field)s can only contain up to %(count)d characters"), context) %>
    • ', - required: '
  • <%- _.sprintf(gettext("%(field)s is required"), context) %>
    • ', + email: '
  • <%- gettext("The email address you\'ve provided isn\'t formatted correctly.") %>
    • ', + min: '
  • <%- _.sprintf(gettext("%(field)s must have at least %(count)d characters."), context) %>
    • ', + max: '
  • <%- _.sprintf(gettext("%(field)s can only contain up to %(count)d characters."), context) %>
    • ', + required: '
  • <%- _.sprintf(gettext("The %(field)s field cannot be empty."), context) %>
    • ', custom: '
  • <%= content %>
    • ' }, @@ -73,12 +73,6 @@ var edx = edx || {}; var max = $el.attr('maxlength') || false; return ( !!max ) ? max >= $el.val().length : true; - }, - - capitalizeFirstLetter: function( str ) { - str = str.replace('_', ' '); - - return str.charAt(0).toUpperCase() + str.slice(1); } }, @@ -121,16 +115,21 @@ var edx = edx || {}; } }, + getLabel: function( id ) { + // Extract the field label, remove the asterisk (if it appears) and any extra whitespace + return $("label[for=" + id + "]").text().split("*")[0].trim(); + }, + getMessage: function( $el, tests ) { var txt = [], tpl, - name, + label, obj, customMsg; _.each( tests, function( value, key ) { if ( !value ) { - name = $el.attr('name'); + label = _fn.validate.getLabel( $el.attr('id') ); customMsg = $el.data('errormsg-' + key) || false; // If the field has a custom error msg attached, use it @@ -147,7 +146,7 @@ var edx = edx || {}; // We pass the context object to the template so that // we can perform variable interpolation using sprintf context: { - field: _fn.validate.str.capitalizeFirstLetter( name ) + field: label } }; diff --git a/lms/templates/student_account/access.underscore b/lms/templates/student_account/access.underscore index 65b66bd4cf..6117f11d9e 100644 --- a/lms/templates/student_account/access.underscore +++ b/lms/templates/student_account/access.underscore @@ -5,7 +5,7 @@ diff --git a/lms/templates/student_account/register.underscore b/lms/templates/student_account/register.underscore index d588940666..8d93a2427c 100644 --- a/lms/templates/student_account/register.underscore +++ b/lms/templates/student_account/register.underscore @@ -18,7 +18,7 @@
    From 1a5c3977af84bfb24211829b2dee9aabf7f644ac Mon Sep 17 00:00:00 2001 From: AlasdairSwan Date: Wed, 12 Nov 2014 08:52:51 -0500 Subject: [PATCH 82/97] ECOM-632 added asterix explanation footnote --- lms/static/sass/views/_login-register.scss | 7 +++++++ lms/templates/student_account/register.underscore | 2 ++ 2 files changed, 9 insertions(+) diff --git a/lms/static/sass/views/_login-register.scss b/lms/static/sass/views/_login-register.scss index fcbd19b796..ae74f77ff4 100644 --- a/lms/static/sass/views/_login-register.scss +++ b/lms/static/sass/views/_login-register.scss @@ -95,6 +95,13 @@ } } + .note { + @extend %t-copy-sub2; + display: block; + font-weight: normal; + color: $gray; + margin-top: 15px; + } /** The forms **/ diff --git a/lms/templates/student_account/register.underscore b/lms/templates/student_account/register.underscore index 01ffe07f47..5d06426531 100644 --- a/lms/templates/student_account/register.underscore +++ b/lms/templates/student_account/register.underscore @@ -25,4 +25,6 @@ <%= context.fields %> + +

    * <%- gettext("Required fields") %>

    From d50137bb64146eddbf297bcddfc4e06b7529b3a3 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Wed, 12 Nov 2014 09:33:06 -0500 Subject: [PATCH 83/97] Redirect to a valid URL when cancelling third party auth --- .../djangoapps/third_party_auth/middleware.py | 11 +++++-- .../djangoapps/third_party_auth/pipeline.py | 32 ++++++++++++++++--- .../third_party_auth/tests/specs/base.py | 22 ++++++------- 3 files changed, 48 insertions(+), 17 deletions(-) diff --git a/common/djangoapps/third_party_auth/middleware.py b/common/djangoapps/third_party_auth/middleware.py index 677534d0fc..fe843f6a7b 100644 --- a/common/djangoapps/third_party_auth/middleware.py +++ b/common/djangoapps/third_party_auth/middleware.py @@ -9,10 +9,17 @@ class ExceptionMiddleware(SocialAuthExceptionMiddleware): """Custom middleware that handles conditional redirection.""" def get_redirect_uri(self, request, exception): + # Fall back to django settings's SOCIAL_AUTH_LOGIN_ERROR_URL. + redirect_uri = super(ExceptionMiddleware, self).get_redirect_uri(request, exception) + # Safe because it's already been validated by # pipeline.parse_query_params. If that pipeline step ever moves later # in the pipeline stack, we'd need to validate this value because it # would be an injection point for attacker data. auth_entry = request.session.get(pipeline.AUTH_ENTRY_KEY) - # Fall back to django settings's SOCIAL_AUTH_LOGIN_ERROR_URL. - return '/' + auth_entry if auth_entry else super(ExceptionMiddleware, self).get_redirect_uri(request, exception) + + # Check if we have an auth entry key we can use instead + if auth_entry and auth_entry in pipeline.AUTH_DISPATCH_URLS: + redirect_uri = pipeline.AUTH_DISPATCH_URLS[auth_entry] + + return redirect_uri diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 966fae9497..4114b5ee36 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -119,6 +119,30 @@ AUTH_ENTRY_REGISTER_2 = 'account_register' AUTH_ENTRY_API = 'api' +# URLs associated with auth entry points +# These are used to request additional user information +# (for example, account credentials when logging in), +# and when the user cancels the auth process +# (e.g., refusing to grant permission on the provider's login page). +# We don't use "reverse" here because doing so may cause modules +# to load that depend on this module. +AUTH_DISPATCH_URLS = { + AUTH_ENTRY_DASHBOARD: '/dashboard', + AUTH_ENTRY_LOGIN: '/login', + AUTH_ENTRY_REGISTER: '/register', + + # TODO (ECOM-369): Replace the dispatch URLs + # for `AUTH_ENTRY_LOGIN` and `AUTH_ENTRY_REGISTER` + # with these values, but DO NOT DELETE THESE KEYS. + AUTH_ENTRY_LOGIN_2: '/account/login/', + AUTH_ENTRY_REGISTER_2: '/account/register/', + + # If linking/unlinking an account from the new student profile + # page, redirect to the profile page. Only used if + # `FEATURES['ENABLE_NEW_DASHBOARD']` is true. + AUTH_ENTRY_PROFILE: '/profile/', +} + _AUTH_ENTRY_CHOICES = frozenset([ AUTH_ENTRY_DASHBOARD, AUTH_ENTRY_LOGIN, @@ -483,20 +507,20 @@ def ensure_user_information( return if dispatch_to_login: - return redirect('/login', name='signin_user') + return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_LOGIN], name='signin_user') # TODO (ECOM-369): Consolidate this with `dispatch_to_login` # once the A/B test completes. if dispatch_to_login_2: - return redirect(reverse(AUTH_ENTRY_LOGIN_2)) + return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_LOGIN_2]) if is_register and user_unset: - return redirect('/register', name='register_user') + return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_REGISTER], name='register_user') # TODO (ECOM-369): Consolidate this with `is_register` # once the A/B test completes. if is_register_2 and user_unset: - return redirect(reverse(AUTH_ENTRY_REGISTER_2)) + return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_REGISTER_2]) @partial.partial diff --git a/common/djangoapps/third_party_auth/tests/specs/base.py b/common/djangoapps/third_party_auth/tests/specs/base.py index a019fa83d2..da29b9b9a3 100644 --- a/common/djangoapps/third_party_auth/tests/specs/base.py +++ b/common/djangoapps/third_party_auth/tests/specs/base.py @@ -142,7 +142,7 @@ class IntegrationTest(testutil.TestCase, test.TestCase): self.assertEqual('link' if linked else 'unlink', icon_state) self.assertEqual(self.PROVIDER_CLASS.NAME, provider_name) - def assert_exception_redirect_looks_correct(self, auth_entry=None): + def assert_exception_redirect_looks_correct(self, expected_uri, auth_entry=None): """Tests middleware conditional redirection. middleware.ExceptionMiddleware makes sure the user ends up in the right @@ -157,13 +157,7 @@ class IntegrationTest(testutil.TestCase, test.TestCase): self.assertEqual(302, response.status_code) self.assertIn('canceled', location) self.assertIn(self.backend_name, location) - - if auth_entry: - # Custom redirection to form. - self.assertTrue(location.startswith('/' + auth_entry)) - else: - # Stock framework redirection to root. - self.assertTrue(location.startswith('/?')) + self.assertTrue(location.startswith(expected_uri + '?')) def assert_first_party_auth_trumps_third_party_auth(self, email=None, password=None, success=None): """Asserts first party auth was used in place of third party auth. @@ -410,13 +404,19 @@ class IntegrationTest(testutil.TestCase, test.TestCase): # Actual tests, executed once per child. def test_canceling_authentication_redirects_to_login_when_auth_entry_login(self): - self.assert_exception_redirect_looks_correct(auth_entry=pipeline.AUTH_ENTRY_LOGIN) + self.assert_exception_redirect_looks_correct('/login', auth_entry=pipeline.AUTH_ENTRY_LOGIN) def test_canceling_authentication_redirects_to_register_when_auth_entry_register(self): - self.assert_exception_redirect_looks_correct(auth_entry=pipeline.AUTH_ENTRY_REGISTER) + self.assert_exception_redirect_looks_correct('/register', auth_entry=pipeline.AUTH_ENTRY_REGISTER) + + def test_canceling_authentication_redirects_to_login_when_auth_login_2(self): + self.assert_exception_redirect_looks_correct('/account/login/', auth_entry=pipeline.AUTH_ENTRY_LOGIN_2) + + def test_canceling_authentication_redirects_to_login_when_auth_register_2(self): + self.assert_exception_redirect_looks_correct('/account/register/', auth_entry=pipeline.AUTH_ENTRY_REGISTER_2) def test_canceling_authentication_redirects_to_root_when_auth_entry_not_set(self): - self.assert_exception_redirect_looks_correct() + self.assert_exception_redirect_looks_correct('/') def test_full_pipeline_succeeds_for_linking_account(self): # First, create, the request and strategy that store pipeline state, From 02485be3e3e5cef013cf4ab6f3722ce5f61e3b72 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Wed, 12 Nov 2014 10:27:25 -0500 Subject: [PATCH 84/97] Clean up redirect logic --- common/djangoapps/user_api/helpers.py | 25 +++++++++++-------- .../djangoapps/user_api/tests/test_helpers.py | 6 ++--- .../js/student_account/views/RegisterView.js | 4 --- 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/common/djangoapps/user_api/helpers.py b/common/djangoapps/user_api/helpers.py index 06eb2497d7..cf7452b058 100644 --- a/common/djangoapps/user_api/helpers.py +++ b/common/djangoapps/user_api/helpers.py @@ -323,9 +323,6 @@ def shim_student_view(view_func, check_logged_in=False): (instead of always using status 200, but setting "success" to False in the JSON-serialized content of the response) - * Use status code 302 for redirects instead of - "redirect_url" in the JSON-serialized content of the response. - * Use status code 403 to indicate a login failure. The shim will preserve any cookies set by the view. @@ -389,17 +386,30 @@ def shim_student_view(view_func, check_logged_in=False): # 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. + # + # We ignore the "redirect_url" parameter, because we don't need it: + # 1) It's used to redirect on change enrollment, which + # our client will handle directly + # (that's why we strip out the enrollment params from the request) + # 2) It's used by third party auth when a user has already successfully + # authenticated and we're not sending login credentials. However, + # this case is never encountered in practice: on the old login page, + # the login form would be submitted directly, so third party auth + # would always be "trumped" by first party auth. If a user has + # successfully authenticated with us, we redirect them to the dashboard + # regardless of how they authenticated; and if a user is completing + # the third party auth pipeline, we redirect them from the pipeline + # completion end-point directly. try: response_dict = json.loads(response.content) msg = response_dict.get("value", u"") - redirect_url = response_dict.get("redirect_url") or response_dict.get("redirect") success = response_dict.get("success") except (ValueError, TypeError): msg = response.content success = True - redirect_url = None # If the user is not authenticated when we expect them to be # send the appropriate status code. @@ -426,11 +436,6 @@ def shim_student_view(view_func, check_logged_in=False): response.status_code = 403 response.content = msg - # If the view wants to redirect us, send a status 302 - elif redirect_url is not None: - response.status_code = 302 - response['Location'] = redirect_url - # If an error condition occurs, send a status 400 elif response.status_code != 200 or not success: # The student views tend to send status 200 even when an error occurs diff --git a/common/djangoapps/user_api/tests/test_helpers.py b/common/djangoapps/user_api/tests/test_helpers.py index 215957dd40..693afd2e0d 100644 --- a/common/djangoapps/user_api/tests/test_helpers.py +++ b/common/djangoapps/user_api/tests/test_helpers.py @@ -177,7 +177,7 @@ class StudentViewShimTest(TestCase): self.assertEqual(response.content, "Not a JSON dict") @ddt.data("redirect", "redirect_url") - def test_redirect_from_json(self, redirect_key): + def test_ignore_redirect_from_json(self, redirect_key): view = self._shimmed_view( HttpResponse(content=json.dumps({ "success": True, @@ -185,8 +185,8 @@ class StudentViewShimTest(TestCase): })) ) response = view(HttpRequest()) - self.assertEqual(response.status_code, 302) - self.assertEqual(response['Location'], "/redirect") + self.assertEqual(response.status_code, 200) + self.assertEqual(response.content, "") def test_error_from_json(self): view = self._shimmed_view( diff --git a/lms/static/js/student_account/views/RegisterView.js b/lms/static/js/student_account/views/RegisterView.js index 2fd857f0ee..fe8d28c013 100644 --- a/lms/static/js/student_account/views/RegisterView.js +++ b/lms/static/js/student_account/views/RegisterView.js @@ -57,9 +57,5 @@ var edx = edx || {}; saveSuccess: function() { this.trigger('auth-complete'); }, - - redirect: function( url ) { - window.location.href = url; - } }); })(jQuery, _, gettext); From 0a35af5530de0a16b6377bf63791351dd1b6e842 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Wed, 12 Nov 2014 11:01:32 -0500 Subject: [PATCH 85/97] Remove extra viewport meta tag --- lms/templates/main.html | 1 - 1 file changed, 1 deletion(-) diff --git a/lms/templates/main.html b/lms/templates/main.html index ceafa142e8..24bd8d4870 100644 --- a/lms/templates/main.html +++ b/lms/templates/main.html @@ -10,7 +10,6 @@ %> - % if responsive: From 8d02efb02145928bdfe31abb446fe75f7a2658d2 Mon Sep 17 00:00:00 2001 From: Renzo Lucioni Date: Mon, 10 Nov 2014 13:25:04 -0500 Subject: [PATCH 86/97] Clean up pep8 and pylint violations Also fixes failing Python unit tests --- common/djangoapps/enrollment/api.py | 19 +-- common/djangoapps/enrollment/data.py | 9 +- common/djangoapps/enrollment/serializers.py | 3 +- .../enrollment/tests/fake_data_api.py | 10 +- .../djangoapps/enrollment/tests/test_data.py | 3 +- .../djangoapps/enrollment/tests/test_views.py | 3 +- common/djangoapps/enrollment/views.py | 6 +- .../djangoapps/third_party_auth/pipeline.py | 12 +- common/djangoapps/user_api/api/account.py | 1 - common/djangoapps/user_api/api/course_tag.py | 7 +- common/djangoapps/user_api/helpers.py | 6 +- .../user_api/tests/test_constants.py | 2 +- .../djangoapps/user_api/tests/test_helpers.py | 16 +-- .../djangoapps/user_api/tests/test_views.py | 57 ++++++-- common/djangoapps/user_api/urls.py | 1 + common/djangoapps/user_api/views.py | 130 ++++++++++++++++-- common/djangoapps/util/testing.py | 1 + lms/djangoapps/student_account/helpers.py | 4 +- 18 files changed, 222 insertions(+), 68 deletions(-) diff --git a/common/djangoapps/enrollment/api.py b/common/djangoapps/enrollment/api.py index ea76e032a8..7a66d3b335 100644 --- a/common/djangoapps/enrollment/api.py +++ b/common/djangoapps/enrollment/api.py @@ -11,33 +11,36 @@ log = logging.getLogger(__name__) class CourseEnrollmentError(Exception): - """ Generic Course Enrollment Error. + """Generic Course Enrollment Error. Describes any error that may occur when reading or updating enrollment information for a student or a course. """ def __init__(self, msg, data=None): - super(Exception, self).__init__(msg) + super(CourseEnrollmentError, self).__init__(msg) # Corresponding information to help resolve the error. self.data = data class CourseModeNotFoundError(CourseEnrollmentError): + """The requested course mode could not be found.""" pass class EnrollmentNotFoundError(CourseEnrollmentError): + """The requested enrollment could not be found.""" pass class EnrollmentApiLoadError(CourseEnrollmentError): + """The data API could not be loaded.""" pass DEFAULT_DATA_API = 'enrollment.data' def get_enrollments(student_id): - """ Retrieves all the courses a student is enrolled in. + """Retrieves all the courses a student is enrolled in. Takes a student and retrieves all relative enrollments. Includes information regarding how the student is enrolled in the the course. @@ -107,7 +110,7 @@ def get_enrollments(student_id): def get_enrollment(student_id, course_id): - """ Retrieves all enrollment information for the student in respect to a specific course. + """Retrieves all enrollment information for the student in respect to a specific course. Gets all the course enrollment information specific to a student in a course. @@ -151,7 +154,7 @@ def get_enrollment(student_id, course_id): def add_enrollment(student_id, course_id, mode='honor', is_active=True): - """ Enrolls a student in a course. + """Enrolls a student in a course. Enrolls a student in a course. If the mode is not specified, this will default to 'honor'. @@ -199,7 +202,7 @@ def add_enrollment(student_id, course_id, mode='honor', is_active=True): def deactivate_enrollment(student_id, course_id): - """ Un-enrolls a student in a course + """Un-enrolls a student in a course Deactivate the enrollment of a student in a course. We will not remove the enrollment data, but simply flag it as inactive. @@ -249,7 +252,7 @@ def deactivate_enrollment(student_id, course_id): def update_enrollment(student_id, course_id, mode): - """ Updates the course mode for the enrolled user. + """Updates the course mode for the enrolled user. Update a course enrollment for the given student and course. @@ -295,7 +298,7 @@ def update_enrollment(student_id, course_id, mode): def get_course_enrollment_details(course_id): - """ Get the course modes for course. Also get enrollment start and end date, invite only, etc. + """Get the course modes for course. Also get enrollment start and end date, invite only, etc. Given a course_id, return a serializable dictionary of properties describing course enrollment information. diff --git a/common/djangoapps/enrollment/data.py b/common/djangoapps/enrollment/data.py index 82db6734fa..fe1e07552f 100644 --- a/common/djangoapps/enrollment/data.py +++ b/common/djangoapps/enrollment/data.py @@ -7,7 +7,6 @@ import logging from django.contrib.auth.models import User from opaque_keys.edx.keys import CourseKey from xmodule.modulestore.django import modulestore -from xmodule.modulestore.exceptions import ItemNotFoundError from enrollment.serializers import CourseEnrollmentSerializer, CourseField from student.models import CourseEnrollment, NonExistentCourseError @@ -17,7 +16,7 @@ log = logging.getLogger(__name__) def get_course_enrollments(student_id): """Retrieve a list representing all aggregated data for a student's course enrollments. - Construct a representation of all course enrollment data for a specific student.. + Construct a representation of all course enrollment data for a specific student. Args: student_id (str): The name of the student to retrieve course enrollment information for. @@ -29,7 +28,7 @@ def get_course_enrollments(student_id): qset = CourseEnrollment.objects.filter( user__username=student_id, is_active=True ).order_by('created') - return CourseEnrollmentSerializer(qset).data + return CourseEnrollmentSerializer(qset).data # pylint: disable=no-member def get_course_enrollment(student_id, course_id): @@ -50,7 +49,7 @@ def get_course_enrollment(student_id, course_id): enrollment = CourseEnrollment.objects.get( user__username=student_id, course_id=course_key ) - return CourseEnrollmentSerializer(enrollment).data + return CourseEnrollmentSerializer(enrollment).data # pylint: disable=no-member except CourseEnrollment.DoesNotExist: return None @@ -79,7 +78,7 @@ def update_course_enrollment(student_id, course_id, mode=None, is_active=None): enrollment.update_enrollment(is_active=is_active, mode=mode) enrollment.save() - return CourseEnrollmentSerializer(enrollment).data + return CourseEnrollmentSerializer(enrollment).data # pylint: disable=no-member def get_course_enrollment_info(course_id): diff --git a/common/djangoapps/enrollment/serializers.py b/common/djangoapps/enrollment/serializers.py index d01deb0023..5306f228cd 100644 --- a/common/djangoapps/enrollment/serializers.py +++ b/common/djangoapps/enrollment/serializers.py @@ -3,7 +3,6 @@ Serializers for all Course Enrollment related return objects. """ from rest_framework import serializers -from rest_framework.fields import Field from student.models import CourseEnrollment from course_modes.models import CourseMode @@ -36,7 +35,7 @@ class CourseField(serializers.RelatedField): def to_native(self, course): course_id = unicode(course.id) - course_modes = ModeSerializer(CourseMode.modes_for_course(course.id)).data + course_modes = ModeSerializer(CourseMode.modes_for_course(course.id)).data # pylint: disable=no-member return { "course_id": course_id, diff --git a/common/djangoapps/enrollment/tests/fake_data_api.py b/common/djangoapps/enrollment/tests/fake_data_api.py index d1374c3fef..80140aaec5 100644 --- a/common/djangoapps/enrollment/tests/fake_data_api.py +++ b/common/djangoapps/enrollment/tests/fake_data_api.py @@ -20,6 +20,7 @@ _ENROLLMENTS = [] _COURSES = [] +# pylint: disable=unused-argument def get_course_enrollments(student_id): """Stubbed out Enrollment data request.""" return _ENROLLMENTS @@ -48,18 +49,21 @@ def get_course_enrollment_info(course_id): def _get_fake_enrollment(student_id, course_id): + """Get an enrollment from the enrollments array.""" for enrollment in _ENROLLMENTS: if student_id == enrollment['student'] and course_id == enrollment['course']['course_id']: return enrollment def _get_fake_course_info(course_id): + """Get a course from the courses array.""" for course in _COURSES: if course_id == course['course_id']: return course def add_enrollment(student_id, course_id, is_active=True, mode='honor'): + """Append an enrollment to the enrollments array.""" enrollment = { "created": datetime.datetime.now(), "mode": mode, @@ -72,6 +76,7 @@ def add_enrollment(student_id, course_id, is_active=True, mode='honor'): def add_course(course_id, enrollment_start=None, enrollment_end=None, invite_only=False, course_modes=None): + """Append course to the courses array.""" course_info = { "course_id": course_id, "enrollment_end": enrollment_end, @@ -90,7 +95,8 @@ def add_course(course_id, enrollment_start=None, enrollment_end=None, invite_onl def reset(): - global _COURSES + """Set the enrollments and courses arrays to be empty.""" + global _COURSES # pylint: disable=global-statement _COURSES = [] - global _ENROLLMENTS + global _ENROLLMENTS # pylint: disable=global-statement _ENROLLMENTS = [] diff --git a/common/djangoapps/enrollment/tests/test_data.py b/common/djangoapps/enrollment/tests/test_data.py index 9655ecb00d..f069f68bcc 100644 --- a/common/djangoapps/enrollment/tests/test_data.py +++ b/common/djangoapps/enrollment/tests/test_data.py @@ -34,7 +34,7 @@ class EnrollmentDataTest(ModuleStoreTestCase): PASSWORD = "edx" def setUp(self): - """ Create a course and user, then log in. """ + """Create a course and user, then log in. """ super(EnrollmentDataTest, self).setUp() self.course = CourseFactory.create() self.user = UserFactory.create(username=self.USERNAME, email=self.EMAIL, password=self.PASSWORD) @@ -163,6 +163,7 @@ class EnrollmentDataTest(ModuleStoreTestCase): data.get_course_enrollment_info("this/is/bananas") def _create_course_modes(self, course_modes, course=None): + """Create the course modes required for a test. """ course_id = course.id if course else self.course.id for mode_slug in course_modes: CourseModeFactory.create( diff --git a/common/djangoapps/enrollment/tests/test_views.py b/common/djangoapps/enrollment/tests/test_views.py index 0b4adbc738..06e55a6d02 100644 --- a/common/djangoapps/enrollment/tests/test_views.py +++ b/common/djangoapps/enrollment/tests/test_views.py @@ -60,7 +60,7 @@ class EnrollmentTest(ModuleStoreTestCase, APITestCase): mode_display_name=mode_slug, ) - # Enroll in the course and verify the URL we get sent to + # Create an enrollment self._create_enrollment() self.assertTrue(CourseEnrollment.is_enrolled(self.user, self.course.id)) @@ -159,6 +159,7 @@ class EnrollmentTest(ModuleStoreTestCase, APITestCase): self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) def _create_enrollment(self): + """Enroll in the course and verify the URL we are sent to. """ resp = self.client.post(reverse('courseenrollment', kwargs={'course_id': (unicode(self.course.id))})) self.assertEqual(resp.status_code, status.HTTP_200_OK) data = json.loads(resp.content) diff --git a/common/djangoapps/enrollment/views.py b/common/djangoapps/enrollment/views.py index 35a3fc603d..c3970cfe58 100644 --- a/common/djangoapps/enrollment/views.py +++ b/common/djangoapps/enrollment/views.py @@ -14,7 +14,9 @@ from student.models import NonExistentCourseError, CourseEnrollmentException class EnrollmentUserThrottle(UserRateThrottle): - rate = '50/second' # TODO Limit significantly after performance testing. + """Limit the number of requests users can make to the enrollment API.""" + # TODO Limit significantly after performance testing. # pylint: disable=fixme + rate = '50/second' class SessionAuthenticationAllowInactiveUser(SessionAuthentication): @@ -48,7 +50,7 @@ class SessionAuthenticationAllowInactiveUser(SessionAuthentication): """ # Get the underlying HttpRequest object - request = request._request + request = request._request # pylint: disable=protected-access user = getattr(request, 'user', None) # Unauthenticated, CSRF validation not required diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 4114b5ee36..6fb7951fb5 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -111,7 +111,8 @@ AUTH_ENTRY_LOGIN = 'login' AUTH_ENTRY_PROFILE = 'profile' AUTH_ENTRY_REGISTER = 'register' -# TODO (ECOM-369): Repace `AUTH_ENTRY_LOGIN` and `AUTH_ENTRY_REGISTER` +# pylint: disable=fixme +# TODO (ECOM-369): Replace `AUTH_ENTRY_LOGIN` and `AUTH_ENTRY_REGISTER` # with these values once the A/B test completes, then delete # these constants. AUTH_ENTRY_LOGIN_2 = 'account_login' @@ -153,6 +154,7 @@ _AUTH_ENTRY_CHOICES = frozenset([ # login/registration, we needed to introduce two # additional end-points. Once the test completes, # delete these constants from the choices list. + # pylint: disable=fixme AUTH_ENTRY_LOGIN_2, AUTH_ENTRY_REGISTER_2, @@ -445,6 +447,7 @@ def parse_query_params(strategy, response, *args, **kwargs): # TODO (ECOM-369): Delete these once the A/B test # for the combined login/registration form completes. + # pylint: disable=fixme 'is_login_2': auth_entry == AUTH_ENTRY_LOGIN_2, 'is_register_2': auth_entry == AUTH_ENTRY_REGISTER_2, } @@ -457,6 +460,7 @@ def parse_query_params(strategy, response, *args, **kwargs): # these kwargs in `redirect_to_supplementary_form`, but # these should redirect to the same location as "is_login" and "is_register" # (whichever login/registration end-points win in the test). +# pylint: disable=fixme @partial.partial def ensure_user_information( strategy, @@ -500,7 +504,7 @@ def ensure_user_information( return HttpResponseBadRequest() # TODO (ECOM-369): Consolidate this with `dispatch_to_login` - # once the A/B test completes. + # once the A/B test completes. # pylint: disable=fixme dispatch_to_login_2 = is_login_2 and (user_unset or user_inactive) if is_dashboard or is_profile: @@ -510,7 +514,7 @@ def ensure_user_information( return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_LOGIN], name='signin_user') # TODO (ECOM-369): Consolidate this with `dispatch_to_login` - # once the A/B test completes. + # once the A/B test completes. # pylint: disable=fixme if dispatch_to_login_2: return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_LOGIN_2]) @@ -518,7 +522,7 @@ def ensure_user_information( return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_REGISTER], name='register_user') # TODO (ECOM-369): Consolidate this with `is_register` - # once the A/B test completes. + # once the A/B test completes. # pylint: disable=fixme if is_register_2 and user_unset: return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_REGISTER_2]) diff --git a/common/djangoapps/user_api/api/account.py b/common/djangoapps/user_api/api/account.py index 06c9c5b035..118838c7b4 100644 --- a/common/djangoapps/user_api/api/account.py +++ b/common/djangoapps/user_api/api/account.py @@ -8,7 +8,6 @@ information and preferences). """ from django.conf import settings from django.db import transaction, IntegrityError -from django.db.models import Q from django.core.validators import validate_email, validate_slug, ValidationError from django.contrib.auth.forms import PasswordResetForm diff --git a/common/djangoapps/user_api/api/course_tag.py b/common/djangoapps/user_api/api/course_tag.py index 7c7a01ce34..43907fbe72 100644 --- a/common/djangoapps/user_api/api/course_tag.py +++ b/common/djangoapps/user_api/api/course_tag.py @@ -53,6 +53,10 @@ def set_course_tag(user, course_id, key, value): key: arbitrary (<=255 char string) value: arbitrary string """ + # pylint: disable=W0511 + # TODO: There is a risk of IntegrityErrors being thrown here given + # simultaneous calls from many processes. Handle by retrying after + # a short delay? record, _ = UserCourseTag.objects.get_or_create( user=user, @@ -61,6 +65,3 @@ def set_course_tag(user, course_id, key, value): record.value = value record.save() - - # TODO: There is a risk of IntegrityErrors being thrown here given - # simultaneous calls from many processes. Handle by retrying after a short delay? diff --git a/common/djangoapps/user_api/helpers.py b/common/djangoapps/user_api/helpers.py index 06eb2497d7..233284e791 100644 --- a/common/djangoapps/user_api/helpers.py +++ b/common/djangoapps/user_api/helpers.py @@ -72,9 +72,9 @@ def require_post_params(required_params): HttpResponse """ - def _decorator(func): + def _decorator(func): # pylint: disable=missing-docstring @wraps(func) - def _wrapped(*args, **kwargs): + def _wrapped(*args, **_kwargs): # pylint: disable=missing-docstring request = args[0] missing_params = set(required_params) - set(request.POST.keys()) if len(missing_params) > 0: @@ -342,7 +342,7 @@ def shim_student_view(view_func, check_logged_in=False): """ @wraps(view_func) - def _inner(request): + def _inner(request): # pylint: disable=missing-docstring # Ensure that the POST querydict is mutable request.POST = request.POST.copy() diff --git a/common/djangoapps/user_api/tests/test_constants.py b/common/djangoapps/user_api/tests/test_constants.py index 7f2b086847..7508965559 100644 --- a/common/djangoapps/user_api/tests/test_constants.py +++ b/common/djangoapps/user_api/tests/test_constants.py @@ -250,4 +250,4 @@ SORTED_COUNTRIES = [ (u'ZM', u'Zambia'), (u'ZW', u'Zimbabwe'), (u'AX', u'\xc5land Islands') -] \ No newline at end of file +] diff --git a/common/djangoapps/user_api/tests/test_helpers.py b/common/djangoapps/user_api/tests/test_helpers.py index 215957dd40..94101e04c0 100644 --- a/common/djangoapps/user_api/tests/test_helpers.py +++ b/common/djangoapps/user_api/tests/test_helpers.py @@ -14,12 +14,12 @@ from user_api.helpers import ( class FakeInputException(Exception): - """Fake exception that should be intercepted. """ + """Fake exception that should be intercepted.""" pass class FakeOutputException(Exception): - """Fake exception that should be raised. """ + """Fake exception that should be raised.""" pass @@ -36,9 +36,7 @@ def intercepted_function(raise_error=None): class InterceptErrorsTest(TestCase): - """ - Tests for the decorator that intercepts errors. - """ + """Tests for the decorator that intercepts errors.""" @raises(FakeOutputException) def test_intercepts_errors(self): @@ -73,7 +71,7 @@ class InterceptErrorsTest(TestCase): class FormDescriptionTest(TestCase): - + """Tests of helper functions which generate form descriptions.""" def test_to_json(self): desc = FormDescription("post", "/submit") desc.add_field( @@ -134,7 +132,7 @@ class FormDescriptionTest(TestCase): @ddt.ddt class StudentViewShimTest(TestCase): - + "Tests of the student view shim." def setUp(self): self.captured_request = None @@ -211,8 +209,8 @@ class StudentViewShimTest(TestCase): response = view(HttpRequest()) self.assertEqual(response.status_code, 403) - def _shimmed_view(self, response, check_logged_in=False): - def stub_view(request): + def _shimmed_view(self, response, check_logged_in=False): # pylint: disable=missing-docstring + def stub_view(request): # pylint: disable=missing-docstring self.captured_request = request return response return shim_student_view(stub_view, check_logged_in=check_logged_in) diff --git a/common/djangoapps/user_api/tests/test_views.py b/common/djangoapps/user_api/tests/test_views.py index 49d3741875..01886c0093 100644 --- a/common/djangoapps/user_api/tests/test_views.py +++ b/common/djangoapps/user_api/tests/test_views.py @@ -112,6 +112,11 @@ class ApiTestCase(TestCase): self.assertEqual(response.status_code, 405) def assertAuthDisabled(self, method, uri): + """ + Assert that the Django rest framework does not interpret basic auth + headers for views exposed to anonymous users as an attempt to authenticate. + + """ # Django rest framework interprets basic auth headers # as an attempt to authenticate with the API. # We don't want this for views available to anonymous users. @@ -987,7 +992,7 @@ class RegistrationViewTest(ApiTestCase): ) def test_register_form_year_of_birth(self): - this_year = datetime.datetime.now(UTC).year + this_year = datetime.datetime.now(UTC).year # pylint: disable=maybe-no-member year_options = ( [{"value": "", "name": "--", "default": True}] + [ {"value": unicode(year), "name": unicode(year)} @@ -1067,13 +1072,17 @@ class RegistrationViewTest(ApiTestCase): self._assert_reg_field( {"honor_code": "required"}, { - "label": "I agree to the Terms of Service and Honor Code", + "label": "I agree to the {platform_name} Terms of Service and Honor Code.".format( + platform_name=settings.PLATFORM_NAME + ), "name": "honor_code", "defaultValue": False, "type": "checkbox", "required": True, "errorMessages": { - "required": "You must agree to the Terms of Service and Honor Code" + "required": "You must agree to the {platform_name} Terms of Service and Honor Code.".format( + platform_name=settings.PLATFORM_NAME + ) } } ) @@ -1084,13 +1093,17 @@ class RegistrationViewTest(ApiTestCase): self._assert_reg_field( {"honor_code": "required"}, { - "label": "I agree to the Terms of Service and Honor Code", + "label": "I agree to the {platform_name} Terms of Service and Honor Code.".format( + platform_name=settings.PLATFORM_NAME + ), "name": "honor_code", "defaultValue": False, "type": "checkbox", "required": True, "errorMessages": { - "required": "You must agree to the Terms of Service and Honor Code" + "required": "You must agree to the {platform_name} Terms of Service and Honor Code.".format( + platform_name=settings.PLATFORM_NAME + ) } } ) @@ -1107,13 +1120,17 @@ class RegistrationViewTest(ApiTestCase): self._assert_reg_field( {"honor_code": "required", "terms_of_service": "required"}, { - "label": "I agree to the Honor Code", + "label": "I agree to the {platform_name} Honor Code.".format( + platform_name=settings.PLATFORM_NAME + ), "name": "honor_code", "defaultValue": False, "type": "checkbox", "required": True, "errorMessages": { - "required": "You must agree to the Honor Code" + "required": "You must agree to the {platform_name} Honor Code.".format( + platform_name=settings.PLATFORM_NAME + ) } } ) @@ -1122,13 +1139,17 @@ class RegistrationViewTest(ApiTestCase): self._assert_reg_field( {"honor_code": "required", "terms_of_service": "required"}, { - "label": "I agree to the Terms of Service", + "label": "I agree to the {platform_name} Terms of Service.".format( + platform_name=settings.PLATFORM_NAME + ), "name": "terms_of_service", "defaultValue": False, "type": "checkbox", "required": True, "errorMessages": { - "required": "You must agree to the Terms of Service" + "required": "You must agree to the {platform_name} Terms of Service.".format( + platform_name=settings.PLATFORM_NAME + ) } } ) @@ -1141,13 +1162,17 @@ class RegistrationViewTest(ApiTestCase): self._assert_reg_field( {"honor_code": "required", "terms_of_service": "required"}, { - "label": "I agree to the Honor Code", + "label": "I agree to the {platform_name} Honor Code.".format( + platform_name=settings.PLATFORM_NAME + ), "name": "honor_code", "defaultValue": False, "type": "checkbox", "required": True, "errorMessages": { - "required": "You must agree to the Honor Code" + "required": "You must agree to the {platform_name} Honor Code.".format( + platform_name=settings.PLATFORM_NAME + ) } } ) @@ -1156,13 +1181,17 @@ class RegistrationViewTest(ApiTestCase): self._assert_reg_field( {"honor_code": "required", "terms_of_service": "required"}, { - "label": "I agree to the Terms of Service", + "label": "I agree to the {platform_name} Terms of Service.".format( + platform_name=settings.PLATFORM_NAME + ), "name": "terms_of_service", "defaultValue": False, "type": "checkbox", "required": True, "errorMessages": { - "required": "You must agree to the Terms of Service" + "required": "You must agree to the {platform_name} Terms of Service.".format( + platform_name=settings.PLATFORM_NAME + ) } } ) @@ -1372,7 +1401,7 @@ class RegistrationViewTest(ApiTestCase): self.assertEqual(response.status_code, 409) self.assertEqual( response.content, - "It looks like {} belongs to an existing account. Try again with a different email address and username.".format( + "It looks like {} belongs to an existing account. Try again with a different username.".format( self.USERNAME ) ) diff --git a/common/djangoapps/user_api/urls.py b/common/djangoapps/user_api/urls.py index 7ae4f5f350..55a2e077cd 100644 --- a/common/djangoapps/user_api/urls.py +++ b/common/djangoapps/user_api/urls.py @@ -1,3 +1,4 @@ +# pylint: disable=missing-docstring from django.conf import settings from django.conf.urls import include, patterns, url from rest_framework import routers diff --git a/common/djangoapps/user_api/views.py b/common/djangoapps/user_api/views.py index 4791f15d2d..c6739c30c3 100644 --- a/common/djangoapps/user_api/views.py +++ b/common/djangoapps/user_api/views.py @@ -1,6 +1,5 @@ """HTTP end-points for the User API. """ import copy -import json from django.conf import settings from django.contrib.auth.models import User @@ -25,7 +24,6 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey from edxmako.shortcuts import marketing_link import third_party_auth -from microsite_configuration import microsite from user_api.api import account as account_api, profile as profile_api from user_api.helpers import FormDescription, shim_student_view, require_post_params @@ -54,7 +52,7 @@ class LoginSessionView(APIView): # so do not require authentication. authentication_classes = [] - def get(self, request): + def get(self, request): # pylint: disable=unused-argument """Return a description of the login form. This decouples clients from the API definition: @@ -64,9 +62,6 @@ class LoginSessionView(APIView): See `user_api.helpers.FormDescription` for examples of the JSON-encoded form description. - Arguments: - request (HttpRequest) - Returns: HttpResponse @@ -307,6 +302,15 @@ class RegistrationView(APIView): return shim_student_view(create_account)(request) def _add_email_field(self, form_desc, required=True): + """Add an email field to a form description. + + Arguments: + form_desc: A form description + + Keyword Arguments: + required (Boolean): Whether this field is required; defaults to True + + """ # Translators: This label appears above a field on the registration form # meant to hold the user's email address. email_label = _(u"Email") @@ -328,6 +332,15 @@ class RegistrationView(APIView): ) def _add_name_field(self, form_desc, required=True): + """Add a name field to a form description. + + Arguments: + form_desc: A form description + + Keyword Arguments: + required (Boolean): Whether this field is required; defaults to True + + """ # Translators: This label appears above a field on the registration form # meant to hold the user's full name. name_label = _(u"Full Name") @@ -347,6 +360,15 @@ class RegistrationView(APIView): ) def _add_username_field(self, form_desc, required=True): + """Add a username field to a form description. + + Arguments: + form_desc: A form description + + Keyword Arguments: + required (Boolean): Whether this field is required; defaults to True + + """ # Translators: This label appears above a field on the registration form # meant to hold the user's public username. username_label = _(u"Username") @@ -369,6 +391,15 @@ class RegistrationView(APIView): ) def _add_password_field(self, form_desc, required=True): + """Add a password field to a form description. + + Arguments: + form_desc: A form description + + Keyword Arguments: + required (Boolean): Whether this field is required; defaults to True + + """ # Translators: This label appears above a field on the registration form # meant to hold the user's password. password_label = _(u"Password") @@ -385,6 +416,15 @@ class RegistrationView(APIView): ) def _add_level_of_education_field(self, form_desc, required=True): + """Add a level of education field to a form description. + + Arguments: + form_desc: A form description + + Keyword Arguments: + required (Boolean): Whether this field is required; defaults to True + + """ # Translators: This label appears above a dropdown menu on the registration # form used to select the user's highest completed level of education. education_level_label = _(u"Highest Level of Education Completed") @@ -399,6 +439,15 @@ class RegistrationView(APIView): ) def _add_gender_field(self, form_desc, required=True): + """Add a gender field to a form description. + + Arguments: + form_desc: A form description + + Keyword Arguments: + required (Boolean): Whether this field is required; defaults to True + + """ # Translators: This label appears above a dropdown menu on the registration # form used to select the user's gender. gender_label = _(u"Gender") @@ -413,6 +462,15 @@ class RegistrationView(APIView): ) def _add_year_of_birth_field(self, form_desc, required=True): + """Add a year of birth field to a form description. + + Arguments: + form_desc: A form description + + Keyword Arguments: + required (Boolean): Whether this field is required; defaults to True + + """ # Translators: This label appears above a dropdown menu on the registration # form used to select the user's year of birth. yob_label = _(u"Year of Birth") @@ -428,6 +486,15 @@ class RegistrationView(APIView): ) def _add_mailing_address_field(self, form_desc, required=True): + """Add a mailing address field to a form description. + + Arguments: + form_desc: A form description + + Keyword Arguments: + required (Boolean): Whether this field is required; defaults to True + + """ # Translators: This label appears above a field on the registration form # meant to hold the user's mailing address. mailing_address_label = _(u"Mailing Address") @@ -440,6 +507,15 @@ class RegistrationView(APIView): ) def _add_goals_field(self, form_desc, required=True): + """Add a goals field to a form description. + + Arguments: + form_desc: A form description + + Keyword Arguments: + required (Boolean): Whether this field is required; defaults to True + + """ # Translators: This phrase appears above a field on the registration form # meant to hold the user's reasons for registering with edX. goals_label = _( @@ -454,6 +530,15 @@ class RegistrationView(APIView): ) def _add_city_field(self, form_desc, required=True): + """Add a city field to a form description. + + Arguments: + form_desc: A form description + + Keyword Arguments: + required (Boolean): Whether this field is required; defaults to True + + """ # Translators: This label appears above a field on the registration form # which allows the user to input the city in which they live. city_label = _(u"City") @@ -465,6 +550,15 @@ class RegistrationView(APIView): ) def _add_country_field(self, form_desc, required=True): + """Add a country field to a form description. + + Arguments: + form_desc: A form description + + Keyword Arguments: + required (Boolean): Whether this field is required; defaults to True + + """ # Translators: This label appears above a dropdown menu on the registration # form used to select the country in which the user lives. country_label = _(u"Country") @@ -486,6 +580,15 @@ class RegistrationView(APIView): ) def _add_honor_code_field(self, form_desc, required=True): + """Add an honor code field to a form description. + + Arguments: + form_desc: A form description + + Keyword Arguments: + required (Boolean): Whether this field is required; defaults to True + + """ # Separate terms of service and honor code checkboxes if self._is_field_visible("terms_of_service"): terms_text = _(u"Honor Code") @@ -531,6 +634,15 @@ class RegistrationView(APIView): ) def _add_terms_of_service_field(self, form_desc, required=True): + """Add a terms of service field to a form description. + + Arguments: + form_desc: A form description + + Keyword Arguments: + required (Boolean): Whether this field is required; defaults to True + + """ # Translators: This is a legal document users must agree to # in order to register a new account. terms_text = _(u"Terms of Service") @@ -615,6 +727,7 @@ class RegistrationView(APIView): restrictions={} ) + class PasswordResetView(APIView): """HTTP end-point for GETting a description of the password reset form. """ @@ -622,7 +735,7 @@ class PasswordResetView(APIView): # so do not require authentication. authentication_classes = [] - def get(self, request): + def get(self, request): # pylint: disable=unused-argument """Return a description of the password reset form. This decouples clients from the API definition: @@ -632,9 +745,6 @@ class PasswordResetView(APIView): See `user_api.helpers.FormDescription` for examples of the JSON-encoded form description. - Arguments: - request (HttpRequest) - Returns: HttpResponse diff --git a/common/djangoapps/util/testing.py b/common/djangoapps/util/testing.py index 311ed89244..ccfd29e91d 100644 --- a/common/djangoapps/util/testing.py +++ b/common/djangoapps/util/testing.py @@ -20,6 +20,7 @@ class UrlResetMixin(object): """ def _reset_urls(self, urlconf_modules): + """Reset `urls.py` for a set of Django apps.""" for urlconf in urlconf_modules: if urlconf in sys.modules: reload(sys.modules[urlconf]) diff --git a/lms/djangoapps/student_account/helpers.py b/lms/djangoapps/student_account/helpers.py index 7683791a3b..cd2e8d947f 100644 --- a/lms/djangoapps/student_account/helpers.py +++ b/lms/djangoapps/student_account/helpers.py @@ -1,4 +1,4 @@ """Helper functions for the student account app. """ -# TODO: move this function here instead of importing it from student -from student.helpers import auth_pipeline_urls +# TODO: move this function here instead of importing it from student # pylint: disable=fixme +from student.helpers import auth_pipeline_urls # pylint: disable=unused-import From 3eb287151cc9cb534d86872f8ef636c58e6aaf12 Mon Sep 17 00:00:00 2001 From: AlasdairSwan Date: Wed, 12 Nov 2014 13:40:57 -0500 Subject: [PATCH 87/97] ECOM-369 accessibility updates --- common/static/js/utils/edx.utils.validate.js | 7 +++++ .../js/student_account/views/FormView.js | 29 +++++++++++++++++-- .../js/student_account/views/RegisterView.js | 2 ++ .../student_account/form_field.underscore | 15 ++++++---- .../student_account/login.underscore | 2 +- .../student_account/password_reset.underscore | 2 +- .../student_account/register.underscore | 5 ++-- 7 files changed, 49 insertions(+), 13 deletions(-) diff --git a/common/static/js/utils/edx.utils.validate.js b/common/static/js/utils/edx.utils.validate.js index 60366c6630..de3281de6a 100644 --- a/common/static/js/utils/edx.utils.validate.js +++ b/common/static/js/utils/edx.utils.validate.js @@ -51,6 +51,8 @@ var edx = edx || {}; response.isValid = required && min && max && email; if ( !response.isValid ) { + _fn.validate.removeDefault( $el ); + response.message = _fn.validate.getMessage( $el, { required: required, min: min, @@ -162,6 +164,11 @@ var edx = edx || {}; }); return txt.join(' '); + }, + + // Removes the default HTML5 validation pop-up + removeDefault: function( $el ) { + $el.setCustomValidity(' '); } } }; diff --git a/lms/static/js/student_account/views/FormView.js b/lms/static/js/student_account/views/FormView.js index 4442d1f531..e8dcc768a9 100644 --- a/lms/static/js/student_account/views/FormView.js +++ b/lms/static/js/student_account/views/FormView.js @@ -28,6 +28,9 @@ var edx = edx || {}; // String to append to required label fields requiredStr: '*', + // Id of required footnote + requiredNote: 'register-footnote', + initialize: function( data ) { this.model = data.model; this.preRender( data ); @@ -71,7 +74,8 @@ var edx = edx || {}; var html = [], i, len = data.length, - fieldTpl = this.fieldTpl; + fieldTpl = this.fieldTpl, + requiredNote = ''; this.fields = data; @@ -80,9 +84,12 @@ var edx = edx || {}; data[i].errorMessages = this.escapeStrings( data[i].errorMessages ); } + requiredNote = data[i].required ? this.requiredNote : ''; + html.push( _.template( fieldTpl, $.extend( data[i], { form: this.formType, - requiredStr: this.requiredStr + requiredStr: this.requiredStr, + requiredNote: requiredNote }) ) ); } @@ -116,6 +123,21 @@ var edx = edx || {}; return obj; }, + focusFirstError: function() { + var $error = this.$form.find('.error').first(), + $field = {}, + $parent = {}; + + if ( $error.is('label') ) { + $parent = $error.parent('.form-field'); + $error = $parent.find('input') || $parent.find('select'); + } else { + $field = $error; + } + + $error.focus(); + }, + forgotPassword: function( event ) { event.preventDefault(); @@ -184,6 +206,9 @@ var edx = edx || {}; $('html,body').animate({ scrollTop: this.$errors.offset().top },'slow'); + + // Focus on first error field + this.focusFirstError(); }, submitForm: function( event ) { diff --git a/lms/static/js/student_account/views/RegisterView.js b/lms/static/js/student_account/views/RegisterView.js index fe8d28c013..6f20acaad5 100644 --- a/lms/static/js/student_account/views/RegisterView.js +++ b/lms/static/js/student_account/views/RegisterView.js @@ -18,6 +18,8 @@ var edx = edx || {}; formType: 'register', + requiredNote: 'register-footnote', + preRender: function( data ) { this.providers = data.thirdPartyAuth.providers || []; this.currentProvider = data.thirdPartyAuth.currentProvider || ''; diff --git a/lms/templates/student_account/form_field.underscore b/lms/templates/student_account/form_field.underscore index 46b13dd24d..42925b2ad6 100644 --- a/lms/templates/student_account/form_field.underscore +++ b/lms/templates/student_account/form_field.underscore @@ -10,8 +10,9 @@ minlength="<%= restrictions.min_length %>"<% } %> <% if ( restrictions.max_length ) { %> maxlength="<%= restrictions.max_length %>"<% } %> - <% if ( required ) { %> required<% } %> + <% if ( required ) { %> aria-required="true" required<% } %> <% if ( typeof errorMessages !== 'undefined' ) { _.each(errorMessages, function( msg, type ) {%> data-errormsg-<%= type %>="<%= msg %>" diff --git a/lms/templates/student_account/login.underscore b/lms/templates/student_account/login.underscore index 6cb07529ea..9e0b54b4d6 100644 --- a/lms/templates/student_account/login.underscore +++ b/lms/templates/student_account/login.underscore @@ -7,7 +7,7 @@ <% } %> -