diff --git a/cms/djangoapps/contentstore/tests/test_import.py b/cms/djangoapps/contentstore/tests/test_import.py index 41b26b5501..77bdd05527 100644 --- a/cms/djangoapps/contentstore/tests/test_import.py +++ b/cms/djangoapps/contentstore/tests/test_import.py @@ -179,3 +179,24 @@ class ContentStoreImportTest(ModuleStoreTestCase): u'i4x://testX/peergrading_copy/combinedopenended/SampleQuestion', peergrading_module.link_to_location ) + + def test_rewrite_reference_value_dict(self): + module_store = modulestore('direct') + target_location = Location(['i4x', 'testX', 'split_test_copy', 'course', 'copy_run']) + import_from_xml( + module_store, + 'common/test/data/', + ['split_test_module'], + target_location_namespace=target_location + ) + split_test_module = module_store.get_item( + Location(['i4x', 'testX', 'split_test_copy', 'split_test', 'split1']) + ) + self.assertIsNotNone(split_test_module) + self.assertEqual( + { + "0": "i4x://testX/split_test_copy/vertical/sample_0", + "2": "i4x://testX/split_test_copy/vertical/sample_2", + }, + split_test_module.group_id_to_child, + ) diff --git a/common/djangoapps/user_api/middleware.py b/common/djangoapps/user_api/middleware.py index 60757d095e..3a33db7143 100644 --- a/common/djangoapps/user_api/middleware.py +++ b/common/djangoapps/user_api/middleware.py @@ -1,8 +1,14 @@ +""" +Middleware for user api. +Adds user's tags to tracking event context. +""" from track.contexts import COURSE_REGEX from eventtracking import tracker from user_api.models import UserCourseTag + class UserTagsEventContextMiddleware(object): + """Middleware that adds a user's tags to tracking event context.""" CONTEXT_NAME = 'user_tags_context' def process_request(self, request): @@ -41,4 +47,4 @@ class UserTagsEventContextMiddleware(object): except: # pylint: disable=bare-except pass - return response \ No newline at end of file + return response diff --git a/common/djangoapps/user_api/models.py b/common/djangoapps/user_api/models.py index 48a85c2bc7..36ed30eddc 100644 --- a/common/djangoapps/user_api/models.py +++ b/common/djangoapps/user_api/models.py @@ -8,7 +8,7 @@ class UserPreference(models.Model): key = models.CharField(max_length=255, db_index=True) value = models.TextField() - class Meta: + class Meta: # pylint: disable=missing-docstring unique_together = ("user", "key") @classmethod @@ -45,5 +45,5 @@ class UserCourseTag(models.Model): course_id = models.CharField(max_length=255, db_index=True) value = models.TextField() - class Meta: + class Meta: # pylint: disable=missing-docstring unique_together = ("user", "course_id", "key") diff --git a/common/djangoapps/user_api/tests/factories.py b/common/djangoapps/user_api/tests/factories.py index 8a692cf994..535e888a59 100644 --- a/common/djangoapps/user_api/tests/factories.py +++ b/common/djangoapps/user_api/tests/factories.py @@ -1,9 +1,11 @@ +"""Provides factories for User API models.""" from factory.django import DjangoModelFactory from factory import SubFactory from student.tests.factories import UserFactory from user_api.models import UserPreference, UserCourseTag - +# Factories don't have __init__ methods, and are self documenting +# pylint: disable=W0232, C0111 class UserPreferenceFactory(DjangoModelFactory): FACTORY_FOR = UserPreference diff --git a/common/djangoapps/user_api/tests/test_middleware.py b/common/djangoapps/user_api/tests/test_middleware.py index 0a3c8a0ec4..290ca22048 100644 --- a/common/djangoapps/user_api/tests/test_middleware.py +++ b/common/djangoapps/user_api/tests/test_middleware.py @@ -1,7 +1,8 @@ +"""Tests for user API middleware""" from mock import Mock, patch from unittest import TestCase -from django.http import HttpRequest, HttpResponse +from django.http import HttpResponse from django.test.client import RequestFactory from student.tests.factories import UserFactory, AnonymousUserFactory @@ -41,7 +42,8 @@ class TagsMiddlewareTest(TestCase): self.assertEquals(self.middleware.process_request(self.request), None) def assertContextSetTo(self, context): - self.tracker.get_tracker.return_value.enter_context.assert_called_with( + """Asserts UserTagsEventContextMiddleware.CONTEXT_NAME matches ``context``""" + self.tracker.get_tracker.return_value.enter_context.assert_called_with( # pylint: disable=maybe-no-member UserTagsEventContextMiddleware.CONTEXT_NAME, context ) @@ -98,7 +100,7 @@ class TagsMiddlewareTest(TestCase): self.assertContextSetTo({'course_id': self.course_id, 'course_user_tags': {}}) def test_remove_context(self): - get_tracker = self.tracker.get_tracker + get_tracker = self.tracker.get_tracker # pylint: disable=maybe-no-member exit_context = get_tracker.return_value.exit_context # The middleware should clean up the context when the request is done diff --git a/common/djangoapps/user_api/user_service.py b/common/djangoapps/user_api/user_service.py index a6355cd1ef..5bfe7fe6f0 100644 --- a/common/djangoapps/user_api/user_service.py +++ b/common/djangoapps/user_api/user_service.py @@ -9,6 +9,10 @@ UserCourseTag model. from user_api.models import UserCourseTag +# Scopes +# (currently only allows per-course tags. Can be expanded to support +# global tags (e.g. using the existing UserPreferences table)) +COURSE_SCOPE = 'course' def get_course_tag(user, course_id, key): """ diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index c728da1cae..bb9b3508aa 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -158,6 +158,7 @@ class TextbookList(List): class UserPartitionList(List): + """Special List class for listing UserPartitions""" def from_json(self, values): return [UserPartition.from_json(v) for v in values] @@ -175,8 +176,9 @@ class CourseFields(object): # advanced_settings. user_partitions = UserPartitionList( help="List of user partitions of this course into groups, used e.g. for experiments", - default=[], scope=Scope.content) - + default=[], + scope=Scope.content + ) wiki_slug = String(help="Slug that points to the wiki for this course", scope=Scope.content) enrollment_start = Date(help="Date that enrollment for this class is opened", scope=Scope.settings) diff --git a/common/lib/xmodule/xmodule/js/fixtures/split_test_staff.html b/common/lib/xmodule/xmodule/js/fixtures/split_test_staff.html new file mode 100644 index 0000000000..89bff6d93f --- /dev/null +++ b/common/lib/xmodule/xmodule/js/fixtures/split_test_staff.html @@ -0,0 +1,19 @@ +
+ + +
+ <div class='condition-text'>condition 0</div> +
+
+ <div class='condition-text'>condition 1</div> +
+
+ <div class='condition-text'>condition 2</div> +
+ +
+
diff --git a/common/lib/xmodule/xmodule/js/js_test.yml b/common/lib/xmodule/xmodule/js/js_test.yml index b0eb282f07..8aeeaba122 100644 --- a/common/lib/xmodule/xmodule/js/js_test.yml +++ b/common/lib/xmodule/xmodule/js/js_test.yml @@ -57,6 +57,7 @@ lib_paths: - common_static/js/vendor/analytics.js - common_static/js/test/add_ajax_prefix.js - common_static/js/src/utility.js + - public/js/split_test_staff.js # Paths to spec (test) JavaScript files spec_paths: diff --git a/common/lib/xmodule/xmodule/js/public b/common/lib/xmodule/xmodule/js/public new file mode 120000 index 0000000000..986ba8c3a3 --- /dev/null +++ b/common/lib/xmodule/xmodule/js/public @@ -0,0 +1 @@ +../public/ \ No newline at end of file diff --git a/common/lib/xmodule/xmodule/js/spec/split_test/staff_view_spec.js b/common/lib/xmodule/xmodule/js/spec/split_test/staff_view_spec.js new file mode 100644 index 0000000000..e357a7bc06 --- /dev/null +++ b/common/lib/xmodule/xmodule/js/spec/split_test/staff_view_spec.js @@ -0,0 +1,37 @@ +describe('Tests for split_test staff view switching', function() { + var ab_module; + var elem; + + beforeEach(function() { + loadFixtures('split_test_staff.html'); + elem = $('#split-test'); + window.XBlock = jasmine.createSpyObj('XBlock', ['initializeBlocks']); + ab_module = ABTestSelector(null, elem); + }); + + afterEach(function() { + delete window.XBlock; + }); + + it("test that we have only one visible condition", function() { + var containers = elem.find('.split-test-child-container').length; + var conditions_shown = elem.find('.split-test-child-container .condition-text').length; + expect(containers).toEqual(1); + expect(conditions_shown).toEqual(1); + expect(XBlock.initializeBlocks).toHaveBeenCalled(); + }); + + it("test that the right child is visible when selected", function() { + var groups = ['0', '1', '2']; + + for(var i = 0; i < groups.length; i++) { + var to_select = groups[i]; + elem.find('.split-test-select').val(to_select).change(); + var child_text = elem.find('.split-test-child-container .condition-text').text(); + expect(child_text).toContain(to_select); + expect(XBlock.initializeBlocks).toHaveBeenCalled(); + } + + }); + +}); diff --git a/common/lib/xmodule/xmodule/js/src/sequence/display.coffee b/common/lib/xmodule/xmodule/js/src/sequence/display.coffee index 01a3d32b02..4f9a33329b 100644 --- a/common/lib/xmodule/xmodule/js/src/sequence/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/sequence/display.coffee @@ -23,8 +23,8 @@ class @Sequence updatePageTitle: -> # update the page title to include the current section position_link = @link_for(@position) - if position_link and position_link.attr('title') - document.title = position_link.attr('title') + @base_page_title + if position_link and position_link.data('page-title') + document.title = position_link.data('page-title') + @base_page_title hookUpProgressEvent: -> $('.problems-wrapper').bind 'progressChanged', @updateProgress @@ -98,10 +98,10 @@ class @Sequence # Added for aborting video bufferization, see ../video/10_main.js @el.trigger "sequence:change" @mark_active new_position - + current_tab = @contents.eq(new_position - 1) @content_container.html(current_tab.text()).attr("aria-labelledby", current_tab.attr("aria-labelledby")) - + XBlock.initializeBlocks(@content_container) window.update_schematics() # For embedded circuit simulator exercises in 6.002x @@ -115,8 +115,8 @@ class @Sequence sequence_links.click @goto # Focus on the first available xblock. @content_container.find('.vert .xblock :first').focus() - @$("a.active").blur() - + @$("a.active").blur() + goto: (event) => event.preventDefault() if $(event.target).hasClass 'seqnav' # Links from courseware ... diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index a8a36a8667..61ca0b62f1 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -6,7 +6,7 @@ import json from .xml import XMLModuleStore, ImportSystem, ParentTracker from xmodule.modulestore import Location -from xblock.fields import Scope, Reference, ReferenceList +from xblock.fields import Scope, Reference, ReferenceList, ReferenceValueDict from xmodule.contentstore.content import StaticContent from .inheritance import own_metadata from xmodule.errortracker import make_error_tracker @@ -547,15 +547,25 @@ def remap_namespace(module, target_location_namespace): ).url() return new_ref - for field in all_fields: - if isinstance(module.fields.get(field), Reference): - new_ref = convert_ref(getattr(module, field)) - setattr(module, field, new_ref) + for field_name in all_fields: + field_object = module.fields.get(field_name) + if isinstance(field_object, Reference): + new_ref = convert_ref(getattr(module, field_name)) + setattr(module, field_name, new_ref) module.save() - elif isinstance(module.fields.get(field), ReferenceList): - references = getattr(module, field) + elif isinstance(field_object, ReferenceList): + references = getattr(module, field_name) new_references = [convert_ref(reference) for reference in references] - setattr(module, field, new_references) + setattr(module, field_name, new_references) + module.save() + elif isinstance(field_object, ReferenceValueDict): + reference_dict = getattr(module, field_name) + new_reference_dict = { + key: convert_ref(reference) + for key, reference + in reference_dict.items() + } + setattr(module, field_name, new_reference_dict) module.save() return module diff --git a/common/lib/xmodule/xmodule/partitions/partitions_service.py b/common/lib/xmodule/xmodule/partitions/partitions_service.py index 3fb5f06e6c..82b283167e 100644 --- a/common/lib/xmodule/xmodule/partitions/partitions_service.py +++ b/common/lib/xmodule/xmodule/partitions/partitions_service.py @@ -88,7 +88,7 @@ class PartitionService(object): and persist the info. """ key = self._key_for_partition(user_partition) - scope = self._user_tags_service.COURSE + scope = self._user_tags_service.COURSE_SCOPE group_id = self._user_tags_service.get_tag(scope, key) if group_id is not None: @@ -133,6 +133,6 @@ class PartitionService(object): 'partition_name': user_partition.name } # TODO: Use the XBlock publish api instead - self._track_function('edx.split_test.assigned_user_to_partition', event_info) + self._track_function('xmodule.partitions.assigned_user_to_partition', event_info) return group.id diff --git a/common/lib/xmodule/xmodule/partitions/test_partitions.py b/common/lib/xmodule/xmodule/partitions/test_partitions.py index d468af307b..81d04abd53 100644 --- a/common/lib/xmodule/xmodule/partitions/test_partitions.py +++ b/common/lib/xmodule/xmodule/partitions/test_partitions.py @@ -3,6 +3,7 @@ Test the partitions and partitions service """ +from collections import defaultdict from unittest import TestCase from mock import Mock, MagicMock @@ -49,6 +50,131 @@ class TestGroup(TestCase): self.assertEqual(group.id, test_id) self.assertEqual(group.name, name) + def test_from_json_broken(self): + test_id = 5 + name = "Grendel" + # Bad version + jsonified = { + "id": test_id, + "name": name, + "version": 9001 + } + with self.assertRaisesRegexp(TypeError, "has unexpected version"): + group = Group.from_json(jsonified) + + # Missing key "id" + jsonified = { + "name": name, + "version": Group.VERSION + } + with self.assertRaisesRegexp(TypeError, "missing value key 'id'"): + group = Group.from_json(jsonified) + + # Has extra key - should not be a problem + jsonified = { + "id": test_id, + "name": name, + "version": Group.VERSION, + "programmer": "Cale" + } + group = Group.from_json(jsonified) + self.assertNotIn("programmer", group.to_json()) + + +class TestUserPartition(TestCase): + """Test constructing UserPartitions""" + def test_construct(self): + groups = [Group(0, 'Group 1'), Group(1, 'Group 2')] + user_partition = UserPartition(0, 'Test Partition', 'for testing purposes', groups) + self.assertEqual(user_partition.id, 0) + self.assertEqual(user_partition.name, "Test Partition") + self.assertEqual(user_partition.description, "for testing purposes") + self.assertEqual(user_partition.groups, groups) + + def test_string_id(self): + groups = [Group(0, 'Group 1'), Group(1, 'Group 2')] + user_partition = UserPartition("70", 'Test Partition', 'for testing purposes', groups) + self.assertEqual(user_partition.id, 70) + + def test_to_json(self): + groups = [Group(0, 'Group 1'), Group(1, 'Group 2')] + upid = 0 + upname = "Test Partition" + updesc = "for testing purposes" + user_partition = UserPartition(upid, upname, updesc, groups) + + jsonified = user_partition.to_json() + act_jsonified = { + "id": upid, + "name": upname, + "description": updesc, + "groups": [group.to_json() for group in groups], + "version": user_partition.VERSION + } + self.assertEqual(jsonified, act_jsonified) + + def test_from_json(self): + groups = [Group(0, 'Group 1'), Group(1, 'Group 2')] + upid = 1 + upname = "Test Partition" + updesc = "For Testing Purposes" + + jsonified = { + "id": upid, + "name": upname, + "description": updesc, + "groups": [group.to_json() for group in groups], + "version": UserPartition.VERSION + } + user_partition = UserPartition.from_json(jsonified) + self.assertEqual(user_partition.id, upid) + self.assertEqual(user_partition.name, upname) + self.assertEqual(user_partition.description, updesc) + for act_group in user_partition.groups: + self.assertIn(act_group.id, [0, 1]) + exp_group = groups[act_group.id] + self.assertEqual(exp_group.id, act_group.id) + self.assertEqual(exp_group.name, act_group.name) + + def test_from_json_broken(self): + groups = [Group(0, 'Group 1'), Group(1, 'Group 2')] + upid = 1 + upname = "Test Partition" + updesc = "For Testing Purposes" + + # Missing field + jsonified = { + "name": upname, + "description": updesc, + "groups": [group.to_json() for group in groups], + "version": UserPartition.VERSION + } + with self.assertRaisesRegexp(TypeError, "missing value key 'id'"): + user_partition = UserPartition.from_json(jsonified) + + # Wrong version (it's over 9000!) + jsonified = { + 'id': upid, + "name": upname, + "description": updesc, + "groups": [group.to_json() for group in groups], + "version": 9001 + } + with self.assertRaisesRegexp(TypeError, "has unexpected version"): + user_partition = UserPartition.from_json(jsonified) + + # Has extra key - should not be a problem + jsonified = { + 'id': upid, + "name": upname, + "description": updesc, + "groups": [group.to_json() for group in groups], + "version": UserPartition.VERSION, + "programmer": "Cale" + } + user_partition = UserPartition.from_json(jsonified) + self.assertNotIn("programmer", user_partition.to_json()) + class StaticPartitionService(PartitionService): """ @@ -63,32 +189,37 @@ class StaticPartitionService(PartitionService): return self._partitions +class MemoryUserTagsService(object): + """ + An implementation of a user_tags XBlock service that + uses an in-memory dictionary for storage + """ + COURSE_SCOPE = 'course' + + def __init__(self): + self._tags = defaultdict(dict) + + def get_tag(self, scope, key): + """Sets the value of ``key`` to ``value``""" + print 'GETTING', scope, key, self._tags + return self._tags[scope].get(key) + + def set_tag(self, scope, key, value): + """Gets the value of ``key``""" + self._tags[scope][key] = value + print 'SET', scope, key, value, self._tags + + class TestPartitionsService(TestCase): """ Test getting a user's group out of a partition - """ def setUp(self): groups = [Group(0, 'Group 1'), Group(1, 'Group 2')] self.partition_id = 0 - # construct the user_service - self.user_tags = dict() - self.user_tags_service = MagicMock() - - def mock_set_tag(_scope, key, value): - """Sets the value of ``key`` to ``value``""" - self.user_tags[key] = value - - def mock_get_tag(_scope, key): - """Gets the value of ``key``""" - if key in self.user_tags: - return self.user_tags[key] - return None - - self.user_tags_service.set_tag = mock_set_tag - self.user_tags_service.get_tag = mock_get_tag + self.user_tags_service = MemoryUserTagsService() user_partition = UserPartition(self.partition_id, 'Test Partition', 'for testing purposes', groups) self.partitions_service = StaticPartitionService( diff --git a/common/lib/xmodule/xmodule/public/js/split_test_staff.js b/common/lib/xmodule/xmodule/public/js/split_test_staff.js index 8dd68bced6..66cf1cc57b 100644 --- a/common/lib/xmodule/xmodule/public/js/split_test_staff.js +++ b/common/lib/xmodule/xmodule/public/js/split_test_staff.js @@ -4,30 +4,26 @@ * @constructor */ -function ABTestSelector(elem) { - me = this; - me.elem = $(elem); +function ABTestSelector(runtime, elem) { + var _this = this; + _this.elem = $(elem); + _this.children = _this.elem.find('.split-test-child'); + _this.content_container = _this.elem.find('.split-test-child-container'); - select_child = function(group_id) { + function select_child(group_id) { // iterate over all the children and hide all the ones that haven't been selected // and show the one that was selected - me.elem.find('.split-test-child').each(function() { + _this.children.each(function() { // force this id to remain a string, even if it looks like something else - child_group_id = $(this).data('group-id').toString(); + var child_group_id = $(this).data('group-id').toString(); if(child_group_id === group_id) { - $(this).show(); + _this.content_container.html($(this).text()); + XBlock.initializeBlocks(_this.content_container); } - else { - $(this).hide(); - } - }); } - // hide all the children - me.elem.find('.split-test-child').hide(); - - select = me.elem.find('.split-test-select'); + select = _this.elem.find('.split-test-select'); cur_group_id = select.val(); select_child(cur_group_id); diff --git a/common/lib/xmodule/xmodule/public/js/split_test_student.js b/common/lib/xmodule/xmodule/public/js/split_test_student.js index c3bebb1dce..f8c09083ed 100644 --- a/common/lib/xmodule/xmodule/public/js/split_test_student.js +++ b/common/lib/xmodule/xmodule/public/js/split_test_student.js @@ -1,4 +1,4 @@ -/* Javascript for the Acid XBlock. */ +/* Javascript for the Split Test XBlock. */ function SplitTestStudentView(runtime, element) { $.post(runtime.handlerUrl(element, 'log_child_render')); return {}; diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 51aa2d4261..5fc55212a8 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -88,13 +88,12 @@ class SequenceModule(SequenceFields, XModule): rendered_child = child.render('student_view', context) fragment.add_frag_resources(rendered_child) + titles = child.get_content_titles() + print titles childinfo = { 'content': rendered_child.content, - 'title': "\n".join( - grand_child.display_name - for grand_child in child.get_children() - if grand_child.display_name is not None - ), + 'title': "\n".join(titles), + 'page_title': titles[0] if titles else '', 'progress_status': Progress.to_js_status_str(progress), 'progress_detail': Progress.to_js_detail_str(progress), 'type': child.get_icon_class(), diff --git a/common/lib/xmodule/xmodule/split_test_module.py b/common/lib/xmodule/xmodule/split_test_module.py index f960ab9d4c..8d3298006b 100644 --- a/common/lib/xmodule/xmodule/split_test_module.py +++ b/common/lib/xmodule/xmodule/split_test_module.py @@ -12,15 +12,18 @@ from xmodule.x_module import XModule, module_attr from lxml import etree from xblock.core import XBlock -from xblock.fields import Scope, Integer, Dict +from xblock.fields import Scope, Integer, ReferenceValueDict from xblock.fragment import Fragment log = logging.getLogger('edx.' + __name__) class SplitTestFields(object): - user_partition_id = Integer(help="Which user partition is used for this test", - scope=Scope.content) + """Fields needed for split test module""" + user_partition_id = Integer( + help="Which user partition is used for this test", + scope=Scope.content + ) # group_id is an int # child is a serialized UsageId (aka Location). This child @@ -31,9 +34,10 @@ class SplitTestFields(object): # TODO: is there a way to add some validation around this, to # be run on course load or in studio or .... - group_id_to_child = Dict(help="Which child module students in a particular " - "group_id should see", - scope=Scope.content) + group_id_to_child = ReferenceValueDict( + help="Which child module students in a particular group_id should see", + scope=Scope.content + ) @XBlock.needs('user_tags') @@ -79,6 +83,25 @@ class SplitTestModule(SplitTestFields, XModule): return None + def get_content_titles(self): + """ + Returns list of content titles for split_test's child. + + This overwrites the get_content_titles method included in x_module by default. + + WHY THIS OVERWRITE IS NECESSARY: If we fetch *all* of split_test's children, + we'll end up getting all of the possible conditions users could ever see. + Ex: If split_test shows a video to group A and HTML to group B, the + regular get_content_titles in x_module will get the title of BOTH the video + AND the HTML. + + We only want the content titles that should actually be displayed to the user. + + split_test's .child property contains *only* the child that should actually + be shown to the user, so we call get_content_titles() on only that child. + """ + return self.child.get_content_titles() + def get_child_descriptors(self): """ For grading--return just the chosen child. @@ -127,13 +150,9 @@ class SplitTestModule(SplitTestFields, XModule): fragment.add_content(self.system.render_template('split_test_staff_view.html', { 'items': contents, })) - frag_js = """ - $(document).ready(function() {{ - ABTestSelector($('.split-test-view')); - }}); - """ - fragment.add_javascript(frag_js) + fragment.add_css('.split-test-child { display: none; }') fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/split_test_staff.js')) + fragment.initialize_js('ABTestSelector') return fragment def student_view(self, context): @@ -159,9 +178,12 @@ class SplitTestModule(SplitTestFields, XModule): return fragment @XBlock.handler - def log_child_render(self, request, suffix=''): + def log_child_render(self, request, suffix=''): # pylint: disable=unused-argument + """ + Record in the tracking logs which child was rendered + """ # TODO: use publish instead, when publish is wired to the tracking logs - self.system.track_function('split-test-child-render', {'child-id': self.child.scope_ids.usage_id}) + self.system.track_function('xblock.split_test.child_render', {'child-id': self.child.scope_ids.usage_id}) return Response() def get_icon_class(self): @@ -184,6 +206,7 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor): child_descriptor = module_attr('child_descriptor') log_child_render = module_attr('log_child_render') + get_content_titles = module_attr('get_content_titles') def definition_to_xml(self, resource_fs): @@ -200,3 +223,4 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor): makes it use module.get_child_descriptors(). """ return True + diff --git a/common/lib/xmodule/xmodule/tests/test_split_module.py b/common/lib/xmodule/xmodule/tests/test_split_module.py index de82427042..17abb1d489 100644 --- a/common/lib/xmodule/xmodule/tests/test_split_module.py +++ b/common/lib/xmodule/xmodule/tests/test_split_module.py @@ -8,9 +8,7 @@ from xmodule.tests.xml import factories as xml from xmodule.tests.xml import XModuleXmlImportTest from xmodule.tests import get_test_system from xmodule.partitions.partitions import Group, UserPartition -from xmodule.partitions.partitions_service import PartitionService -from xmodule.partitions.test_partitions import StaticPartitionService - +from xmodule.partitions.test_partitions import StaticPartitionService, MemoryUserTagsService class SplitTestModuleFactory(xml.XmlImportFactory): @@ -38,15 +36,24 @@ class SplitTestModuleTest(XModuleXmlImportTest): 'group_id_to_child': '{"0": "i4x://edX/xml_test_course/html/split_test_cond0", "1": "i4x://edX/xml_test_course/html/split_test_cond1"}' } ) - xml.HtmlFactory(parent=split_test, url_name='split_test_cond0') - xml.HtmlFactory(parent=split_test, url_name='split_test_cond1') + xml.HtmlFactory(parent=split_test, url_name='split_test_cond0', text='HTML FOR GROUP 0') + xml.HtmlFactory(parent=split_test, url_name='split_test_cond1', text='HTML FOR GROUP 1') self.course = self.process_xml(course) course_seq = self.course.get_children()[0] self.module_system = get_test_system() - self.tags_service = Mock(name='user_tags') - self.module_system._services['user_tags'] = self.tags_service + def get_module(descriptor): + module_system = get_test_system() + module_system.get_module = get_module + descriptor.bind_for_student(module_system, descriptor._field_data) + return descriptor + + self.module_system.get_module = get_module + self.module_system.descriptor_system = self.course.runtime + + self.tags_service = MemoryUserTagsService() + self.module_system._services['user_tags'] = self.tags_service # pylint: disable=protected-access self.partitions_service = StaticPartitionService( [ @@ -57,18 +64,59 @@ class SplitTestModuleTest(XModuleXmlImportTest): course_id=self.course.id, track_function=Mock(name='track_function'), ) - self.module_system._services['partitions'] = self.partitions_service + self.module_system._services['partitions'] = self.partitions_service # pylint: disable=protected-access + self.split_test_module = course_seq.get_children()[0] + self.split_test_module.bind_for_student(self.module_system, self.split_test_module._field_data) - self.split_test_descriptor = course_seq.get_children()[0] - self.split_test_descriptor.bind_for_student( - self.module_system, - self.split_test_descriptor._field_data - ) @ddt.data(('0', 'split_test_cond0'), ('1', 'split_test_cond1')) @ddt.unpack def test_child(self, user_tag, child_url_name): - self.tags_service.get_tag.return_value = user_tag + self.tags_service.set_tag( + self.tags_service.COURSE_SCOPE, + 'xblock.partition_service.partition_0', + user_tag + ) - self.assertEquals(self.split_test_descriptor.child_descriptor.url_name, child_url_name) + self.assertEquals(self.split_test_module.child_descriptor.url_name, child_url_name) + + @ddt.data(('0',), ('1',)) + @ddt.unpack + def test_child_old_tag_value(self, user_tag): + # If user_tag has a stale value, we should still get back a valid child url + self.tags_service.set_tag( + self.tags_service.COURSE_SCOPE, + 'xblock.partition_service.partition_0', + '2' + ) + + self.assertIn(self.split_test_module.child_descriptor.url_name, ['split_test_cond0', 'split_test_cond1']) + + @ddt.data(('0', 'HTML FOR GROUP 0'), ('1', 'HTML FOR GROUP 1')) + @ddt.unpack + def test_get_html(self, user_tag, child_content): + self.tags_service.set_tag( + self.tags_service.COURSE_SCOPE, + 'xblock.partition_service.partition_0', + user_tag + ) + + self.assertIn( + child_content, + self.module_system.render(self.split_test_module, 'student_view').content + ) + + @ddt.data(('0',), ('1',)) + @ddt.unpack + def test_child_missing_tag_value(self, user_tag): + # If user_tag has a missing value, we should still get back a valid child url + self.assertIn(self.split_test_module.child_descriptor.url_name, ['split_test_cond0', 'split_test_cond1']) + + @ddt.data(('100',), ('200',), ('300',), ('400',), ('500',), ('600',), ('700',), ('800',), ('900',), ('1000',)) + @ddt.unpack + def test_child_persist_new_tag_value_when_tag_missing(self, user_tag): + # If a user_tag has a missing value, a group should be saved/persisted for that user. + # So, we check that we get the same url_name when we call on the url_name twice. + # We run the test ten times so that, if our storage is failing, we'll be most likely to notice it. + self.assertEquals(self.split_test_module.child_descriptor.url_name, self.split_test_module.child_descriptor.url_name) diff --git a/common/lib/xmodule/xmodule/tests/xml/factories.py b/common/lib/xmodule/xmodule/tests/xml/factories.py index ec8020b44e..a173758280 100644 --- a/common/lib/xmodule/xmodule/tests/xml/factories.py +++ b/common/lib/xmodule/xmodule/tests/xml/factories.py @@ -146,6 +146,10 @@ class SequenceFactory(XmlImportFactory): """Factory for nodes""" tag = 'sequential' +class VerticalFactory(XmlImportFactory): + """Factory for nodes""" + tag = 'vertical' + class ProblemFactory(XmlImportFactory): """Factory for nodes""" @@ -154,5 +158,5 @@ class ProblemFactory(XmlImportFactory): class HtmlFactory(XmlImportFactory): - """Factory for nodes""" + """Factory for nodes""" tag = 'html' diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 6d9ada7314..defa9a3b8b 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -217,6 +217,31 @@ class XModuleMixin(XBlockMixin): self.save() return self._field_data._kvs # pylint: disable=protected-access + def get_content_titles(self): + """ + Returns list of content titles for all of self's children. + + SEQUENCE + | + VERTICAL + / \ + SPLIT_TEST DISCUSSION + / \ + VIDEO A VIDEO B + + Essentially, this function returns a list of display_names (e.g. content titles) + for all of the leaf nodes. In the diagram above, calling get_content_titles on + SEQUENCE would return the display_names of `VIDEO A`, `VIDEO B`, and `DISCUSSION`. + + This is most obviously useful for sequence_modules, which need this list to display + tooltips to users, though in theory this should work for any tree that needs + the display_names of all its leaf nodes. + """ + if self.has_children: + return sum((child.get_content_titles() for child in self.get_children()), []) + else: + return [self.display_name_with_default] + def get_children(self): """Returns a list of XBlock instances for the children of this module""" diff --git a/common/test/data/split_test_module/course.xml b/common/test/data/split_test_module/course.xml new file mode 100644 index 0000000000..0880f8ccd4 --- /dev/null +++ b/common/test/data/split_test_module/course.xml @@ -0,0 +1,19 @@ + + + + + + + Here is a prompt for group 0, please respond in the discussion. + + + + + Here is a prompt for group 2, please respond in the discussion. + + + + + + + \ No newline at end of file diff --git a/lms/djangoapps/courseware/tests/test_split_module.py b/lms/djangoapps/courseware/tests/test_split_module.py new file mode 100644 index 0000000000..75d1b0be10 --- /dev/null +++ b/lms/djangoapps/courseware/tests/test_split_module.py @@ -0,0 +1,244 @@ +""" +Test for split test XModule +""" +import ddt +from mock import MagicMock, patch, Mock +from django.core.urlresolvers import reverse +from django.test.utils import override_settings + +from student.tests.factories import UserFactory, CourseEnrollmentFactory, AdminFactory +from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE +from xmodule.modulestore.tests.factories import ItemFactory, CourseFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase + +from xmodule.partitions.partitions import Group, UserPartition +from xmodule.partitions.test_partitions import StaticPartitionService +from user_api.tests.factories import UserCourseTagFactory +from xmodule.partitions.partitions import Group, UserPartition + + +@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +class SplitTestBase(ModuleStoreTestCase): + __test__ = False + + def setUp(self): + self.partition = UserPartition( + 0, + 'first_partition', + 'First Partition', + [ + Group(0, 'alpha'), + Group(1, 'beta') + ] + ) + + self.course = CourseFactory.create( + number=self.COURSE_NUMBER, + user_partitions=[self.partition] + ) + + self.chapter = ItemFactory.create( + parent_location=self.course.location, + category="chapter", + display_name="test chapter", + ) + self.sequential = ItemFactory.create( + parent_location=self.chapter.location, + category="sequential", + display_name="Split Test Tests", + ) + + self.student = UserFactory.create() + CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id) + self.client.login(username=self.student.username, password='test') + + def _video(self, parent, group): + return ItemFactory.create( + parent_location=parent.location, + category="video", + display_name="Group {} Sees This Video".format(group), + ) + + def _problem(self, parent, group): + return ItemFactory.create( + parent_location=parent.location, + category="problem", + display_name="Group {} Sees This Problem".format(group), + data="

No Problem Defined Yet!

", + ) + + def _html(self, parent, group): + return ItemFactory.create( + parent_location=parent.location, + category="html", + display_name="Group {} Sees This HTML".format(group), + data="Some HTML for group {}".format(group), + ) + + def test_split_test_0(self): + self._check_split_test(0) + + def test_split_test_1(self): + self._check_split_test(1) + + def _check_split_test(self, user_tag): + tag_factory = UserCourseTagFactory( + user=self.student, + course_id=self.course.id, + key='xblock.partition_service.partition_{0}'.format(self.partition.id), + value=str(user_tag) + ) + + resp = self.client.get(reverse('courseware_section', + kwargs={'course_id': self.course.id, + 'chapter': self.chapter.url_name, + 'section': self.sequential.url_name} + )) + + content = resp.content + print content + + # Assert we see the proper icon in the top display + self.assertIn('