From 200ed6a844dc69f855c16995d5ab9510cdb03bf8 Mon Sep 17 00:00:00 2001
From: tamsin johnson <tomjohnson@ucsb.edu>
Date: Thu, 31 Aug 2023 16:53:37 -0700
Subject: [PATCH 1/6] port most FileSetPresenter specs to Valkyrie

this passes most FileSetPresenter specs, but reveals a potential issue that i
haven't yet been able to determine the correct behavior about.

it changes the presenter's #to_partial_path from `"file_sets/file_set"` to
`"hyrax/file_sets/file_set`". the behavior in the original `::FileSet` is
provided by `Hyrax::Naming`.

`PcdmCollection` uses a custom name class `Hyrax::CollectionName` to ensure
ActiveModel naming behavior is compatible with `::Collection`, and it seems
natural to assume `Hyrax::FileSet` should do similarly. BUT, why is this only
coming up now? `.koppie` and `nurax-pg` are passing many automated and manual
tests using the current `#model_name` implementation. Why?

maybe it's okay to just change the test and allow that the partial path change
for Valkyrie native apps? what's going on here?
---
 spec/factories/hyrax_file_metadata.rb         |   6 +-
 spec/factories/hyrax_file_set.rb              |   4 +-
 spec/factories/uploaded_files.rb              |   4 +
 .../hyrax/file_set_presenter_spec.rb          | 289 +++++++++---------
 4 files changed, 154 insertions(+), 149 deletions(-)

diff --git a/spec/factories/hyrax_file_metadata.rb b/spec/factories/hyrax_file_metadata.rb
index 5d824e7368..cf6762ca85 100644
--- a/spec/factories/hyrax_file_metadata.rb
+++ b/spec/factories/hyrax_file_metadata.rb
@@ -3,7 +3,7 @@
 ##
 # Use this factory for FileMetadata for Files in valkyrie.
 FactoryBot.define do
-  factory :hyrax_file_metadata, class: 'Hyrax::FileMetadata' do
+  factory :hyrax_file_metadata, class: 'Hyrax::FileMetadata', aliases: [:file_metadata] do
     transient do
       use { nil }
       visibility_setting { nil }
@@ -32,6 +32,10 @@
       end
     end
 
+    trait :original_file do
+      use { Hyrax::FileMetadata::Use.uri_for(use: :original_file) }
+    end
+
     trait :image do
       mime_type { 'image/png' }
     end
diff --git a/spec/factories/hyrax_file_set.rb b/spec/factories/hyrax_file_set.rb
index 52046801e8..5ac4a9d4c9 100644
--- a/spec/factories/hyrax_file_set.rb
+++ b/spec/factories/hyrax_file_set.rb
@@ -31,7 +31,7 @@
       file_set.permission_manager.edit_groups = evaluator.edit_groups
       file_set.permission_manager.edit_users  = evaluator.edit_users
       file_set.permission_manager.read_users  = evaluator.read_users
-      file_set.permission_manager.read_users  = evaluator.read_groups
+      file_set.permission_manager.read_groups = evaluator.read_groups
     end
 
     after(:create) do |file_set, evaluator|
@@ -44,7 +44,7 @@
       file_set.permission_manager.edit_groups = evaluator.edit_groups
       file_set.permission_manager.edit_users  = evaluator.edit_users
       file_set.permission_manager.read_users  = evaluator.read_users
-      file_set.permission_manager.read_users  = evaluator.read_groups
+      file_set.permission_manager.read_groups = evaluator.read_groups
 
       file_set.permission_manager.acl.save
 
diff --git a/spec/factories/uploaded_files.rb b/spec/factories/uploaded_files.rb
index ac56049bf7..df5694e210 100644
--- a/spec/factories/uploaded_files.rb
+++ b/spec/factories/uploaded_files.rb
@@ -3,5 +3,9 @@
   factory :uploaded_file, class: Hyrax::UploadedFile do
     user
     file { File.open('spec/fixtures/image.jp2') }
+
+    trait :audio do
+      file { File.open('spec/fixtures/sample_mpeg4.mp4') }
+    end
   end
 end
diff --git a/spec/presenters/hyrax/file_set_presenter_spec.rb b/spec/presenters/hyrax/file_set_presenter_spec.rb
index 5cfad6422b..cc1fcafc12 100644
--- a/spec/presenters/hyrax/file_set_presenter_spec.rb
+++ b/spec/presenters/hyrax/file_set_presenter_spec.rb
@@ -1,87 +1,76 @@
 # frozen_string_literal: true
 require 'iiif_manifest'
 
-# rubocop:disable RSpec/SubjectStub
 RSpec.describe Hyrax::FileSetPresenter do
   subject(:presenter) { described_class.new(solr_document, ability) }
   let(:solr_document) { SolrDocument.new(attributes) }
   let(:ability) { Ability.new(user) }
-  let(:attributes) { file.to_solr }
-
-  let(:file) do
-    build(:file_set,
-          id: '123abc',
-          user: user,
-          title: ["File title"],
-          depositor: user.user_key,
-          label: "filename.tif")
+  let(:attributes) { Hyrax::ValkyrieIndexer.for(resource: file_set).to_solr }
+  let(:user) { FactoryBot.create(:admin) }
+
+  let(:file_set) do
+    FactoryBot.valkyrie_create(:hyrax_file_set,
+                               depositor: user.user_key,
+                               edit_users: [user],
+                               label: "filename.tif",
+                               title: ["File title"])
   end
