Merge pull request #47 from open-craft/dropdown-pagination

Instructor Tool: Drop-down for "Root block ID" section, pagination improvements (OC-783)

problem_builder/instructor_tool.py

@@ -23,14 +23,17 @@ Instructor Tool: An XBlock for instructors to export student answers from a cour
 All processing is done offline.
 """
 import json
+from django.core.paginator import Paginator
 from xblock.core import XBlock
 from xblock.exceptions import JsonHandlerError
-from xblock.fields import Scope, String, Dict
+from xblock.fields import Scope, String, Dict, List
 from xblock.fragment import Fragment
 from xblockutils.resources import ResourceLoader
 
 loader = ResourceLoader(__name__)
 
+PAGE_SIZE = 15
+
 
 # Make '_' a no-op so we can scrape strings
 def _(text):
@@ -63,6 +66,12 @@ class InstructorToolBlock(XBlock):
         default=None,
         scope=Scope.user_state,
     )
+    display_data = List(
+        # The list of results associated with the most recent successful export.
+        # Stored separately to avoid the overhead of sending it to the client.
+        default=None,
+        scope=Scope.user_state,
+    )
     has_author_view = True
 
     @property
@@ -75,6 +84,11 @@ class InstructorToolBlock(XBlock):
         # different celery queues; our task listener is waiting for tasks on the LMS queue)
         return Fragment(u'<p>Instructor Tool Block</p><p>This block only works from the LMS.</p>')
 
+    def studio_view(self, context=None):
+        """ View for editing Instructor Tool block in Studio. """
+        # Display friendly message explaining that the block is not editable.
+        return Fragment(u'<p>This is a preconfigured block. It is not editable.</p>')
+
     def check_pending_export(self):
         """
         If we're waiting for an export, see if it has finished, and if so, get the result.
@@ -90,11 +104,26 @@ class InstructorToolBlock(XBlock):
         self.active_export_task_id = ''
         if task_result.successful():
             if isinstance(task_result.result, dict) and not task_result.result.get('error'):
+                self.display_data = task_result.result['display_data']
+                del task_result.result['display_data']
                 self.last_export_result = task_result.result
             else:
                 self.last_export_result = {'error': u'Unexpected result: {}'.format(repr(task_result.result))}
+                self.display_data = None
         else:
             self.last_export_result = {'error': unicode(task_result.result)}
+            self.display_data = None
+
+    @XBlock.json_handler
+    def get_result_page(self, data, suffix=''):
+        """ Return requested page of `last_export_result`. """
+        paginator = Paginator(self.display_data, PAGE_SIZE)
+        page = data.get('page', None)
+        return {
+            'display_data': paginator.page(page).object_list,
+            'num_results': len(self.display_data),
+            'page_size': PAGE_SIZE
+        }
 
     def student_view(self, context=None):
         """ Normal View """
@@ -105,9 +134,87 @@ class InstructorToolBlock(XBlock):
             _('Rating Question'): 'RatingBlock',
             _('Long Answer'): 'AnswerBlock',
         }
+        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 root_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",
+        }
+        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])
+
         html = loader.render_template(
             'templates/html/instructor_tool.html',
-            {'block_choices': block_choices}
+            {'block_choices': block_choices, 'block_tree': flat_block_tree}
         )
         fragment = Fragment(html)
         fragment.add_css_url(self.runtime.local_resource_url(self, 'public/css/instructor_tool.css'))
@@ -144,6 +251,7 @@ class InstructorToolBlock(XBlock):
         self.last_export_result = {
             'error': message,
         }
+        self.display_data = None
         raise JsonHandlerError(code, message)
 
     @XBlock.json_handler
@@ -157,6 +265,7 @@ class InstructorToolBlock(XBlock):
 
     def _delete_export(self):
         self.last_export_result = None
+        self.display_data = None
         self.active_export_task_id = ''
 
     @XBlock.json_handler
@@ -187,9 +296,6 @@ class InstructorToolBlock(XBlock):
             root_block_id = self.scope_ids.usage_id
             # Block ID not in workbench runtime.
             root_block_id = unicode(getattr(root_block_id, 'block_id', root_block_id))
