diff --git a/app/actors/hyrax/actors/base_actor.rb b/app/actors/hyrax/actors/base_actor.rb index 7a6882daa0..33f6839e10 100644 --- a/app/actors/hyrax/actors/base_actor.rb +++ b/app/actors/hyrax/actors/base_actor.rb @@ -71,7 +71,8 @@ def apply_deposit_date(env) def save(env, use_valkyrie: false) return env.curation_concern.save unless use_valkyrie - resource = valkyrie_save(resource: env.curation_concern.valkyrie_resource) + # don't run validations again on the converted object if they've already passed + resource = valkyrie_save(resource: env.curation_concern.valkyrie_resource, is_valid: env.curation_concern.save) # we need to manually set the id and reload, because the actor stack requires # `env.curation_concern` to be the exact same instance throughout. @@ -115,9 +116,9 @@ def multivalued_form_attributes(attributes) attributes.select { |_, v| v.respond_to?(:select) && !v.respond_to?(:read) } end - def valkyrie_save(resource:) + def valkyrie_save(resource:, is_valid:) permissions = resource.permission_manager.acl.permissions - resource = Hyrax.persister.save(resource: resource) + resource = Hyrax.persister.save(resource: resource, perform_af_validation: !is_valid) resource.permission_manager.acl.permissions = permissions resource.permission_manager.acl.save diff --git a/app/actors/hyrax/actors/lease_actor.rb b/app/actors/hyrax/actors/lease_actor.rb index 6933e58f59..5b57da5e32 100644 --- a/app/actors/hyrax/actors/lease_actor.rb +++ b/app/actors/hyrax/actors/lease_actor.rb @@ -15,8 +15,11 @@ def destroy case work when Valkyrie::Resource lease_manager = Hyrax::LeaseManager.new(resource: work) - lease_manager.release && Hyrax::AccessControlList(work).save - lease_manager.nullify + return if lease_manager.lease.lease_expiration_date.blank? + + lease_manager.deactivate! + work.lease = Hyrax.persister.save(resource: lease_manager.lease) + Hyrax::AccessControlList(work).save else work.lease_visibility! # If the lease has lapsed, update the current visibility. work.deactivate_lease! diff --git a/app/controllers/concerns/hyrax/works_controller_behavior.rb b/app/controllers/concerns/hyrax/works_controller_behavior.rb index 74ff31eb9a..1b4b7b6b23 100644 --- a/app/controllers/concerns/hyrax/works_controller_behavior.rb +++ b/app/controllers/concerns/hyrax/works_controller_behavior.rb @@ -180,15 +180,18 @@ def actor ## # @return [#errors] + # rubocop:disable Metrics/MethodLength def create_valkyrie_work form = build_form - return after_create_error(form_err_msg(form)) unless form.validate(params[hash_key_for_curation_concern]) + # fallback to an empty hash to avoid: # NoMethodError: undefined method `has_key?` for nil:NilClass + original_input_params_for_form = params[hash_key_for_curation_concern] ? params[hash_key_for_curation_concern] : {} + return after_create_error(form_err_msg(form), original_input_params_for_form) unless form.validate(original_input_params_for_form) result = transactions['change_set.create_work'] .with_step_args( 'work_resource.add_to_parent' => { parent_id: params[:parent_id], user: current_user }, - 'work_resource.add_file_sets' => { uploaded_files: uploaded_files, file_set_params: params[hash_key_for_curation_concern][:file_set] }, + 'work_resource.add_file_sets' => { uploaded_files: uploaded_files, file_set_params: original_input_params_for_form[:file_set] }, 'change_set.set_user_as_depositor' => { user: current_user }, 'work_resource.change_depositor' => { user: ::User.find_by_user_key(form.on_behalf_of) }, 'work_resource.save_acl' => { permissions_params: form.input_params["permissions"] } @@ -197,6 +200,7 @@ def create_valkyrie_work @curation_concern = result.value_or { return after_create_error(transaction_err_msg(result)) } after_create_response end + # rubocop:enable Metrics/MethodLength def update_valkyrie_work form = build_form diff --git a/app/forms/hyrax/forms/work_lease_form.rb b/app/forms/hyrax/forms/work_lease_form.rb index 99a59608e0..469ac65cb9 100644 --- a/app/forms/hyrax/forms/work_lease_form.rb +++ b/app/forms/hyrax/forms/work_lease_form.rb @@ -11,6 +11,7 @@ module Forms # +LeasesControllerBehavior+. class WorkLeaseForm < Hyrax::ChangeSet property :lease, form: Hyrax::Forms::Lease, populator: :lease_populator, prepopulator: :lease_populator + property :lease_history, virtual: true, prepopulator: proc { |_opts| self.lease_history = model.lease&.lease_history } property :lease_expiration_date, virtual: true, prepopulator: proc { |_opts| self.lease_expiration_date = model.lease&.lease_expiration_date } property :visibility_after_lease, virtual: true, prepopulator: proc { |_opts| self.visibility_after_lease = model.lease&.visibility_after_lease } property :visibility_during_lease, virtual: true, prepopulator: proc { |_opts| self.visibility_during_lease = model.lease&.visibility_during_lease } diff --git a/app/indexers/hyrax/valkyrie_file_set_indexer.rb b/app/indexers/hyrax/valkyrie_file_set_indexer.rb index 30afec87e0..1e7fe68e30 100644 --- a/app/indexers/hyrax/valkyrie_file_set_indexer.rb +++ b/app/indexers/hyrax/valkyrie_file_set_indexer.rb @@ -22,6 +22,8 @@ def to_solr # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Met solr_doc['hasRelatedMediaFragment_ssim'] = resource.representative_id.to_s solr_doc['hasRelatedImage_ssim'] = resource.thumbnail_id.to_s + index_lease(solr_doc) + # Add in metadata from the original file. file_metadata = Hyrax::FileSetFileService.new(file_set: resource).original_file return solr_doc unless file_metadata @@ -116,5 +118,17 @@ def file_format(file) file.format_label end end + + def index_lease(doc) + if resource.lease&.active? + doc['lease_expiration_date_dtsi'] = resource.lease.lease_expiration_date&.to_datetime + doc['visibility_after_lease_ssim'] = resource.lease.visibility_after_lease + doc['visibility_during_lease_ssim'] = resource.lease.visibility_during_lease + else + doc['lease_history_ssim'] = resource&.lease&.lease_history + end + + doc + end end end diff --git a/app/indexers/hyrax/valkyrie_work_indexer.rb b/app/indexers/hyrax/valkyrie_work_indexer.rb index 7df5f4bb00..ef04954c3c 100644 --- a/app/indexers/hyrax/valkyrie_work_indexer.rb +++ b/app/indexers/hyrax/valkyrie_work_indexer.rb @@ -56,10 +56,12 @@ def index_embargo(doc) end def index_lease(doc) - if Hyrax::LeaseManager.new(resource: resource).under_lease? + if resource.lease&.active? doc['lease_expiration_date_dtsi'] = resource.lease.lease_expiration_date&.to_datetime doc['visibility_after_lease_ssim'] = resource.lease.visibility_after_lease doc['visibility_during_lease_ssim'] = resource.lease.visibility_during_lease + else + doc['lease_history_ssim'] = resource&.lease&.lease_history end doc diff --git a/app/models/hyrax/resource.rb b/app/models/hyrax/resource.rb index 652b8824f2..c409071914 100644 --- a/app/models/hyrax/resource.rb +++ b/app/models/hyrax/resource.rb @@ -36,7 +36,7 @@ class Resource < Valkyrie::Resource attribute :alternate_ids, Valkyrie::Types::Array.of(Valkyrie::Types::ID) attribute :embargo_id, Valkyrie::Types::ID - attribute :lease, Hyrax::Lease.optional + attribute :lease_id, Valkyrie::Types::ID delegate :edit_groups, :edit_groups=, :edit_users, :edit_users=, @@ -119,6 +119,18 @@ def embargo @embargo = Hyrax.query_service.find_by(id: embargo_id) if embargo_id.present? end + def lease=(value) + raise TypeError "can't convert #{value.class} into Hyrax::Lease" unless value.is_a? Hyrax::Lease + + @lease = value + self.lease_id = @lease.id + end + + def lease + return @lease if @lease + @lease = Hyrax.query_service.find_by(id: lease_id) if lease_id.present? + end + protected def visibility_writer diff --git a/app/services/hyrax/embargo_manager.rb b/app/services/hyrax/embargo_manager.rb index 5462064f0e..32a1d95596 100644 --- a/app/services/hyrax/embargo_manager.rb +++ b/app/services/hyrax/embargo_manager.rb @@ -126,13 +126,13 @@ def deactivate! embargo_record = embargo_history_message( embargo_state, Time.zone.today, - DateTime.parse(embargo.embargo_release_date), + embargo.embargo_release_date, embargo.visibility_during_embargo, embargo.visibility_after_embargo ) - nullify(force: true) release(force: true) + nullify(force: true) embargo.embargo_history += [embargo_record] end @@ -180,14 +180,17 @@ def embargo end ## - # Drop the embargo by setting its release date to `nil`. + # 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 # # @return [void] 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 end ## diff --git a/app/services/hyrax/lease_manager.rb b/app/services/hyrax/lease_manager.rb index c0fee709f4..d9f663a0ae 100644 --- a/app/services/hyrax/lease_manager.rb +++ b/app/services/hyrax/lease_manager.rb @@ -4,7 +4,22 @@ module Hyrax ## # Provides utilities for managing the lifecycle of an `Hyrax::Lease` on a # `Hyrax::Resource`. - class LeaseManager + # + # The lease terminology used here is as follows: + # + # - "Expiration Date" is the day a lease is scheduled to expire. + # - "Under Lease" means the lease is "active"; i.e. that its expiration + # date is today or later. + # - "Applied" means the lease's pre-expiration visibility has been set on + # the resource. + # - "Released" means the lease's post-expiration visibility has been set on + # the resource. + # - "Enforced" means the object's visibility matches the pre-expiration + # visibility of the lease; i.e. the lease has been applied, + # but not released. + # - "Deactivate" means that the existing lease will be removed + # + class LeaseManager # rubocop:disable Metrics/ClassLength ## # @!attribute [rw] resource # @return [Hyrax::Resource] @@ -47,6 +62,51 @@ def release_lease_for!(resource:, query_service: Hyrax.query_service) new(resource: resource, query_service: query_service) .release! end + + # Creates or updates an existing lease on a member to match the lease on the parent work + # @param [Array] members + # @param [Hyrax::Work] work + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength + def create_or_update_lease_on_members(members, work) + # TODO: account for all members and levels, not just file sets. ref: #6131 + + members.each do |member| + member_lease_needs_updating = work.lease.updated_at > member.lease&.updated_at if member.lease + + if member.lease && member_lease_needs_updating + member.lease.lease_expiration_date = work.lease['lease_expiration_date'] + member.lease.visibility_during_lease = work.lease['visibility_during_lease'] + member.lease.visibility_after_lease = work.lease['visibility_after_lease'] + member.lease = Hyrax.persister.save(resource: member.lease) + else + work_lease_manager = Hyrax::LeaseManager.new(resource: work) + work_lease_manager.copy_lease_to(target: member) + member = Hyrax.persister.save(resource: member) + end + + user ||= ::User.find_by_user_key(member.depositor) + # the line below works in that it indexes the file set with the necessary lease properties + # I do not know however if this is the best event_id to pass + Hyrax.publisher.publish('object.metadata.updated', object: member, user: user) + end + end + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength + end + + # Deactivates the lease and logs a message to the lease_history property + def deactivate! + lease_state = lease.active? ? 'active' : 'expired' + lease_record = lease_history_message( + lease_state, + Time.zone.today, + lease.lease_expiration_date, + lease.visibility_during_lease, + lease.visibility_after_lease + ) + + release(force: true) + nullify(force: true) + lease.lease_history += [lease_record] end ## @@ -58,7 +118,7 @@ def release_lease_for!(resource:, query_service: Hyrax.query_service) def copy_lease_to(target:) return false unless under_lease? - target.lease = Lease.new(clone_attributes) + target.lease = Hyrax.persister.save(resource: Lease.new(clone_attributes)) self.class.apply_lease_for(resource: target) end @@ -80,7 +140,8 @@ def apply! ## # @return [Boolean] def enforced? - lease.visibility_during_lease.to_s == resource.visibility + lease.lease_expiration_date.present? && + lease.visibility_during_lease.to_s == resource.visibility end ## @@ -90,18 +151,27 @@ def lease end ## - # Drop the lease by setting its release date to `nil`. + # Drop the lease by setting its release date and visibility settings to `nil`. # + # @param force [boolean] force the nullify even when the lease period is current # @return [void] - def nullify - return unless under_lease? + def nullify(force: false) + return false if !force && under_lease? + lease.lease_expiration_date = nil + lease.visibility_during_lease = nil + lease.visibility_after_lease = nil end ## + # Sets the visibility of the resource to the lease's after lease visibility. + # no-op if the lease period is current and the force flag is false. + # + # @param force [boolean] force the release even when the lease period is current + # # @return [Boolean] - def release - return false if under_lease? + def release(force: false) + return false if !force && under_lease? return true if lease.visibility_after_lease.nil? resource.visibility = lease.visibility_after_lease @@ -131,5 +201,15 @@ def clone_attributes def core_attribute_keys [:visibility_after_lease, :visibility_during_lease, :lease_expiration_date] end + + # Create the log message used when deactivating a lease + def lease_history_message(state, deactivate_date, expiration_date, visibility_during, visibility_after) + I18n.t 'hydra.lease.history_message', + state: state, + deactivate_date: deactivate_date, + expiration_date: expiration_date, + visibility_during: visibility_during, + visibility_after: visibility_after + end end end diff --git a/app/views/hyrax/base/_form_permission_lease.html.erb b/app/views/hyrax/base/_form_permission_lease.html.erb index 537bde1b8c..5c58989c4d 100644 --- a/app/views/hyrax/base/_form_permission_lease.html.erb +++ b/app/views/hyrax/base/_form_permission_lease.html.erb @@ -1,6 +1,6 @@
<%= visibility_badge(Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_LEASE) %> <%= f.input :visibility_during_lease, wrapper: :inline, collection: visibility_options(:loosen), include_blank: false %> - <%= f.input :lease_expiration_date, wrapper: :inline, input_html: { value: f.object.lease_expiration_date || Date.tomorrow, class: 'datepicker' } %> + <%= f.input :lease_expiration_date, wrapper: :inline, input_html: { value: f.object.lease_expiration_date&.to_date || Date.tomorrow, class: 'datepicker' } %> <%= f.input :visibility_after_lease, wrapper: :inline, collection: visibility_options(:restrict), include_blank: false %>
diff --git a/app/views/hyrax/base/_form_visibility_component.html.erb b/app/views/hyrax/base/_form_visibility_component.html.erb index a8726e0ad5..662932b3b5 100644 --- a/app/views/hyrax/base/_form_visibility_component.html.erb +++ b/app/views/hyrax/base/_form_visibility_component.html.erb @@ -39,7 +39,7 @@
<%= f.input :visibility_during_embargo, wrapper: :inline, collection: visibility_options(:restrict), include_blank: false %> <%= t('hyrax.works.form.visibility_until') %> - <%= f.date_field :embargo_release_date, wrapper: :inline, value: f.object.embargo_release_date || Date.tomorrow, class: 'datepicker form-control' %> + <%= f.date_field :embargo_release_date, wrapper: :inline, value: f.object.embargo_release_date&.to_date || Date.tomorrow, class: 'datepicker form-control' %> <%= f.input :visibility_after_embargo, wrapper: :inline, collection: visibility_options(:loosen), include_blank: false %>
@@ -55,7 +55,7 @@
<%= f.input :visibility_during_lease, wrapper: :inline, collection: visibility_options(:loosen), include_blank: false %> <%= t('hyrax.works.form.visibility_until') %> - <%= f.date_field :lease_expiration_date, wrapper: :inline, value: f.object.lease_expiration_date || Date.tomorrow, class: 'datepicker form-control' %> + <%= f.date_field :lease_expiration_date, wrapper: :inline, value: f.object.lease_expiration_date&.to_date || Date.tomorrow, class: 'datepicker form-control' %> <%= f.input :visibility_after_lease, wrapper: :inline, collection: visibility_options(:restrict), include_blank: false %>
diff --git a/lib/hyrax/transactions/steps/add_file_sets.rb b/lib/hyrax/transactions/steps/add_file_sets.rb index 4a22b4bb36..97a2ce7e25 100644 --- a/lib/hyrax/transactions/steps/add_file_sets.rb +++ b/lib/hyrax/transactions/steps/add_file_sets.rb @@ -25,6 +25,14 @@ def initialize(handler: Hyrax::WorkUploadsHandler) # @return [Dry::Monads::Result] def call(obj, uploaded_files: [], file_set_params: []) if @handler.new(work: obj).add(files: uploaded_files, file_set_params: file_set_params).attach + if obj.lease + file_sets = obj.member_ids.map do |member| + Hyrax.query_service.find_by(id: member) if Hyrax.query_service.find_by(id: member).is_a? Hyrax::FileSet + end + + Hyrax::LeaseManager.create_or_update_lease_on_members(file_sets, obj) + end + Success(obj) else Failure[:failed_to_attach_file_sets, uploaded_files] diff --git a/lib/hyrax/transactions/steps/save.rb b/lib/hyrax/transactions/steps/save.rb index 19b435881b..a68ea8092b 100644 --- a/lib/hyrax/transactions/steps/save.rb +++ b/lib/hyrax/transactions/steps/save.rb @@ -29,15 +29,15 @@ def initialize(persister: Hyrax.persister, publisher: Hyrax.publisher) # @return [Dry::Monads::Result] `Success(work)` if the change_set is # applied and the resource is saved; # `Failure([#to_s, change_set.resource])`, otherwise. - # rubocop:disable Metrics/MethodLength + # rubocop:disable Metrics/MethodLength, Metrics/AbcSize def call(change_set, user: nil) begin + valid_future_date?(change_set.lease, 'lease_expiration_date') if change_set.respond_to?(:lease) && change_set.lease + valid_future_date?(change_set.embargo, 'embargo_release_date') if change_set.respond_to?(:embargo) && change_set.embargo new_collections = changed_collection_membership(change_set) + unsaved = change_set.sync - if unsaved.embargo.present? - unsaved.embargo.embargo_release_date = unsaved.embargo.embargo_release_date&.to_datetime - unsaved.embargo = @persister.save(resource: unsaved.embargo) - end + save_lease_or_embargo(unsaved) saved = @persister.save(resource: unsaved) rescue StandardError => err return Failure(["Failed save on #{change_set}\n\t#{err.message}", change_set.resource]) @@ -53,10 +53,25 @@ def call(change_set, user: nil) publish_changes(resource: saved, user: user, new: unsaved.new_record, new_collections: new_collections) Success(saved) end - # rubocop:enable Metrics/MethodLength + # rubocop:enable Metrics/MethodLength, Metrics/AbcSize private + def valid_future_date?(item, attribute) + raise StandardError, "#{item.model} must use a future date" if item.fields[attribute] < Time.zone.now + end + + def save_lease_or_embargo(unsaved) + if unsaved.embargo.present? + unsaved.embargo.embargo_release_date = unsaved.embargo.embargo_release_date&.to_datetime + unsaved.embargo = @persister.save(resource: unsaved.embargo) + end + return if unsaved.lease.blank? + + unsaved.lease.lease_expiration_date = unsaved.lease.lease_expiration_date&.to_datetime + unsaved.lease = @persister.save(resource: unsaved.lease) + end + ## # @param [Hyrax::ChangeSet] change_set # diff --git a/lib/wings/active_fedora_converter.rb b/lib/wings/active_fedora_converter.rb index 31168bccd8..1734c0a24e 100644 --- a/lib/wings/active_fedora_converter.rb +++ b/lib/wings/active_fedora_converter.rb @@ -130,6 +130,7 @@ def normal_attributes ## # apply attributes to the ActiveFedora model + # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength def apply_attributes_to_model(af_object) case af_object when Hydra::AccessControl @@ -141,11 +142,20 @@ def apply_attributes_to_model(af_object) members = Array.wrap(converted_attrs.delete(:members)) files = converted_attrs.delete(:files) af_object.attributes = converted_attrs + if resource.try(:lease) && af_object.reflections.include?(:lease) + # TODO(#6134): af_object.lease.class has the same name as resource.lease.class; however, each class has a different object_id + # so a type mismatch happens. the code below coerces the one object into the other + unless af_object.lease&.id + resource_lease_dup = af_object.reflections.fetch(:lease).klass.new(resource.lease.attributes.except(:id, :internal_resource, :created_at, :updated_at, :new_record)) + af_object.lease = resource_lease_dup + end + end members.empty? ? af_object.try(:ordered_members)&.clear : af_object.try(:ordered_members=, members) af_object.try(:members)&.replace(members) af_object.files.build_or_set(files) if files end end + # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength # Add attributes from resource which aren't AF properties into af_object def add_access_control_attributes(af_object) diff --git a/lib/wings/model_transformer.rb b/lib/wings/model_transformer.rb index 091e644cf9..847fad228b 100644 --- a/lib/wings/model_transformer.rb +++ b/lib/wings/model_transformer.rb @@ -34,7 +34,7 @@ def initialize(pcdm_object:) # # @param pcdm_object [ActiveFedora::Base] # - # @return [::Valkyrie::Resource] a resource mirroiring `pcdm_object` + # @return [::Valkyrie::Resource] a resource mirroring `pcdm_object` def self.for(pcdm_object) new(pcdm_object: pcdm_object).build end @@ -43,6 +43,7 @@ def self.for(pcdm_object) # Builds a `Valkyrie::Resource` equivalent to the `pcdm_object` # # @return [::Valkyrie::Resource] a resource mirroring `pcdm_object` + # rubocop:disable Metrics/AbcSize def build klass = cache.fetch(pcdm_object.class) do OrmConverter.to_valkyrie_resource_class(klass: pcdm_object.class) @@ -53,7 +54,11 @@ def build attrs = attributes.tap { |hash| hash[:new_record] = pcdm_object.new_record? } attrs[:alternate_ids] = [::Valkyrie::ID.new(pcdm_object.id)] if pcdm_object.id - klass.new(**attrs).tap { |resource| ensure_current_permissions(resource) } + klass.new(**attrs).tap do |resource| + resource.lease = pcdm_object.lease&.valkyrie_resource if pcdm_object.respond_to?(:lease) && pcdm_object.lease + ensure_current_permissions(resource) + end + # rubocop:enable Metrics/AbcSize end def ensure_current_permissions(resource) @@ -155,7 +160,6 @@ def lock_token def append_embargo(attrs) return unless pcdm_object.try(:embargo) embargo_attrs = pcdm_object.embargo.attributes.symbolize_keys - embargo_attrs[:embargo_history] = embargo_attrs[:embargo_history].to_a embargo_attrs[:id] = ::Valkyrie::ID.new(embargo_attrs[:id]) if embargo_attrs[:id] attrs[:embargo] = Hyrax::Embargo.new(**embargo_attrs) @@ -164,7 +168,6 @@ def append_embargo(attrs) def append_lease(attrs) return unless pcdm_object.try(:lease) lease_attrs = pcdm_object.lease.attributes.symbolize_keys - lease_attrs[:lease_history] = lease_attrs[:embargo_history].to_a lease_attrs[:id] = ::Valkyrie::ID.new(lease_attrs[:id]) if lease_attrs[:id] attrs[:lease] = Hyrax::Lease.new(**lease_attrs) diff --git a/lib/wings/valkyrie/persister.rb b/lib/wings/valkyrie/persister.rb index 2f24ff2d32..e89bf08fdb 100644 --- a/lib/wings/valkyrie/persister.rb +++ b/lib/wings/valkyrie/persister.rb @@ -22,8 +22,9 @@ def initialize(adapter:) # Persists a resource using ActiveFedora # @param [Valkyrie::Resource] resource + # @param [Boolean] perform_af_validation # @return [Valkyrie::Resource] the persisted/updated resource - def save(resource:) + def save(resource:, perform_af_validation: false) af_object = resource_factory.from_resource(resource: resource) check_lock_tokens(af_object: af_object, resource: resource) @@ -31,7 +32,8 @@ def save(resource:) # the #save! api differs between ActiveFedora::Base and ActiveFedora::File objects, # if we get a falsey response, we expect we have a File that has failed to save due # to empty content - af_object.save! || + # we disable validation on the Active Fedora object, only if validations have already been run and passed + af_object.save!(validate: perform_af_validation) || raise(FailedSaveError.new("#{af_object.class}#save! returned non-true. It might be missing required content.", obj: af_object)) resource_factory.to_resource(object: af_object) diff --git a/spec/actors/hyrax/actors/collections_membership_actor_spec.rb b/spec/actors/hyrax/actors/collections_membership_actor_spec.rb index d4cf8a77d3..c00ddec51b 100644 --- a/spec/actors/hyrax/actors/collections_membership_actor_spec.rb +++ b/spec/actors/hyrax/actors/collections_membership_actor_spec.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true -RSpec.describe Hyrax::Actors::CollectionsMembershipActor, skip: !(Hyrax.config.collection_class < ActiveFedora::Base) do +RSpec.describe Hyrax::Actors::CollectionsMembershipActor, skip: (!(Hyrax.config.collection_class < ActiveFedora::Base) || Hyrax.config.use_valkyrie?) do let(:user) { create(:user) } let(:ability) { ::Ability.new(user) } let(:curation_concern) { build(:work, user: user) } diff --git a/spec/actors/hyrax/actors/interpret_visibility_actor_spec.rb b/spec/actors/hyrax/actors/interpret_visibility_actor_spec.rb index 992300602b..3f795ae62c 100644 --- a/spec/actors/hyrax/actors/interpret_visibility_actor_spec.rb +++ b/spec/actors/hyrax/actors/interpret_visibility_actor_spec.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true -RSpec.describe Hyrax::Actors::InterpretVisibilityActor do +RSpec.describe Hyrax::Actors::InterpretVisibilityActor, unless: Hyrax.config.use_valkyrie? do let(:user) { create(:user) } let(:ability) { ::Ability.new(user) } let(:curation_concern) { GenericWork.new } diff --git a/spec/actors/hyrax/actors/lease_actor_spec.rb b/spec/actors/hyrax/actors/lease_actor_spec.rb index e52cc3b6b2..0d3adf3237 100644 --- a/spec/actors/hyrax/actors/lease_actor_spec.rb +++ b/spec/actors/hyrax/actors/lease_actor_spec.rb @@ -7,8 +7,9 @@ describe "#destroy" do let(:work) do - FactoryBot.valkyrie_create(:hyrax_resource, :under_lease) + FactoryBot.valkyrie_create(:hyrax_resource, lease: lease) end + let(:lease) { FactoryBot.create(:hyrax_lease) } before do work.visibility = public_vis @@ -16,15 +17,23 @@ end it "removes the lease" do + actor.destroy + + expect(work.lease.lease_expiration_date).to eq nil + expect(work.lease.visibility_after_lease).to eq nil + expect(work.lease.visibility_during_lease).to eq nil + end + + it "releases the lease" do expect { actor.destroy } .to change { Hyrax::LeaseManager.new(resource: work).under_lease? } .from(true) .to false end - it "does not change the visibility" do + it "changes the visibility" do expect { actor.destroy } - .not_to change { work.visibility } + .to change { work.visibility } .from public_vis end @@ -33,17 +42,20 @@ FactoryBot.valkyrie_create(:hyrax_resource, lease: lease) end - let(:lease) { FactoryBot.build(:hyrax_lease) } + let(:lease) { FactoryBot.create(:hyrax_lease, :expired) } before do allow(Hyrax::TimeService) .to receive(:time_in_utc) - .and_return(work.lease.lease_expiration_date + 1) + .and_return(work.lease.lease_expiration_date.to_datetime + 1) end - it "leaves the lease in place" do - expect { actor.destroy } - .not_to change { work.lease.lease_expiration_date } + it "removes the lease" do + actor.destroy + + expect(work.lease.lease_expiration_date).to eq nil + expect(work.lease.visibility_after_lease).to eq nil + expect(work.lease.visibility_during_lease).to eq nil end it "releases the lease" do diff --git a/spec/controllers/hyrax/generic_works_controller_json_spec.rb b/spec/controllers/hyrax/generic_works_controller_json_spec.rb index 9ab2162b5d..7d27b0c867 100644 --- a/spec/controllers/hyrax/generic_works_controller_json_spec.rb +++ b/spec/controllers/hyrax/generic_works_controller_json_spec.rb @@ -55,9 +55,14 @@ # The clean is here because this test depends on the repo not having an AdminSet/PermissionTemplate created yet describe 'failed create', :clean_repo do before { post :create, params: { generic_work: {}, format: :json } } - it "returns 422 and the errors" do - created_resource = assigns[:curation_concern] - expect(response).to respond_unprocessable_entity(errors: created_resource.errors.messages.as_json) + + it 'returns 422 and the errors' do + # NOTE: this passes in all four 2.7/valkyrie and 3.2/valkyrie pipelines, but does not pass locally in koppie + # because Hyrax::WorksControllerBehavior#form_err_msg doesn't return the same type of message that + # Hyrax::WorksControllerBehavior#create does + resource = assigns[:curation_concern].respond_to?(:errors) ? assigns[:curation_concern] : assigns[:form] + expect(response.code).to eq '422' + expect(response).to respond_unprocessable_entity(errors: resource.errors.messages.as_json) end end diff --git a/spec/factories/hyrax_lease.rb b/spec/factories/hyrax_lease.rb index 39aea7aa30..f8cd7ceada 100644 --- a/spec/factories/hyrax_lease.rb +++ b/spec/factories/hyrax_lease.rb @@ -1,16 +1,18 @@ # frozen_string_literal: true FactoryBot.define do factory :hyrax_lease, class: "Hyrax::Lease" do - lease_expiration_date { Time.zone.today + 10 } + lease_expiration_date { (Time.zone.today + 10).to_datetime } visibility_after_lease { 'authenticated' } visibility_during_lease { 'open' } 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 - lease_expiration_date { Time.zone.today - 1 } + lease_expiration_date { (Time.zone.today - 2).to_datetime } end end end diff --git a/spec/factories/hyrax_resource.rb b/spec/factories/hyrax_resource.rb index 5605e54324..14f73c09c4 100644 --- a/spec/factories/hyrax_resource.rb +++ b/spec/factories/hyrax_resource.rb @@ -9,7 +9,7 @@ end trait :under_lease do - association :lease, factory: :hyrax_lease + lease_id { FactoryBot.create(:hyrax_lease).id } end end end diff --git a/spec/features/lease_spec.rb b/spec/features/lease_spec.rb index 42829c6965..c871e37b67 100644 --- a/spec/features/lease_spec.rb +++ b/spec/features/lease_spec.rb @@ -6,8 +6,8 @@ sign_in user end describe 'create a new leased object' do - let(:future_date) { 5.days.from_now } - let(:later_future_date) { 10.days.from_now } + let(:future_date) { Time.zone.today + 5 } + let(:later_future_date) { Time.zone.today + 10 } it 'can be created, displayed and updated', :clean_repo, :workflow do visit '/concern/generic_works/new' @@ -19,18 +19,18 @@ click_button 'Save' # chosen lease date is on the show page - expect(page).to have_content(future_date.to_date.to_formatted_s(:standard)) + expect(page).to have_content(future_date.to_formatted_s(:standard)) click_link 'Edit' click_link 'Lease Management Page' expect(page).to have_content('This Generic Work is under lease.') - expect(page).to have_xpath("//input[@name='generic_work[lease_expiration_date]' and @value='#{future_date.to_datetime.iso8601}']") # current lease date is pre-populated in edit field + expect(page).to have_xpath("//input[@name='generic_work[lease_expiration_date]' and @value='#{future_date}']") # current lease date is pre-populated in edit field fill_in 'until', with: later_future_date.to_s click_button 'Update Lease' - expect(page).to have_content(later_future_date.to_date.to_formatted_s(:standard)) # new lease date is displayed in message + expect(page).to have_content(later_future_date.to_formatted_s(:standard)) # new lease date is displayed in message click_link 'Edit' fill_in 'Title', with: 'Lease test CHANGED' diff --git a/spec/indexers/hyrax/valkyrie_file_set_indexer_spec.rb b/spec/indexers/hyrax/valkyrie_file_set_indexer_spec.rb index 2825a88a0a..5baccc7812 100644 --- a/spec/indexers/hyrax/valkyrie_file_set_indexer_spec.rb +++ b/spec/indexers/hyrax/valkyrie_file_set_indexer_spec.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true -RSpec.describe Hyrax::ValkyrieFileSetIndexer do +RSpec.describe Hyrax::ValkyrieFileSetIndexer, if: Hyrax.config.use_valkyrie? do include Hyrax::FactoryHelpers let(:fileset_id) { 'fs1' } @@ -239,6 +239,32 @@ expect(subject['original_file_id_ssi']).to eq "#{file_set.id}/files/#{file_set.original_file_id}" end end + + context 'with a valid lease' do + let(:lease) { FactoryBot.create(:hyrax_lease) } + + before { allow(file_set).to receive(:lease_id).and_return(lease.id) } + + it 'sets the lease expiration date and visibility settings' do + expect(subject['lease_expiration_date_dtsi']).to eq lease.lease_expiration_date + expect(subject['visibility_after_lease_ssim']).to eq lease.visibility_after_lease + expect(subject['visibility_during_lease_ssim']).to eq lease.visibility_during_lease + expect(subject['lease_history_ssim']).to be nil + end + end + + context 'with an expired lease' do + let(:lease) { FactoryBot.create(:hyrax_lease, :expired) } + + before { allow(file_set).to receive(:lease_id).and_return(lease.id) } + + it 'sets the lease expiration date and visibility settings' do + expect(subject['lease_expiration_date_dtsi']).to be nil + expect(subject['visibility_after_lease_ssim']).to be nil + expect(subject['visibility_during_lease_ssim']).to be nil + expect(subject['lease_history_ssim']).to eq lease.lease_history + end + end end describe '#file_format' do diff --git a/spec/models/hyrax/resource_spec.rb b/spec/models/hyrax/resource_spec.rb index 3621b9e2dc..6593739b93 100644 --- a/spec/models/hyrax/resource_spec.rb +++ b/spec/models/hyrax/resource_spec.rb @@ -33,7 +33,9 @@ subject(:resource) { described_class.new(lease: lease) } let(:lease) { FactoryBot.build(:hyrax_lease) } - it 'saves the lease' do + it 'saves the lease id' do + resource.lease = Hyrax.persister.save(resource: lease) + expect(Hyrax.persister.save(resource: resource).lease) .to have_attributes(lease_expiration_date: lease.lease_expiration_date) end diff --git a/spec/services/hyrax/admin_set_create_service_spec.rb b/spec/services/hyrax/admin_set_create_service_spec.rb index b96566e4b5..084471e2a5 100644 --- a/spec/services/hyrax/admin_set_create_service_spec.rb +++ b/spec/services/hyrax/admin_set_create_service_spec.rb @@ -291,9 +291,8 @@ context "when the admin_set is invalid" do let(:admin_set) { FactoryBot.build(:invalid_hyrax_admin_set) } # Missing title - it 'will not call the workflow_importer' do - expect { service.create! }.to raise_error(RuntimeError) - expect(workflow_importer).not_to have_received(:call) + it 'will not find the sipity workflow' do + expect { service.create! }.to raise_error(ActiveRecord::RecordNotFound) end end end diff --git a/spec/services/hyrax/lease_manager_spec.rb b/spec/services/hyrax/lease_manager_spec.rb index 2bb9ef8a3b..2ba61f0224 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.create(:hyrax_lease, :expired) } end describe '#apply' do @@ -90,7 +90,7 @@ expect(manager.lease) .to have_attributes visibility_after_lease: 'authenticated', visibility_during_lease: 'open', - lease_expiration_date: an_instance_of(Date), + lease_expiration_date: an_instance_of(DateTime), lease_history: be_empty end end diff --git a/spec/wings/active_fedora_converter_spec.rb b/spec/wings/active_fedora_converter_spec.rb index f96ee8b6c8..7d0c87d065 100644 --- a/spec/wings/active_fedora_converter_spec.rb +++ b/spec/wings/active_fedora_converter_spec.rb @@ -33,9 +33,17 @@ expect(work.errors.full_messages).to eq(converted_work.errors.full_messages) end end - context 'with an invalid FileSet object' do + + context 'with an invalid FileSet object', unless: Hyrax.config.use_valkyrie? do + # valkyrie backed resources do not respond to `valid?`. + let(:expired_lease) { Hydra::AccessControls::Lease.new(lease_expiration_date: (Time.zone.today - 2).to_datetime) } + let(:file_set) { ::FileSet.new } + + before do + allow(file_set).to receive(:lease).and_return(expired_lease) + end + it 'round trip converts to an FileSet object this is also invalid' do - file_set = ::FileSet.new(lease_expiration_date: 1.day.ago) expect(file_set).not_to be_valid converted_file_set = described_class.new(resource: file_set.valkyrie_resource).convert expect(converted_file_set).not_to be_valid