Always save nonpersisted ACLs (#6930)

* Always save nonpersisted ACLs

* Convert Ldp::Gone to ObjectNotFoundError

Ldp::Gone is the error raised by the fedora adapter when the resource is a tombstone after being deleted.

* Test using a real adapter

* Fix incorrect use of block with raise_error

This was causing any raised error to be considered a pass, which is clearly not the intent.

Fixing this revealed a problem caused by using Valkyrie::Resource directly. Required attributes
were missing because they are only defined when the Valkyrie::Resource is inherited.

* Fix specs that didn't expect an ACL save

* Skip when using Wings

Wings has its own custom query that is tested in spec/wings/services/custom_queries/find_access_control_spec.rb

* Compare acl by id

The ACL is resaved at some point resulting in a slight difference in updated_at

app/services/hyrax/access_control_list.rb

@@ -136,7 +136,7 @@ def grant(mode)
     #
     # @return [Boolean]
     def pending_changes?
-      change_set.changed?
+      !change_set.resource.persisted? || change_set.changed?
     end
 
     ##

app/services/hyrax/custom_queries/find_access_control.rb

@@ -20,7 +20,7 @@ def find_access_control_for(resource:)
           .find_inverse_references_by(resource: resource, property: :access_to)
           .find { |r| r.is_a?(Hyrax::AccessControl) } ||
           raise(Valkyrie::Persistence::ObjectNotFoundError)
-      rescue ArgumentError # some adapters raise ArgumentError for missing resources
+      rescue ArgumentError, Ldp::Gone # some adapters raise ArgumentError for missing resources
         raise(Valkyrie::Persistence::ObjectNotFoundError)
       end
     end

spec/services/hyrax/access_control_list_spec.rb

@@ -119,10 +119,21 @@
         .from be_empty
     end
 
-    it 'does not publish when permissions are unchanged' do
-      expect { acl.save }
-        .not_to change { listener.object_acl_updated }
-        .from nil
+    context 'when not persisted' do
+      it 'publishes when permissions are unchanged' do
+        expect { acl.save }
+          .to change { listener.object_acl_updated }
+          .from nil
+      end
+    end
+
+    context 'when persisted' do
+      before { acl.save }
+
+      it 'does not publish when permissions are unchanged' do
+        expect { acl.save }
+          .not_to change { listener.object_acl_updated }
+      end
     end
 
     context 'with additions' do

spec/services/hyrax/custom_queries/find_access_control_spec.rb

@@ -1,17 +1,28 @@
 # frozen_string_literal: true
-RSpec.describe Hyrax::CustomQueries::FindAccessControl do
+RSpec.describe Hyrax::CustomQueries::FindAccessControl, skip: !Hyrax.config.disable_wings do
   subject(:query_handler) { described_class.new(query_service: query_service) }
-  let(:adapter)           { Valkyrie::MetadataAdapter.find(:test_adapter) }
+  let(:adapter)           { Hyrax.metadata_adapter }
   let(:persister)         { adapter.persister }
   let(:query_service)     { adapter.query_service }
 
   describe '#find_access_control' do
     context 'for missing object' do
-      let(:resource) { Valkyrie::Resource.new }
+      let(:resource) { Hyrax::Resource.new }
 
       it 'raises ObjectNotFoundError' do
         expect { query_handler.find_access_control_for(resource: resource) }
-          .to raise_error { Valkyrie::Persistence::ObjectNotFoundError }
+          .to raise_error Valkyrie::Persistence::ObjectNotFoundError
+      end
+    end
+
+    context 'for deleted object' do
+      let(:resource) { persister.save(resource: Hyrax::Resource.new) }
+
+      before { persister.delete(resource: resource) }
+
+      it 'raises ObjectNotFoundError' do
+        expect { query_handler.find_access_control_for(resource: resource) }
+          .to raise_error Valkyrie::Persistence::ObjectNotFoundError
       end
     end
 
@@ -22,8 +33,8 @@
       before { acl } # ensure the acl gets saved
 
       it 'returns the acl' do
-        expect(query_handler.find_access_control_for(resource: resource))
-          .to eq acl
+        expect(query_handler.find_access_control_for(resource: resource).id)
+          .to eq acl.id
       end
     end
 
@@ -39,7 +50,7 @@
 
       it 'raises ObjectNotFoundError' do
         expect { query_handler.find_access_control_for(resource: resource) }
-          .to raise_error { Valkyrie::Persistence::ObjectNotFoundError }
+          .to raise_error Valkyrie::Persistence::ObjectNotFoundError
       end
     end
   end

spec/services/hyrax/listeners/file_metadata_listener_spec.rb

@@ -20,8 +20,8 @@
     it 'indexes the file_set' do
       expect { listener.on_file_metadata_updated(event) }
         .to change { fake_adapter.saved_resources }
-        .from(be_empty)
-        .to contain_exactly(file_set)
+        .from(contain_exactly(file_set)) # Saving the ACL triggers an index
+        .to(contain_exactly(file_set, file_set))
     end
 
     context 'when the file is not in a file set' do
@@ -53,7 +53,7 @@
       it 'does not index the file_set' do
         expect { listener.on_file_metadata_updated(event) }
           .not_to change { fake_adapter.saved_resources }
-          .from(be_empty)
+          .from(contain_exactly(file_set)) # Saving the ACL triggers an index
       end
     end
   end

spec/wings/services/custom_queries/find_access_control_spec.rb

@@ -13,11 +13,11 @@
 
   describe '#find_access_control' do
     context 'for missing object' do
-      let(:resource) { Valkyrie::Resource.new }
+      let(:resource) { Hyrax::Resource.new }
 
       it 'raises ObjectNotFoundError' do
         expect { query_handler.find_access_control_for(resource: resource) }
-          .to raise_error { Valkyrie::Persistence::ObjectNotFoundError }
+          .to raise_error Valkyrie::Persistence::ObjectNotFoundError
       end
     end