diff --git a/circle.yml b/circle.yml index 5443559e..eddaf231 100644 --- a/circle.yml +++ b/circle.yml @@ -11,7 +11,7 @@ dependencies: - "pip install -r requirements.txt" - "pip install -r $VIRTUAL_ENV/src/xblock-sdk/requirements/base.txt" - "pip install -r $VIRTUAL_ENV/src/xblock-sdk/requirements/test.txt" - - "pip uninstall -y xblock-problem-builder && python setup.py sdist && pip install dist/xblock-problem-builder-2.6.1.tar.gz" + - "pip uninstall -y xblock-problem-builder && python setup.py sdist && pip install dist/xblock-problem-builder-2.6.2.tar.gz" - "pip install -r test_requirements.txt" - "mkdir var" test: diff --git a/problem_builder/answer.py b/problem_builder/answer.py index a9631130..cc3fc0a1 100644 --- a/problem_builder/answer.py +++ b/problem_builder/answer.py @@ -260,6 +260,13 @@ def get_template(cls, template_id): return {'data': {'name': uuid.uuid4().hex[:7]}} return {'metadata': {}, 'data': {}} + def student_view_data(self): + """ + Returns a JSON representation of the student_view of this XBlock, + retrievable from the Course Block API. + """ + return {'question': self.question} + @XBlock.needs("i18n") class AnswerRecapBlock(AnswerMixin, StudioEditableXBlockMixin, XBlock): diff --git a/problem_builder/instructor_tool.py b/problem_builder/instructor_tool.py index c353e6f5..b4e0b7f3 100644 --- a/problem_builder/instructor_tool.py +++ b/problem_builder/instructor_tool.py @@ -34,6 +34,10 @@ PAGE_SIZE = 15 +# URL Path to the Course Blocks REST API. +# Note that we add a trailing slash to avoid the API's redirect hit. +COURSE_BLOCKS_API = '/api/courses/v1/blocks/' + # Make '_' a no-op so we can scrape strings def _(text): @@ -135,12 +139,11 @@ def student_view(self, context=None): _('Long Answer'): 'AnswerBlock', } - flat_block_tree = self._build_course_tree() - - html = loader.render_template( - 'templates/html/instructor_tool.html', - {'block_choices': block_choices, 'block_tree': flat_block_tree} - ) + html = loader.render_template('templates/html/instructor_tool.html', { + 'block_choices': block_choices, + 'course_blocks_api': COURSE_BLOCKS_API, + 'root_block_id': unicode(getattr(self.runtime, 'course_id', 'course_id')), + }) fragment = Fragment(html) fragment.add_css_url(self.runtime.local_resource_url(self, 'public/css/instructor_tool.css')) fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/instructor_tool.js')) @@ -150,91 +153,6 @@ def student_view(self, context=None): fragment.initialize_js('InstructorToolBlock') return fragment - def _build_course_tree(self): - """ - Return flat tree of blocks belonging to this block's parent course. - """ - eligible_block_types = ('pb-mcq', 'pb-rating', 'pb-answer') - flat_block_tree = [] - - def get_block_id(block): - """ - Return ID of `block`, taking into account needs of both LMS/CMS and workbench runtimes. - """ - usage_id = block.scope_ids.usage_id - # Try accessing block ID. If usage_id does not have it, return usage_id itself - return unicode(getattr(usage_id, 'block_id', usage_id)) - - def get_block_name(block): - """ - Return name of `block`. - - Try attributes in the following order: - - block.question - - block.name (fallback for old courses) - - block.display_name - - block ID - """ - for attribute in ('question', 'name', 'display_name'): - if getattr(block, attribute, None): - return getattr(block, attribute, None) - return get_block_id(block) - - def get_block_type(block): - """ - Return type of `block`, taking into account different key styles that might be in use. - """ - try: - block_type = block.runtime.id_reader.get_block_type(block.scope_ids.def_id) - except AttributeError: - block_type = block.runtime.id_reader.get_block_type(block.scope_ids.usage_id) - return block_type - - def build_tree(block, ancestors): - """ - Build up a tree of information about the XBlocks descending from `block`. - """ - block_id = get_block_id(block) - block_name = get_block_name(block) - block_type = get_block_type(block) - if block_type != 'pb-choice': - eligible = block_type in eligible_block_types - if eligible: - # If this block is a question whose answers we can export, - # we mark all of its ancestors as exportable too - if ancestors and not ancestors[-1]["eligible"]: - for ancestor in ancestors: - ancestor["eligible"] = True - - new_entry = { - "depth": len(ancestors), - "id": block_id, - "name": block_name, - "eligible": eligible, - } - flat_block_tree.append(new_entry) - if block.has_children and not getattr(block, "has_dynamic_children", lambda: False)(): - for child_id in block.children: - build_tree(block.runtime.get_block(child_id), ancestors=(ancestors + [new_entry])) - - root_block = self - while root_block.parent: - root_block = root_block.get_parent() - root_block_id = get_block_id(root_block) - root_entry = { - "depth": 0, - "id": root_block_id, - "name": "All", - "eligible": False, - } - flat_block_tree.append(root_entry) - - for child_id in root_block.children: - child_block = root_block.runtime.get_block(child_id) - build_tree(child_block, [root_entry]) - - return flat_block_tree - @property def download_url_for_last_report(self): """ Get the URL for the last report, if any """ diff --git a/problem_builder/public/js/instructor_tool.js b/problem_builder/public/js/instructor_tool.js index 1a4dc74c..f9d58e99 100644 --- a/problem_builder/public/js/instructor_tool.js +++ b/problem_builder/public/js/instructor_tool.js @@ -250,6 +250,111 @@ function InstructorToolBlock(runtime, element) { if (statusChanged) updateView(); } + // Block types with answers we can export + var questionBlockTypes = ['pb-mcq', 'pb-rating', 'pb-answer']; + + // Fetch this course's blocks from the REST API, and add them to the + // list of blocks in the Section/Question drop-down list. + function getCourseBlocks() { + $.ajax({ + type: 'GET', + url: $rootBlockId.data('course-blocks-api'), + data: { + course_id: $element.data('course-id'), + requested_fields: 'name,display_name,block_type,children', + student_view_data: questionBlockTypes.join(','), + all_blocks: true, + depth: 'all' + }, + success: updateBlockOptions, + dataType: 'json' + }); + } + + // Appends the blocks returned by the Course Blocks API as options for + // the Section/Question drop-down list, arranged as a tree. + function updateBlockOptions(data) { + + // Constructs an element from the given block to add to the + // list of root blocks. + // + // Uses the block's: + // * question, name, or display name as the label. + // * depth in the course to indent the label, to make the tree + // structure more visible. + // * 'enabled' attribute to decide whether the + // element is selectable, i.e. available as a download filter. + // + // Returns the element so that it can be enabled later, + // if it's found to have a descendant that is enabled. + var appendBlock = function(block) { + var blockId = block.id.split('+block@').pop(), + padding = Array(2*block.depth).join(' '), + disabled = (block.enabled ? undefined : 'disabled'), + labelAttr, + label, + $option; + + // Merge any fields exposed by student_view_data, so they can be + // candidates for the label attribute. + block = _.extend(block, block['student_view_data']); + + // Find the best label attribute available for the block. + labelAttr = _.find( + ['question', 'name', 'display_name'], + function(attr) { + return block[attr]; + } + ); + label = padding + (block[labelAttr] || blockId); + $option = $('', {value: blockId, html: label, disabled: disabled}); + + $rootBlockId.append($option); + return $option; + }, + + // Builds the tree of course blocks. + buildTree = function(block, ancestors) { + + // Omit pb-choice blocks + if (block.type == 'pb-choice') return; + + // Enable the exportable blocks, and their ancestors. + if (_.contains(questionBlockTypes, block.type)) { + block.enabled = true; + + for (var i = ancestors.length; i > 0; --i) { + var ancestor = ancestors[i-1]; + + // No need to continue; these ancestors are already enabled. + if (ancestor.enabled) break; + + ancestor.enabled = true; + ancestor.element.removeAttr('disabled'); + } + } + + block.depth = ancestors.length; + block.element = appendBlock(block); + + // Recurse over all the child blocks, including the current block as an ancestor. + var childAncestors = ancestors.concat([block]); + _.each(block.children, function(child_id) { + buildTree(data.blocks[child_id], childAncestors); + }); + }, + root = data.blocks[data.root]; + + // Label the root block as "All" + root.name = gettext('All'); + + // Remove any existing options + $rootBlockId.empty(); + + // Build the course blocks tree from the root. + buildTree(root, []); + } + function disableActions() { $startButton.prop('disabled', true); $cancelButton.prop('disabled', true); @@ -369,6 +474,7 @@ function InstructorToolBlock(runtime, element) { showSpinner(); disableActions(); + getCourseBlocks(); getStatus(); } diff --git a/problem_builder/questionnaire.py b/problem_builder/questionnaire.py index 595bb25f..c9bf446e 100644 --- a/problem_builder/questionnaire.py +++ b/problem_builder/questionnaire.py @@ -235,3 +235,10 @@ def message_formatted(self): format_html = getattr(self.runtime, 'replace_urls', lambda html: html) return format_html(self.message) return "" + + def student_view_data(self): + """ + Returns a JSON representation of the student_view of this XBlock, + retrievable from the Course Block API. + """ + return {'question': self.question} diff --git a/problem_builder/templates/html/instructor_tool.html b/problem_builder/templates/html/instructor_tool.html index 59f5edcd..6804f044 100644 --- a/problem_builder/templates/html/instructor_tool.html +++ b/problem_builder/templates/html/instructor_tool.html @@ -28,14 +28,8 @@ {% trans "Filters" %} {% trans "Section/Question:" %} - - {% for block in block_tree %} - - {% for _ in ""|ljust:block.depth %} {% endfor %} - {{ block.name }} - - {% endfor %} + + {% trans "All" %} diff --git a/problem_builder/tests/unit/test_instructor_tool.py b/problem_builder/tests/unit/test_instructor_tool.py index a39c6d46..11e1c613 100644 --- a/problem_builder/tests/unit/test_instructor_tool.py +++ b/problem_builder/tests/unit/test_instructor_tool.py @@ -5,7 +5,7 @@ import unittest from mock import Mock, patch from xblock.field_data import DictFieldData -from problem_builder.instructor_tool import InstructorToolBlock +from problem_builder.instructor_tool import InstructorToolBlock, COURSE_BLOCKS_API @ddt.ddt @@ -31,188 +31,15 @@ def _get_block(self, block_info): return block def setUp(self): + self.course_id = 'course-v1:edX+DemoX+Demo_Course' self.runtime_mock = Mock() self.runtime_mock.get_block = self._get_block + self.runtime_mock.course_id = self.course_id scope_ids_mock = Mock() scope_ids_mock.usage_id = u'0' self.block = InstructorToolBlock( self.runtime_mock, field_data=DictFieldData({}), scope_ids=scope_ids_mock ) - self.block.children = [ - # No attributes: Prefer usage_id - {'usage_id': u'1'}, - # Single attribute: Prefer attribute that's present - {'usage_id': u'2', 'preferred_attr': 'question', 'attrs': {'question': 'question'}}, - {'usage_id': u'3', 'preferred_attr': 'name', 'attrs': {'name': 'name'}}, - {'usage_id': u'4', 'preferred_attr': 'display_name', 'attrs': {'display_name': 'display_name'}}, - # Two attributes (question, name): Prefer question - { - 'usage_id': u'5', - 'preferred_attr': - 'question', 'attrs': {'question': 'question', 'name': 'name'} - }, - # Two attributes (question, display_name): Prefer question - { - 'usage_id': u'6', - 'preferred_attr': 'question', - 'attrs': {'question': 'question', 'display_name': 'display_name'} - }, - # Two attributes (name, display_name): Prefer name - { - 'usage_id': u'7', - 'preferred_attr': 'name', - 'attrs': {'name': 'name', 'display_name': 'display_name'} - }, - # All attributes: Prefer question - { - 'usage_id': u'8', - 'preferred_attr': 'question', - 'attrs': {'question': 'question', 'name': 'name', 'display_name': 'display_name'} - }, - ] - - def test_build_course_tree_uses_preferred_attrs(self): - """ - Check if `_build_course_tree` method uses preferred block - attributes for `id` and `name` of each block. - - Each entry of the block tree returned by `_build_course_tree` - is a dictionary that must contain an `id` key and a `name` - key. - - - `id` must be set to the ID (usage_id or block_id) - of the corresponding block. - - - `name` must be set to the value of one of the following attributes - of the corresponding block: - - - question - - name (question ID) - - display_name (question title) - - block ID - - Note that the attributes are listed in order of preference; - i.e., if `block.question` has a meaningful value, that value - should be used for `name` (irrespective of what the values - of the other attributes might be). - """ - block_tree = self.block._build_course_tree() - - def check_block(usage_id, expected_name): - # - Does block_tree contain single entry whose `id` matches `usage_id` of block? - matching_blocks = [block for block in block_tree if block['id'] == usage_id] - self.assertEqual(len(matching_blocks), 1) - # - Is `name` of that entry set to `expected_name`? - matching_block = matching_blocks[0] - self.assertEqual(matching_block['name'], expected_name) - - # Check size of block_tree - num_blocks = len(self.block.children) + 1 - self.assertEqual(len(block_tree), num_blocks) - - # Check block_tree for root entry - check_block(usage_id=self.block.scope_ids.usage_id, expected_name='All') - - # Check block_tree for children - for child in self.block.children: - usage_id = child.get('usage_id') - attrs = child.get('attrs', {}) - if not attrs: - expected_name = usage_id - else: - preferred_attr = child.get('preferred_attr') - expected_name = attrs[preferred_attr] - check_block(usage_id, expected_name) - - def test_build_course_tree_excludes_choice_blocks(self): - """ - Check if `_build_course_tree` method excludes 'pb-choice' blocks. - """ - # Pretend that all blocks in self.block.children are of type 'pb-choice: - self.runtime_mock.id_reader = Mock() - self.runtime_mock.id_reader.get_block_type.return_value = 'pb-choice' - - block_tree = self.block._build_course_tree() - - # Check size of block_tree: Should only include root block - self.assertEqual(len(block_tree), 1) - - @ddt.data('pb-mcq', 'pb-rating', 'pb-answer') - def test_build_course_tree_eligible_blocks(self, block_type): - """ - Check if `_build_course_tree` method correctly marks MCQ, Rating, - and Answer blocks as eligible. - - A block is eligible if its type is one of {'pb-mcq', 'pb-rating', 'pb-answer'}. - """ - # Pretend that all blocks in self.block.children are eligible: - self.runtime_mock.id_reader = Mock() - self.runtime_mock.id_reader.get_block_type.return_value = block_type - - block_tree = self.block._build_course_tree() - - # Check size of block_tree: All blocks should be included - num_blocks = len(self.block.children) + 1 - self.assertEqual(len(block_tree), num_blocks) - - # Check if all blocks are eligible: - self.assertTrue(all(block['eligible'] for block in block_tree)) - - @ddt.data( - 'problem-builder', - 'pb-table', - 'pb-column', - 'pb-answer-recap', - 'pb-mrq', - 'pb-message', - 'pb-tip', - 'pb-dashboard', - 'pb-data-export', - 'pb-instructor-tool', - ) - def test_build_course_tree_ineligible_blocks(self, block_type): - """ - Check if `_build_course_tree` method correctly marks blocks that - aren't MCQ, Rating, or Answer blocks as ineligible. - """ - # Pretend that none of the blocks in self.block.children are eligible: - self.runtime_mock.id_reader = Mock() - self.runtime_mock.id_reader.get_block_type.return_value = block_type - - block_tree = self.block._build_course_tree() - - # Check size of block_tree: All blocks should be included (they are not of type 'pb-choice') - num_blocks = len(self.block.children) + 1 - self.assertEqual(len(block_tree), num_blocks) - - # Check if all blocks are ineligible: - self.assertTrue(all(not block['eligible'] for block in block_tree)) - - def test_build_course_tree_supports_new_style_keys(self): - """ - Check if `_build_course_tree` method correctly handles new-style keys. - - To determine eligibility of a given block, - `_build_course_tree` has to obtain the blocks's type. It uses - `block.runtime.id_reader.get_block_type` to do this. It first - tries to pass `block.scope_ids.def_id` as an argument. - - **If old-style keys are enabled, this will work. If new-style - keys are enabled, this will fail with an AttributeError.** - - `_build_course_tree` should not let this error bubble up. - Instead, it should catch the error and try again, this time - passing `block.scope_ids.usage_id` to the method mentioned - above (which will work if new-style keys are enabled). - """ - # Pretend that new-style keys are enabled: - self.block.scope_ids.def_id = Mock() - self.block.scope_ids.def_id.block_type.side_effect = AttributeError() - - try: - self.block._build_course_tree() - except AttributeError: - self.fail('student_view breaks if new-style keys are enabled.') def test_student_view_template_args(self): """ @@ -224,16 +51,15 @@ def test_student_view_template_args(self): 'Rating Question': 'RatingBlock', 'Long Answer': 'AnswerBlock', } - flat_block_tree = ['block{}'.format(i) for i in range(10)] - self.block._build_course_tree = Mock(return_value=flat_block_tree) with patch('problem_builder.instructor_tool.loader') as patched_loader: patched_loader.render_template.return_value = u'' self.block.student_view() - patched_loader.render_template.assert_called_once_with( - 'templates/html/instructor_tool.html', - {'block_choices': block_choices, 'block_tree': flat_block_tree} - ) + patched_loader.render_template.assert_called_once_with('templates/html/instructor_tool.html', { + 'block_choices': block_choices, + 'course_blocks_api': COURSE_BLOCKS_API, + 'root_block_id': self.course_id, + }) def test_author_view(self): """ diff --git a/setup.py b/setup.py index 3fbdd3c1..98dbd22e 100644 --- a/setup.py +++ b/setup.py @@ -70,7 +70,7 @@ def package_data(pkg, root_list): setup( name='xblock-problem-builder', - version='2.6.1', + version='2.6.2', description='XBlock - Problem Builder', packages=['problem_builder', 'problem_builder.v1'], install_requires=[