From 9792c93612bde698a8d77d0f26a3c682bb4870eb Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Wed, 9 Nov 2016 13:12:20 +1030
Subject: [PATCH 1/2] Uses Course Blocks API to populate the Instructor Tool's
 root_block_id drop-down.

* Removes the python course tree traversal logic, and replaces it with Javascript.
* Adds student_view_data() to downloadable block types to reveal the "question"
  text to the Course Blocks API
---
 problem_builder/answer.py                     |   7 +
 problem_builder/instructor_tool.py            | 100 +--------
 problem_builder/public/js/instructor_tool.js  | 106 ++++++++++
 problem_builder/questionnaire.py              |   7 +
 .../templates/html/instructor_tool.html       |  10 +-
 .../tests/unit/test_instructor_tool.py        | 190 +-----------------
 6 files changed, 139 insertions(+), 281 deletions(-)

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 <option> 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 <option>
+        //    element is selectable, i.e. available as a download filter.
+        //
+        // Returns the <option> 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('&nbsp;'),
+                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 = $('<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 @@ <h3>{% trans "Filters" %}</h3>
       <div class="data-export-field">
         <label>
           <span>{% trans "Section/Question:" %}</span>
-          <select name="root_block_id">
-            {% for block in block_tree %}
-            <option value="{{ block.id }}"
-                    {% if not block.eligible %} disabled="disabled" {% endif %}>
-              {% for _ in ""|ljust:block.depth %}&nbsp;&nbsp;{% endfor %}
-              {{ block.name }}
-            </option>
-            {% endfor %}
+          <select name="root_block_id" data-course-blocks-api="{{course_blocks_api}}">
+            <option value="{{root_block_id}}">{% trans "All" %}</option>
           </select>
         </label>
       </div>
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):
         """

From 0f143d8369d1a53a8d233eeb82ef1fc1e3df952c Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Wed, 9 Nov 2016 13:28:10 +1030
Subject: [PATCH 2/2] Bumps version to 2.6.2

---
 circle.yml | 2 +-
 setup.py   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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/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=[