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.

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

Allow asset to be replaced with another asset #499

Merged
merged 7 commits into from
Feb 27, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion app/controllers/assets_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def asset_params
exclude_blank_redirect_url(
params
.require(:asset)
.permit(:file, :draft, :redirect_url, access_limited: [])
.permit(:file, :draft, :redirect_url, :replacement_id, access_limited: [])
)
end

Expand Down
4 changes: 4 additions & 0 deletions app/controllers/base_media_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,8 @@ def redirect_to_draft_assets_host_for?(asset)
def redirect_to_draft_assets_host
redirect_to host: AssetManager.govuk.draft_assets_host, format: params[:format]
end

def redirect_to_replacement_for(asset)
redirect_to asset.replacement.public_url_path, status: :moved_permanently
end
end
6 changes: 6 additions & 0 deletions app/controllers/media_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ def download
return
end

if asset.replacement.present?
set_expiry(cache_control)
redirect_to_replacement_for(asset)
return
end

respond_to do |format|
format.any do
set_expiry(cache_control)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/whitehall_assets_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def asset_params
params
.require(:asset)
.permit(
:file, :draft, :redirect_url,
:file, :draft, :redirect_url, :replacement_id,
:legacy_url_path, :legacy_etag, :legacy_last_modified,
access_limited: []
)
Expand Down
6 changes: 6 additions & 0 deletions app/controllers/whitehall_media_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ def download
return
end

if asset.replacement.present?
set_expiry(cache_control)
redirect_to_replacement_for(asset)
return
end

if asset.unscanned? || asset.clean?
set_expiry(cache_control.expires_in(1.minute))
if asset.image?
Expand Down
10 changes: 10 additions & 0 deletions app/models/asset.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ class Asset
include Mongoid::Paranoia
include Mongoid::Timestamps

belongs_to :replacement, class_name: 'Asset', optional: true

field :state, type: String, default: 'unscanned'
field :filename_history, type: Array, default: -> { [] }
protected :filename_history=
Expand Down Expand Up @@ -39,6 +41,8 @@ class Asset
message: 'must match the format defined in rfc4122'
}

validate :check_specified_replacement_exists

mount_uploader :file, AssetUploader

before_save :store_metadata, unless: :uploaded?
Expand Down Expand Up @@ -152,4 +156,10 @@ def schedule_virus_scan
def file_stat
File.stat(file.path)
end

def check_specified_replacement_exists
if replacement_id.present? && replacement.blank?
errors.add(:replacement, 'not found')
end
end
end
3 changes: 3 additions & 0 deletions app/presenters/asset_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ def as_json(options = {})
if @asset.redirect_url.present?
json[:redirect_url] = @asset.redirect_url
end
if @asset.replacement.present?
json[:replacement_id] = @asset.replacement_id.to_s
end
json
end
end
89 changes: 89 additions & 0 deletions spec/controllers/assets_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,60 @@
end
end
end

context 'when attributes include a replacement_id' do
let(:replacement) { FactoryBot.create(:asset) }
let(:replacement_id) { replacement.id.to_s }
let(:attributes) { valid_attributes.merge(replacement_id: replacement_id) }

it 'stores replacement asset' do
post :create, params: { asset: attributes }

expect(assigns(:asset).replacement).to eq(replacement)
end

context 'and replacement_id is blank' do
let(:replacement_id) { '' }

it 'stores no replacement' do
post :create, params: { asset: attributes }

expect(assigns(:asset).replacement).to be_blank
end

it 'stores replacement_id as nil' do
post :create, params: { asset: attributes }

expect(assigns(:asset).replacement_id).to be_nil
end
end

context 'and replacement_id does not match an existing asset' do
let(:replacement_id) { 'non-existent-asset-id' }

it 'responds with unprocessable entity status' do
post :create, params: { asset: attributes }

expect(response).to have_http_status(:unprocessable_entity)
end

it 'includes error message in response' do
post :create, params: { asset: attributes }

body = JSON.parse(response.body)
status = body['_response_info']['status']
expect(status).to include('Replacement not found')
end
end

it 'includes the replacement_id in the response' do
post :create, params: { asset: attributes }

body = JSON.parse(response.body)

expect(body['replacement_id']).to eq(replacement_id)
end
end
end

describe 'PUT update' do
Expand Down Expand Up @@ -150,6 +204,41 @@
expect(assigns(:asset).redirect_url).to be_nil
end

it 'stores replacement on existing asset' do
replacement = FactoryBot.create(:asset)
replacement_id = replacement.id.to_s
attributes = valid_attributes.merge(replacement_id: replacement_id)
put :update, params: { id: asset.id, asset: attributes }

expect(assigns(:asset).replacement).to eq(replacement)
end

it 'stores replacement_id as nil if replacement_id is blank' do
replacement_id = ''
attributes = valid_attributes.merge(replacement_id: replacement_id)
put :update, params: { id: asset.id, asset: attributes }

expect(assigns(:asset).replacement_id).to be_nil
end

