From 40ff86598966b57e84c35d44971bcf20af09a765 Mon Sep 17 00:00:00 2001
From: Veronica Berglyd Olsen <1619840+vkbo@users.noreply.github.com>
Date: Fri, 11 Jun 2021 00:01:58 +0200
Subject: [PATCH] Make XML parser for items less picky

---
 nw/core/item.py                   |  7 ++++---
 tests/test_core/test_core_item.py | 34 ++++++++++++++++---------------
 2 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/nw/core/item.py b/nw/core/item.py
index 18eada78..6ed3f515 100644
--- a/nw/core/item.py
+++ b/nw/core/item.py
@@ -106,7 +106,6 @@ class NWItem():
         if "order" in xItem.attrib:
             self.setOrder(xItem.attrib["order"])
 
-        retStatus = True
         for xValue in xItem:
             if xValue.tag == "name":
                 self.setName(xValue.text)
@@ -131,10 +130,12 @@ class NWItem():
             elif xValue.tag == "cursorPos":
                 self.setCursorPos(xValue.text)
             else:
+                # Sliently skip as we may otherwise cause orphaned
+                # items if an otherwise valid file is opened by a
+                # version of novelWriter that doesn't know the tag.
                 logger.error("Unknown tag '%s'" % xValue.tag)
-                retStatus = False
 
-        return retStatus
+        return True
 
     @staticmethod
     def _subPack(xParent, name, attrib=None, text=None, none=True):
diff --git a/tests/test_core/test_core_item.py b/tests/test_core/test_core_item.py
index e4ed227e..c93140fe 100644
--- a/tests/test_core/test_core_item.py
+++ b/tests/test_core/test_core_item.py
@@ -270,7 +270,7 @@ def testCoreItem_LayoutSetter(dummyGUI):
 # END Test testCoreItem_LayoutSetter
 
 @pytest.mark.core
-def testCoreItem_XMLPackUnpack(dummyGUI):
+def testCoreItem_XMLPackUnpack(dummyGUI, caplog):
     """Test packing and unpacking XML objects for the NWItem class.
     """
     theProject = NWProject(dummyGUI)
@@ -370,31 +370,33 @@ def testCoreItem_XMLPackUnpack(dummyGUI):
     # Errors
 
     ## Not an Item
-    xDummy = etree.SubElement(nwXML, "stuff")
-    assert not theItem.unpackXML(xDummy)
+    mockXml = etree.SubElement(nwXML, "stuff")
+    assert theItem.unpackXML(mockXml) is False
 
     ## Item without Handle
-    xDummy = etree.SubElement(nwXML, "item", attrib={"stuff": "nah"})
-    assert not theItem.unpackXML(xDummy)
+    mockXml = etree.SubElement(nwXML, "item", attrib={"stuff": "nah"})
+    assert theItem.unpackXML(mockXml) is False
 
-    ## Item with Invalid SubElement
-    xDummy = etree.SubElement(nwXML, "item", attrib={"handle": "0123456789abc"})
-    xParam = etree.SubElement(xDummy, "invalid")
+    ## Item with Invalid SubElement is Accepted w/Error
+    mockXml = etree.SubElement(nwXML, "item", attrib={"handle": "0123456789abc"})
+    xParam = etree.SubElement(mockXml, "invalid")
     xParam.text = "stuff"
-    assert not theItem.unpackXML(xDummy)
+    caplog.clear()
+    assert theItem.unpackXML(mockXml) is True
+    assert "Unknown tag 'invalid'" in caplog.text
 
     # Pack Valid Item
-    xDummy = etree.SubElement(nwXML, "group")
-    theItem._subPack(xDummy, "subGroup", {"one": "two"}, "value", False)
-    assert etree.tostring(xDummy, pretty_print=False, encoding="utf-8") == (
+    mockXml = etree.SubElement(nwXML, "group")
+    theItem._subPack(mockXml, "subGroup", {"one": "two"}, "value", False)
+    assert etree.tostring(mockXml, pretty_print=False, encoding="utf-8") == (
         b"<group><subGroup one=\"two\">value</subGroup></group>"
     )
 
     # Pack Not Allowed None
-    xDummy = etree.SubElement(nwXML, "group")
-    assert theItem._subPack(xDummy, "subGroup", {}, None, False) is None
-    assert theItem._subPack(xDummy, "subGroup", {}, "None", False) is None
-    assert etree.tostring(xDummy, pretty_print=False, encoding="utf-8") == (
+    mockXml = etree.SubElement(nwXML, "group")
+    assert theItem._subPack(mockXml, "subGroup", {}, None, False) is None
+    assert theItem._subPack(mockXml, "subGroup", {}, "None", False) is None
+    assert etree.tostring(mockXml, pretty_print=False, encoding="utf-8") == (
         b"<group/>"
     )