-            get_root = True
-        else:
-            get_root = False
 
         # Launch task
         from .tasks import export_data as export_data_task  # Import here since this is edX LMS specific
@@ -203,7 +309,6 @@ class InstructorToolBlock(XBlock):
             block_types,
             user_id,
             match_string,
-            get_root=get_root
         )
         if async_result.ready():
             # In development mode, the task may have executed synchronously.

problem_builder/public/css/instructor_tool.css

@@ -25,6 +25,12 @@
     display: table-cell;
     padding-left: 1em;
 }
+.data-export-field-container {
+    width: 43%;
+}
+.data-export-options .data-export-actions {
+    max-width: 10%;
+}
 .data-export-field {
     margin-top: .5em;
     margin-bottom: .5em;
@@ -34,7 +40,7 @@
     vertical-align: middle;
 }
 .data-export-field input, .data-export-field select {
-    max-width: 60%;
+    width: 55%;
     float: right;
 }
 .data-export-results, .data-export-download, .data-export-cancel, .data-export-delete {

problem_builder/tasks.py

@@ -6,7 +6,6 @@ import time
 from celery.task import task
 from celery.utils.log import get_task_logger
 from instructor_task.models import ReportStore
-from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
 from student.models import user_by_anonymous_id
 from xmodule.modulestore.django import modulestore
@@ -21,7 +20,7 @@ logger = get_task_logger(__name__)
 
 
 @task()
-def export_data(course_id, source_block_id_str, block_types, user_id, match_string, get_root=True):
+def export_data(course_id, source_block_id_str, block_types, user_id, match_string):
     """
     Exports student answers to all MCQ questions to a CSV file.
     """
@@ -31,18 +30,10 @@ def export_data(course_id, source_block_id_str, block_types, user_id, match_stri
     try:
         course_key = CourseKey.from_string(course_id)
         src_block = modulestore().get_items(course_key, qualifiers={'name': source_block_id_str}, depth=0)[0]
-        if src_block is None:
-            raise InvalidKeyError
-    except InvalidKeyError:
+    except IndexError:
         raise ValueError("Could not find the specified Block ID.")
     course_key_str = unicode(course_key)
 
-    root = src_block
-    if get_root:
-        # Get the root block for the course.
-        while root.parent:
-            root = root.get_parent()
-
     type_map = {cls.__name__: cls for cls in [MCQBlock, RatingBlock, AnswerBlock]}
 
     if not block_types:
@@ -65,7 +56,7 @@ def export_data(course_id, source_block_id_str, block_types, user_id, match_stri
                     # Blocks may refer to missing children. Don't break in this case.
                     pass
 
-    scan_for_blocks(root)
+    scan_for_blocks(src_block)
 
     # Define the header row of our CSV:
     rows = []
@@ -106,7 +97,7 @@ def _extract_data(course_key_str, block, user_id, match_string):
     block_type = _get_type(block)
 
     # Extract info for "Question" column
-    block_question = block.question
+    block_question = _get_question(block)
 
     # Extract info for "Answer" and "Username" columns
     # - Get all of the most recent student submissions for this block:
@@ -150,6 +141,13 @@ def _get_type(block):
     return block.scope_ids.block_type
 
 
+def _get_question(block):
+    """
+    Return question for `block`; default to question ID if `question` is not set.
+    """
+    return block.question or block.name
+
+
 def _get_submissions(course_key_str, block, user_id):
     """
     Return submissions for `block`.
@@ -158,6 +156,8 @@ def _get_submissions(course_key_str, block, user_id):
     # Note this requires one giant query that retrieves all student submissions for `block` at once.
     block_id = unicode(block.scope_ids.usage_id.replace(branch=None, version_guid=None))
     block_type = _get_type(block)
+    if block_type == 'pb-answer':
+        block_id = block.name  # item_id of Long Answer submission matches question ID and not block ID
     if not user_id:
         return sub_api.get_all_submissions(course_key_str, block_id, block_type)
     else:

problem_builder/templates/html/instructor_tool.html

@@ -27,8 +27,16 @@
     <div class="data-export-field-container">
       <div class="data-export-field">
         <label>
-          <span>{% trans "Root block ID:" %}</span>
-          <input type="text" name="root_block_id" />
+          <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>
         </label>
       </div>
     </div>

problem_builder/tests/integration/test_instructor_tool.py

@@ -7,7 +7,7 @@ from mock import patch, Mock
 from selenium.common.exceptions import NoSuchElementException
 from xblockutils.base_test import SeleniumXBlockTest
 
-from problem_builder.instructor_tool import InstructorToolBlock
+from problem_builder.instructor_tool import PAGE_SIZE, InstructorToolBlock
 
 
 class MockTasksModule(object):
@@ -62,6 +62,36 @@ class InstructorToolTest(SeleniumXBlockTest):
         'instructor_task.models': MockInstructorTaskModelsModule(),
     })
     @patch.object(InstructorToolBlock, 'user_is_staff', Mock(return_value=True))
+    def test_data_export_delete(self):
+        instructor_tool = self.go_to_view()
+        start_button = instructor_tool.find_element_by_class_name('data-export-start')
+        result_block = instructor_tool.find_element_by_class_name('data-export-results')
+        status_area = instructor_tool.find_element_by_class_name('data-export-status')
+        download_button = instructor_tool.find_element_by_class_name('data-export-download')
+        cancel_button = instructor_tool.find_element_by_class_name('data-export-cancel')
+        delete_button = instructor_tool.find_element_by_class_name('data-export-delete')
+
+        start_button.click()
+
+        self.wait_until_visible(result_block)
+        self.wait_until_visible(delete_button)
+
+        delete_button.click()
+
+        self.wait_until_hidden(result_block)
+        self.wait_until_hidden(delete_button)
+
+        self.assertTrue(start_button.is_enabled())
+        self.assertEqual('', status_area.text)
+        self.assertFalse(download_button.is_displayed())
+        self.assertFalse(cancel_button.is_displayed())
+
+    @patch.dict('sys.modules', {
+        'problem_builder.tasks': MockTasksModule(successful=True),
+        'instructor_task': True,
+        'instructor_task.models': MockInstructorTaskModelsModule(),
+    })
+    @patch.object(InstructorToolBlock, 'user_is_staff', Mock(return_value=True))
     def test_data_export_success(self):
         instructor_tool = self.go_to_view()
         start_button = instructor_tool.find_element_by_class_name('data-export-start')
@@ -146,6 +176,7 @@ class InstructorToolTest(SeleniumXBlockTest):
         start_button.click()
 
         self.wait_until_visible(result_block)
+        time.sleep(1)  # Allow some time for result block to fully fade in
 
         self.assertFalse(first_page_button.is_enabled())
         self.assertFalse(prev_page_button.is_enabled())
@@ -179,6 +210,7 @@ class InstructorToolTest(SeleniumXBlockTest):
         start_button.click()
 
         self.wait_until_visible(result_block)
+        time.sleep(1)  # Allow some time for result block to fully fade in
 
         for contents in [
                 'Test section', 'Test subsection', 'Test unit',
@@ -199,7 +231,7 @@ class InstructorToolTest(SeleniumXBlockTest):
             successful=True, display_data=[[
                 'Test section', 'Test subsection', 'Test unit',
                 'Test type', 'Test question', 'Test answer', 'Test username'
-            ] for _ in range(45)]),
+            ] for _ in range(PAGE_SIZE*3)]),
         'instructor_task': True,
         'instructor_task.models': MockInstructorTaskModelsModule(),
     })
@@ -218,13 +250,14 @@ class InstructorToolTest(SeleniumXBlockTest):
         start_button.click()
 
         self.wait_until_visible(result_block)
+        time.sleep(1)  # Allow some time for result block to fully fade in
 
         for contents in [
                 'Test section', 'Test subsection', 'Test unit',
                 'Test type', 'Test question', 'Test answer', 'Test username'
         ]:
             occurrences = re.findall(contents, result_block.text)
-            self.assertEqual(len(occurrences), 15)
+            self.assertEqual(len(occurrences), PAGE_SIZE)
 
         self.assertFalse(first_page_button.is_enabled())
         self.assertFalse(prev_page_button.is_enabled())