-  let(:user) { create(:admin) }
 
   describe 'stats_path' do
-    before do
-      # https://github.com/samvera/active_fedora/issues/1251
-      allow(file).to receive(:persisted?).and_return(true)
-    end
-    it { expect(presenter.stats_path).to eq Hyrax::Engine.routes.url_helpers.stats_file_path(id: file, locale: 'en') }
+    its(:stats_path) { is_expected.to eq Hyrax::Engine.routes.url_helpers.stats_file_path(id: file_set, locale: 'en') }
   end
 
   describe "#to_s" do
-    subject { presenter.to_s }
-
-    it { is_expected.to eq 'File title' }
+    its(:to_s) { is_expected.to eq 'File title' }
   end
 
   describe "#human_readable_type" do
-    subject { presenter.human_readable_type }
-
-    it { is_expected.to eq 'File' }
+    its(:human_readable_type) { is_expected.to eq 'File Set' }
   end
 
   describe "#model_name" do
-    subject { presenter.model_name }
-
-    it { is_expected.to be_kind_of ActiveModel::Name }
+    it do
+      name = subject.model_name
+      require 'pry'; binding.pry
+    end
+    its(:model_name) { is_expected.to be_kind_of ActiveModel::Name }
   end
 
   describe "#to_partial_path" do
-    subject { presenter.to_partial_path }
-
-    it { is_expected.to eq 'file_sets/file_set' }
+    its(:to_partial_path) { is_expected.to eq 'file_sets/file_set' }
   end
 
   describe "office_document?" do
-    subject { presenter.office_document? }
-
-    it { is_expected.to be false }
+    it { is_expected.not_to be_office_document }
   end
 
   describe "#user_can_perform_any_action?" do
-    subject { presenter.user_can_perform_any_action? }
     let(:current_ability) { ability }
 
     it 'is deprecated' do
       expect(Deprecation).to receive(:warn)
-      subject
+
+      presenter.user_can_perform_any_action?
     end
 
     context 'when user can perform at least 1 action' do
       before do
-        expect(current_ability).to receive(:can?).with(:edit, presenter.id).and_return false
-        expect(current_ability).to receive(:can?).with(:destroy, presenter.id).and_return false
-        expect(current_ability).to receive(:can?).with(:download, presenter.id).and_return true
+        expect(ability).to receive(:can?).with(:edit, presenter.id).and_return false
+        expect(ability).to receive(:can?).with(:destroy, presenter.id).and_return false
+        expect(ability).to receive(:can?).with(:download, presenter.id).and_return true
       end
 
-      it { is_expected.to be true }
+      its(:user_can_perform_any_action?) { is_expected.to eq true }
     end
+
     context 'when user cannot perform any action' do
       before do
-        expect(current_ability).to receive(:can?).with(:edit, presenter.id).and_return false
-        expect(current_ability).to receive(:can?).with(:destroy, presenter.id).and_return false
-        expect(current_ability).to receive(:can?).with(:download, presenter.id).and_return false
+        expect(ability).to receive(:can?).with(:edit, presenter.id).and_return false
+        expect(ability).to receive(:can?).with(:destroy, presenter.id).and_return false
+        expect(ability).to receive(:can?).with(:download, presenter.id).and_return false
       end
 
-      it { is_expected.to be false }
+      its(:user_can_perform_any_action?) { is_expected.to eq false }
     end
   end
 
@@ -101,6 +90,7 @@
         presenter.send(property)
       end
     end
+
     it { is_expected.to delegate_method(:depositor).to(:solr_document) }
     it { is_expected.to delegate_method(:keyword).to(:solr_document) }
     it { is_expected.to delegate_method(:date_created).to(:solr_document) }
@@ -113,9 +103,13 @@
 
   describe '#link_name' do
     context "with a user who can view the file" do
-      before do
-        allow(ability).to receive(:can?).with(:read, "123abc").and_return(true)
+      let(:file_set) do
+        FactoryBot.valkyrie_create(:hyrax_file_set,
+                                   read_users: [user],
+                                   label: "filename.tif",
+                                   title: ["File title"])
       end
+
       it "shows the title" do
         expect(presenter.link_name).to eq 'File title'
         expect(presenter.link_name).not_to eq 'filename.tif'
@@ -123,9 +117,9 @@
     end
 
     context "with a user who cannot view the file" do
-      before do
-        allow(ability).to receive(:can?).with(:read, "123abc").and_return(false)
-      end
+      let(:ability) { Ability.new(other_user) }
+      let(:other_user) { FactoryBot.create(:user) }
+
       it "hides the title" do
         expect(presenter.link_name).to eq 'File'
       end
@@ -133,18 +127,18 @@
   end
 
   describe '#tweeter' do
-    subject { presenter.tweeter }
-
     it 'delegates the depositor as the user_key to TwitterPresenter.call' do
-      expect(Hyrax::TwitterPresenter).to receive(:twitter_handle_for).with(user_key: solr_document.depositor)
-      subject
+      expect(Hyrax::TwitterPresenter)
+        .to receive(:twitter_handle_for)
+        .with(user_key: solr_document.depositor)
+        .and_return(:fake_result)
+
+      expect(presenter.tweeter).to eq :fake_result
     end
   end
 
   describe "#event_class" do
