From 43ba31a048bd2a6b19693fd4fac30a279059f280 Mon Sep 17 00:00:00 2001
From: James Mead <james@floehopper.org>
Date: Mon, 26 Feb 2018 16:23:17 +0000
Subject: [PATCH 1/7] Add Asset#replacement association

When a Whitehall attachment is "replaced", requests for its URL result
in a redirect to the attachment which replaced it. We're going to need
to implement this functionality in Asset Manager in order to serve
Whitehall attachments without changing the externally visible behaviour.
This is a first step in that direction.

Note that as of Rails v5, a belongs_to association is required by
default. Hence why I've had to specify the `optional: true` option.

I didn't see any point in implementing the inverse relation at this
point, because it's not obvious we're going to need it.

Note that some of the model examples I've added to the Asset spec may
not be needed, but I wanted to make sure I understood how this
association was persisted by Mongoid.
---
 app/models/asset.rb       |  2 ++
 spec/models/asset_spec.rb | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/app/models/asset.rb b/app/models/asset.rb
index 8533fbd6..a2ef8eed 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=
diff --git a/spec/models/asset_spec.rb b/spec/models/asset_spec.rb
index 35cb149e..af601bf3 100644
--- a/spec/models/asset_spec.rb
+++ b/spec/models/asset_spec.rb
@@ -845,4 +845,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

From 2f49103617ab06a1ad476a3517c19e99b20eb5bf Mon Sep 17 00:00:00 2001
From: James Mead <james@floehopper.org>
Date: Mon, 26 Feb 2018 17:22:53 +0000
Subject: [PATCH 2/7] Redirect asset request to replacement if asset has one

This implements this bit of behaviour [1] in Whitehall's
AttachmentsController#fail method.

Note that I plan to address the call to `expires_headers` in a
subsequent commit.

Note also that I've implemented this for both Whitehall and Mainstream
assets for completeness, even though the immediate requirement is only
to do this for Whitehall assets.

[1]:
https://github.com/alphagov/whitehall/blob/d32f0b1a3fa331c32dd92bd61174900026a0e41d/app/controllers/attachments_controller.rb#L45-L47
---
 app/controllers/base_media_controller.rb      |  4 ++++
 app/controllers/media_controller.rb           |  5 +++++
 app/controllers/whitehall_media_controller.rb |  5 +++++
 spec/controllers/media_controller_spec.rb     | 17 +++++++++++++++
 .../whitehall_media_controller_spec.rb        | 21 +++++++++++++++++++
 5 files changed, 52 insertions(+)

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..8e37da20 100644
--- a/app/controllers/media_controller.rb
+++ b/app/controllers/media_controller.rb
@@ -25,6 +25,11 @@ def download
       return
     end
 
+    if asset.replacement.present?
+      redirect_to_replacement_for(asset)
+      return
+    end
+
     respond_to do |format|
       format.any do
         set_expiry(cache_control)
diff --git a/app/controllers/whitehall_media_controller.rb b/app/controllers/whitehall_media_controller.rb
index 3466f9e8..825d9646 100644
--- a/app/controllers/whitehall_media_controller.rb
+++ b/app/controllers/whitehall_media_controller.rb
@@ -20,6 +20,11 @@ def download
       return
     end
 
+    if asset.replacement.present?
+      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/spec/controllers/media_controller_spec.rb b/spec/controllers/media_controller_spec.rb
index 622df154..63a35b10 100644
--- a/spec/controllers/media_controller_spec.rb
+++ b/spec/controllers/media_controller_spec.rb
@@ -176,5 +176,22 @@
         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
+    end
   end
 end
diff --git a/spec/controllers/whitehall_media_controller_spec.rb b/spec/controllers/whitehall_media_controller_spec.rb
index fe78e58d..8de8e1e4 100644
--- a/spec/controllers/whitehall_media_controller_spec.rb
+++ b/spec/controllers/whitehall_media_controller_spec.rb
@@ -116,6 +116,27 @@
       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
+    end
+
     context 'when asset is draft and access limited' do
       let(:user) { FactoryBot.build(:user) }
       let(:state) { 'uploaded' }

From f6c5f929154fb296b1ad74ecf813648848791f27 Mon Sep 17 00:00:00 2001
From: James Mead <james@floehopper.org>
Date: Tue, 27 Feb 2018 11:01:10 +0000
Subject: [PATCH 3/7] Set Cache-Control header for replacement redirect

The header now set in WhitehallMediaController#download is intended to
match that set in Whitehall's AttachmentsController#fail method [1] for
anonymous users. I haven't addressed the case where the user is signed
in, because this is a more general problem which I've captured in this
GitHub issue [2].

I've done something similar in MediaController#download, but using the
default expiry for Mainstream assets for consistency. Hopefully we can
bring these two values into line in the not too distant future.

[1]:
https://github.com/alphagov/whitehall/blob/446e60b80fee198540e5abe3a7077a5a7f5f63e5/app/controllers/attachments_controller.rb#L46
[2]: https://github.com/alphagov/asset-manager/issues/494
---
 app/controllers/media_controller.rb                 | 1 +
 app/controllers/whitehall_media_controller.rb       | 1 +
 spec/controllers/media_controller_spec.rb           | 6 ++++++
 spec/controllers/whitehall_media_controller_spec.rb | 6 ++++++
 4 files changed, 14 insertions(+)

diff --git a/app/controllers/media_controller.rb b/app/controllers/media_controller.rb
index 8e37da20..fee0fcdf 100644
--- a/app/controllers/media_controller.rb
+++ b/app/controllers/media_controller.rb
@@ -26,6 +26,7 @@ def download
     end
 
     if asset.replacement.present?
