Refactoring to better accomodate objects without files

app/helpers/generic_file_helper.rb

@@ -20,10 +20,17 @@ def render_download_icon(title = nil)
     end
   end
 
-  def render_download_link(text = nil)
-    if @generic_file.label.present? || !@generic_file.filename.empty?
-      link_to (text || "Download"), download_path(@generic_file), { id: "file_download", target: "_new", data: { label: @generic_file.id } }
-    end
+  # if passed a block, this method conditionally renders the markup in the block only if the path is present. This
+  # provides an easy way of sanely dealing with some items having files (and thus download paths) and others not.
+  def render_download_link(item, text = nil)
+    # in Communities, Collections appear in lists alongside files, and so they are rendered through the same partial
+    # but collections have no download link
+    path = download_path(item)
+    return '' unless path.present?
+
+    download_link = link_to (text || 'Download'), path, { id: 'file_download', target: '_blank', rel: 'noopener noreferrer', data: { label: item.id } }
+    yield download_link if block_given?
+    return download_link
   end
 
   # sufia.download path is from Sufia::Engine.routes.url_helpers
@@ -34,26 +41,28 @@ def render_download_link(text = nil)
   # download_path(id: @asset)
   # download_path document, file: 'thumbnail'
   #
-  # we shouldn't be overloading this to accept everything under the sun as it leads to all
-  # manner of bugs and corner cases, but these usages are imposed by Hydra/Sufia, so we're forced to live with
-  # it
+  # we shouldn't be overloading this to accept everything under the sun (GenericFiles, SolrDocs, Strings,
+  # Collections...) as it has lead to all manner of bugs and corner cases, but these usages are imposed by Hydra/Sufia,
+  # so we're forced to live with it.
+  #
+  # The precondition here is that if you're calling this, you're only going to get a sensible result on objects that
+  # actually have files. Not every object does, and there are various valid cases in which an object won't have one
+  # (Weiwei mentions that both dataverse objects and migrated items from Thesis deposit may not).
+  #
+  # Nil is returned for objects without files, to push decisions on how to sanely handle objects with no files up
+  # to the calling context. You probably want to use render_download_link or a different client function to generate
+  # the link for you -- it can cleanly not render the surrounding markup at all if this returns nil.
   def download_path(*args)
     raise ArgumentError unless args.present?
+
     item = item_for_download(args.shift)
+    return nil unless item.present? && (item.label.present? || item.filename.present? || item.doi_url.present?)
 
     # doi supercedes anything else
     return item.doi_url if item.doi_url.present?
 
-    # this shouldn't happen normally in production, because it indicates an item with no file
-    # but a BUNCH of our tests do create items like this
-    unless item.label.present? || item.filename.present?
-      logger.error "item with no file found! id: #{item.id}"
-      # this would be a good place for hoptoad/airbrake/newrelic-style alerting
-      return "/files/#{item.id}/"
-    end
-
     # otherwise path is /files/noid/(label or filename)
-    path = "/files/#{item.id}/" + (item.label.nil? ? URI::encode(item.filename) : URI::encode(item.label))
+    path = "/files/#{item.id}/" + (item.label.nil? ? URI::encode(item.filename.first) : URI::encode(item.label))
 
     # appeand query args to download path, eg. /files/noid/label?file=thumbnail or file=mp3, etc
     unless args.empty?
@@ -83,11 +92,13 @@ def display_multiple(value)
 
   private
 
-  # this could be a string (item ID), a SolrDocument corresponding to the cache of a GenericFile, or a fully reified
-  # GenericFile itself (all of these are usages of download_path imposed on us by Sufia, unfortunately, so this can't
-  # be easily rationalized). Ideally, SolrDocuments are preferable to GenericFiles for performance reasons, so we try
+  # this could be a string (item ID), a SolrDocument corresponding to the cache of a GenericFile, a fully reified
+  # GenericFile itself, or a Collection which isn't downloadable but which is rendered through the same partials. (
+  # All of these are usages of download_path imposed on us by Sufia, unfortunately, so this can't
+  # be easily rationalized. Ideally, SolrDocuments are preferable to GenericFiles for performance reasons, so we try
   # to minimize reification of IDs whenever possible.
   def item_for_download(candidate)
+    return nil if candidate.is_a?(Collection)
     return candidate if candidate.is_a?(SolrDocument) || candidate.is_a?(GenericFile)
 
     # TODO could we fish this out of Solr and send back a SolrDocument instead?

