fix: tests

Author: irtazaakram

xblocks_contrib/html/html.py

@@ -24,7 +24,7 @@
 from path import Path as path
 from web_fragments.fragment import Fragment
 from xblock.core import XML_NAMESPACES, XBlock
-from xblock.fields import Boolean, Dict, List, Scope, String, UserScope
+from xblock.fields import Boolean, Dict, Scope, ScopeIds, String, UserScope
 from xblock.utils.resources import ResourceLoader
 
 log = logging.getLogger(__name__)
@@ -213,6 +213,38 @@ def serialize_field(value):
     return json.dumps(value, cls=EdxJSONEncoder)
 
 
+def deserialize_field(field, value):
+    """
+    Deserialize the string version to the value stored internally.
+
+    Note that this is not the same as the value returned by from_json, as model types typically store
+    their value internally as JSON. By default, this method will return the result of calling json.loads
+    on the supplied value, unless json.loads throws a TypeError, or the type of the value returned by json.loads
+    is not supported for this class (from_json throws an Error). In either of those cases, this method returns
+    the input value.
+    """
+    try:
+        deserialized = json.loads(value)
+        if deserialized is None:
+            return deserialized
+        try:
+            field.from_json(deserialized)
+            return deserialized
+        except (ValueError, TypeError):
+            # Support older serialized version, which was just a string, not result of json.dumps.
+            # If the deserialized version cannot be converted to the type (via from_json),
+            # just return the original value. For example, if a string value of '3.4' was
+            # stored for a String field (before we started storing the result of json.dumps),
+            # then it would be deserialized as 3.4, but 3.4 is not supported for a String
+            # field. Therefore field.from_json(3.4) will throw an Error, and we should
+            # actually return the original value of '3.4'.
+            return value
+
+    except (ValueError, TypeError):
+        # Support older serialized version.
+        return value
+
+
 def own_metadata(block):
     """
     Return a JSON-friendly dictionary that contains only non-inherited field
@@ -298,18 +330,6 @@ class HtmlBlock(XBlock):
 
     metadata_to_export_to_policy = ("discussion_topics",)
 
-    children = List(
-        help=_("Child blocks of this XBlock."),
-        default=[],
-        scope=Scope.content,
-    )
-
-    url_name = String(
-        help=_("Unique URL-friendly identifier of the block."),
-        scope=Scope.settings,
-        default="",
-    )
-
     @property
     def category(self):
         return self.scope_ids.block_type
@@ -327,7 +347,7 @@ def location(self, value):
         )
 
     @property
-    def _url_name(self):
+    def url_name(self):
         return self.location.block_id
 
     @property
@@ -669,6 +689,40 @@ def load_definition(cls, xml_object, system, location, id_generator):  # pylint:
                 # add more info and re-raise
                 raise Exception(msg).with_traceback(sys.exc_info()[2])
 
+    @classmethod
+    def load_metadata(cls, xml_object):
+        """
+        Read the metadata attributes from this xml_object.
+
+        Returns a dictionary {key: value}.
+        """
+        metadata = {"xml_attributes": {}}
+        for attr, val in xml_object.attrib.items():
+
+            if attr in cls.metadata_to_strip:
+                # don't load these
+                continue
+
+            if attr not in cls.fields:  # pylint: disable=unsupported-membership-test
+                metadata["xml_attributes"][attr] = val
+            else:
+                metadata[attr] = deserialize_field(cls.fields[attr], val)  # pylint: disable=unsubscriptable-object
+        return metadata
+
+    @classmethod
+    def apply_policy(cls, metadata, policy):
+        """
+        Add the keys in policy to metadata, after processing them
+        through the attrmap.  Updates the metadata dict in place.
+        """
+        for attr, value in policy.items():
+            if attr not in cls.fields:  # pylint: disable=unsupported-membership-test
+                # Store unknown attributes coming from policy.json
+                # in such a way that they will export to xml unchanged
+                metadata["xml_attributes"][attr] = value
+            else:
+                metadata[attr] = value
+
     @classmethod
     def parse_xml(cls, node, runtime, keys):
         """
