allow branding info to be processed with Hyrax::PcdmCollectionForm #5458

blancoj · 2022-02-23T03:24:47Z

This PR will allow the user to update the branding files: banner, and logo, for the Hyrax::PcdmCollectionForm.

In order to test this, you will need configure the collection model:

Edit /config/initializers/hyrax.rb and set:

config.collection_model = "Hyrax::PcdmCollection"

I want to point out that in order to get the Prepopulators to work, I had to add this line of code in the form method in collection_controller.rb. Perhaps there may be a better way to do this in the future.
form.prepopulate!

Also, in the update method of the collection_controller.rb file, I added a call to process_branding, and in there I check what model is being used. You don't want to do any processing in the method if the model is Pcdm, since that will be taken care by the ChangeSet and steps defined further down in that file. Not sure this is the best way to do this. :

def process_branding
  if Hyrax.config.collection_model.present? && Hyrax.config.collection_model == "Hyrax::PcdmCollection"
  elsif !params[:update_collection].nil?
    process_banner_input
    process_logo_input
  end
end

Guidance for testing, such as acceptance criteria or new user interface behaviors:
First
To isolate issues in other tabs, create a collection_type with all options off except Branding.

navigate to Dashboard -> Settings -> Collection Types
click button: Create new collection type
set Type name: Brandable only (or other meaningful name)
click button: Save
click tab: Settings
uncheck all but Branding (as seen below)
click button: Save changes

Then, you can test the uploading and creation of banner and logo:

navigate to Dashboard -> Collections
click button: New Collection
select type: Brandable only (or the name you used when creating the collection type)
click button: Create Collection
set Title: My Branded Collection (or other meaningful name)
Update a banner and logo. You can do a couple of logos if you'd like.
click Save

@samvera/hyrax-code-reviewers

elrayle

A few pre-final review comments.

elrayle · 2022-03-14T16:16:47Z

app/controllers/hyrax/dashboard/collections_controller.rb

@@ -234,10 +237,20 @@ def valkyrie_create
      end

      def valkyrie_update
+        # To handle the update of the Branding Tab, Edit collection page


I don't think this comment is needed. This method handles more than just the branding.

Agree. I put it in there as I was debugging.

elrayle · 2022-03-14T16:18:12Z

app/controllers/hyrax/dashboard/collections_controller.rb

@@ -498,7 +514,9 @@ def form
        @form ||=
          case @collection
          when Valkyrie::Resource
-            Hyrax::Forms::ResourceForm.for(@collection)
+            form = Hyrax::Forms::ResourceForm.for(@collection)
+            form.prepopulate!


I'll have to double check this, but I thought prepopulate! happened automatically.

lib/hyrax/transactions/steps/save_collection_banner.rb

elrayle · 2022-03-14T20:35:11Z

lib/hyrax/transactions/steps/save_collection_banner.rb

+        def process_banner_input(collection_id:, banner_unchanged_indicator:, update_banner_file_ids:)
+          return if banner_unchanged_indicator == "true"


This check of banner_unchanged_indicator here isn't needed anymore if the check is made in #call.

Suggested change

def process_banner_input(collection_id:, banner_unchanged_indicator:, update_banner_file_ids:)

return if banner_unchanged_indicator == "true"

def process_banner_input(collection_id:, update_banner_file_ids:)

elrayle

It really is nice having the processing of banner and logos, each in their own transaction step. Makes it easier to tell what is happening.

I have a few suggestions and questions, but overall, this looks really good. For some suggestions, you should be able to click the Commit suggestion button if the change looks good to you.

elrayle · 2022-04-12T14:30:36Z

app/controllers/hyrax/dashboard/collections_controller.rb

-          process_banner_input
-          process_logo_input
-        end
+        process_branding


This is a minor suggestion. If you move process_branding after the return, then you won't have to check the model in the process_branding method.

def update process_member_changes return valkyrie_update if @collection.is_a?(Valkyrie::Resource) process_branding ... # remainder of AF processing end

elrayle · 2022-04-12T14:32:58Z

app/controllers/hyrax/dashboard/collections_controller.rb

@@ -163,6 +160,12 @@ def update
        end
      end

+      def process_branding
+        return if Hyrax.config.collection_model == "Hyrax::PcdmCollection"


If you continue to need to check this here, it needs to be a more general to check if @collection.is_a?(Valkyrie::Resource). If you take the suggestion in the update method, then this check can be removed.

spec/controllers/hyrax/dashboard/collections_controller_spec.rb

elrayle · 2022-04-12T14:50:15Z

spec/forms/hyrax/forms/pcdm_collection_form_spec.rb