-    subject { presenter.event_class }
-
-    it { is_expected.to eq 'FileSet' }
+    its(:event_class) { is_expected.to eq 'FileSet' }
   end
 
   describe '#events' do
@@ -174,26 +168,22 @@
 
   describe "characterization" do
     describe "#characterization_metadata" do
-      subject { presenter.characterization_metadata }
-
       it "only has set attributes are in the metadata" do
-        expect(subject[:height]).to be_blank
-        expect(subject[:page_count]).to be_blank
+        expect(presenter.characterization_metadata).not_to have_key(:height)
+        expect(presenter.characterization_metadata).not_to have_key(:page_count)
       end
 
       context "when height is set" do
         let(:attributes) { { height_is: '444' } }
 
-        it "only has set attributes are in the metadata" do
-          expect(subject[:height]).not_to be_blank
-          expect(subject[:page_count]).to be_blank
+        it "has set attributes are in the metadata" do
+          expect(presenter.characterization_metadata[:height]).to eq '444'
+          expect(presenter.characterization_metadata).not_to have_key(:page_count)
         end
       end
     end
 
     describe "#characterized?" do
-      subject { presenter }
-
       it { is_expected.not_to be_characterized }
 
       context "when height is set" do
@@ -210,9 +200,9 @@
     end
 
     describe "#label_for_term" do
-      subject { presenter.label_for_term(:titleized_key) }
-
-      it { is_expected.to eq("Titleized Key") }
+      it "titleizes the input" do
+        expect(presenter.label_for_term(:titleized_key)).to eq("Titleized Key")
+      end
     end
 
     describe "with additional characterization metadata" do
@@ -223,31 +213,32 @@
         }
       end
 
-      before { allow(presenter).to receive(:additional_characterization_metadata).and_return(additional_metadata) }
-      subject { presenter }
+      # this is a little absurd, but it's not clear to me what the interface is supposed to be
+      # how do i actually inject additional metadata?
+      before { allow(presenter).to receive(:additional_characterization_metadata).and_return(additional_metadata) } # rubocop:disable RSpec/SubjectStub
 
-      specify do
-        expect(subject).to be_characterized
+      it "adds the metadata" do
+        expect(presenter).to be_characterized
         expect(subject.characterization_metadata[:foo]).to contain_exactly("bar")
         expect(subject.characterization_metadata[:fud]).to contain_exactly("bars", "cars")
       end
     end
 
     describe "characterization values" do
-      before { allow(presenter).to receive(:characterization_metadata).and_return(mock_metadata) }
+      before { allow(presenter).to receive(:characterization_metadata).and_return(mock_metadata) } # rubocop:disable RSpec/SubjectStub
 
       context "with a limited set of short values" do
         let(:mock_metadata) { { term: ["asdf", "qwer"] } }
 
         describe "#primary_characterization_values" do
-          subject { presenter.primary_characterization_values(:term) }
-
-          it { is_expected.to contain_exactly("asdf", "qwer") }
+          it "includes the characterization metadata" do
+            expect(presenter.primary_characterization_values(:term))
+              .to contain_exactly("asdf", "qwer")
+          end
         end
-        describe "#secondary_characterization_values" do
-          subject { presenter.secondary_characterization_values(:term) }
 
-          it { is_expected.to be_empty }
+        describe "#secondary_characterization_values" do
+          it("is empty") { expect(presenter.secondary_characterization_values(:term)).to be_empty }
         end
       end
 
@@ -255,14 +246,17 @@
         let(:mock_metadata) { { term: ["1", "2", "3", "4", "5", "6", "7", "8"] } }
 
         describe "#primary_characterization_values" do
-          subject { presenter.primary_characterization_values(:term) }
-
-          it { is_expected.to contain_exactly("1", "2", "3", "4", "5") }
+          it "contains the configured number of values" do
+            expect(presenter.primary_characterization_values(:term))
+              .to contain_exactly("1", "2", "3", "4", "5")
+          end
         end
-        describe "#secondary_characterization_values" do
-          subject { presenter.secondary_characterization_values(:term) }
 
-          it { is_expected.to contain_exactly("6", "7", "8") }
+        describe "#secondary_characterization_values" do
+          it "includes the excess" do
+            expect(presenter.secondary_characterization_values(:term))
+              .to contain_exactly("6", "7", "8")
+          end
         end
       end
 
@@ -270,14 +264,17 @@
         let(:mock_metadata) { { term: [("a" * 251), "2", "3", "4", "5", "6", ("b" * 251)] } }
 
         describe "#primary_characterization_values" do
-          subject { presenter.primary_characterization_values(:term) }
-
-          it { is_expected.to contain_exactly(("a" * 247) + "...", "2", "3", "4", "5") }
+          it "truncates" do
+            expect(presenter.primary_characterization_values(:term))
+              .to contain_exactly(("a" * 247) + "...", "2", "3", "4", "5")
+          end
         end
-        describe "#secondary_characterization_values" do
-          subject { presenter.secondary_characterization_values(:term) }
 
