Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow IngestLocalFileJob to receive either an ActiveFedora object or a Valkyrie resource #4196

Closed
randalldfloyd opened this issue Jan 22, 2020 · 0 comments · Fixed by #4925
Closed

Allow IngestLocalFileJob to receive either an ActiveFedora object or a Valkyrie resource #4196

randalldfloyd opened this issue Jan 22, 2020 · 0 comments · Fixed by #4925
Assignees
@jeremyf
Labels
wings

Comments

@randalldfloyd
Copy link
Contributor

Descriptive summary

Convert IngestLocalFileJob to allow it to receive either an ActiveFedora based object or a Valkyrie::Resource equivalent object.

Rationale

Allow jobs to execute correctly with existing AF implementation or the Wings Valkyrie implementation.

Expected behavior

IngestLocalFileJob receives a Valkyrie::Resource and performs the action.

Actual behavior

IngestLocalFileJob raises an exception when passed a Valkyrie::Resource.

Approach for Conversion

See Pattern for Jobs Receiving Parameter that may be an AF Object or a Valkyrie Resource

Related work

These have all be modified to support AF object or Valkyrie resource using the established pattern.
@jeremyf jeremyf self-assigned this May 5, 2021
jeremyf added a commit that referenced this issue May 5, 2021
@jeremyf
Refactoring to make an easier change
787513f 
This is code change is a simple restructuring to enable an easier review
for future work on #4196.  _I know I struggle reviewing a code change
that both wraps a chunk of code with a containing block AND changes the
logic of that block._

Related to #4196
@jeremyf jeremyf mentioned this issue May 5, 2021
Merged
elrayle added a commit that referenced this issue May 5, 2021
@elrayle
Merge pull request #4916 from samvera/gh-4196-ingest-local-file-job-v…
7f346f9 
…alkyrization

Refactoring to make an easier change
jeremyf added a commit that referenced this issue May 6, 2021
@jeremyf
Ensuring IngestLocalFileJob handles valkyrie obj
0406d40 
This commit documents and tests that the IngestLocalFileJob handles
valkyrie resources.  We wouldn't have needed to make any changes to this
job to verify that Valkyrie worked.  However, by favoring an explicit
change a future maintainer can quickly look at this class and say "Cool,
I don't need to check if this works with Valkyrie!"

Closes #4196
@jeremyf jeremyf mentioned this issue May 6, 2021
Merged
no-reply pushed a commit that referenced this issue May 6, 2021
@jeremyf
Ensuring IngestLocalFileJob handles valkyrie obj
2546b4f 
This commit documents and tests that the IngestLocalFileJob handles
valkyrie resources.  We wouldn't have needed to make any changes to this
job to verify that Valkyrie worked.  However, by favoring an explicit
change a future maintainer can quickly look at this class and say "Cool,
I don't need to check if this works with Valkyrie!"

Closes #4196
jeremyf added a commit that referenced this issue May 6, 2021
@jeremyf
Refactoring to make an easier change
b03f9fb 
This is code change is a simple restructuring to enable an easier review
for future work on #4196.  _I know I struggle reviewing a code change
that both wraps a chunk of code with a containing block AND changes the
logic of that block._

Related to #4196

