From 594d28a6537170ea28d32c2684a940ca24bc319b Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 6 May 2013 16:03:41 -0400 Subject: [PATCH 1/2] 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 c6f63140a27a517846ce34a9ceb3cf4a5f052203 Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Tue, 7 May 2013 11:42:39 -0400 Subject: [PATCH 2/2] 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