diff --git a/app/controllers/files_controller.rb b/app/controllers/files_controller.rb index bbfc2cc42f9..5020882d827 100644 --- a/app/controllers/files_controller.rb +++ b/app/controllers/files_controller.rb @@ -626,12 +626,12 @@ class FilesController < ApplicationController redirect_to(named_context_url(@context, :context_file_url, attachment.id)) end else - send_stored_file(attachment, false, true) + send_stored_file(attachment, false) end end protected :send_attachment - def send_stored_file(attachment, inline=true, redirect_to_s3=false) + def send_stored_file(attachment, inline=true) user = @current_user user ||= api_find(User, params[:user_id]) if params[:user_id].present? attachment.context_module_action(user, :read) if user && !params[:preview] @@ -639,8 +639,7 @@ class FilesController < ApplicationController render_or_redirect_to_stored_file( attachment: attachment, verifier: params[:verifier], - inline: inline, - redirect_to_s3: redirect_to_s3 + inline: inline ) end protected :send_stored_file diff --git a/app/controllers/lti/submissions_api_controller.rb b/app/controllers/lti/submissions_api_controller.rb index 4e0f9731017..7b955260037 100644 --- a/app/controllers/lti/submissions_api_controller.rb +++ b/app/controllers/lti/submissions_api_controller.rb @@ -173,7 +173,7 @@ module Lti attachment = Attachment.find(params[:attachment_id]) render_unauthorized and return unless attachment_for_submission?(attachment) render_or_redirect_to_stored_file( - attachment: attachment, redirect_to_s3: true) + attachment: attachment) end diff --git a/app/helpers/attachment_helper.rb b/app/helpers/attachment_helper.rb index f96fa679788..88e975f3a1a 100644 --- a/app/helpers/attachment_helper.rb +++ b/app/helpers/attachment_helper.rb @@ -57,18 +57,15 @@ module AttachmentHelper } end - def render_or_redirect_to_stored_file(attachment:, verifier: nil, inline: false, redirect_to_s3: false) + def render_or_redirect_to_stored_file(attachment:, verifier: nil, inline: false) set_cache_header(attachment) if safer_domain_available? redirect_to safe_domain_file_url(attachment, @safer_domain_host, verifier, !inline) elsif Attachment.local_storage? @headers = false if @files_domain send_file(attachment.full_filename, :type => attachment.content_type_with_encoding, :disposition => (inline ? 'inline' : 'attachment'), :filename => attachment.display_name) - elsif redirect_to_s3 - redirect_to(inline ? attachment.inline_url : attachment.download_url) else - send_file_headers!( :length=> attachment.s3object.content_length, :filename=>attachment.filename, :disposition => 'inline', :type => attachment.content_type_with_encoding) - render :status => 200, :text => attachment.s3object.get.body.read + redirect_to(inline ? attachment.inline_url : attachment.download_url) end end diff --git a/spec/controllers/files_controller_spec.rb b/spec/controllers/files_controller_spec.rb index b391b827df8..94c5000b5b0 100644 --- a/spec/controllers/files_controller_spec.rb +++ b/spec/controllers/files_controller_spec.rb @@ -294,7 +294,7 @@ describe FilesController do it "should force download when download_frd is set" do user_session(@teacher) # this call should happen inside of FilesController#send_attachment - FilesController.any_instance.expects(:send_stored_file).with(@file, false, true) + FilesController.any_instance.expects(:send_stored_file).with(@file, false) get 'show', :course_id => @course.id, :id => @file.id, :download => 1, :verifier => @file.uuid, :download_frd => 1 end diff --git a/spec/integration/files_spec.rb b/spec/integration/files_spec.rb index 495493a08dd..785525beecd 100644 --- a/spec/integration/files_spec.rb +++ b/spec/integration/files_spec.rb @@ -48,8 +48,8 @@ describe FilesController do remove_user_session get location - expect(response).to be_success - expect(response.content_type).to eq 'image/png' + # could be success or redirect, depending on S3 config + expect([200, 302]).to be_include(response.status) # ensure that the user wasn't logged in by the normal means expect(controller.instance_variable_get(:@current_user)).to be_nil end @@ -57,8 +57,8 @@ describe FilesController do it "without safefiles" do HostUrl.stubs(:file_host_with_shard).returns(['test.host', Shard.default]) get "http://test.host/files/#{@submission.attachment.id}/download", :inline => '1', :verifier => @submission.attachment.uuid - expect(response).to be_success - expect(response.content_type).to eq 'image/png' + # could be success or redirect, depending on S3 config + expect([200, 302]).to be_include(response.status) expect(response['Pragma']).to be_nil expect(response['Cache-Control']).not_to match(/no-cache/) end @@ -164,8 +164,8 @@ describe FilesController do remove_user_session get location - expect(response).to be_success - expect(response.content_type).to eq 'image/png' + # could be success or redirect, depending on S3 config + expect([200, 302]).to be_include(response.status) # ensure that the user wasn't logged in by the normal means expect(controller.instance_variable_get(:@current_user)).to be_nil end @@ -258,7 +258,8 @@ describe FilesController do # and verify the module progress was recorded remove_user_session get location - expect(response).to be_success + # could be success or redirect, depending on S3 config + expect([200, 302]).to be_include(response.status) expect(@module.evaluate_for(@user).state).to eql(:completed) end diff --git a/spec/selenium/grades/speedgrader/speedgrader_teacher_submission_spec.rb b/spec/selenium/grades/speedgrader/speedgrader_teacher_submission_spec.rb index 5f533fb964d..53f4a0f79ad 100644 --- a/spec/selenium/grades/speedgrader/speedgrader_teacher_submission_spec.rb +++ b/spec/selenium/grades/speedgrader/speedgrader_teacher_submission_spec.rb @@ -240,9 +240,11 @@ describe "speed grader submissions" do # validates the image\attachment is inside the iframe as expected image_element = f('img') - expect(image_element.attribute('src')).to include('download') - expect(image_element.attribute('style')).to include('max-height: 100') - expect(image_element.attribute('style')).to include('max-width: 100') + if Attachment.local_storage? + expect(image_element.attribute('src')).to include('download') + else + expect(image_element.attribute('src')).to include('amazonaws') + end end end