Refactor AdminSetCreateService to work with both AdminSet and AdministrativeSet

app/services/hyrax/admin_set_create_service.rb

@@ -1,12 +1,12 @@
 # frozen_string_literal: true
 module Hyrax
-  # Responsible for creating an AdminSet and its corresponding data:
+  # Responsible for creating a Hyrax::AdministrativeSet and its corresponding data.
   #
   # * An associated permission template
   # * Available workflows
   # * An active workflow
   #
-  # @see AdminSet
+  # @see Hyrax::AdministrativeSet
   # @see Hyrax::PermissionTemplate
   # @see Sipity::Workflow
   class AdminSetCreateService # rubocop:disable Metrics/ClassLength
@@ -18,28 +18,24 @@ class AdminSetCreateService # rubocop:disable Metrics/ClassLength
 
     class << self
       # @api public
-      # Creates the default AdminSet and corresponding data
+      # Creates the default Hyrax::AdministrativeSet and corresponding data
       # @param admin_set_id [String] The default admin set ID
       # @param title [Array<String>] The title of the default admin set
       # @return [TrueClass]
-      # @see AdminSet
+      # @see Hyrax::AdministrativeSet
       # @deprecated
+      # TODO: When this deprecated method is removed, update private method
+      #       .create_default_admin_set! to remove the parameters.
       def create_default_admin_set(admin_set_id: DEFAULT_ID, title: DEFAULT_TITLE)
         Deprecation.warn("'##{__method__}' will be removed in Hyrax 4.0.  " \
                          "Instead, use 'Hyrax::AdminSetCreateService.find_or_create_default_admin_set'.")
-        admin_set = AdminSet.new(id: admin_set_id, title: Array.wrap(title))
-        begin
-          new(admin_set: admin_set, creating_user: nil).create
-        rescue ActiveFedora::IllegalOperation
-          # It is possible that another thread created the AdminSet just before this method
-          # was called, so ActiveFedora will raise IllegalOperation. In this case we can safely
-          # ignore the error.
-          Rails.logger.error("AdminSet ID=#{AdminSet::DEFAULT_ID} may or may not have been created due to threading issues.")
-        end
+        create_default_admin_set!(admin_set_id: admin_set_id, title: title).present?
+      rescue RuntimeError => _err
+        false
       end
 
       # @api public
-      # Finds the default AdminSet if it exists; otherwise, creates it and corresponding data
+      # Finds the default AdministrativeSet if it exists; otherwise, creates it and corresponding data
       # @return [Hyrax::AdministrativeSet] The default admin set.
       # @see Hyrax::AdministrativeSet
       # @raise [RuntimeError] if admin set cannot be persisted
@@ -57,26 +53,43 @@ def default_admin_set?(id:)
       end
 
       # @api public
-      # Creates a non-default AdminSet and corresponding data
-      # @param admin_set [AdminSet] the admin set to operate on
+      # Creates a non-default Hyrax::AdministrativeSet and corresponding data
+      # @param admin_set [Hyrax::AdministrativeSet | AdminSet] the admin set to operate on
       # @param creating_user [User] the user who created the admin set
       # @return [TrueClass, FalseClass] true if it was successful
-      # @see AdminSet
+      # @see Hyrax::AdministrativeSet
       # @raise [RuntimeError] if you attempt to create a default admin set via this mechanism
       def call(admin_set:, creating_user:, **kwargs)
+        call!(admin_set: admin_set, creating_user: creating_user, **kwargs).present?
+      rescue RuntimeError => err
+        raise err if default_admin_set?(id: admin_set.id)
+        false
+      end
+
+      # @api public
+      # Creates a non-default Hyrax::AdministrativeSet and corresponding data
+      # @param admin_set [Hyrax::AdministrativeSet] the admin set to operate on
+      # @param creating_user [User] the user who created the admin set
+      # @return [Hyrax::AdministrativeSet] The fully created admin set.
+      # @see Hyrax::AdministrativeSet
+      # @raise [RuntimeError] if you attempt to create a default admin set via this mechanism
+      # @raise [RuntimeError] if admin set cannot be persisted
+      def call!(admin_set:, creating_user:, **kwargs)
         raise "Use .find_or_create_default_admin_set to create a default admin set" if default_admin_set?(id: admin_set.id)
-        new(admin_set: admin_set, creating_user: creating_user, **kwargs).create
+        new(admin_set: admin_set, creating_user: creating_user, **kwargs).create!
       end
 
       private
 
