From 7437d2f79be5fa7427105a299cb0f4822e30eea2 Mon Sep 17 00:00:00 2001
From: tom johnson <tomjohnson@ucsb.edu>
Date: Wed, 29 Apr 2020 14:22:22 -0700
Subject: [PATCH 01/10] add `ResourceForm.model_class` for parity with
 Presenter/HydraEditor

---
 app/forms/hyrax/forms/resource_form.rb       | 12 ++++++++++++
 spec/forms/hyrax/forms/resource_form_spec.rb | 13 +++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/app/forms/hyrax/forms/resource_form.rb b/app/forms/hyrax/forms/resource_form.rb
index 7e95570122..4342f989af 100644
--- a/app/forms/hyrax/forms/resource_form.rb
+++ b/app/forms/hyrax/forms/resource_form.rb
@@ -13,6 +13,8 @@ module Forms
     #
     def self.ResourceForm(work_class)
       Class.new(Hyrax::Forms::ResourceForm) do
+        self.model_class = work_class
+
         (work_class.fields - work_class.reserved_attributes).each do |field|
           property field, default: nil
         end
@@ -24,6 +26,8 @@ def self.ResourceForm(work_class)
     #
     # This form wraps `Hyrax::ChangeSet` in the `HydraEditor::Form` interface.
     class ResourceForm < Hyrax::ChangeSet
+      class_attribute :model_class
+
       class << self
         ##
         # @api public
@@ -67,6 +71,14 @@ def required_fields=(fields)
       def []=(attr, value)
         public_send("#{attr}=".to_sym, value)
       end
+
+      ##
+      # @deprecated use model.class instead
+      #
+      # @return [Class]
+      def model_class # rubocop:disable Rails/Delegate
+        model.class
+      end
     end
   end
 end
diff --git a/spec/forms/hyrax/forms/resource_form_spec.rb b/spec/forms/hyrax/forms/resource_form_spec.rb
index 1ae43e24b4..75e59064ff 100644
--- a/spec/forms/hyrax/forms/resource_form_spec.rb
+++ b/spec/forms/hyrax/forms/resource_form_spec.rb
@@ -24,6 +24,13 @@
     end
   end
 
+  describe '.model_class' do
+    it 'is the class of the configured work' do
+      expect(Hyrax::Forms::ResourceForm(work.class).model_class)
+        .to eq work.class
+    end
+  end
+
   describe '#[]' do
     it 'supports access to work attributes' do
       expect(form[:title]).to eq work.title
@@ -44,6 +51,12 @@
     end
   end
 
+  describe '#model_class' do
+    it 'is the class of the model' do
+      expect(form.model_class).to eq work.class
+    end
+  end
+
   describe '#required?' do
     it 'is false for non-required fields' do
       expect(form.required?(:title)).to eq false

From ff43ebb05f9adc0f385eee7992e9a8acfa976fef Mon Sep 17 00:00:00 2001
From: tom johnson <tomjohnson@ucsb.edu>
Date: Mon, 4 May 2020 13:56:45 -0700
Subject: [PATCH 02/10] test work edit behavior for valkyrie models on
 controller

tests existing behavior on the controller side for the edit form. view tests are
still needed, and are the next step. probably, there are behavioral problems
there.
---
 .../hyrax/works_controller_behavior_spec.rb   | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb b/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb
index e27145d6ca..909d5b036c 100644
--- a/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb
+++ b/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb
@@ -68,6 +68,35 @@
         expect(response.status).to eq 401
       end
     end
+
+    context 'when the user has edit access' do
+      include_context 'with a logged in user'
+
+      before do
+        allow(controller.current_ability)
+          .to receive(:can?)
+          .with(any_args)
+          .and_return true
+      end
+
+      it 'is a success' do
+        get :edit, params: { id: work.id }
+
+        expect(response.status).to eq 200
+      end
+
+      it 'assigns a form with the current work as model' do
+        get :edit, params: { id: work.id }
+
+        expect(assigns[:form].model.id).to eq work.id
+      end
+
+      it 'renders the form' do
+        get :edit, params: { id: work.id }
+
+        expect(controller).to render_template('hyrax/base/edit')
+      end
+    end
   end
 
   describe '#new' do

From 8520352a614867ed79c585dc3fc9305c4b156573 Mon Sep 17 00:00:00 2001
From: tom johnson <tomjohnson@ucsb.edu>
Date: Tue, 5 May 2020 16:54:43 -0700
Subject: [PATCH 03/10] document some concerning issues about
 AdminSetOptionsPresenter's use

---
 app/presenters/hyrax/admin_set_options_presenter.rb | 5 +++++
 app/views/hyrax/base/_form_relationships.html.erb   | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/app/presenters/hyrax/admin_set_options_presenter.rb b/app/presenters/hyrax/admin_set_options_presenter.rb
index 7c7a796c12..3268beba0a 100644
--- a/app/presenters/hyrax/admin_set_options_presenter.rb
+++ b/app/presenters/hyrax/admin_set_options_presenter.rb
@@ -1,11 +1,16 @@
 module Hyrax
   # Presents the options for the AdminSet widget on the create/edit form
   class AdminSetOptionsPresenter
+    ##
+    # @param [Hyrax::AdminSetService] service
     def initialize(service)
       @service = service
     end
 
     # Return AdminSet selectbox options based on access type
+    #
+    # @todo this hits the Solr from the view. it would be better to avoid this.
+    #
     # @param [Symbol] access :deposit, :read, or :edit
     def select_options(access = :deposit)
       @service.search_results(access).map do |admin_set|