+      set_expiry(cache_control)
       redirect_to_replacement_for(asset)
       return
     end
diff --git a/app/controllers/whitehall_media_controller.rb b/app/controllers/whitehall_media_controller.rb
index 825d9646..cf81cb73 100644
--- a/app/controllers/whitehall_media_controller.rb
+++ b/app/controllers/whitehall_media_controller.rb
@@ -21,6 +21,7 @@ def download
     end
 
     if asset.replacement.present?
+      set_expiry(cache_control)
       redirect_to_replacement_for(asset)
       return
     end
diff --git a/spec/controllers/media_controller_spec.rb b/spec/controllers/media_controller_spec.rb
index 63a35b10..f0cc0f3a 100644
--- a/spec/controllers/media_controller_spec.rb
+++ b/spec/controllers/media_controller_spec.rb
@@ -192,6 +192,12 @@
 
         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_media_controller_spec.rb b/spec/controllers/whitehall_media_controller_spec.rb
index 8de8e1e4..386a4f4d 100644
--- a/spec/controllers/whitehall_media_controller_spec.rb
+++ b/spec/controllers/whitehall_media_controller_spec.rb
@@ -135,6 +135,12 @@
 
         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

From 8e1fedfbb4001c5d3c297fb5fa7e12363a91df4a Mon Sep 17 00:00:00 2001
From: James Mead <james@floehopper.org>
Date: Mon, 26 Feb 2018 16:33:23 +0000
Subject: [PATCH 4/7] Include replacement_id in JSON response for GET API
 endpoint

If asset has a replacement then include its ID in the JSON response. The
convention seems to be to include such attributes in the response by
default; the only exception being if there is some kind of security
consideration. In this case it seems fine to include the new attribute.
---
 app/presenters/asset_presenter.rb       |  3 +++
 spec/presenters/asset_presenter_spec.rb | 23 +++++++++++++++++++++++
 2 files changed, 26 insertions(+)

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/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

From 32cda8a3b50a31c35e0c9485df323d0daf446d19 Mon Sep 17 00:00:00 2001
From: James Mead <james@floehopper.org>
Date: Mon, 26 Feb 2018 16:34:28 +0000
Subject: [PATCH 5/7] Allow replacement_id to be specified via the API

This commit makes it possible to specify a replacement_id when creating
or updating an asset.

Although the most likely use case is when *updating* an existing asset,
I've also allowed it when creating a new asset for completeness.

Although the immediate need for this if only for Whitehall assets, I've
also allowed it for Mainstream assets, because it seems generally
useful. Note that I suspect we could probably replace the
MediaController#redirect_to_current_filename behaviour with this more
general mechanism at some point.
---
 app/controllers/assets_controller.rb          |  2 +-
 .../whitehall_assets_controller.rb            |  2 +-
 spec/controllers/assets_controller_spec.rb    | 53 +++++++++++++++++++
 .../whitehall_assets_controller_spec.rb       |  8 +++
 4 files changed, 63 insertions(+), 2 deletions(-)

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/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/spec/controllers/assets_controller_spec.rb b/spec/controllers/assets_controller_spec.rb
index cada5fc8..ea9e40be 100644
--- a/spec/controllers/assets_controller_spec.rb
+++ b/spec/controllers/assets_controller_spec.rb
@@ -107,6 +107,42 @@
         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
+
+      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 +186,23 @@
         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 success status' do
         put :update, params: { id: asset.id, asset: valid_attributes }
 
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 }
 

From 68e57766d63a9cb0087285bfe1a69b2357efcd0e Mon Sep 17 00:00:00 2001
From: James Mead <james@floehopper.org>
Date: Tue, 27 Feb 2018 12:33:20 +0000
Subject: [PATCH 6/7] DRY up asset validation examples in Asset spec

---
 spec/models/asset_spec.rb | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/spec/models/asset_spec.rb b/spec/models/asset_spec.rb
index af601bf3..20932e7a 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

From 4f00fd6f0afc881ab6ea84e5056ac9a45ad4ccd9 Mon Sep 17 00:00:00 2001
From: James Mead <james@floehopper.org>
Date: Tue, 27 Feb 2018 12:40:16 +0000
Subject: [PATCH 7/7] Add validation for Asset#replacement

If Asset#replacement_id is specified, it should match an existing asset,
otherwise we should fail fast with a 422 Unprocessable Entity response.
---
 app/models/asset.rb                        |  8 +++++
 spec/controllers/assets_controller_spec.rb | 36 ++++++++++++++++++++++
 spec/models/asset_spec.rb                  | 34 ++++++++++++++++++++
 3 files changed, 78 insertions(+)

diff --git a/app/models/asset.rb b/app/models/asset.rb
index a2ef8eed..09c281b3 100644
--- a/app/models/asset.rb
+++ b/app/models/asset.rb
@@ -41,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?
@@ -154,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/spec/controllers/assets_controller_spec.rb b/spec/controllers/assets_controller_spec.rb
index ea9e40be..c30e3842 100644
--- a/spec/controllers/assets_controller_spec.rb
+++ b/spec/controllers/assets_controller_spec.rb
@@ -135,6 +135,24 @@
         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 }
 
@@ -203,6 +221,24 @@
         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/models/asset_spec.rb b/spec/models/asset_spec.rb
index 20932e7a..2155db09 100644
--- a/spec/models/asset_spec.rb
+++ b/spec/models/asset_spec.rb
@@ -27,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