Merge pull request #64 from learningtapestry/43-fix-lesson-metadata-specs

paranoicsan · web-flow · commit 6d9ea9c3e9ab · 2026-06-11T10:10:43.000+07:00
(#43) Complete new-spec lesson-metadata migration in resource pipeline
diff --git a/app/controllers/documents_controller.rb b/app/controllers/documents_controller.rb
@@ -14,8 +14,8 @@ class DocumentsController < Admin::AdminController
   def preview_pdf
     link_keys = %w(preview pdf)
 
-    if !ENV.fetch("FORCE_PREVIEW_GENERATION", false) && (url = @document.preview_links.dig(*link_keys)).present?
-      return redirect_to url
+    if !ENV.fetch("FORCE_PREVIEW_GENERATION", false) && (url = @document.preview_links.dig(*link_keys, "url")).present?
+      return redirect_to url, allow_other_host: true
     end
 
     job_options = {
@@ -34,8 +34,8 @@ def preview_pdf
   def preview_gdoc
     link_keys = %w(preview gdoc)
 
-    if !ENV.fetch("FORCE_PREVIEW_GENERATION", false) && (url = @document.preview_links.dig(*link_keys)).present?
-      return redirect_to url
+    if !ENV.fetch("FORCE_PREVIEW_GENERATION", false) && (url = @document.preview_links.dig(*link_keys, "url")).present?
+      return redirect_to url, allow_other_host: true
     end
 
     job_options = {
diff --git a/app/controllers/materials_controller.rb b/app/controllers/materials_controller.rb
@@ -8,8 +8,8 @@ class MaterialsController < Admin::AdminController
   def preview_pdf
     link_keys = %w(preview pdf)
 
-    if !ENV.fetch("FORCE_PREVIEW_GENERATION", false) && (url = @material.preview_links.dig(*link_keys)).present?
-      return redirect_to url
+    if !ENV.fetch("FORCE_PREVIEW_GENERATION", false) && (url = @material.preview_links.dig(*link_keys, "url")).present?
+      return redirect_to url, allow_other_host: true
     end
 
     job_options = {
@@ -28,8 +28,8 @@ def preview_pdf
   def preview_gdoc
     link_keys = %w(preview gdoc)
 
-    if !ENV.fetch("FORCE_PREVIEW_GENERATION", false) && (url = @material.preview_links.dig(*link_keys)).present?
-      return redirect_to url
+    if !ENV.fetch("FORCE_PREVIEW_GENERATION", false) && (url = @material.preview_links.dig(*link_keys, "url")).present?
+      return redirect_to url, allow_other_host: true
     end
 
     job_options = {
diff --git a/app/models/document.rb b/app/models/document.rb
@@ -115,13 +115,7 @@ def tmp_link(key)
   def clean_curriculum_metadata
     return unless metadata.present?
 
-    # downcase subjects
     metadata["subject"] = metadata["subject"]&.downcase
-
-    # to store only the lesson number
-    # or alphanumeric - needed by OPR type, see https://github.com/learningtapestry/unbounded/issues/557
-    lesson = metadata["lesson"]
-    metadata["lesson"] = lesson.match(/lesson (\w+)/i).try(:[], 1) || lesson if lesson.present?
   end
 
   def destroy_connected_resource
diff --git a/app/services/html_sanitizer.rb b/app/services/html_sanitizer.rb
@@ -121,7 +121,8 @@ def default_config
           "tr" => %w(style)
         },
         protocols: {
-          "a" => { "href" => ["http", "https", :relative] }
+          "a" => { "href" => ["http", "https", :relative] },
+          "img" => { "src" => ["data", "http", "https", :relative] }
         },
         css: {
           properties: %w(background-color border-bottom-width border-left-width border-right-width border-top-width
@@ -233,8 +234,10 @@ def fix_external_target(node)
     end
 
     def fix_inline_img(node)
-      # TODO: test if it's working fine with all inline images
-      node["src"] = node["src"].gsub!(/%(20|0A)/, "") if node["src"].to_s.start_with?("data:")
+      return unless node["src"].to_s.start_with?("data:")
+
+      # `gsub!` returns nil when nothing matches — guard against wiping a clean src.
+      node["src"] = node["src"].gsub(/%(20|0A)/, "")
     end
 
     def fix_googlechart_img(node)
diff --git a/app/services/section_resource_upsert_service.rb b/app/services/section_resource_upsert_service.rb
@@ -35,12 +35,12 @@ def context
 
   def context_metadata
     {
-      description: description_summary,
-      grade: metadata[:grade].to_s,
-      section: metadata[:section_number].to_s,
-      subject: metadata[:subject],
-      title:,
-      unit: metadata[:unit_id].to_s
+      "description" => description_summary,
+      "grade" => metadata[:grade].to_s,
+      "section-number" => metadata[:section_number].to_s,
+      "subject" => metadata[:subject],
+      "title" => title,
+      "unit-id" => metadata[:unit_id].to_s
     }
   end
 
diff --git a/app/services/unit_resource_upsert_service.rb b/app/services/unit_resource_upsert_service.rb
@@ -34,11 +34,11 @@ def context
 
   def context_metadata
     {
-      description: description_summary,
-      grade: metadata[:grade],
-      subject: metadata[:subject],
-      title:,
-      unit: short_title
+      "description" => description_summary,
+      "grade" => metadata[:grade],
+      "subject" => metadata[:subject],
+      "title" => title,
+      "unit-id" => short_title
     }
   end
 
diff --git a/app/views/admin/documents/_header.html.erb b/app/views/admin/documents/_header.html.erb
@@ -5,7 +5,7 @@
         <div class="o-title u-text--uppercase">
           <span class="o-title__type"><%= document.short_title %></span>
         </div>
-        <h1><%= document.title %></h1>
+        <h1><%= document.lesson_title %></h1>
         <div class="u-txt--teaser o-social-sharing__teaser">
           <%= raw(document.teaser) %>
         </div>
diff --git a/app/views/documents/export_gdoc.html.erb b/app/views/documents/export_gdoc.html.erb
@@ -1,4 +1,4 @@
-<% set_page_title @document.title.html_safe %>
+<% set_page_title @document.lesson_title.html_safe %>
 
 <style >
   <%= raw(@document.css_styles) %>
diff --git a/app/views/documents/gdoc/_header.html.erb b/app/views/documents/gdoc/_header.html.erb
@@ -1,4 +1,4 @@
-<p class="title u-txt--title"><span><%= document.title %></span></p>
+<p class="title u-txt--title"><span><%= document.lesson_title %></span></p>
 <% if document.teaser.present? %>
   <p class="subtitle u-txt--header-teaser">
     <%= raw document.teaser  %>
diff --git a/lib/doc_template/tables/lms_materials.rb b/lib/doc_template/tables/lms_materials.rb
@@ -8,8 +8,9 @@ class LmsMaterials < Base
 
       # Parses the first lms-materials table found in the fragment.
       # Returns an Array of { "material-id" => ..., "access-type" => ... } hashes.
+      # Accepts both bare ("lms-materials") and bracketed ("[lms-materials]") header forms.
       def parse(fragment, *_args)
-        el = fragment.at_xpath(xpath_meta_headers, XpathFunctions.new)
+        el = fragment.at_xpath(".//table/*/tr[1]/td[1][contains(., '#{self.class::HEADER_LABEL}')]")
         return [] unless el
 
         table = self.class.flatten_table(el.ancestors("table").first)
diff --git a/lib/doc_template/tags/material_tag.rb b/lib/doc_template/tags/material_tag.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module DocTemplate
+  module Tags
+    class MaterialTag < BaseTag
+      TAG_NAME = "material"
+
+      def parse(node, opts = {})
+        @opts = opts
+        identifier = opts[:value].to_s.strip.downcase
+        material = ::Material.find_by(identifier: identifier)
+
+        @content =
+          if material
+            %(<a href="/materials/#{material.id}" class="o-ld-material" target="_blank" rel="noopener">) \
+              "#{identifier}</a>"
+          else
+            %(<span class="badge text-bg-danger">Unknown material: #{identifier}</span>)
+          end
+
+        replace_tag node
+        self
+      end
+    end
+  end
+
+  Template.register_tag(Tags::MaterialTag::TAG_NAME, Tags::MaterialTag)
+end
diff --git a/lib/lt/lcms/metadata/context.rb b/lib/lt/lcms/metadata/context.rb
@@ -147,49 +147,45 @@ def last_item?(index)
         end
 
         def lesson
-          @lesson ||=
-            if (value = context[:lesson].to_s.downcase) =~ RE_NUMBER
-              value.to_i
-            else
-              value
-            end
+          @lesson ||= numerize(context["lesson-number"])
         end
 
         def unit
-          @unit ||=
-            if (value = context[:unit].to_s.downcase) =~ RE_NUMBER
-              value.to_i
-            else
-              value
-            end
+          @unit ||= numerize(context["unit-id"])
         end
 
         def subject
           @subject ||= begin
-            value = context[:subject]&.downcase
-            value if SUBJECTS.include?(value)
+            value = context["subject"]&.downcase
+            if value.blank?
+              nil
+            elsif SUBJECTS.include?(value)
+              value
+            else
+              raise "Unsupported subject #{value.inspect}; allowed: #{SUBJECTS.keys.join(', ')}"
+            end
           end
         end
 
         def teaser
-          context[:teaser]
+          context["teaser"]
         end
 
         def title
-          context[:title].presence || default_title
+          context["title"].presence || default_title
         end
 
         def type
-          context[:type]&.downcase
+          context["type"]&.downcase
         end
 
         def section
-          @section ||=
-            if (value = context[:section].to_s.downcase) =~ RE_NUMBER
-              value.to_i
-            else
-              value
-            end
+          @section ||= numerize(context["section-number"])
+        end
+
+        def numerize(value)
+          value = value.to_s.downcase
+          value =~ RE_NUMBER ? value.to_i : value
         end
 
         def update(resource)
@@ -205,9 +201,13 @@ def update(resource)
         end
 
         def set_lesson_position(parent, resource)
+          raise "Cannot place lesson without parent resource (insufficient curriculum metadata: #{directory.inspect})" \
+            if parent.nil?
+
+          current_lesson = lesson.to_s[RE_NUM].to_i
           next_lesson = parent.children.detect do |r|
             # first lesson with a bigger lesson num
-            r.metadata["lesson"].to_s[RE_NUM].to_i > context[:lesson].to_s[RE_NUM].to_i
+            r.metadata["lesson"].to_s[RE_NUM].to_i > current_lesson
           end
           next_lesson ? next_lesson.prepend_sibling(resource) : resource.save!
         end
diff --git a/spec/controllers/api/document_jobs_controller_spec.rb b/spec/controllers/api/document_jobs_controller_spec.rb
@@ -25,7 +25,7 @@
       # The real FailedExecution.message reads error["message"] from the
       # serialized JSON column. We mock the delegated method directly.
       let(:failed_execution) { instance_double(SolidQueue::FailedExecution, message: "boom") }
-      let(:sq_job)           { instance_double(SolidQueue::Job, failed_execution: failed_execution) }
+      let(:sq_job) { instance_double(SolidQueue::Job, failed_execution: failed_execution) }
 
       before { allow(relation).to receive(:find_by).with(active_job_id: job_id).and_return(sq_job) }
 
diff --git a/spec/fixtures/tables/lms-materials-bracketed.html b/spec/fixtures/tables/lms-materials-bracketed.html
@@ -0,0 +1,22 @@
+<html>
+<head>
+  <meta content="text/html; charset=UTF-8" http-equiv="content-type">
+</head>
+<body>
+  <table>
+    <tbody>
+      <tr>
+        <td colspan="2"><span>[lms-materials]</span></td>
+      </tr>
+      <tr>
+        <td><span>mat-handout-1</span></td>
+        <td><span>individual-submission</span></td>
+      </tr>
+      <tr>
+        <td><span>mat-reference-1</span></td>
+        <td><span>view-only</span></td>
+      </tr>
+    </tbody>
+  </table>
+</body>
+</html>
diff --git a/spec/lib/doc_template/tables/lms_materials_spec.rb b/spec/lib/doc_template/tables/lms_materials_spec.rb
@@ -22,6 +22,18 @@
       end
     end
 
+    context "when the header is bracketed ([lms-materials]) per the lesson spec" do
+      let(:data) { file_fixture("tables/lms-materials-bracketed.html").read }
+
+      it "extracts entries and removes the table" do
+        expect(subject).to eq([
+          { "material-id" => "mat-handout-1", "access-type" => "individual-submission" },
+          { "material-id" => "mat-reference-1", "access-type" => "view-only" }
+        ])
+        expect(fragment.to_html).not_to include("lms-materials")
+      end
+    end
+
     context "when lms-materials table is absent" do
       let(:data) { "<html><body><p>No table here</p></body></html>" }
 
diff --git a/spec/lib/doc_template/tags/material_tag_spec.rb b/spec/lib/doc_template/tags/material_tag_spec.rb
@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+describe DocTemplate::Tags::MaterialTag do
+  let(:tag) { described_class.new }
+  let(:fragment) { Nokogiri::HTML.fragment(html) }
+  let(:node) { fragment.at_xpath(".//p") }
+
+  describe "#parse" do
+    let(:html) { %(<p>provide copies of <span>[material: #{identifier}]</span>.</p>) }
+    let(:identifier) { "10s.10.gg.l7.worksheet01" }
+
+    subject(:parsed) { tag.parse(node, value: identifier) }
+
+    context "when the material exists" do
+      let!(:material) { create(:material, identifier: identifier) }
+
+      it "renders an anchor pointing to the material" do
+        expect(parsed.content).to include(%(href="/materials/#{material.id}"))
+        expect(parsed.content).to include(identifier)
+      end
+
+      it "leaves no errors" do
+        expect(parsed.errors).to be_empty
+      end
+    end
+
+    context "when the material does not exist" do
+      it "renders a red badge with the identifier" do
+        expect(parsed.content).to include("badge text-bg-danger")
+        expect(parsed.content).to include("Unknown material: #{identifier}")
+      end
+
+      it "does not report an error" do
+        expect(parsed.errors).to be_empty
+      end
+    end
+  end
+end
diff --git a/spec/lib/lt/lcms/metadata/context_spec.rb b/spec/lib/lt/lcms/metadata/context_spec.rb

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-<% set_page_title @document.title.html_safe %>`
	`1`	`+<% set_page_title @document.lesson_title.html_safe %>`
`2`	`2`
`3`	`3`	`<style >`
`4`	`4`	`<%= raw(@document.css_styles) %>`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-<p class="title u-txt--title"><span><%= document.title %></span></p>`
	`1`	`+<p class="title u-txt--title"><span><%= document.lesson_title %></span></p>`
`2`	`2`	`<% if document.teaser.present? %>`
`3`	`3`	`<p class="subtitle u-txt--header-teaser">`
`4`	`4`	`<%= raw document.teaser %>`