diff --git a/app/views/hyrax/base/_form_relationships.html.erb b/app/views/hyrax/base/_form_relationships.html.erb
index 0594059f94..5efc210459 100644
--- a/app/views/hyrax/base/_form_relationships.html.erb
+++ b/app/views/hyrax/base/_form_relationships.html.erb
@@ -1,5 +1,5 @@
 <% if Flipflop.assign_admin_set? %>
-  <%# TODO: consider `Hyrax::AdminSetOptionsPresenter.select_options_for(controller: controller)` instead %>
+  <%# TODO: avoid direct dependency on AdminSetService and AdminSetOptionsPresenter in the view!! make the controller provide the options! %>
   <%= f.input :admin_set_id, as: :select,
       include_blank: false,
       collection: Hyrax::AdminSetOptionsPresenter.new(Hyrax::AdminSetService.new(controller)).select_options,

From 051a2886f23e616e62f5925761ee38b86d6583c6 Mon Sep 17 00:00:00 2001
From: tom johnson <tomjohnson@ucsb.edu>
Date: Thu, 7 May 2020 18:11:36 -0700
Subject: [PATCH 04/10] extend embargo/lease helpers to support valkyrie and
 change sets

adds valkyrie work and ChangeSet/Hyrax::ResourceForm support for the
`embargo/lease_enforced?` helper methods. this generalizes certain view partials
to support a new form style.
---
 app/helpers/hyrax/embargo_helper.rb       | 10 +++-
 app/helpers/hyrax/lease_helper.rb         |  9 +++-
 app/services/hyrax/lease_manager.rb       | 15 ++++++
 spec/helpers/hyrax/embargo_helper_spec.rb | 60 +++++++++++++++++++++++
 spec/helpers/hyrax/lease_helper_spec.rb   | 58 ++++++++++++++++++++++
 5 files changed, 150 insertions(+), 2 deletions(-)

diff --git a/app/helpers/hyrax/embargo_helper.rb b/app/helpers/hyrax/embargo_helper.rb
index 7dee17dd02..5da5352508 100644
--- a/app/helpers/hyrax/embargo_helper.rb
+++ b/app/helpers/hyrax/embargo_helper.rb
@@ -20,7 +20,15 @@ def assets_with_deactivated_embargoes
     # @return [Boolean] whether the resource has an embargo that is currently
     #   enforced (regardless of whether it has expired)
     def embargo_enforced?(resource)
-      !resource.embargo_release_date.nil?
+      case resource
+      when Hydra::AccessControls::Embargoable
+        !resource.embargo_release_date.nil?
+      when Valkyrie::ChangeSet
+        Hyrax::EmbargoManager.new(resource: resource.model).enforced?
+
+      else
+        Hyrax::EmbargoManager.new(resource: resource).enforced?
+      end
     end
   end
 end
diff --git a/app/helpers/hyrax/lease_helper.rb b/app/helpers/hyrax/lease_helper.rb
index d95f385b77..f42a2d39d6 100644
--- a/app/helpers/hyrax/lease_helper.rb
+++ b/app/helpers/hyrax/lease_helper.rb
@@ -20,7 +20,14 @@ def assets_with_deactivated_leases
     # @return [Boolean] whether the resource has an lease that is currently
     #   enforced (regardless of whether it has expired)
     def lease_enforced?(resource)
-      !resource.lease_expiration_date.nil?
+      case resource
+      when Hydra::AccessControls::Embargoable
+        !resource.lease_expiration_date.nil?
+      when Valkyrie::ChangeSet
+        Hyrax::LeaseManager.new(resource: resource.model).enforced?
+      else
+        Hyrax::LeaseManager.new(resource: resource).enforced?
+      end
     end
   end
 end
diff --git a/app/services/hyrax/lease_manager.rb b/app/services/hyrax/lease_manager.rb
index 97fbf6c8e1..1b59f24e6b 100644
--- a/app/services/hyrax/lease_manager.rb
+++ b/app/services/hyrax/lease_manager.rb
@@ -28,10 +28,25 @@ def apply_lease_for(resource:, query_service: Hyrax.query_service)
           .apply
       end
 
+      def apply_lease_for!(resource:, query_service: Hyrax.query_service)
+        new(resource: resource, query_service: query_service)
+          .apply!
+      end
+
       def lease_for(resource:, query_service: Hyrax.query_service)
         new(resource: resource, query_service: query_service)
           .lease
       end
+
+      def release_lease_for(resource:, query_service: Hyrax.query_service)
+        new(resource: resource, query_service: query_service)
+          .release
+      end
+
+      def release_lease_for!(resource:, query_service: Hyrax.query_service)
+        new(resource: resource, query_service: query_service)
+          .release!
+      end
     end
 
     ##
diff --git a/spec/helpers/hyrax/embargo_helper_spec.rb b/spec/helpers/hyrax/embargo_helper_spec.rb
index b32ece3c38..107b62934b 100644
--- a/spec/helpers/hyrax/embargo_helper_spec.rb
+++ b/spec/helpers/hyrax/embargo_helper_spec.rb
@@ -4,6 +4,66 @@
   let(:resource) { build(:monograph) }
 
   describe 'embargo_enforced?' do