-          it { is_expected.to contain_exactly("6", (("b" * 247) + "...")) }
+        describe "#secondary_characterization_values" do
+          it "truncates" do
+            expect(presenter.secondary_characterization_values(:term))
+              .to contain_exactly("6", (("b" * 247) + "..."))
+          end
         end
       end
 
@@ -285,14 +282,14 @@
         let(:mock_metadata) { { term: "string" } }
 
         describe "#primary_characterization_values" do
-          subject { presenter.primary_characterization_values(:term) }
-
-          it { is_expected.to contain_exactly("string") }
+          it "contains the string value" do
+            expect(presenter.primary_characterization_values(:term))
+              .to contain_exactly("string")
+          end
         end
-        describe "#secondary_characterization_values" do
-          subject { presenter.secondary_characterization_values(:term) }
 
-          it { is_expected.to be_empty }
+        describe "#secondary_characterization_values" do
+          it("is empty") { expect(presenter.secondary_characterization_values(:term)).to be_empty }
         end
       end
 
@@ -313,42 +310,38 @@ def uri_segment_escape(uri)
       ActionDispatch::Journey::Router::Utils.escape_segment(uri)
     end
 
-    let(:file_set) { create(:file_set) }
-    let(:solr_document) { SolrDocument.new(file_set.to_solr) }
+    subject(:presenter) { described_class.new(solr_document, ability, request) }
+
+    let(:file) { FactoryBot.create(:uploaded_file) }
+    let(:file_metadata) { FactoryBot.valkyrie_create(:file_metadata, :original_file, :with_file, file: file) }
+    let(:file_set) { FactoryBot.valkyrie_create(:hyrax_file_set) }
     let(:request) { double('request', base_url: 'http://test.host') }
-    let(:presenter) { described_class.new(solr_document, ability, request) }
-    let(:id) { Hyrax::Base.uri_to_id(file_set.original_file.versions.last.uri) }
+    let(:id) { "#{file_set.id}/files/#{file_metadata.id}" }
 
     describe "#display_image" do
-      subject { presenter.display_image }
-
       context 'without a file' do
         let(:id) { 'bogus' }
 
-        it { is_expected.to be_nil }
+        its(:display_image) { is_expected.to be_nil }
       end
 
       context 'with a file' do
-        before do
-          Hydra::Works::AddFileToFileSet.call(file_set,
-                                              file_path, :original_file)
+        let(:file_set) do
+          FactoryBot.valkyrie_create(:hyrax_file_set,
+                                     :with_files,
+                                     files: [file_metadata],
+                                     original_file: file_metadata)
         end
 
         context "when the file is not an image" do
-          let(:file_path) { File.open(fixture_path + '/hyrax_generic_stub.txt') }
+          let(:file) { FactoryBot.create(:uploaded_file, :audio) }
 
-          it { is_expected.to be_nil }
+          its(:display_image) { is_expected.to be_nil }
         end
 
         context "when the file is an image" do
-          let(:file_path) { File.open(fixture_path + '/world.png') }
-
-          before do
-            allow(solr_document).to receive(:image?).and_return(true)
-          end
-
-          it { is_expected.to be_instance_of IIIFManifest::DisplayImage }
-          its(:url) { is_expected.to eq "http://test.host/images/#{uri_segment_escape(id)}/full/600,/0/default.jpg" }
+          its(:display_image) { is_expected.to be_instance_of IIIFManifest::DisplayImage }
+          its(:display_image) { is_expected.to have_attributes(url: "http://test.host/images/#{uri_segment_escape(id)}/full/600,/0/default.jpg") }
 
           context 'with custom image size default' do
             let(:custom_image_size) { '666,' }
@@ -360,12 +353,12 @@ def uri_segment_escape(uri)
               Hyrax.config.iiif_image_size_default = default_image_size
             end
 
-            it { is_expected.to be_instance_of IIIFManifest::DisplayImage }
-            its(:url) { is_expected.to eq "http://test.host/images/#{uri_segment_escape(id)}/full/#{custom_image_size}/0/default.jpg" }
+            its(:display_image) { is_expected.to be_instance_of IIIFManifest::DisplayImage }
+            its(:display_image) { is_expected.to have_attributes(url: "http://test.host/images/#{uri_segment_escape(id)}/full/#{custom_image_size}/0/default.jpg") }
           end
 
           context 'with custom image url builder' do
-            let(:id) { file_set.original_file.id }
+            let(:id) { file_set.id.to_s }
             let(:custom_builder) do
               ->(file_id, base_url, _size, _format) { "#{base_url}/downloads/#{file_id.split('/').first}" }
             end
@@ -377,14 +370,15 @@ def uri_segment_escape(uri)
               Hyrax.config.iiif_image_url_builder = default_builder
             end
 
-            it { is_expected.to be_instance_of IIIFManifest::DisplayImage }
-            its(:url) { is_expected.to eq "http://test.host/downloads/#{id.split('/').first}" }
+            its(:display_image) { is_expected.to be_instance_of IIIFManifest::DisplayImage }
+            its(:display_image) { is_expected.to have_attributes(url: "http://test.host/downloads/#{id.split('/').first}") }
           end
 
           context "when the user doesn't have permission to view the image" do
-            let(:user) { create(:user) }
+            let(:ability) { Ability.new(other_user) }
+            let(:other_user) { FactoryBot.create(:user) }
 
