5844 deactivated embargoes (#6098)

* update the solr_params[:fq] value in #with_deactivated_embargos to match whats in the dassie app.

ref: #5844

* properly set "embargo.embargo_history" when deactivating an embargo on koppie

* save the embargo on the work after its been updated, when deactivating the embargo.
remove an errant pry

* wip: able to display past embargoes on the edit embargo page, and under the deactivated embargoes tab.

the edit page now does not reflect that the embargo is gone though.

co-authored-by: braydon <braydon.justice@1268456bcltd.ca>

* verify that there is an embargo release date in #enforced? before saying there is an embargo.

before this commit if a user simply changed the visibility of a work to "restricted", then the app would think there was an embargo on the work since the embargo visibility was also "restricted".

co-authored-by: braydon <braydon.justice@1268456bcltd.ca>

* index the "embargo_history_ssim" value in solr when an embargo has been deactivated so that it shows up on the "deactivated embargoes" tab of embargoes/index.

co-authored-by: braydon <braydon.justice@1268456bcltd.ca>

* stop persisting the work in EmbargoActor#destroy because the AccessControlList persists it for us.

something is still going on weird though. it appears that the embargo object in the database is different than the embargo metadata thats getting saved on the work. this means that the "past embargoes" section on the embargo/edit page shows different data than the data in the "deactivated embargoes" tab on the embargo/index page.

co-authored-by: braydon <braydon.justice@1268456bcltd.ca>

* save the embargo id to the work as a reference instead of the embargo itself.

this change fixes the problem where the updated embargo is different than the embargo that got saved on the work.
co-authored-by: braydon <braydon.justice@1268456bcltd.ca>

* lease structure now matches embargo structure

* save the embargo and lease before setting it, so we have access to the id

* embargoes and leases are no longer saved to the work itself. instead, we need to find them by their id

* rubocop fixes

* embargoes and leases now save during changeset sync along with the resource. embargo_manager lazy loads embargo.

* update leases to work like embargoes. use the safe navigation operator when calling methods in the valkyrie_work_indexer

* fix some specs.

co-authored-by: braydon <braydon.justice@1268456bcltd.ca>

* fix lease spec issues

* embargo actor spec fixes

* decouple the deactivated lease work from the deactivated embargo work. that work can now be found on #6111.

* fix one more embargo manager spec

* move a few more lease related changes to #6111

* Update app/services/hyrax/embargo_manager.rb

Co-authored-by: Benjamin Kiah Stroud <32469930+bkiahstroud@users.noreply.github.com>

* Update app/models/hyrax/resource.rb

Co-authored-by: Benjamin Kiah Stroud <32469930+bkiahstroud@users.noreply.github.com>

---------

Co-authored-by: braydon <braydon.justice@1268456bcltd.ca>
Co-authored-by: Rob Kaufman <rob@notch8.com>
Co-authored-by: braydonjustice <braydonjustice@gmail.com>
Co-authored-by: Benjamin Kiah Stroud <32469930+bkiahstroud@users.noreply.github.com>

app/actors/hyrax/actors/embargo_actor.rb

@@ -16,8 +16,9 @@ def destroy
         when Valkyrie::Resource
           embargo_manager = Hyrax::EmbargoManager.new(resource: work)
           return if embargo_manager.embargo.embargo_release_date.blank?
-          embargo_manager.nullify(force: true)
-          embargo_manager.release(force: true)
+
+          embargo_manager.deactivate!
+          work.embargo = Hyrax.persister.save(resource: embargo_manager.embargo)
           Hyrax::AccessControlList(work).save
         else
           work.embargo_visibility! # If the embargo has lapsed, update the current visibility.

app/controllers/concerns/hyrax/embargoes_controller_behavior.rb

@@ -15,7 +15,7 @@ def index
     # Removes a single embargo
     def destroy
       Hyrax::Actors::EmbargoActor.new(curation_concern).destroy
-      flash[:notice] = embargo_history(curation_concern)
+      flash[:notice] = embargo_history(curation_concern).last
       if curation_concern.work? && work_has_file_set_members?(curation_concern)
         redirect_to confirm_permission_path
       else

app/forms/hyrax/forms/work_embargo_form.rb

@@ -11,6 +11,7 @@ module Forms
     #   +EmbargoesControllerBehavior+.
     class WorkEmbargoForm < Hyrax::ChangeSet
       property :embargo, form: Hyrax::Forms::Embargo, populator: :embargo_populator, prepopulator: :embargo_populator
+      property :embargo_history, virtual: true, prepopulator: proc { |_opts| self.embargo_history = model.embargo&.embargo_history }
       property :embargo_release_date, virtual: true, prepopulator: proc { |_opts| self.embargo_release_date = model.embargo&.embargo_release_date }
       property :visibility_after_embargo, virtual: true, prepopulator: proc { |_opts| self.visibility_after_embargo = model.embargo&.visibility_after_embargo }
       property :visibility_during_embargo, virtual: true, prepopulator: proc { |_opts| self.visibility_during_embargo = model.embargo&.visibility_during_embargo }

app/indexers/hyrax/valkyrie_work_indexer.rb

@@ -44,11 +44,14 @@ def admin_set_label(resource)
     end
 
     def index_embargo(doc)
-      if Hyrax::EmbargoManager.new(resource: resource).under_embargo?
+      if resource.embargo&.active?
         doc['embargo_release_date_dtsi'] = resource.embargo.embargo_release_date&.to_datetime
         doc['visibility_after_embargo_ssim'] = resource.embargo.visibility_after_embargo
         doc['visibility_during_embargo_ssim'] = resource.embargo.visibility_during_embargo
+      else
+        doc['embargo_history_ssim'] = resource&.embargo&.embargo_history
       end
+
       doc
     end
 

app/models/hyrax/resource.rb

@@ -35,7 +35,7 @@ class Resource < Valkyrie::Resource
     include Hyrax::WithEvents
 
     attribute :alternate_ids, Valkyrie::Types::Array.of(Valkyrie::Types::ID)
-    attribute :embargo,       Hyrax::Embargo.optional
+    attribute :embargo_id,    Valkyrie::Types::ID
     attribute :lease,         Hyrax::Lease.optional
 
     delegate :edit_groups, :edit_groups=,
@@ -103,6 +103,17 @@ def visibility
       visibility_reader.read
     end
 
+    def embargo=(value)
+      raise TypeError "can't convert #{value.class} into Hyrax::Embargo" unless value.is_a? Hyrax::Embargo
+
+      @embargo = value
+      self.embargo_id = @embargo.id
+    end
+
+    def embargo
+      @embargo ||= Hyrax.query_service.find_by(id: embargo_id) if embargo_id.present?
+    end
+
     protected
 
     def visibility_writer

app/services/hyrax/embargo_manager.rb

@@ -17,6 +17,7 @@ module Hyrax
   #    - "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
   #
   # 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
@@ -119,6 +120,22 @@ def release_embargo_for!(resource:, query_service: Hyrax.query_service)
       end
     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,
+        DateTime.parse(embargo.embargo_release_date),
+        embargo.visibility_during_embargo,
+        embargo.visibility_after_embargo
+      )
+
+      nullify(force: true)
+      release(force: true)
+      embargo.embargo_history += [embargo_record]
+    end
+
     ##
     # Copies and applies the embargo to a new (target) resource.
     #
