Address Sarina's review comments

README.rst

@@ -67,9 +67,11 @@ Supported field types:
   ``field_name = String(multiline_editor=True, resettable_editor=False)``
 * String (html):
   ``field_name = String(multiline_editor='html', resettable_editor=False)``
-* Any of the above will use a dropdown menu if they have a pre-defined
-  list of possible values.
-* List of undordered unique values (i.e. sets) drawn from a small set of
+
+Any of the above will use a dropdown menu if they have a pre-defined
+list of possible values.
+
+* List of unordered unique values (i.e. sets) drawn from a small set of
   possible values:
   ``field_name = List(list_style='set', list_values_provider=some_method)``
 
@@ -78,9 +80,9 @@ Supported field types:
   - The ``List`` declaration must also define a ``list_values_provider`` method
     which will be called with the block as its only parameter and which must
     return a list of possible values.
-* Rudimentary support for List, Dict, and any other JSONField-derived field types
+* Rudimentary support for Dict, ordered List, and any other JSONField-derived field types
 
-  - ``list_field = List(display_name="Normal List", default=[])``
+  - ``list_field = List(display_name="Ordered List", default=[])``
   - ``dict_field = Dict(display_name="Normal Dict", default={})``
 
 Supported field options (all field types):
@@ -117,14 +119,14 @@ StudioContainerXBlockMixin
 
     from xblockutils.studio_editable import StudioContainerXBlockMixin
 
-This mixin helps with creating XBlocks that want to allow content
-authors to add/remove/reorder child blocks. By removing any existing
+This mixin helps to create XBlocks that allow content authors to add,
+remove, or reorder child blocks. By removing any existing
 ``author_view`` and adding this mixin, you'll get editable,
-re-orderable, deletable child support in Studio. To enable authors to
-add new children, simply override ``author_edit_view`` and set
-``can_add=True`` when calling ``render_children`` - see the source code.
-To enable authors to add only a limited subset of children requires
-custom HTML.
+re-orderable, and deletable child support in Studio. To enable authors to
+add arbitrary blocks as children, simply override ``author_edit_view`` 
+and set ``can_add=True`` when calling ``render_children`` - see the 
+source code. To restrict authors so they can add only specific types of
+child blocks or a limited number of children requires custom HTML.
 
 An example is the mentoring XBlock: |Screenshot 2|
 
@@ -177,9 +179,17 @@ child\_isinstance
 
 If your XBlock needs to find children/descendants of a particular
 class/mixin, you should use
-``child_isinstance(self, child_usage_id, SomeXBlockClassOrMixin)``
+
+.. code:: python
+
+    child_isinstance(self, child_usage_id, SomeXBlockClassOrMixin)
+
 rather than calling
-``isinstance(self.runtime.get_block(child_usage_id), SomeXBlockClassOrMixin)``.
+
+.. code:: python
+
+    ``isinstance(self.runtime.get_block(child_usage_id), SomeXBlockClassOrMixin)``.
+
 On runtimes such as those in edx-platform, ``child_isinstance`` is
 orders of magnitude faster.
 

tests/integration/test_studio_editable.py

@@ -7,7 +7,7 @@ from xblockutils.studio_editable_test import StudioEditableBaseTest
 
 class EditableXBlock(StudioEditableXBlockMixin, XBlock):
     """
-    A Studio-editable XBlock
+    A basic Studio-editable XBlock (for use in tests)
     """
     color = String(default="red")
     count = Integer(default=42)
@@ -15,8 +15,11 @@ class EditableXBlock(StudioEditableXBlockMixin, XBlock):
     editable_fields = ('color', 'count', 'comment')
 
     def validate_field_data(self, validation, data):
-        """ Basic validation method for these tests """
-        if data.count <=0:
+        """
+        A validation method to check that 'count' is positive and prevent
+        swearing in the 'comment' field.
+        """
+        if data.count < 0:
             validation.add(ValidationMessage(ValidationMessage.ERROR, u"Count cannot be negative"))
         if "damn" in data.comment.lower():
             validation.add(ValidationMessage(ValidationMessage.ERROR, u"No swearing allowed"))
