From 594d28a6537170ea28d32c2684a940ca24bc319b Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 6 May 2013 16:03:41 -0400 Subject: [PATCH 01/14] Pass default value through. --- cms/templates/widgets/metadata-edit.html | 10 +-- .../xmodule/modulestore/inheritance.py | 13 +++- .../lib/xmodule/xmodule/tests/test_import.py | 58 +++++++++++++++++ .../xmodule/xmodule/tests/test_xml_module.py | 62 ++++++++++++------- common/lib/xmodule/xmodule/x_module.py | 21 ++++--- github-requirements.txt | 2 +- 6 files changed, 124 insertions(+), 42 deletions(-) diff --git a/cms/templates/widgets/metadata-edit.html b/cms/templates/widgets/metadata-edit.html index 9693c18e9c..aada438f38 100644 --- a/cms/templates/widgets/metadata-edit.html +++ b/cms/templates/widgets/metadata-edit.html @@ -8,7 +8,7 @@ % for field_name, field_value in editable_metadata_fields.items():
  • % if field_name == 'source_code': - % if field_value['is_default'] is False: + % if field_value['explicitly_set'] is True: Edit High Level Source % endif % else: @@ -26,8 +26,10 @@ % if False: - - + + + + % if field_value['field'].values: % for value in field_value['field'].values: @@ -40,7 +42,7 @@ % endfor - % if 'source_code' in editable_metadata_fields and not editable_metadata_fields['source_code']['is_default']: + % if 'source_code' in editable_metadata_fields and editable_metadata_fields['source_code']['explicitly_set']: <%include file="source-edit.html" /> % endif diff --git a/common/lib/xmodule/xmodule/modulestore/inheritance.py b/common/lib/xmodule/xmodule/modulestore/inheritance.py index 62b351999d..a816aa9776 100644 --- a/common/lib/xmodule/xmodule/modulestore/inheritance.py +++ b/common/lib/xmodule/xmodule/modulestore/inheritance.py @@ -31,15 +31,22 @@ def inherit_metadata(descriptor, model_data): Only metadata specified in self.inheritable_metadata will be inherited """ + # The inherited values that are actually being used. if not hasattr(descriptor, '_inherited_metadata'): setattr(descriptor, '_inherited_metadata', {}) + # All inheritable metadata values (for which a value exists in model_data). + if not hasattr(descriptor, '_inheritable_metadata'): + setattr(descriptor, '_inheritable_metadata', {}) + # Set all inheritable metadata from kwargs that are # in self.inheritable_metadata and aren't already set in metadata for attr in INHERITABLE_METADATA: - if attr not in descriptor._model_data and attr in model_data: - descriptor._inherited_metadata[attr] = model_data[attr] - descriptor._model_data[attr] = model_data[attr] + if attr in model_data: + descriptor._inheritable_metadata[attr] = model_data[attr] + if attr not in descriptor._model_data: + descriptor._inherited_metadata[attr] = model_data[attr] + descriptor._model_data[attr] = model_data[attr] def own_metadata(module): diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 9d73fdcc17..aedca1d058 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -151,6 +151,10 @@ class ImportTestCase(BaseCourseTestCase): # Check that the child inherits due correctly child = descriptor.get_children()[0] self.assertEqual(child.lms.due, Date().from_json(v)) + self.assertEqual(child._inheritable_metadata, child._inherited_metadata) + self.assertEqual(2, len(child._inherited_metadata)) + self.assertEqual('1970-01-01T00:00:00Z', child._inherited_metadata['start']) + self.assertEqual(v, child._inherited_metadata['due']) # Now export and check things resource_fs = MemoryFS() @@ -184,6 +188,60 @@ class ImportTestCase(BaseCourseTestCase): self.assertEqual(chapter_xml.tag, 'chapter') self.assertFalse('due' in chapter_xml.attrib) + def test_metadata_no_inheritance(self): + """ + Checks that default value of None (for due) does not get marked as inherited. + """ + system = self.get_system() + url_name = 'test1' + start_xml = ''' + + + Two houses, ... + + '''.format(org=ORG, course=COURSE, url_name=url_name) + descriptor = system.process_xml(start_xml) + compute_inherited_metadata(descriptor) + + self.assertEqual(descriptor.lms.due, None) + + # Check that the child does not inherit a value for due + child = descriptor.get_children()[0] + self.assertEqual(child.lms.due, None) + self.assertEqual(child._inheritable_metadata, child._inherited_metadata) + self.assertEqual(1, len(child._inherited_metadata)) + self.assertEqual('1970-01-01T00:00:00Z', child._inherited_metadata['start']) + + def test_metadata_override_default(self): + """ + Checks that due date can be overriden at child level. + """ + system = self.get_system() + course_due = 'March 20 17:00' + child_due = 'April 10 00:00' + url_name = 'test1' + start_xml = ''' + + + Two houses, ... + + '''.format(due=course_due, org=ORG, course=COURSE, url_name=url_name) + descriptor = system.process_xml(start_xml) + child = descriptor.get_children()[0] + child._model_data['due'] = child_due + compute_inherited_metadata(descriptor) + + self.assertEqual(descriptor.lms.due, Date().from_json(course_due)) + self.assertEqual(child.lms.due, Date().from_json(child_due)) + # Test inherited metadata. Due does not appear here (because explicitly set on child). + self.assertEqual(1, len(child._inherited_metadata)) + self.assertEqual('1970-01-01T00:00:00Z', child._inherited_metadata['start']) + # Test inheritable metadata. This has the course inheritable value for due. + self.assertEqual(2, len(child._inheritable_metadata)) + self.assertEqual(course_due, child._inheritable_metadata['due']) + def test_is_pointer_tag(self): """ Check that is_pointer_tag works properly. diff --git a/common/lib/xmodule/xmodule/tests/test_xml_module.py b/common/lib/xmodule/xmodule/tests/test_xml_module.py index 06d5b0b0a3..e41bcdd73a 100644 --- a/common/lib/xmodule/xmodule/tests/test_xml_module.py +++ b/common/lib/xmodule/xmodule/tests/test_xml_module.py @@ -9,13 +9,13 @@ from mock import Mock class TestFields(object): # Will be returned by editable_metadata_fields. - max_attempts = StringyInteger(scope=Scope.settings) + max_attempts = StringyInteger(scope=Scope.settings, default=1000) # Will not be returned by editable_metadata_fields because filtered out by non_editable_metadata_fields. due = Date(scope=Scope.settings) # Will not be returned by editable_metadata_fields because is not Scope.settings. student_answers = Object(scope=Scope.user_state) # Will be returned, and can override the inherited value from XModule. - display_name = String(scope=Scope.settings) + display_name = String(scope=Scope.settings, default='local default') class EditableMetadataFieldsTest(unittest.TestCase): @@ -25,27 +25,45 @@ class EditableMetadataFieldsTest(unittest.TestCase): # Tests that the xblock fields (currently tags and name) get filtered out. # Also tests that xml_attributes is filtered out of XmlDescriptor. self.assertEqual(1, len(editable_fields), "Expected only 1 editable field for xml descriptor.") - self.assert_display_name_default(editable_fields) + self.assert_field_values(editable_fields, 'display_name', XModuleFields.display_name, + explicitly_set=False, inheritable=False, value=None, default_value=None) def test_override_default(self): - # Tests that is_default is correct when a value overrides the default. + # Tests that explicitly_set is correct when a value overrides the default (not inheritable). editable_fields = self.get_xml_editable_fields({'display_name': 'foo'}) - display_name = editable_fields['display_name'] - self.assertFalse(display_name['is_default']) - self.assertEqual('foo', display_name['value']) + self.assert_field_values(editable_fields, 'display_name', XModuleFields.display_name, + explicitly_set=True, inheritable=False, value='foo', default_value=None) def test_additional_field(self): - editable_fields = self.get_module_editable_fields({'max_attempts' : '7'}) + descriptor = self.get_descriptor({'max_attempts' : '7'}) + editable_fields = descriptor.editable_metadata_fields self.assertEqual(2, len(editable_fields)) - self.assert_field_values(editable_fields, 'max_attempts', TestFields.max_attempts, False, False, 7) - self.assert_display_name_default(editable_fields) + self.assert_field_values(editable_fields, 'max_attempts', TestFields.max_attempts, + explicitly_set=True, inheritable=False, value=7, default_value=1000) + self.assert_field_values(editable_fields, 'display_name', XModuleFields.display_name, + explicitly_set=False, inheritable=False, value='local default', default_value='local default') - editable_fields = self.get_module_editable_fields({}) - self.assert_field_values(editable_fields, 'max_attempts', TestFields.max_attempts, True, False, None) + editable_fields = self.get_descriptor({}).editable_metadata_fields + self.assert_field_values(editable_fields, 'max_attempts', TestFields.max_attempts, + explicitly_set=False, inheritable=False, value=1000, default_value=1000) def test_inherited_field(self): - editable_fields = self.get_module_editable_fields({'display_name' : 'inherited'}) - self.assert_field_values(editable_fields, 'display_name', XModuleFields.display_name, False, True, 'inherited') + model_val = {'display_name' : 'inherited'} + descriptor = self.get_descriptor(model_val) + # Mimic an inherited value for display_name (inherited and inheritable are the same in this case). + descriptor._inherited_metadata = model_val + descriptor._inheritable_metadata = model_val + editable_fields = descriptor.editable_metadata_fields + self.assert_field_values(editable_fields, 'display_name', XModuleFields.display_name, + explicitly_set=False, inheritable=True, value='inherited', default_value='inherited') + + descriptor = self.get_descriptor({'display_name' : 'explicit'}) + # Mimic the case where display_name WOULD have been inherited, except we explicitly set it. + descriptor._inheritable_metadata = {'display_name' : 'inheritable value'} + descriptor._inherited_metadata = {} + editable_fields = descriptor.editable_metadata_fields + self.assert_field_values(editable_fields, 'display_name', XModuleFields.display_name, + explicitly_set=True, inheritable=True, value='explicit', default_value='inheritable value') # Start of helper methods def get_xml_editable_fields(self, model_data): @@ -53,7 +71,7 @@ class EditableMetadataFieldsTest(unittest.TestCase): system.render_template = Mock(return_value="
    Test Template HTML
    ") return XmlDescriptor(system=system, location=None, model_data=model_data).editable_metadata_fields - def get_module_editable_fields(self, model_data): + def get_descriptor(self, model_data): class TestModuleDescriptor(TestFields, XmlDescriptor): @property @@ -64,16 +82,12 @@ class EditableMetadataFieldsTest(unittest.TestCase): system = test_system() system.render_template = Mock(return_value="
    Test Template HTML
    ") - descriptor = TestModuleDescriptor(system=system, location=None, model_data=model_data) - descriptor._inherited_metadata = {'display_name' : 'inherited'} - return descriptor.editable_metadata_fields + return TestModuleDescriptor(system=system, location=None, model_data=model_data) - def assert_display_name_default(self, editable_fields): - self.assert_field_values(editable_fields, 'display_name', XModuleFields.display_name, True, False, None) - - def assert_field_values(self, editable_fields, name, field, is_default, is_inherited, value): + def assert_field_values(self, editable_fields, name, field, explicitly_set, inheritable, value, default_value): test_field = editable_fields[name] self.assertEqual(field, test_field['field']) - self.assertEqual(is_default, test_field['is_default']) - self.assertEqual(is_inherited, test_field['is_inherited']) + self.assertEqual(explicitly_set, test_field['explicitly_set']) + self.assertEqual(inheritable, test_field['inheritable']) self.assertEqual(value, test_field['value']) + self.assertEqual(default_value, test_field['default_value']) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 749ca66258..7c24d593e3 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -624,27 +624,28 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock): Can be limited by extending `non_editable_metadata_fields`. """ inherited_metadata = getattr(self, '_inherited_metadata', {}) + inheritable_metadata = getattr(self, '_inheritable_metadata', {}) metadata = {} for field in self.fields: if field.scope != Scope.settings or field in self.non_editable_metadata_fields: continue - inherited = False - default = False + inheritable = False value = getattr(self, field.name) - if field.name in self._model_data: - default = False + default_value = field.default + explicitly_set = field.name in self._model_data + if field.name in inheritable_metadata: + inheritable = True + default_value = field.from_json(inheritable_metadata.get(field.name)) if field.name in inherited_metadata: - if self._model_data.get(field.name) == inherited_metadata.get(field.name): - inherited = True - else: - default = True + explicitly_set = False metadata[field.name] = {'field': field, 'value': value, - 'is_inherited': inherited, - 'is_default': default} + 'default_value': default_value, + 'inheritable': inheritable, + 'explicitly_set': explicitly_set } return metadata diff --git a/github-requirements.txt b/github-requirements.txt index 3b71d228e7..ca2669789a 100644 --- a/github-requirements.txt +++ b/github-requirements.txt @@ -7,4 +7,4 @@ -e git://github.com/dementrock/pystache_custom.git@776973740bdaad83a3b029f96e415a7d1e8bec2f#egg=pystache_custom-dev # Our libraries: --e git+https://github.com/edx/XBlock.git@5ce6f70a#egg=XBlock +-e git+https://github.com/edx/XBlock.git@483e0cb1#egg=XBlock From 1b0b365fa6816b32b3f625709bcb21b8a5f6235f Mon Sep 17 00:00:00 2001 From: Will Daly Date: Mon, 6 May 2013 17:08:46 -0400 Subject: [PATCH 02/14] Added unit tests for safe_key() to resolve bug 392. Updated safe_key() so that it: (a) avoids creating keys that are too long for memcache, and (b) handles unicode in keys, prefixes, and versions Added __init__.py, which should have been in the last commit Pep8/Pylint fixes --- common/djangoapps/util/memcache.py | 43 +++++-- common/djangoapps/util/tests/__init__.py | 1 + common/djangoapps/util/tests/test_memcache.py | 114 ++++++++++++++++++ .../util/{tests.py => tests/test_zendesk.py} | 2 +- 4 files changed, 152 insertions(+), 8 deletions(-) create mode 100644 common/djangoapps/util/tests/__init__.py create mode 100644 common/djangoapps/util/tests/test_memcache.py rename common/djangoapps/util/{tests.py => tests/test_zendesk.py} (99%) diff --git a/common/djangoapps/util/memcache.py b/common/djangoapps/util/memcache.py index 540cf96539..db921d9845 100644 --- a/common/djangoapps/util/memcache.py +++ b/common/djangoapps/util/memcache.py @@ -8,15 +8,44 @@ 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 the key + # and combine the parts again + if len(combined) > 250: + key = fasthash(key) + combined = ":".join([key_prefix, version, key]) + + # 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..85b60c75f1 --- /dev/null +++ b/common/djangoapps/util/tests/test_memcache.py @@ -0,0 +1,114 @@ +""" +Tests for memcache in util app +""" + +from django.test import TestCase +from django.core.cache import get_cache +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): + + key = safe_key('a' * 300, 'prefix', 'version') + self.assertEqual(key[0:15], 'prefix:version:') + + 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 From 258aebed2031280c83dc065dd73224eae75a4248 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 7 May 2013 12:58:49 -0400 Subject: [PATCH 03/14] Fix anonymoususer 500 error --- .../open_ended_grading/open_ended_notifications.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/open_ended_grading/open_ended_notifications.py b/lms/djangoapps/open_ended_grading/open_ended_notifications.py index 6d5f2a3eb4..0f97ea2a85 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__) @@ -121,17 +122,20 @@ def combined_notifications(course, user): success, notification_dict = get_value_from_cache(student_id, course_id, notification_type) if success: return notification_dict + if user.is_authenticated(): + last_login = user.last_login + else: + last_login = datetime.datetime.now() - min_time_to_query = user.last_login 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: last_time_viewed = last_module_seen[0]['modified'] - datetime.timedelta(seconds=(NOTIFICATION_CACHE_TIME + 60)) else: - last_time_viewed = user.last_login + last_time_viewed = last_login pending_grading = False From 9e03280f5065124fd2b8edcc8ecb835f80f82e0c Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 7 May 2013 15:33:15 -0400 Subject: [PATCH 04/14] Make peer grading fields stringy and fix js to avoid strange error --- .../js/src/peergrading/peer_grading.coffee | 23 ++++++++++--------- .../peergrading/peer_grading_problem.coffee | 1 - .../xmodule/xmodule/peer_grading_module.py | 12 +++++----- 3 files changed, 18 insertions(+), 18 deletions(-) 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..2ce7a09b92 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,21 @@ 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 @activate_problem() + else + @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/js/src/peergrading/peer_grading_problem.coffee b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee index 9483932f80..001ef93001 100644 --- a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee +++ b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee @@ -27,7 +27,6 @@ class @PeerGradingProblemBackend else # if this post request fails, the error callback will catch it $.post(@ajax_url + cmd, data, callback) - .error => callback({success: false, error: "Error occured while performing this operation"}) mock: (cmd, data) -> if cmd == 'is_student_calibrated' diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index 35f2fa2d76..cf10e7c87c 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)): + if not isinstance(self.max_grade, int): #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) + raise TypeError("max_grade needs to be an integer.") def closed(self): return self._closed(self.timeinfo) From 1398b55713433adc0baca4bb5e957e1fe51602ee Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 7 May 2013 15:48:19 -0400 Subject: [PATCH 05/14] Comment touched modules --- .../js/src/peergrading/peer_grading.coffee | 2 + .../xmodule/xmodule/peer_grading_module.py | 2 +- .../open_ended_notifications.py | 40 ++++++++++++++----- 3 files changed, 33 insertions(+), 11 deletions(-) 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 2ce7a09b92..676cc75d11 100644 --- a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee +++ b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee @@ -10,8 +10,10 @@ class @PeerGrading @ajax_url = @peer_grading_container.data('ajax-url') if @use_single_location + #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')) diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index cf10e7c87c..bb14eec8b5 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/common/lib/xmodule/xmodule/peer_grading_module.py @@ -93,8 +93,8 @@ class PeerGradingModule(PeerGradingFields, XModule): if not self.ajax_url.endswith("/"): self.ajax_url = self.ajax_url + "/" + #This could result in an exception, but not wrapping in a try catch block so it moves up the stack if not isinstance(self.max_grade, int): - #This could result in an exception, but not wrapping in a try catch block so it moves up the stack raise TypeError("max_grade needs to be an integer.") def closed(self): diff --git a/lms/djangoapps/open_ended_grading/open_ended_notifications.py b/lms/djangoapps/open_ended_grading/open_ended_notifications.py index 0f97ea2a85..1d6fa22929 100644 --- a/lms/djangoapps/open_ended_grading/open_ended_notifications.py +++ b/lms/djangoapps/open_ended_grading/open_ended_notifications.py @@ -105,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, @@ -113,44 +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 - if user.is_authenticated(): - last_login = user.last_login - else: - last_login = datetime.datetime.now() + #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=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 = 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( @@ -161,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 From aaa383b8ca714b0f84db959339fe9a1496e76d78 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 7 May 2013 15:48:51 -0400 Subject: [PATCH 06/14] safe_key() now hashes the prefix/version as well, just in case these are configured to be too long in the settings. --- common/djangoapps/util/memcache.py | 6 ++---- common/djangoapps/util/tests/test_memcache.py | 12 +++++++++++- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/common/djangoapps/util/memcache.py b/common/djangoapps/util/memcache.py index db921d9845..ee450e68cb 100644 --- a/common/djangoapps/util/memcache.py +++ b/common/djangoapps/util/memcache.py @@ -41,11 +41,9 @@ def safe_key(key, key_prefix, 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 the key - # and combine the parts again + # If the total length is too long for memcache, hash it if len(combined) > 250: - key = fasthash(key) - combined = ":".join([key_prefix, version, key]) + combined = fasthash(combined) # Return the result return combined diff --git a/common/djangoapps/util/tests/test_memcache.py b/common/djangoapps/util/tests/test_memcache.py index 85b60c75f1..de8d352c38 100644 --- a/common/djangoapps/util/tests/test_memcache.py +++ b/common/djangoapps/util/tests/test_memcache.py @@ -4,6 +4,7 @@ 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 @@ -51,8 +52,17 @@ class MemcacheTest(TestCase): def test_long_key_prefix_version(self): + # Long key key = safe_key('a' * 300, 'prefix', 'version') - self.assertEqual(key[0:15], '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): From c6f63140a27a517846ce34a9ceb3cf4a5f052203 Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Tue, 7 May 2013 11:42:39 -0400 Subject: [PATCH 07/14] Hide the API key during logging. --- lms/lib/comment_client/utils.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lms/lib/comment_client/utils.py b/lms/lib/comment_client/utils.py index 860035dc06..53bdd462ad 100644 --- a/lms/lib/comment_client/utils.py +++ b/lms/lib/comment_client/utils.py @@ -37,6 +37,10 @@ def perform_request(method, url, data_or_params=None, *args, **kwargs): else: response = requests.request(method, url, params=data_or_params, timeout=5) except Exception as err: + # remove API key if it is in the params + if 'api_key' in data_or_params: + log.info('Deleting API key from params') + del data_or_params['api_key'] log.exception("Trying to call {method} on {url} with params {params}".format( method=method, url=url, params=data_or_params)) # Reraise with a single exception type From e0d1eca6aa387196531300a9c96bcb11ed80e895 Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 7 May 2013 16:56:08 -0400 Subject: [PATCH 08/14] Delete two lines. without this, the xblock fields are never created in cases where the module is "fresh" (ie has no existing data) --- common/lib/xmodule/xmodule/combined_open_ended_module.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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): From 2648a19cc2b6c9e7449c583575539d31c2aba04e Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 7 May 2013 17:08:14 -0400 Subject: [PATCH 09/14] Fix check for use for single location --- .../lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 676cc75d11..48980c7d88 100644 --- a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee +++ b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading.coffee @@ -9,7 +9,7 @@ class @PeerGrading @peer_grading_outer_container = $('.peer-grading-container') @ajax_url = @peer_grading_container.data('ajax-url') - 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 From 530ac51c1c2c7fcd784e1c7bfac844feae296f3f Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 7 May 2013 17:33:34 -0400 Subject: [PATCH 10/14] Add .error callback back in --- .../xmodule/js/src/peergrading/peer_grading_problem.coffee | 1 + 1 file changed, 1 insertion(+) diff --git a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee index 001ef93001..9483932f80 100644 --- a/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee +++ b/common/lib/xmodule/xmodule/js/src/peergrading/peer_grading_problem.coffee @@ -27,6 +27,7 @@ class @PeerGradingProblemBackend else # if this post request fails, the error callback will catch it $.post(@ajax_url + cmd, data, callback) + .error => callback({success: false, error: "Error occured while performing this operation"}) mock: (cmd, data) -> if cmd == 'is_student_calibrated' From 5cd9641f24ef49e1084b78db3d59cca2370f296a Mon Sep 17 00:00:00 2001 From: Vik Paruchuri Date: Tue, 7 May 2013 17:35:00 -0400 Subject: [PATCH 11/14] Update peer grading comment --- common/lib/xmodule/xmodule/peer_grading_module.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index bb14eec8b5..1ad31922f5 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/common/lib/xmodule/xmodule/peer_grading_module.py @@ -93,7 +93,7 @@ class PeerGradingModule(PeerGradingFields, XModule): if not self.ajax_url.endswith("/"): self.ajax_url = self.ajax_url + "/" - #This could result in an exception, but not wrapping in a try catch block so it moves up the stack + #StringyInteger could return None, so keep this check. if not isinstance(self.max_grade, int): raise TypeError("max_grade needs to be an integer.") From 25dd2c7d678574ed1c2b41b998b2e41b8bb8d820 Mon Sep 17 00:00:00 2001 From: Brian Talbot Date: Wed, 8 May 2013 15:30:17 -0400 Subject: [PATCH 12/14] studio - alerts: synced up example HTML to show advanced settings live notification, aria roles, and action classes; integrated typography mixins --- cms/static/sass/elements/_alerts.scss | 24 ++--- cms/static/sass/elements/_typography.scss | 6 +- cms/templates/ux-alerts.html | 111 +++++++++++++++++++--- 3 files changed, 114 insertions(+), 27 deletions(-) diff --git a/cms/static/sass/elements/_alerts.scss b/cms/static/sass/elements/_alerts.scss index 49aa015313..1e8b79b3fe 100644 --- a/cms/static/sass/elements/_alerts.scss +++ b/cms/static/sass/elements/_alerts.scss @@ -6,7 +6,7 @@ @include box-sizing(border-box); .copy { - @include font-size(13); + @extend .t-copy-sub2; } } @@ -184,12 +184,12 @@ } .action-primary { - @include font-size(13); + @extend .t-action3; font-weight: 600; } .action-secondary { - @include font-size(13); + @extend .t-action3; } } } @@ -367,12 +367,12 @@ } .copy { - @include font-size(13); + @extend .t-copy-sub2; width: flex-grid(10, 12); color: $gray-l2; .title { - @include font-size(14); + @extend .t-title-4; margin-bottom: 0; color: $white; } @@ -409,13 +409,13 @@ .action-primary { @include blue-button(); - @include font-size(13); + @extend .t-action3; border-color: $blue-d2; font-weight: 600; } .action-secondary { - @include font-size(13); + @extend .t-action3; } } @@ -504,7 +504,7 @@ // adopted alerts .alert { - @include font-size(14); + @extend .t-copy-sub2; @include box-sizing(border-box); @include clearfix(); margin: 0 auto; @@ -530,7 +530,7 @@ } .copy { - @include font-size(13); + @extend .t-copy-sub2; width: flex-grid(10, 12); color: $gray-l2; @@ -568,12 +568,12 @@ } .action-primary { - @include font-size(13); + @extend .t-action3; font-weight: 600; } .action-secondary { - @include font-size(13); + @extend .t-action3; } } } @@ -730,7 +730,7 @@ body.uxdesign.alerts { border-radius: 3px; background: #fbf6e1; // background: #edbd3c; - font-size: 14px; + @extend .t-copy-sub1; @include clearfix; .alert-message { diff --git a/cms/static/sass/elements/_typography.scss b/cms/static/sass/elements/_typography.scss index 32c4b3928b..a58fe27eb8 100644 --- a/cms/static/sass/elements/_typography.scss +++ b/cms/static/sass/elements/_typography.scss @@ -2,7 +2,7 @@ // ==================== // headings/titles -.t-title-1, .t-title-2, .t-title-3, .t-title-4, .t-title-5, .t-title-5 { +.t-title-1, .t-title-2, .t-title-3, .t-title-4, .t-title-5 { color: $gray-d3; } @@ -21,7 +21,7 @@ } .t-title-4 { - + @include font-size(14); } .t-title-5 { @@ -82,4 +82,4 @@ // misc .t-icon { line-height: 0; -} \ No newline at end of file +} diff --git a/cms/templates/ux-alerts.html b/cms/templates/ux-alerts.html index de062e471e..b9c5fd6053 100644 --- a/cms/templates/ux-alerts.html +++ b/cms/templates/ux-alerts.html @@ -114,6 +114,7 @@
  • Show Announcement
  • Show Announcement with Actions
  • Show Activiation
    • +
  • Alert with three actions
    • @@ -128,6 +129,10 @@

    Different Static Examples of Notifications

    @@ -182,6 +191,33 @@ <%block name="view_alerts"> + +
    +
    + ⚠ + +
    +

    You are editing a draft

    +

    Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor. Aenean massa.

    +
    + + +
    +
    +
    @@ -196,10 +232,10 @@

    Alert Actions

    @@ -220,10 +256,10 @@

    Alert Actions

    @@ -297,7 +333,7 @@

    Alert Actions

    @@ -367,13 +403,13 @@ <%block name="view_notifications"> -
    + + + + + + +