@@ -128,7 +145,7 @@ def release_embargo_for!(resource:, query_service: Hyrax.query_service)
     def copy_embargo_to(target:)
       return false unless under_embargo?
 
-      target.embargo = Embargo.new(clone_attributes)
+      target.embargo = Hyrax.persister.save(resource: Embargo.new(clone_attributes))
       self.class.apply_embargo_for(resource: target)
     end
 
@@ -152,7 +169,8 @@ def apply!
     ##
     # @return [Boolean]
     def enforced?
-      embargo.visibility_during_embargo.to_s == resource.visibility
+      embargo.embargo_release_date.present? &&
+        (embargo.visibility_during_embargo.to_s == resource.visibility)
     end
 
     ##
@@ -213,5 +231,17 @@ def clone_attributes
     def core_attribute_keys
       [:visibility_after_embargo, :visibility_during_embargo, :embargo_release_date]
     end
+
+    protected
+
+    # Create the log message used when deactivating an embargo
+    def embargo_history_message(state, deactivate_date, release_date, visibility_during, visibility_after)
+      I18n.t 'hydra.embargo.history_message',
+              state: state,
+              deactivate_date: deactivate_date,
+              release_date: release_date,
+              visibility_during: visibility_during,
+              visibility_after: visibility_after
+    end
   end
 end