@@ -33,6 +36,18 @@ class TestEditableXBlock_StudioView(StudioEditableBaseTest):
         self.go_to_view("studio_view")
         self.fix_js_environment()
 
+    def assert_unchanged(self, block, orig_field_values=None, explicitly_set=False):
+        """
+        Check that all field values on 'block' match with either the value in orig_field_values
+        (if provided) or the default value.
+        If 'explitly_set' is False (default) it asserts that no fields have an explicit value
+        set. If 'explititly_set' is True it expects all fields to be explicitly set.
+        """
+        for field_name in block.editable_fields:
+            expected_value = orig_field_values[field_name] if orig_field_values else block.fields[field_name].default
+            self.assertEqual(getattr(block, field_name), expected_value)
+            self.assertEqual(block.fields[field_name].is_set_on(block), explicitly_set)
+
     def test_no_changes_with_defaults(self):
         """
         If we load the edit form and then save right away, there should be no changes.
@@ -40,9 +55,7 @@ class TestEditableXBlock_StudioView(StudioEditableBaseTest):
         block = self.load_root_xblock()
         orig_values = {field_name: getattr(block, field_name) for field_name in EditableXBlock.editable_fields}
         self.click_save()
-        for field_name in EditableXBlock.editable_fields:
-            self.assertEqual(getattr(block, field_name), orig_values[field_name])
-            self.assertFalse(block.fields[field_name].is_set_on(block))
+        self.assert_unchanged(block, orig_values)
 
     def test_no_changes_with_values_set(self):
         """
@@ -61,9 +74,7 @@ class TestEditableXBlock_StudioView(StudioEditableBaseTest):
 
         self.click_save()
         block = self.load_root_xblock()  # Need to reload the block to bypass its cache
-        for field_name in EditableXBlock.editable_fields:
-            self.assertEqual(getattr(block, field_name), orig_values[field_name])
-            self.assertTrue(block.fields[field_name].is_set_on(block))
+        self.assert_unchanged(block, orig_values, explicitly_set=True)
 
     def test_explicit_overrides(self):
         """
@@ -71,33 +82,29 @@ class TestEditableXBlock_StudioView(StudioEditableBaseTest):
         value will be saved explicitly.
         """
         block = self.load_root_xblock()
-        for field_name in EditableXBlock.editable_fields:
-            self.assertFalse(block.fields[field_name].is_set_on(block))
-
-        color_control = self.get_element_for_field('color')
-        color_control.clear()
-        color_control.send_keys('red')
+        self.assert_unchanged(block)
 
-        count_control = self.get_element_for_field('count')
-        count_control.clear()
-        count_control.send_keys('42')
+        field_names = EditableXBlock.editable_fields
+        # It is crucial to this test that at least one of the fields is a String field with
+        # an empty string as its default value:
+        defaults = set([block.fields[field_name].default for field_name in field_names])
+        self.assertIn(u'', defaults)
 
-        comment_control = self.get_element_for_field('comment')
-        comment_control.send_keys('forcing a change')
-        comment_control.clear()
+        for field_name in field_names:
+            control = self.get_element_for_field(field_name)
+            control.send_keys('9999')  # In case the field is blank and the new value is blank, this forces a change
+            control.clear()
+            control.send_keys(str(block.fields[field_name].default))
 
         self.click_save()
-        for field_name in EditableXBlock.editable_fields:
-            self.assertEqual(getattr(block, field_name), block.fields[field_name].default)
-            self.assertTrue(block.fields[field_name].is_set_on(block))
+        self.assert_unchanged(block, explicitly_set=True)
 
     def test_set_and_reset(self):
         """
         Test that we can set values, save, then reset to defaults.
         """
         block = self.load_root_xblock()
-        for field_name in EditableXBlock.editable_fields:
-            self.assertFalse(block.fields[field_name].is_set_on(block))
+        self.assert_unchanged(block)
 
         for field_name in EditableXBlock.editable_fields:
             color_control = self.get_element_for_field(field_name)
@@ -106,6 +113,8 @@ class TestEditableXBlock_StudioView(StudioEditableBaseTest):
 
         self.click_save()
 