-  let(:file_set) do FileSet.new(import_url:
-    "http://example.org#{file_hash}", label: label) do |f|
-    f.apply_depositor_metadata(user.user_key) end end

   let(:operation) { create(:operation) } - let(:actor) {
   instance_double(Hyrax::Actors::FileSetActor, create_content: true) }

   let(:mock_retriever) { double } let(:inbox) { user.mailbox.inbox }

   before do - allow(Hyrax::Actors::FileSetActor).to
   receive(:new).with(file_set, user).and_return(actor) -
   response_headers = { 'Content-Type' => 'image/png', 'Content-Length'
   => 1, @@ -40,142 +31,156 @@ allow(mock_retriever).to
   receive(:retrieve) end

-  context 'before enqueueing the job' do before do file_set.id =
-    'fsid123' end

-    describe '.operation' do it 'fetches the operation' do
-      described_class.perform_later(file_set, operation) expect {
-      subject.operation.to eq Hyrax::Operation } + context 'when
-      use_valkyrie is false' do + let(:file_set) do +
-      FileSet.new(import_url: "http://example.org#{file_hash}", +
-      label: label) do |f| + f.apply_depositor_metadata(user.user_key)
-      end end end context 'after running the job' do let!(:tmpdir) {
-      Rails.root.join("tmp/spec/#{Process.pid}") } + let(:actor) {
-      instance_double(Hyrax::Actors::FileSetActor, create_content:
-      true) } before do file_set.id = 'abc123' allow(file_set).to
-      receive(:reload) FileUtils.mkdir_p(tmpdir) allow(Dir).to
-      receive(:mktmpdir).and_return(tmpdir) +
-      allow(Hyrax::Actors::FileSetActor).to
-      receive(:new).with(file_set, user).and_return(actor) end

-    after do FileUtils.remove_entry(tmpdir) end + context 'before
-      enqueueing the job' do + before do + file_set.id = 'fsid123' +
-      end

-    it 'creates the content and updates the associated operation' do
-      expect(actor).to receive(:create_content).with(File, from_url:
-      true).and_return(true) described_class.perform_now(file_set,
-      operation) expect(operation).to be_success + describe
-      '.operation' do + it 'fetches the operation' do +
-      described_class.perform_later(file_set, operation) + expect {
-      subject.operation.to eq Hyrax::Operation } + end + end end

-    it 'leaves the temp directory in place' do
-      described_class.perform_now(file_set, operation) file_name =
-      File.basename(file_set.label)
-      expect(File.exist?(File.join(tmpdir, file_name))).to be true end
-      + context 'after running the job' do + let!(:tmpdir) {
-      Rails.root.join("tmp/spec/#{Process.pid}") }

-    context 'when the FileSet has an existing label' do let(:label) {
-      "example.tif" } before do allow(Rails.logger).to receive(:debug)
-      + file_set.id = 'abc123' + allow(file_set).to receive(:reload) +
-      + FileUtils.mkdir_p(tmpdir) + allow(Dir).to
-      receive(:mktmpdir).and_return(tmpdir) end it 'uses the FileSet
-      label' do + + after do + FileUtils.remove_entry(tmpdir) + end + +
-      it 'creates the content and updates the associated operation' do
-      + expect(actor).to receive(:create_content).with(File, from_url:
-      true).and_return(true) described_class.perform_now(file_set,
-      operation) tmp_file_path = Rails.root.join(tmpdir, label)
-      expect(Rails.logger).to have_received(:debug).with("ImportUrlJob:
-      Closing #{tmp_file_path}")
-      expect(File.exist?(tmp_file_path.to_s)).to be true +
-      expect(operation).to be_success end end end

-  context "when a batch update job is running too" do let(:title) { {
-    file_set.id => ['File One'] } } let(:file_set_id) { file_set.id } +
-    it 'leaves the temp directory in place' do +
-    described_class.perform_now(file_set, operation) + file_name =
-    File.basename(file_set.label) +
-    expect(File.exist?(File.join(tmpdir, file_name))).to be true + end

-    before do file_set.save!  allow(ActiveFedora::Base).to
-      receive(:find).and_call_original allow(ActiveFedora::Base).to
-      receive(:find).with(file_set_id).and_return(file_set) # run the
-      batch job to set the title file_set.update(title: ['File One']) +
-      context 'when the FileSet has an existing label' do + let(:label)
-      { "example.tif" } + before do + allow(Rails.logger).to
-      receive(:debug) + end + it 'uses the FileSet label' do +
-      described_class.perform_now(file_set, operation) + tmp_file_path
-      = Rails.root.join(tmpdir, label) + expect(Rails.logger).to
-      have_received(:debug).with("ImportUrlJob: Closing
-      #{tmp_file_path}") + expect(File.exist?(tmp_file_path.to_s)).to
-      be true + end + end end

-    it "does not kill all the metadata set by other processes" do # run
-    the import job described_class.perform_now(file_set, operation) #
-    import job should not override the title set another process file =
-    FileSet.find(file_set_id) expect(file.title).to eq(['File One'])
-    end end + context "when a batch update job is running too" do +
-    let(:title) { { file_set.id => ['File One'] } } + let(:file_set_id)
-    { file_set.id }

-  context 'when the remote file is unavailable' do before do
-    stub_request(:get, "http://example.org#{file_hash}").with(headers:
-    { 'Range' => 'bytes=0-0' }).to_return( body: '', status: 406,
-    headers: {} ) end + before do + file_set.save!  +
-    allow(ActiveFedora::Base).to receive(:find).and_call_original +
-    allow(ActiveFedora::Base).to
-    receive(:find).with(file_set_id).and_return(file_set) + # run the
-    batch job to set the title + file_set.update(title: ['File One']) +
-    end

-    it 'sends error message' do expect(operation).to receive(:fail!)
-      expect(file_set.original_file).to be_nil
-      described_class.perform_now(file_set, operation)
-      expect(inbox.count).to eq(1) last_message = inbox[0].last_message
-      expect(last_message.subject).to eq('File Import Error')
-      expect(last_message.body).to eq("Error: Expired URL") + it "does
-      not kill all the metadata set by other processes" do + # run the
-      import job + described_class.perform_now(file_set, operation) + #
-      import job should not override the title set another process +
-      file = FileSet.find(file_set_id) + expect(file.title).to
-      eq(['File One']) + end end end

-  context 'when retrieval fails' do before { allow(mock_retriever).to
-    receive(:retrieve).and_raise(StandardError, 'Timeout') } it 'sends
-    error message' do expect(operation).to receive(:fail!)
-    expect(file_set.original_file).to be_nil
-    described_class.perform_now(file_set, operation)
-    expect(inbox.count).to eq(1) last_message = inbox[0].last_message
-    expect(last_message.subject).to eq('File Import Error')
-    expect(last_message.body).to eq("Error: Timeout") end end + context
-    'when the remote file is unavailable' do + before do +
-    stub_request(:get, "http://example.org#{file_hash}").with(headers:
-    { 'Range' => 'bytes=0-0' }).to_return( + body: '', status: 406,
-    headers: {} + ) + end

-  context 'when the URL to the remote file has headers' do
-    let(:import_url) { "http://example.org#{file_hash}" } let(:headers)
-    do { "Authorization" => "OAuth <ACCESS_TOKEN>" } end let(:file_set)
-    do FileSet.new(import_url: import_url, label: file_path) do |f|
-    f.apply_depositor_metadata(user.user_key) + it 'sends error
-    message' do + expect(operation).to receive(:fail!)  +
-    expect(file_set.original_file).to be_nil +
-    described_class.perform_now(file_set, operation) +
-    expect(inbox.count).to eq(1) + last_message = inbox[0].last_message
-    + expect(last_message.subject).to eq('File Import Error') +
-    expect(last_message.body).to eq("Error: Expired URL") end end
-    let(:operation) { create(:operation) } let(:import_uri) {
-    URI(import_url) }

-    before do allow(BrowseEverything::Retriever).to
-      receive(:can_retrieve?).and_return(true)
-      described_class.perform_now(file_set, operation, headers) end +
-      context 'when retrieval fails' do + before {
-      allow(mock_retriever).to
-      receive(:retrieve).and_raise(StandardError, 'Timeout') }

-    it 'submits a request to the cloud server with auth headers' do
-      expect(BrowseEverything::Retriever).to
-      have_received(:can_retrieve?).with(import_uri, headers) + it
-      'sends error message' do + expect(operation).to receive(:fail!)
-      + expect(file_set.original_file).to be_nil +
-      described_class.perform_now(file_set, operation) +
-      expect(inbox.count).to eq(1) + last_message =
-      inbox[0].last_message + expect(last_message.subject).to eq('File
-      Import Error') + expect(last_message.body).to eq("Error:
-      Timeout") + end end

-    it 'retrieves the cloud server resources with the auth headers' do
-      expect(mock_retriever).to have_received(:retrieve).with( "url" =>
-      import_uri, "headers" => headers ) + context 'when the URL to the
-      remote file has headers' do + let(:import_url) {
-      "http://example.org#{file_hash}" } + let(:headers) do + { +
-      "Authorization" => "OAuth <ACCESS_TOKEN>" + } + end +
-      let(:file_set) do + FileSet.new(import_url: import_url, label:
-      file_path) do |f| + f.apply_depositor_metadata(user.user_key) +
-      end + end + let(:operation) { create(:operation) } +
-      let(:import_uri) { URI(import_url) } + + before do +
-      allow(BrowseEverything::Retriever).to
-      receive(:can_retrieve?).and_return(true) +
-      described_class.perform_now(file_set, operation, headers) + end +
-      + it 'submits a request to the cloud server with auth headers' do
-      + expect(BrowseEverything::Retriever).to
-      have_received(:can_retrieve?).with(import_uri, headers) + end + +
-      it 'retrieves the cloud server resources with the auth headers'
-      do + expect(mock_retriever).to have_received(:retrieve).with( +
-      "url" => import_uri, + "headers" => headers + ) + end end end end
jeremyf added a commit that referenced this issue May 6, 2021
@jeremyf
Refactoring to make an easier change
d5dec6e 
This is code change is a simple restructuring to enable an easier review
for future work on #4196.  _I know I struggle reviewing a code change
that both wraps a chunk of code with a containing block AND changes the
logic of that block._

Related to #4196
@jeremyf jeremyf mentioned this issue May 6, 2021
Closed
jeremyf added a commit that referenced this issue May 6, 2021
@jeremyf
WIP
d44fcd5 
Adding a possible approach to resolving #4196; I want to put this up for
comment before I proceed with further tests.
jeremyf added a commit that referenced this issue May 6, 2021
@jeremyf
Refactoring to make an easier change
db5c4a7 
This is code change is a simple restructuring to enable an easier review
for future work on #4196.  _I know I struggle reviewing a code change
that both wraps a chunk of code with a containing block AND changes the
logic of that block._

Related to #4196
@jeremyf jeremyf mentioned this issue May 6, 2021
Merged
jeremyf added a commit that referenced this issue May 7, 2021
@jeremyf
Restructuring spec to ease review for Valkyrie
8d8e540 
This is code change is a simple restructuring to enable an easier review
for future work on #4196.  _I know I struggle reviewing a code change
that both wraps a chunk of code with a containing block AND changes the
logic of that block._

Related to #4196
no-reply pushed a commit that referenced this issue May 10, 2021
@jeremyf
Restructuring spec to ease review for Valkyrie
81a5d8c 
This is code change is a simple restructuring to enable an easier review
for future work on #4196.  _I know I struggle reviewing a code change
that both wraps a chunk of code with a containing block AND changes the
logic of that block._

Related to #4196
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jeremyf jeremyf

Labels
wings
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Gh 4196 step 2 samvera/hyrax
2 participants
@jeremyf @randalldfloyd