Fix unique ids in loops (#89)

Uses the new DOM logic to prevent document corruption when shapes and images are present in a loop construct. This PR also fixes a backwards compatibility issue that arose in HTML insertion when plain text was a top level node in the HTML being parsed. * Allow DOM entries to be accessed by name using [] * Add logic to Loop operation to update unique docPr and cNvPr tags * Update Loop operation to not require namespaces for docPr and cNvPr tags These can exist under multiple namespaces and while the id attribute probably only has to be unique per namespace it is far simpler to update them all in unison. If this is found to break something then we will address the issue then. * Update mock DOM class to work with new unique ID logic in Loop operation * Add a unit test for unique id incrementing inside loops * Fix duplicate class name in sablon_test This prevented the CV test case from executing, a test case that now fails due to changes to HTML insertion. * Fix test failure due to header text being changed to current time This wasn't checked prior to my changes to checking docx equality since the old method only checked the document.xml instead of every XML file in the archive. * Fix CV test case for Sablon, HTML content needed wrapped in <span> * Add integration test for loops with shapes and images inside them * Add class method to ASTBuilder that checks for any block tags in a nodeset This change also required moving the fetch_tag method to the class level. * Only remove text nodes from root element if block level tags exist This fixes a backwards compatabilty issue created by my refactor of HTML insertion to be "inline" or "block". * Remove span tag wrapping around "about_me" value in CV integration test
senny · Feb 5, 2018 · 4a7b82a · 4a7b82a
1 parent b27f50a
commit 4a7b82a
Show file tree

Hide file tree

Showing 12 changed files with 298 additions and 14 deletions.
diff --git a/lib/sablon/document_object_model/model.rb b/lib/sablon/document_object_model/model.rb
@@ -51,6 +51,11 @@ def initialize(zip_io_stream)
         @dom = build_dom(@zip_contents)
       end
 
+      # Returns the corresponding DOM handled file
+      def [](entry_name)
+        @dom[entry_name]
+      end
+
       private
 
       # Determines how the content in the zip file entry should be wrapped

diff --git a/lib/sablon/html/ast.rb b/lib/sablon/html/ast.rb
@@ -130,8 +130,11 @@ def <<(node)
     class Root < Collection
       def initialize(env, node)
         # strip text nodes from the root level element, these are typically
-        # extra whitespace from indenting the markup
-        node.search('./text()').remove
+        # extra whitespace from indenting the markup if there are any
+        # block level tags at the top level
+        if ASTBuilder.any_block_tags?(node.children)
+          node.search('./text()').remove
+        end
 
         # convert children from HTML to AST nodes
         super(ASTBuilder.html_to_ast(env, node.children, {}))

diff --git a/lib/sablon/html/ast_builder.rb b/lib/sablon/html/ast_builder.rb
@@ -9,6 +9,23 @@ def self.html_to_ast(env, nodes, properties)
         builder.nodes
       end
 
+      # Checks if there are any block level tags in the current node set
+      # this is used at the root level to determine if top level text nodes
+      # should be removed
+      def self.any_block_tags?(nodes)
+        nodes.detect { |node| fetch_tag(node.name).type == :block }
+      end
+
+      # Retrieves a HTMLTag instance from the permitted_html_tags hash or
+      # raises an ArgumentError if the tag is not registered
+      def self.fetch_tag(tag_name)
+        tag_name = tag_name.to_sym
+        unless Sablon::Configuration.instance.permitted_html_tags[tag_name]
+          raise ArgumentError, "Don't know how to handle HTML tag: #{tag_name}"
+        end
+        Sablon::Configuration.instance.permitted_html_tags[tag_name]
+      end
+
       private
 
       def initialize(env, nodes, properties)
@@ -42,11 +59,7 @@ def process_nodes(html_nodes, properties)
       # retrieves a HTMLTag instance from the cpermitted_html_tags hash or
       # raises an ArgumentError if the tag is not registered in the hash
       def fetch_tag(tag_name)
-        tag_name = tag_name.to_sym
-        unless Sablon::Configuration.instance.permitted_html_tags[tag_name]
-          raise ArgumentError, "Don't know how to handle HTML tag: #{tag_name}"
-        end
-        Sablon::Configuration.instance.permitted_html_tags[tag_name]
+        self.class.fetch_tag(tag_name)
       end
 
       # Checking that the current tag is an allowed child of the parent_tag.

diff --git a/lib/sablon/operations.rb b/lib/sablon/operations.rb
@@ -21,8 +21,36 @@ def evaluate(env)
           iter_env = env.alter_context(iterator_name => item)
           block.process(iter_env)
         end
+        update_unique_ids(env, content)
         block.replace(content.reverse)
       end
+
+      private
+
+      # updates all unique id's present in the xml being copied
+      def update_unique_ids(env, content)
+        doc_xml = env.document.zip_contents[env.document.current_entry]
+        dom_entry = env.document[env.document.current_entry]
+        #
+        # update all docPr tags created
+        selector = "//*[local-name() = 'docPr']"
+        init_id_val = dom_entry.max_attribute_value(doc_xml, selector, 'id')
+        update_tag_attribute(content, 'docPr', 'id', init_id_val)
+        #
+        # update all cNvPr tags created
+        selector = "//*[local-name() = 'cNvPr']"
+        init_id_val = dom_entry.max_attribute_value(doc_xml, selector, 'id')
+        update_tag_attribute(content, 'cNvPr', 'id', init_id_val)
+      end
+
+      # Increments the attribute value of each element with the id by 1
+      def update_tag_attribute(content, tag_name, attr_name, init_val)
+        content.each do |nodeset|
+          nodeset.xpath(".//*[local-name() = '#{tag_name}']").each do |node|
+            node[attr_name] = (init_val += 1).to_s
+          end
+        end
+      end
     end
 
     class Condition < Struct.new(:conditon_expr, :block, :predicate)

diff --git a/test/fixtures/cv_sample.docx b/test/fixtures/cv_sample.docx
diff --git a/test/fixtures/loops_sample.docx b/test/fixtures/loops_sample.docx
diff --git a/test/fixtures/loops_template.docx b/test/fixtures/loops_template.docx
diff --git a/test/fixtures/xml/loop_with_unique_ids.xml b/test/fixtures/xml/loop_with_unique_ids.xml
@@ -0,0 +1,152 @@
+<w:p>
+  <w:pPr>
+    <w:rPr>
+      <w:noProof/>
+    </w:rPr>
+  </w:pPr>
+</w:p>
+<w:p>
+  <w:pPr>
+    <w:rPr>
+      <w:noProof/>
+    </w:rPr>
+  </w:pPr>
+</w:p>
+<w:p>
+  <w:r>
+    <w:fldChar w:fldCharType="begin"/>
+  </w:r>
+  <w:r>
+    <w:instrText xml:space="preserve"> MERGEFIELD </w:instrText>
+  </w:r>
+  <w:r>
+    <w:instrText>cars</w:instrText>
+  </w:r>
+  <w:r>
+    <w:instrText xml:space="preserve">:each(item) \* MERGEFORMAT </w:instrText>
+  </w:r>
+  <w:r>
+    <w:fldChar w:fldCharType="separate"/>
+  </w:r>
+  <w:r>
+    <w:rPr>
+      <w:noProof/>
+    </w:rPr>
+    <w:t>«cars:each(item)»</w:t>
+  </w:r>
+  <w:r>
+    <w:rPr>
+      <w:noProof/>
+    </w:rPr>
+    <w:fldChar w:fldCharType="end"/>
+  </w:r>
+</w:p>
+<w:p>
+  <w:pPr>
+    <w:pStyle w:val="ListParagraph"/>
+    <w:numPr>
+      <w:ilvl w:val="0"/>
+      <w:numId w:val="1"/>
+    </w:numPr>
+  </w:pPr>
+  <w:r>
+    <w:fldChar w:fldCharType="begin"/>
+  </w:r>
+  <w:r>
+    <w:instrText xml:space="preserve"> MERGEFIELD =item.name \* MERGEFORMAT </w:instrText>
+  </w:r>
+  <w:r>
+    <w:fldChar w:fldCharType="separate"/>
+  </w:r>
+  <w:r>
+    <w:rPr>
+      <w:noProof/>
+    </w:rPr>
+    <w:t>«=item.name»</w:t>
+  </w:r>
+  <w:r>
+    <w:fldChar w:fldCharType="end"/>
+  </w:r>
+  <w:r>
+    <w:t xml:space="preserve">
+    </w:t>
+  </w:r>
+  <w:r>
+    <w:rPr>
+      <w:noProof/>
+    </w:rPr>
+    <w:drawing>
+      <wp:inline distT="0" distB="0" distL="0" distR="0">
+        <wp:extent cx="1254868" cy="889278"/>
+        <wp:effectExtent l="0" t="0" r="2540" b="0"/>
+        <wp:docPr id="2" name="Picture 3"/>
+        <wp:cNvGraphicFramePr>
+          <a:graphicFrameLocks xmlns:a="http://schemas.openxmlformats.org/drawingml/2006/main" noChangeAspect="1"/>
+        </wp:cNvGraphicFramePr>
+        <a:graphic xmlns:a="http://schemas.openxmlformats.org/drawingml/2006/main">
+          <a:graphicData uri="http://schemas.openxmlformats.org/drawingml/2006/picture">
+            <pic:pic xmlns:pic="http://schemas.openxmlformats.org/drawingml/2006/picture">
+              <pic:nvPicPr>
+                <pic:cNvPr id="3" name="MTF 500.pdf"/>
+                <pic:cNvPicPr/>
+              </pic:nvPicPr>
+              <pic:blipFill>
+                <a:blip r:embed="rId5" cstate="print">
+                  <a:extLst>
+                    <a:ext uri="{28A0092B-C50C-407E-A947-70E740481C1C}">
+                      <a14:useLocalDpi xmlns:a14="http://schemas.microsoft.com/office/drawing/2010/main" val="0"/>
+                    </a:ext>
+                  </a:extLst>
+                </a:blip>
+                <a:stretch>
+                  <a:fillRect/>
+                </a:stretch>
+              </pic:blipFill>
+              <pic:spPr>
+                <a:xfrm>
+                  <a:off x="0" y="0"/>
+                  <a:ext cx="1286502" cy="911696"/>
+                </a:xfrm>
+                <a:prstGeom prst="rect">
+                  <a:avLst/>
+                </a:prstGeom>
+              </pic:spPr>
+            </pic:pic>
+          </a:graphicData>
+        </a:graphic>
+      </wp:inline>
+    </w:drawing>
+  </w:r>
+  <w:r>
+    <w:br/>
+  </w:r>
+</w:p>
+<w:p>
+  <w:r>
+    <w:fldChar w:fldCharType="begin"/>
+  </w:r>
+  <w:r>
+    <w:instrText xml:space="preserve"> MERGEFIELD </w:instrText>
+  </w:r>
+  <w:r>
+    <w:instrText>cars</w:instrText>
+  </w:r>
+  <w:r>
+    <w:instrText xml:space="preserve">:endEach \* MERGEFORMAT </w:instrText>
+  </w:r>
+  <w:r>
+    <w:fldChar w:fldCharType="separate"/>
+  </w:r>
+  <w:r>
+    <w:rPr>
+      <w:noProof/>
+    </w:rPr>
+    <w:t>«cars:endEach»</w:t>
+  </w:r>
+  <w:r>
+    <w:rPr>
+      <w:noProof/>
+    </w:rPr>
+    <w:fldChar w:fldCharType="end"/>
+  </w:r>
+</w:p>
diff --git a/test/fixtures/xml/mock_document/word/document.xml b/test/fixtures/xml/mock_document/word/document.xml
@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<w:document xmlns:wpc="http://schemas.microsoft.com/office/word/2010/wordprocessingCanvas" xmlns:mo="http://schemas.microsoft.com/office/mac/office/2008/main" xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" xmlns:mv="urn:schemas-microsoft-com:mac:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:r="http://schemas.openxmlformats.org/officeDocument/2006/relationships" xmlns:m="http://schemas.openxmlformats.org/officeDocument/2006/math" xmlns:v="urn:schemas-microsoft-com:vml" xmlns:wp14="http://schemas.microsoft.com/office/word/2010/wordprocessingDrawing" xmlns:wp="http://schemas.openxmlformats.org/drawingml/2006/wordprocessingDrawing" xmlns:w10="urn:schemas-microsoft-com:office:word" xmlns:w="http://schemas.openxmlformats.org/wordprocessingml/2006/main" xmlns:w14="http://schemas.microsoft.com/office/word/2010/wordml" xmlns:w15="http://schemas.microsoft.com/office/word/2012/wordml" xmlns:wpg="http://schemas.microsoft.com/office/word/2010/wordprocessingGroup" xmlns:wpi="http://schemas.microsoft.com/office/word/2010/wordprocessingInk" xmlns:wne="http://schemas.microsoft.com/office/word/2006/wordml" xmlns:wps="http://schemas.microsoft.com/office/word/2010/wordprocessingShape" mc:Ignorable="w14 w15 wp14">
+  <w:body>
+    <w:p></w:p>
+    <w:sectPr>
+      <w:pgSz w:w="12240" w:h="15840"/>
+      <w:pgMar w:top="1440" w:right="1440" w:bottom="1440" w:left="1440" w:header="720" w:footer="720" w:gutter="0"/>
+      <w:cols w:space="720"/>
+      <w:docGrid w:linePitch="360"/>
+    </w:sectPr>
+  </w:body>
+</w:document>
diff --git a/test/processor/document_test.rb b/test/processor/document_test.rb
@@ -311,7 +311,6 @@ def test_single_row_table_loop
         </w:tc>
       </w:tr>
     </w:tbl>
-
     document
   end
 
@@ -348,6 +347,22 @@ def test_loop_with_missing_end_raises_error
     assert_equal "Could not find end field for «technologies:each(technology)». Was looking for «technologies:endEach»", e.message
   end
 
+  def test_loop_incrementing_unique_ids
+    context = {
+      fruits: %w[Apple Blueberry Cranberry Date].map { |i| { name: i } },
+      cars: %w[Silverado Serria Ram Tundra].map { |i| { name: i } }
+    }
+    #
+    xml = Nokogiri::XML(process(snippet('loop_with_unique_ids'), context))
+    #
+    # all unique ids should get incremented to stay unique
+    ids = xml.xpath("//*[local-name() = 'docPr']").map { |n| n.attr('id') }
+    assert_equal %w[1 2 3 4], ids
+    #
+    ids = xml.xpath("//*[local-name() = 'cNvPr']").map { |n| n.attr('id') }
+    assert_equal %w[1 2 3 4], ids
+  end
+
   def test_conditional_with_missing_end_raises_error
     e = assert_raises Sablon::TemplateError do
       process(snippet("conditional_without_ending"), {})
@@ -446,7 +461,7 @@ def test_comment_block_and_comment_as_key
     assert_xml_equal <<-document, result
     <w:r><w:t xml:space="preserve">Before </w:t></w:r>
     <w:r><w:t xml:space="preserve">After </w:t></w:r>
-    <w:p>           
+    <w:p>
       <w:r w:rsidR="004B49F0">
         <w:rPr><w:noProof/></w:rPr>
         <w:t>Contents of comment key</w:t>
@@ -458,7 +473,8 @@ def test_comment_block_and_comment_as_key
   private
 
   def process(document, context)
-    env = Sablon::Environment.new(nil, context)
+    env = Sablon::Environment.new(MockTemplate.new, context)
+    env.document.current_entry = 'word/document.xml'
     @processor.process(wrap(document), env).to_xml
   end
 end
diff --git a/test/sablon_test.rb b/test/sablon_test.rb
@@ -23,7 +23,7 @@ def test_generate_document_from_template
     referee = Struct.new(:name, :company, :position, :phone)
 
     context = {
-      current_time: Time.now.strftime("%d.%m.%Y %H:%M"),
+      current_time: '15.04.2015 14:57',
       metadata: { generator: "Sablon" },
       title: "Resume",
       person: OpenStruct.new("first_name" => "Ronald", "last_name" => "Anderson",
@@ -84,7 +84,7 @@ def test_generate_document_from_template
   end
 end
 
-class SablonTest < Sablon::TestCase
+class SablonConditionalsTest < Sablon::TestCase
   include XMLSnippets
 
   def setup
@@ -97,7 +97,38 @@ def setup
 
   def test_generate_document_from_template
     template = Sablon.template @template_path
-    context = {paragraph: true, inline: true, table: true, table_inline: true, content: "Some Content"}
+    context = {
+      paragraph: true,
+      inline: true,
+      table: true,
+      table_inline: true,
+      content: "Some Content"
+    }
+    #
+    context = { paragraph: true, inline: true, table: true, table_inline: true, content: "Some Content" }
+    template.render_to_file @output_path, context
+    assert_docx_equal @sample_path, @output_path
+  end
+end
+
+class SablonLoopsTest < Sablon::TestCase
+  include XMLSnippets
+
+  def setup
+    super
+    @base_path = Pathname.new(File.expand_path("../", __FILE__))
+    @template_path = @base_path + "fixtures/loops_template.docx"
+    @output_path = @base_path + "sandbox/loops.docx"
+    @sample_path = @base_path + "fixtures/loops_sample.docx"
+  end
+
+  def test_generate_document_from_template
+    template = Sablon.template @template_path
+    context = {
+      fruits: %w[Apple Blueberry Cranberry Date].map { |i| { name: i } },
+      cars: %w[Silverado Serria Ram Tundra].map { |i| { name: i } }
+    }
+
     template.render_to_file @output_path, context
     assert_docx_equal @sample_path, @output_path
   end