+        block = self.load_root_xblock()  # Need to reload the block to bypass its cache
+
         self.assertEqual(block.color, '1000')
         self.assertEqual(block.count, 1000)
         self.assertEqual(block.comment, '1000')
@@ -115,19 +124,12 @@ class TestEditableXBlock_StudioView(StudioEditableBaseTest):
 
         self.click_save()
         block = self.load_root_xblock()  # Need to reload the block to bypass its cache
-        for field_name in EditableXBlock.editable_fields:
-            self.assertEqual(getattr(block, field_name), block.fields[field_name].default)
-            self.assertFalse(block.fields[field_name].is_set_on(block))
+        self.assert_unchanged(block)
 
     def test_invalid_data(self):
         """
         Test that we get notified when there's a problem with our data.
         """
-        def assert_unchanged():
-            block = self.load_root_xblock()
-            for field_name in EditableXBlock.editable_fields:
-                self.assertEqual(getattr(block, field_name), block.fields[field_name].default)
-                self.assertFalse(block.fields[field_name].is_set_on(block))
         def expect_error_message(expected_message):
             notification = self.dequeue_runtime_notification()
             self.assertEqual(notification[0], "error")
@@ -147,14 +149,14 @@ class TestEditableXBlock_StudioView(StudioEditableBaseTest):
 
         self.click_save(expect_success=False)
         expect_error_message("Count cannot be negative, No swearing allowed")
-        assert_unchanged()
+        self.assert_unchanged(self.load_root_xblock())
 
         count_control.clear()
         count_control.send_keys('10')
 
         self.click_save(expect_success=False)
         expect_error_message("No swearing allowed")
-        assert_unchanged()
+        self.assert_unchanged(self.load_root_xblock())
 
         comment_control.clear()
 

xblockutils/base_test.py

@@ -115,7 +115,7 @@ class SeleniumBaseTest(SeleniumXBlockTest):
     """
     Selenium Base Test for loading a whole folder of XML scenarios and then running tests.
     This is kept for compatibility, but it is recommended that SeleniumXBlockTest be used
-    instead, since it is faster and more flexible (specifically, senarios are only loaded
+    instead, since it is faster and more flexible (specifically, scenarios are only loaded
     as needed, and can be defined inline with the tests).
     """
     module_name = None  # You must set this to __name__ in any subclass so ResourceLoader can find scenario XML files

xblockutils/helpers.py

@@ -5,11 +5,19 @@ Useful helper methods
 
 def child_isinstance(block, child_id, block_class_or_mixin):
     """
-    Is "block"'s child identified by usage_id "child_id" an instance of
-    "block_class_or_mixin"?
+    Efficiently check if a child of an XBlock is an instance of the given class.
 
-    This is a bit complicated since it avoids the need to actually
-    instantiate the child block.
+    Arguments:
+    block -- the parent (or ancestor) of the child block in question
+    child_id -- the usage key of the child block we are wondering about
+    block_class_or_mixin -- We return true if block's child indentified by child_id is an
+    instance of this.
+
+    This method is equivalent to
+
+        isinstance(block.runtime.get_block(child_id), block_class_or_mixin)
+
+    but is far more efficient, as it avoids the need to instantiate the child.
     """
     def_id = block.runtime.id_reader.get_definition_id(child_id)
     type_name = block.runtime.id_reader.get_block_type(def_id)

xblockutils/studio_editable.py

@@ -32,8 +32,25 @@ loader = ResourceLoader(__name__)
 class FutureFields(object):
     """
     A helper class whose attribute values come from the specified dictionary or fallback object.
+
+    This is only used by StudioEditableXBlockMixin and is not meant to be re-used anywhere else!
+
+    This class wraps an XBlock and makes it appear that some of the block's field values have
+    been changed to new values or deleted (and reset to default values). It does so without
+    actually modifying the XBlock. The only reason we need this is because the XBlock validation
+    API is built around attribute access, but often we want to validate data that's stored in a
+    dictionary before making changes to an XBlock's attributes (since any changes made to the
+    XBlock may get persisted even if validation fails).
     """
     def __init__(self, new_fields_dict, newly_removed_fields, fallback_obj):