lib/hyrax/transactions/steps/save.rb

@@ -33,6 +33,7 @@ def call(change_set, user: nil)
           begin
             new_collections = changed_collection_membership(change_set)
             unsaved = change_set.sync
+            unsaved.embargo = @persister.save(resource: unsaved.embargo) if unsaved.embargo.present?
             saved = @persister.save(resource: unsaved)
           rescue StandardError => err
             return Failure(["Failed save on #{change_set}\n\t#{err.message}", change_set.resource])

spec/actors/hyrax/actors/embargo_actor_spec.rb

@@ -6,8 +6,9 @@
 
   describe "#destroy" do
     let(:work) do
-      FactoryBot.valkyrie_create(:hyrax_resource, :under_embargo)
+      FactoryBot.valkyrie_create(:hyrax_resource, embargo: embargo)
     end
+    let(:embargo) { FactoryBot.create(:hyrax_embargo) }
 
     before do
       work.visibility = authenticated_vis
@@ -33,14 +34,14 @@
         FactoryBot.valkyrie_create(:hyrax_resource, embargo: embargo)
       end
 
-      let(:embargo) { FactoryBot.build(:hyrax_embargo) }
+      let(:embargo) { FactoryBot.create(:hyrax_embargo) }
       let(:embargo_manager) { Hyrax::EmbargoManager.new(resource: work) }
       let(:embargo_release_date) { work.embargo.embargo_release_date }
 
       before do
         allow(Hyrax::TimeService)
           .to receive(:time_in_utc)
-          .and_return(embargo_release_date + 1)
+          .and_return(embargo_release_date.to_datetime + 1)
         expect(embargo_manager.under_embargo?).to eq false
       end
 

spec/factories/hyrax_embargo.rb

@@ -1,12 +1,14 @@
 # frozen_string_literal: true
 FactoryBot.define do
   factory :hyrax_embargo, class: "Hyrax::Embargo" do
-    embargo_release_date      { Time.zone.today + 10 }
+    embargo_release_date      { (Time.zone.today + 10).to_s }
     visibility_after_embargo  { 'open' }
     visibility_during_embargo { 'authenticated' }
 
     to_create do |instance|
-      Valkyrie.config.metadata_adapter.persister.save(resource: instance)
+      saved_instance = Valkyrie.config.metadata_adapter.persister.save(resource: instance)
+      instance.id = saved_instance.id
+      saved_instance
     end
 
     trait :expired do

spec/factories/hyrax_resource.rb

@@ -5,7 +5,7 @@
 FactoryBot.define do
   factory :hyrax_resource, class: "Hyrax::Resource" do
     trait :under_embargo do
-      association :embargo, factory: :hyrax_embargo
+      embargo_id { FactoryBot.create(:hyrax_embargo).id }
     end
 
     trait :under_lease do

spec/models/hyrax/resource_spec.rb

@@ -19,7 +19,9 @@
     subject(:resource) { described_class.new(embargo: embargo) }
     let(:embargo)      { FactoryBot.build(:hyrax_embargo) }
 
-    it 'saves the embargo' do
+    it 'saves the embargo id' do
+      resource.embargo = Hyrax.persister.save(resource: embargo)
+
       expect(Hyrax.persister.save(resource: resource).embargo)
         .to have_attributes(embargo_release_date: embargo.embargo_release_date)
     end

spec/services/hyrax/embargo_manager_spec.rb

@@ -129,8 +129,8 @@
         expect(manager.embargo)
           .to have_attributes visibility_after_embargo: 'open',
                               visibility_during_embargo: 'authenticated',
-                              embargo_release_date: an_instance_of(Date),
-                              embargo_history: be_empty
+                              embargo_release_date: an_instance_of(String),
+                              embargo_history: match_array([])
       end
     end
   end