Update VisibilityTranslator to handle various edge cases

app/services/visibility_translator.rb

@@ -17,6 +17,7 @@
 #
 # @see Hydra::AccessControls::Visibility
 class VisibilityTranslator
+  RESTRICTED      = Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PRIVATE
   ALL_EMBARGOED   = 'all_restricted'.freeze
   FILES_EMBARGOED = 'files_restricted'.freeze
   TOC_EMBARGOED   = 'toc_restricted'.freeze
@@ -44,10 +45,14 @@ def self.visibility(obj:)
   # When checking whether a value is true, check whether it has a "true" string too.
   def visibility
     return proxy.visibility if obj.hidden? || !obj.under_embargo?
+
+    return OPEN             if embargo_length_none?(obj)
     return ALL_EMBARGOED    if obj.abstract_embargoed.to_s == "true"
     return TOC_EMBARGOED    if obj.toc_embargoed.to_s == "true"
+    return FILES_EMBARGOED  if obj.files_embargoed.to_s == "true"
 
-    FILES_EMBARGOED
+    Rails.logger.error("Invalid embargo values. Returning RESTRICTED for ID: #{obj.id}")
+    RESTRICTED
   end
 
   def visibility=(value)
@@ -85,6 +90,21 @@ def proxy
     VisibilityProxy.new(obj)
   end
 
+  def embargo_length_none?(obj)
+    # cast length to a String in case obj is a SolrDocument and obj.embargo_length is an arrray
+    length = obj.embargo_length.to_s
+    Rails.logger.warn "Treating nil embargo_length as open for ID: #{obj.id}" if length.empty?
+    if length.empty? || length.downcase.include?('none')
+      if obj.abstract_embargoed.to_s == "true" || obj.toc_embargoed.to_s == "true" || obj.files_embargoed.to_s == "true"
+        Rails.logger.error "Boolean embargo values conflict with embargo_length in ID: #{obj.id}"
+      end
+
+      return true
+    end
+
+    false
+  end
+
   class InvalidVisibilityError < ArgumentError
     def initialize(msg = 'Invalid visibility level:', level = nil, obj = nil)
       @level = level

spec/factories/etd.rb

@@ -125,8 +125,17 @@
         patents { "true" }
       end
 
+      factory :sample_data_with_nothing_embargoed do
+        title { ["Sample Data With Nothing Embargoed: #{FFaker::Book.title}"] }
+        embargo { FactoryBot.create(:embargo, embargo_release_date: (Time.zone.today + 14.days)) }
+        embargo_length { "None - open access immediately" }
+        files_embargoed { "false" }
+        abstract_embargoed { "false" }
+        toc_embargoed { "false" }
+      end
+
       factory :sample_data_with_everything_embargoed do
-        title { ["Sample Data With Full Embargo: #{FFaker::Book.title}"] }
+        title { ["Sample Data With Everything Embargoed: #{FFaker::Book.title}"] }
         embargo { FactoryBot.create(:embargo, embargo_release_date: (Time.zone.today + 14.days)) }
         embargo_length { "6 months" }
         files_embargoed { "true" }

spec/services/visibility_translator_spec.rb

@@ -29,12 +29,56 @@
       end
     end
 
+    context 'when not under embargo' do
+      let(:obj) { FactoryBot.create(:sample_data_with_nothing_embargoed) }
+
+      it 'is open access' do
+        expect(translator.visibility).to eq described_class::OPEN
+      end
+
+      it 'treats nil embargo_length as open access' do
+        allow(Rails.logger).to receive(:warn)
+
+        # handle cases where the UI has not saved the default select option
+        # TODO: Update UI validations to prevent this state from occurrring
+        obj.embargo_length = nil
+
+        expect(translator.visibility).to eq described_class::OPEN
+        expect(Rails.logger).to have_received(:warn).with("Treating nil embargo_length as open for ID: #{obj.id}")
+      end
+
+      it 'logs an error when state is inconsistent' do
+        allow(Rails.logger).to receive(:error)
+
+        # none of the booleans should be true if there is no embargo requested
+        obj.abstract_embargoed = true
+        obj.files_embargoed = false
+        obj.embargo_length = 'None - open access immediately'
+
+        expect(translator.visibility).to eq described_class::OPEN
+        expect(Rails.logger).to have_received(:error).with("Boolean embargo values conflict with embargo_length in ID: #{obj.id}")
+      end
+    end
+
     context 'when under full embargo' do
       let(:obj) { FactoryBot.create(:sample_data_with_everything_embargoed) }
 
       it 'is embargo (all)' do
         expect(translator.visibility).to eq described_class::ALL_EMBARGOED
       end
+
+      it 'restricts the object when state is inconsistent' do
+        allow(Rails.logger).to receive(:error)
+
+        # one or more of the booleans should be true if there is a non-zero embargo length requested
+        obj.abstract_embargoed = false
+        obj.toc_embargoed = false
+        obj.files_embargoed = false
+        obj.embargo_length = '6 - Months'
+
+        expect(translator.visibility).to eq described_class::RESTRICTED
+        expect(Rails.logger).to have_received(:error).with("Invalid embargo values. Returning RESTRICTED for ID: #{obj.id}")
+      end
     end
 
     context 'when under file-only embargo' do