+    context 'with a Hyrax::Work' do
+      let(:resource) { build(:hyrax_work) }
+
+      it 'returns false' do
+        expect(embargo_enforced?(resource)).to be false
+      end
+
+      context 'when an embargo is enforced on the resource' do
+        let(:resource) { build(:hyrax_work, :under_embargo) }
+
+        before { Hyrax::EmbargoManager.apply_embargo_for!(resource: resource) }
+
+        it 'returns true' do
+          expect(embargo_enforced?(resource)).to be true
+        end
+
+        context 'and the embargo is expired' do
+          before do
+            resource.embargo.embargo_release_date = Time.zone.today - 1
+          end
+
+          it 'returns true' do
+            expect(embargo_enforced?(resource)).to be true
+          end
+        end
+      end
+    end
+
+    context 'with a change set' do
+      let(:resource) { Hyrax::ChangeSet.for(build(:hyrax_work)) }
+
+      it 'returns false' do
+        expect(embargo_enforced?(resource)).to be false
+      end
+
+      context 'when an embargo is enforced on the resource' do
+        let(:resource) do
+          Hyrax::ChangeSet.for(build(:hyrax_work, :under_embargo))
+        end
+
+        before do
+          Hyrax::EmbargoManager.apply_embargo_for!(resource: resource.model)
+        end
+
+        it 'returns true' do
+          expect(embargo_enforced?(resource)).to be true
+        end
+
+        context 'and the embargo is expired' do
+          before do
+            resource.model.embargo.embargo_release_date = Time.zone.today - 1
+          end
+
+          it 'returns true' do
+            expect(embargo_enforced?(resource)).to be true
+          end
+        end
+      end
+    end
+
     context 'with an ActiveFedora resource' do
       let(:resource) { build(:work) }
 
diff --git a/spec/helpers/hyrax/lease_helper_spec.rb b/spec/helpers/hyrax/lease_helper_spec.rb
index 53fdb25bd0..40d31f88a1 100644
--- a/spec/helpers/hyrax/lease_helper_spec.rb
+++ b/spec/helpers/hyrax/lease_helper_spec.rb
@@ -4,6 +4,64 @@
   let(:resource) { build(:monograph) }
 
   describe 'lease_enforced?' do
+    context 'with a Hyrax::Work' do
+      let(:resource) { build(:hyrax_work) }
+
+      it 'returns false' do
+        expect(lease_enforced?(resource)).to be false
+      end
+
+      context 'when a lease is enforced on the resource' do
+        let(:resource) { build(:hyrax_work, :under_lease) }
+
+        before { Hyrax::LeaseManager.apply_lease_for!(resource: resource) }
+
+        it 'returns true' do
+          expect(lease_enforced?(resource)).to be true
+        end
+
+        context 'and the lease is expired' do
+          before do
+            resource.lease.lease_expiration_date = Time.zone.today - 1
+          end
+
+          it 'returns true' do
+            expect(lease_enforced?(resource)).to be true
+          end
+        end
+      end
+    end
+
+    context 'with a change set' do
+      let(:resource) { Hyrax::ChangeSet.for(build(:hyrax_work)) }
+
+      it 'returns false' do
+        expect(lease_enforced?(resource)).to be false
+      end
+
+      context 'when a lease is enforced on the resource' do
+        let(:resource) { Hyrax::ChangeSet.for(build(:hyrax_work, :under_lease)) }
+
+        before do
+          Hyrax::LeaseManager.apply_lease_for!(resource: resource.model)
+        end
+
+        it 'returns true' do
+          expect(lease_enforced?(resource)).to be true
+        end
+
+        context 'and the lease is expired' do
+          before do
+            resource.model.lease.lease_expiration_date = Time.zone.today - 1
+          end
+
+          it 'returns true' do
+            expect(lease_enforced?(resource)).to be true
+          end
+        end
+      end
+    end
+
     context 'with an ActiveFedora resource' do
       let(:resource) { build(:work) }
 

From 434e2474b4201bffa7e0161279b720728251c5e0 Mon Sep 17 00:00:00 2001
From: tom johnson <tomjohnson@ucsb.edu>
Date: Mon, 11 May 2020 15:31:01 -0700
Subject: [PATCH 05/10] add handling for embargo/lease and agreements to the
 ChangeSet form

since the new valkyrie models don't have pass-through attributes for
embargo/lease settings, this form uses Reform's virtual properties to accept
this input from the existing views. when saving the form, it will be necessary
to interpret the virtual property states to determine appropriate changes to
embargoes and leases. the values are prepopulated appropriately when
`#prepopulate!` is called.

for `#agreeement_accepted` we use a similar approach, setting to `false` by
default, and to `true` when prepopulating with an existing model object.
---
 app/forms/hyrax/forms/resource_form.rb       |  27 +++
 spec/forms/hyrax/forms/resource_form_spec.rb | 165 ++++++++++++++++++-
 2 files changed, 190 insertions(+), 2 deletions(-)

diff --git a/app/forms/hyrax/forms/resource_form.rb b/app/forms/hyrax/forms/resource_form.rb
index 4342f989af..1bb08e7bc9 100644
--- a/app/forms/hyrax/forms/resource_form.rb
+++ b/app/forms/hyrax/forms/resource_form.rb
@@ -28,6 +28,21 @@ def self.ResourceForm(work_class)
     class ResourceForm < Hyrax::ChangeSet
       class_attribute :model_class
 
+      delegate :human_readable_type, to: :model
+
+      property :visibility # visibility has an accessor on the model
+
+      property :agreement_accepted, virtual: true, default: false, prepopulator: ->(_opts) { self.agreement_accepted = !model.new_record }
+
+      # virtual properties for embargo/lease;
+      property :embargo_release_date, virtual: true, prepopulator: ->(_opts) { self.embargo_release_date = embargo&.embargo_release_date }
+      property :visibility_after_embargo, virtual: true, prepopulator: ->(_opts) { self.visibility_after_embargo = embargo&.visibility_after_embargo }
+      property :visibility_during_embargo, virtual: true, prepopulator: ->(_opts) { self.visibility_during_embargo = embargo&.visibility_during_embargo }
+
+      property :lease_expiration_date, virtual: true,  prepopulator: ->(_opts) { self.lease_expiration_date = lease&.lease_expiration_date }
+      property :visibility_after_lease, virtual: true, prepopulator: ->(_opts) { self.visibility_after_lease = lease&.visibility_after_lease }
+      property :visibility_during_lease, virtual: true, prepopulator: ->(_opts) { self.visibility_during_lease = lease&.visibility_during_lease }
+
       class << self
         ##
         # @api public
