From 5bc0b4864a558363945c0cb701024b4898c3b9fb Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Sat, 17 Nov 2012 15:24:23 -0500 Subject: [PATCH 1/8] moving cert link and survey logic into views, adding tests. In-progress. --- common/djangoapps/student/tests.py | 41 +++++++++++----- common/djangoapps/student/views.py | 77 ++++++++++++++++++++++++++++-- 2 files changed, 102 insertions(+), 16 deletions(-) diff --git a/common/djangoapps/student/tests.py b/common/djangoapps/student/tests.py index cde95153fd..8a46b2e458 100644 --- a/common/djangoapps/student/tests.py +++ b/common/djangoapps/student/tests.py @@ -6,11 +6,14 @@ Replace this with more appropriate tests for your application. """ import logging from datetime import datetime +from hashlib import sha1 from django.test import TestCase +from mock import patch, Mock from nose.plugins.skip import SkipTest from .models import User, UserProfile, CourseEnrollment, replicate_user, USER_FIELDS_TO_COPY +import .views COURSE_1 = 'edX/toy/2012_Fall' COURSE_2 = 'edx/full/6.002_Spring_2012' @@ -55,7 +58,7 @@ class ReplicationTest(TestCase): # This hasattr lameness is here because we don't want this test to be # triggered when we're being run by CMS tests (Askbot doesn't exist # there, so the test will fail). - # + # # seen_response_count isn't a field we care about, so it shouldn't have # been copied over. if hasattr(portal_user, 'seen_response_count'): @@ -74,7 +77,7 @@ class ReplicationTest(TestCase): # During this entire time, the user data should never have made it over # to COURSE_2 - self.assertRaises(User.DoesNotExist, + self.assertRaises(User.DoesNotExist, User.objects.using(COURSE_2).get, id=portal_user.id) @@ -108,19 +111,19 @@ class ReplicationTest(TestCase): # Grab all the copies we expect course_user = User.objects.using(COURSE_1).get(id=portal_user.id) self.assertEquals(portal_user, course_user) - self.assertRaises(User.DoesNotExist, + self.assertRaises(User.DoesNotExist, User.objects.using(COURSE_2).get, id=portal_user.id) course_enrollment = CourseEnrollment.objects.using(COURSE_1).get(id=portal_enrollment.id) self.assertEquals(portal_enrollment, course_enrollment) - self.assertRaises(CourseEnrollment.DoesNotExist, + self.assertRaises(CourseEnrollment.DoesNotExist, CourseEnrollment.objects.using(COURSE_2).get, id=portal_enrollment.id) course_user_profile = UserProfile.objects.using(COURSE_1).get(id=portal_user_profile.id) self.assertEquals(portal_user_profile, course_user_profile) - self.assertRaises(UserProfile.DoesNotExist, + self.assertRaises(UserProfile.DoesNotExist, UserProfile.objects.using(COURSE_2).get, id=portal_user_profile.id) @@ -174,30 +177,44 @@ class ReplicationTest(TestCase): portal_user.save() portal_user_profile.gender = 'm' portal_user_profile.save() - - # Grab all the copies we expect, and make sure it doesn't end up in + + # Grab all the copies we expect, and make sure it doesn't end up in # places we don't expect. course_user = User.objects.using(COURSE_1).get(id=portal_user.id) self.assertEquals(portal_user, course_user) - self.assertRaises(User.DoesNotExist, + self.assertRaises(User.DoesNotExist, User.objects.using(COURSE_2).get, id=portal_user.id) course_enrollment = CourseEnrollment.objects.using(COURSE_1).get(id=portal_enrollment.id) self.assertEquals(portal_enrollment, course_enrollment) - self.assertRaises(CourseEnrollment.DoesNotExist, + self.assertRaises(CourseEnrollment.DoesNotExist, CourseEnrollment.objects.using(COURSE_2).get, id=portal_enrollment.id) course_user_profile = UserProfile.objects.using(COURSE_1).get(id=portal_user_profile.id) self.assertEquals(portal_user_profile, course_user_profile) - self.assertRaises(UserProfile.DoesNotExist, + self.assertRaises(UserProfile.DoesNotExist, UserProfile.objects.using(COURSE_2).get, id=portal_user_profile.id) +class CourseEndingTest(TestCase): + """Test things related to course endings: certificates, surveys, etc""" + def test_process_survey_link(self): + username = "fred" + id = sha1(username) + link1 = "http://www.mysurvey.com" + self.assertEqual(process_survey_link(link1), link1) + link2 = "http://www.mysurvey.com?unique={UNIQUE_ID}" + link2_expected = "http://www.mysurvey.com?unique={UNIQUE_ID}".format(UNIQUE_ID=id) + self.assertEqual(views.process_survey_link(link2), link2_expected) + def test_cert_info(self): + user = Mock(username="fred") + survey_url = "http://a_survey.com" + course = Mock(end_of_course_survey_url=survey_url) + cert_status = None - - + self.assertEqual(views._cert_info(user, course, None), {'status': 'processing'}) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index e7562f83d0..2ebb98da7a 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -39,6 +39,8 @@ from xmodule.modulestore.exceptions import ItemNotFoundError from datetime import date from collections import namedtuple +from hashlib import sha1 + from courseware.courses import get_courses_by_university from courseware.access import has_access @@ -107,9 +109,9 @@ def get_date_for_press(publish_date): # strip off extra months, and just use the first: date = re.sub(multimonth_pattern, ", ", publish_date) if re.search(day_pattern, date): - date = datetime.datetime.strptime(date, "%B %d, %Y") - else: - date = datetime.datetime.strptime(date, "%B, %Y") + date = datetime.datetime.strptime(date, "%B %d, %Y") + else: + date = datetime.datetime.strptime(date, "%B, %Y") return date def press(request): @@ -127,6 +129,73 @@ def press(request): return render_to_response('static_templates/press.html', {'articles': articles}) +def process_survey_link(survey_link, user): + """ + If {UNIQUE_ID} appears in the link, replace it with a unique id for the user. + Currently, this is sha1(user.username). Otherwise, return survey_link. + """ + to_replace = '{UNIQUE_ID}' + if to_replace in survey_link: + unique_id = sha1(user.username) + return survey_link.replace(to_replace, unique_id) + + return survey_link + + +def cert_info(user, course): + """ + Get the certificate info needed to render the dashboard section for the given + student and course. Returns a dictionary with keys: + + 'status': one of 'generating', 'ready', 'notpassing', 'processing' + 'show_download_url': bool + 'download_url': url, only present if show_download_url is True + 'show_survey_button': bool + 'survey_url': url, only if show_survey_button is True + 'grade': if status is not 'processing' + """ + if not course.has_ended(): + return {} + + return _cert_info(user, course, certificate_status_for_student(user, course.id)) + +def _cert_info(user, course, cert_status): + """ + Implements the logic for cert_info -- split out for testing. + """ + default_status = 'processing' + if cert_status is None: + return {'status': default_status} + + # simplify the status for the template using this lookup table + template_state = { + CertificateStatuses.generating: 'generating', + CertificateStatuses.regenerating: 'generating', + CertificateStatuses.downloadable: 'ready', + CertificateStatuses.notpassing: 'notpassing', + } + + status = template_state.get(cert_status['status'], default_status) + + d = {'status': status, + 'show_download_url': status in ('generating', 'ready'),} + + if (status in ('generating', 'ready', 'not-available') and + course.end_of_course_survey_url is not None): + d.update({ + 'show_survey_button': True, + 'survey_url': process_survey_link(course.end_of_course_survey_url, user)}) + else: + d['show_survey_button'] = False + + if template_state == 'ready': + d['download_url'] = cert_status['download_url'] + + if template_state in 'generating', 'ready', 'notpassing': + d['grade'] = cert_status['grade'] + + return d + @login_required @ensure_csrf_cookie def dashboard(request): @@ -163,7 +232,7 @@ def dashboard(request): # TODO: workaround to not have to zip courses and certificates in the template # since before there is a migration to certificates if settings.MITX_FEATURES.get('CERTIFICATES_ENABLED'): - cert_statuses = { course.id: certificate_status_for_student(request.user, course.id) for course in courses} + cert_statuses = { course.id: cert_info(request.user, course) for course in courses} else: cert_statuses = {} From 3a44f043d29060fb7fe997dbec6f6e1dce727394 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Sat, 17 Nov 2012 15:58:49 -0500 Subject: [PATCH 2/8] Add tests, make them pass. --- common/djangoapps/student/tests.py | 80 +++++++++++++++++++++++++-- common/djangoapps/student/views.py | 21 ++++--- lms/djangoapps/certificates/models.py | 6 +- 3 files changed, 92 insertions(+), 15 deletions(-) diff --git a/common/djangoapps/student/tests.py b/common/djangoapps/student/tests.py index 8a46b2e458..16eec86379 100644 --- a/common/djangoapps/student/tests.py +++ b/common/djangoapps/student/tests.py @@ -13,7 +13,7 @@ from mock import patch, Mock from nose.plugins.skip import SkipTest from .models import User, UserProfile, CourseEnrollment, replicate_user, USER_FIELDS_TO_COPY -import .views +from .views import process_survey_link, _cert_info, unique_id_for_user COURSE_1 = 'edX/toy/2012_Fall' COURSE_2 = 'edx/full/6.002_Spring_2012' @@ -204,17 +204,85 @@ class CourseEndingTest(TestCase): def test_process_survey_link(self): username = "fred" - id = sha1(username) + user = Mock(username=username) + id = unique_id_for_user(user) link1 = "http://www.mysurvey.com" - self.assertEqual(process_survey_link(link1), link1) + self.assertEqual(process_survey_link(link1, user), link1) + link2 = "http://www.mysurvey.com?unique={UNIQUE_ID}" link2_expected = "http://www.mysurvey.com?unique={UNIQUE_ID}".format(UNIQUE_ID=id) - self.assertEqual(views.process_survey_link(link2), link2_expected) + self.assertEqual(process_survey_link(link2, user), link2_expected) def test_cert_info(self): user = Mock(username="fred") survey_url = "http://a_survey.com" course = Mock(end_of_course_survey_url=survey_url) - cert_status = None - self.assertEqual(views._cert_info(user, course, None), {'status': 'processing'}) + self.assertEqual(_cert_info(user, course, None), + {'status': 'processing', + 'show_disabled_download_button': False, + 'show_download_url': False, + 'show_survey_button': False,}) + + cert_status = {'status': 'unavailable'} + self.assertEqual(_cert_info(user, course, cert_status), + {'status': 'processing', + 'show_disabled_download_button': False, + 'show_download_url': False, + 'show_survey_button': False}) + + cert_status = {'status': 'generating', 'grade': '67'} + self.assertEqual(_cert_info(user, course, cert_status), + {'status': 'generating', + 'show_disabled_download_button': True, + 'show_download_url': False, + 'show_survey_button': True, + 'survey_url': survey_url, + 'grade': '67' + }) + + cert_status = {'status': 'regenerating', 'grade': '67'} + self.assertEqual(_cert_info(user, course, cert_status), + {'status': 'generating', + 'show_disabled_download_button': True, + 'show_download_url': False, + 'show_survey_button': True, + 'survey_url': survey_url, + 'grade': '67' + }) + + download_url = 'http://s3.edx/cert' + cert_status = {'status': 'downloadable', 'grade': '67', + 'download_url': download_url} + self.assertEqual(_cert_info(user, course, cert_status), + {'status': 'ready', + 'show_disabled_download_button': False, + 'show_download_url': True, + 'download_url': download_url, + 'show_survey_button': True, + 'survey_url': survey_url, + 'grade': '67' + }) + + cert_status = {'status': 'notpassing', 'grade': '67', + 'download_url': download_url} + self.assertEqual(_cert_info(user, course, cert_status), + {'status': 'notpassing', + 'show_disabled_download_button': False, + 'show_download_url': False, + 'show_survey_button': True, + 'survey_url': survey_url, + 'grade': '67' + }) + + # Test a course that doesn't have a survey specified + course2 = Mock(end_of_course_survey_url=None) + cert_status = {'status': 'notpassing', 'grade': '67', + 'download_url': download_url} + self.assertEqual(_cert_info(user, course2, cert_status), + {'status': 'notpassing', + 'show_disabled_download_button': False, + 'show_download_url': False, + 'show_survey_button': False, + 'grade': '67' + }) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 2ebb98da7a..250eddff57 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -129,6 +129,9 @@ def press(request): return render_to_response('static_templates/press.html', {'articles': articles}) +def unique_id_for_user(user): + return sha1(user.username).hexdigest() + def process_survey_link(survey_link, user): """ If {UNIQUE_ID} appears in the link, replace it with a unique id for the user. @@ -136,8 +139,7 @@ def process_survey_link(survey_link, user): """ to_replace = '{UNIQUE_ID}' if to_replace in survey_link: - unique_id = sha1(user.username) - return survey_link.replace(to_replace, unique_id) + return survey_link.replace(to_replace, unique_id_for_user(user)) return survey_link @@ -150,6 +152,7 @@ def cert_info(user, course): 'status': one of 'generating', 'ready', 'notpassing', 'processing' 'show_download_url': bool 'download_url': url, only present if show_download_url is True + 'show_disabled_download_button': bool -- true if state is 'generating' 'show_survey_button': bool 'survey_url': url, only if show_survey_button is True 'grade': if status is not 'processing' @@ -165,7 +168,10 @@ def _cert_info(user, course, cert_status): """ default_status = 'processing' if cert_status is None: - return {'status': default_status} + return {'status': default_status, + 'show_disabled_download_button': False, + 'show_download_url': False, + 'show_survey_button': False} # simplify the status for the template using this lookup table template_state = { @@ -178,9 +184,10 @@ def _cert_info(user, course, cert_status): status = template_state.get(cert_status['status'], default_status) d = {'status': status, - 'show_download_url': status in ('generating', 'ready'),} + 'show_download_url': status == 'ready', + 'show_disabled_download_button': status == 'generating',} - if (status in ('generating', 'ready', 'not-available') and + if (status in ('generating', 'ready', 'notpassing') and course.end_of_course_survey_url is not None): d.update({ 'show_survey_button': True, @@ -188,10 +195,10 @@ def _cert_info(user, course, cert_status): else: d['show_survey_button'] = False - if template_state == 'ready': + if status == 'ready': d['download_url'] = cert_status['download_url'] - if template_state in 'generating', 'ready', 'notpassing': + if status in ('generating', 'ready', 'notpassing'): d['grade'] = cert_status['grade'] return d diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index 2d6f384443..b9bd55b9af 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -75,7 +75,9 @@ def certificate_status_for_student(student, course_id): This returns a dictionary with a key for status, and other information. The status is one of the following: - unavailable - A student is not eligible for a certificate. + unavailable - No entry for this student--if they are actually in + the course, they probably have not been graded for + certificate generation yet. generating - A request has been made to generate a certificate, but it has not been generated yet. regenerating - A request has been made to regenerate a certificate, @@ -90,7 +92,7 @@ def certificate_status_for_student(student, course_id): "download_url". If the student has been graded, the dictionary also contains their - grade for the course. + grade for the course with the key "grade". ''' try: From 03dd7b375a6a824ca331822d517d9ad1969adc8e Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Sat, 17 Nov 2012 15:59:12 -0500 Subject: [PATCH 3/8] get rid of submodule init--not needed post-askbot --- rakefile | 1 - 1 file changed, 1 deletion(-) diff --git a/rakefile b/rakefile index f06b9c3633..c95d822531 100644 --- a/rakefile +++ b/rakefile @@ -54,7 +54,6 @@ default_options = { task :predjango do sh("find . -type f -name *.pyc -delete") sh('pip install -q --upgrade -r local-requirements.txt') - sh('git submodule update --init') end task :clean_test_files do From 003dc7ef517901cc701307f6d761b7014bf4f702 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Sat, 17 Nov 2012 16:19:52 -0500 Subject: [PATCH 4/8] Get rid of check for non-existent CERTIFICATES_ENABLED flag... --- common/djangoapps/student/views.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 250eddff57..b354b8c20a 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -236,12 +236,7 @@ def dashboard(request): show_courseware_links_for = frozenset(course.id for course in courses if has_access(request.user, course, 'load')) - # TODO: workaround to not have to zip courses and certificates in the template - # since before there is a migration to certificates - if settings.MITX_FEATURES.get('CERTIFICATES_ENABLED'): - cert_statuses = { course.id: cert_info(request.user, course) for course in courses} - else: - cert_statuses = {} + cert_statuses = { course.id: cert_info(request.user, course) for course in courses} context = {'courses': courses, 'message': message, From 61ddec46dd58b2991e19718a23d8aa175ccaa97b Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Sat, 17 Nov 2012 16:20:17 -0500 Subject: [PATCH 5/8] Use params from view in template. --- lms/templates/dashboard.html | 55 +++++++++++++++--------------------- 1 file changed, 22 insertions(+), 33 deletions(-) diff --git a/lms/templates/dashboard.html b/lms/templates/dashboard.html index 2069685a6c..423f53aa35 100644 --- a/lms/templates/dashboard.html +++ b/lms/templates/dashboard.html @@ -159,54 +159,43 @@ %> % if course.has_ended() and cert_status: <% - passing_grade = False - cert_button = False - survey_button = False - if cert_status['status'] in [CertificateStatuses.generating, CertificateStatuses.regenerating]: + if cert_status['status'] == 'generating': status_css_class = 'course-status-certrendering' - cert_button = True - survey_button = True - passing_grade = True - elif cert_status['status'] == CertificateStatuses.downloadable: + elif cert_status['status'] == 'ready': status_css_class = 'course-status-certavailable' - cert_button = True - survey_button = True - passing_grade = True - elif cert_status['status'] == CertificateStatuses.notpassing: + elif cert_status['status'] == 'notpassing': status_css_class = 'course-status-certnotavailable' - survey_button = True else: - # This is primarily the 'unavailable' state, but also 'error', 'deleted', etc. status_css_class = 'course-status-processing' - - if survey_button and not course.end_of_course_survey_url: - survey_button = False %>
- % if cert_status['status'] == CertificateStatuses.unavailable: -

