Merge branch 'remove_ransack_from_collections' into remove_ransack

app/components/download_dropdown_component.rb

@@ -10,12 +10,6 @@
 #     # Don't know if it's a work or an asset?
 #     DownloadDropdownComponent.new(member.leaf_representative, display_parent_work: work)
 #
-# This class uses other "DownloadOptions" classes to actually figure out the
-# appropriate options for a given asset of given type and state, if you need
-# that info directly to present in some other way, see those classes in
-# app/presenters/download_options. (Right now the logic for picking _which_ one of
-# those to use for a given asset is in here, but could be extracted out.)
-#
 # `display_parent_work` is used for determining any "whole-work" download options (zip or pdf
 # of images), and will have it's `members` and their leaf_representatives accessed,
 # so should have them pre-loaded to avoid n+1 queries if needed.
@@ -41,29 +35,40 @@
 #
 # (This is a bit hacky)
 #
+# ## Whole-work options
+#
+# Are passed in as an array of DownloadOptions.  These can be complicated and expensive to calculate,
+# so if you are displaying many DownloadDropdownComponent for same work, best to cache them
+# and re-use them. WorkDownloadOptionsCreator class can calculate them.
+#
 class DownloadDropdownComponent < ApplicationComponent
-  attr_reader :display_parent_work, :asset, :aria_label, :btn_class_name, :include_whole_work_options
+  attr_reader :display_parent_work, :asset, :aria_label, :btn_class_name, :work_download_options
 
 
   # @param asset [Asset] asset to display download links for
   # @param display_parent_work [Work] the Work we are in the context of displaying, used
-  #   to determine whole-work download links (zip or pdf of all images), will have it's
-  #   `members` and their `leaf_representative`s accessed so should be pre-loaded if needed for performance.
+  #   for rights statements, counts of children in headings, and other things.
   #
   #   We don't just get from asset.parent, because intervening child work hieararchy
   #   may make it complicated, we need to be told our display parent context.
   #
-  #   display_parent_work is also used for determining "rights" statement.
   # @param use_link [Boolean], default false, if true will make the link that opens the menu an
   #   ordinary hyperlink, instead of a button (used on audio playlist).
+  #
   # @param aria_label [String] aria_label for the primary "Download" button, we
   #   use to make it more specific like "Download Image 1" to avoid tons of
   #   identical "Download" buttons on page.
+  #
   # @param btn_class_name [String] default "btn-brand-alt", but maybe you want "btn-brand-main".
   #   The specific btn theme added on to bootstrap `btn` that will be there anyway.
