Merge pull request #89 from alphagov/options-as-hashes

Use hashes as options for govspeak extensions

lib/govspeak.rb

@@ -1,5 +1,6 @@
 require 'kramdown'
 require 'active_support/core_ext/hash'
+require 'active_support/core_ext/array'
 require 'govspeak/header_extractor'
 require 'govspeak/structured_header_extractor'
 require 'govspeak/html_validator'
@@ -10,6 +11,7 @@
 require 'kramdown/parser/kramdown_with_automatic_external_links'
 require 'htmlentities'
 require 'presenters/attachment_presenter'
+require 'presenters/contact_presenter'
 require 'presenters/h_card_presenter'
 require 'erb'
 
@@ -33,9 +35,9 @@ def initialize(source, options = {})
       options.deep_symbolize_keys!
       @source = source ? source.dup : ""
       @images = options.delete(:images) || []
-      @attachments = Array(options.delete(:attachments))
-      @links = Array(options.delete(:links))
-      @contacts = Array(options.delete(:contacts))
+      @attachments = Array.wrap(options.delete(:attachments))
+      @links = Array.wrap(options.delete(:links))
+      @contacts = Array.wrap(options.delete(:contacts))
       @locale = options.fetch(:locale, "en")
       @options = {input: PARSER_CLASS_NAME}.merge(options)
       @options[:entity_output] = :symbolic
@@ -188,19 +190,22 @@ def insert_strong_inside_p(body, parser=Govspeak::Document)
     end
 
     extension('attachment', /\[embed:attachments:([0-9a-f-]+)\]/) do |content_id, body|
-      attachment = attachments.detect { |a| a.content_id.match(content_id) }
+      attachment = attachments.detect { |a| a[:content_id].match(content_id) }
       next "" unless attachment
       attachment = AttachmentPresenter.new(attachment)
-      content = File.read('lib/govspeak/extension/attachment.html.erb')
+      content = File.read(__dir__ + '/templates/attachment.html.erb')
       ERB.new(content).result(binding)
     end
 
     extension('attachment inline', /\[embed:attachments:inline:([0-9a-f-]+)\]/) do |content_id, body|
-      attachment = attachments.detect { |a| a.content_id.match(content_id) }
+      attachment = attachments.detect { |a| a[:content_id].match(content_id) }
       next "" unless attachment
       attachment = AttachmentPresenter.new(attachment)