+        """
+        Create an instance whose attributes come from new_fields_dict and fallback_obj.
+
+        Arguments:
+        new_fields_dict -- A dictionary of values that will appear as attributes of this object
+        newly_removed_fields -- A list of field names for which we will not use fallback_obj
+        fallback_obj -- An XBlock to use as a provider for any attributes not in new_fields_dict
+        """
         self._new_fields_dict = new_fields_dict
         self._blacklist = newly_removed_fields
         self._fallback_obj = fallback_obj
@@ -123,10 +140,11 @@ class StudioEditableXBlockMixin(object):
             if info["default"] is None:
                 info["default"] = []
             info["default"] = json.dumps(info["default"])
-        if info["type"] == "generic":
+        elif info["type"] == "generic":
             # Convert value to JSON string if we're treating this field generically:
             info["value"] = json.dumps(info["value"])
             info["default"] = json.dumps(info["default"])
+
         if 'values_provider' in field.runtime_options:
             values = field.runtime_options["values_provider"](self)
         else:
@@ -137,24 +155,32 @@ class StudioEditableXBlockMixin(object):
             # Protip: when defining the field, values= can be a callable.
             if isinstance(field.values, dict) and isinstance(field, (Float, Integer)):
                 # e.g. {"min": 0 , "max": 10, "step": .1}
-                info["min"] = field.values["min"]
-                info["max"] = field.values["max"]
-                info["step"] = field.values["step"]
-            else:
-                # e.g. [1, 2, 3] or [ {"display_name": "Always", "value": "always"}, {...}, ... ]
-                if not isinstance(values[0], dict) or "display_name" not in values[0]:
-                    values = [{"display_name": val, "value": val} for val in values]
+                for option in ("min", "max", "step"):
+                    val = field.values.get(option)
+                    if val is None:
+                        raise KeyError("Field is missing required values key '{}'".format(option))
+                    info[option] = val
+            elif isinstance(values[0], dict) and "display_name" in values[0] and "value" in values[0]:
+                # e.g. [ {"display_name": "Always", "value": "always"}, ... ]
+                for value in values:
+                    assert "display_name" in value and "value" in value
                 info['values'] = values
+            else:
+                # e.g. [1, 2, 3] - we need to convert it to the [{"display_name": x, "value": x}] format
+                info['values'] = [{"display_name": unicode(val), "value": val} for val in values]
         if info["type"] in ("list", "set") and field.runtime_options.get('list_values_provider'):
             list_values = field.runtime_options['list_values_provider'](self)
             # list_values must be a list of values or {"display_name": x, "value": y} objects
             # Furthermore, we need to convert all values to JSON since they could be of any type
-            if list_values and (not isinstance(list_values[0], dict) or "display_name" not in list_values[0]):
-                list_values = [json.dumps(val) for val in list_values]
-                list_values = [{"display_name": val, "value": val} for val in list_values]
-            else:
+            if list_values and isinstance(list_values[0], dict) and "display_name" in list_values[0]:
+                # e.g. [ {"display_name": "Always", "value": "always"}, ... ]
                 for entry in list_values:
+                    assert "display_name" in entry and "value" in entry
                     entry["value"] = json.dumps(entry["value"])
+            else:
+                # e.g. [1, 2, 3] - we need to convert it to the [{"display_name": x, "value": x}] format
+                list_values = [json.dumps(val) for val in list_values]
+                list_values = [{"display_name": unicode(val), "value": val} for val in list_values]
             info['list_values'] = list_values
             info['has_list_values'] = True
         return info
@@ -270,9 +296,8 @@ class StudioContainerXBlockMixin(object):
         otherwise just show the normal 'author_preview_view' or 'student_view' preview.
         """
         root_xblock = context.get('root_xblock')
-        is_root = root_xblock and root_xblock.location == self.location
 
-        if is_root:
+        if root_xblock and root_xblock.location == self.location:
             # User has clicked the "View" link. Show an editable preview of this block's children
             return self.author_edit_view(context)
         else:

xblockutils/templates/studio_edit.html

@@ -74,7 +74,7 @@
                       </label>
                     </li>
                   {% empty %}
-                    <li>{% trans "No choices available." %}</li>
+                    <li>{% trans "None Available" %}</li>
                   {% endfor %}
                 </ul>
               </div>