-      def create_default_admin_set!
-        create_default_admin_set
-        Hyrax.query_service.find_by(id: DEFAULT_ID)
+      # TODO: Parameters admin_set_id and title are defined to support .create_default_admin_set
+      #       which is deprecated.  When it is removed, the parameters will no longer be required.
+      def create_default_admin_set!(admin_set_id: DEFAULT_ID, title: DEFAULT_TITLE)
+        admin_set = Hyrax::AdministrativeSet.new(id: admin_set_id, title: Array.wrap(title))
+        new(admin_set: admin_set, creating_user: nil).create!
       end
     end
 
-    # @param admin_set [AdminSet] the admin set to operate on
+    # @param admin_set [Hyrax::AdministrativeSet | AdminSet] the admin set to operate on
     # @param creating_user [User] the user who created the admin set (if any).
     # @param workflow_importer [#call] imports the workflow
     def initialize(admin_set:, creating_user:, workflow_importer: default_workflow_importer)
@@ -90,8 +103,34 @@ def initialize(admin_set:, creating_user:, workflow_importer: default_workflow_i
     # Creates an admin set, setting the creator and the default access controls.
     # @return [TrueClass, FalseClass] true if it was successful
     def create
+      create!.persisted?
+    rescue RuntimeError => _err
+      false
+    end
+
+    # Creates an admin set, setting the creator and the default access controls.
+    # @return [Hyrax::AdministrativeSet] The fully created admin set.
+    # @raise [RuntimeError] if admin set cannot be persisted
+    def create!
+      admin_set.respond_to?(:valkyrie_resource) ? active_fedora_create! : valkyrie_create!
+    end
+
+    private
+
+    def default_admin_set?(id:)
+      self.class.default_admin_set?(id: id)
+    end
+
+    def admin_group_name
+      ::Ability.admin_group_name
+    end
+
+    # Creates an admin set, setting the creator and the default access controls.
+    # @return [Hyrax::AdministrativeSet] The fully created admin set.
+    # @raise [RuntimeError] if admin set cannot be persisted
+    def valkyrie_create!
       admin_set.creator = [creating_user.user_key] if creating_user
-      admin_set.save.tap do |result|
+      updated_admin_set = Hyrax.persister.save(resource: admin_set).tap do |result|
         if result
           ActiveRecord::Base.transaction do
             permission_template = permissions_create_service.create_default(collection: admin_set,
@@ -101,16 +140,27 @@ def create
           end
         end
       end
+      Hyrax.publisher.publish('collection.metadata.updated', collection: updated_admin_set, user: creating_user)
+      updated_admin_set
     end
 
-    private
-
-    def default_admin_set?(id:)
-      self.class.default_admin_set?(id: id)
-    end
-
-    def admin_group_name
-      ::Ability.admin_group_name
+    # Creates an admin set, setting the creator and the default access controls.
+    # @return [Hyrax::AdministrativeSet] The fully created admin set.
+    # @raise [RuntimeError] if admin set cannot be persisted
+    def active_fedora_create!
+      admin_set.creator = [creating_user.user_key] if creating_user
+      admin_set.save.tap do |result|
+        if result
+          ActiveRecord::Base.transaction do
+            permission_template = permissions_create_service.create_default(collection: admin_set,
+                                                                            creating_user: creating_user)
+            workflow = create_workflows_for(permission_template: permission_template)
+            create_default_access_for(permission_template: permission_template, workflow: workflow) if default_admin_set?(id: admin_set.id)
+          end
+        end
+      end
+      raise 'Admin set failed to persist.' unless admin_set.persisted?
+      admin_set.valkyrie_resource
     end
 
     def create_workflows_for(permission_template:)
@@ -145,7 +195,7 @@ def workflow_agents
       end
     end
 
-    # Gives deposit access to registered users to default AdminSet
+    # Give registered users deposit access to default admin set
     def create_default_access_for(permission_template:, workflow:)
       permission_template.access_grants.create(agent_type: 'group', agent_id: ::Ability.registered_group_name, access: Hyrax::PermissionTemplateAccess::DEPOSIT)
       deposit = Sipity::Role[Hyrax::RoleRegistry::DEPOSITING]

spec/services/hyrax/admin_set_create_service_spec.rb

@@ -1,38 +1,34 @@
 # frozen_string_literal: true
 RSpec.describe Hyrax::AdminSetCreateService do
-  let(:user) { create(:user) }
+  let(:user) { FactoryBot.create(:user) }
+  let(:persister) { Hyrax.persister }
+  let(:query_service) { Hyrax.query_service }
 
   describe '.create_default_admin_set', :clean_repo do
-    let(:admin_set) { AdminSet.find(AdminSet::DEFAULT_ID) }
-
-    # It is important to test the side-effects as a default admin set is a fundamental assumption for Hyrax.
-    it 'creates AdminSet, Hyrax::PermissionTemplate, Sipity::Workflow(s), and activates a Workflow', slow: true do
-      described_class.create_default_admin_set(admin_set_id: AdminSet::DEFAULT_ID, title: AdminSet::DEFAULT_TITLE)
-      expect(admin_set.permission_template).to be_persisted
-      expect(admin_set.active_workflow).to be_persisted
-      # 7 responsibilities because:
-      #  * 1 agent (admin group), multiplied by
-      #  * 2 available workflows, multiplied by
-      #  * 3 roles (from Hyrax::RoleRegistry), plus
-      #  * 1 depositing role for the registered group in the default workflow, equals
-      #  * 7
-      expect(Sipity::WorkflowResponsibility.count).to eq 7
-      expect(admin_set.read_groups).not_to include('public')
-      expect(admin_set.edit_groups).to eq ['admin']
-      # 2 access grants because:
-      #  * 1 providing deposit access to registered group
-      #  * 1 providing manage access to admin group
-      expect(admin_set.permission_template.access_grants.count).to eq 2
-      # Agents should be created for both the 'admin' and 'registered' groups
-      expect(Sipity::Agent.distinct.pluck(:proxy_for_id)).to include('admin', 'registered')
-      expect(Sipity::Agent.distinct.pluck(:proxy_for_type)).to include('Hyrax::Group')
+    subject(:status) { described_class.create_default_admin_set }
+
+    context "when new admin set persists" do
+      it "is a convenience method for .create_default_admin_set!" do
+        expect(described_class).to receive(:create_default_admin_set!).and_call_original
+        expect(status).to eq true
+      end
+    end
+
+    context "when new admin set fails to persist" do
+      before do
+        allow(persister).to receive(:save).with(resource: instance_of(Hyrax::AdministrativeSet))
+                                          .and_raise(RuntimeError)
+      end
+
+      it "returns false" do
+        expect(described_class).to receive(:create_default_admin_set!).and_call_original
+        expect(status).to eq false
+      end
     end
   end
 
   describe '.find_or_create_default_admin_set', :clean_repo do
-    let(:query_service) { Hyrax.query_service }
-    let(:persister) { Hyrax.persister }
-    let(:default_admin_set) { build(:default_hyrax_admin_set) }
+    let(:default_admin_set) { FactoryBot.build(:default_hyrax_admin_set) }
 
     subject(:admin_set) { described_class.find_or_create_default_admin_set }
 
@@ -41,8 +37,7 @@
         expect(query_service).to receive(:find_by).with(id: described_class::DEFAULT_ID)
                                                   .and_raise(Valkyrie::Persistence::ObjectNotFoundError)
         expect(described_class).to receive(:create_default_admin_set!).and_call_original
-        expect(query_service).to receive(:find_by).with(id: described_class::DEFAULT_ID)
-                                                  .and_return(default_admin_set)
+        expect(query_service).to receive(:find_by).with(id: anything).and_call_original # permission template
         expect(admin_set.title).to eq described_class::DEFAULT_TITLE
       end
     end
@@ -53,22 +48,22 @@
       it "returns existing default admin set" do
         expect(query_service).to receive(:find_by).with(id: described_class::DEFAULT_ID)
                                                   .and_return(default_admin_set)
-        expect(described_class).not_to receive(:create_default_admin_set)
+        expect(described_class).not_to receive(:create_default_admin_set!)
         expect(admin_set.title).to eq described_class::DEFAULT_TITLE
       end
     end
   end
 
   describe ".default_admin_set?" do
-    let(:admin_set) { build(:default_hyrax_admin_set) }
+    let(:admin_set) { FactoryBot.build(:default_hyrax_admin_set) }
     context "when admin_set is the default" do
       it "returns true" do
         expect(described_class.default_admin_set?(id: admin_set.id)).to eq true
       end
     end
 
     context "when admin_set isn't the default" do
-      let(:admin_set) { build(:hyrax_admin_set, title: ['test']) }
+      let(:admin_set) { FactoryBot.build(:hyrax_admin_set, title: ['test']) }
       it "returns false" do
         expect(described_class.default_admin_set?(id: admin_set.id)).to eq false
       end
@@ -78,20 +73,41 @@
   describe ".call" do
     subject { described_class.call(admin_set: admin_set, creating_user: user) }
 
-    let(:admin_set) { AdminSet.new(title: ['test']) }
+    let(:admin_set) { FactoryBot.build(:hyrax_admin_set, title: ['test']) }
 
     context "when using the default admin set", :clean_repo do
-      let(:admin_set) { AdminSet.new(id: AdminSet::DEFAULT_ID) }
+      let(:admin_set) { FactoryBot.build(:hyrax_admin_set, id: described_class::DEFAULT_ID) }
 
-      it 'will raise ActiveFedora::IllegalOperation if you attempt to a default admin set' do
+      it 'will raise RuntimeError if you attempt to a default admin set' do
         expect { subject }.to raise_error(RuntimeError)
       end
     end
 
-    it "is a convenience method for .new#create" do
+    it "is a convenience method for .new#create!" do
       service = instance_double(described_class)
       expect(described_class).to receive(:new).and_return(service)
-      expect(service).to receive(:create)
+      expect(service).to receive(:create!)
+      subject
+    end
+  end
+
+  describe ".call!" do
+    subject { described_class.call!(admin_set: admin_set, creating_user: user) }
+
+    let(:admin_set) { FactoryBot.build(:hyrax_admin_set, title: ['test']) }
+
+    context "when using the default admin set", :clean_repo do
+      let(:admin_set) { FactoryBot.build(:hyrax_admin_set, id: described_class::DEFAULT_ID) }
+
+      it 'will raise RuntimeError if you attempt to a default admin set' do
+        expect { subject }.to raise_error(RuntimeError)
+      end
+    end
+
+    it "is a convenience method for .new#create!" do
+      service = instance_double(described_class)
+      expect(described_class).to receive(:new).and_return(service)
+      expect(service).to receive(:create!)
       subject
     end
   end
@@ -100,14 +116,36 @@
     subject { service }
 
     let(:workflow_importer) { double(call: true) }
-    let(:admin_set) { AdminSet.new(title: ['test']) }
+    let(:admin_set) { FactoryBot.build(:hyrax_admin_set, title: ['test']) }
     let(:service) { described_class.new(admin_set: admin_set, creating_user: user, workflow_importer: workflow_importer) }
 
     its(:default_workflow_importer) { is_expected.to respond_to(:call) }
 
     describe "#create" do
       subject { service.create }
 
+      context "when the admin_set is valid" do
+        let(:updated_admin_set) { FactoryBot.valkyrie_create(:hyrax_admin_set) }
+
+        it "is a convenience method for #create! that returns true" do
+          expect(service).to receive(:create!).and_return(updated_admin_set)
+          expect(subject).to eq true
+        end
+      end
+
+      context "when the admin_set is invalid" do
+        it "is a convenience method for #create! that returns false" do
+          expect(service).to receive(:create!).and_raise(RuntimeError)
+          expect(subject).to eq false
+        end
+      end
+    end
+
+    describe "#create!" do
+      let(:admin_set) { AdminSet.new(title: ['test']) }
+
+      subject { service.create! }
+
       context "when the admin_set is valid" do
         let(:permission_template) { Hyrax::PermissionTemplate.find_by(source_id: admin_set.id) }
         let(:grants) { permission_template.access_grants }
@@ -121,10 +159,10 @@
         end
         # rubocop:enable RSpec/AnyInstance
 
-        it "creates an AdminSet, PermissionTemplate, Workflows, activates the default workflow, and sets access" do
+        it "creates an AdministrativeSet, PermissionTemplate, Workflows, activates the default workflow, and sets access" do
           expect(Sipity::Workflow).to receive(:activate!).with(permission_template: kind_of(Hyrax::PermissionTemplate), workflow_name: Hyrax.config.default_active_workflow_name)
           expect do
-            expect(subject).to be true
+            expect(subject).to be_kind_of Hyrax::AdministrativeSet
           end.to change { admin_set.persisted? }.from(false).to(true)
                                                 .and change { Sipity::WorkflowResponsibility.count }.by(12)
           # 12 responsibilities because:
@@ -148,11 +186,10 @@
       end
 
       context "when the admin_set is invalid" do
-        let(:admin_set) { AdminSet.new } # Missing title
+        let(:admin_set) { FactoryBot.build(:invalid_hyrax_admin_set) } # Missing title
 
-        it { is_expected.to be false }
         it 'will not call the workflow_importer' do
-          subject
+          expect { subject }.to raise_error(RuntimeError)
           expect(workflow_importer).not_to have_received(:call)
         end
       end