-            it { is_expected.to be_nil }
+            its(:display_image) { is_expected.to be_nil }
           end
         end
       end
@@ -395,8 +389,6 @@ def uri_segment_escape(uri)
 
       before do
         allow(Hyrax.config).to receive(:iiif_image_server?).and_return(riiif_enabled)
-        Hydra::Works::AddFileToFileSet.call(file_set,
-                                            File.open(fixture_path + '/world.png'), :original_file)
       end
 
       context 'with iiif_image_server enabled' do
@@ -430,73 +422,78 @@ def uri_segment_escape(uri)
   describe "#parent" do
     let(:read_permission) { true }
     let(:edit_permission) { false }
-    let(:parent_work_active) do
-      create(:work, :public, state: ::RDF::URI('http://fedora.info/definitions/1/0/access/ObjState#active'))
+
+    let(:active) do
+      ::RDF::URI('http://fedora.info/definitions/1/0/access/ObjState#active')
     end
-    let(:file_set_active) do
-      create(:file_set, read_groups: ['public']).tap do |file_set|
-        parent_work_active.ordered_members << file_set
-        parent_work_active.save!
-      end
+
+    let(:inactive) do
+      ::RDF::URI('http://fedora.info/definitions/1/0/access/ObjState#inactive')
     end
-    let(:parent_work_inactive) do
-      create(:work, :public, state: ::RDF::URI('http://fedora.info/definitions/1/0/access/ObjState#inactive'))
+
+    let(:file_set) do
+      FactoryBot.valkyrie_create(:hyrax_file_set, read_groups: ['public'])
     end
+
     let(:file_set_inactive) do
-      create(:file_set, read_groups: ['public']).tap do |file_set|
-        parent_work_inactive.ordered_members << file_set
-        parent_work_inactive.save!
-      end
+      FactoryBot.valkyrie_create(:hyrax_file_set, read_groups: ['public'])
     end
 
     describe "active parent" do
       let(:read_permission) { true }
       let(:edit_permission) { false }
-      let(:solr_document) { SolrDocument.new(file_set_active.to_solr) }
-      let(:solr_document_work) { SolrDocument.new(parent_work_active.to_solr) }
+      let(:solr_document) { SolrDocument.new(Hyrax::ValkyrieIndexer.for(resource: file_set).to_solr) }
+      let(:solr_document_work) { SolrDocument.new(Hyrax::ValkyrieIndexer.for(resource: parent).to_solr) }
       let(:request) { double(base_url: 'http://test.host') }
       let(:presenter) { described_class.new(solr_document, ability, request) }
 
+      let!(:parent) do
+        FactoryBot.valkyrie_create(:hyrax_work, :public, state: active, member_ids: [file_set.id])
+      end
+
+
       before do
         allow(ability).to receive(:can?).with(:read, anything) do |_read, solr_doc|
           solr_document_work.id == solr_doc.id && read_permission
         end
+
         allow(ability).to receive(:can?).with(:edit, anything) do |_read, solr_doc|
           solr_document_work.id == solr_doc.id && edit_permission
         end
       end
 
       context "is created when parent work is active" do
-        subject { presenter.parent }
-
-        it { is_expected.not_to be_nil }
+        its(:parent) { is_expected.not_to be_nil }
       end
     end
 
     describe "inactive parent" do
       let(:read_permission) { true }
       let(:edit_permission) { false }
-      let(:solr_document) { SolrDocument.new(file_set_inactive.to_solr) }
-      let(:solr_document_work) { SolrDocument.new(parent_work_inactive.to_solr) }
+      let(:solr_document) { SolrDocument.new(Hyrax::ValkyrieIndexer.for(resource: file_set).to_solr) }
+      let(:solr_document_work) { SolrDocument.new(Hyrax::ValkyrieIndexer.for(resource: parent).to_solr) }
       let(:request) { double(base_url: 'http://test.host') }
       let(:presenter) { described_class.new(solr_document, ability, request) }
 
+      let!(:parent) do
+        FactoryBot.valkyrie_create(:hyrax_work, :public, state: inactive, member_ids: [file_set.id])
+      end
+
       before do
         allow(ability).to receive(:can?).with(:read, anything) do |_read, solr_doc|
           solr_document_work.id == solr_doc.id && read_permission
         end
+
         allow(ability).to receive(:can?).with(:edit, anything) do |_read, solr_doc|
           solr_document_work.id == solr_doc.id && edit_permission
         end
       end
 
       context "is created when parent work is active" do
-        subject { presenter.parent }
         it "raises an error" do
-          expect { subject }.to raise_error(Hyrax::WorkflowAuthorizationException)
+          expect { presenter.parent }.to raise_error(Hyrax::WorkflowAuthorizationException)
         end
       end
     end
   end
 end
-# rubocop:enable RSpec/SubjectStub

From 40d4b521ab833333126d4fd78098c997df6f21d8 Mon Sep 17 00:00:00 2001
From: Daniel Pierce <dlpierce@indiana.edu>
Date: Tue, 19 Sep 2023 02:31:59 -0400
Subject: [PATCH 2/6] Updated #to_partial_path

---
 spec/presenters/hyrax/file_set_presenter_spec.rb | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/spec/presenters/hyrax/file_set_presenter_spec.rb b/spec/presenters/hyrax/file_set_presenter_spec.rb
