From e5846e64bd3177fead4709a096c87b4f1616fcb2 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 29 Apr 2013 10:40:07 -0400 Subject: [PATCH] Modify course registration flow for new login and registration pages Now that we are using separate pages for login and registration rather than modals, clicking the registration button for a course should direct an unauthenticated user to the registration page, and the user should be enrolled in the course upon successful registration. Likewise, an unauthenticated user attempting to unenroll from a course should be directed to the login page and subsequently unenrolled from the course upon successful login. The enrollment change service also now uses HTTP status codes rather than JSON to communicate status to the front end. --- common/djangoapps/student/views.py | 73 +++++++++++-------- lms/djangoapps/courseware/tests/tests.py | 22 ++---- .../sass/multicourse/_course_about.scss | 9 +++ lms/templates/courseware/course_about.html | 68 ++++++----------- lms/templates/dashboard.html | 18 +++-- lms/templates/login.html | 5 ++ lms/templates/register.html | 5 ++ lms/urls.py | 2 +- 8 files changed, 105 insertions(+), 97 deletions(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 645c1249e0..e028f14b7c 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -20,7 +20,7 @@ from django.core.mail import send_mail from django.core.urlresolvers import reverse from django.core.validators import validate_email, validate_slug, ValidationError from django.db import IntegrityError -from django.http import HttpResponse, HttpResponseRedirect, Http404 +from django.http import HttpResponse, HttpResponseBadRequest, HttpResponseForbidden, HttpResponseNotAllowed, HttpResponseRedirect, Http404 from django.shortcuts import redirect from django_future.csrf import ensure_csrf_cookie, csrf_exempt from django.utils.http import cookie_date @@ -216,14 +216,21 @@ def signin_user(request): """ This view will display the non-modal login form """ - context = {} + context = { + 'course_id': request.GET.get('course_id'), + 'enrollment_action': request.GET.get('enrollment_action') + } return render_to_response('login.html', context) + def register_user(request): """ This view will display the non-modal registration form """ - context = {} + context = { + 'course_id': request.GET.get('course_id'), + 'enrollment_action': request.GET.get('enrollment_action') + } return render_to_response('register.html', context) @@ -290,35 +297,47 @@ def try_change_enrollment(request): """ if 'enrollment_action' in request.POST: try: - enrollment_output = change_enrollment(request) + enrollment_response = change_enrollment(request) # There isn't really a way to display the results to the user, so we just log it # We expect the enrollment to be a success, and will show up on the dashboard anyway - log.info("Attempted to automatically enroll after login. Results: {0}".format(enrollment_output)) + log.info( + "Attempted to automatically enroll after login. Response code: {0}; response body: {1}".format( + enrollment_response.status_code, + enrollment_response.content + ) + ) except Exception, e: log.exception("Exception automatically enrolling after login: {0}".format(str(e))) -@login_required -def change_enrollment_view(request): - """Delegate to change_enrollment to actually do the work.""" - return HttpResponse(json.dumps(change_enrollment(request))) - - - def change_enrollment(request): + """ + Modify the enrollment status for the logged-in user. + + The request parameter must be a POST request (other methods return 405) + that specifies course_id and enrollment_action parameters. If course_id or + enrollment_action is not specified, if course_id is not valid, if + enrollment_action is something other than "enroll" or "unenroll", if + enrollment_action is "enroll" and enrollment is closed for the course, or + if enrollment_action is "unenroll" and the user is not enrolled in the + course, a 400 error will be returned. If the user is not logged in, 403 + will be returned; it is important that only this case return 403 so the + front end can redirect the user to a registration or login page when this + happens. This function should only be called from an AJAX request or + as a post-login/registration helper, so the error messages in the responses + should never actually be user-visible. + """ if request.method != "POST": - raise Http404 + return HttpResponseNotAllowed(["POST"]) user = request.user if not user.is_authenticated(): - raise Http404 + return HttpResponseForbidden() - action = request.POST.get("enrollment_action", "") - - course_id = request.POST.get("course_id", None) + action = request.POST.get("enrollment_action") + course_id = request.POST.get("course_id") if course_id is None: - return HttpResponse(json.dumps({'success': False, - 'error': 'There was an error receiving the course id.'})) + return HttpResponseBadRequest("Course id not specified") if action == "enroll": # Make sure the course exists @@ -328,12 +347,10 @@ def change_enrollment(request): except ItemNotFoundError: log.warning("User {0} tried to enroll in non-existent course {1}" .format(user.username, course_id)) - return {'success': False, 'error': 'The course requested does not exist.'} + return HttpResponseBadRequest("Course id is invalid") if not has_access(user, course, 'enroll'): - return {'success': False, - 'error': 'enrollment in {} not allowed at this time' - .format(course.display_name_with_default)} + return HttpResponseBadRequest("Enrollment is closed") org, course_num, run = course_id.split("/") statsd.increment("common.student.enrollment", @@ -347,7 +364,7 @@ def change_enrollment(request): # If we've already created this enrollment in a separate transaction, # then just continue pass - return {'success': True} + return HttpResponse() elif action == "unenroll": try: @@ -360,13 +377,11 @@ def change_enrollment(request): "course:{0}".format(course_num), "run:{0}".format(run)]) - return {'success': True} + return HttpResponse() except CourseEnrollment.DoesNotExist: - return {'success': False, 'error': 'You are not enrolled for this course.'} + return HttpResponseBadRequest("You are not enrolled in this course") else: - return {'success': False, 'error': 'Invalid enrollment_action.'} - - return {'success': False, 'error': 'We weren\'t able to unenroll you. Please try again.'} + return HttpResponseBadRequest("Enrollment action is invalid") @ensure_csrf_cookie diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 4c9f592797..25a7876a2a 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -220,25 +220,20 @@ class LoginEnrollmentTestCase(TestCase): # Now make sure that the user is now actually activated self.assertTrue(get_user(email).is_active) - def _enroll(self, course): - """Post to the enrollment view, and return the parsed json response""" + def try_enroll(self, course): + """Try to enroll. Return bool success instead of asserting it.""" resp = self.client.post('/change_enrollment', { 'enrollment_action': 'enroll', 'course_id': course.id, }) - return parse_json(resp) - - def try_enroll(self, course): - """Try to enroll. Return bool success instead of asserting it.""" - data = self._enroll(course) - print ('Enrollment in %s result: %s' - % (course.location.url(), str(data))) - return data['success'] + print ('Enrollment in %s result status code: %s' + % (course.location.url(), str(resp.status_code))) + return resp.status_code == 200 def enroll(self, course): """Enroll the currently logged-in user, and check that it worked.""" - data = self._enroll(course) - self.assertTrue(data['success']) + result = self.try_enroll(course) + self.assertTrue(result) def unenroll(self, course): """Unenroll the currently logged-in user, and check that it worked.""" @@ -246,8 +241,7 @@ class LoginEnrollmentTestCase(TestCase): 'enrollment_action': 'unenroll', 'course_id': course.id, }) - data = parse_json(resp) - self.assertTrue(data['success']) + self.assertTrue(resp.status_code == 200) def check_for_get_code(self, code, url): """ diff --git a/lms/static/sass/multicourse/_course_about.scss b/lms/static/sass/multicourse/_course_about.scss index 0982577f42..195760721e 100644 --- a/lms/static/sass/multicourse/_course_about.scss +++ b/lms/static/sass/multicourse/_course_about.scss @@ -154,6 +154,15 @@ @include transition(); width: flex-grid(5, 8); } + + #register_error { + background: $error-red; + border: 1px solid rgb(202, 17, 17); + color: rgb(143, 14, 14); + display: none; + padding: 12px; + margin-top: 5px; + } } } diff --git a/lms/templates/courseware/course_about.html b/lms/templates/courseware/course_about.html index dc1dc17532..4a1a3e59ca 100644 --- a/lms/templates/courseware/course_about.html +++ b/lms/templates/courseware/course_about.html @@ -13,42 +13,26 @@ <%block name="js_extra"> - % if not registered: - %if user.is_authenticated(): - ## If the user is authenticated, clicking the enroll button just submits a form - - %else: - ## If the user is not authenticated, clicking the enroll button pops up the register - ## field. We also slip in the registration fields into the login/register fields so - ## the user is automatically registered after logging in / registering - - %endif - %endif + $('#class_enroll_form').on('ajax:complete', function(event, xhr) { + if(xhr.status == 200) { + location.href = "${reverse('dashboard')}"; + } else if (xhr.status == 403) { + location.href = "${reverse('register_user')}?course_id=${course.id}&enrollment_action=enroll"; + } else { + $('#register_error').html( + (xhr.responseText ? xhr.responseText : 'An error occurred. Please try again later.') + ).css("display", "block"); + } + }); + })(this) + @@ -66,8 +50,7 @@
- %if user.is_authenticated(): - %if registered: + %if user.is_authenticated() and registered: %if show_courseware_link: %endif @@ -76,16 +59,9 @@ View Courseware %endif - %else: - Register for ${course.number} -
- %endif %else: - Log In -% endif - to enroll.'>Register for ${course.number} + Register for ${course.number} +
%endif
diff --git a/lms/templates/dashboard.html b/lms/templates/dashboard.html index d23609801f..d9eca70e4d 100644 --- a/lms/templates/dashboard.html +++ b/lms/templates/dashboard.html @@ -59,14 +59,16 @@ $("#unenroll_course_number").text( $(event.target).data("course-number") ); }); - $(document).delegate('#unenroll_form', 'ajax:success', function(data, json, xhr) { - if(json.success) { - location.href="${reverse('dashboard')}"; + $('#unenroll_form').on('ajax:complete', function(event, xhr) { + if(xhr.status == 200) { + location.href = "${reverse('dashboard')}"; + } else if (xhr.status == 403) { + location.href = "${reverse('signin_user')}?course_id=" + + $("#unenroll_course_id").val() + "&enrollment_action=unenroll"; } else { - if($('#unenroll_error').length == 0) { - $('#unenroll_form').prepend(''); - } - $('#unenroll_error').text(json.error).stop().css("display", "block"); + $('#unenroll_error').html( + xhr.responseText ? xhr.responseText : "An error occurred. Please try again later." + ).stop().css("display", "block"); } }); @@ -355,6 +357,8 @@
+ +
diff --git a/lms/templates/login.html b/lms/templates/login.html index 2e0281f4f6..2cfb40d6bc 100644 --- a/lms/templates/login.html +++ b/lms/templates/login.html @@ -126,6 +126,11 @@ +% if course_id and enrollment_action: + + +% endif +
diff --git a/lms/templates/register.html b/lms/templates/register.html index ee06a3caa0..663bfd14d5 100644 --- a/lms/templates/register.html +++ b/lms/templates/register.html @@ -213,6 +213,11 @@ +% if course_id and enrollment_action: + + +% endif +
diff --git a/lms/urls.py b/lms/urls.py index 49dae19d58..fd76e7fcdf 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -180,7 +180,7 @@ if settings.COURSEWARE_ENABLED: url(r'^courses/?$', 'branding.views.courses', name="courses"), url(r'^change_enrollment$', - 'student.views.change_enrollment_view', name="change_enrollment"), + 'student.views.change_enrollment', name="change_enrollment"), #About the course url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/about$',