diff --git a/app/controllers/assets_controller.rb b/app/controllers/assets_controller.rb index fa93e2d0..ec61074d 100644 --- a/app/controllers/assets_controller.rb +++ b/app/controllers/assets_controller.rb @@ -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 diff --git a/app/controllers/base_media_controller.rb b/app/controllers/base_media_controller.rb index 74849cc3..1a0a702c 100644 --- a/app/controllers/base_media_controller.rb +++ b/app/controllers/base_media_controller.rb @@ -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 diff --git a/app/controllers/media_controller.rb b/app/controllers/media_controller.rb index bafdef45..fee0fcdf 100644 --- a/app/controllers/media_controller.rb +++ b/app/controllers/media_controller.rb @@ -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) diff --git a/app/controllers/whitehall_assets_controller.rb b/app/controllers/whitehall_assets_controller.rb index 5a4339ae..5ae8666e 100644 --- a/app/controllers/whitehall_assets_controller.rb +++ b/app/controllers/whitehall_assets_controller.rb @@ -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: [] ) diff --git a/app/controllers/whitehall_media_controller.rb b/app/controllers/whitehall_media_controller.rb index 3466f9e8..cf81cb73 100644 --- a/app/controllers/whitehall_media_controller.rb +++ b/app/controllers/whitehall_media_controller.rb @@ -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? diff --git a/app/models/asset.rb b/app/models/asset.rb index 8533fbd6..09c281b3 100644 --- a/app/models/asset.rb +++ b/app/models/asset.rb @@ -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= @@ -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? @@ -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 diff --git a/app/presenters/asset_presenter.rb b/app/presenters/asset_presenter.rb index 726a8bd7..90bbeb89 100644 --- a/app/presenters/asset_presenter.rb +++ b/app/presenters/asset_presenter.rb @@ -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 diff --git a/spec/controllers/assets_controller_spec.rb b/spec/controllers/assets_controller_spec.rb index cada5fc8..c30e3842 100644 --- a/spec/controllers/assets_controller_spec.rb +++ b/spec/controllers/assets_controller_spec.rb @@ -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 @@ -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 } diff --git a/spec/controllers/media_controller_spec.rb b/spec/controllers/media_controller_spec.rb index 622df154..f0cc0f3a 100644 --- a/spec/controllers/media_controller_spec.rb +++ b/spec/controllers/media_controller_spec.rb @@ -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 diff --git a/spec/controllers/whitehall_assets_controller_spec.rb b/spec/controllers/whitehall_assets_controller_spec.rb index 1c6f9119..c88b5a6c 100644 --- a/spec/controllers/whitehall_assets_controller_spec.rb +++ b/spec/controllers/whitehall_assets_controller_spec.rb @@ -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 } diff --git a/spec/controllers/whitehall_media_controller_spec.rb b/spec/controllers/whitehall_media_controller_spec.rb index fe78e58d..386a4f4d 100644 --- a/spec/controllers/whitehall_media_controller_spec.rb +++ b/spec/controllers/whitehall_media_controller_spec.rb @@ -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' } diff --git a/spec/models/asset_spec.rb b/spec/models/asset_spec.rb index 35cb149e..2155db09 100644 --- a/spec/models/asset_spec.rb +++ b/spec/models/asset_spec.rb @@ -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 @@ -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 @@ -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 diff --git a/spec/presenters/asset_presenter_spec.rb b/spec/presenters/asset_presenter_spec.rb index 5dd81dc2..1e2c29c8 100644 --- a/spec/presenters/asset_presenter_spec.rb +++ b/spec/presenters/asset_presenter_spec.rb @@ -113,5 +113,28 @@ expect(json).to have_key(:redirect_url) end end + + context 'when asset has no replacement' do + before do + asset.replacement = nil + end + + it 'returns hash without replacement_id' do + expect(json).not_to have_key(:replacement_id) + end + end + + context 'when asset has replacement' do + let(:replacement) { FactoryBot.create(:asset) } + let(:replacement_id) { replacement.id.to_s } + + before do + asset.replacement = replacement + end + + it 'returns hash including replacement_id' do + expect(json).to include(replacement_id: replacement_id) + end + end end end