it 'responds with unprocessable entity status if replacement is not found' do
replacement_id = 'non-existent-asset-id'
attributes = valid_attributes.merge(replacement_id: replacement_id)
put :update, params: { id: asset.id, asset: attributes }

expect(response).to have_http_status(:unprocessable_entity)
end

it 'includes error message in response if replacement is not found' do
replacement_id = 'non-existent-asset-id'
attributes = valid_attributes.merge(replacement_id: replacement_id)
put :update, params: { id: asset.id, asset: attributes }

body = JSON.parse(response.body)
status = body['_response_info']['status']
expect(status).to include('Replacement not found')
end

it 'responds with success status' do
put :update, params: { id: asset.id, asset: valid_attributes }

Expand Down
23 changes: 23 additions & 0 deletions spec/controllers/media_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -176,5 +176,28 @@
expect(response).to redirect_to(redirect_url)
end
end

context 'when asset has a replacement' do
let(:replacement) { FactoryBot.create(:uploaded_asset) }
let(:asset) { FactoryBot.create(:uploaded_asset, replacement: replacement) }

it 'redirects to replacement for asset' do
get :download, params

expect(response).to redirect_to(replacement.public_url_path)
end

it 'responds with 301 moved permanently status' do
get :download, params

expect(response).to have_http_status(:moved_permanently)
end

it 'sets the Cache-Control response header to 24 hours' do
get :download, params

expect(response.headers['Cache-Control']).to eq('max-age=86400, public')
end
end
end
end
8 changes: 8 additions & 0 deletions spec/controllers/whitehall_assets_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@
expect(assigns(:asset).access_limited).to eq(['user-id'])
end

it "stores replacement on asset" do
replacement = FactoryBot.create(:asset)
replacement_id = replacement.id.to_s
post :create, params: { asset: { replacement_id: replacement_id } }

expect(assigns(:asset).replacement).to eq(replacement)
end

it "returns a created status" do
post :create, params: { asset: attributes }

Expand Down
27 changes: 27 additions & 0 deletions spec/controllers/whitehall_media_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,33 @@
end
end

context 'when asset has a replacement' do
let(:state) { 'uploaded' }
let(:replacement) { FactoryBot.create(:uploaded_asset) }

before do
asset.replacement = replacement
end

it 'redirects to replacement for asset' do
get :download, params: { path: path, format: format }

expect(response).to redirect_to(replacement.public_url_path)
end

it 'responds with 301 moved permanently status' do
get :download, params: { path: path, format: format }

expect(response).to have_http_status(:moved_permanently)
end

it 'sets the Cache-Control response header to 4 hours' do
get :download, params: { path: path, format: format }

expect(response.headers['Cache-Control']).to eq('max-age=14400, public')
end
end

context 'when asset is draft and access limited' do
let(:user) { FactoryBot.build(:user) }
let(:state) { 'uploaded' }
Expand Down
76 changes: 74 additions & 2 deletions spec/models/asset_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,16 @@

RSpec.describe Asset, type: :model do
describe 'validation' do
subject(:asset) { FactoryBot.build(:asset) }
subject(:asset) { FactoryBot.build(:asset, attributes) }

let(:attributes) { {} }

it 'is valid when built from factory' do
expect(asset).to be_valid
end

context 'when file is not specified' do
subject(:asset) { FactoryBot.build(:asset, file: nil) }
let(:attributes) { { file: nil } }

it 'is not valid' do
expect(asset).not_to be_valid
Expand All @@ -25,6 +27,40 @@
end
end
end

context 'when replacement_id is not specified' do
let(:attributes) { { replacement_id: nil } }

it 'is valid' do
expect(asset).to be_valid
end
end

context 'when replacement_id is specified' do
let(:attributes) { { replacement_id: replacement_id } }

context 'and replacement asset exists' do
let(:replacement) { FactoryBot.create(:asset) }
let(:replacement_id) { replacement.id.to_s }

it 'is valid' do
expect(asset).to be_valid
end
end

context 'and replacement asset does not exist' do
let(:replacement_id) { 'non-existent-asset-id' }

it 'is not valid' do
expect(asset).not_to be_valid
end

it 'includes error for replacement not found' do
asset.valid?
expect(asset.errors[:replacement]).to include('not found')
end
end
end
end

describe 'creation' do
Expand Down Expand Up @@ -845,4 +881,40 @@
end
end
end

describe '#replacement' do
let(:asset) { FactoryBot.build(:asset, replacement: replacement) }

context 'when replacement is nil' do
let(:replacement) { nil }

it 'is valid' do
expect(asset).to be_valid
end

it 'has no replacement_id' do
expect(asset.replacement_id).to be_nil
end
end

context 'when replacement is set' do
let(:replacement) { FactoryBot.create(:asset) }

it 'is valid' do
expect(asset).to be_valid
end

it 'persists replacement when saved' do
asset.save!

expect(asset.reload.replacement).to eq(replacement)
end

it 'persists replacement_id when saved' do
asset.save!

expect(asset.reload.replacement_id).to eq(replacement.id)
end
end
end
end
Loading