-      content = File.read('lib/govspeak/extension/inline_attachment.html.erb')
-      ERB.new(content).result(binding)
+      span_id = attachment.id ? %{ id="attachment_#{attachment.id}"} : ""
+      # new lines inside our title cause problems with govspeak rendering as this is expected to be on one line.
+      link = attachment.link(attachment.title.gsub("\n", " "), attachment.url)
+      attributes = attachment.attachment_attributes.empty? ? "" : " (#{attachment.attachment_attributes})"
+      %{<span#{span_id} class="attachment-inline">#{link}#{attributes}</span>}
     end
 
     def render_image(url, alt_text, caption = nil)
@@ -275,12 +280,12 @@ def self.devolved_options
     end
 
     extension('embed link', /\[embed:link:([0-9a-f-]+)\]/) do |content_id|
-      link = links.detect { |l| l.content_id.match(content_id) }
+      link = links.detect { |l| l[:content_id].match(content_id) }
       next "" unless link
-      if link.url
-        %Q{<a href="#{encode(link.url)}">#{encode(link.title)}</a>}
+      if link[:url]
+        %Q{<a href="#{encode(link[:url])}">#{link[:title]}</a>}
       else
-        encode(link.title)
+        link[:title]
       end
     end
 
@@ -290,9 +295,10 @@ def render_hcard_address(contact)
     private :render_hcard_address
 
     extension('Contact', /\[Contact:([0-9a-f-]+)\]/) do |content_id|
-      contact = contacts.detect { |c| c.content_id.match(content_id) }
+      contact = contacts.detect { |c| c[:content_id].match(content_id) }
       next "" unless contact
-      @renderer ||= ERB.new(File.read('lib/templates/contact.html.erb'))
+      contact = ContactPresenter.new(contact)
+      @renderer ||= ERB.new(File.read(__dir__ + '/templates/contact.html.erb'))
       @renderer.result(binding)
     end
   end

lib/presenters/attachment_presenter.rb

@@ -1,77 +1,80 @@
 require "action_view"
 require "money"
+require "htmlentities"
 
 class AttachmentPresenter
   attr_reader :attachment
   include ActionView::Helpers::TagHelper
   include ActionView::Helpers::NumberHelper
   include ActionView::Helpers::AssetTagHelper
+  include ActionView::Helpers::TextHelper
 
   def initialize(attachment)
     @attachment = attachment
   end
 
   def id
-    attachment.id
+    attachment[:id]
   end
 
   def order_url
-    attachment.order_url
+    attachment[:order_url]
   end
 
   def opendocument?
-    attachment.opendocument?
+    attachment[:opendocument?]
   end
 
   def url
-    attachment.url
+    attachment[:url]
   end
 
   def external?
-    attachment.external?
+    attachment[:external?]
   end
 
   def price
-    return unless attachment.price
-    Money.from_amount(attachment.price, 'GBP').format
+    return unless attachment[:price]
+    Money.from_amount(attachment[:price], 'GBP').format
   end
 
   def accessible?
-    attachment.accessible?
+    attachment[:accessible?]
   end
 
   def thumbnail_link
     return if hide_thumbnail?
     return if previewable?
-    link(attachment_thumbnail, url, "aria-hidden=true class=#{attachment_class}")
+    link(attachment_thumbnail, url, "aria-hidden" => "true", "class" => attachment_class)
   end
 
   def help_block_toggle_id
-    "attachment-#{attachment.id}-accessibility-request"
+    "attachment-#{id}-accessibility-request"
   end
 
   def section_class
-    attachment.external? ? "hosted-externally" : "embedded"
+    attachment[:external?] ? "hosted-externally" : "embedded"
   end
 
   def mail_to(email_address, name, options = {})
-    "<a href='mailto:#{email_address}?Subject=#{options[:subject]}&body=#{options[:body]}'>#{name}</a>"
+    query_string = options.slice(:subject, :body).map { |k, v| "#{urlencode(k)}=#{urlencode(v)}" }.join("&")
+    "<a href='mailto:#{encode(email_address)}?#{encode(query_string)}'>#{name}</a>"
   end
 
   def alternative_format_order_link
     attachment_info = []
-    attachment_info << "  Title: #{attachment.title}"
-    attachment_info << "  Original format: #{attachment.file_extension}"
-    attachment_info << "  ISBN: #{attachment.isbn}" if attachment.isbn.present?
-    attachment_info << "  Unique reference: #{attachment.unique_reference}" if attachment.unique_reference.present?
-    attachment_info << "  Command paper number: #{attachment.command_paper_number}" if attachment.command_paper_number.present?
-    if attachment.hoc_paper_number.present?
-      attachment_info << "  House of Commons paper number: #{attachment.hoc_paper_number}"
-      attachment_info << "  Parliamentary session: #{attachment.parliamentary_session}"
+    attachment_info << "  Title: #{title}"
+    attachment_info << "  Original format: #{file_extension}" if file_extension.present?
+    attachment_info << "  ISBN: #{attachment[:isbn]}" if attachment[:isbn].present?
+    attachment_info << "  Unique reference: #{attachment[:unique_reference]}" if attachment[:unique_reference].present?
+    attachment_info << "  Command paper number: #{attachment[:command_paper_number]}" if attachment[:command_paper_number].present?
+    if attachment[:hoc_paper_number].present?
+      attachment_info << "  House of Commons paper number: #{attachment[:hoc_paper_number]}"
+      attachment_info << "  Parliamentary session: #{attachment[:parliamentary_session]}"
     end
 
     options = {
-      subject: "Request for '#{attachment.title}' in an alternative format",
+      subject: "Request for '#{title}' in an alternative format",
       body: body_for_mail(attachment_info)
     }
 
@@ -80,7 +83,7 @@ def alternative_format_order_link
 
   def body_for_mail(attachment_info)
     <<-END
-    Details of document required:
+Details of document required:
 
 #{attachment_info.join("\n")}
 
@@ -95,66 +98,93 @@ def alternative_format_contact_email
     "[email protected]"
   end
 
+  # FIXME: usage of image_tag will cause these to render at /images/ which seems
+  # very host dependent. I assume this will need links to static urls.
   def attachment_thumbnail
-    if attachment.pdf?
-      image_tag(attachment.file.thumbnail.url)
-    elsif attachment.html?
+    if file_extension == "pdf" && attachment[:thumbnail_url]
+      image_tag(attachment[:thumbnail_url])
+    elsif file_extension == "html"
       image_tag('pub-cover-html.png')
-    elsif %w{doc docx odt}.include? attachment.file_extension
+    elsif %w{doc docx odt}.include?(file_extension)
       image_tag('pub-cover-doc.png')
-    elsif %w{xls xlsx ods csv}.include? attachment.file_extension
+    elsif %w{xls xlsx ods csv}.include?(file_extension)
       image_tag('pub-cover-spreadsheet.png')
     else
       image_tag('pub-cover.png')
     end
   end
 
-  def references
+  def reference
+    ref = []
+    if attachment[:isbn].present?
+      ref << "ISBN " + content_tag(:span, attachment[:isbn], class: "isbn")
+    end
+
+    if attachment[:unique_reference].present?
+      ref << content_tag(:span, attachment[:unique_reference], class: "unique_reference")
+    end
+
+    if attachment[:command_paper_number].present?
+      ref << content_tag(:span, attachment[:command_paper_number], class: "command_paper_number")
+    end
+
+    if attachment[:hoc_paper_number].present?
+      ref << content_tag(:span, "HC #{attachment[:hoc_paper_number]}", class: 'house_of_commons_paper_number') + ' ' +
+        content_tag(:span, attachment[:parliamentary_session], class: 'parliamentary_session')
+    end
+
+    ref.join(', ').html_safe
+  end
+
+  # FIXME this has english in it so will cause problems if the locale is not en
+  def references_for_title
     references = []
-    references << "ISBN: #{attachment.isbn}" if attachment.isbn.present?
-    references << "Unique reference: #{attachment.unique_reference}" if attachment.unique_reference.present?
-    references << "Command paper number: #{attachment.command_paper_number}" if attachment.command_paper_number.present?
-    references << "HC: #{attachment.hoc_paper_number} #{attachment.parliamentary_session}" if attachment.hoc_paper_number.present?
+    references << "ISBN: #{attachment[:isbn]}" if attachment[:isbn].present?
+    references << "Unique reference: #{attachment[:unique_reference]}" if attachment[:unique_reference].present?
+    references << "Command paper number: #{attachment[:command_paper_number]}" if attachment[:command_paper_number].present?
+    references << "HC: #{attachment[:hoc_paper_number]} #{attachment[:parliamentary_session]}" if attachment[:hoc_paper_number].present?
     prefix = references.size == 1 ? "and its reference" : "and its references"
     references.any? ? ", #{prefix} (" + references.join(", ") + ")" : ""
   end
 
   def references?
-    !attachment.isbn.to_s.empty? || !attachment.unique_reference.to_s.empty? || !attachment.command_paper_number.to_s.empty? || !attachment.hoc_paper_number.to_s.empty?
+    !attachment[:isbn].to_s.empty? || !attachment[:unique_reference].to_s.empty? || !attachment[:command_paper_number].to_s.empty? || !attachment[:hoc_paper_number].to_s.empty?
   end
 
   def attachment_class
-    attachment.external? ? "hosted-externally" : "embedded"
+    attachment[:external?] ? "hosted-externally" : "embedded"
   end
 
   def unnumbered_paper?
-    attachment.unnumbered_command_paper? || attachment.unnumbered_hoc_paper?
+    attachment[:unnumbered_command_paper?] || attachment[:unnumbered_hoc_paper?]
   end
 
   def unnumbered_command_paper?
-    attachment.unnumbered_command_paper?
+    attachment[:unnumbered_command_paper?]
   end
 
   def download_link
-    link(attachment.preview_url, "<strong>Download #{attachment.file_extension.upcase}</strong>", number_to_human_size(attachment.file_size))
+    options = {}
+    options[:title] = number_to_human_size(attachment[:file_size]) if attachment[:file_size].present?
+    link("<strong>Download #{file_extension.upcase}</strong>", attachment[:url], options)
   end
 
   def attachment_attributes
     attributes = []
-    if attachment.html?
+    if file_extension == "html"
       attributes << content_tag(:span, 'HTML', class: 'type')
-    elsif attachment.external?
+    elsif attachment[:external?]
       attributes << content_tag(:span, url, class: 'url')
     else
-      attributes << content_tag(:span, humanized_content_type(attachment.file_extension), class: 'type')
-      attributes << content_tag(:span, number_to_human_size(attachment.file_size), class: 'file-size')
-      attributes << content_tag(:span, pluralize(attachment.number_of_pages, "page") , class: 'page-length') if attachment.number_of_pages.present?
+      attributes << content_tag(:span, humanized_content_type(file_extension), class: 'type') if file_extension
+      attributes << content_tag(:span, number_to_human_size(attachment[:file_size]), class: 'file-size') if attachment[:file_size]
+      attributes << content_tag(:span, pluralize(attachment[:number_of_pages], "page"), class: 'page-length') if attachment[:number_of_pages]
     end
     attributes.join(', ').html_safe
   end
 
   def preview_url
-    url << '/preview'
+    url + '/preview'
   end
 
   MS_WORD_DOCUMENT_HUMANIZED_CONTENT_TYPE = "MS Word Document"
@@ -205,37 +235,52 @@ def humanized_content_type(file_extension)
   end
 
   def previewable?
-    attachment.csv?
+    file_extension == "csv"
   end
 
   def title
-    attachment.title
+    attachment[:title]
+  end
+
+  def file_extension
+    # Note: this is a separate parameter rather than being calculated from the
+    # filename because at the time of writing not all apps were using the effects
+    # of this field.
+    attachment[:file_extension]
   end
 
   def hide_thumbnail?
     defined?(hide_thumbnail) && hide_thumbnail
   end
 
-  def attachement_details
+  def attachment_details
     return if previewable?
-    link(attachment.title, url, title_link_options)
+    link(title, url, title_link_options)
   end
 
   def title_link_options
-    title_link_options = ''
-    title_link_options << "rel=external" if attachment.external?
-    title_link_options << "aria-describedby=#{help_block_id}" unless attachment.accessible?
+    title_link_options = {}
+    title_link_options["rel"] = "external" if attachment[:external?]
+    title_link_options["aria-describedby"] = help_block_id unless attachment[:accessible?]
+    title_link_options
   end
 
   def help_block_id
-    "attachment-#{attachment.id}-accessibility-help"
+    "attachment-#{id}-accessibility-help"
   end
 
-  def link(body, url, options={})
-    <<-END
-      <a href="#{url} #{options}">
-        #{body}
-      </a>
-    END
+  def link(body, url, options = {})
+    options_str = options.map { |k, v| %{#{encode(k)}="#{encode(v)}"} }.join(" ")
+    %{<a href="#{encode(url)}" #{options_str}>#{body}</a>}
+  end
+
+private
+
+  def encode(text)
+    HTMLEntities.new.encode(text)
+  end
+
+  def urlencode(text)
+    ERB::Util.url_encode(text)
   end
 end