Sync this repo with RuboCop GOV.UK

.rubocop.yml

@@ -1,40 +1,17 @@
-require: rubocop-rspec
 inherit_gem:
   rubocop-govuk:
     - "config/default.yml"
     - "config/rails.yml"
-
-inherit_from: .rubocop_todo.yml
+    - "config/rspec.yml"
 
 inherit_mode:
   merge:
     - Exclude
 
-RSpec/NestedGroups:
-  Max: 4
-
-RSpec/MultipleExpectations:
-  Max: 10
-
-RSpec/ExampleLength:
-  Enabled: false
-
-# This recently introduced cop seems to have stirred up some controversy
-Style/AccessModifierDeclarations:
-  Enabled: false
-
-Layout/AccessModifierIndentation:
-  Enabled: false
-
-RSpec/ContextWording:
-  Enabled: false
-
-Naming/MemoizedInstanceVariableName:
-  Enabled: false
-
-# Mongoid doesn't support some of the methods that Rubocop prefers
+# This isn't supported by Mongoid
 Rails/FindBy:
   Enabled: false
 
+# This isn't supported by Mongoid
 Rails/FindEach:
   Enabled: false

app/controllers/assets_controller.rb

@@ -13,22 +13,14 @@ def update
 
   def destroy
     @asset = find_asset
-
-    if @asset.destroy
-      render json: AssetPresenter.new(@asset, view_context).as_json(status: :success)
-    else
-      error 422, @asset.errors.full_messages
-    end
+    @asset.destroy
+    render json: AssetPresenter.new(@asset, view_context).as_json(status: :success)
   end
 
   def restore
     @asset = find_asset(include_deleted: true)
-
-    if @asset.restore
-      render json: AssetPresenter.new(@asset, view_context).as_json(status: :success)
-    else
-      error 422, @asset.errors.full_messages
-    end
+    @asset.restore
+    render json: AssetPresenter.new(@asset, view_context).as_json(status: :success)
   end
 
 private

app/models/asset.rb

@@ -146,7 +146,7 @@ def last_modified_from_file
   end
 
   def md5_hexdigest_from_file
-    @md5_hexdigest ||= Digest::MD5.hexdigest(file.file.read)
+    @md5_hexdigest_from_file ||= Digest::MD5.hexdigest(file.file.read)
   end
 
   def size_from_file
@@ -178,11 +178,11 @@ def backpropagate_replacement
   # can be used with 'deleted' and 'undeleted' scopes.
   #
   def destroy(_options = nil)
-    update(deleted_at: Time.zone.now)
+    update!(deleted_at: Time.zone.now)
   end
 
   def restore
-    update(deleted_at: nil)
+    update!(deleted_at: nil)
   end
 
   def deleted?

spec/controllers/assets_controller_spec.rb

@@ -206,7 +206,7 @@
   end
 
   describe "PUT update" do
-    context "an existing asset" do
+    context "with an existing asset" do
       let(:asset) { FactoryBot.create(:asset) }
       let(:file) { load_fixture_file("asset2.jpg") }
       let(:valid_attributes) { { file: file } }
@@ -390,7 +390,7 @@
   end
 
   describe "DELETE destroy" do
-    context "an existing asset" do
+    context "with an existing asset" do
       let(:asset) { FactoryBot.create(:asset) }
 
       it "deletes the asset" do
@@ -404,28 +404,9 @@
 
         expect(response).to have_http_status(:success)
       end
-
-      context "when Asset#destroy fails" do
-        let(:errors) { ActiveModel::Errors.new(asset) }
-
-        before do
-          errors.add(:base, "Something went wrong")
-          allow_any_instance_of(Asset).to receive(:destroy).and_return(false)
-          allow_any_instance_of(Asset).to receive(:errors).and_return(errors)
-          delete :destroy, params: { id: asset.id }
-        end
-
-        it "responds with unprocessable entity status" do
-          expect(response).to have_http_status(:unprocessable_entity)
-        end
-
-        it "includes the errors in the response" do
-          expect(response.body).to match(/Something went wrong/)
-        end
-      end
     end
 
-    context "no existing asset" do
+    context "with no existing asset" do
       it "responds with not found status" do
         delete :destroy, params: { id: "12345" }
         expect(response).to have_http_status(:not_found)
@@ -434,7 +415,7 @@
   end
 
   describe "GET show" do
-    context "an asset which exists" do
+    context "with an asset which exists" do
       let(:asset) { FactoryBot.create(:asset) }
 
       it "responds with success status" do
@@ -465,7 +446,7 @@
       end
     end
 
-    context "an asset that has been deleted" do
+    context "with an asset that has been deleted" do
       let(:asset) { FactoryBot.create(:deleted_asset) }
 
       it "responds with success status" do
@@ -475,7 +456,7 @@
       end
     end
 
-    context "no existing asset" do
+    context "with no existing asset" do
       it "responds with not found status" do
         get :show, params: { id: "some-gif-or-other" }
 
@@ -492,7 +473,7 @@
   end
 
   describe "POST restore" do