app/models/concerns/hydranorth/solr_document_behavior.rb

@@ -25,7 +25,7 @@ def dissertant
     end
 
     def filename
-      return Array(self[Solrizer.solr_name('filename')]).first if self.has_key?(Solrizer.solr_name('filename'))
+      return Array(self[Solrizer.solr_name('filename')]) if self.has_key?(Solrizer.solr_name('filename'))
 
       # we can try to fish it out of the object profile, but this is kind of a hack
       # if it doesn't work, we just return a link to the show page. This should be less
@@ -34,9 +34,7 @@ def filename
         # cache this, as it can get called repeatedly on the same object and
         # parsing JSON is relatively expensive (though cheaper than a trip to Fedora)
         @json ||= JSON.parse(self['object_profile_ssm'].first)
-        if @json.has_key?('filename') && @json['filename'].present?
-          return @json['filename'].first
-        end
+        return @json['filename'] if @json.has_key?('filename') && @json['filename'].present?
       rescue
         # this would be a good place for hoptoad/airbrake/newrelic-style alerting
         # this should indicate either a corrupted object cache in Solr or an object

app/views/collections/_show_document_list_menu.html.erb

@@ -17,10 +17,11 @@
             <%= link_to raw('<i class="glyphicon glyphicon-link over"></i> Single-Use Link to File'), '#',
                 class: "copypaste itemicon itemcode", title: "Single-Use Link to File", id: "copy_link_#{id}" %>
         </li>
-        <li>
-            <%= link_to raw('<i class="glyphicon glyphicon-download-alt"></i> Download File'), sufia.download_path(id),
-                class: 'itemicon itemdownload', title: 'Download File', target: '_new' %>
-        </li>
+        <% render_download_link(gf, raw('<i class="glyphicon glyphicon-download-alt"></i> Download File')) do |download_link| %>
+          <li>
+            <%= download_link %>
+          </li>
+        <% end %>
         <% if current_user %>
             <li>
                 <%= display_trophy_link(current_user, id) do |text| %>

app/views/generic_files/_generic_file.html.erb

@@ -1,7 +1,9 @@
 <li>
   <h2>
     <%= link_to generic_file.title_or_label, sufia.generic_file_path(generic_file) %>
-    <small>[<%= link_to 'Download', download_path(generic_file), target: '_blank', rel: 'noopener noreferrer' %>]</small>
+    <% render_download_link(generic_file) do |download_link| %>
+      <small>[<%= download_link %>]</small>
+    <% end %>
   </h2>
   <div class="row">
     <div class="col-sm-3">

app/views/generic_files/_show_actions.html.erb

@@ -1,7 +1,7 @@
 <div id="show_actions">
 <h2 class="non lower">Actions</h2>
     <p>
-      <%= render_download_link %>
+      <%= render_download_link(@generic_file) %>
       <% if Sufia.config.analytics %>
         &nbsp;|&nbsp;
         <%= link_to "Analytics", sufia.stats_generic_file_path(@generic_file), id: 'stats' %>

app/views/my/_action_menu.html.erb

@@ -15,10 +15,11 @@
       class: 'itemicon itemedit', title: 'Edit File' %>
     </li>
     <% end %>
-    <li role="menuitem" tabindex="-1">
-    <%= link_to raw('<i class="glyphicon glyphicon-download-alt" aria-hidden="true"></i> Download File'), download_path(gf),
-      class: 'itemicon itemdownload', title: 'Download File', target: '_new' %>
-    </li>
+    <% render_download_link(gf, raw('<i class="glyphicon glyphicon-download-alt" aria-hidden="true"></i> Download File')) do |download_link| %>
+      <li role="menuitem" tabindex="-1">
+        <%= download_link %>
+      </li>
+    <% end %>
     <li role="menuitem" tabindex="-1">
     <% if current_user.admin? %>
       <%= link_to raw('<i class="glyphicon glyphicon-trash" aria-hidden="true"></i> Delete File'), sufia.generic_file_path(id),

spec/features/collection/collection_spec.rb

@@ -364,7 +364,6 @@
       expect(page).to have_content(generic_file.title.first)
 
       expect(page).to have_content("Test Item")
-      expect(page).to have_content("Download")
 
       click_link ('Test Item')
       expect(page).not_to have_content("Edit")