Merge branch 'tag-1.5'

Conflicts:
	app/helpers/generic_file_helper.rb
	app/views/collections/_show_document_list_menu.html.erb
	app/views/my/_action_menu.html.erb

app/controllers/catalog_controller.rb

@@ -29,7 +29,6 @@ def self.date_created_field
   end
 
   def set_solr_search_fields
-
     blacklight_config.configure do |config|
 
       if !current_user.nil?
@@ -69,8 +68,6 @@ def set_solr_search_fields
           end
         end
       end
-
-
     end
   end
 
@@ -150,9 +147,8 @@ def set_solr_search_fields
     config.add_show_field solr_name("fedora3uuid", :stored_searchable), label: "UUID"
 
     config.add_sort_field "score desc, #{uploaded_field} desc", label: "Relevance \u25BC"
-    # TODO: uncomment once a re-index is complete and sortable_title is in Solr
-    #config.add_sort_field "sortable_title_ssi asc", label: "Title A-Z"
-    #config.add_sort_field "sortable_title_ssi desc", label: "Title Z-A"
+    config.add_sort_field "sortable_title_ssi asc", label: "Title A-Z"
+    config.add_sort_field "sortable_title_ssi desc", label: "Title Z-A"
     config.add_sort_field "#{date_created_field} desc", label: "Date (newest first)"
     config.add_sort_field "#{date_created_field} asc", label: "Date (oldest first)"
     config.add_sort_field "#{uploaded_field} desc", label: "New items"

app/controllers/concerns/hydranorth/collections/base_query.rb

@@ -12,13 +12,22 @@ def access_levels
     { read: [:read, :edit], edit: [:edit] }
   end
 
-  def perform_collection_query(query_string, access_level)
+  def perform_collection_query(query_string, access_level, sort_order='sortable_title_ssi asc')
     authenticate_user! unless access_level.blank?
+    query_elements = {q: query_string}
+
+    # This seems silly, but
+    # we need to pass the sort order through the search_builder because it does some sort-key demangling, but
+    # SearchBuilder#query fails to include the sort order in the final query, which I THINK is a bug, so we have to
+    # put the demangled version back in anyways.
+    query_elements[:sort] = sort_order if sort_order.present?
+
+    search_builder = collections_search_builder(access_level).with(query_elements)
+    sort = search_builder.sort
+    query = search_builder.query
+    query[:sort] = sort if sort.present?
 
-    # run the solr query to find the collections
-    query = collections_search_builder(access_level).with({q: query_string}).query
     response = repository.search(query)
-    # return the user's collections (or public collections if no access_level is applied)
 
     response.documents
   end

app/controllers/concerns/hydranorth/communities_controller_behavior.rb

@@ -7,13 +7,6 @@ module CommunitiesControllerBehavior
 
     def index
       @user_communities = find_communities
-
-      # TODO we should move this into the query itself, but that can't happen until after the re-index of communities &
-      # collections populates sortable_title_ssi
-      @user_communities.sort! do |a,b|
-        a.title.downcase <=> b.title.downcase
-      end
-
       @user_collections, @grouped_user_collections = find_collections_grouped_by_community
     end
 

app/helpers/generic_file_helper.rb

@@ -10,7 +10,7 @@ def present_terms(presenter, terms=:all, &block)
     Hydranorth::PresenterRenderer.new(presenter, self).fields(terms, &block)
   end
 
-  def render_download_icon title = nil
+  def render_download_icon(title = nil)
     if @generic_file.label.present? || @generic_file.doi_url.present? || !@generic_file.filename.empty?
       if title.nil?
         link_to download_image_tag, download_path(@generic_file), { target: '_blank', rel: 'noopener noreferrer', title: "Download the document", id: "file_download", data: { label: @generic_file.id } }
@@ -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: '_blank', rel: 'noopener noreferrer', 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
@@ -33,41 +40,36 @@ def render_download_link text = nil
   # download_path(@generic_file, file: 'webm')
   # download_path(id: @asset)
   # download_path document, file: 'thumbnail'
+  #
+  # 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)
-    if args.first.is_a? String
-      item = GenericFile.find(args.first)
-    else
-      item = args.first
-    end
-    return sufia.download_path(*args) unless (item.is_a?(SolrDocument) || item.is_a?(GenericFile))
-    return item.doi_url if item.respond_to?(:doi_url) && item.doi_url.present?
+    raise ArgumentError unless args.present?
 
-    # not all doi_urls will be in SolrDocuments until a full reindex happens
-    if item.is_a?(SolrDocument) && !item.doi_url_indexed?
-      gf = GenericFile.find(item.id)
-      return gf.doi_url if gf.doi_url.present?
-    end
-    if args[1].nil?
-      if !item.label.nil?
-        return "/files/" + item.id + "/" + URI::encode(item.label)
-      elsif !item.filename.empty?
-        return "/files/" + item.id + "/" + URI::encode(item.filename.first)
-      else
-      # items with no file (which should never happen in production) return nil
-      end
-    else
-      # handle thumbnail requests, in this form:
-      #   /downloads/<noid>?file=thumbnail
-      # args[1] is therefore {:file=>"thumbnail"}
-      path = "/files/" + item.id + "?"
-      i = 0
-      args[1].each do |key,value|
-        path = path + "&" if i > 0
-        i = i + 1
-        path = path + key.to_s + "=" + URI::encode(value)
-      end
-      return path
+    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?
+
+    # otherwise path is /files/noid/(label or filename)
+    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?
+      path += Hydranorth::RawFedora::stringify_args(args.shift)
     end