index cc1fcafc12..cc0b3d6832 100644
--- a/spec/presenters/hyrax/file_set_presenter_spec.rb
+++ b/spec/presenters/hyrax/file_set_presenter_spec.rb
@@ -29,15 +29,11 @@
   end
 
   describe "#model_name" do
-    it do
-      name = subject.model_name
-      require 'pry'; binding.pry
-    end
     its(:model_name) { is_expected.to be_kind_of ActiveModel::Name }
   end
 
   describe "#to_partial_path" do
-    its(:to_partial_path) { is_expected.to eq 'file_sets/file_set' }
+    its(:to_partial_path) { is_expected.to eq 'hyrax/file_sets/file_set' }
   end
 
   describe "office_document?" do
@@ -451,7 +447,6 @@ def uri_segment_escape(uri)
         FactoryBot.valkyrie_create(:hyrax_work, :public, state: active, member_ids: [file_set.id])
       end
 
-
       before do
         allow(ability).to receive(:can?).with(:read, anything) do |_read, solr_doc|
           solr_document_work.id == solr_doc.id && read_permission

From cec1373c7c737ed181a8bcc24d1e38346809d174 Mon Sep 17 00:00:00 2001
From: Daniel Pierce <dlpierce@indiana.edu>
Date: Tue, 19 Sep 2023 02:34:13 -0400
Subject: [PATCH 3/6] Set file uri when available from the resource

Add file spec
---
 lib/wings/active_fedora_converter.rb       |  6 ++++++
 spec/wings/active_fedora_converter_spec.rb | 20 ++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/lib/wings/active_fedora_converter.rb b/lib/wings/active_fedora_converter.rb
index 5b5dcfe1fd..a709b7b741 100644
--- a/lib/wings/active_fedora_converter.rb
+++ b/lib/wings/active_fedora_converter.rb
@@ -170,6 +170,7 @@ def add_access_control_attributes(af_object)
 
     # for files, add attributes to metadata_node, plus some other work
     def add_file_attributes(af_object)
+      add_file_uri(af_object)
       converted_attrs = normal_attributes
       pcdm_use = converted_attrs.delete(:pcdm_use)
       af_object.metadata_node.attributes = converted_attrs
@@ -180,6 +181,11 @@ def add_file_attributes(af_object)
       af_object.mime_type = resource.mime_type
     end
 
+    def add_file_uri(af_object)
+      file_uri = Hyrax.storage_adapter.fedora_identifier(id: resource.file_identifier)
+      af_object.uri = file_uri.to_s if af_object.uri.to_s.blank? && file_uri.to_s.present?
+    end
+
     def perform_lease_conversion(af_object:, resource:)
       # TODO(#6134): af_object.lease.class has the same name as resource.lease.class; however, each class has a different object_id
       # so a type mismatch happens. the code below coerces the one object into the other
diff --git a/spec/wings/active_fedora_converter_spec.rb b/spec/wings/active_fedora_converter_spec.rb
index 3137a73f0f..f4e7c67ed8 100644
--- a/spec/wings/active_fedora_converter_spec.rb
+++ b/spec/wings/active_fedora_converter_spec.rb
@@ -73,14 +73,30 @@
 
     context 'when given a FileMetadata node' do
       let(:resource) { Hyrax::FileMetadata.new(file_identifier: file.id) }
+      let(:io) { fixture_file_upload('/world.png', 'image/png') }
       let(:file) do
-        io = fixture_file_upload('/world.png', 'image/png')
         file_set = FactoryBot.valkyrie_create(:hyrax_file_set)
         storage_adapter.upload(file: io, resource: file_set, original_filename: 'test-world.png')
       end
 
       context 'when it describes an ActiveFedora File' do
-        it 'converts to a Hydra::Pcdm::File'
+        let(:storage_adapter) { Valkyrie::StorageAdapter.find(:active_fedora) }
+
+        it 'converts to a Hydra::Pcdm::File' do
+          expect(converter.convert).to be_a Hydra::PCDM::File
+        end
+
+        it 'refers to the correct file id' do
+          expect(converter.convert)
+            .to have_attributes(uri: storage_adapter.fedora_identifier(id: file.id))
+        end
+
+        it 'round trips' do
+          af = converter.convert
+          af.save
+          io.rewind
+          expect(Hydra::PCDM::File.find(af.id).content).to eq io.read
+        end
       end
 
       context 'when it describes a file for an arbitrary storage adapter' do

From ac6258695bf06ead7ec70aaf8e6eee5d22893f0e Mon Sep 17 00:00:00 2001
From: Daniel Pierce <dlpierce@indiana.edu>
Date: Tue, 19 Sep 2023 02:38:14 -0400
Subject: [PATCH 4/6] Replace use specific *_id attributes in Hyrax::FileSet
 with lookups

Use dedicated custom queries for finding files

Use test adapter so custom queries can find files; add tests for file accessors

Additional factory uses
---
 app/models/hyrax/file_set.rb                | 39 +++++++++++++++++++--
 app/services/hyrax/valkyrie_upload.rb       | 26 --------------
 lib/hyrax/specs/shared_specs/hydra_works.rb | 27 ++++++++++----
 spec/factories/hyrax_file_metadata.rb       |  8 +++++
 spec/factories/hyrax_file_set.rb            |  3 --
 5 files changed, 64 insertions(+), 39 deletions(-)