@@ -79,6 +94,18 @@ def []=(attr, value)
       def model_class # rubocop:disable Rails/Delegate
         model.class
       end
+
+      def primary_terms
+        []
+      end
+
+      def secondary_terms
+        []
+      end
+
+      def display_additional_fields?
+        secondary_terms.any?
+      end
     end
   end
 end
diff --git a/spec/forms/hyrax/forms/resource_form_spec.rb b/spec/forms/hyrax/forms/resource_form_spec.rb
index 75e59064ff..5140a6d8b2 100644
--- a/spec/forms/hyrax/forms/resource_form_spec.rb
+++ b/spec/forms/hyrax/forms/resource_form_spec.rb
@@ -1,8 +1,8 @@
 # frozen_string_literal: true
 
 RSpec.describe Hyrax::Forms::ResourceForm do
-  subject(:form)   { described_class.for(work) }
-  let(:work)       { Hyrax::Work.new }
+  subject(:form) { described_class.for(work) }
+  let(:work)     { Hyrax::Work.new }
 
   describe '.required_fields=' do
     subject(:form) { form_class.new(work) }
@@ -51,12 +51,78 @@
     end
   end
 
+  describe '#agreement_accepted' do
+    it { is_expected.to have_attributes(agreement_accepted: false) }
+
+    it 'remains false when prepopulated' do
+      expect { form.prepopulate! }
+        .not_to change { form.agreement_accepted }
+        .from false
+    end
+
+    context 'when the work already exists' do
+      let(:work) { FactoryBot.valkyrie_create(:hyrax_work) }
+
+      it 'prepopulates to true' do
+        expect { form.prepopulate! }
+          .to change { form.agreement_accepted }
+          .to true
+      end
+    end
+  end
+
+  describe '#embargo_release_date' do
+    context 'without an embargo' do
+      it 'is nil' do
+        expect { form.prepopulate! }
+          .not_to change { form.embargo_release_date }
+          .from(nil)
+      end
+    end
+
+    context 'with a work under embargo' do
+      let(:work) { FactoryBot.build(:hyrax_work, :under_embargo) }
+
+      it 'defaults to the embargo date' do
+        expect { form.prepopulate! }
+          .to change { form.embargo_release_date }
+          .to(work.embargo.embargo_release_date)
+      end
+    end
+  end
+
+  describe '#lease_expiration_date' do
+    context 'without a lease' do
+      it 'is nil' do
+        expect { form.prepopulate! }
+          .not_to change { form.lease_expiration_date }
+          .from(nil)
+      end
+    end
+
+    context 'with a work under embargo' do
+      let(:work) { FactoryBot.build(:hyrax_work, :under_lease) }
+
+      it 'defaults to the embargo date' do
+        expect { form.prepopulate! }
+          .to change { form.lease_expiration_date }
+          .to(work.lease.lease_expiration_date)
+      end
+    end
+  end
+
   describe '#model_class' do
     it 'is the class of the model' do
       expect(form.model_class).to eq work.class
     end
   end
 
+  describe '#human_readable_type' do
+    it 'delegates to model' do
+      expect(form.human_readable_type).to eq work.human_readable_type
+    end
+  end
+
   describe '#required?' do
     it 'is false for non-required fields' do
       expect(form.required?(:title)).to eq false
@@ -80,4 +146,99 @@
       end
     end
   end
+
+  describe '#visibility' do
+    it 'can set visibility' do
+      form.visibility = 'open'
+
+      expect { form.sync }
+        .to change { work.permission_manager.acl.permissions }
+        .from(be_empty)
+        .to contain_exactly(have_attributes(mode: :read, agent: 'group/public'))
+    end
+  end
+
+  describe '#visibility_after_embargo' do
+    context 'without an embargo' do
+      it 'is nil' do
+        expect { form.prepopulate! }
+          .not_to change { form.visibility_after_embargo }
+          .from(nil)
+      end
+    end
+
+    context 'with a work under embargo' do
+      let(:work) { FactoryBot.build(:hyrax_work, :under_embargo) }
+
+      it 'defaults to the embargo visibility' do
+        expect { form.prepopulate! }
+          .to change { form.visibility_after_embargo }
+          .from(nil)
+          .to('open')
+      end
+    end
+  end
+
+  describe '#visibility_during_embargo' do
+    context 'without an embargo' do
+      it 'is nil' do
+        expect { form.prepopulate! }
+          .not_to change { form.visibility_during_embargo }
+          .from(nil)
+      end
+    end
+
+    context 'with a work under embargo' do
+      let(:work) { FactoryBot.build(:hyrax_work, :under_embargo) }
+
+      it 'defaults to the embargo visibility' do
+        expect { form.prepopulate! }
+          .to change { form.visibility_during_embargo }
+          .from(nil)
+          .to('authenticated')
+      end
+    end
+  end
+
+  describe '#visibility_after_lease' do
+    context 'without a lease' do
+      it 'is nil' do
+        expect { form.prepopulate! }
+          .not_to change { form.visibility_after_lease }
+          .from(nil)
+      end
+    end
+
+    context 'with a work under lease' do
+      let(:work) { FactoryBot.build(:hyrax_work, :under_lease) }
+
+      it 'defaults to the lease visibility' do
+        expect { form.prepopulate! }
+          .to change { form.visibility_after_lease }
+          .from(nil)
+          .to('authenticated')
+      end
+    end
+  end
+
+  describe '#visibility_during_lease' do
+    context 'without a lease' do
+      it 'is nil' do
+        expect { form.prepopulate! }
+          .not_to change { form.visibility_during_lease }
+          .from(nil)
+      end
+    end
+
+    context 'with a work under lease' do
+      let(:work) { FactoryBot.build(:hyrax_work, :under_lease) }
+
+      it 'defaults to the lease visibility' do
+        expect { form.prepopulate! }
+          .to change { form.visibility_during_lease }
+          .from(nil)
+          .to('open')
+      end
+    end
+  end
 end

From 99195146610d80eee535af27d4f374c907a93ed7 Mon Sep 17 00:00:00 2001
From: tom johnson <tomjohnson@ucsb.edu>
Date: Mon, 11 May 2020 18:42:39 -0700
Subject: [PATCH 06/10] add permission support to ChangeSet forms

adds support for permissions to change set style forms for use with valkyrie
objects.
---
 app/forms/hyrax/forms/resource_form.rb       | 14 +++++++++++++
 spec/forms/hyrax/forms/resource_form_spec.rb | 22 ++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/app/forms/hyrax/forms/resource_form.rb b/app/forms/hyrax/forms/resource_form.rb
index 1bb08e7bc9..fe3b8f6187 100644
--- a/app/forms/hyrax/forms/resource_form.rb
+++ b/app/forms/hyrax/forms/resource_form.rb
@@ -26,6 +26,18 @@ def self.ResourceForm(work_class)
     #
     # This form wraps `Hyrax::ChangeSet` in the `HydraEditor::Form` interface.
     class ResourceForm < Hyrax::ChangeSet
+      ##
+      # Nested form for permissions.
+      #
+      # @note due to historical oddities with Hydra::AccessControls and Hydra
+      #   Editor, Hyrax's views rely on `agent_name` and `access` as field
+      #   names. we provide these as virtual fields andprepopulate these from
+      #   `Hyrax::Permission`.
+      class Permission < Hyrax::ChangeSet
+        property :agent_name, virtual: true, prepopulator: ->(_opts) { self.agent_name = model.agent }
+        property :access, virtual: true, prepopulator: ->(_opts) { self.access = model.mode }
+      end
+
       class_attribute :model_class
 
       delegate :human_readable_type, to: :model
@@ -34,6 +46,8 @@ class ResourceForm < Hyrax::ChangeSet
 
       property :agreement_accepted, virtual: true, default: false, prepopulator: ->(_opts) { self.agreement_accepted = !model.new_record }
 
+      collection :permissions, virtual: true, default: [], form: Permission, prepopulator: ->(_opts) { self.permissions = Hyrax::AccessControl.for(resource: model).permissions }
+
       # virtual properties for embargo/lease;
       property :embargo_release_date, virtual: true, prepopulator: ->(_opts) { self.embargo_release_date = embargo&.embargo_release_date }
       property :visibility_after_embargo, virtual: true, prepopulator: ->(_opts) { self.visibility_after_embargo = embargo&.visibility_after_embargo }
diff --git a/spec/forms/hyrax/forms/resource_form_spec.rb b/spec/forms/hyrax/forms/resource_form_spec.rb
index 5140a6d8b2..38f59fe57e 100644
--- a/spec/forms/hyrax/forms/resource_form_spec.rb
+++ b/spec/forms/hyrax/forms/resource_form_spec.rb
@@ -117,6 +117,28 @@
     end
   end
 
+  describe '#permissions' do
+    it 'for a new object has empty permissions' do
+      expect(form.permissions).to be_empty
+    end
+
+    it 'for a new object prepopulates with empty permissions' do
+      expect { form.prepopulate! }
+        .not_to change { form.permissions }
+        .from(be_empty)
+    end
+
+    context 'with existing permissions' do
+      let(:work) { FactoryBot.valkyrie_create(:hyrax_work, :public) }
+
+      it 'prepopulates with the work permissions' do
+        expect { form.prepopulate! }
+          .to change { form.permissions }
+          .to contain_exactly(have_attributes(agent_name: 'group/public', access: :read))
+      end
+    end
+  end
+
   describe '#human_readable_type' do
     it 'delegates to model' do
       expect(form.human_readable_type).to eq work.human_readable_type

From 77a96f9e221e126642d2dcb526e2cbfc1dc6ded1 Mon Sep 17 00:00:00 2001
From: tom johnson <tomjohnson@ucsb.edu>
Date: Mon, 11 May 2020 19:14:10 -0700
Subject: [PATCH 07/10] test form views with ChangeSet style form
 objects--create

recent work has extended the ChangeSet style `Hyrax::ResourceForm` to support
most of the view behavior for work create forms. this adds tests of the relevant
form partials with the new form object.

the partial that handles collection membership is stubbed, since its behavior is
more complex and reproducing it in the new form should be considered separately.

likewise, tests for these partials as an edit form (for existing objects) are
left out; mainly because versions/optimistic locking becomes an issue in this
case. there's more groundwork to lay there.

to support this, a registration step is added to the new model generator. this
sets up the routes when we bulid the test app.
---
 .regen                                        |   2 +-
 .../work_resource/work_resource_generator.rb  |  24 +++
 spec/views/hyrax/base/_form.html.erb_spec.rb  | 154 ++++++++++++------
 3 files changed, 127 insertions(+), 53 deletions(-)

diff --git a/.regen b/.regen
index 60d3b2f4a4..b6a7d89c68 100644
--- a/.regen
+++ b/.regen
@@ -1 +1 @@
-15
+16
diff --git a/lib/generators/hyrax/work_resource/work_resource_generator.rb b/lib/generators/hyrax/work_resource/work_resource_generator.rb
index 249d8c737e..d94e3c5308 100644
--- a/lib/generators/hyrax/work_resource/work_resource_generator.rb
+++ b/lib/generators/hyrax/work_resource/work_resource_generator.rb
@@ -33,6 +33,23 @@ def create_model_spec
       rspec_installed?
   end
 