+
+    return path
   end
 
   def render_collection_list(gf)
@@ -90,6 +92,21 @@ def display_multiple(value)
 
   private
 
+  # 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?
+    # it would be much faster
+    return GenericFile.find(candidate) if candidate.is_a?(String)
+    raise ArgumentError
+  end
+
   def download_image_tag(title = nil)
     if title.nil?
       image_tag "default.png", { alt: "No preview available", class: "img-responsive" }

app/helpers/sufia_helper.rb

@@ -5,25 +5,21 @@ module SufiaHelper
 
   # override SufiaHelperBehavior method for institutional_access? condition
   def sufia_thumbnail_tag(document, options)
-    # collection
     if document.collection?
-      content_tag(:span, "", class: "glyphicon glyphicon-th collection-icon-search")
-
-    # file
+      content_tag(:span, '', class: "glyphicon glyphicon-th collection-icon-search")
     else
       path =
         if cannot?(:download, document)
-          "default.png"
+          'default.png'
         elsif document.image? || document.pdf? || document.video? || document.office_document?
-          sufia.download_path document, file: 'thumbnail'
+          download_path(document, file: 'thumbnail')
         elsif document.audio?
-          "audio.png"
+          'audio.png'
         else
-          "default.png"
+          'default.png'
         end
-      options[:alt] = ""
-      image_tag path, options
+      options[:alt] = ''
+      image_tag(path, options)
     end
-  end 
-
+  end
 end

app/models/concerns/hydranorth/generic_file/export.rb

@@ -1,4 +1,4 @@
-module Hydranorth 
+module Hydranorth
   module GenericFile
     module Export
 
@@ -19,30 +19,31 @@ def export_as_openurl_ctx_kev
           license: 'license' ,
           rights: 'rights'
         }
-        Rails.logger.debug "field_map #{field_map}"
+        # this is making the rails dev console VERY noisy, so I'm disabling it for now
+        # Rails.logger.debug "field_map #{field_map}"
         field_map.each do |element, kev|
-          Rails.logger.debug "self #{self.inspect}"
-          Rails.logger.debug "self.title #{self.title.inspect}"
-          Rails.logger.debug "self.creator #{self.creator}"
-          Rails.logger.debug "self.subject #{self.subject}"
-          Rails.logger.debug "self.description #{self.description}"
-          Rails.logger.debug "self.publisher #{self.publisher}"
-          Rails.logger.debug "self.date_create #{self.date_created}"
-          Rails.logger.debug "self.resource_type #{self.resource_type}"
-          Rails.logger.debug "self.language #{self.language}"
-          Rails.logger.debug "self.license #{self.license}"
-          Rails.logger.debug "self.rights #{self.rights}"
-          Rails.logger.debug "self.contributor #{self.contributor.inspect}"
+          # Rails.logger.debug "self #{self.inspect}"
+          # Rails.logger.debug "self.title #{self.title.inspect}"
+          # Rails.logger.debug "self.creator #{self.creator}"
+          # Rails.logger.debug "self.subject #{self.subject}"
+          # Rails.logger.debug "self.description #{self.description}"
+          # Rails.logger.debug "self.publisher #{self.publisher}"
+          # Rails.logger.debug "self.date_create #{self.date_created}"
+          # Rails.logger.debug "self.resource_type #{self.resource_type}"
+          # Rails.logger.debug "self.language #{self.language}"
+          # Rails.logger.debug "self.license #{self.license}"
+          # Rails.logger.debug "self.rights #{self.rights}"
+          # Rails.logger.debug "self.contributor #{self.contributor.inspect}"
           values = self.send(element)
-          Rails.logger.debug "values #{values}"
+          # Rails.logger.debug "values #{values}"
           next if values.nil? || values.first.nil? || values.empty?
           if values.respond_to?(:each)
 
             values.each do |value|
               export_text << "rft.#{kev}=#{CGI::escape(value.to_s)}"
             end
-          else 
-            
+          else
+
             export_text << "rft.#{kev}=#{CGI::escape(values.to_s)}"
           end
 

app/models/concerns/hydranorth/solr_document_behavior.rb

@@ -24,6 +24,25 @@ def dissertant
       Array(self[Solrizer.solr_name('dissertant')]).first
     end
 
+    def 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
+      # necessary after the re-index
+      begin
+        # 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)
+        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
+        # without an attached file. We return nil and deal with this in the caller
+        return nil
+      end
+    end
+
     def doi_url
       doi = self[Solrizer.solr_name('doi_url')]
       return doi.first if doi.present? && !doi.first.nil? && doi.first != 'false'