From acc0161c0f67f4bbed670e6c18f7a50745116b89 Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Mon, 16 Nov 2015 23:23:41 +0000 Subject: [PATCH] Add custom registration form extension feature. --- common/djangoapps/student/forms.py | 17 +++ common/djangoapps/student/views.py | 23 +++- .../pages/lms/login_and_register.py | 7 +- common/test/acceptance/tests/lms/test_lms.py | 5 +- lms/envs/aws.py | 1 + lms/envs/bok_choy.env.json | 1 + lms/envs/common.py | 11 ++ openedx/core/djangoapps/user_api/helpers.py | 38 +++++- .../djangoapps/user_api/tests/test_helpers.py | 60 +++++++++ .../djangoapps/user_api/tests/test_views.py | 123 ++++++++++++++++-- openedx/core/djangoapps/user_api/views.py | 33 +++++ 11 files changed, 295 insertions(+), 24 deletions(-) diff --git a/common/djangoapps/student/forms.py b/common/djangoapps/student/forms.py index bf85d55a21..41859bcdf0 100644 --- a/common/djangoapps/student/forms.py +++ b/common/djangoapps/student/forms.py @@ -1,6 +1,8 @@ """ Utility functions for validating forms """ +from importlib import import_module + from django import forms from django.forms import widgets from django.core.exceptions import ValidationError @@ -246,3 +248,18 @@ class AccountCreationForm(forms.Form): for key, value in self.cleaned_data.items() if key in self.extended_profile_fields and value is not None } + + +def get_registration_extension_form(*args, **kwargs): + """ + Convenience function for getting the custom form set in settings.REGISTRATION_EXTENSION_FORM. + + An example form app for this can be found at http://github.com/open-craft/custom-form-app + """ + if not settings.FEATURES.get("ENABLE_COMBINED_LOGIN_REGISTRATION"): + return None + if not getattr(settings, 'REGISTRATION_EXTENSION_FORM', None): + return None + module, klass = settings.REGISTRATION_EXTENSION_FORM.rsplit('.', 1) + module = import_module(module) + return getattr(module, klass)(*args, **kwargs) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 4083954f77..c5510b2bbb 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -54,9 +54,8 @@ from student.models import ( CourseEnrollmentAllowed, UserStanding, LoginFailures, create_comments_service_user, PasswordHistory, UserSignupSource, DashboardConfiguration, LinkedInAddToProfileConfiguration, ManualEnrollmentAudit, ALLOWEDTOENROLL_TO_ENROLLED) -from student.forms import AccountCreationForm, PasswordResetFormNoActive - -from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification +from student.forms import AccountCreationForm, PasswordResetFormNoActive, get_registration_extension_form +from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification # pylint: disable=import-error from certificates.models import CertificateStatuses, certificate_status_for_student from certificates.api import ( # pylint: disable=import-error get_certificate_url, @@ -1439,7 +1438,7 @@ def user_signup_handler(sender, **kwargs): # pylint: disable=unused-argument log.info(u'user {} originated from a white labeled "Microsite"'.format(kwargs['instance'].id)) -def _do_create_account(form): +def _do_create_account(form, custom_form=None): """ Given cleaned post variables, create the User and UserProfile objects, as well as the registration for this user. @@ -1448,8 +1447,13 @@ def _do_create_account(form): Note: this function is also used for creating test users. """ - if not form.is_valid(): - raise ValidationError(form.errors) + errors = {} + errors.update(form.errors) + if custom_form: + errors.update(custom_form.errors) + + if errors: + raise ValidationError(errors) user = User( username=form.cleaned_data["username"], @@ -1464,6 +1468,10 @@ def _do_create_account(form): try: with transaction.atomic(): user.save() + if custom_form: + custom_model = custom_form.save(commit=False) + custom_model.user = user + custom_model.save() except IntegrityError: # Figure out the cause of the integrity error if len(User.objects.filter(username=user.username)) > 0: @@ -1593,11 +1601,12 @@ def create_account_with_params(request, params): enforce_password_policy=enforce_password_policy, tos_required=tos_required, ) + custom_form = get_registration_extension_form(data=params) # Perform operations within a transaction that are critical to account creation with transaction.atomic(): # first, create the account - (user, profile, registration) = _do_create_account(form) + (user, profile, registration) = _do_create_account(form, custom_form) # next, link the account with social auth, if provided via the API. # (If the user is using the normal register page, the social auth pipeline does the linking, not this code) diff --git a/common/test/acceptance/pages/lms/login_and_register.py b/common/test/acceptance/pages/lms/login_and_register.py index 2794b3584a..d187ee8a94 100644 --- a/common/test/acceptance/pages/lms/login_and_register.py +++ b/common/test/acceptance/pages/lms/login_and_register.py @@ -168,7 +168,10 @@ class CombinedLoginAndRegisterPage(PageObject): "Finish toggling to the other form" ).fulfill() - def register(self, email="", password="", username="", full_name="", country="", terms_of_service=False): + def register( + self, email="", password="", username="", full_name="", country="", favorite_movie="", + terms_of_service=False + ): """Fills in and submits the registration form. Requires that the "register" form is visible. @@ -197,6 +200,8 @@ class CombinedLoginAndRegisterPage(PageObject): self.q(css="#register-password").fill(password) if country: self.q(css="#register-country option[value='{country}']".format(country=country)).click() + if favorite_movie: + self.q(css="#register-favorite_movie").fill(favorite_movie) if terms_of_service: self.q(css="#register-honor_code").click() diff --git a/common/test/acceptance/tests/lms/test_lms.py b/common/test/acceptance/tests/lms/test_lms.py index 6165a59270..1c4251d339 100644 --- a/common/test/acceptance/tests/lms/test_lms.py +++ b/common/test/acceptance/tests/lms/test_lms.py @@ -262,6 +262,7 @@ class RegisterFromCombinedPageTest(UniqueCourseTest): username=username, full_name="Test User", country="US", + favorite_movie="Mad Max: Fury Road", terms_of_service=True ) @@ -276,6 +277,7 @@ class RegisterFromCombinedPageTest(UniqueCourseTest): # Enter a blank for the username field, which is required # Don't agree to the terms of service / honor code. # Don't specify a country code, which is required. + # Don't specify a favorite movie. username = "test_{uuid}".format(uuid=self.unique_id[0:6]) email = "{user}@example.com".format(user=username) self.register_page.register( @@ -291,6 +293,7 @@ class RegisterFromCombinedPageTest(UniqueCourseTest): self.assertIn(u'Please enter your Public username.', errors) self.assertIn(u'You must agree to the edX Terms of Service and Honor Code.', errors) self.assertIn(u'Please select your Country.', errors) + self.assertIn(u'Please tell us your favorite movie.', errors) def test_toggle_to_login_form(self): self.register_page.visit().toggle_form() @@ -317,7 +320,7 @@ class RegisterFromCombinedPageTest(UniqueCourseTest): self.assertIn("Galactica1", self.register_page.username_value) # Set country, accept the terms, and submit the form: - self.register_page.register(country="US", terms_of_service=True) + self.register_page.register(country="US", favorite_movie="Battlestar Galactica", terms_of_service=True) # Expect that we reach the dashboard and we're auto-enrolled in the course course_names = self.dashboard_page.wait_for_page().available_courses diff --git a/lms/envs/aws.py b/lms/envs/aws.py index 4918a21404..192163012b 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -158,6 +158,7 @@ SESSION_COOKIE_SECURE = ENV_TOKENS.get('SESSION_COOKIE_SECURE', SESSION_COOKIE_S SESSION_SAVE_EVERY_REQUEST = ENV_TOKENS.get('SESSION_SAVE_EVERY_REQUEST', SESSION_SAVE_EVERY_REQUEST) REGISTRATION_EXTRA_FIELDS = ENV_TOKENS.get('REGISTRATION_EXTRA_FIELDS', REGISTRATION_EXTRA_FIELDS) +REGISTRATION_EXTENSION_FORM = ENV_TOKENS.get('REGISTRATION_EXTENSION_FORM', REGISTRATION_EXTENSION_FORM) # Set the names of cookies shared with the marketing site # These have the same cookie domain as the session, which in production diff --git a/lms/envs/bok_choy.env.json b/lms/envs/bok_choy.env.json index feb29527af..da32851348 100644 --- a/lms/envs/bok_choy.env.json +++ b/lms/envs/bok_choy.env.json @@ -99,6 +99,7 @@ "MEDIA_URL": "", "MKTG_URL_LINK_MAP": {}, "PLATFORM_NAME": "edX", + "REGISTRATION_EXTENSION_FORM": "openedx.core.djangoapps.user_api.tests.test_helpers.TestCaseForm", "REGISTRATION_EXTRA_FIELDS": { "level_of_education": "optional", "gender": "optional", diff --git a/lms/envs/common.py b/lms/envs/common.py index f59b9d66f7..088779a71e 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2662,3 +2662,14 @@ FINANCIAL_ASSISTANCE_MAX_LENGTH = 2500 # Course Content Bookmarks Settings MAX_BOOKMARKS_PER_COURSE = 100 + +#### Registration form extension. #### +# Only used if combined login/registration is enabled. +# This can be used to add fields to the registration page. +# It must be a path to a valid form, in dot-separated syntax. +# IE: custom_form_app.forms.RegistrationExtensionForm +# Note: If you want to use a model to store the results of the form, you will +# need to add the model's app to the ADDL_INSTALLED_APPS array in your +# lms.env.json file. + +REGISTRATION_EXTENSION_FORM = None diff --git a/openedx/core/djangoapps/user_api/helpers.py b/openedx/core/djangoapps/user_api/helpers.py index 296ddece5b..ff59f1410b 100644 --- a/openedx/core/djangoapps/user_api/helpers.py +++ b/openedx/core/djangoapps/user_api/helpers.py @@ -6,8 +6,12 @@ from collections import defaultdict from functools import wraps import logging import json -from django.http import HttpResponseBadRequest +from django import forms +from django.core.serializers.json import DjangoJSONEncoder +from django.http import HttpResponseBadRequest +from django.utils.encoding import force_text +from django.utils.functional import Promise LOGGER = logging.getLogger(__name__) @@ -121,6 +125,16 @@ class FormDescription(object): "email": ["min_length", "max_length"], } + FIELD_TYPE_MAP = { + forms.CharField: "text", + forms.PasswordInput: "password", + forms.ChoiceField: "select", + forms.TypedChoiceField: "select", + forms.Textarea: "textarea", + forms.BooleanField: "checkbox", + forms.EmailField: "email", + } + OVERRIDE_FIELD_PROPERTIES = [ "label", "type", "defaultValue", "placeholder", "instructions", "required", "restrictions", @@ -141,9 +155,9 @@ class FormDescription(object): self._field_overrides = defaultdict(dict) def add_field( - self, name, label=u"", field_type=u"text", default=u"", - placeholder=u"", instructions=u"", required=True, restrictions=None, - options=None, include_default_option=False, error_messages=None + self, name, label=u"", field_type=u"text", default=u"", + placeholder=u"", instructions=u"", required=True, restrictions=None, + options=None, include_default_option=False, error_messages=None, ): """Add a field to the form description. @@ -297,7 +311,7 @@ class FormDescription(object): "method": self.method, "submit_url": self.submit_url, "fields": self.fields - }) + }, cls=LocalizedJSONEncoder) def override_field_properties(self, field_name, **kwargs): """Override properties of a field. @@ -330,6 +344,20 @@ class FormDescription(object): }) +class LocalizedJSONEncoder(DjangoJSONEncoder): + """ + JSON handler that evaluates ugettext_lazy promises. + """ + # pylint: disable=method-hidden + def default(self, obj): + """ + Forces evaluation of ugettext_lazy promises. + """ + if isinstance(obj, Promise): + return force_text(obj) + super(LocalizedJSONEncoder, self).default(obj) + + def shim_student_view(view_func, check_logged_in=False): """Create a "shim" view for a view function from the student Django app. diff --git a/openedx/core/djangoapps/user_api/tests/test_helpers.py b/openedx/core/djangoapps/user_api/tests/test_helpers.py index 0d36d995e1..ab71ae379f 100644 --- a/openedx/core/djangoapps/user_api/tests/test_helpers.py +++ b/openedx/core/djangoapps/user_api/tests/test_helpers.py @@ -4,6 +4,7 @@ Tests for helper functions. import json import mock import ddt +from django import forms from django.http import HttpRequest, HttpResponse from django.test import TestCase from nose.tools import raises @@ -214,3 +215,62 @@ class StudentViewShimTest(TestCase): self.captured_request = request return response return shim_student_view(stub_view, check_logged_in=check_logged_in) + + +class DummyRegistrationExtensionModel(object): + """ + Dummy registration object + """ + user = None + + def save(self): + """ + Dummy save method for dummy model. + """ + return None + + +class TestCaseForm(forms.Form): + """ + Test registration extension form. + """ + DUMMY_STORAGE = {} + + MOVIE_MIN_LEN = 3 + MOVIE_MAX_LEN = 100 + + FAVORITE_EDITOR = ( + ('vim', 'Vim'), + ('emacs', 'Emacs'), + ('np', 'Notepad'), + ('cat', 'cat > filename') + ) + + favorite_movie = forms.CharField( + label="Fav Flick", min_length=MOVIE_MIN_LEN, max_length=MOVIE_MAX_LEN, error_messages={ + "required": u"Please tell us your favorite movie.", + "invalid": u"We're pretty sure you made that movie up." + } + ) + favorite_editor = forms.ChoiceField(label="Favorite Editor", choices=FAVORITE_EDITOR, required=False, initial='cat') + + def save(self, commit=None): # pylint: disable=unused-argument + """ + Store the result in the dummy storage dict. + """ + self.DUMMY_STORAGE.update({ + 'favorite_movie': self.cleaned_data.get('favorite_movie'), + 'favorite_editor': self.cleaned_data.get('favorite_editor'), + }) + dummy_model = DummyRegistrationExtensionModel() + return dummy_model + + class Meta(object): + """ + Set options for fields which can't be conveyed in their definition. + """ + serialization_options = { + 'favorite_editor': { + 'default': 'vim', + }, + } diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py index bad05115da..490e675cf8 100644 --- a/openedx/core/djangoapps/user_api/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/tests/test_views.py @@ -30,6 +30,7 @@ from third_party_auth.tests.testutil import simulate_running_pipeline, ThirdPart from third_party_auth.tests.utils import ( ThirdPartyOAuthTestMixin, ThirdPartyOAuthTestMixinFacebook, ThirdPartyOAuthTestMixinGoogle ) +from .test_helpers import TestCaseForm from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from ..accounts.api import get_account_settings @@ -932,6 +933,62 @@ class RegistrationViewTest(ThirdPartyAuthTestMixin, ApiTestCase): } ) + @override_settings(REGISTRATION_EXTENSION_FORM='openedx.core.djangoapps.user_api.tests.test_helpers.TestCaseForm') + def test_extension_form_fields(self): + no_extra_fields_setting = {} + + # Verify other fields didn't disappear for some reason. + self._assert_reg_field( + no_extra_fields_setting, + { + u"name": u"email", + u"type": u"email", + u"required": True, + u"label": u"Email", + u"placeholder": u"username@domain.com", + u"restrictions": { + "min_length": EMAIL_MIN_LENGTH, + "max_length": EMAIL_MAX_LENGTH + }, + } + ) + + self._assert_reg_field( + no_extra_fields_setting, + { + u"name": u"favorite_editor", + u"type": u"select", + u"required": False, + u"label": u"Favorite Editor", + u"placeholder": u"cat", + u"defaultValue": u"vim", + u"errorMessages": { + u'required': u'This field is required.', + u'invalid_choice': u'Select a valid choice. %(value)s is not one of the available choices.', + } + } + ) + + self._assert_reg_field( + no_extra_fields_setting, + { + u"name": u"favorite_movie", + u"type": u"text", + u"required": True, + u"label": u"Fav Flick", + u"placeholder": None, + u"defaultValue": None, + u"errorMessages": { + u'required': u'Please tell us your favorite movie.', + u'invalid': u"We're pretty sure you made that movie up." + }, + u"restrictions": { + "min_length": TestCaseForm.MOVIE_MIN_LEN, + "max_length": TestCaseForm.MOVIE_MAX_LEN, + } + } + ) + def test_register_form_third_party_auth_running(self): no_extra_fields_setting = {} @@ -1309,16 +1366,19 @@ class RegistrationViewTest(ThirdPartyAuthTestMixin, ApiTestCase): } ) - @override_settings(REGISTRATION_EXTRA_FIELDS={ - "level_of_education": "optional", - "gender": "optional", - "year_of_birth": "optional", - "mailing_address": "optional", - "goals": "optional", - "city": "optional", - "country": "required", - "honor_code": "required", - }) + @override_settings( + REGISTRATION_EXTRA_FIELDS={ + "level_of_education": "optional", + "gender": "optional", + "year_of_birth": "optional", + "mailing_address": "optional", + "goals": "optional", + "city": "optional", + "country": "required", + "honor_code": "required", + }, + REGISTRATION_EXTENSION_FORM='openedx.core.djangoapps.user_api.tests.test_helpers.TestCaseForm', + ) def test_field_order(self): response = self.client.get(self.url) self.assertHttpOK(response) @@ -1331,6 +1391,8 @@ class RegistrationViewTest(ThirdPartyAuthTestMixin, ApiTestCase): "name", "username", "password", + "favorite_movie", + "favorite_editor", "city", "country", "gender", @@ -1405,6 +1467,47 @@ class RegistrationViewTest(ThirdPartyAuthTestMixin, ApiTestCase): self.assertEqual(account_settings["goals"], self.GOALS) self.assertEqual(account_settings["country"], self.COUNTRY) + @override_settings(REGISTRATION_EXTENSION_FORM='openedx.core.djangoapps.user_api.tests.test_helpers.TestCaseForm') + @mock.patch('openedx.core.djangoapps.user_api.tests.test_helpers.TestCaseForm.DUMMY_STORAGE', new_callable=dict) + @mock.patch( + 'openedx.core.djangoapps.user_api.tests.test_helpers.DummyRegistrationExtensionModel', + ) + def test_with_extended_form(self, dummy_model, storage_dict): + dummy_model_instance = mock.Mock() + dummy_model.return_value = dummy_model_instance + # Create a new registration + self.assertEqual(storage_dict, {}) + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": self.NAME, + "username": self.USERNAME, + "password": self.PASSWORD, + "honor_code": "true", + "favorite_movie": "Inception", + "favorite_editor": "cat", + }) + self.assertHttpOK(response) + self.assertIn(settings.EDXMKTG_LOGGED_IN_COOKIE_NAME, self.client.cookies) + self.assertIn(settings.EDXMKTG_USER_INFO_COOKIE_NAME, self.client.cookies) + + user = User.objects.get(username=self.USERNAME) + request = RequestFactory().get('/url') + request.user = user + account_settings = get_account_settings(request) + + self.assertEqual(self.USERNAME, account_settings["username"]) + self.assertEqual(self.EMAIL, account_settings["email"]) + self.assertFalse(account_settings["is_active"]) + self.assertEqual(self.NAME, account_settings["name"]) + + self.assertEqual(storage_dict, {'favorite_movie': "Inception", "favorite_editor": "cat"}) + self.assertEqual(dummy_model_instance.user, user) + + # 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) + def test_activation_email(self): # Register, which should trigger an activation email response = self.client.post(self.url, { diff --git a/openedx/core/djangoapps/user_api/views.py b/openedx/core/djangoapps/user_api/views.py index cc7669a222..e0d027392a 100644 --- a/openedx/core/djangoapps/user_api/views.py +++ b/openedx/core/djangoapps/user_api/views.py @@ -1,5 +1,6 @@ """HTTP end-points for the User API. """ import copy + from opaque_keys import InvalidKeyError from django.conf import settings from django.contrib.auth.models import User @@ -24,6 +25,7 @@ from openedx.core.lib.api.permissions import ApiKeyHeaderPermission import third_party_auth from django_comment_common.models import Role from edxmako.shortcuts import marketing_link +from student.forms import get_registration_extension_form from student.views import create_account_with_params from student.cookies import set_logged_in_cookies from openedx.core.lib.api.authentication import SessionAuthenticationAllowInactiveUser @@ -229,6 +231,37 @@ class RegistrationView(APIView): for field_name in self.DEFAULT_FIELDS: self.field_handlers[field_name](form_desc, required=True) + # Custom form fields can be added via the form set in settings.REGISTRATION_EXTENSION_FORM + custom_form = get_registration_extension_form() + + if custom_form: + for field_name, field in custom_form.fields.items(): + restrictions = {} + if getattr(field, 'max_length', None): + restrictions['max_length'] = field.max_length + if getattr(field, 'min_length', None): + restrictions['min_length'] = field.min_length + field_options = getattr( + getattr(custom_form, 'Meta', None), 'serialization_options', {} + ).get(field_name, {}) + field_type = field_options.get('field_type', FormDescription.FIELD_TYPE_MAP.get(field.__class__)) + if not field_type: + raise ImproperlyConfigured( + "Field type '{}' not recognized for registration extension field '{}'.".format( + field_type, + field_name + ) + ) + form_desc.add_field( + field_name, label=field.label, + default=field_options.get('default'), + field_type=field_options.get('field_type', FormDescription.FIELD_TYPE_MAP.get(field.__class__)), + placeholder=field.initial, instructions=field.help_text, required=field.required, + restrictions=restrictions, + options=getattr(field, 'choices', None), error_messages=field.error_messages, + include_default_option=field_options.get('include_default_option'), + ) + # Extra fields configured in Django settings # may be required, optional, or hidden for field_name in self.EXTRA_FIELDS: