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

Handle errors when attaching files #4125

Merged
merged 3 commits into from
May 15, 2020
Merged

Handle errors when attaching files #4125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
55baaf7
Handle errors when attaching files
cjcolvar May 11, 2020
9f19cfb
Add default to SpeedyAF call to avoid reifying
cjcolvar May 11, 2020
255ecf6
Use Object#const_get instead of ActiveEncode::Base.descendants to get
cjcolvar May 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion app/controllers/media_objects_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,14 +515,19 @@ def move_preview
protected

def master_file_presenters
# NOTE: Defaults are set on returned SpeedyAF::Base objects if field isn't present in the solr doc.
# This is important otherwise speedy_af will reify from fedora when trying to access this field.
# When adding a new property to the master file model that will be used in the interface,
# add it to the default below to avoid reifying for master files lacking a value for the property.
SpeedyAF::Proxy::MasterFile.where("isPartOf_ssim:#{@media_object.id}",
order: -> { @media_object.indexed_master_file_ids },
defaults: {
permalink: nil,
title: nil,
encoder_classname: nil,
workflow_id: nil,
comment: []
comment: [],
supplemental_files_json: nil
})
end

Expand Down
16 changes: 9 additions & 7 deletions app/controllers/supplemental_files_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,15 @@ def create
raise Avalon::BadRequest, "Missing required parameters" unless supplemental_file_params[:file]

@supplemental_file = SupplementalFile.new(label: supplemental_file_params[:label])
@supplemental_file.attach_file(supplemental_file_params[:file])
# TODO: Add rollback handling in case masterfile fails to save
begin
@supplemental_file.attach_file(supplemental_file_params[:file])
rescue StandardError => e
raise Avalon::SaveError, "File could not be attached: #{e.full_message}"
end

# Raise errror if file wasn't attached
raise Avalon::SaveError, "File could not be attached." unless @supplemental_file.file.attached?

raise Avalon::SaveError, @supplemental_files.errors.full_messages unless @supplemental_file.save

@master_file.supplemental_files += [@supplemental_file]
Expand Down Expand Up @@ -108,11 +115,6 @@ def set_master_file
@master_file = MasterFile.find(params[:master_file_id])
end

# def build_supplemental_file
# # Note that built supplemental file does not have an absolute_path
# @supplemental_file = MasterFile::SupplementalFile.new(id: params[:id], label: supplemental_file_params[:label])
# end

def supplemental_file_params
# TODO: Add parameters for minio and s3
params.fetch(:supplemental_file, {}).permit(:label, :file)
Expand Down
4 changes: 3 additions & 1 deletion app/models/master_file.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,9 @@ def update_ingest_batch
end

def find_encoder_class(klass_name)
ActiveEncode::Base.descendants.find { |c| c.name == klass_name }
return nil if klass_name.blank? || !Object.const_defined?(klass_name)
klass = Object.const_get(klass_name)
return klass if klass.ancestors.include?(ActiveEncode::Base)
end

def stop_processing!
Expand Down
4 changes: 3 additions & 1 deletion app/presenters/speedy_af/proxy/master_file.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ def encoder_class
end

def find_encoder_class(klass_name)
ActiveEncode::Base.descendants.find { |c| c.name == klass_name }
return nil if klass_name.blank? || !Object.const_defined?(klass_name)
klass = Object.const_get(klass_name)
return klass if klass.ancestors.include?(ActiveEncode::Base)
end
Comment on lines 24 to 28
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of duplicated code (not just this method) between this proxy and the model, is there a way to dry it out using mixin or something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, this is probably a good time to clean it up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked but couldn't come up with any good alternative to this duplication right now. I think the proper solution is the separation of the UI from the model by true presenters powered only by solr documents instead of speedy_af proxies powered by both solr and fedora, but that is considerable more work to go through the UI and make sure that we are using the presenter only. It is a larger refactor that probably shouldn't hold up this PR. Maybe there could be changes made to speedy_af that could move us in that direction without having to do quite as large a refactor? But this effort might not really be worth it given the eventual move to avalon-bundle.


def display_title
Expand Down
32 changes: 32 additions & 0 deletions spec/controllers/supplemental_files_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,38 @@
end
end
end

context 'does not attach' do
let(:supplemental_file) { FactoryBot.build(:supplemental_file) }
before do
allow(SupplementalFile).to receive(:new).and_return(supplemental_file)
allow(supplemental_file).to receive(:attach_file).and_return(nil)
end

it 'does not create a SupplementalFile record' do
expect {
post :create, params: { master_file_id: master_file.id, supplemental_file: valid_create_attributes, format: :json }, session: valid_session
}.not_to change { SupplementalFile.count }#.and_not_to change { master_file.reload.supplemental_files.size }
expect(response).to have_http_status(500)
expect(JSON.parse(response.body)["errors"]).to be_present
end
end

context 'throws error while attaching' do
let(:supplemental_file) { FactoryBot.build(:supplemental_file) }
before do
allow(SupplementalFile).to receive(:new).and_return(supplemental_file)
allow(supplemental_file).to receive(:attach_file).and_raise("ERROR!!!!")
end

it 'does not create a SupplementalFile record' do
expect {
post :create, params: { master_file_id: master_file.id, supplemental_file: valid_create_attributes, format: :json }, session: valid_session
}.not_to change { SupplementalFile.count }#.and_not_to change { master_file.reload.supplemental_files.size }
expect(response).to have_http_status(500)
expect(JSON.parse(response.body)["errors"]).to be_present
end
end
end