diff --git a/app/models/hyrax/file_set.rb b/app/models/hyrax/file_set.rb
index f74a6accca..175b6662e8 100644
--- a/app/models/hyrax/file_set.rb
+++ b/app/models/hyrax/file_set.rb
@@ -60,9 +60,42 @@ def self.model_name(name_class: Hyrax::Name)
     self.characterization_proxy = Hyrax.config.characterization_proxy
 
     attribute :file_ids, Valkyrie::Types::Array.of(Valkyrie::Types::ID) # id for FileMetadata resources
-    attribute :thumbnail_id, Valkyrie::Types::ID.optional # id for FileMetadata resource
-    attribute :original_file_id, Valkyrie::Types::ID.optional # id for FileMetadata resource
-    attribute :extracted_text_id, Valkyrie::Types::ID.optional # id for FileMetadata resource
+
+    # @return [Hyrax::FileMetadata, nil]
+    def original_file
+      Hyrax.custom_queries.find_original_file(file_set: self)
+    rescue Valkyrie::Persistence::ObjectNotFoundError
+      nil
+    end
+
+    # @return [Valkyrie::ID, nil]
+    def original_file_id
+      original_file&.id
+    end
+
+    # @return [Hyrax::FileMetadata, nil]
+    def thumbnail
+      Hyrax.custom_queries.find_thumbnail(file_set: self)
+    rescue Valkyrie::Persistence::ObjectNotFoundError
+      nil
+    end
+
+    # @return [Valkyrie::ID, nil]
+    def thumbnail_id
+      thumbnail&.id
+    end
+
+    # @return [Hyrax::FileMetadata, nil]
+    def extracted_text
+      Hyrax.custom_queries.find_extracted_text(file_set: self)
+    rescue Valkyrie::Persistence::ObjectNotFoundError
+      nil
+    end
+
+    # @return [Valkyrie::ID, nil]
+    def extracted_text_id
+      extracted_text&.id
+    end
 
     ##
     # @return [Valkyrie::ID]
diff --git a/app/services/hyrax/valkyrie_upload.rb b/app/services/hyrax/valkyrie_upload.rb
index 4f8fea721f..30f9a4993d 100644
--- a/app/services/hyrax/valkyrie_upload.rb
+++ b/app/services/hyrax/valkyrie_upload.rb
@@ -81,33 +81,7 @@ def version_upload(file_set:, io:, user:)
   # @return [Hyrax::FileSet] updated file set
   def add_file_to_file_set(file_set:, file_metadata:, user:)
     file_set.file_ids << file_metadata.id
-    set_file_use_ids(file_set, file_metadata)
-
     Hyrax.persister.save(resource: file_set)
     Hyrax.publisher.publish('object.membership.updated', object: file_set, user: user)
   end
-
-  private
-
-  # @api private
-  # @param [Hyrax::FileSet] file_set the file set to add to
-  # @param [Hyrax::FileMetadata] file_metadata the metadata object representing
-  #   the file to add
-  # @return [void]
-  def set_file_use_ids(file_set, file_metadata)
-    file_metadata.pcdm_use.each do |type|
-      case type
-      when Hyrax::FileMetadata::Use::ORIGINAL_FILE
-        file_set.original_file_id = file_metadata.id
-      when Hyrax::FileMetadata::Use::THUMBNAIL
-        file_set.thumbnail_id = file_metadata.id
-      when Hyrax::FileMetadata::Use::EXTRACTED_TEXT
-        file_set.extracted_text_id = file_metadata.id
-      when Hyrax::FileMetadata::Use::SERVICE_FILE
-        # do nothing
-      else
-        Hyrax.logger.warn "Unknown file use #{file_metadata.type} specified for #{file_metadata.file_identifier}"
-      end
-    end
-  end
 end
diff --git a/lib/hyrax/specs/shared_specs/hydra_works.rb b/lib/hyrax/specs/shared_specs/hydra_works.rb
index a5c8bddc31..00858bbfb6 100644
--- a/lib/hyrax/specs/shared_specs/hydra_works.rb
+++ b/lib/hyrax/specs/shared_specs/hydra_works.rb
@@ -249,7 +249,7 @@
   end
 end
 
-RSpec.shared_examples 'a Hyrax::FileSet' do
+RSpec.shared_examples 'a Hyrax::FileSet', valkyrie_adapter: :test_adapter do
   subject(:fileset)   { described_class.new }
   let(:adapter)       { Valkyrie::MetadataAdapter.find(:test_adapter) }
   let(:persister)     { adapter.persister }
@@ -275,12 +275,10 @@
     end
 
     context 'with files' do
-      let(:file_class) { Hyrax::FileMetadata }
-      let(:files) do
-        [file_class.new, file_class.new, file_class.new]
-          .map! { |f| persister.save(resource: f) }
-      end
-
+      let(:original_file) { FactoryBot.valkyrie_create :hyrax_file_metadata, :original_file }
+      let(:thumbnail) { FactoryBot.valkyrie_create :hyrax_file_metadata, :thumbnail }
+      let(:extracted_text) { FactoryBot.valkyrie_create :hyrax_file_metadata, :extracted_text }
+      let(:files) { [original_file, thumbnail, extracted_text] }
       let(:file_ids) { files.map(&:id) }
 
       before { fileset.file_ids = file_ids }