@@ -17,6 +17,41 @@
    end
  end

+  describe '#banner_info' do
+    let(:banner_info) do


This and the logo_info test are great to have.

spec/controllers/hyrax/dashboard/collections_controller_with_resource_spec.rb

lib/hyrax/transactions/steps/save_collection_logo.rb

elrayle · 2022-04-12T15:00:32Z

lib/hyrax/transactions/steps/save_collection_logo.rb

+          logo_info = CollectionBrandingInfo.where(collection_id: collection_id).where(role: "logo").where(local_path: uploaded_file_id.to_s)
+          logo_info.first.alt_text = alttext
+          logo_info.first.target_url = linkurl
+          logo_info.first.local_path = uploaded_file_id
+          logo_info.first.save(uploaded_file_id, false)


Would it work to set logo_info to the first result to avoid calling first multiple times?

Suggested change

logo_info = CollectionBrandingInfo.where(collection_id: collection_id).where(role: "logo").where(local_path: uploaded_file_id.to_s)

logo_info.first.alt_text = alttext

logo_info.first.target_url = linkurl

logo_info.first.local_path = uploaded_file_id

logo_info.first.save(uploaded_file_id, false)

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)

elrayle · 2022-04-12T15:11:12Z

lib/hyrax/transactions/steps/save_collection_banner.rb

+      # 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


This needs tests. You might be able to copy in the tests from the controller and make some minor adjustments for them to work for the transaction code.

elrayle · 2022-04-12T15:11:35Z

lib/hyrax/transactions/steps/save_collection_logo.rb

+      # 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


Same for this step. It needs tests. You might be able to copy in the tests from the controller and make some minor adjustments for them to work for the transaction code.

elrayle · 2022-05-03T13:15:52Z

spec/hyrax/transactions/steps/save_collection_logo_spec.rb

+    end
+
+    it 'does not save linkurl containing html; target_url is empty' do
+      expect(step.call(collection, update_logo_file_ids: [uploaded.id.to_s], alttext_values: ["Logo alt Text"], linkurl_values: ["<script>remove_me</script>"])).to be_success


Good to have the negative tests included too to catch edge cases.

elrayle

Thanks for pushing this all the way through, especially in time for Virtual Connect. All release tests pass for nurax-pg with this PR.

blancoj force-pushed the issue-5330 branch 3 times, most recently from 0e31bfd to e5897ef Compare March 2, 2022 03:01

elrayle reviewed Mar 21, 2022

View reviewed changes

blancoj force-pushed the issue-5330 branch from e5897ef to 6ecfeb0 Compare March 25, 2022 16:18

elrayle requested changes Apr 12, 2022

View reviewed changes

blancoj force-pushed the issue-5330 branch from c080890 to 6e48fd6 Compare April 27, 2022 17:50

elrayle mentioned this pull request Apr 29, 2022

Improve error handling for updating valkyrie collections #5610

Merged

blancoj force-pushed the issue-5330 branch 2 times, most recently from 7279983 to e48fb18 Compare May 2, 2022 19:10

allow branding info to be processed with Hyrax::PcdmCollectionForm

22f0d64

blancoj force-pushed the issue-5330 branch from e48fb18 to 22f0d64 Compare May 2, 2022 20:21

elrayle reviewed May 3, 2022

View reviewed changes

elrayle approved these changes May 3, 2022

View reviewed changes

elrayle merged commit 3ce78e7 into main May 3, 2022

elrayle deleted the issue-5330 branch May 3, 2022 14:29

dlpierce added the notes-bugfix Release Notes: Fixed a bug label Jun 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow branding info to be processed with Hyrax::PcdmCollectionForm #5458

allow branding info to be processed with Hyrax::PcdmCollectionForm #5458

blancoj commented Feb 23, 2022

elrayle left a comment

elrayle Mar 14, 2022

blancoj Mar 24, 2022

elrayle Mar 14, 2022

elrayle Mar 14, 2022

elrayle left a comment

elrayle Apr 12, 2022

elrayle Apr 12, 2022

elrayle Apr 12, 2022

elrayle Apr 12, 2022

elrayle Apr 12, 2022

elrayle Apr 12, 2022

elrayle May 3, 2022

elrayle left a comment

		def process_banner_input(collection_id:, banner_unchanged_indicator:, update_banner_file_ids:)
		return if banner_unchanged_indicator == "true"

allow branding info to be processed with Hyrax::PcdmCollectionForm #5458

allow branding info to be processed with Hyrax::PcdmCollectionForm #5458

Conversation

blancoj commented Feb 23, 2022

elrayle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elrayle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elrayle left a comment

Choose a reason for hiding this comment