Merge pull request #6241 from samvera/refine-embargo-search

refine Hyrax::EmbargoSearch behavior to find only enforced embargoes

app/actors/hyrax/actors/embargo_actor.rb

@@ -14,12 +14,9 @@ def initialize(work)
       def destroy
         case work
         when Valkyrie::Resource
-          embargo_manager = Hyrax::EmbargoManager.new(resource: work)
-          return if embargo_manager.embargo.embargo_release_date.blank?
-
-          embargo_manager.deactivate!
-          work.embargo = Hyrax.persister.save(resource: embargo_manager.embargo)
-          Hyrax::AccessControlList(work).save
+          Hyrax::EmbargoManager.deactivate_embargo_for(resource: work) &&
+            Hyrax.persister.save(resource: work.embargo) &&
+            Hyrax::AccessControlList(work).save
         else
           work.embargo_visibility! # If the embargo has lapsed, update the current visibility.
           work.deactivate_embargo!

app/models/concerns/hyrax/solr_document_behavior.rb

@@ -91,9 +91,9 @@ def creator
     end
 
     def visibility
-      @visibility ||= if embargo_release_date.present?
+      @visibility ||= if embargo_enforced?
                         Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_EMBARGO
-                      elsif lease_expiration_date.present?
+                      elsif lease_enforced?
                         Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_LEASE
                       elsif public?
                         Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PUBLIC
@@ -108,6 +108,23 @@ def collection_type_gid
       first(Hyrax.config.collection_type_index_field)
     end
 
+    def embargo_enforced?
+      return false if embargo_release_date.blank?
+
+      indexed_embargo_visibility = first('visibility_during_embargo_ssim')
+      # if we didn't index an embargo visibility, assume the release date means
+      # it's enforced
+      return true if indexed_embargo_visibility.blank?
+
+      # if the visibility and the visibility during embargo are the same, we're
+      # enforcing the embargo
+      self['visibility_ssi'] == indexed_embargo_visibility
+    end
+
+    def lease_enforced?
+      lease_expiration_date.present?
+    end
+
     private
 
     def model_classifier(classifier)

app/presenters/hyrax/embargo_presenter.rb

@@ -23,5 +23,9 @@ def visibility_after_embargo
     def embargo_history
       solr_document['embargo_history_ssim']
     end
+
+    def enforced?
+      solr_document.embargo_enforced?
+    end
   end
 end

app/services/hyrax/embargo_manager.rb

@@ -12,12 +12,13 @@ module Hyrax
   #       date is today or later.
   #    - "Applied" means the embargo's pre-release visibility has been set on
   #      the resource.
-  #    - "Released" means the embargo's post-release visibility has been set on
-  #      the resource.
   #    - "Enforced" means the object's visibility matches the pre-release
   #      visibility of the embargo; i.e. the embargo has been applied,
   #      but not released.
-  #    - "Deactivate" means that the existing embargo will be removed
+  #    - "Released" means the embargo's post-release visibility has been set on
+  #      the resource.
+  #    - "Deactivate" means that the existing embargo will be removed, even
+  #      if it active.
   #
   # Note that an resource may be `#under_embargo?` even if the embargo is not
   # be `#enforced?` (in this case, the application should seek to apply the
@@ -109,11 +110,25 @@ def embargo_for(resource:, query_service: Hyrax.query_service)
           .embargo
       end
 
+      # @return [Boolean]
+      def deactivate_embargo_for(resource:, query_service: Hyrax.query_service)
+        new(resource: resource, query_service: query_service)
+          .deactivate
+      end
+
+      # @return [Boolean]
+      def deactivate_embargo_for!(resource:, query_service: Hyrax.query_service)
+        new(resource: resource, query_service: query_service)
+          .deactivate!
+      end
+
+      # @return [Boolean]
       def release_embargo_for(resource:, query_service: Hyrax.query_service)
         new(resource: resource, query_service: query_service)
           .release
       end
 
+      # @return [Boolean]
       def release_embargo_for!(resource:, query_service: Hyrax.query_service)
         new(resource: resource, query_service: query_service)
           .release!
@@ -149,20 +164,20 @@ def create_or_update_embargo_on_members(members, work)
       # rubocop:enable Metrics/AbcSize, Metrics/MethodLength
     end
 
-    # Deactivates the embargo and logs a message to the embargo_history property
-    def deactivate!
-      embargo_state = embargo.active? ? 'active' : 'expired'
-      embargo_record = embargo_history_message(
-        embargo_state,
-        Time.zone.today,
-        embargo.embargo_release_date,
-        embargo.visibility_during_embargo,
-        embargo.visibility_after_embargo
-      )
+    ##
+    # Deactivates the embargo
+    # @return [Boolean]
+    def deactivate
+      release(force: true) &&
+        nullify(force: true)
+    end
 