@@ -686,6 +740,15 @@ def parse_xml(cls, node, runtime, keys):
 
         """
 
+        if keys is None:
+            # Passing keys=None is against the XBlock API but some platform tests do it.
+            def_id = runtime.id_generator.create_definition(node.tag, node.get("url_name"))
+            keys = ScopeIds(None, node.tag, def_id, runtime.id_generator.create_usage(def_id))
+        aside_children = []
+
+        # Let the runtime construct the block. It will have a proper, inheritance-aware field data store.
+        block = runtime.construct_xblock_from_class(cls, keys)
+
         # VS[compat]
         # In 2012, when the platform didn't have CMS, and all courses were handwritten XML files, problem tags
         # contained XML problem descriptions withing themselves. Later, when Studio has been created, and "pointer" tags
@@ -696,32 +759,70 @@ def parse_xml(cls, node, runtime, keys):
         if is_pointer_tag(node):
             # new style:
             # read the actual definition file--named using url_name.replace(':','/')
-            definition_xml, _ = cls.load_definition_xml(node, runtime, keys.def_id)
+            definition_xml, filepath = cls.load_definition_xml(node, runtime, keys.def_id)
+            aside_children = runtime.parse_asides(definition_xml, keys.def_id, keys.usage_id, runtime.id_generator)
         else:
+            filepath = None
             definition_xml = node
 
-        block = runtime.construct_xblock_from_class(cls, keys)
+        # Note: removes metadata.
+        definition, children = cls.load_definition(definition_xml, runtime, keys.def_id, runtime.id_generator)
+
+        # VS[compat]
+        # Make Ike's github preview links work in both old and new file layouts.
+        if is_pointer_tag(node):
+            # new style -- contents actually at filepath
+            definition["filename"] = [filepath, filepath]
+
+        metadata = cls.load_metadata(definition_xml)
+
+        # move definition metadata into dict
+        dmdata = definition.get("definition_metadata", "")
+        if dmdata:
+            metadata["definition_metadata_raw"] = dmdata
+            try:
+                metadata.update(json.loads(dmdata))
+            except Exception as err:  # lint-amnesty, pylint: disable=broad-except
+                log.debug("Error in loading metadata %r", dmdata, exc_info=True)
+                metadata["definition_metadata_err"] = str(err)
+
+        definition_aside_children = definition.pop("aside_children", None)
+        if definition_aside_children:
+            aside_children.extend(definition_aside_children)
+
+        # Set/override any metadata specified by policy
+        cls.apply_policy(metadata, runtime.get_policy(keys.usage_id))
+
+        field_data = {**metadata, **definition}
+
+        for field_name, value in field_data.items():
+            # The 'xml_attributes' field has a special setter logic in its Field class,
+            # so we must handle it carefully to avoid duplicating data.
+            if field_name == "xml_attributes":
+                # The 'filename' attribute is specially handled for git links.
+                value["filename"] = definition.get("filename", ["", None])
+                block.xml_attributes.update(value)
+            elif field_name in block.fields:
+                setattr(block, field_name, value)
+
+        block.children = children
 
-        for source_xml in [node, definition_xml]:
-            for name, value in source_xml.items():
-                if name in block.fields:
-                    field = block.fields[name]
-                    try:
-                        converted_value = field.from_string(value)
-                        setattr(block, name, converted_value)
-                    except Exception:  # pylint: disable=broad-exception-caught
-                        log.warning("Could not set field '%s' from string value '%s'", name, value, exc_info=True)
-                elif not hasattr(block, "xml_attributes") or name not in block.xml_attributes:
-                    if not hasattr(block, "xml_attributes"):
-                        block.xml_attributes = {}
-                    block.xml_attributes[name] = value
-
-        definition, _ = cls.load_definition(definition_xml, runtime, keys.def_id, runtime.id_generator)
-        if "data" in definition:
-            block.data = definition["data"]
+        if aside_children:
+            cls.add_applicable_asides_to_block(block, runtime, aside_children)
 
         return block
 
+    @classmethod
+    def add_applicable_asides_to_block(cls, block, runtime, aside_children):
+        """
+        Add asides to the block. Moved this out of the parse_xml method to use it in the VideoBlock.parse_xml
+        """
+        asides_tags = [aside_child.tag for aside_child in aside_children]
+        asides = runtime.get_asides(block)
+        for aside in asides:
+            if aside.scope_ids.block_type in asides_tags:
+                block.add_aside(aside)
+
     @classmethod
     def parse_xml_new_runtime(cls, node, runtime, keys):
         """
@@ -823,8 +924,6 @@ def definition_to_xml(self, resource_fs):
         This version creates a self-contained definition file that includes
         all necessary metadata for a successful re-import.
         """
-        if not self.url_name:
-            self.url_name = self.location.block_id
 
         # Write html to file, return an empty tag
         pathname = name_to_pathname(self.url_name)
@@ -840,16 +939,6 @@ def definition_to_xml(self, resource_fs):
 
         elt = etree.Element("html")
         elt.set("filename", relname)
-
-        # This loop adds other metadata attributes from the block.
-        for field_name in self.fields:  # pylint: disable=not-an-iterable
-            # pylint: disable=unsubscriptable-object
-            if field_name not in ["data", "children", "parent"] and self.fields[field_name].is_set_on(self):
-                field = self.fields[field_name]  # pylint: disable=unsubscriptable-object
-                value = field.read_from(self)
-                if value is not None:
-                    elt.set(field_name, str(value))
-
         return elt
 
     @property