+  #
+  # @param work_download_options [Array<DownloadOption>] should we include whole-work download options?
+  #   As they are somewhat expensive to calculate and context-dependent, caller should pass in
+  #   already calculated if desired! And up to caller to make sure they are computed and cached
+  #   efficiently and not calculated many times over and over again for same work.
   def initialize(asset,
       display_parent_work:,
-      include_whole_work_options: false,
+      work_download_options: nil,
       use_link: false,
       viewer_template: false,
       aria_label: nil,
@@ -80,7 +85,7 @@ def initialize(asset,
     @asset = asset
     @aria_label = aria_label
     @btn_class_name = btn_class_name
-    @include_whole_work_options = include_whole_work_options
+    @work_download_options = work_download_options || [] # empty array not nil or false
   end
 
   def call
@@ -122,7 +127,10 @@ def asset_download_options
       # want it in individual image download options, as per
       # https://github.com/sciencehistory/scihist_digicoll/issues/2278
       #
-      DownloadOptions::ImageDownloadOptions.new(asset, show_pdf_link: (!has_work_download_options? && display_parent_work&.published? && asset.content_type.start_with?("image/"))).options
+      # If we only have ONE member, we just merge any whole work options in
+      # to accomodate this and other edge cases
+      #
+      ( display_parent_work&.member_count == 1 ? work_download_options : []) + DownloadOptions::ImageDownloadOptions.new(asset).options
    end
   end
 
@@ -198,9 +206,9 @@ def menu_items
       elements << content_tag("div", nil,  class:'dropdown-divider')
     end
 
-    if include_whole_work_options && has_work_download_options?
+    if has_work_download_options?
       elements << content_tag("h3", "Download all #{display_parent_work.member_count} images", class:'dropdown-header')
-      whole_work_download_options.each do |download_option|
+      work_download_options.each do |download_option|
         elements << format_download_option(download_option)
       end
       elements << content_tag("div", nil,  class:'dropdown-divider')
@@ -239,41 +247,9 @@ def rights_statement_item
     render(RightsIconComponent.new(mode: :dropdown_item, rights_id: display_parent_work&.rights, work: display_parent_work))
   end
 
-  # have a PUBLISHED parent work, with more than 1 child, and AT LEAST ONE of it's children are images,
-  # provide multi-image downloads. These are the only whole-work-download options we provide at present.
-  #
-  # (Our current PDF and Zip creators only create for published items, they can't create
-  # for non-published items. But we don't have an easy way to efficiently access
-  # number of published child items, we just use the parent being unpublished as a proxy
-  # for "not ready")
-  #
-  # NOTE: We had been checking to make sure ALL members were images, but that was FAR
-  # too resource intensive, it destroyed ramelli. Checking just one is okay though.
   def has_work_download_options?
-    return @has_work_download_options if defined?(@has_work_download_options)
-
-    @has_work_download_options = self.class.work_has_multiple_published_images?(display_parent_work)
-  end
-
-  # Extracted for re-use in other places, see #has_work_download_options?
-  def self.work_has_multiple_published_images?(work)
-    work &&
-    work.published? &&
-    work.member_count > 1 &&
-    work.member_content_types(mode: :query).all? {|t| t.start_with?("image/")}
-  end
-
-  def whole_work_download_options
-    return [] unless has_work_download_options?
-
-    [
-      DownloadOption.for_on_demand_derivative(
-        label: "PDF", derivative_type: "pdf_file", work_friendlier_id: @asset&.parent&.friendlier_id
-      ),
-      DownloadOption.for_on_demand_derivative(
-        label: "ZIP", derivative_type: "zip_file", work_friendlier_id: @asset&.parent&.friendlier_id
-      )
-    ]
+    # if the member count is only 1, we don't display a whole-work section, we just merge
+    # any whole-work options into the current asset section
+    display_parent_work&.member_count.to_i > 1 && work_download_options.present?
   end
-
 end

app/components/member_image_component.rb

@@ -33,7 +33,7 @@
 # we don't want to show people the placeholder, this is just a fail-safe to avoid
 # showing non-public content in case of other errors.
 class MemberImageComponent < ApplicationComponent
-  attr_reader :size, :lazy, :member, :image_label
+  attr_reader :size, :lazy, :member, :image_label, :work_download_options
 
   delegate :can?, to: :helpers
 
@@ -57,12 +57,16 @@ def self.viewer_trigger_data_attrs(member_id:, work_id:)
   #    Will be used to construct labels like "View Image 10".  Will default to `alt_text` set on
   #    Asset, if present. Not actually necessarily suitable "alt" text, instead
   #    it's used to construct action labels like that!
-  def initialize(member, size: :standard, lazy: false, image_label: nil)
+  #
+  #  @param work_download_options [Array<DownloadOption>] sometimes we want to show them, sometimes we
+  #     don't, and they are expensive, so pass them in if you want them.
+  def initialize(member, size: :standard, lazy: false, image_label: nil, work_download_options: nil)
     @lazy = !!lazy
     @size = size
     @member = member
 
     @image_label = image_label
+    @work_download_options = work_download_options
   end
 
   def call
@@ -166,7 +170,7 @@ def btn_class_name
   def download_button
     # We only include whole-work download menu section for large size, used as hero.
     render DownloadDropdownComponent.new(representative_asset,
-                                          include_whole_work_options: (size == :large),
+                                          work_download_options: work_download_options,
                                           display_parent_work: member.parent,
                                           aria_label: ("Download #{image_label}" if image_label),
                                           btn_class_name: btn_class_name)

app/components/work_download_links_component.html.erb

@@ -1,58 +1,27 @@
 <div class="on-page-work-downloads">
-
-  <% if has_any_pdf? %>
+  <% download_options.each do |download_option| %>
     <div class="on-page-work-downloads-item">
       <div class="image">
-        <%= link_to(pdf_download_option.url, data: pdf_download_option.data_attrs, tabindex: -1, "aria-label": "Download PDF") do %>
-          <%= helpers.file_earmark_pdf_fill_svg %>
+        <%= link_to(download_option.url, data: download_option.data_attrs, tabindex: -1, "aria-label": download_button_label(download_option)) do %>
+          <%= download_file_icon(download_option) %>
         <% end %>
       </div>
 
       <div class="text">
-          <%= link_to(pdf_download_option.url, data: pdf_download_option.data_attrs, tabindex: -1) do %>
-            <% if has_searchable_pdf? %>
-              Searchable PDF
-            <% else %>
-              PDF
-            <% end %>
+          <%= link_to(download_option.url, data: download_option.data_attrs, tabindex: -1) do %>
+            <%= download_option.label %>
           <% end %>
 
-          <% if has_searchable_pdf? %>
+          <% if download_option.subhead.present? %>
             <div class="hint">
-              <small>may contain errors</small>
+              <small><%= download_option.subhead %></small>
             </div>
           <% end %>
       </div>
 
       <div class="action">
-        <%= link_to(pdf_download_option.url, data: pdf_download_option.data_attrs, class: "btn btn-brand-main less-padding") do %>
-          Download PDF
-        <% end %>
-      </div>
-    </div>
-  <% end %>
-
-  <% if has_downloadable_zip? %>
-    <div class="on-page-work-downloads-item">
-      <div class="image">
-        <%= link_to(zip_download_option.url, data: zip_download_option.data_attrs, tabindex: -1, "aria-label": "Download ZIP") do %>
-          <%= helpers.file_earmark_zip_fill_svg %>
-        <% end %>
-      </div>
-
-      <div class="text">
-          <%= link_to(zip_download_option.url, data: zip_download_option.data_attrs, tabindex: -1) do %>
-            ZIP
-          <% end %>
-
-          <div>
-            <small>of full-sized JPGs</small>
-          </div>
-      </div>
-
-      <div class="action" style="border-top: none; padding-right: 0;">
-        <%= link_to(zip_download_option.url, data: zip_download_option.data_attrs, class: "btn btn-brand-main less-padding") do %>
-          Download ZIP
+        <%= link_to(download_option.url, data: download_option.data_attrs, class: "btn btn-brand-main less-padding") do %>
+          <%= download_button_label(download_option) %>
         <% end %>
       </div>
     </div>

app/components/work_download_links_component.rb

@@ -1,11 +1,27 @@
 # frozen_string_literal: true
 
 # ON-PAGE download links for Work page, for whole-work downloads
+#
+# We use a list of DownloadOption elements, but format them on page -- and change name/subhead
+# from usual in some cases.
 class WorkDownloadLinksComponent < ApplicationComponent
-  attr_reader :work
+  attr_reader :work, :download_options
 
-  def initialize(work)
+  def initialize(work, download_options:)
     @work = work
+
+    @download_options = download_options
+
+    if has_searchable_pdf?
+      # override some values in options
+      @download_options.collect! do |option|
+        if option.analyticsAction.to_s == "download_pdf"
+          option.dup_with("Searchable PDF", subhead: "may contain errors")
+        else
+          option
+        end
+      end
+    end
   end
 
   def has_searchable_pdf?
@@ -18,35 +34,20 @@ def has_searchable_pdf?
     @has_searchable_pdf = work.ocr_requested? # && !WorkShowOcrComponent.new(work).asset_ocr_count_warning?
   end
 
-  # has some PDF as long as we have at least ONE published image
-  def has_any_pdf?
-    return @has_any_pdf if defined?(@has_any_pdf)
-
-    @has_any_pdf = work &&
-      work.published? &&
-      work.member_count > 0 &&
-      work.member_content_types(mode: :query).any? {|t| t.start_with?("image/")}
-  end
-
-  def has_downloadable_zip?
-    return @has_downloadable_zip if defined?(@has_downloadable_zip)
-
-    # We use DownloadDropdownComponent to try to decide if it's going to have a ZIP link or not
-    # using same logic it will...
-    @has_downloadable_zip = DownloadDropdownComponent.work_has_multiple_published_images?(work)
-  end
 
-  def pdf_download_option
-    # We don't actually use label, but want to get attributes out
-    @pdf_download_option ||= DownloadOption.for_on_demand_derivative(
-        label: "", derivative_type: "pdf_file", work_friendlier_id: work.friendlier_id
-      )
+  def download_button_label(download_option)
+    case download_option.data_attrs[:derivative_type].to_s
+    when "pdf_file" ; "Download PDF"
+    when "zip_file" ; "Download ZIP"
+    else "Download"
+    end
   end
 
-  def zip_download_option
-    # We don't actually use label, but want to get attributes out
-    @zip_download_option ||= DownloadOption.for_on_demand_derivative(
-      label: "", derivative_type: "zip_file", work_friendlier_id: work.friendlier_id
-    )
+  def download_file_icon(download_option)
+    case download_option.data_attrs[:derivative_type].to_s
+    when "pdf_file" ; helpers.file_earmark_pdf_fill_svg
+    when "zip_file" ; helpers.file_earmark_zip_fill_svg
+    else "x"
+    end
   end
 end

app/components/work_image_show_component.html.erb

@@ -11,10 +11,11 @@
   </div>
 
   <div class="show-hero">
-    <%= render MemberImageComponent.new(representative_member, size: :large) %>
+    <%# note the large hero one gets work_download_options %>
+    <%= render MemberImageComponent.new(representative_member, size: :large, work_download_options: work_download_options) %>
     <%= render "works/rights_and_social", work: work %>
 
-    <%= render WorkDownloadLinksComponent.new(work) %>
+    <%= render WorkDownloadLinksComponent.new(work, download_options: work_download_options) %>
   </div>
 
   <div class="show-metadata">
@@ -80,4 +81,4 @@
 </div>
 
 <%# hidden modal used by viewer %>
-<%= render 'scihist_image_viewer/scihist_viewer_modal', work: work  %>
+<%= render 'scihist_image_viewer/scihist_viewer_modal', work: work, work_download_options: work_download_options  %>

app/components/work_image_show_component.rb

@@ -6,10 +6,14 @@
 class WorkImageShowComponent < ApplicationComponent
   delegate :construct_page_title, :current_user, to: :helpers
 
-  attr_reader :work
+  attr_reader :work, :work_download_options
 
   def initialize(work)
     @work = work
+
+    # work download options are expensive, so we calculate them here so we can use them
+    # in several places
+    @work_download_options = WorkDownloadOptionsCreator.new(work).options
   end
 
   # Public members, ordered, to be displayed as thumbnails

app/models/download_option.rb

@@ -8,7 +8,7 @@ class DownloadOption
 
   # Create a DownloadOption for one of our on-demand derivatives, DRY it up here
   # so we can re-use equivalently.
-  def self.for_on_demand_derivative(label:, derivative_type:, work_friendlier_id:)
+  def self.for_on_demand_derivative(label:, derivative_type:, work_friendlier_id:, subhead:nil)
     derivative_type = derivative_type.to_s
 
     unless derivative_type.in?(["pdf_file", "zip_file"])
@@ -20,7 +20,8 @@ def self.for_on_demand_derivative(label:, derivative_type:, work_friendlier_id:)
       "zip_file" => "download_zip"
     }[derivative_type]
 
-    subhead = {
+    # defaults\
+    subhead ||= {
      "pdf_file" => nil,
       "zip_file" => "of full-sized JPGs"
     }[derivative_type]
@@ -103,4 +104,10 @@ def as_json(options={})
       analyticsAction: analyticsAction
     }
   end
+
+  # sometimes we want to modify one, treat as immutable but let modify like this!
+  # Can override any element as if it were initializer
+  def dup_with(label=self.label, url: self.url, work_friendlier_id: self.work_friendlier_id, subhead:self.subhead, analyticsAction:self.analyticsAction, data_attrs: self.data_attrs)
+    self.class.new(label, url: url, work_friendlier_id: work_friendlier_id, subhead:subhead, analyticsAction:analyticsAction, data_attrs: data_attrs)
+  end
 end