-      release(force: true)
+    ##
+    # Deactivates the embargo
+    # @return [Boolean]
+    def deactivate!
+      release!(force: true)
       nullify(force: true)
-      embargo.embargo_history += [embargo_record]
     end
 
     ##
@@ -211,15 +226,16 @@ def embargo
     ##
     # Drop the embargo by setting its release date and visibility settings to `nil`.
     #
-    # @param force [boolean] force the nullify even when the embargo period is current
+    # @param force [Boolean] force the nullify even when the embargo period is current
     #
-    # @return [void]
+    # @return [Boolean]
     def nullify(force: false)
       return false if !force && under_embargo?
 
       embargo.embargo_release_date = nil
       embargo.visibility_during_embargo = nil
       embargo.visibility_after_embargo = nil
+      true
     end
 
     ##
@@ -231,6 +247,17 @@ def nullify(force: false)
     # @return [Boolean] truthy if the embargo has been applied
     def release(force: false)
       return false if !force && under_embargo?
+
+      embargo_state = embargo.active? ? 'active' : 'expired'
+      history_record = embargo_history_message(
+        embargo_state,
+        Hyrax::TimeService.time_in_utc,
+        embargo.embargo_release_date,
+        embargo.visibility_during_embargo,
+        embargo.visibility_after_embargo
+      )
+      embargo.embargo_history += [history_record]
+
       return true if embargo.visibility_after_embargo.nil?
 
       resource.visibility = embargo.visibility_after_embargo
@@ -266,7 +293,7 @@ def core_attribute_keys
 
     protected
 
-    # Create the log message used when deactivating an embargo
+    # Create the log message used when releasing an embargo
     def embargo_history_message(state, deactivate_date, release_date, visibility_during, visibility_after)
       I18n.t 'hydra.embargo.history_message',
               state: state,

app/services/hyrax/embargo_service.rb

@@ -1,24 +1,26 @@
 # frozen_string_literal: true
 module Hyrax
+  ##
+  # Methods for Querying Repository to find Embargoed Objects
   class EmbargoService < RestrictionService
     class << self
-      #
-      # Methods for Querying Repository to find Embargoed Objects
-      #
-
       # Returns all assets with embargo release date set to a date in the past
-      def assets_with_expired_embargoes
+      def assets_with_expired_enforced_embargoes
         builder = Hyrax::ExpiredEmbargoSearchBuilder.new(self)
         presenters(builder)
       end
+      alias assets_with_expired_embargoes assets_with_expired_enforced_embargoes
 
-      # Returns all assets with embargo release date set
-      #   (assumes that when lease visibility is applied to assets
-      #    whose leases have expired, the lease expiration date will be removed from its metadata)
-      def assets_under_embargo
+      ##
+      # Returns all assets with embargoes that are currently enforced,
+      # regardless of whether the embargoes are active or expired.
+      #
+      # @see Hyrax::EmbargoManager
+      def assets_with_enforced_embargoes
         builder = Hyrax::EmbargoSearchBuilder.new(self)
-        presenters(builder)
+        presenters(builder).select(&:enforced?)
       end
+      alias assets_under_embargo assets_with_enforced_embargoes
 
       # Returns all assets that have had embargoes deactivated in the past.
       def assets_with_deactivated_embargoes

lib/tasks/embargo_lease.rake

@@ -8,6 +8,7 @@ namespace :hyrax do
 
       Hyrax.query_service.find_many_by_ids(ids: ids).each do |resource|
         Hyrax::EmbargoManager.release_embargo_for(resource: resource) &&
+          Hyrax.persister.save(resource: resource.embargo) &&
           Hyrax::AccessControlList(resource).save
       end
     end

spec/actors/hyrax/actors/embargo_actor_spec.rb

@@ -1,41 +1,39 @@
 # frozen_string_literal: true
-RSpec.describe Hyrax::Actors::EmbargoActor, :active_fedora do
+RSpec.describe Hyrax::Actors::EmbargoActor, :clean_repo do
   let(:actor) { described_class.new(work) }
-  let(:authenticated_vis) { Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_AUTHENTICATED }
-  let(:public_vis) { Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PUBLIC }
+  let(:restricted_vis) { 'restricted' }
+  let(:authenticated_vis) { 'authenticated' }
+  let(:public_vis) { 'open' }
 
-  describe '#destroy' do
-    context 'on a Valkyrie backed model', if: Hyrax.config.use_valkyrie? do
-      let(:work) { FactoryBot.valkyrie_create(:hyrax_resource, embargo: embargo) }
-      let(:embargo) { FactoryBot.create(:hyrax_embargo) }
-      let(:embargo_manager) { Hyrax::EmbargoManager.new(resource: work) }
-      let(:active_embargo_release_date) { work.embargo.embargo_release_date }
-
-      before do
-        work.visibility = authenticated_vis
-        Hyrax::AccessControlList(work).save
-      end
-
-      it 'removes the embargo' do
-        actor.destroy
+  def embargo_manager(work)
+    Hyrax::EmbargoManager
+      .new(resource: Hyrax.query_service.find_by(id: work.id))
+  end
 
-        expect(work.embargo.embargo_release_date).to eq nil
-        expect(work.embargo.visibility_after_embargo).to eq nil
-        expect(work.embargo.visibility_during_embargo).to eq nil
-      end
+  describe '#destroy' do
+    context 'on a Valkyrie backed model' do
+      let(:work) { FactoryBot.valkyrie_create(:hyrax_work, :under_embargo) }
 
       it 'releases the embargo' do
         expect { actor.destroy }
-          .to change { embargo_manager.enforced? }
+          .to change { embargo_manager(work).enforced? }
           .from(true)
           .to false
       end
 
-      it 'changes the embargo release date' do
+      it 'adds embargo history' do
+        expect { actor.destroy }
+          .to change { embargo_manager(work).embargo.embargo_history }
+          .to include start_with("An active embargo was deactivated")
+      end
+
+      it 'removes the embargo from the UI' do
+        helper = Class.new { include Hyrax::EmbargoHelper }
+
         expect { actor.destroy }
-          .to change { work.embargo.embargo_release_date }
-          .from(active_embargo_release_date)
-          .to nil
+          .to change { helper.new.assets_under_embargo }
+          .from(contain_exactly(have_attributes(id: work.id)))
+          .to be_empty
       end
 
       it 'changes the visibility' do
@@ -46,21 +44,35 @@
       end
 
       context 'with an expired embargo' do
-        let(:work) { valkyrie_create(:hyrax_resource, embargo: expired_embargo) }
-        let(:expired_embargo) { create(:hyrax_embargo, :expired) }
-        let(:embargo_manager) { Hyrax::EmbargoManager.new(resource: work) }
-        let(:embargo_release_date) { work.embargo.embargo_release_date }
+        let!(:work) { FactoryBot.valkyrie_create(:hyrax_work, :with_expired_enforced_embargo) }
+
+        it 'releases the embargo' do
+          expect { actor.destroy }
+            .to change { embargo_manager(work).enforced? }
+            .from(true)
+            .to false
+        end
+
+        it 'adds embargo history' do
+          expect { actor.destroy }
+            .to change { embargo_manager(work).embargo.embargo_history }
+            .to include start_with("An expired embargo was deactivated")
+        end
+
+        it 'removes the embargo from the UI' do
+          helper = Class.new { include Hyrax::EmbargoHelper }
+
+          work # create it
 
-        it 'removes the embargo' do
           expect { actor.destroy }
-            .to change { work.embargo.embargo_release_date }
-            .from(embargo_release_date)
-            .to nil
+            .to change { helper.new.assets_under_embargo }
+            .from(contain_exactly(have_attributes(id: work.id)))
+            .to be_empty
         end
       end
     end
 
-    context 'with an ActiveFedora model', unless: Hyrax.config.use_valkyrie? do
+    context 'with an ActiveFedora model', :active_fedora do
       let(:work) do
         GenericWork.new do |work|
           work.apply_depositor_metadata 'foo'
@@ -73,6 +85,8 @@
         end
       end
 
+      let(:embargo_release_date) { work.embargo.embargo_release_date }
+
       context 'with an active embargo' do
         let(:embargo_release_date) { Time.zone.today + 2 }
 

spec/factories/hyrax_work.rb

@@ -6,6 +6,25 @@
   factory :hyrax_work, class: 'Hyrax::Test::SimpleWork' do
     trait :under_embargo do
       association :embargo, factory: :hyrax_embargo
+
+      after(:create) do |work, _e|
+        Hyrax::EmbargoManager.new(resource: work).apply
+        work.permission_manager.acl.save
+      end
+    end
+
+    trait :with_expired_enforced_embargo do
+      after(:build) do |work, _evaluator|
+        work.embargo = FactoryBot.valkyrie_create(:hyrax_embargo, :expired)
+      end
+
+      after(:create) do |work, _evaluator|
+        allow(Hyrax::TimeService).to receive(:time_in_utc).and_return(10.days.ago)
+        Hyrax::EmbargoManager.new(resource: work).apply
+        allow(Hyrax::TimeService).to receive(:time_in_utc).and_call_original
+
+        work.permission_manager.acl.save
+      end
     end
 
     trait :under_lease do

spec/helpers/hyrax/embargo_helper_spec.rb

@@ -154,7 +154,7 @@
   end
 
   describe '#embargo_history' do
-    context 'with an ActiveFedora resource' do
+    context 'with an ActiveFedora resource', :active_fedora do
       let(:resource) { FactoryBot.build(:work) }
 
       it 'is empty' do