Final course details are being wrapped up at this time. - Your final standing will be available shortly.

- % elif passing_grade: -

You have received a grade of - ${cert_status['grade']} - in this course.

- % elif cert_status['status'] == CertificateStatuses.notpassing: -

You did not complete the necessary requirements for completion of this course. -

+ % if cert_status['status'] == 'processing': +

Final course details are being wrapped up at + this time. Your final standing will be available shortly.

+ % elif cert_status['status'] in ('generating', 'ready'): +

You have received a grade of + ${cert_status['grade']} + in this course.

+ % elif cert_status['status'] == 'notpassing': +

You did not complete the necessary requirements for + completion of this course.

% endif - % if cert_button or survey_button: + + % if cert_status['show_disabled_download_button'] or cert_status['show_download_url'] or cert_status['show_survey_button']: From 71b585bb614d80e79c8866b46145c6bdf6e95f20 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Sat, 17 Nov 2012 16:33:00 -0500 Subject: [PATCH 6/8] move unique_id_for_user into student/models.py --- common/djangoapps/student/models.py | 47 +++++++++++++++++++---------- common/djangoapps/student/tests.py | 6 ++-- common/djangoapps/student/views.py | 6 +--- 3 files changed, 36 insertions(+), 23 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 0eded21df1..4c91682ca6 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -36,10 +36,12 @@ file and check it in at the same time as your model changes. To do that, 3. Add the migration file created in mitx/common/djangoapps/student/migrations/ """ from datetime import datetime +from hashlib import sha1 import json import logging import uuid + from django.conf import settings from django.contrib.auth.models import User from django.db import models @@ -125,9 +127,9 @@ class UserProfile(models.Model): self.meta = json.dumps(js) class TestCenterUser(models.Model): - """This is our representation of the User for in-person testing, and + """This is our representation of the User for in-person testing, and specifically for Pearson at this point. A few things to note: - + * Pearson only supports Latin-1, so we have to make sure that the data we capture here will work with that encoding. * While we have a lot of this demographic data in UserProfile, it's much @@ -135,9 +137,9 @@ class TestCenterUser(models.Model): UserProfile, but we'll need to have a step where people who are signing up re-enter their demographic data into the fields we specify. * Users are only created here if they register to take an exam in person. - + The field names and lengths are modeled on the conventions and constraints - of Pearson's data import system, including oddities such as suffix having + of Pearson's data import system, including oddities such as suffix having a limit of 255 while last_name only gets 50. """ # Our own record keeping... @@ -148,21 +150,21 @@ class TestCenterUser(models.Model): # and is something Pearson needs to know to manage updates. Unlike # updated_at, this will not get incremented when we do a batch data import. user_updated_at = models.DateTimeField(db_index=True) - + # Unique ID given to us for this User by the Testing Center. It's null when # we first create the User entry, and is assigned by Pearson later. candidate_id = models.IntegerField(null=True, db_index=True) - + # Unique ID we assign our user for a the Test Center. client_candidate_id = models.CharField(max_length=50, db_index=True) - + # Name first_name = models.CharField(max_length=30, db_index=True) last_name = models.CharField(max_length=50, db_index=True) middle_name = models.CharField(max_length=30, blank=True) suffix = models.CharField(max_length=255, blank=True) salutation = models.CharField(max_length=50, blank=True) - + # Address address_1 = models.CharField(max_length=40) address_2 = models.CharField(max_length=40, blank=True) @@ -175,7 +177,7 @@ class TestCenterUser(models.Model): postal_code = models.CharField(max_length=16, blank=True, db_index=True) # country is a ISO 3166-1 alpha-3 country code (e.g. "USA", "CAN", "MNG") country = models.CharField(max_length=3, db_index=True) - + # Phone phone = models.CharField(max_length=35) extension = models.CharField(max_length=8, blank=True, db_index=True) @@ -183,14 +185,27 @@ class TestCenterUser(models.Model): fax = models.CharField(max_length=35, blank=True) # fax_country_code required *if* fax is present. fax_country_code = models.CharField(max_length=3, blank=True) - + # Company company_name = models.CharField(max_length=50, blank=True) - + @property def email(self): return self.user.email +def unique_id_for_user(user): + """ + Return a unique id for a user, suitable for inserting into + e.g. personalized survey links. + + Currently happens to be implemented as a sha1 hash of the username + (and thus assumes that usernames don't change). + """ + return sha1(user.username).hexdigest() + + + + ## TODO: Should be renamed to generic UserGroup, and possibly # Given an optional field for type of group class UserTestGroup(models.Model): @@ -363,10 +378,10 @@ def replicate_user_save(sender, **kwargs): # @receiver(post_save, sender=CourseEnrollment) def replicate_enrollment_save(sender, **kwargs): - """This is called when a Student enrolls in a course. It has to do the + """This is called when a Student enrolls in a course. It has to do the following: - 1. Make sure the User is copied into the Course DB. It may already exist + 1. Make sure the User is copied into the Course DB. It may already exist (someone deleting and re-adding a course). This has to happen first or the foreign key constraint breaks. 2. Replicate the CourseEnrollment. @@ -410,9 +425,9 @@ USER_FIELDS_TO_COPY = ["id", "username", "first_name", "last_name", "email", def replicate_user(portal_user, course_db_name): """Replicate a User to the correct Course DB. This is more complicated than - it should be because Askbot extends the auth_user table and adds its own + it should be because Askbot extends the auth_user table and adds its own fields. So we need to only push changes to the standard fields and leave - the rest alone so that Askbot changes at the Course DB level don't get + the rest alone so that Askbot changes at the Course DB level don't get overridden. """ try: @@ -457,7 +472,7 @@ def is_valid_course_id(course_id): """Right now, the only database that's not a course database is 'default'. I had nicer checking in here originally -- it would scan the courses that were in the system and only let you choose that. But it was annoying to run - tests with, since we don't have course data for some for our course test + tests with, since we don't have course data for some for our course test databases. Hence the lazy version. """ return course_id != 'default' diff --git a/common/djangoapps/student/tests.py b/common/djangoapps/student/tests.py index 16eec86379..4c7c9e2592 100644 --- a/common/djangoapps/student/tests.py +++ b/common/djangoapps/student/tests.py @@ -12,8 +12,10 @@ from django.test import TestCase from mock import patch, Mock from nose.plugins.skip import SkipTest -from .models import User, UserProfile, CourseEnrollment, replicate_user, USER_FIELDS_TO_COPY -from .views import process_survey_link, _cert_info, unique_id_for_user +from .models import (User, UserProfile, CourseEnrollment, + replicate_user, USER_FIELDS_TO_COPY, + unique_id_for_user) +from .views import process_survey_link, _cert_info COURSE_1 = 'edX/toy/2012_Fall' COURSE_2 = 'edx/full/6.002_Spring_2012' diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index b354b8c20a..ac3a97bd89 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -28,7 +28,7 @@ from django.core.cache import cache from django_future.csrf import ensure_csrf_cookie, csrf_exempt from student.models import (Registration, UserProfile, PendingNameChange, PendingEmailChange, - CourseEnrollment) + CourseEnrollment, unique_id_for_user) from certificates.models import CertificateStatuses, certificate_status_for_student @@ -39,7 +39,6 @@ from xmodule.modulestore.exceptions import ItemNotFoundError from datetime import date from collections import namedtuple -from hashlib import sha1 from courseware.courses import get_courses_by_university from courseware.access import has_access @@ -129,9 +128,6 @@ def press(request): return render_to_response('static_templates/press.html', {'articles': articles}) -def unique_id_for_user(user): - return sha1(user.username).hexdigest() - def process_survey_link(survey_link, user): """ If {UNIQUE_ID} appears in the link, replace it with a unique id for the user. From d1eec9bcf4fb5fb760e20a9ee03ad217ecc779f7 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 19 Nov 2012 11:34:31 -0500 Subject: [PATCH 7/8] add a salt. using user.id since that's handy. --- common/djangoapps/student/models.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 4c91682ca6..5975853a21 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -201,9 +201,10 @@ def unique_id_for_user(user): Currently happens to be implemented as a sha1 hash of the username (and thus assumes that usernames don't change). """ - return sha1(user.username).hexdigest() - - + # Using the user id as the salt because it's sort of random, and is already + # in the db. + salt = str(user.id) + return sha1(salt + user.username).hexdigest() ## TODO: Should be renamed to generic UserGroup, and possibly From d723bb71037aed46c30d7d1fb8db885c96d0abbd Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 19 Nov 2012 11:39:43 -0500 Subject: [PATCH 8/8] simpify process_survey_link --- common/djangoapps/student/views.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index ac3a97bd89..91627150fc 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -133,11 +133,7 @@ def process_survey_link(survey_link, user): If {UNIQUE_ID} appears in the link, replace it with a unique id for the user. Currently, this is sha1(user.username). Otherwise, return survey_link. """ - to_replace = '{UNIQUE_ID}' - if to_replace in survey_link: - return survey_link.replace(to_replace, unique_id_for_user(user)) - - return survey_link + return survey_link.format(UNIQUE_ID=unique_id_for_user(user)) def cert_info(user, course):