-    context "an asset marked as deleted" do
+    context "with an asset marked as deleted" do
       let(:asset) { FactoryBot.create(:asset, deleted_at: 10.minutes.ago) }
 
       before do
@@ -508,25 +489,6 @@
         expect(restored_asset).not_to be_nil
         expect(restored_asset.deleted_at).to be_nil
       end
-
-      context "when restoring fails" do
-        let(:errors) { ActiveModel::Errors.new(asset) }
-
-        before do
-          errors.add(:base, "Something went wrong")
-          allow_any_instance_of(Asset).to receive(:restore).and_return(false)
-          allow_any_instance_of(Asset).to receive(:errors).and_return(errors)
-          post :restore, params: { id: asset.id }
-        end
-
-        it "responds with unprocessable entity status" do
-          expect(response).to have_http_status(:unprocessable_entity)
-        end
-
-        it "includes the errors in the response" do
-          expect(response.body).to match(/Something went wrong/)
-        end
-      end
     end
   end
 end

spec/controllers/media_controller_spec.rb

@@ -23,7 +23,7 @@ def download
 
     describe "#proxy_to_s3_via_nginx" do
       let(:asset) { FactoryBot.build(:asset, id: "123") }
-      let(:cloud_storage) { double(:cloud_storage) }
+      let(:cloud_storage) { instance_double(S3Storage) }
       let(:presigned_url) { "https://s3-host.test/presigned-url" }
       let(:last_modified) { Time.zone.parse("2017-01-01 00:00") }
       let(:content_disposition) { instance_double(ContentDispositionConfiguration) }
@@ -195,7 +195,7 @@ def download
 
       context "when the file name in the URL represents an old version" do
         let(:old_file_name) { "an_old_filename.pdf" }
-        let(:scope) { double(:undeleted_scope) }
+        let(:scope) { class_double(Asset) }
 
         before do
           allow(Asset).to receive(:undeleted).and_return(scope)
@@ -306,7 +306,7 @@ def download
       context "when a user is authenticated" do
         let(:user) { FactoryBot.build(:user) }
         let(:asset) { FactoryBot.create(:uploaded_asset, draft: true) }
-        let(:scope) { double(:undeleted_scope) }
+        let(:scope) { class_double(Asset) }
 
         before do
           allow(Asset).to receive(:undeleted).and_return(scope)

spec/controllers/whitehall_assets_controller_spec.rb

@@ -10,7 +10,7 @@
   describe "POST create" do
     let(:attributes) { FactoryBot.attributes_for(:whitehall_asset, :with_legacy_metadata) }
 
-    context "a valid asset" do
+    context "with a valid asset" do
       it "is persisted" do
         post :create, params: { asset: attributes }
 
@@ -104,7 +104,7 @@
       end
     end
 
-    context "an invalid asset" do
+    context "with an invalid asset" do
       let(:attributes) { { file: nil } }
 
       it "is not persisted" do
@@ -121,7 +121,7 @@
       end
     end
 
-    context "an asset with the same legacy_url_path as an existing asset" do
+    context "with an asset with the same legacy_url_path as an existing asset" do
       let!(:existing_asset) { FactoryBot.create(:whitehall_asset, legacy_url_path: attributes[:legacy_url_path]) }
 
       it "marks the existing asset as deleted" do
@@ -131,7 +131,7 @@
       end
     end
 
-    context "a draft asset" do
+    context "with a draft asset" do
       let(:attributes) { FactoryBot.attributes_for(:whitehall_asset, :with_legacy_metadata, draft: true) }
 
       it "stores draft status on asset" do
@@ -148,7 +148,7 @@
       end
     end
 
-    context "an asset with a redirect URL" do
+    context "with an asset with a redirect URL" do
       let(:redirect_url) { "https://example.com/path/file.ext" }
       let(:attributes) { FactoryBot.attributes_for(:whitehall_asset, redirect_url: redirect_url) }
 

spec/lib/content_disposition_configuration_spec.rb

@@ -1,32 +1,31 @@
 require "rails_helper"
 
 RSpec.describe ContentDispositionConfiguration do
-  subject { described_class.new(type: type) }
-
+  let(:config) { described_class.new(type: type) }
   let(:type) { "inline" }
   let(:asset) { FactoryBot.build(:asset) }
 
   describe "#type" do
     let(:type) { "attachment" }
 
     it "returns type supplied to constructor" do
-      expect(subject.type).to eq("attachment")
+      expect(config.type).to eq("attachment")
     end
   end
 
   describe "#options_for" do
     it "returns options including filename for asset" do
-      expect(subject.options_for(asset)).to include(filename: "asset.png")
+      expect(config.options_for(asset)).to include(filename: "asset.png")
     end
 
     it "returns options including disposition for asset" do
-      expect(subject.options_for(asset)).to include(disposition: "inline")
+      expect(config.options_for(asset)).to include(disposition: "inline")
     end
   end
 
   describe "#header_for" do
     it "returns Content-Disposition header value" do
-      expect(subject.header_for(asset)).to eq(%(inline; filename="asset.png"))
+      expect(config.header_for(asset)).to eq(%(inline; filename="asset.png"))
     end
   end
 end