allow branding info to be processed with Hyrax::PcdmCollectionForm

app/controllers/hyrax/dashboard/collections_controller.rb

@@ -127,6 +127,11 @@ def update
         end
       end
 
+      def process_branding
+        process_banner_input
+        process_logo_input
+      end
+
       def after_destroy(_id)
         # leaving id to avoid changing the method's parameters prior to release
         respond_to do |format|
@@ -209,12 +214,8 @@ def create_valkyrie_collection
       end
 
       def update_active_fedora_collection
-        unless params[:update_collection].nil?
-          process_banner_input
-          process_logo_input
-        end
-
         process_member_changes
+        process_branding
 
         @collection.visibility = Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PRIVATE unless @collection.discoverable?
         # we don't have to reindex the full graph when updating collection
@@ -230,16 +231,17 @@ def update_valkyrie_collection
         return after_update_errors(form_err_msg(form)) unless form.validate(collection_params)
 
         result = transactions['change_set.update_collection']
+                 .with_step_args(
+                          'collection_resource.save_collection_banner' => { update_banner_file_ids: params["banner_files"],
+                                                                            banner_unchanged_indicator: params["banner_unchanged"] },
+                          'collection_resource.save_collection_logo' => { update_logo_file_ids: params["logo_files"],
+                                                                          alttext_values: params["alttext"],
+                                                                          linkurl_values: params["linkurl"] }
+                        )
                  .call(form)
         @collection = result.value_or { return after_update_errors(result.failure.first) }
 
         process_member_changes
-
-        unless params[:update_collection].nil?
-          process_banner_input
-          process_logo_input
-        end
-
         after_update_response
       end
 
@@ -505,7 +507,9 @@ def form
         @form ||=
           case @collection
           when Valkyrie::Resource
-            Hyrax::Forms::ResourceForm.for(@collection)
+            form = Hyrax::Forms::ResourceForm.for(@collection)
+            form.prepopulate!
+            form
           else
             form_class.new(@collection, current_ability, repository)
           end

app/forms/hyrax/forms/pcdm_collection_form.rb

@@ -8,6 +8,31 @@ module Forms
     class PcdmCollectionForm < Valkyrie::ChangeSet # rubocop:disable Metrics/ClassLength
       include Hyrax::FormFields(:core_metadata)
 
+      BannerInfoPrepopulator = lambda do |_options|
+        self.banner_info ||= begin
+          banner_info = CollectionBrandingInfo.where(collection_id: id.to_s, role: "banner")
+          banner_file = File.split(banner_info.first.local_path).last unless banner_info.empty?
+          alttext = banner_info.first.alt_text unless banner_info.empty?
+          file_location = banner_info.first.local_path unless banner_info.empty?
+          relative_path = "/" + banner_info.first.local_path.split("/")[-4..-1].join("/") unless banner_info.empty?
+          { file: banner_file, full_path: file_location, relative_path: relative_path, alttext: alttext }
+        end
+      end
+
+      LogoInfoPrepopulator = lambda do |_options|
+        self.logo_info ||= begin
+          logos_info = CollectionBrandingInfo.where(collection_id: id.to_s, role: "logo")
+
+          logos_info.map do |logo_info|
+            logo_file = File.split(logo_info.local_path).last
+            relative_path = "/" + logo_info.local_path.split("/")[-4..-1].join("/")
+            alttext = logo_info.alt_text
+            linkurl = logo_info.target_url
+            { file: logo_file, full_path: logo_info.local_path, relative_path: relative_path, alttext: alttext, linkurl: linkurl }
+          end
+        end
+      end
+
       property :human_readable_type, writable: false
       property :date_modified, readable: false
       property :date_uploaded, readable: false
@@ -20,6 +45,9 @@ class PcdmCollectionForm < Valkyrie::ChangeSet # rubocop:disable Metrics/ClassLe
 
       validates :collection_type_gid, presence: true
 
+      property :banner_info, virtual: true, prepopulator: BannerInfoPrepopulator
+      property :logo_info, virtual: true, prepopulator: LogoInfoPrepopulator
+
       class << self
         def model_class
           Hyrax::PcdmCollection

app/views/hyrax/dashboard/collections/_form_branding.html.erb

@@ -34,6 +34,7 @@
     <div class="row branding-banner-list">
       <div class="col-xs-12">
         <div class="container">
+        <%# Where the request to display the branding comes from. %>
         <% if f.object.banner_info[:file] %>
           <div id="banner">
             <div class="row branding-banner-row">

lib/hyrax/transactions/collection_update.rb

@@ -9,6 +9,8 @@ module Transactions
     # @since 3.2.0
     class CollectionUpdate < Transaction
       DEFAULT_STEPS = ['change_set.apply',
+                       'collection_resource.save_collection_banner',
+                       'collection_resource.save_collection_logo',
                        'collection_resource.save_acl'].freeze
 
       ##

lib/hyrax/transactions/container.rb

@@ -42,6 +42,8 @@ class Container # rubocop:disable Metrics/ClassLength
       require 'hyrax/transactions/steps/remove_file_set_from_work'
       require 'hyrax/transactions/steps/save'
       require 'hyrax/transactions/steps/save_access_control'
+      require 'hyrax/transactions/steps/save_collection_banner'
+      require 'hyrax/transactions/steps/save_collection_logo'
       require 'hyrax/transactions/steps/set_default_admin_set'
       require 'hyrax/transactions/steps/set_modified_date'
       require 'hyrax/transactions/steps/set_uploaded_date_unless_present'
@@ -195,6 +197,14 @@ class Container # rubocop:disable Metrics/ClassLength
         ops.register 'save_acl' do
           Steps::SaveAccessControl.new
         end
+
+        ops.register 'save_collection_banner' do
+          Steps::SaveCollectionBanner.new
+        end
+
+        ops.register 'save_collection_logo' do
+          Steps::SaveCollectionLogo.new
+        end
       end
 
       namespace 'work_resource' do |ops| # Hyrax::Work resource

lib/hyrax/transactions/steps/save_collection_banner.rb

@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+module Hyrax
+  module Transactions
+    module Steps
+      ##
+      # Adds banner info via `ChangeSet`.
+      #
+      # During the update collection process this step is called to update the file
+      # to be used as a the banner for the collection.
+      #
+      class SaveCollectionBanner
+        include Dry::Transaction::Operation
+
+        ##
+        # @param [Hyrax::ChangeSet] change_set
+        # @param [Array<Integer>] update_banner_file_ids
+        # @param [Boolean] banner_unchanged_indicator
+        #
+        # @return [Dry::Monads::Result] `Failure` if the banner info fails to save;
+        #   `Success(input)`, otherwise.
+        def call(collection_resource, update_banner_file_ids: nil, banner_unchanged_indicator: true)
+          return Success(collection_resource) if ActiveModel::Type::Boolean.new.cast(banner_unchanged_indicator)
+          collection_id = collection_resource.id.to_s
+          process_banner_input(collection_id: collection_id, update_banner_file_ids: update_banner_file_ids)
+          Success(collection_resource)
+        end
+
+        private
+
+        def process_banner_input(collection_id:, update_banner_file_ids:)
+          remove_banner(collection_id: collection_id)
+          add_new_banner(collection_id: collection_id, uploaded_file_ids: update_banner_file_ids) if update_banner_file_ids
+        end
+
+        def remove_banner(collection_id:)
+          banner_info = CollectionBrandingInfo.where(collection_id: collection_id).where(role: "banner")
+          banner_info&.delete_all
+        end
+
+        def add_new_banner(collection_id:, uploaded_file_ids:)
+          f = uploaded_files(uploaded_file_ids).first
+          banner_info = CollectionBrandingInfo.new(
+            collection_id: collection_id,
+            filename: File.split(f.file_url).last,
+            role: "banner",
+            alt_txt: "",
+            target_url: ""
+          )
+          banner_info.save f.file_url
+        end
+
+        def uploaded_files(uploaded_file_ids)
+          return [] if uploaded_file_ids.empty?
+          UploadedFile.find(uploaded_file_ids)
+        end
+      end
+    end
+  end
+end

lib/hyrax/transactions/steps/save_collection_logo.rb

@@ -0,0 +1,109 @@
+# frozen_string_literal: true
+module Hyrax
+  module Transactions
+    module Steps
+      ##
+      # Adds logo info via `ChangeSet`.
+      #
+      # During the update collection process this step is called to update the file(s)
+      # to be used as logo(s) for the collection.
+      #
+      class SaveCollectionLogo
+        include Dry::Transaction::Operation
+
+        ##
+        # @param [Hyrax::ChangeSet] change_set
+        # @param [Array<#Integer>] update_logo_file_ids
+        # @param [Array<String>] alttext_values
+        # @param [Array<String>] linkurl_values
+        #
+        # @return [Dry::Monads::Result] `Failure` if the work fails to save;
+        #   `Success(input)`, otherwise.
+        def call(collection_resource, update_logo_file_ids: nil, alttext_values: nil, linkurl_values: nil)
+          collection_id = collection_resource.id.to_s
+          process_logo_input(collection_id: collection_id, update_logo_file_ids: update_logo_file_ids, alttext_values: alttext_values, linkurl_values: linkurl_values)
+          Success(collection_resource)
+        end
+
+        private
+
+        def process_logo_input(collection_id:, update_logo_file_ids:, alttext_values:, linkurl_values:)
+          uploaded_file_ids = update_logo_file_ids
+          public_files = []
+
+          if uploaded_file_ids.nil?
+            # all logo files were removed, so delete all files previously uploaded
+            remove_redundant_files(collection_id: collection_id, public_files: public_files)
+            return
+          end
+
+          public_files = process_logo_records(collection_id: collection_id, uploaded_file_ids: uploaded_file_ids, alttext_values: alttext_values, linkurl_values: linkurl_values)
+          remove_redundant_files(collection_id: collection_id, public_files: public_files)
+        end
+
+        def process_logo_records(collection_id:, uploaded_file_ids:, alttext_values:, linkurl_values:)
+          public_files = []
+          uploaded_file_ids.each_with_index do |ufi, i|
+            # If the user has chosen a new logo, the ufi will be an integer
+            # If the logo was previously chosen, the ufi will be a path
+            # If it is a path, update the rec, else create a new rec
+            if !ufi.match(/\D/).nil?
+              update_logo_info(collection_id: collection_id, uploaded_file_id: ufi, alttext: alttext_values[i], linkurl: verify_linkurl(linkurl_values[i]))
+              public_files << ufi
+            else # brand new one, insert in the database
+              logo_info = create_logo_info(collection_id: collection_id, uploaded_file_id: ufi, alttext: alttext_values[i], linkurl: verify_linkurl(linkurl_values[i]))
+              public_files << logo_info.local_path
+            end
+          end
+          public_files
+        end
+
+        def update_logo_info(collection_id:, uploaded_file_id:, alttext:, linkurl:)
+          logo_info = CollectionBrandingInfo.where(collection_id: collection_id).where(role: "logo").where(local_path: uploaded_file_id.to_s).first
+          logo_info.alt_text = alttext
+          logo_info.target_url = linkurl
+          logo_info.local_path = uploaded_file_id
+          logo_info.save(uploaded_file_id, false)
+        end
+
+        def create_logo_info(collection_id:, uploaded_file_id:, alttext:, linkurl:)
+          file = uploaded_files(uploaded_file_id)
+          logo_info = CollectionBrandingInfo.new(
+            collection_id: collection_id,
+            filename: File.split(file.file_url).last,
+            role: "logo",
+            alt_txt: alttext,
+            target_url: linkurl
+          )
+          logo_info.save file.file_url
+          logo_info
+        end
+
+        def uploaded_files(uploaded_file_ids)
+          return [] if uploaded_file_ids.empty?
+          UploadedFile.find(uploaded_file_ids)
+        end
+
+        def remove_redundant_files(collection_id:, public_files:)
+          # remove any public ones that were not included in the selection.
+          logos_info = CollectionBrandingInfo.where(collection_id: collection_id).where(role: "logo")
+          logos_info.each do |logo_info|
+            logo_info.delete(logo_info.local_path) unless public_files.include? logo_info.local_path
+            logo_info.destroy unless public_files.include? logo_info.local_path
+          end
+        end
+
+        # Only accept HTTP|HTTPS urls;
+        # @return <String> the url
+        def verify_linkurl(linkurl)
+          url = Loofah.scrub_fragment(linkurl, :prune).to_s
+          url if valid_url?(url)
+        end
+
+        def valid_url?(url)
+          (url =~ URI.regexp(['http', 'https']))
+        end
+      end
+    end
+  end
+end

spec/controllers/hyrax/dashboard/collections_controller_spec.rb

@@ -344,6 +344,17 @@
       end
     end
 
+    context "updating a collection's visibility" do
+      it "saves the visibility" do
+        expect { put :update, params: { id: collection, collection: { title: ['Moomin in Space'], visibility: 'restricted' } } }
+          .to change { Hyrax.query_service.find_by(id: collection.id).visibility }
+          .from('open')
+          .to('restricted')
+
+        expect(flash[:notice]).to eq "Collection was successfully updated."
+      end
+    end
+
     context "when update fails" do
       let(:collection) { FactoryBot.create(:collection_lw) }
       let(:repository) { instance_double(Blacklight::Solr::Repository, search: result) }
@@ -384,35 +395,22 @@
       let(:uploaded) { FactoryBot.create(:uploaded_file) }
 
       it "saves banner metadata" do
-        put :update, params: { id: collection,
-                               banner_files: [uploaded.id],
-                               collection: { creator: ['Emily'] },
-                               update_collection: true }
-
-        expect(CollectionBrandingInfo
-                 .where(collection_id: collection.id.to_s, role: "banner")
-                 .where("local_path LIKE '%#{uploaded.file.filename}'"))
-          .to exist
-      end
-
-      it "don't save banner metadata" do
         put :update, params: { id: collection,
                                banner_files: [uploaded.id],
                                collection: { creator: ['Emily'] } }
 
         expect(CollectionBrandingInfo
                  .where(collection_id: collection.id.to_s, role: "banner")
                  .where("local_path LIKE '%#{uploaded.file.filename}'"))
-          .not_to exist
+          .to exist
       end
 
       it "saves logo metadata" do
         put :update, params: { id: collection,
                                logo_files: [uploaded.id],
                                alttext: ["Logo alt Text"],
                                linkurl: ["http://abc.com"],
-                               collection: { creator: ['Emily'] },
-                               update_collection: true }
+                               collection: { creator: ['Emily'] } }
 
         expect(CollectionBrandingInfo
                  .where(collection_id: collection.id.to_s,
@@ -430,8 +428,7 @@
           put :update, params: { id: collection,
                                  logo_files: [uploaded.id],
                                  alttext: ["Logo alt Text"], linkurl: ["<script>remove_me</script>"],
-                                 collection: { creator: ['Emily'] },
-                                 update_collection: true }
+                                 collection: { creator: ['Emily'] } }
 
           expect(
             CollectionBrandingInfo.where(
@@ -446,8 +443,7 @@
                                  logo_files: [uploaded.id],
                                  alttext: ["Logo alt Text"],
                                  linkurl: ['javascript:alert("remove_me")'],
-                                 collection: { creator: ['Emily'] },
-                                 update_collection: true }
+                                 collection: { creator: ['Emily'] } }
 
           expect(
             CollectionBrandingInfo.where(