From 8a784b3972fd88f67dadc834ca08a75ef2a78f28 Mon Sep 17 00:00:00 2001 From: tom johnson Date: Thu, 7 May 2020 15:28:07 -0700 Subject: [PATCH 1/3] expand the `EmbargoManager` interface to handle releasing embargos for Valkyrie models, we provide `Hyrax::EmbargoManager` to make the embargo lifecycle easy to work with. to date, this has only supported applying embargoes. this adds the ability to release an embargo after its period has expired. a concept of 'enforcement' is added, represented by `#enforced?`. this addresses the issue of embargoes which are not current (their period is past) but have not yet been released. the terminology used by the `EmbargoManager` is documented. see also: https://github.com/samvera/hydra-head/issues/511. --- app/services/hyrax/embargo_manager.rb | 112 +++++++++++++++++++- spec/services/hyrax/embargo_manager_spec.rb | 72 ++++++++++++- 2 files changed, 178 insertions(+), 6 deletions(-) diff --git a/app/services/hyrax/embargo_manager.rb b/app/services/hyrax/embargo_manager.rb index b481ac1354..c03474b86d 100644 --- a/app/services/hyrax/embargo_manager.rb +++ b/app/services/hyrax/embargo_manager.rb @@ -5,7 +5,24 @@ module Hyrax # Provides utilities for managing the lifecycle of an `Hyrax::Embargo` on a # `Hyrax::Resource`. # - # This can do things like + # The embargo terminology used here is as follows: + # + # - "Release Date" is the day an embargo is scheduled to be released. + # - "Under Embargo" means the embargo is "active"; i.e. that its release + # 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. + # + # 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 + # embargo, e.g. via a scheduled job). Additionally, an embargo may be + # `#enforced?` after its release date (in this case, the application should + # seek to release the embargo). # # @example check whether a resource is under an active embargo # manager = EmbargoManager.new(resource: my_resource) @@ -21,9 +38,42 @@ module Hyrax # # manager = EmbargoManager.new(resource: resource) # - # manager.apply + # manager.apply! + # manager.enforced? => true # resource.visibility # => 'restricted' # + # @example releasing an embargo + # embargo = Hyrax::Embargo.new(visibility_during_embargo: 'restricted', + # visibility_after_embargo: 'open', + # embargo_release_date: Time.zone.today + 1000) + # + # @example releasing an embargo + # embargo = Hyrax::Embargo.new(visibility_during_embargo: 'restricted', + # visibility_after_embargo: 'open', + # embargo_release_date: Time.zone.today + 1) + # + # resource = Hyrax::Resource.new(embargo: embargo) + # manager = EmbargoManager.new(resource: resource) + # + # manager.under_embargo? => true + # manager.enforced? => false + # + # manager.apply! + # + # resource.visibility # => 'restricted' + # manager.enforced? => true + # + # manager.release! # => NotReleasableError + # + # # ONE DAY LATER + # manager.under_embargo? => false + # manager.enforced? => true + # + # manager.release! + # + # resource.visibility # => 'open' + # manager.enforced? => false + # class EmbargoManager ## # @!attribute [rw] resource @@ -48,10 +98,25 @@ def apply_embargo_for(resource:, query_service: Hyrax.query_service) .apply end + def apply_embargo_for!(resource:, query_service: Hyrax.query_service) + new(resource: resource, query_service: query_service) + .apply! + end + def embargo_for(resource:, query_service: Hyrax.query_service) new(resource: resource, query_service: query_service) .embargo end + + def release_embargo_for(resource:, query_service: Hyrax.query_service) + new(resource: resource, query_service: query_service) + .release + end + + def release_embargo_for!(resource:, query_service: Hyrax.query_service) + new(resource: resource, query_service: query_service) + .release! + end end ## @@ -68,6 +133,8 @@ def copy_embargo_to(target:) end ## + # Sets the visibility of the resource to the embargo's visibility condition + # # @return [Boolean] def apply return false unless under_embargo? @@ -76,17 +143,54 @@ def apply end ## - # @return [Valkyrie::Resource] + # @return [void] + # @raise [NotEnforcableError] when trying to apply an embargo that isn't active + def apply! + apply || raise(NotEnforcableError) + end + + ## + # @return [Boolean] + def enforced? + embargo.visibility_during_embargo.to_s == resource.visibility + end + + ## + # @return [Hyrax::Embargo] def embargo resource.embargo || Embargo.new end ## - # @return [Boolean] + # Sets the visibility of the resource to the embargo's visibility condition. + # no-op if the embargo period is current. + # + # @return [Boolean] truthy if the embargo has been applied + def release + return false if under_embargo? + return true if embargo.visibility_after_embargo.nil? + + resource.visibility = embargo.visibility_after_embargo + end + + ## + # @return [void] + # @raise [NotEnforcableError] when trying to release an embargo that + # is currently active + def release! + release || raise(NotReleasableError) + end + + ## + # @return [Boolean] indicates whether the date range for the embargo's + # applicability includes the present date. def under_embargo? embargo.active? end + class NotEnforcableError < RuntimeError; end + class NotReleasableError < RuntimeError; end + private def clone_attributes diff --git a/spec/services/hyrax/embargo_manager_spec.rb b/spec/services/hyrax/embargo_manager_spec.rb index 446a748b3f..310515e256 100644 --- a/spec/services/hyrax/embargo_manager_spec.rb +++ b/spec/services/hyrax/embargo_manager_spec.rb @@ -21,7 +21,7 @@ end end - context 'with expried embargo' do + context 'with expired embargo' do include_context 'with expired embargo' it 'is a no-op' do @@ -52,7 +52,7 @@ .from nil end - context 'with expried embargo' do + context 'with expired embargo' do include_context 'with expired embargo' it 'does not copy the embargo' do @@ -83,6 +83,36 @@ end end + describe '#enforced' do + it { is_expected.not_to be_enforced } + + context 'when under embargo' do + include_context 'when under embargo' + + it { is_expected.not_to be_enforced } + + context 'and it is applied' do + before { manager.apply! } + + it { is_expected.to be_enforced } + end + end + + context 'with expired embargo' do + include_context 'with expired embargo' + + it { is_expected.not_to be_enforced } + end + + context 'with an embargo that is in force, but expired' do + include_context 'with expired embargo' + + before { resource.visibility = embargo.visibility_during_embargo } + + it { is_expected.to be_enforced } + end + end + describe '#embargo' do it 'gives an inactive embargo' do expect(manager.embargo).not_to be_active @@ -105,6 +135,44 @@ end end + describe '#release' do + context 'with no embargo' do + it 'is a no-op' do + expect { manager.release } + .not_to change { resource.visibility } + end + end + + context 'with expired embargo' do + include_context 'with expired embargo' + + it 'ensures the post-embargo visibility is set' do + manager.release + expect(resource.visibility).to eq embargo.visibility_after_embargo + end + + context 'and embargo was applied' do + before { resource.visibility = embargo.visibility_during_embargo } + + it 'ensures the post-embargo visibility is set' do + expect { manager.release } + .to change { resource.visibility } + .from(embargo.visibility_during_embargo) + .to embargo.visibility_after_embargo + end + end + end + + context 'when under embargo' do + include_context 'when under embargo' + + it 'is a no-op' do + expect { manager.release } + .not_to change { resource.visibility } + end + end + end + describe '#under_embargo?' do it { is_expected.not_to be_under_embargo } From a43faeee07ed03826b0d388ce6b7bf0ec988d0cb Mon Sep 17 00:00:00 2001 From: tom johnson Date: Thu, 7 May 2020 16:08:07 -0700 Subject: [PATCH 2/3] document and test `Hyrax::LeaseManager` `LeaseManager` is substantially similar to `EmbargoManager`, and got in without tests, with the idea that a refactor might quickly take place. it didn't and at this point it needs a test harness if that's ever going to happen. in the meanwhile. there are features we want to add to cover the remainder of the lifecycle. --- app/services/hyrax/lease_manager.rb | 21 +++++- spec/services/hyrax/lease_manager_spec.rb | 84 +++++++++++++++++++++++ 2 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 spec/services/hyrax/lease_manager_spec.rb diff --git a/app/services/hyrax/lease_manager.rb b/app/services/hyrax/lease_manager.rb index e95bfef42f..05692a5b80 100644 --- a/app/services/hyrax/lease_manager.rb +++ b/app/services/hyrax/lease_manager.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true module Hyrax + ## + # Provides utilities for managing the lifecycle of an `Hyrax::Lease` on a + # `Hyrax::Resource`. class LeaseManager ## # @!attribute [rw] resource @@ -31,6 +34,12 @@ def lease_for(resource:, query_service: Hyrax.query_service) end end + ## + # Copies and applies the lease to a new (target) resource. + # + # @param target [Hyrax::Resource] + # + # @return [Boolean] def copy_lease_to(target:) return false unless under_lease? @@ -47,7 +56,14 @@ def apply end ## - # @return [Valkyrie::Resource] + # @return [void] + # @raise [NotEnforcableError] when trying to apply an lease that isn't active + def apply! + apply || raise(NotEnforcableError) + end + + ## + # @return [Hyrax::Lease] def lease resource.lease || Lease.new end @@ -58,6 +74,9 @@ def under_lease? lease.active? end + class NotEnforcableError < RuntimeError; end + class NotReleasableError < RuntimeError; end + private def clone_attributes diff --git a/spec/services/hyrax/lease_manager_spec.rb b/spec/services/hyrax/lease_manager_spec.rb new file mode 100644 index 0000000000..a5e2c22e75 --- /dev/null +++ b/spec/services/hyrax/lease_manager_spec.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +RSpec.describe Hyrax::LeaseManager do + subject(:manager) { described_class.new(resource: resource) } + let(:resource) { Hyrax::Resource.new } + + shared_context 'when under lease' do + let(:resource) { FactoryBot.build(:hyrax_resource, :under_lease) } + end + + shared_context 'with expired lease' do + let(:resource) { FactoryBot.build(:hyrax_resource, lease: lease) } + let(:lease) { FactoryBot.build(:hyrax_lease, :expired) } + end + + describe '#apply' do + context 'with no lease' do + it 'is a no-op' do + expect { manager.apply } + .not_to change { resource.visibility } + end + end + + context 'with expired lease' do + include_context 'with expired lease' + + it 'is a no-op' do + expect { manager.apply } + .not_to change { resource.visibility } + end + end + + context 'when under lease' do + include_context 'when under lease' + + before { resource.visibility = 'restricted' } + + it 'applies the active lease visibility' do + expect { manager.apply } + .to change { resource.visibility } + .from('restricted') + .to 'open' + end + end + end + + describe '#lease' do + it 'gives an inactive lease' do + expect(manager.lease).not_to be_active + end + + context 'when under lease' do + include_context 'when under lease' + + it 'gives an active lease' do + expect(manager.lease).to be_active + end + + it 'has lease attributes' do + expect(manager.lease) + .to have_attributes visibility_after_lease: 'authenticated', + visibility_during_lease: 'open', + lease_expiration_date: an_instance_of(Date), + lease_history: be_empty + end + end + end + + describe '#under_lease?' do + it { is_expected.not_to be_under_lease } + + context 'when under lease' do + include_context 'when under lease' + + it { is_expected.to be_under_lease } + end + + context 'with expired lease' do + include_context 'with expired lease' + + it { is_expected.not_to be_under_lease } + end + end +end From 4e868ba41507c47353babd67d796e5d52b1a0b13 Mon Sep 17 00:00:00 2001 From: tom johnson Date: Thu, 7 May 2020 16:44:59 -0700 Subject: [PATCH 3/3] expand the `LeaseManager` interface to handle releasing leases --- app/services/hyrax/lease_manager.rb | 21 +++++++ spec/services/hyrax/lease_manager_spec.rb | 70 ++++++++++++++++++++++- 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/app/services/hyrax/lease_manager.rb b/app/services/hyrax/lease_manager.rb index 05692a5b80..97fbf6c8e1 100644 --- a/app/services/hyrax/lease_manager.rb +++ b/app/services/hyrax/lease_manager.rb @@ -62,12 +62,33 @@ def apply! apply || raise(NotEnforcableError) end + ## + # @return [Boolean] + def enforced? + lease.visibility_during_lease.to_s == resource.visibility + end + ## # @return [Hyrax::Lease] def lease resource.lease || Lease.new end + ## + # @return [Boolean] + def release + return false if under_lease? + return true if lease.visibility_after_lease.nil? + + resource.visibility = lease.visibility_after_lease + end + + ## + # @return [void] + def release! + release || raise(NotReleasableError) + end + ## # @return [Boolean] def under_lease? diff --git a/spec/services/hyrax/lease_manager_spec.rb b/spec/services/hyrax/lease_manager_spec.rb index a5e2c22e75..2bb9ef8a3b 100644 --- a/spec/services/hyrax/lease_manager_spec.rb +++ b/spec/services/hyrax/lease_manager_spec.rb @@ -10,7 +10,7 @@ shared_context 'with expired lease' do let(:resource) { FactoryBot.build(:hyrax_resource, lease: lease) } - let(:lease) { FactoryBot.build(:hyrax_lease, :expired) } + let(:lease) { FactoryBot.build(:hyrax_lease, :expired) } end describe '#apply' do @@ -44,6 +44,36 @@ end end + describe '#enforced' do + it { is_expected.not_to be_enforced } + + context 'when under lease' do + include_context 'when under lease' + + it { is_expected.not_to be_enforced } + + context 'and it is applied' do + before { manager.apply! } + + it { is_expected.to be_enforced } + end + end + + context 'with expired lease' do + include_context 'with expired lease' + + it { is_expected.not_to be_enforced } + end + + context 'with an lease that is in force, but expired' do + include_context 'with expired lease' + + before { resource.visibility = lease.visibility_during_lease } + + it { is_expected.to be_enforced } + end + end + describe '#lease' do it 'gives an inactive lease' do expect(manager.lease).not_to be_active @@ -66,6 +96,44 @@ end end + describe '#release' do + context 'with no lease' do + it 'is a no-op' do + expect { manager.release } + .not_to change { resource.visibility } + end + end + + context 'with expired lease' do + include_context 'with expired lease' + + it 'ensures the post-lease visibility is set' do + manager.release + expect(resource.visibility).to eq lease.visibility_after_lease + end + + context 'and lease was applied' do + before { resource.visibility = lease.visibility_during_lease } + + it 'ensures the post-lease visibility is set' do + expect { manager.release } + .to change { resource.visibility } + .from(lease.visibility_during_lease) + .to lease.visibility_after_lease + end + end + end + + context 'when under lease' do + include_context 'when under lease' + + it 'is a no-op' do + expect { manager.release } + .not_to change { resource.visibility } + end + end + end + describe '#under_lease?' do it { is_expected.not_to be_under_lease }