[ESSI-2062] unify #versioned_lookup_id, #basic_lookup_id split

Author: aploshay

Diff for: app/presenters/hyrax/displays_image.rb

@@ -24,7 +24,7 @@ def display_image
 
     private
       def iiif_path_service
-        @iiif_path_service ||= IIIFFileSetPathService.new(solr_document, versioned_lookup: true)
+        @iiif_path_service ||= IIIFFileSetPathService.new(solr_document)
       end
 
       def iiif_endpoint

Diff for: app/services/iiif_file_set_path_service.rb

@@ -1,10 +1,8 @@
 class IIIFFileSetPathService
-  attr_reader :file_set, :versioned_lookup
+  attr_reader :file_set
 
   # @param [ActiveFedora::SolrHit, FileSet, Hash, SolrDocument, Hyrax::FileSetPresenter] file_set
-  # @param [TrueClass, FalseClass, NilClass] versioned_lookup: whether to use versioned file lookup if original_file_id fails
-  def initialize(file_set, versioned_lookup: false)
-    @versioned_lookup = versioned_lookup
+  def initialize(file_set)
     file_set = SolrDocument.new(file_set) if file_set.class.in? [ActiveFedora::SolrHit, Hash]
     if [:id, :content_location, :original_file_id].map { |m| file_set.respond_to?(m) }.all?
       @file_set = file_set
@@ -14,7 +12,7 @@ def initialize(file_set, versioned_lookup: false)
   end
 
   def lookup_id
-    @lookup_id ||= (@versioned_lookup ? versioned_lookup_id : basic_lookup_id)
+    @lookup_id ||= versioned_lookup_id
   end
 
   # @return [String] a URL that resolves to an image provided by a IIIF image server
@@ -30,27 +28,18 @@ def iiif_info_url(base_url)
   end
 
   private
-    # imported logic from IIIFThumbnailPathService, etc
-    def basic_lookup_id
-      @basic_lookup_id ||= (file_set.content_location || file_set.original_file_id)
-    end
-
-    # imported from Hyrax::DisplaysImage
     def versioned_lookup_id
-      @versioned_lookup_id ||= begin
-        result = basic_lookup_id
-        if result.blank?
-          Rails.logger.warn "original_file_id for #{file_set.id} not found, falling back to Fedora."
-          # result = Hyrax::VersioningService.versioned_file_id(original_file)
-          result = versioned_file_id(original_file) if original_file
-
-        end
-        if result.blank?
-          Rails.logger.warn "original_file for #{file_set.id} not found, versioned_lookup_id failed."
-          nil
-        else
-          result
-        end
+      result = file_set.content_location || file_set.original_file_id
+      if result.blank?
+        Rails.logger.warn "original_file_id for #{file_set.id} not found, falling back to Fedora."
+        # result = Hyrax::VersioningService.versioned_file_id(original_file)
+        result = versioned_file_id(original_file) if original_file
+      end
+      if result.blank?
+        Rails.logger.warn "original_file for #{file_set.id} not found, versioned_lookup_id failed."
+        nil
+      else
+        result
       end
     end
 

Diff for: spec/services/iiif_file_set_path_service_spec.rb

@@ -14,7 +14,7 @@
   let(:file_set_presenter) { Hyrax::FileSetPresenter.new(source, Ability.new(FactoryBot.build(:user))) }
   let(:versioned_lookup) { false } # default, override when desired
   # must explicitly define :resource
-  let(:service) { described_class.new(resource, versioned_lookup: versioned_lookup) }
+  let(:service) { described_class.new(resource) }
 
   # requires defining lookup_id, url
   shared_examples "url examples" do
@@ -41,23 +41,14 @@
     end
     context "with versioned_file_id lookup" do
       let(:file_set) { empty_file_set }
-      let(:versioned_lookup) { true }
-      context "that fails" do
-        it "returns nil" do
-          expect(lookup_id).to be_nil
-          expect(url).to be_nil
-        end
+      before do
+        allow(service).to receive(:original_file).and_return(local_file_set.original_file)
       end
-      context "that succeeds" do
-        before do
-          allow(service).to receive(:original_file).and_return(local_file_set.original_file)
-        end
-        it "returns a url" do
-          expect(lookup_id).not_to match /^s3/
-          expect(url).to match /^http/
-          # below is proxy check that #versioned_file_id was called
-          expect(service.instance_variable_get(:@versioned_file_id)).not_to be_nil
-        end
+      it "returns a url" do
+        expect(lookup_id).not_to match /^s3/
+        expect(url).to match /^http/
+        # below is proxy check that #versioned_file_id was called
+        expect(service.instance_variable_get(:@versioned_file_id)).not_to be_nil
       end
     end
   end