From c3e3ca4126b121ff4e6c4a70c652de6992cb15e5 Mon Sep 17 00:00:00 2001 From: Andromeda Yelton Date: Sat, 16 Nov 2019 13:15:28 -0500 Subject: [PATCH 1/2] Update deprecated render option :text was removed in 5.1; calling render :text causes this function to fail, meaning files can't be downloaded. render :plain restores the desired behavior. --- app/controllers/files_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/files_controller.rb b/app/controllers/files_controller.rb index 5f89d352d..634305428 100644 --- a/app/controllers/files_controller.rb +++ b/app/controllers/files_controller.rb @@ -4,7 +4,7 @@ class FilesController < ApplicationController def show @upload = FileUpload.where(id: params[:id]).first if params_match? && viewing_allowed? - render text: File.read(file_path), content_type: @upload.file_content_type + render plain: File.read(file_path), content_type: @upload.file_content_type else redirect_to( root_path, From f5e229b4e51c7e2e550fb8fde872fbdf13e676c0 Mon Sep 17 00:00:00 2001 From: Andromeda Yelton Date: Sat, 16 Nov 2019 13:23:54 -0500 Subject: [PATCH 2/2] Add regresssion testing expect_download didn't check for the http status code, so the test could succeed even when the download failed. This allowed us to push broken code to prod even though the tests exercised the functionality in question. Let's fix that! :D --- spec/integration/downloading_documents.rb | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/spec/integration/downloading_documents.rb b/spec/integration/downloading_documents.rb index cf377c1db..7f9f70158 100644 --- a/spec/integration/downloading_documents.rb +++ b/spec/integration/downloading_documents.rb @@ -36,7 +36,7 @@ expect_download end - scenario 'won\'t download a document for a guest user with an expired access token', js: true do + scenario "won't download a document for a guest user with an expired access token", js: true do token_url = create( :token_url, notice: notice, @@ -48,7 +48,7 @@ expect_no_download end - scenario 'won\'t download a document for a logged-in user with no access', js: true do + scenario "won't download a document for a logged-in user with no access", js: true do sign_in(create(:user, :researcher)) visit notice.file_uploads.first.url @@ -56,7 +56,7 @@ expect_no_download end - scenario 'won\'t download a document for a guest user', js: true do + scenario "won't download a document for a guest user", js: true do visit notice.file_uploads.first.url expect_no_download @@ -70,7 +70,7 @@ expect(page).to have_content('Documents notification has been disabled.') end - scenario 'won\'t disable a document notification for a user with a wrong token', js: true do + scenario "won't disable a document notification for a user with a wrong token", js: true do visit disable_documents_notification_token_url_path(token_url, token: 'hacky-token') expect(TokenUrl.last.documents_notification).to eq(true) @@ -78,7 +78,7 @@ expect(page).to have_content('Wrong token provided.') end - scenario 'won\'t disable a document notification for a non-exisiting token url', js: true do + scenario "won't disable a document notification for a non-exisiting token url", js: true do token_url.destroy visit disable_documents_notification_token_url_path(token_url, token: 'hohoho') @@ -91,6 +91,7 @@ def expect_download expect(page.response_headers['Content-Type']).to include('application/pdf') + expect(page.status_code).to be 200 end def expect_no_download