@@ -301,6 +299,21 @@
         expect { fileset.file_ids << file_ids.first }
           .not_to change { query_service.custom_queries.find_files(file_set: fileset) }
       end
+
+      it 'returns an original_file' do
+        expect(fileset.original_file).to eq original_file
+        expect(fileset.original_file_id).to eq original_file.id
+      end
+
+      it 'returns a thumbnail' do
+        expect(fileset.thumbnail).to eq thumbnail
+        expect(fileset.thumbnail_id).to eq thumbnail.id
+      end
+
+      it 'returns an extracted_text' do
+        expect(fileset.extracted_text).to eq extracted_text
+        expect(fileset.extracted_text_id).to eq extracted_text.id
+      end
     end
   end
 end
diff --git a/spec/factories/hyrax_file_metadata.rb b/spec/factories/hyrax_file_metadata.rb
index cf6762ca85..f618fd5df3 100644
--- a/spec/factories/hyrax_file_metadata.rb
+++ b/spec/factories/hyrax_file_metadata.rb
@@ -36,6 +36,14 @@
       use { Hyrax::FileMetadata::Use.uri_for(use: :original_file) }
     end
 
+    trait :thumbnail do
+      use { Hyrax::FileMetadata::Use.uri_for(use: :thumbnail_file) }
+    end
+
+    trait :extracted_text do
+      use { Hyrax::FileMetadata::Use.uri_for(use: :extracted_file) }
+    end
+
     trait :image do
       mime_type { 'image/png' }
     end
diff --git a/spec/factories/hyrax_file_set.rb b/spec/factories/hyrax_file_set.rb
index 5ac4a9d4c9..320814936e 100644
--- a/spec/factories/hyrax_file_set.rb
+++ b/spec/factories/hyrax_file_set.rb
@@ -24,9 +24,6 @@
           .assign_access_for(visibility: evaluator.visibility_setting)
       end
       file_set.file_ids = evaluator.files.map(&:id) if evaluator.files
-      file_set.original_file_id = evaluator.original_file.id if evaluator.original_file
-      file_set.extracted_text_id = evaluator.extracted_text.id if evaluator.extracted_text
-      file_set.thumbnail_id = evaluator.thumbnail.id if evaluator.thumbnail
 
       file_set.permission_manager.edit_groups = evaluator.edit_groups
       file_set.permission_manager.edit_users  = evaluator.edit_users

From 7827c8027b825951803f367b9a5fb15c77c25355 Mon Sep 17 00:00:00 2001
From: Daniel Pierce <dlpierce@indiana.edu>
Date: Tue, 19 Sep 2023 17:34:44 -0400
Subject: [PATCH 5/6] Short id prevented correct file id creation; use factory
 to create persisted file

---
 spec/wings/model_transformer_spec.rb | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/spec/wings/model_transformer_spec.rb b/spec/wings/model_transformer_spec.rb
index c4eccbc4e4..7ec21c38ef 100644
--- a/spec/wings/model_transformer_spec.rb
+++ b/spec/wings/model_transformer_spec.rb
@@ -349,16 +349,11 @@ class Book < Hyrax::Resource
   end
 
   context 'build for file' do
-    let(:id)       { '123' }
-    let(:file_set) { FactoryBot.create(:file_set, id: id) }
-    let(:file) { file_set.build_original_file }
+    let(:file_set) { FactoryBot.create(:file_set, :image) }
+    let(:file) { file_set.original_file }
     let(:pcdm_object) { file_set }
 
     context 'with content' do
-      before do
-        file.content = 'foo'
-      end
-
       it 'sets file id in file set resource' do
         expect(subject.build.original_file_id).to eq file.id
       end

From b7a3c084f230dc1ce72f89d3ca7e05548de8a2fe Mon Sep 17 00:00:00 2001
From: Daniel Pierce <dlpierce@indiana.edu>
Date: Tue, 19 Sep 2023 20:12:07 -0400
Subject: [PATCH 6/6] Mock response for mock_text

---
 spec/indexers/hyrax/valkyrie_file_set_indexer_spec.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/spec/indexers/hyrax/valkyrie_file_set_indexer_spec.rb b/spec/indexers/hyrax/valkyrie_file_set_indexer_spec.rb
index 22221bb2ab..c624933c38 100644
--- a/spec/indexers/hyrax/valkyrie_file_set_indexer_spec.rb
+++ b/spec/indexers/hyrax/valkyrie_file_set_indexer_spec.rb
@@ -128,6 +128,7 @@
       allow(Hyrax.custom_queries).to receive(:find_original_file).with(file_set: file_set).and_return(mock_file)
       allow(Hyrax.custom_queries).to receive(:find_file_metadata_by).with(id: file_set.original_file_id).and_return(mock_file)
       allow(Hyrax.custom_queries).to receive(:find_thumbnail).with(file_set: file_set).and_return(mock_thumbnail)
+      allow(Hyrax.custom_queries).to receive(:find_extracted_text).with(file_set: file_set).and_return(mock_text)
       allow(mock_file).to receive(:file_name).and_return(file_name)
     end
     subject { indexer.generate_solr_document }