diff --git a/common/djangoapps/util/memcache.py b/common/djangoapps/util/memcache.py index 540cf96539..ee450e68cb 100644 --- a/common/djangoapps/util/memcache.py +++ b/common/djangoapps/util/memcache.py @@ -8,15 +8,42 @@ import urllib def fasthash(string): - m = hashlib.new("md4") - m.update(string) - return m.hexdigest() + """ + Hashes `string` into a string representation of a 128-bit digest. + """ + md4 = hashlib.new("md4") + md4.update(string) + return md4.hexdigest() + + +def cleaned_string(val): + """ + Converts `val` to unicode and URL-encodes special characters + (including quotes and spaces) + """ + return urllib.quote_plus(smart_str(val)) def safe_key(key, key_prefix, version): - safe_key = urllib.quote_plus(smart_str(key)) + """ + Given a `key`, `key_prefix`, and `version`, + return a key that is safe to use with memcache. - if len(safe_key) > 250: - safe_key = fasthash(safe_key) + `key`, `key_prefix`, and `version` can be numbers, strings, or unicode. + """ - return ":".join([key_prefix, str(version), safe_key]) + # Clean for whitespace and control characters, which + # cause memcache to raise an exception + key = cleaned_string(key) + key_prefix = cleaned_string(key_prefix) + version = cleaned_string(version) + + # Attempt to combine the prefix, version, and key + combined = ":".join([key_prefix, version, key]) + + # If the total length is too long for memcache, hash it + if len(combined) > 250: + combined = fasthash(combined) + + # Return the result + return combined diff --git a/common/djangoapps/util/tests/__init__.py b/common/djangoapps/util/tests/__init__.py new file mode 100644 index 0000000000..8b13789179 --- /dev/null +++ b/common/djangoapps/util/tests/__init__.py @@ -0,0 +1 @@ + diff --git a/common/djangoapps/util/tests/test_memcache.py b/common/djangoapps/util/tests/test_memcache.py new file mode 100644 index 0000000000..de8d352c38 --- /dev/null +++ b/common/djangoapps/util/tests/test_memcache.py @@ -0,0 +1,124 @@ +""" +Tests for memcache in util app +""" + +from django.test import TestCase +from django.core.cache import get_cache +from django.conf import settings +from util.memcache import safe_key + + +class MemcacheTest(TestCase): + """ + Test memcache key cleanup + """ + + # Test whitespace, control characters, and some non-ASCII UTF-16 + UNICODE_CHAR_CODES = ([c for c in range(0, 30)] + [127] + + [129, 500, 2 ** 8 - 1, 2 ** 8 + 1, 2 ** 16 - 1]) + + def setUp(self): + self.cache = get_cache('default') + + def test_safe_key(self): + key = safe_key('test', 'prefix', 'version') + self.assertEqual(key, 'prefix:version:test') + + def test_numeric_inputs(self): + + # Numeric key + self.assertEqual(safe_key(1, 'prefix', 'version'), 'prefix:version:1') + + # Numeric prefix + self.assertEqual(safe_key('test', 5, 'version'), '5:version:test') + + # Numeric version + self.assertEqual(safe_key('test', 'prefix', 5), 'prefix:5:test') + + def test_safe_key_long(self): + + # Choose lengths close to memcached's cutoff (250) + for length in [248, 249, 250, 251, 252]: + + # Generate a key of that length + key = 'a' * length + + # Make the key safe + key = safe_key(key, '', '') + + # The key should now be valid + self.assertTrue(self._is_valid_key(key), + msg="Failed for key length {0}".format(length)) + + def test_long_key_prefix_version(self): + + # Long key + key = safe_key('a' * 300, 'prefix', 'version') + self.assertTrue(self._is_valid_key(key)) + + # Long prefix + key = safe_key('key', 'a' * 300, 'version') + self.assertTrue(self._is_valid_key(key)) + + # Long version + key = safe_key('key', 'prefix', 'a' * 300) + self.assertTrue(self._is_valid_key(key)) + + def test_safe_key_unicode(self): + + for unicode_char in self.UNICODE_CHAR_CODES: + + # Generate a key with that character + key = unichr(unicode_char) + + # Make the key safe + key = safe_key(key, '', '') + + # The key should now be valid + self.assertTrue(self._is_valid_key(key), + msg="Failed for unicode character {0}".format(unicode_char)) + + def test_safe_key_prefix_unicode(self): + + for unicode_char in self.UNICODE_CHAR_CODES: + + # Generate a prefix with that character + prefix = unichr(unicode_char) + + # Make the key safe + key = safe_key('test', prefix, '') + + # The key should now be valid + self.assertTrue(self._is_valid_key(key), + msg="Failed for unicode character {0}".format(unicode_char)) + + def test_safe_key_version_unicode(self): + + for unicode_char in self.UNICODE_CHAR_CODES: + + # Generate a version with that character + version = unichr(unicode_char) + + # Make the key safe + key = safe_key('test', '', version) + + # The key should now be valid + self.assertTrue(self._is_valid_key(key), + msg="Failed for unicode character {0}".format(unicode_char)) + + def _is_valid_key(self, key): + """ + Test that a key is memcache-compatible. + Based on Django's validator in core.cache.backends.base + """ + + # Check the length + if len(key) > 250: + return False + + # Check that there are no spaces or control characters + for char in key: + if ord(char) < 33 or ord(char) == 127: + return False + + return True diff --git a/common/djangoapps/util/tests.py b/common/djangoapps/util/tests/test_zendesk.py similarity index 99% rename from common/djangoapps/util/tests.py rename to common/djangoapps/util/tests/test_zendesk.py index d829676eaf..51d06a92ed 100644 --- a/common/djangoapps/util/tests.py +++ b/common/djangoapps/util/tests/test_zendesk.py @@ -1,4 +1,4 @@ -"""Tests for the util package""" +"""Tests for the Zendesk""" from django.conf import settings from django.contrib.auth.models import AnonymousUser diff --git a/common/lib/xmodule/xmodule/combined_open_ended_module.py b/common/lib/xmodule/xmodule/combined_open_ended_module.py index 239adcaa41..67ff206e89 100644 --- a/common/lib/xmodule/xmodule/combined_open_ended_module.py +++ b/common/lib/xmodule/xmodule/combined_open_ended_module.py @@ -203,9 +203,7 @@ class CombinedOpenEndedModule(CombinedOpenEndedFields, XModule): def save_instance_data(self): for attribute in self.student_attributes: - child_attr = getattr(self.child_module, attribute) - if child_attr != getattr(self, attribute): - setattr(self, attribute, getattr(self.child_module, attribute)) + setattr(self, attribute, getattr(self.child_module, attribute)) class CombinedOpenEndedDescriptor(CombinedOpenEndedFields, RawDescriptor): diff --git a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee index 45c678bad9..48980c7d88 100644 --- a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee +++ b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee @@ -8,20 +8,23 @@ class @PeerGrading @use_single_location = @peer_grading_container.data('use-single-location') @peer_grading_outer_container = $('.peer-grading-container') @ajax_url = @peer_grading_container.data('ajax-url') - @error_container = $('.error-container') - @error_container.toggle(not @error_container.is(':empty')) - @message_container = $('.message-container') - @message_container.toggle(not @message_container.is(':empty')) - - @problem_button = $('.problem-button') - @problem_button.click @show_results - - @problem_list = $('.problem-list') - @construct_progress_bar() - - if @use_single_location + if @use_single_location.toLowerCase() == "true" + #If the peer grading element is linked to a single location, then activate the backend for that location @activate_problem() + else + #Otherwise, activate the panel view. + @error_container = $('.error-container') + @error_container.toggle(not @error_container.is(':empty')) + + @message_container = $('.message-container') + @message_container.toggle(not @message_container.is(':empty')) + + @problem_button = $('.problem-button') + @problem_button.click @show_results + + @problem_list = $('.problem-list') + @construct_progress_bar() construct_progress_bar: () => problems = @problem_list.find('tr').next() diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index 35f2fa2d76..1ad31922f5 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/common/lib/xmodule/xmodule/peer_grading_module.py @@ -11,7 +11,7 @@ from xmodule.raw_module import RawDescriptor from xmodule.modulestore.django import modulestore from .timeinfo import TimeInfo from xblock.core import Object, Integer, Boolean, String, Scope -from xmodule.fields import Date, StringyFloat +from xmodule.fields import Date, StringyFloat, StringyInteger, StringyBoolean from xmodule.open_ended_grading_classes.peer_grading_service import PeerGradingService, GradingServiceError, MockPeerGradingService from open_ended_grading_classes import combined_open_ended_rubric @@ -28,14 +28,14 @@ EXTERNAL_GRADER_NO_CONTACT_ERROR = "Failed to contact external graders. Please class PeerGradingFields(object): - use_for_single_location = Boolean(help="Whether to use this for a single location or as a panel.", + use_for_single_location = StringyBoolean(help="Whether to use this for a single location or as a panel.", default=USE_FOR_SINGLE_LOCATION, scope=Scope.settings) link_to_location = String(help="The location this problem is linked to.", default=LINK_TO_LOCATION, scope=Scope.settings) - is_graded = Boolean(help="Whether or not this module is scored.", default=IS_GRADED, scope=Scope.settings) + is_graded = StringyBoolean(help="Whether or not this module is scored.", default=IS_GRADED, scope=Scope.settings) due_date = Date(help="Due date that should be displayed.", default=None, scope=Scope.settings) grace_period_string = String(help="Amount of grace to give on the due date.", default=None, scope=Scope.settings) - max_grade = Integer(help="The maximum grade that a student can receieve for this problem.", default=MAX_SCORE, + max_grade = StringyInteger(help="The maximum grade that a student can receieve for this problem.", default=MAX_SCORE, scope=Scope.settings) student_data_for_location = Object(help="Student data for a given peer grading problem.", scope=Scope.user_state) @@ -93,9 +93,9 @@ class PeerGradingModule(PeerGradingFields, XModule): if not self.ajax_url.endswith("/"): self.ajax_url = self.ajax_url + "/" - if not isinstance(self.max_grade, (int, long)): - #This could result in an exception, but not wrapping in a try catch block so it moves up the stack - self.max_grade = int(self.max_grade) + #StringyInteger could return None, so keep this check. + if not isinstance(self.max_grade, int): + raise TypeError("max_grade needs to be an integer.") def closed(self): return self._closed(self.timeinfo) diff --git a/lms/djangoapps/open_ended_grading/open_ended_notifications.py b/lms/djangoapps/open_ended_grading/open_ended_notifications.py index 6d5f2a3eb4..1d6fa22929 100644 --- a/lms/djangoapps/open_ended_grading/open_ended_notifications.py +++ b/lms/djangoapps/open_ended_grading/open_ended_notifications.py @@ -11,6 +11,7 @@ from util.cache import cache import datetime from xmodule.x_module import ModuleSystem from mitxmako.shortcuts import render_to_string +import datetime log = logging.getLogger(__name__) @@ -104,6 +105,25 @@ def peer_grading_notifications(course, user): def combined_notifications(course, user): + """ + Show notifications to a given user for a given course. Get notifications from the cache if possible, + or from the grading controller server if not. + @param course: The course object for which we are getting notifications + @param user: The user object for which we are getting notifications + @return: A dictionary with boolean pending_grading (true if there is pending grading), img_path (for notification + image), and response (actual response from grading controller server). + """ + #Set up return values so that we can return them for error cases + pending_grading = False + img_path = "" + notifications={} + notification_dict = {'pending_grading': pending_grading, 'img_path': img_path, 'response': notifications} + + #We don't want to show anonymous users anything. + if not user.is_authenticated(): + return notification_dict + + #Define a mock modulesystem system = ModuleSystem( ajax_url=None, track_function=None, @@ -112,41 +132,44 @@ def combined_notifications(course, user): replace_urls=None, xblock_model_data= {} ) + #Initialize controller query service using our mock system controller_qs = ControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, system) student_id = unique_id_for_user(user) user_is_staff = has_access(user, course, 'staff') course_id = course.id notification_type = "combined" + #See if we have a stored value in the cache success, notification_dict = get_value_from_cache(student_id, course_id, notification_type) if success: return notification_dict - min_time_to_query = user.last_login + #Get the time of the last login of the user + last_login = user.last_login + + #Find the modules they have seen since they logged in last_module_seen = StudentModule.objects.filter(student=user, course_id=course_id, - modified__gt=min_time_to_query).values('modified').order_by( + modified__gt=last_login).values('modified').order_by( '-modified') last_module_seen_count = last_module_seen.count() if last_module_seen_count > 0: + #The last time they viewed an updated notification (last module seen minus how long notifications are cached) last_time_viewed = last_module_seen[0]['modified'] - datetime.timedelta(seconds=(NOTIFICATION_CACHE_TIME + 60)) else: - last_time_viewed = user.last_login + #If they have not seen any modules since they logged in, then don't refresh + return {'pending_grading': False, 'img_path': img_path, 'response': notifications} - pending_grading = False - - img_path = "" try: + #Get the notifications from the grading controller controller_response = controller_qs.check_combined_notifications(course.id, student_id, user_is_staff, last_time_viewed) - log.debug(controller_response) notifications = json.loads(controller_response) if notifications['success']: if notifications['overall_need_to_check']: pending_grading = True except: #Non catastrophic error, so no real action - notifications = {} #This is a dev_facing_error log.exception( "Problem with getting notifications from controller query service for course {0} user {1}.".format( @@ -157,6 +180,7 @@ def combined_notifications(course, user): notification_dict = {'pending_grading': pending_grading, 'img_path': img_path, 'response': notifications} + #Store the notifications in the cache set_value_in_cache(student_id, course_id, notification_type, notification_dict) return notification_dict diff --git a/lms/static/sass/shared/_modal.scss b/lms/static/sass/shared/_modal.scss index 2da64d54a6..8ff58c1c14 100644 --- a/lms/static/sass/shared/_modal.scss +++ b/lms/static/sass/shared/_modal.scss @@ -149,7 +149,7 @@ } label { - color: #999; + color: #646464; &.field-error { display: block; diff --git a/lms/templates/courseware/courseware.html b/lms/templates/courseware/courseware.html index 33dc9562a7..4889da25ca 100644 --- a/lms/templates/courseware/courseware.html +++ b/lms/templates/courseware/courseware.html @@ -156,7 +156,7 @@
- +
Hints @@ -176,8 +176,8 @@
- - + +
diff --git a/lms/templates/forgot_password_modal.html b/lms/templates/forgot_password_modal.html index 97b41d4be9..f0f571a817 100644 --- a/lms/templates/forgot_password_modal.html +++ b/lms/templates/forgot_password_modal.html @@ -12,19 +12,19 @@
- - + +
-
+

✕

-
+ @@ -40,5 +40,10 @@ $('#pwd_error').stop().css("display", "block"); } }); + + // removing close link's default behavior + $('#login-modal .close-modal').click(function(e) { + e.preventDefault(); + }); })(this) diff --git a/lms/templates/login_modal.html b/lms/templates/login_modal.html index 1587cca767..de1c437caf 100644 --- a/lms/templates/login_modal.html +++ b/lms/templates/login_modal.html @@ -9,14 +9,17 @@
- - - - -