From c9e3bf4eacc94c5bfffa47b30afffd38c00c9e6f Mon Sep 17 00:00:00 2001 From: Matt Barnett Date: Tue, 30 Aug 2016 10:59:22 -0600 Subject: [PATCH] Refactoring to better accomodate objects without files --- app/helpers/generic_file_helper.rb | 49 ++++++++++++------- .../hydranorth/solr_document_behavior.rb | 6 +-- .../_show_document_list_menu.html.erb | 9 ++-- .../generic_files/_generic_file.html.erb | 4 +- .../generic_files/_show_actions.html.erb | 2 +- app/views/my/_action_menu.html.erb | 9 ++-- spec/features/collection/collection_spec.rb | 1 - 7 files changed, 46 insertions(+), 34 deletions(-) diff --git a/app/helpers/generic_file_helper.rb b/app/helpers/generic_file_helper.rb index 9d3c922e..cc2a0c5f 100644 --- a/app/helpers/generic_file_helper.rb +++ b/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? diff --git a/app/models/concerns/hydranorth/solr_document_behavior.rb b/app/models/concerns/hydranorth/solr_document_behavior.rb index ac6a3505..12cfe845 100644 --- a/app/models/concerns/hydranorth/solr_document_behavior.rb +++ b/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 diff --git a/app/views/collections/_show_document_list_menu.html.erb b/app/views/collections/_show_document_list_menu.html.erb index 77e9c02f..f0ad89dd 100755 --- a/app/views/collections/_show_document_list_menu.html.erb +++ b/app/views/collections/_show_document_list_menu.html.erb @@ -17,10 +17,11 @@ <%= link_to raw(' Single-Use Link to File'), '#', class: "copypaste itemicon itemcode", title: "Single-Use Link to File", id: "copy_link_#{id}" %> -
  • - <%= link_to raw(' Download File'), sufia.download_path(id), - class: 'itemicon itemdownload', title: 'Download File', target: '_new' %> -
    • + <% render_download_link(gf, raw(' Download File')) do |download_link| %> +
  • + <%= download_link %> +
    • + <% end %> <% if current_user %>
  • <%= display_trophy_link(current_user, id) do |text| %> diff --git a/app/views/generic_files/_generic_file.html.erb b/app/views/generic_files/_generic_file.html.erb index b6f6d100..47bedee7 100644 --- a/app/views/generic_files/_generic_file.html.erb +++ b/app/views/generic_files/_generic_file.html.erb @@ -1,7 +1,9 @@

  • <%= link_to generic_file.title_or_label, sufia.generic_file_path(generic_file) %> - [<%= link_to 'Download', download_path(generic_file), target: '_blank', rel: 'noopener noreferrer' %>] + <% render_download_link(generic_file) do |download_link| %> + [<%= download_link %>] + <% end %>

    diff --git a/app/views/generic_files/_show_actions.html.erb b/app/views/generic_files/_show_actions.html.erb index b61ba353..ed8f1756 100644 --- a/app/views/generic_files/_show_actions.html.erb +++ b/app/views/generic_files/_show_actions.html.erb @@ -1,7 +1,7 @@

    Actions

    - <%= render_download_link %> + <%= render_download_link(@generic_file) %> <% if Sufia.config.analytics %>  |  <%= link_to "Analytics", sufia.stats_generic_file_path(@generic_file), id: 'stats' %> diff --git a/app/views/my/_action_menu.html.erb b/app/views/my/_action_menu.html.erb index fb0c974a..26550558 100755 --- a/app/views/my/_action_menu.html.erb +++ b/app/views/my/_action_menu.html.erb @@ -15,10 +15,11 @@ class: 'itemicon itemedit', title: 'Edit File' %>

    • <% end %> -
  • - <%= link_to raw(' Download File'), download_path(gf), - class: 'itemicon itemdownload', title: 'Download File', target: '_new' %> -
    • + <% render_download_link(gf, raw(' Download File')) do |download_link| %> +
  • + <%= download_link %> +
    • + <% end %>
  • <% if current_user.admin? %> <%= link_to raw(' Delete File'), sufia.generic_file_path(id), diff --git a/spec/features/collection/collection_spec.rb b/spec/features/collection/collection_spec.rb index 6d3261b6..8a926b0c 100755 --- a/spec/features/collection/collection_spec.rb +++ b/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")