describe "PUT #update" do
Expand Down
31 changes: 5 additions & 26 deletions spec/models/master_file_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,29 +322,14 @@
end

describe "#encoder_class" do
subject { FactoryBot.create(:master_file) }

before :all do
class WorkflowEncode < ActiveEncode::Base
end

module EncoderModule
class MyEncoder < ActiveEncode::Base
end
end
end

after :all do
EncoderModule.send(:remove_const, :MyEncoder)
Object.send(:remove_const, :EncoderModule)
Object.send(:remove_const, :WorkflowEncode)
end
subject { FactoryBot.build(:master_file) }

it "should default to WatchedEncode" do
expect(subject.encoder_class).to eq(WatchedEncode)
end

it "should infer the class from a workflow name" do
stub_const("WorkflowEncode", Class.new(ActiveEncode::Base))
subject.workflow_name = 'workflow'
expect(subject.encoder_class).to eq(WorkflowEncode)
end
Expand All @@ -355,6 +340,7 @@ class MyEncoder < ActiveEncode::Base
end

it "should resolve an explicitly named encoder class" do
stub_const("EncoderModule::MyEncoder", Class.new(ActiveEncode::Base))
subject.encoder_classname = 'EncoderModule::MyEncoder'
expect(subject.encoder_class).to eq(EncoderModule::MyEncoder)
end
Expand All @@ -365,6 +351,7 @@ class MyEncoder < ActiveEncode::Base
end

it "should correctly set the encoder classname from the encoder" do
stub_const("EncoderModule::MyEncoder", Class.new(ActiveEncode::Base))
subject.encoder_class = EncoderModule::MyEncoder
expect(subject.encoder_classname).to eq('EncoderModule::MyEncoder')
end
Expand All @@ -374,16 +361,8 @@ class MyEncoder < ActiveEncode::Base
end

context 'with an encoder class named after the engine adapter' do
before :all do
class TestEncode < ActiveEncode::Base
end
end

after :all do
Object.send(:remove_const, :TestEncode)
end

it "should find the encoder class" do
stub_const("TestEncode", Class.new(ActiveEncode::Base))
expect(Settings.encoding.engine_adapter).to eq "test"
expect(subject.encoder_class).to eq(TestEncode)
end
Expand Down
36 changes: 7 additions & 29 deletions spec/presenters/speedy_af/proxy/master_file_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,47 +19,32 @@
subject(:presenter) { described_class.find(master_file.id) }

describe "#encoder_class" do
before :all do
class WorkflowEncode < ActiveEncode::Base
end

module EncoderModule
class MyEncoder < ActiveEncode::Base
end
end
end

after :all do
EncoderModule.send(:remove_const, :MyEncoder)
Object.send(:remove_const, :EncoderModule)
Object.send(:remove_const, :WorkflowEncode)
end

before do
stub_const("MasterFile::WORKFLOWS", ['fullaudio', 'avalon', 'pass_through', 'avalon-skip-transcoding', 'avalon-skip-transcoding-audio', 'workflow', 'nonexistent_workflow_encoder'])
end

it "should default to WatchedEncode" do
expect(subject.encoder_class).to eq(WatchedEncode)
end

context 'with workflow name' do
let(:master_file) { FactoryBot.create(:master_file, workflow_name: 'workflow') }

it "should infer the class from a workflow name" do
stub_const("MasterFile::WORKFLOWS", ['workflow'])
stub_const("WorkflowEncode", Class.new(ActiveEncode::Base))
expect(subject.encoder_class).to eq(WorkflowEncode)
end
end

context 'with bad workflow name' do
let(:master_file) { FactoryBot.create(:master_file, workflow_name: 'nonexistent_workflow_encoder') }
it "should fall back to Watched when a workflow class can't be resolved" do
stub_const("MasterFile::WORKFLOWS", ['nonexistent_workflow_encoder'])
expect(subject.encoder_class).to eq(WatchedEncode)
end
end

context 'with encoder class name' do
let(:master_file) { FactoryBot.create(:master_file, encoder_classname: 'EncoderModule::MyEncoder') }
it "should resolve an explicitly named encoder class" do
stub_const("EncoderModule::MyEncoder", Class.new(ActiveEncode::Base))
expect(subject.encoder_class).to eq(EncoderModule::MyEncoder)
end
end
Expand All @@ -74,21 +59,14 @@ class MyEncoder < ActiveEncode::Base
context 'with bad encoder class name' do
let(:master_file) { FactoryBot.create(:master_file, encoder_class: EncoderModule::MyEncoder) }
it "should correctly set the encoder classname from the encoder" do
stub_const("EncoderModule::MyEncoder", Class.new(ActiveEncode::Base))
expect(subject.encoder_classname).to eq('EncoderModule::MyEncoder')
end
end

context 'with an encoder class named after the engine adapter' do
before :all do
class TestEncode < ActiveEncode::Base
end
end

after :all do
Object.send(:remove_const, :TestEncode)
end

it "should find the encoder class" do
stub_const("TestEncode", Class.new(ActiveEncode::Base))
expect(Settings.encoding.engine_adapter).to eq "test"
expect(subject.encoder_class).to eq(TestEncode)
end
Expand Down