From 6882b4b53d416205ead51f13779d755b26efae95 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Fri, 10 Aug 2012 11:40:41 -0400 Subject: [PATCH] responding to review comments on #383 --- common/djangoapps/student/views.py | 43 ++++++++++++------------ lms/djangoapps/courseware/grades.py | 5 ++- lms/djangoapps/courseware/tests/tests.py | 8 ++--- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index ac928a7bd4..8093a5a51a 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -287,15 +287,15 @@ def _do_create_account(post_vars): Note: this function is also used for creating test users. """ - u = User(username=post_vars['username'], + user = User(username=post_vars['username'], email=post_vars['email'], is_active=False) - u.set_password(post_vars['password']) - r = Registration() + user.set_password(post_vars['password']) + registration = Registration() # TODO: Rearrange so that if part of the process fails, the whole process fails. # Right now, we can have e.g. no registration e-mail sent out and a zombie account try: - u.save() + user.save() except IntegrityError: # Figure out the cause of the integrity error if len(User.objects.filter(username=post_vars['username'])) > 0: @@ -310,25 +310,25 @@ def _do_create_account(post_vars): raise - r.register(u) + registration.register(user) - up = UserProfile(user=u) - up.name = post_vars['name'] - up.level_of_education = post_vars.get('level_of_education') - up.gender = post_vars.get('gender') - up.mailing_address = post_vars.get('mailing_address') - up.goals = post_vars.get('goals') + profile = UserProfile(user=user) + profile.name = post_vars['name'] + profile.level_of_education = post_vars.get('level_of_education') + profile.gender = post_vars.get('gender') + profile.mailing_address = post_vars.get('mailing_address') + profile.goals = post_vars.get('goals') try: - up.year_of_birth = int(post_vars['year_of_birth']) + profile.year_of_birth = int(post_vars['year_of_birth']) except (ValueError, KeyError): - up.year_of_birth = None # If they give us garbage, just ignore it instead + profile.year_of_birth = None # If they give us garbage, just ignore it instead # of asking them to put an integer. try: - up.save() + profile.save() except Exception: - log.exception("UserProfile creation failed for user {0}.".format(u.id)) - return (u, up, r) + log.exception("UserProfile creation failed for user {0}.".format(user.id)) + return (user, profile, registration) @ensure_csrf_cookie @@ -402,10 +402,10 @@ def create_account(request, post_override=None): return HttpResponse(json.dumps(js)) # Ok, looks like everything is legit. Create the account. - (u, up, r) = _do_create_account(post_vars) + (user, profile, registration) = _do_create_account(post_vars) d = {'name': post_vars['name'], - 'key': r.activation_key, + 'key': registration.activation_key, } # composes activation email @@ -417,10 +417,11 @@ def create_account(request, post_override=None): try: if settings.MITX_FEATURES.get('REROUTE_ACTIVATION_EMAIL'): dest_addr = settings.MITX_FEATURES['REROUTE_ACTIVATION_EMAIL'] - message = "Activation for %s (%s): %s\n" % (u, u.email, up.name) + '-' * 80 + '\n\n' + message + message = ("Activation for %s (%s): %s\n" % (user, user.email, profile.name) + + '-' * 80 + '\n\n' + message) send_mail(subject, message, settings.DEFAULT_FROM_EMAIL, [dest_addr], fail_silently=False) elif not settings.GENERATE_RANDOM_USER_CREDENTIALS: - res = u.email_user(subject, message, settings.DEFAULT_FROM_EMAIL) + res = user.email_user(subject, message, settings.DEFAULT_FROM_EMAIL) except: log.exception(sys.exc_info()) js['value'] = 'Could not send activation e-mail.' @@ -539,7 +540,7 @@ def reactivation_email(request): subject = ''.join(subject.splitlines()) message = render_to_string('reactivation_email.txt', d) - res = u.email_user(subject, message, settings.DEFAULT_FROM_EMAIL) + res = user.email_user(subject, message, settings.DEFAULT_FROM_EMAIL) return HttpResponse(json.dumps({'success': True})) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index d5042b25da..192794b6b3 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -1,3 +1,6 @@ +# Compute grades using real division, with no integer truncation +from __future__ import division + import random import logging @@ -219,7 +222,7 @@ def get_score(user, problem, student_module_cache): if total == 0: log.exception("Cannot reweight a problem with zero weight. Problem: " + str(instance_module)) return (correct, total) - correct = float(correct) * weight / total + correct = correct * weight / total total = weight return (correct, total) diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 606d93f1d4..da0688be3d 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -209,8 +209,8 @@ class TestCoursesLoadTestCase(PageLoader): class TestInstructorAuth(PageLoader): """Check that authentication works properly""" - # NOTE: Originally tried putting the imports into a setUpClass() method, - # but that seemed to run before override_settings took effect. Did not debug further. + # NOTE: setUpClass() runs before override_settings takes effect, so + # can't do imports there without manually hacking settings. def setUp(self): xmodule.modulestore.django._MODULESTORES = {} @@ -253,10 +253,8 @@ class TestInstructorAuth(PageLoader): # should work now self.check_for_get_code(200, reverse('courseware', kwargs={'course_id': self.toy.id})) - # TODO: Disabled rest of test for now. Fix once raising a 404 actually - # returns a 404 to the client - def instructor_urls(course): + "list of urls that only instructors/staff should be able to see" urls = [reverse(name, kwargs={'course_id': course.id}) for name in ( 'instructor_dashboard', 'gradebook',