diff --git a/common/djangoapps/external_auth/login_and_register.py b/common/djangoapps/external_auth/login_and_register.py deleted file mode 100644 index 997b2deb70..0000000000 --- a/common/djangoapps/external_auth/login_and_register.py +++ /dev/null @@ -1,92 +0,0 @@ -"""Intercept login and registration requests. - -This module contains legacy code originally from `student.views`. -""" -import re - -from django.conf import settings -from django.shortcuts import redirect -from django.core.urlresolvers import reverse -import external_auth.views - -from xmodule.modulestore.django import modulestore -from opaque_keys.edx.locations import SlashSeparatedCourseKey - - -# pylint: disable=fixme -# TODO: This function is kind of gnarly/hackish/etc and is only used in one location. -# It'd be awesome if we could get rid of it; manually parsing course_id strings form larger strings -# seems Probably Incorrect -def _parse_course_id_from_string(input_str): - """ - Helper function to determine if input_str (typically the queryparam 'next') contains a course_id. - @param input_str: - @return: the course_id if found, None if not - """ - m_obj = re.match(r'^/courses/{}'.format(settings.COURSE_ID_PATTERN), input_str) - if m_obj: - return SlashSeparatedCourseKey.from_deprecated_string(m_obj.group('course_id')) - return None - - -def _get_course_enrollment_domain(course_id): - """ - Helper function to get the enrollment domain set for a course with id course_id - @param course_id: - @return: - """ - course = modulestore().get_course(course_id) - if course is None: - return None - - return course.enrollment_domain - - -def login(request): - """Allow external auth to intercept and handle a login request. - - Arguments: - request (Request): A request for the login page. - - Returns: - Response or None - - """ - # Default to a `None` response, indicating that external auth - # is not handling the request. - response = None - - if settings.FEATURES['AUTH_USE_CERTIFICATES'] and external_auth.views.ssl_get_cert_from_request(request): - # SSL login doesn't require a view, so redirect - # branding and allow that to process the login if it - # is enabled and the header is in the request. - response = external_auth.views.redirect_with_get('root', request.GET) - elif settings.FEATURES.get('AUTH_USE_CAS'): - # If CAS is enabled, redirect auth handling to there - response = redirect(reverse('cas-login')) - elif settings.FEATURES.get('AUTH_USE_SHIB'): - redirect_to = request.GET.get('next') - if redirect_to: - course_id = _parse_course_id_from_string(redirect_to) - if course_id and _get_course_enrollment_domain(course_id): - response = external_auth.views.course_specific_login(request, course_id.to_deprecated_string()) - - return response - - -def register(request): - """Allow external auth to intercept and handle a registration request. - - Arguments: - request (Request): A request for the registration page. - - Returns: - Response or None - - """ - response = None - if settings.FEATURES.get('AUTH_USE_CERTIFICATES_IMMEDIATE_SIGNUP'): - # Redirect to branding to process their certificate if SSL is enabled - # and registration is disabled. - response = external_auth.views.redirect_with_get('root', request.GET) - return response diff --git a/common/djangoapps/external_auth/tests/test_ssl.py b/common/djangoapps/external_auth/tests/test_ssl.py index ba47942b6e..18cee1e35b 100644 --- a/common/djangoapps/external_auth/tests/test_ssl.py +++ b/common/djangoapps/external_auth/tests/test_ssl.py @@ -220,7 +220,8 @@ class SSLClientTest(ModuleStoreTestCase): # Test that they do signin if they don't have a cert response = self.client.get(reverse('signin_user')) self.assertEqual(200, response.status_code) - self.assertTrue('login-and-registration-container' in response.content) + self.assertTrue('login_form' in response.content + or 'login-form' in response.content) # And get directly logged in otherwise response = self.client.get( diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index 4c7e2c5e58..d874595af9 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -4,10 +4,78 @@ from datetime import datetime from pytz import UTC from django.utils.http import cookie_date from django.conf import settings - -from verify_student.models import SoftwareSecurePhotoVerification # pylint: disable=F0401 +from django.core.urlresolvers import reverse +from opaque_keys.edx.keys import CourseKey from course_modes.models import CourseMode -from student_account.helpers import auth_pipeline_urls # pylint: disable=unused-import,import-error +from third_party_auth import ( # pylint: disable=unused-import + pipeline, provider, + is_enabled as third_party_auth_enabled +) +from verify_student.models import SoftwareSecurePhotoVerification # pylint: disable=F0401 + + +def auth_pipeline_urls(auth_entry, redirect_url=None, course_id=None): + """Retrieve URLs for each enabled third-party auth provider. + + These URLs are used on the "sign up" and "sign in" buttons + on the login/registration forms to allow users to begin + authentication with a third-party provider. + + Optionally, we can redirect the user to an arbitrary + url after auth completes successfully. We use this + to redirect the user to a page that required login, + or to send users to the payment flow when enrolling + in a course. + + Args: + auth_entry (string): Either `pipeline.AUTH_ENTRY_LOGIN` or `pipeline.AUTH_ENTRY_REGISTER` + + Keyword Args: + redirect_url (unicode): If provided, send users to this URL + after they successfully authenticate. + + course_id (unicode): The ID of the course the user is enrolling in. + We use this to send users to the track selection page + if the course has a payment option. + Note that `redirect_url` takes precedence over the redirect + to the track selection page. + + Returns: + dict mapping provider names to URLs + + """ + if not third_party_auth_enabled(): + return {} + + if redirect_url is not None: + pipeline_redirect = redirect_url + elif course_id is not None: + # If the course is white-label (paid), then we send users + # to the shopping cart. (There is a third party auth pipeline + # step that will add the course to the cart.) + if CourseMode.is_white_label(CourseKey.from_string(course_id)): + pipeline_redirect = reverse("shoppingcart.views.show_cart") + + # Otherwise, send the user to the track selection page. + # The track selection page may redirect the user to the dashboard + # (if the only available mode is honor), or directly to verification + # (for professional ed). + else: + pipeline_redirect = reverse( + "course_modes_choose", + kwargs={'course_id': unicode(course_id)} + ) + else: + pipeline_redirect = None + + return { + provider.NAME: pipeline.get_login_url( + provider.NAME, auth_entry, + enroll_course_id=course_id, + redirect_url=pipeline_redirect + ) + for provider in provider.Registry.enabled() + } def set_logged_in_cookie(request, response): diff --git a/common/djangoapps/student/tests/test_login.py b/common/djangoapps/student/tests/test_login.py index c205f056c8..828e951cbb 100644 --- a/common/djangoapps/student/tests/test_login.py +++ b/common/djangoapps/student/tests/test_login.py @@ -14,13 +14,18 @@ from django.http import HttpResponseBadRequest, HttpResponse from external_auth.models import ExternalAuthMap import httpretty from mock import patch +from opaque_keys.edx.locations import SlashSeparatedCourseKey from social.apps.django_app.default.models import UserSocialAuth +from xmodule.modulestore.tests.django_utils import TEST_DATA_MOCK_MODULESTORE from student.tests.factories import UserFactory, RegistrationFactory, UserProfileFactory -from student.views import login_oauth_token - +from student.views import ( + _parse_course_id_from_string, + _get_course_enrollment_domain, + login_oauth_token, +) from xmodule.modulestore.tests.factories import CourseFactory -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, TEST_DATA_MOCK_MODULESTORE +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase class LoginTest(TestCase): @@ -319,6 +324,24 @@ class LoginTest(TestCase): self.assertNotIn(log_string, format_string) +class UtilFnTest(TestCase): + """ + Tests for utility functions in student.views + """ + def test__parse_course_id_from_string(self): + """ + Tests the _parse_course_id_from_string util function + """ + COURSE_ID = u'org/num/run' # pylint: disable=invalid-name + COURSE_URL = u'/courses/{}/otherstuff'.format(COURSE_ID) # pylint: disable=invalid-name + NON_COURSE_URL = u'/blahblah' # pylint: disable=invalid-name + self.assertEqual( + _parse_course_id_from_string(COURSE_URL), + SlashSeparatedCourseKey.from_deprecated_string(COURSE_ID) + ) + self.assertIsNone(_parse_course_id_from_string(NON_COURSE_URL)) + + @override_settings(MODULESTORE=TEST_DATA_MOCK_MODULESTORE) class ExternalAuthShibTest(ModuleStoreTestCase): """ @@ -364,6 +387,15 @@ class ExternalAuthShibTest(ModuleStoreTestCase): 'redirect': reverse('shib-login'), }) + @unittest.skipUnless(settings.FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set") + def test__get_course_enrollment_domain(self): + """ + Tests the _get_course_enrollment_domain utility function + """ + self.assertIsNone(_get_course_enrollment_domain(SlashSeparatedCourseKey("I", "DONT", "EXIST"))) + self.assertIsNone(_get_course_enrollment_domain(self.course.id)) + self.assertEqual(self.shib_course.enrollment_domain, _get_course_enrollment_domain(self.shib_course.id)) + @unittest.skipUnless(settings.FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set") def test_login_required_dashboard(self): """ @@ -384,7 +416,7 @@ class ExternalAuthShibTest(ModuleStoreTestCase): noshib_response = self.client.get(TARGET_URL, follow=True) self.assertEqual(noshib_response.redirect_chain[-1], ('http://testserver/accounts/login?next={url}'.format(url=TARGET_URL), 302)) - self.assertContains(noshib_response, ("Log in or Register | {platform_name}" + self.assertContains(noshib_response, ("Log into your {platform_name} Account | {platform_name}" .format(platform_name=settings.PLATFORM_NAME))) self.assertEqual(noshib_response.status_code, 200) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 43986e320f..64faae90b1 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -75,10 +75,6 @@ from django_comment_common.models import Role from external_auth.models import ExternalAuthMap import external_auth.views -from external_auth.login_and_register import ( - login as external_auth_login, - register as external_auth_register -) from bulk_email.models import Optout, CourseAuthorization import shoppingcart @@ -352,14 +348,18 @@ def _cert_info(user, course, cert_status): @ensure_csrf_cookie def signin_user(request): - """This view will display the non-modal login form - - DEPRECATION WARNING: This view will eventually be deprecated and replaced - with the combined login/registration page in `student_account.views`. """ - external_auth_response = external_auth_login(request) - if external_auth_response is not None: - return external_auth_response + This view will display the non-modal login form + """ + if (settings.FEATURES['AUTH_USE_CERTIFICATES'] and + external_auth.views.ssl_get_cert_from_request(request)): + # SSL login doesn't require a view, so redirect + # branding and allow that to process the login if it + # is enabled and the header is in the request. + return external_auth.views.redirect_with_get('root', request.GET) + if settings.FEATURES.get('AUTH_USE_CAS'): + # If CAS is enabled, redirect auth handling to there + return redirect(reverse('cas-login')) if request.user.is_authenticated(): return redirect(reverse('dashboard')) @@ -383,17 +383,15 @@ def signin_user(request): @ensure_csrf_cookie def register_user(request, extra_context=None): - """This view will display the non-modal registration form - - DEPRECATION WARNING: This view will eventually be deprecated and replaced - with the combined login/registration page in `student_account.views`. + """ + This view will display the non-modal registration form """ if request.user.is_authenticated(): return redirect(reverse('dashboard')) - - external_auth_response = external_auth_register(request) - if external_auth_response is not None: - return external_auth_response + if settings.FEATURES.get('AUTH_USE_CERTIFICATES_IMMEDIATE_SIGNUP'): + # Redirect to branding to process their certificate if SSL is enabled + # and registration is disabled. + return external_auth.views.redirect_with_get('root', request.GET) course_id = request.GET.get('course_id') @@ -901,21 +899,56 @@ def change_enrollment(request, check_access=True): return HttpResponseBadRequest(_("Enrollment action is invalid")) +# pylint: disable=fixme +# TODO: This function is kind of gnarly/hackish/etc and is only used in one location. +# It'd be awesome if we could get rid of it; manually parsing course_id strings form larger strings +# seems Probably Incorrect +def _parse_course_id_from_string(input_str): + """ + Helper function to determine if input_str (typically the queryparam 'next') contains a course_id. + @param input_str: + @return: the course_id if found, None if not + """ + m_obj = re.match(r'^/courses/{}'.format(settings.COURSE_ID_PATTERN), input_str) + if m_obj: + return SlashSeparatedCourseKey.from_deprecated_string(m_obj.group('course_id')) + return None + + +def _get_course_enrollment_domain(course_id): + """ + Helper function to get the enrollment domain set for a course with id course_id + @param course_id: + @return: + """ + course = modulestore().get_course(course_id) + if course is None: + return None + + return course.enrollment_domain + + @never_cache @ensure_csrf_cookie def accounts_login(request): """ This view is mainly used as the redirect from the @login_required decorator. I don't believe that the login path linked from the homepage uses it. - - DEPRECATION WARNING: This view will eventually be deprecated and replaced - with the combined login/registration page in `student_account.views`. """ - external_auth_response = external_auth_login(request) - if external_auth_response is not None: - return external_auth_response - + if settings.FEATURES.get('AUTH_USE_CAS'): + return redirect(reverse('cas-login')) + if settings.FEATURES['AUTH_USE_CERTIFICATES']: + # SSL login doesn't require a view, so login + # directly here + return external_auth.views.ssl_login(request) + # see if the "next" parameter has been set, whether it has a course context, and if so, whether + # there is a course-specific place to redirect redirect_to = request.GET.get('next') + if redirect_to: + course_id = _parse_course_id_from_string(redirect_to) + if course_id and _get_course_enrollment_domain(course_id): + return external_auth.views.course_specific_login(request, course_id.to_deprecated_string()) + context = { 'pipeline_running': 'false', 'pipeline_url': auth_pipeline_urls(pipeline.AUTH_ENTRY_LOGIN, redirect_url=redirect_to), diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 3eadf28ca9..4564f5533a 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -111,10 +111,10 @@ AUTH_ENTRY_LOGIN = 'login' AUTH_ENTRY_PROFILE = 'profile' AUTH_ENTRY_REGISTER = 'register' -# This is left-over from an A/B test -# of the new combined login/registration page (ECOM-369) -# We need to keep both the old and new entry points -# until every session from before the test ended has expired. +# 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' AUTH_ENTRY_REGISTER_2 = 'account_register' @@ -129,13 +129,12 @@ AUTH_ENTRY_API = 'api' # to load that depend on this module. AUTH_DISPATCH_URLS = { AUTH_ENTRY_DASHBOARD: '/dashboard', - AUTH_ENTRY_LOGIN: '/account/login', - AUTH_ENTRY_REGISTER: '/account/register', + AUTH_ENTRY_LOGIN: '/login', + AUTH_ENTRY_REGISTER: '/register', - # This is left-over from an A/B test - # of the new combined login/registration page (ECOM-369) - # We need to keep both the old and new entry points - # until every session from before the test ended has expired. + # 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/', @@ -151,10 +150,11 @@ _AUTH_ENTRY_CHOICES = frozenset([ AUTH_ENTRY_PROFILE, AUTH_ENTRY_REGISTER, - # This is left-over from an A/B test - # of the new combined login/registration page (ECOM-369) - # We need to keep both the old and new entry points - # until every session from before the test ended has expired. + # TODO (ECOM-369): For the A/B test of the combined + # 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, @@ -437,16 +437,31 @@ def parse_query_params(strategy, response, *args, **kwargs): # Whether the auth pipeline entered from /dashboard. 'is_dashboard': auth_entry == AUTH_ENTRY_DASHBOARD, # Whether the auth pipeline entered from /login. - 'is_login': auth_entry in [AUTH_ENTRY_LOGIN, AUTH_ENTRY_LOGIN_2], + 'is_login': auth_entry == AUTH_ENTRY_LOGIN, # Whether the auth pipeline entered from /register. - 'is_register': auth_entry in [AUTH_ENTRY_REGISTER, AUTH_ENTRY_REGISTER_2], + 'is_register': auth_entry == AUTH_ENTRY_REGISTER, # Whether the auth pipeline entered from /profile. 'is_profile': auth_entry == AUTH_ENTRY_PROFILE, # Whether the auth pipeline entered from an API 'is_api': auth_entry == AUTH_ENTRY_API, + + # 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, } +# TODO (ECOM-369): Once the A/B test of the combined login/registration +# form completes, we will be able to remove the extra login/registration +# end-points. HOWEVER, users who used the new forms during the A/B +# test may still have values for "is_login_2" and "is_register_2" +# in their sessions. For this reason, we need to continue accepting +# 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, @@ -482,23 +497,36 @@ def ensure_user_information( # invariants have been violated and future misbehavior is likely. user_inactive = user and not user.is_active user_unset = user is None - dispatch_to_login = (is_login or is_login_2) and (user_unset or user_inactive) - dispatch_to_register = (is_register or is_register_2) and user_unset + dispatch_to_login = is_login and (user_unset or user_inactive) reject_api_request = is_api and (user_unset or user_inactive) if reject_api_request: # Content doesn't matter; we just want to exit the pipeline return HttpResponseBadRequest() + # TODO (ECOM-369): Consolidate this with `dispatch_to_login` + # 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: return if dispatch_to_login: return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_LOGIN], name='signin_user') - if dispatch_to_register: + # TODO (ECOM-369): Consolidate this with `dispatch_to_login` + # once the A/B test completes. # pylint: disable=fixme + if dispatch_to_login_2: + return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_LOGIN_2]) + + if is_register and user_unset: 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. # pylint: disable=fixme + if is_register_2 and user_unset: + return redirect(AUTH_DISPATCH_URLS[AUTH_ENTRY_REGISTER_2]) + @partial.partial def set_logged_in_cookie(backend=None, user=None, request=None, is_api=None, *args, **kwargs): diff --git a/common/djangoapps/third_party_auth/tests/specs/base.py b/common/djangoapps/third_party_auth/tests/specs/base.py index 1b6da1739d..3c34754931 100644 --- a/common/djangoapps/third_party_auth/tests/specs/base.py +++ b/common/djangoapps/third_party_auth/tests/specs/base.py @@ -198,6 +198,13 @@ class IntegrationTest(testutil.TestCase, test.TestCase): self.assertFalse(payload.get('success')) self.assertIn('incorrect', payload.get('value')) + def assert_javascript_would_submit_login_form(self, boolean, response): + """Asserts we pass form submit JS the right boolean string.""" + argument_string = re.search( + r'function\ post_form_if_pipeline_running.*\(([a-z]+)\)', response.content, re.DOTALL).groups()[0] + self.assertIn(argument_string, ['true', 'false']) + self.assertEqual(boolean, True if argument_string == 'true' else False) + def assert_json_failure_response_is_inactive_account(self, response): """Asserts failure on /login for inactive account looks right.""" self.assertEqual(200, response.status_code) # Yes, it's a 200 even though it's a failure. @@ -227,14 +234,15 @@ class IntegrationTest(testutil.TestCase, test.TestCase): def assert_login_response_before_pipeline_looks_correct(self, response): """Asserts a GET of /login not in the pipeline looks correct.""" self.assertEqual(200, response.status_code) - # The combined login/registration page dynamically generates the login button, - # but we can still check that the provider name is passed in the data attribute - # for the container element. - self.assertIn(self.PROVIDER_CLASS.NAME, response.content) + self.assertIn('Sign in with ' + self.PROVIDER_CLASS.NAME, response.content) + self.assert_javascript_would_submit_login_form(False, response) + self.assert_signin_button_looks_functional(response.content, pipeline.AUTH_ENTRY_LOGIN) def assert_login_response_in_pipeline_looks_correct(self, response): """Asserts a GET of /login in the pipeline looks correct.""" self.assertEqual(200, response.status_code) + # Make sure the form submit JS is told to submit the form: + self.assert_javascript_would_submit_login_form(True, response) def assert_password_overridden_by_pipeline(self, username, password): """Verifies that the given password is not correct. @@ -254,22 +262,27 @@ class IntegrationTest(testutil.TestCase, test.TestCase): self.assertEqual(auth_settings._SOCIAL_AUTH_LOGIN_REDIRECT_URL, response.get('Location')) def assert_redirect_to_login_looks_correct(self, response): - """Asserts a response would redirect to /account/login.""" + """Asserts a response would redirect to /login.""" self.assertEqual(302, response.status_code) - self.assertEqual('/account/login', response.get('Location')) + self.assertEqual('/' + pipeline.AUTH_ENTRY_LOGIN, response.get('Location')) def assert_redirect_to_register_looks_correct(self, response): - """Asserts a response would redirect to /account/register.""" + """Asserts a response would redirect to /register.""" self.assertEqual(302, response.status_code) - self.assertEqual('/account/register', response.get('Location')) + self.assertEqual('/' + pipeline.AUTH_ENTRY_REGISTER, response.get('Location')) def assert_register_response_before_pipeline_looks_correct(self, response): """Asserts a GET of /register not in the pipeline looks correct.""" self.assertEqual(200, response.status_code) - # The combined login/registration page dynamically generates the register button, - # but we can still check that the provider name is passed in the data attribute - # for the container element. - self.assertIn(self.PROVIDER_CLASS.NAME, response.content) + self.assertIn('Sign up with ' + self.PROVIDER_CLASS.NAME, response.content) + self.assert_signin_button_looks_functional(response.content, pipeline.AUTH_ENTRY_REGISTER) + + def assert_signin_button_looks_functional(self, content, auth_entry): + """Asserts JS is available to signin buttons and has the right args.""" + self.assertTrue(re.search(r'function thirdPartySignin', content)) + self.assertEqual( + pipeline.get_login_url(self.PROVIDER_CLASS.NAME, auth_entry), + re.search(r"thirdPartySignin\(event, '([^']+)", content).groups()[0]) def assert_social_auth_does_not_exist_for_user(self, user, strategy): """Asserts a user does not have an auth with the expected provider.""" @@ -391,10 +404,10 @@ 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('/account/login', 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('/account/register', 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) diff --git a/common/test/acceptance/tests/lms/test_lms.py b/common/test/acceptance/tests/lms/test_lms.py index 499d5768c2..09853250bf 100644 --- a/common/test/acceptance/tests/lms/test_lms.py +++ b/common/test/acceptance/tests/lms/test_lms.py @@ -11,6 +11,8 @@ from bok_choy.web_app_test import WebAppTest from ..helpers import UniqueCourseTest, load_data_str from ...pages.lms.auto_auth import AutoAuthPage from ...pages.common.logout import LogoutPage +from ...pages.lms.find_courses import FindCoursesPage +from ...pages.lms.course_about import CourseAboutPage from ...pages.lms.course_info import CourseInfoPage from ...pages.lms.tab_nav import TabNavPage from ...pages.lms.course_nav import CourseNavPage @@ -23,6 +25,48 @@ from ...pages.lms.login_and_register import CombinedLoginAndRegisterPage from ...fixtures.course import CourseFixture, XBlockFixtureDesc, CourseUpdateDesc +class RegistrationTest(UniqueCourseTest): + """ + Test the registration process. + """ + + def setUp(self): + """ + Initialize pages and install a course fixture. + """ + super(RegistrationTest, self).setUp() + + self.find_courses_page = FindCoursesPage(self.browser) + self.course_about_page = CourseAboutPage(self.browser, self.course_id) + + # Create a course to register for + CourseFixture( + self.course_info['org'], self.course_info['number'], + self.course_info['run'], self.course_info['display_name'] + ).install() + + def test_register(self): + + # Visit the main page with the list of courses + self.find_courses_page.visit() + + # Go to the course about page and click the register button + self.course_about_page.visit() + register_page = self.course_about_page.register() + + # Fill in registration info and submit + username = "test_" + self.unique_id[0:6] + register_page.provide_info( + username + "@example.com", "test", username, "Test User" + ) + dashboard = register_page.submit() + + # We should end up at the dashboard + # Check that we're registered for the course + course_names = dashboard.available_courses + self.assertIn(self.course_info['display_name'], course_names) + + @attr('shard_1') class LoginFromCombinedPageTest(UniqueCourseTest): """Test that we can log in using the combined login/registration page. diff --git a/lms/djangoapps/courseware/features/login.feature b/lms/djangoapps/courseware/features/login.feature new file mode 100644 index 0000000000..3576a1adaf --- /dev/null +++ b/lms/djangoapps/courseware/features/login.feature @@ -0,0 +1,58 @@ +@shard_1 +Feature: LMS.Login in as a registered user + As a registered user + In order to access my content + I want to be able to login in to edX + + Scenario: Login to an unactivated account + Given I am an edX user + And I am an unactivated user + And I visit the homepage + When I click the link with the text "Log in" + And I submit my credentials on the login form + Then I should see the login error message "This account has not been activated" + + # firefox will not redirect properly when the whole suite is run + @skip_firefox + Scenario: Login to an activated account + Given I am an edX user + And I am an activated user + And I visit the homepage + When I click the link with the text "Log in" + And I submit my credentials on the login form + Then I should be on the dashboard page + + Scenario: Logout of a signed in account + Given I am logged in + When I click the dropdown arrow + And I click the link with the text "Log Out" + Then I should see a link with the text "Log in" + And I should see that the path is "/" + + Scenario: Login with valid redirect + Given I am an edX user + And The course "6.002x" exists + And I am registered for the course "6.002x" + And I am not logged in + And I visit the url "/courses/{}/courseware" + And I should see that the path is "/accounts/login?next=/courses/{}/courseware" + When I submit my credentials on the login form + And I wait for "2" seconds + Then the page title should contain "6.002x Courseware" + + Scenario: Login with an invalid redirect + Given I am an edX user + And I am not logged in + And I visit the url "/login?next=http://www.google.com/" + When I submit my credentials on the login form + Then I should be on the dashboard page + + Scenario: Login with a redirect with parameters + Given I am an edX user + And I am not logged in + And I visit the url "/debug/show_parameters?foo=hello&bar=world" + And I should see that the path is "/accounts/login?next=/debug/show_parameters%3Ffoo%3Dhello%26bar%3Dworld" + When I submit my credentials on the login form + And I wait for "2" seconds + Then I should see "foo: u'hello'" somewhere on the page + And I should see "bar: u'world'" somewhere on the page diff --git a/lms/djangoapps/courseware/features/login.py b/lms/djangoapps/courseware/features/login.py new file mode 100644 index 0000000000..0881a81eea --- /dev/null +++ b/lms/djangoapps/courseware/features/login.py @@ -0,0 +1,59 @@ +# pylint: disable=missing-docstring +# pylint: disable=redefined-outer-name + +from lettuce import step, world +from django.contrib.auth.models import User + + +@step('I am an unactivated user$') +def i_am_an_unactivated_user(step): + user_is_an_unactivated_user('robot') + + +@step('I am an activated user$') +def i_am_an_activated_user(step): + user_is_an_activated_user('robot') + + +@step('I submit my credentials on the login form') +def i_submit_my_credentials_on_the_login_form(step): + fill_in_the_login_form('email', 'robot@edx.org') + fill_in_the_login_form('password', 'test') + + def submit_login_form(): + login_form = world.browser.find_by_css('form#login-form') + login_form.find_by_name('submit').click() + world.retry_on_exception(submit_login_form) + + +@step(u'I should see the login error message "([^"]*)"$') +def i_should_see_the_login_error_message(step, msg): + login_error_div = world.browser.find_by_css('.submission-error.is-shown') + assert (msg in login_error_div.text) + + +@step(u'click the dropdown arrow$') +def click_the_dropdown(step): + world.css_click('.dropdown') + +#### helper functions + + +def user_is_an_unactivated_user(uname): + u = User.objects.get(username=uname) + u.is_active = False + u.save() + + +def user_is_an_activated_user(uname): + u = User.objects.get(username=uname) + u.is_active = True + u.save() + + +def fill_in_the_login_form(field, value): + def fill_login_form(): + login_form = world.browser.find_by_css('form#login-form') + form_field = login_form.find_by_name(field) + form_field.fill(value) + world.retry_on_exception(fill_login_form) diff --git a/lms/djangoapps/courseware/features/signup.feature b/lms/djangoapps/courseware/features/signup.feature new file mode 100644 index 0000000000..60067d5b20 --- /dev/null +++ b/lms/djangoapps/courseware/features/signup.feature @@ -0,0 +1,19 @@ +@shard_2 +Feature: LMS.Sign in + In order to use the edX content + As a new user + I want to signup for a student account + + # firefox will not redirect properly + @skip_firefox + Scenario: Sign up from the homepage + Given I visit the homepage + When I click the link with the text "Register Now" + And I fill in "email" on the registration form with "robot2@edx.org" + And I fill in "password" on the registration form with "test" + And I fill in "username" on the registration form with "robot2" + And I fill in "name" on the registration form with "Robot Two" + And I check the checkbox named "terms_of_service" + And I check the checkbox named "honor_code" + And I submit the registration form + Then I should see "Thanks for Registering!" in the dashboard banner diff --git a/lms/djangoapps/courseware/features/signup.py b/lms/djangoapps/courseware/features/signup.py new file mode 100644 index 0000000000..5f35a40e9a --- /dev/null +++ b/lms/djangoapps/courseware/features/signup.py @@ -0,0 +1,33 @@ +# pylint: disable=missing-docstring +# pylint: disable=redefined-outer-name + +from lettuce import world, step + + +@step('I fill in "([^"]*)" on the registration form with "([^"]*)"$') +def when_i_fill_in_field_on_the_registration_form_with_value(step, field, value): + def fill_in_registration(): + register_form = world.browser.find_by_css('form#register-form') + form_field = register_form.find_by_name(field) + form_field.fill(value) + world.retry_on_exception(fill_in_registration) + + +@step('I submit the registration form$') +def i_press_the_button_on_the_registration_form(step): + def submit_registration(): + register_form = world.browser.find_by_css('form#register-form') + register_form.find_by_name('submit').click() + world.retry_on_exception(submit_registration) + + +@step('I check the checkbox named "([^"]*)"$') +def i_check_checkbox(step, checkbox): + css_selector = 'input[name={}]'.format(checkbox) + world.css_check(css_selector) + + +@step('I should see "([^"]*)" in the dashboard banner$') +def i_should_see_text_in_the_dashboard_banner_section(step, text): + css_selector = "section.dashboard-banner h2" + assert (text in world.css_text(css_selector)) diff --git a/lms/djangoapps/courseware/tests/test_registration_extra_vars.py b/lms/djangoapps/courseware/tests/test_registration_extra_vars.py new file mode 100644 index 0000000000..0be1646ce5 --- /dev/null +++ b/lms/djangoapps/courseware/tests/test_registration_extra_vars.py @@ -0,0 +1,293 @@ +# -*- coding: utf-8 +""" +Tests for extra registration variables +""" +import json +from django.conf import settings +from django.test import TestCase +from django.core.urlresolvers import reverse +from mock import patch +from bs4 import BeautifulSoup +from django.utils import translation + + +class TestSortedCountryList(TestCase): + """ + Test that country list is always sorted alphabetically + """ + def setUp(self): + super(TestSortedCountryList, self).setUp() + self.url = reverse('register_user') + + def find_option_by_code(self, options, code): # pylint: disable=missing-docstring + for index, option in enumerate(options): + if option.attrs['value'] == code: + return (index, option) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'country': 'required'}) + def test_country_sorting_english(self): + """ + Test that country list is always sorted alphabetically in English + """ + response = self.client.get(self.url) + soup = BeautifulSoup(response.content) + country = soup.find(id="country") + options = country.findAll("option") + (af_index, af_option) = self.find_option_by_code(options, 'AF') + self.assertEqual( + af_option.text, + u'Afghanistan', + ) + (us_index, us_option) = self.find_option_by_code(options, 'US') + self.assertEqual( + us_option.text, + u'United States', + ) + # testing that the Afghan entry is always before the US entry + self.assertLess(af_index, us_index) + # testing two option elements to be in alphabetical order + self.assertLess(options[1].text, options[10].text) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'country': 'required'}) + def test_country_sorting_french(self): + """ + Test that country list is always sorted alphabetically in French + """ + user_language = 'fr' + with translation.override(user_language): + self.client.cookies['django_language'] = user_language + response = self.client.get(self.url) + soup = BeautifulSoup(response.content) + country = soup.find(id="country") + options = country.findAll("option") + (af_index, af_option) = self.find_option_by_code(options, 'AF') + self.assertEqual( + af_option.text, + u'Afghanistan', + ) + (us_index, us_option) = self.find_option_by_code(options, 'US') + self.assertEqual( + us_option.text, + u'États-Unis', + ) + # testing that the Afghan entry is always before the US entry + self.assertLess(af_index, us_index) + # testing two option elements to be in alphabetical order + self.assertLess(options[1].text, options[10].text) + + +class TestExtraRegistrationVariables(TestCase): + """ + Test that extra registration variables are properly checked according to settings + """ + def setUp(self): + super(TestExtraRegistrationVariables, self).setUp() + self.url = reverse('create_account') + + self.url_params = { + 'username': 'username', + 'name': 'name', + 'email': 'foo_bar@bar.com', + 'password': 'password', + 'terms_of_service': 'true', + 'honor_code': 'true', + } + + def test_default_missing_honor(self): + """ + By default, the honor code must be required + """ + self.url_params['honor_code'] = '' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 400) + obj = json.loads(response.content) + self.assertEqual( + obj['value'], + u'To enroll, you must follow the honor code.', + ) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'honor_code': 'optional'}) + def test_optional_honor(self): + """ + With the honor code is made optional, should pass without extra vars + """ + self.url_params['honor_code'] = '' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 200) + obj = json.loads(response.content) + self.assertEqual(obj['success'], True) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, { + 'level_of_education': 'hidden', + 'gender': 'hidden', + 'year_of_birth': 'hidden', + 'mailing_address': 'hidden', + 'goals': 'hidden', + 'honor_code': 'hidden', + 'city': 'hidden', + 'country': 'hidden'}) + def test_all_hidden(self): + """ + When the fields are all hidden, should pass without extra vars + """ + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 200) + obj = json.loads(response.content) + self.assertTrue(obj['success']) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'city': 'required'}) + def test_required_city_missing(self): + """ + Should require the city if configured as 'required' but missing + """ + self.url_params['city'] = '' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 400) + obj = json.loads(response.content) + self.assertEqual( + obj['value'], + u'A city is required', + ) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'city': 'required'}) + def test_required_city(self): + """ + Should require the city if configured as 'required' but missing + """ + self.url_params['city'] = 'New York' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 200) + obj = json.loads(response.content) + self.assertTrue(obj['success']) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'country': 'required'}) + def test_required_country_missing(self): + """ + Should require the country if configured as 'required' but missing + """ + self.url_params['country'] = '' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 400) + obj = json.loads(response.content) + self.assertEqual( + obj['value'], + u'A country is required', + ) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'country': 'required'}) + def test_required_country(self): + self.url_params['country'] = 'New York' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 200) + obj = json.loads(response.content) + self.assertTrue(obj['success']) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'level_of_education': 'required'}) + def test_required_level_of_education_missing(self): + """ + Should require the level_of_education if configured as 'required' but missing + """ + self.url_params['level_of_education'] = '' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 400) + obj = json.loads(response.content) + self.assertEqual( + obj['value'], + u'A level of education is required', + ) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'level_of_education': 'required'}) + def test_required_level_of_education(self): + self.url_params['level_of_education'] = 'p' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 200) + obj = json.loads(response.content) + self.assertTrue(obj['success']) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'gender': 'required'}) + def test_required_gender_missing(self): + """ + Should require the gender if configured as 'required' but missing + """ + self.url_params['gender'] = '' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 400) + obj = json.loads(response.content) + self.assertEqual( + obj['value'], + u'Your gender is required', + ) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'gender': 'required'}) + def test_required_gender(self): + self.url_params['gender'] = 'm' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 200) + obj = json.loads(response.content) + self.assertTrue(obj['success']) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'year_of_birth': 'required'}) + def test_required_year_of_birth_missing(self): + """ + Should require the year_of_birth if configured as 'required' but missing + """ + self.url_params['year_of_birth'] = '' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 400) + obj = json.loads(response.content) + self.assertEqual( + obj['value'], + u'Your year of birth is required', + ) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'year_of_birth': 'required'}) + def test_required_year_of_birth(self): + self.url_params['year_of_birth'] = '1982' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 200) + obj = json.loads(response.content) + self.assertTrue(obj['success']) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'mailing_address': 'required'}) + def test_required_mailing_address_missing(self): + """ + Should require the mailing_address if configured as 'required' but missing + """ + self.url_params['mailing_address'] = '' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 400) + obj = json.loads(response.content) + self.assertEqual( + obj['value'], + u'Your mailing address is required', + ) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'mailing_address': 'required'}) + def test_required_mailing_address(self): + self.url_params['mailing_address'] = 'my address' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 200) + obj = json.loads(response.content) + self.assertTrue(obj['success']) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'goals': 'required'}) + def test_required_goals_missing(self): + """ + Should require the goals if configured as 'required' but missing + """ + self.url_params['goals'] = '' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 400) + obj = json.loads(response.content) + self.assertEqual( + obj['value'], + u'A description of your goals is required', + ) + + @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'goals': 'required'}) + def test_required_goals(self): + self.url_params['goals'] = 'my goals' + response = self.client.post(self.url, self.url_params) + self.assertEqual(response.status_code, 200) + obj = json.loads(response.content) + self.assertTrue(obj['success']) diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index b16581f6b4..1e9735ee11 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -241,7 +241,7 @@ def get_email_params(course, auto_enroll, secure=True): registration_url = u'{proto}://{site}{path}'.format( proto=protocol, site=stripped_site_name, - path=reverse('register_user') + path=reverse('student.views.register_user') ) course_url = u'{proto}://{site}{path}'.format( proto=protocol, diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py index 600edac6a5..8e8a48513a 100644 --- a/lms/djangoapps/instructor/views/legacy.py +++ b/lms/djangoapps/instructor/views/legacy.py @@ -1470,7 +1470,7 @@ def _do_enroll_students(course, course_key, students, secure=False, overload=Fal registration_url = '{proto}://{site}{path}'.format( proto=protocol, site=stripped_site_name, - path=reverse('register_user') + path=reverse('student.views.register_user') ) course_url = '{proto}://{site}{path}'.format( proto=protocol, diff --git a/lms/djangoapps/student_account/helpers.py b/lms/djangoapps/student_account/helpers.py index faa5416f72..cd2e8d947f 100644 --- a/lms/djangoapps/student_account/helpers.py +++ b/lms/djangoapps/student_account/helpers.py @@ -1,72 +1,4 @@ """Helper functions for the student account app. """ -from django.core.urlresolvers import reverse -from opaque_keys.edx.keys import CourseKey -from course_modes.models import CourseMode -from third_party_auth import ( # pylint: disable=W0611 - pipeline, provider, - is_enabled as third_party_auth_enabled -) - -def auth_pipeline_urls(auth_entry, redirect_url=None, course_id=None): - """Retrieve URLs for each enabled third-party auth provider. - - These URLs are used on the "sign up" and "sign in" buttons - on the login/registration forms to allow users to begin - authentication with a third-party provider. - - Optionally, we can redirect the user to an arbitrary - url after auth completes successfully. We use this - to redirect the user to a page that required login, - or to send users to the payment flow when enrolling - in a course. - - Args: - auth_entry (string): Either `pipeline.AUTH_ENTRY_LOGIN` or `pipeline.AUTH_ENTRY_REGISTER` - - Keyword Args: - redirect_url (unicode): If provided, send users to this URL - after they successfully authenticate. - - course_id (unicode): The ID of the course the user is enrolling in. - We use this to send users to the track selection page - if the course has a payment option. - Note that `redirect_url` takes precedence over the redirect - to the track selection page. - - Returns: - dict mapping provider names to URLs - - """ - if not third_party_auth_enabled(): - return {} - - if redirect_url is not None: - pipeline_redirect = redirect_url - elif course_id is not None: - # If the course is white-label (paid), then we send users - # to the shopping cart. (There is a third party auth pipeline - # step that will add the course to the cart.) - if CourseMode.is_white_label(CourseKey.from_string(course_id)): - pipeline_redirect = reverse("shoppingcart.views.show_cart") - - # Otherwise, send the user to the track selection page. - # The track selection page may redirect the user to the dashboard - # (if the only available mode is honor), or directly to verification - # (for professional ed). - else: - pipeline_redirect = reverse( - "course_modes_choose", - kwargs={'course_id': unicode(course_id)} - ) - else: - pipeline_redirect = None - - return { - provider.NAME: pipeline.get_login_url( - provider.NAME, auth_entry, - enroll_course_id=course_id, - redirect_url=pipeline_redirect - ) - for provider in provider.Registry.enabled() - } +# 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 diff --git a/lms/djangoapps/student_account/test/test_views.py b/lms/djangoapps/student_account/test/test_views.py index 6fe8456bc4..b293292f3f 100644 --- a/lms/djangoapps/student_account/test/test_views.py +++ b/lms/djangoapps/student_account/test/test_views.py @@ -434,14 +434,14 @@ class StudentAccountLoginAndRegistrationTest(ModuleStoreTestCase): { "name": "Facebook", "iconClass": "icon-facebook", - "loginUrl": self._third_party_login_url("facebook", "login"), - "registerUrl": self._third_party_login_url("facebook", "register") + "loginUrl": self._third_party_login_url("facebook", "account_login"), + "registerUrl": self._third_party_login_url("facebook", "account_register") }, { "name": "Google", "iconClass": "icon-google-plus", - "loginUrl": self._third_party_login_url("google-oauth2", "login"), - "registerUrl": self._third_party_login_url("google-oauth2", "register") + "loginUrl": self._third_party_login_url("google-oauth2", "account_login"), + "registerUrl": self._third_party_login_url("google-oauth2", "account_register") } ] self._assert_third_party_auth_data(response, current_provider, expected_providers) @@ -468,12 +468,12 @@ class StudentAccountLoginAndRegistrationTest(ModuleStoreTestCase): "name": "Facebook", "iconClass": "icon-facebook", "loginUrl": self._third_party_login_url( - "facebook", "login", + "facebook", "account_login", course_id=unicode(course.id), redirect_url=course_modes_choose_url ), "registerUrl": self._third_party_login_url( - "facebook", "register", + "facebook", "account_register", course_id=unicode(course.id), redirect_url=course_modes_choose_url ) @@ -482,12 +482,12 @@ class StudentAccountLoginAndRegistrationTest(ModuleStoreTestCase): "name": "Google", "iconClass": "icon-google-plus", "loginUrl": self._third_party_login_url( - "google-oauth2", "login", + "google-oauth2", "account_login", course_id=unicode(course.id), redirect_url=course_modes_choose_url ), "registerUrl": self._third_party_login_url( - "google-oauth2", "register", + "google-oauth2", "account_register", course_id=unicode(course.id), redirect_url=course_modes_choose_url ) @@ -516,12 +516,12 @@ class StudentAccountLoginAndRegistrationTest(ModuleStoreTestCase): "name": "Facebook", "iconClass": "icon-facebook", "loginUrl": self._third_party_login_url( - "facebook", "login", + "facebook", "account_login", course_id=unicode(course.id), redirect_url=shoppingcart_url ), "registerUrl": self._third_party_login_url( - "facebook", "register", + "facebook", "account_register", course_id=unicode(course.id), redirect_url=shoppingcart_url ) @@ -530,12 +530,12 @@ class StudentAccountLoginAndRegistrationTest(ModuleStoreTestCase): "name": "Google", "iconClass": "icon-google-plus", "loginUrl": self._third_party_login_url( - "google-oauth2", "login", + "google-oauth2", "account_login", course_id=unicode(course.id), redirect_url=shoppingcart_url ), "registerUrl": self._third_party_login_url( - "google-oauth2", "register", + "google-oauth2", "account_register", course_id=unicode(course.id), redirect_url=shoppingcart_url ) @@ -546,27 +546,6 @@ class StudentAccountLoginAndRegistrationTest(ModuleStoreTestCase): response = self.client.get(reverse("account_login"), {"course_id": unicode(course.id)}) self._assert_third_party_auth_data(response, None, expected_providers) - @override_settings(SITE_NAME=settings.MICROSITE_TEST_HOSTNAME) - def test_microsite_uses_old_login_page(self): - # Retrieve the login page from a microsite domain - # and verify that we're served the old page. - resp = self.client.get( - reverse("account_login"), - HTTP_HOST=settings.MICROSITE_TEST_HOSTNAME - ) - self.assertContains(resp, "Log into your Test Microsite Account") - self.assertContains(resp, "login-form") - - def test_microsite_uses_old_register_page(self): - # Retrieve the register page from a microsite domain - # and verify that we're served the old page. - resp = self.client.get( - reverse("account_register"), - HTTP_HOST=settings.MICROSITE_TEST_HOSTNAME - ) - self.assertContains(resp, "Register for Test Microsite") - self.assertContains(resp, "register-form") - def _assert_third_party_auth_data(self, response, current_provider, providers): """Verify that third party auth info is rendered correctly in a DOM data attribute. """ expected_data = u"data-third-party-auth='{auth_info}'".format( diff --git a/lms/djangoapps/student_account/views.py b/lms/djangoapps/student_account/views.py index 647427a27a..5239f8f27a 100644 --- a/lms/djangoapps/student_account/views.py +++ b/lms/djangoapps/student_account/views.py @@ -15,14 +15,6 @@ from django.views.decorators.http import require_http_methods from edxmako.shortcuts import render_to_response, render_to_string from microsite_configuration import microsite import third_party_auth -from external_auth.login_and_register import ( - login as external_auth_login, - register as external_auth_register -) -from student.views import ( - signin_user as old_login_view, - register_user as old_register_view -) from user_api.api import account as account_api from user_api.api import profile as profile_api @@ -68,26 +60,13 @@ def login_and_registration_form(request, initial_mode="login"): the user_api. Keyword Args: - initial_mode (string): Either "login" or "register". + initial_mode (string): Either "login" or "registration". """ # If we're already logged in, redirect to the dashboard if request.user.is_authenticated(): return redirect(reverse('dashboard')) - # If this is a microsite, revert to the old login/registration pages. - # We need to do this for now to support existing themes. - if microsite.is_request_in_microsite(): - if initial_mode == "login": - return old_login_view(request) - elif initial_mode == "register": - return old_register_view(request) - - # Allow external auth to intercept and handle the request - ext_auth_response = _external_auth_intercept(request, initial_mode) - if ext_auth_response is not None: - return ext_auth_response - # Otherwise, render the combined login/registration page context = { 'disable_courseware_js': True, @@ -306,14 +285,12 @@ def _third_party_auth_context(request): } course_id = request.GET.get("course_id") - redirect_to = request.GET.get("next") login_urls = auth_pipeline_urls( - third_party_auth.pipeline.AUTH_ENTRY_LOGIN, - course_id=course_id, - redirect_url=redirect_to + third_party_auth.pipeline.AUTH_ENTRY_LOGIN_2, + course_id=course_id ) register_urls = auth_pipeline_urls( - third_party_auth.pipeline.AUTH_ENTRY_REGISTER, + third_party_auth.pipeline.AUTH_ENTRY_REGISTER_2, course_id=course_id ) @@ -336,20 +313,3 @@ def _third_party_auth_context(request): context["currentProvider"] = current_provider.NAME return context - - -def _external_auth_intercept(request, mode): - """Allow external auth to intercept a login/registration request. - - Arguments: - request (Request): The original request. - mode (str): Either "login" or "register" - - Returns: - Response or None - - """ - if mode == "login": - return external_auth_login(request) - elif mode == "register": - return external_auth_register(request) diff --git a/lms/templates/courseware/mktg_course_about.html b/lms/templates/courseware/mktg_course_about.html index 6a0bac8859..c24969de6d 100644 --- a/lms/templates/courseware/mktg_course_about.html +++ b/lms/templates/courseware/mktg_course_about.html @@ -57,9 +57,8 @@ emailOptIn = '&email_opt_in=' + $("input[name='email_opt_in']").val(); } - <% href_url = 'accounts_login' if settings.FEATURES.get("ENABLE_COMBINED_LOGIN_REGISTRATION") else 'register_user' %> ## Ugh. - window.top.location.href = $("a.register").attr("href") || "${reverse(href_url)}?course_id=${course.id | u}&enrollment_action=enroll" + emailOptIn; + window.top.location.href = $("a.register").attr("href") || "${reverse('register_user')}?course_id=${course.id | u}&enrollment_action=enroll&email_opt_in=" + email_opt_in; } else { $('#register_error').html( (xhr.responseText ? xhr.responseText : "${_("An error occurred. Please try again later.")}") @@ -84,8 +83,7 @@ %elif allow_registration: - href="${reverse(href_url)}?course_id=${course.id | u}&enrollment_action=enroll" + href="${reverse('register_user')}?course_id=${course.id | u}&enrollment_action=enroll" %endif >${_("Enroll in")} ${course.display_number_with_default | h} %if len(course_modes) > 1: diff --git a/lms/urls.py b/lms/urls.py index 4730514479..bd25d6c9ca 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -18,6 +18,8 @@ urlpatterns = ('', # nopep8 url(r'^dashboard$', 'student.views.dashboard', name="dashboard"), url(r'^login_ajax$', 'student.views.login_user', name="login"), url(r'^login_ajax/(?P[^/]*)$', 'student.views.login_user'), + url(r'^login$', 'student.views.signin_user', name="signin_user"), + url(r'^register$', 'student.views.register_user', name="register_user"), url(r'^admin_dashboard$', 'dashboard.views.dashboard'), @@ -31,6 +33,7 @@ urlpatterns = ('', # nopep8 url(r'^segmentio/event$', 'track.views.segmentio.segmentio_event'), url(r'^t/(?P