+  # Inserts after the last registered work, or at the top of the config block
+  def register_work
+    config = 'config/initializers/hyrax.rb'
+    lastmatch = nil
+    in_root do
+      File.open(config).each_line do |line|
+        lastmatch = line if line.match?(/config.register_curation_concern :(?!#{file_name})/)
+      end
+      content = "  # Injected via `rails g hyrax:work_resource #{class_name}`\n" \
+                "  config.register_curation_concern #{registration_path_symbol}\n"
+      anchor = lastmatch || "Hyrax.config do |config|\n"
+      inject_into_file config, after: anchor do
+        content
+      end
+    end
+  end
+
   def create_indexer
     template('indexer.rb.erb', File.join('app/indexers/', class_path, "#{file_name}_indexer.rb"))
   end
@@ -67,6 +84,13 @@ def rspec_installed?
       defined?(RSpec) && defined?(RSpec::Rails)
     end
 
+    def registration_path_symbol
+      return ":#{file_name}" if class_path.blank?
+      # creates a symbol with a path like "abc/scholarly_paper" where abc
+      # is the namespace and scholarly_paper is the resource name
+      ":\"#{File.join(class_path, file_name)}\""
+    end
+
     def revoking?
       behavior == :revoke
     end
diff --git a/spec/views/hyrax/base/_form.html.erb_spec.rb b/spec/views/hyrax/base/_form.html.erb_spec.rb
index b712457418..1dd8df2ae2 100644
--- a/spec/views/hyrax/base/_form.html.erb_spec.rb
+++ b/spec/views/hyrax/base/_form.html.erb_spec.rb
@@ -1,80 +1,130 @@
 RSpec.describe 'hyrax/base/_form.html.erb', type: :view do
-  let(:work) do
-    stub_model(GenericWork, id: '456')
-  end
-  let(:ability) { double }
-
-  let(:form) do
-    Hyrax::GenericWorkForm.new(work, ability, controller)
-  end
+  let(:controller_action) { 'new' }
+  let(:controller_class)  { Hyrax::MonographsController }
   let(:options_presenter) { double(select_options: []) }
 
   before do
+    # mock the admin set options presenter to avoid hitting Solr
     allow(Hyrax::AdminSetOptionsPresenter).to receive(:new).and_return(options_presenter)
-    stub_template('hyrax/base/_form_progress.html.erb' => 'Progress')
-    # TODO: stub_model is not stubbing new_record? correctly on ActiveFedora models.
-    allow(work).to receive(:new_record?).and_return(true)
-    allow(work).to receive(:member_ids).and_return([1, 2])
-    allow(view).to receive(:curation_concern).and_return(work)
     allow(controller).to receive(:current_user).and_return(stub_model(User))
     assign(:form, form)
-    allow(controller).to receive(:controller_name).and_return('batch_uploads')
-    allow(controller).to receive(:action_name).and_return('new')
-    allow(controller).to receive(:repository).and_return(Hyrax::GenericWorksController.new.repository)
-    allow(controller).to receive(:blacklight_config).and_return(Hyrax::GenericWorksController.new.blacklight_config)
-
-    allow(form).to receive(:permissions).and_return([])
-    allow(form).to receive(:visibility).and_return('public')
-    stub_template 'hyrax/base/_form_files.html.erb' => 'files'
+    allow(controller).to receive(:action_name).and_return(controller_action)
+    allow(controller).to receive(:repository).and_return(controller_class.new.repository)
+    allow(controller).to receive(:blacklight_config).and_return(controller_class.new.blacklight_config)
   end
 
-  context "for a new object" do
-    let(:work) { GenericWork.new }
+  context 'with a change_set style form' do
+    let(:form) { Hyrax::Forms::ResourceForm.for(work) }
+    let(:work) { build(:monograph, title: 'comet in moominland') }
 
-    context 'with batch_upload on' do
-      before do
-        allow(Flipflop).to receive(:batch_upload?).and_return(true)
+    before do
+      # TODO: unstub these when they are supported by the form
+      stub_template('hyrax/base/_form_member_of_collections.html.erb' => 'collection membership')
+    end
+
+    context 'for a new object' do
+      it 'renders a form' do
         render
+
+        expect(rendered).to have_selector("form[action='/concern/monographs']")
       end
-      it 'shows batch uploads' do
-        expect(rendered).to have_link('Batch upload', href: hyrax.new_batch_upload_path(payload_concern: 'GenericWork'))
-        expect(rendered).to have_selector("form[action='/concern/generic_works'][data-param-key='generic_work']")
-        # Draws the "Share" tab, with data for the javascript.
-        expect(rendered).to have_selector('#share[data-param-key="generic_work"]')
+
+      context 'with batch_upload off' do
+        before do
+          allow(Flipflop).to receive(:batch_upload?).and_return(false)
+        end
+
+        it 'hides batch uploads' do
+          render
+          expect(rendered).not_to have_link('Batch upload', href: hyrax.new_batch_upload_path(payload_concern: 'GenericWork'))
+        end
       end
     end
 
-    context 'with batch_upload off' do
-      before do
-        allow(Flipflop).to receive(:batch_upload?).and_return(false)
+    context 'with an existing object' do
+      let(:work) { FactoryBot.valkyrie_create(:monograph) }
+
+      xit 'renders a form' do
+        # pending handling for #version/optimistic locking, and maybe other
+        # issues. a good way to make progress on ChangeSet forms is to enable
+        # this test, analyze the stacktrace, and try to patch.
         render
-      end
-      it 'hides batch uploads' do
-        expect(rendered).not_to have_link('Batch upload', href: hyrax.new_batch_upload_path(payload_concern: 'GenericWork'))
+
+        expect(rendered).to have_selector("form[action='/concern/monographs/#{work.id}']")
       end
     end
   end
 
-  context "for a persisted object" do
-    let(:work) { stub_model(GenericWork, id: '456') }
+  context 'with a legacy GenericWork' do
+    let(:work) do
+      stub_model(GenericWork, id: '456')
+    end
+    let(:ability) { double }
+
+    let(:form) do
+      Hyrax::GenericWorkForm.new(work, ability, controller)
+    end
 
     before do
-      # Add an error to the work
-      work.errors.add :base, 'broken'
-      work.errors.add :visibility, 'visibility_error'
-      allow(form).to receive(:select_files).and_return([])
-      render
+      stub_template('hyrax/base/_form_progress.html.erb' => 'Progress')
+      # TODO: stub_model is not stubbing new_record? correctly on ActiveFedora models.
+      allow(work).to receive(:new_record?).and_return(true)
+      allow(work).to receive(:member_ids).and_return([1, 2])
+      allow(view).to receive(:curation_concern).and_return(work)
+      allow(controller).to receive(:controller_name).and_return('batch_uploads')
+      allow(form).to receive(:permissions).and_return([])
+      allow(form).to receive(:visibility).and_return('public')
+      stub_template 'hyrax/base/_form_files.html.erb' => 'files'
+    end
+
+    context "for a new object" do
+      let(:work) { GenericWork.new }
+
+      context 'with batch_upload on' do
+        before do
+          allow(Flipflop).to receive(:batch_upload?).and_return(true)
+          render
+        end
+        it 'shows batch uploads' do
+          expect(rendered).to have_link('Batch upload', href: hyrax.new_batch_upload_path(payload_concern: 'GenericWork'))
+          expect(rendered).to have_selector("form[action='/concern/generic_works'][data-param-key='generic_work']")
+          # Draws the "Share" tab, with data for the javascript.
+          expect(rendered).to have_selector('#share[data-param-key="generic_work"]')
+        end
+      end
+
+      context 'with batch_upload off' do
+        before do
+          allow(Flipflop).to receive(:batch_upload?).and_return(false)
+          render
+        end
+        it 'hides batch uploads' do
+          expect(rendered).not_to have_link('Batch upload', href: hyrax.new_batch_upload_path(payload_concern: 'GenericWork'))
+        end
+      end
     end
 
-    it "draws the page" do
-      expect(rendered).to have_selector("form[action='/concern/generic_works/456']")
-      expect(rendered).to have_selector("select#generic_work_resource_type", count: 1)
-      expect(rendered).to have_selector("select#generic_work_thumbnail_id", count: 1)
-      expect(rendered).to have_selector("select#generic_work_representative_id", count: 1)
+    context "for a persisted object" do
+      let(:work) { stub_model(GenericWork, id: '456') }
 
-      # It diplays form errors
-      expect(rendered).to have_content("broken")
-      expect(rendered).to have_content("visibility_error")
+      before do
+        # Add an error to the work
+        work.errors.add :base, 'broken'
+        work.errors.add :visibility, 'visibility_error'
+        allow(form).to receive(:select_files).and_return([])
+        render
+      end
+
+      it "draws the page" do
+        expect(rendered).to have_selector("form[action='/concern/generic_works/456']")
+        expect(rendered).to have_selector("select#generic_work_resource_type", count: 1)
+        expect(rendered).to have_selector("select#generic_work_thumbnail_id", count: 1)
+        expect(rendered).to have_selector("select#generic_work_representative_id", count: 1)
+
+        # It diplays form errors
+        expect(rendered).to have_content("broken")
+        expect(rendered).to have_content("visibility_error")
+      end
     end
   end
 end

From 19386c9d8fb522eb91ef036dc950d3f3339af348 Mon Sep 17 00:00:00 2001
From: tom johnson <tomjohnson@ucsb.edu>
Date: Tue, 12 May 2020 10:35:50 -0700
Subject: [PATCH 08/10] generate valkyrie `Monograph` into the test application
 last

Ensures `GenericWork` remains the "primary concern" for Hyrax, by keeping it at
the top of the registry.

We should really consider a better approach for determining the "primary
concern", and also reducing the amount code that depends on this dubious
concept.
---
 .regen                                                    | 2 +-
 .../lib/generators/test_app_generator.rb                  | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/.regen b/.regen
index b6a7d89c68..d6b24041cf 100644
--- a/.regen
+++ b/.regen
@@ -1 +1 @@
-16
+19
diff --git a/spec/test_app_templates/lib/generators/test_app_generator.rb b/spec/test_app_templates/lib/generators/test_app_generator.rb
index 1ac6fffd5e..feb2582364 100644
--- a/spec/test_app_templates/lib/generators/test_app_generator.rb
+++ b/spec/test_app_templates/lib/generators/test_app_generator.rb
@@ -13,10 +13,6 @@ def browse_everything_install
     generate "browse_everything:install --skip-assets"
   end
 
-  def create_work
-    generate 'hyrax:work_resource Monograph'
-  end
-
   def create_generic_work
     generate 'hyrax:work GenericWork'
   end
@@ -67,6 +63,10 @@ def new_record?
     end
   end
 
+  def create_work
+    generate 'hyrax:work_resource Monograph'
+  end
+
   def banner
     say_status("info", "ADDING OVERRIDES FOR TEST ENVIRONMENT", :blue)
   end

From 3e2464d4aa9cf36ea1bb891bb5810ea689ba666f Mon Sep 17 00:00:00 2001
From: tom johnson <tomjohnson@ucsb.edu>
Date: Tue, 12 May 2020 11:11:01 -0700
Subject: [PATCH 09/10] allow FilterByType to handle models without
 `#to_rdf_representation`

`FilterByType` used to rely on `.to_rdf_representation`, which is `ActiveFedora`
specific. the default implementation is just `.name` ("RDF representation" means
here "unique name referring to the model type", Class.name fits this bill fine).

Add support for Valkyrie works in this context by using `.name` as a
fallback. possibly we'll want a different name for *actual* RDF representations,
but since this is really just for the solr index, probably we can just use
`.name` forever for the new models.
---
 app/search_builders/hyrax/filter_by_type.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/search_builders/hyrax/filter_by_type.rb b/app/search_builders/hyrax/filter_by_type.rb
index db58d157a4..733bbe7149 100644
--- a/app/search_builders/hyrax/filter_by_type.rb
+++ b/app/search_builders/hyrax/filter_by_type.rb
@@ -29,7 +29,7 @@ def models
       end
 
       def models_to_solr_clause
-        models.map(&:to_rdf_representation).join(',')
+        models.map { |model| model.try(:to_rdf_representation) || model.name }.join(',')
       end
 
       def generic_type_field

From a609e409f0d27c2ae4cdfbb8979066ab57f86a4c Mon Sep 17 00:00:00 2001
From: tom johnson <tomjohnson@ucsb.edu>
Date: Tue, 12 May 2020 13:54:01 -0700
Subject: [PATCH 10/10] support HydraEditor::Form objects in
 EmbargoHelper#embargo_enforced?

extends `EmbargoHelper#embargo_enforced?` to handle the case when a form object
is explictly checked for an enforced embargo, instead of its model.

rather than counting on the presence of a delegated method, we call recursively
with `resource.model`. since `#model` is core to the `HydraEditor::Form`
interface, this should be very resiliant.
---
 app/helpers/hyrax/embargo_helper.rb       |  3 ++-
 app/helpers/hyrax/lease_helper.rb         |  2 ++
 spec/helpers/hyrax/embargo_helper_spec.rb | 18 ++++++++++++++++++
 spec/helpers/hyrax/lease_helper_spec.rb   | 18 ++++++++++++++++++
 4 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/app/helpers/hyrax/embargo_helper.rb b/app/helpers/hyrax/embargo_helper.rb
index 5da5352508..37c839cd45 100644
--- a/app/helpers/hyrax/embargo_helper.rb
+++ b/app/helpers/hyrax/embargo_helper.rb
@@ -23,9 +23,10 @@ def embargo_enforced?(resource)
       case resource
       when Hydra::AccessControls::Embargoable
         !resource.embargo_release_date.nil?
+      when HydraEditor::Form
+        embargo_enforced?(resource.model)
       when Valkyrie::ChangeSet
         Hyrax::EmbargoManager.new(resource: resource.model).enforced?
-
       else
         Hyrax::EmbargoManager.new(resource: resource).enforced?
       end
diff --git a/app/helpers/hyrax/lease_helper.rb b/app/helpers/hyrax/lease_helper.rb
index f42a2d39d6..0be8d06155 100644
--- a/app/helpers/hyrax/lease_helper.rb
+++ b/app/helpers/hyrax/lease_helper.rb
@@ -23,6 +23,8 @@ def lease_enforced?(resource)
       case resource
       when Hydra::AccessControls::Embargoable
         !resource.lease_expiration_date.nil?
+      when HydraEditor::Form
+        lease_enforced?(resource.model)
       when Valkyrie::ChangeSet
         Hyrax::LeaseManager.new(resource: resource.model).enforced?
       else
diff --git a/spec/helpers/hyrax/embargo_helper_spec.rb b/spec/helpers/hyrax/embargo_helper_spec.rb
index 107b62934b..3a32c86e3c 100644
--- a/spec/helpers/hyrax/embargo_helper_spec.rb
+++ b/spec/helpers/hyrax/embargo_helper_spec.rb
@@ -92,5 +92,23 @@
         end
       end
     end
+
+    context 'with a HydraEditor::Form' do
+      let(:resource) { Hyrax::GenericWorkForm.new(build(:work), ability, form_controller) }
+      let(:ability) { :FAKE_ABILITY }
+      let(:form_controller) { :FAKE_CONTROLLER }
+
+      it 'returns false' do
+        expect(embargo_enforced?(resource)).to be false
+      end
+
+      context 'when the wrapped work is under embargo' do
+        let(:resource) { Hyrax::GenericWorkForm.new(build(:embargoed_work), ability, form_controller) }
+
+        it 'returns true' do
+          expect(embargo_enforced?(resource)).to be true
+        end
+      end
+    end
   end
 end
diff --git a/spec/helpers/hyrax/lease_helper_spec.rb b/spec/helpers/hyrax/lease_helper_spec.rb
index 40d31f88a1..cdf1c2ff18 100644
--- a/spec/helpers/hyrax/lease_helper_spec.rb
+++ b/spec/helpers/hyrax/lease_helper_spec.rb
@@ -90,5 +90,23 @@
         end
       end
     end
+
+    context 'with a HydraEditor::Form' do
+      let(:resource) { Hyrax::GenericWorkForm.new(build(:work), ability, form_controller) }
+      let(:ability) { :FAKE_ABILITY }
+      let(:form_controller) { :FAKE_CONTROLLER }
+
+      it 'returns false' do
+        expect(lease_enforced?(resource)).to be false
+      end
+
+      context 'when the wrapped work is under lease' do
+        let(:resource) { Hyrax::GenericWorkForm.new(build(:leased_work), ability, form_controller) }
+
+        it 'returns true' do
+          expect(lease_enforced?(resource)).to be true
+        end
+      end
+    end
   end
 end