Merge pull request #1628 from alphagov/sync-with-rubocop-govuk

Sync .rubocop.yml with RuboCop GOV.UK

.rubocop.yml

@@ -1,23 +1,17 @@
-AllCops:
-  Exclude:
-    - 'tmp/**/*'
-
-Lint/SuppressedException:
-  Exclude:
-    - 'spec/**/*'
-
-Naming/VariableNumber:
-  Exclude:
-    - 'spec/models/manual_spec.rb'
-
-Style/FormatStringToken:
-  Exclude:
-    - 'lib/tasks/rebuild_major_publication_logs_for_manuals.rake'
-
-Style/RedundantReturn:
-  Exclude:
-    - 'app/models/manual.rb'
-
 inherit_gem:
   rubocop-govuk:
     - config/default.yml
+    - config/rails.yml
+
+inherit_mode:
+  merge:
+    - Exclude
+
+# This is not supported for Mongoid
+Rails/DynamicFindBy:
+  Whitelist:
+    - find_by_slug!
+
+# This is not supported for Mongoid
+Rails/FindBy:
+  Enabled: false

app/adapters/publishing_adapter.rb

@@ -290,7 +290,7 @@ def unpublish_section(section, manual, republish:)
         Services.publishing_api.unpublish(section.uuid, type: "redirect", alternative_path: "/#{manual.slug}", discard_drafts: true)
         section.withdraw_and_mark_as_exported! unless republish
       rescue GdsApi::HTTPNotFound
-        puts "Content item with section uuid #{section.uuid} not present in the publishing API"
+        Rails.logger.warn "Content item with section uuid #{section.uuid} not present in the publishing API"
       end
     end
   end

app/models/manual.rb

@@ -310,18 +310,13 @@ def current_published_version(manual_record)
       self.class.build_manual_for(manual_record)
     elsif manual_record.latest_edition.state == "draft"
       previous_edition = manual_record.previous_edition
-      if previous_edition.state == "published"
-        self.class.build_manual_for(manual_record, edition: previous_edition, published: true)
-      else
-        # This means the previous edition is withdrawn so we shouldn't
-        # expose it as it's not actually published (we've got a new
-        # draft waiting in the wings for a withdrawn manual)
-        return nil
-      end
-    else
-      # This means the current edition is withdrawn so we shouldn't find
-      # the previously published one
-      return nil
+
+      # This means the previous edition is withdrawn so we shouldn't
+      # expose it as it's not actually published (we've got a new
+      # draft waiting in the wings for a withdrawn manual)
+      return unless previous_edition.state == "published"
+
+      self.class.build_manual_for(manual_record, edition: previous_edition, published: true)
     end
   end
 

app/models/section.rb

@@ -1,13 +1,10 @@
-require "forwardable"
 require "active_model/conversion"
 require "active_model/naming"
 require "slug_generator"
 
 class Section
   include ActiveModel::Validations
 
-  extend Forwardable
-
   validates :summary, presence: true
   validates :title, presence: true
   validates :body, presence: true, safe_html: true
@@ -29,7 +26,16 @@ def self.find(manual, section_uuid, published: false)
     end
   end
 
-  def_delegators :latest_edition, :title, :slug, :summary, :body, :updated_at, :version_number, :change_note, :minor_update, :exported_at
+  delegate :title,
+           :slug,
+           :summary,
+           :body,
+           :updated_at,
+           :version_number,
+           :change_note,
+           :minor_update,
+           :exported_at,
+           to: :latest_edition
 
   attr_reader :uuid
 
@@ -50,7 +56,7 @@ def initialize(manual:, uuid:, previous_edition: nil, latest_edition: nil)
   end
 
   def update_slug!(full_new_section_slug)
-    latest_edition.update_attribute(:slug, full_new_section_slug)
+    latest_edition.update(slug: full_new_section_slug)
   end
 
   def save
@@ -104,9 +110,7 @@ def published?
       (previous_edition && previous_edition.published?)
   end
 
-  def draft?
-    latest_edition.draft?
-  end
+  delegate :draft?, to: :latest_edition
 
   def add_attachment(attributes)
     latest_edition.build_attachment(attributes)
@@ -143,9 +147,7 @@ def needs_exporting?
     latest_edition.exported_at.nil?
   end
 
-  def reload
-    latest_edition.reload
-  end
+  delegate :reload, to: :latest_edition
 
   def mark_as_exported!(exported_at = Time.zone.now)
     latest_edition.exported_at = exported_at
@@ -192,7 +194,7 @@ def link_check_report
   attr_reader :slug_generator, :latest_edition, :previous_edition
 
   def change_note_ok
-    if change_note_required? && !change_note.present?
+    if change_note_required? && change_note.blank?
       errors.add(:change_note, "You must provide a change note or indicate minor update")
     end
   end

app/services/attachment/update_service.rb

@@ -11,7 +11,7 @@ def call
     manual = Manual.find(manual_id, user)
     section = manual.find_section(section_uuid)
     attachment = section.find_attachment_by_id(attachment_id)
-    attachment.update_attributes(attributes.merge(filename: attributes[:file].original_filename))
+    attachment.update(attributes.merge(filename: attributes[:file].original_filename))
 
     manual.save(user)
 

app/services/section/remove_service.rb

@@ -16,7 +16,7 @@ def call
     end
 
     section = manual.find_section(section_uuid)
-    raise SectionNotFoundError, section_uuid unless section.present?
+    raise SectionNotFoundError, section_uuid if section.blank?
 
     change_note_params = {
       minor_update: attributes.fetch(:minor_update, "0"),

app/workers/publish_manual_worker.rb

@@ -38,7 +38,7 @@ def requeue_task(manual_id, error)
   end
 
   def abort_task(task, error)
-    task.update_attribute(:error, error.message)
+    task.update(error: error.message)
     task.abort!
   end
 

config/application.rb

@@ -19,10 +19,10 @@ class Application < Rails::Application
 
     # These paths are non-standard (they are subdirectories of
     # app/models) so they need to be added to the autoload_paths
-    config.autoload_paths << "#{Rails.root}/app/exporters/formatters"
-    config.autoload_paths << "#{Rails.root}/app/models/validators"
-    config.autoload_paths << "#{Rails.root}/app/services/manual"
-    config.autoload_paths << "#{Rails.root}/app/services/section"
-    config.autoload_paths << "#{Rails.root}/app/services/attachment"
+    config.autoload_paths << Rails.root.join("app/exporters/formatters")
+    config.autoload_paths << Rails.root.join("app/models/validators")
+    config.autoload_paths << Rails.root.join("app/services/manual")
+    config.autoload_paths << Rails.root.join("app/services/section")
+    config.autoload_paths << Rails.root.join("app/services/attachment")
   end
 end

db/migrate/20200127131013_change_change_notes_for_mot_inspection_manual.rb

@@ -39,7 +39,6 @@ def self.up
       publication_log.updated_at = "2019-10-30T16:42:58Z".to_datetime
       publication_log.save(validate: false)
     end
-    puts "Created #{changes.count} publication logs"
 
     logger = Logger.new(STDOUT)
     logger.formatter = Logger::Formatter.new

db/seeds.rb

@@ -1,13 +1,10 @@
-if User.where(name: "Test user").present?
-  puts "Skipping because user already exists"
-  exit
-end
-
-gds_organisation_id = "af07d5a5-df63-4ddc-9383-6a666845ebe9"
+if User.where(name: "Test user").blank?
+  gds_organisation_id = "af07d5a5-df63-4ddc-9383-6a666845ebe9"
 
-User.create!(
-  name: "Test user",
-  permissions: %w[signin gds_editor],
-  organisation_content_id: gds_organisation_id,
-  organisation_slug: "test-organisation",
-)
+  User.create!(
+    name: "Test user",
+    permissions: %w[signin gds_editor],
+    organisation_content_id: gds_organisation_id,
+    organisation_slug: "test-organisation",
+  )
+end

features/step_definitions/previously_published_manual_steps.rb

@@ -7,7 +7,7 @@
   }
   @manual_slug = "guidance/example-manual-title"
 
-  @originally_published_at = Time.zone.parse("14-Dec-#{Date.today.year - 10} 09:30")
+  @originally_published_at = Time.zone.parse("14-Dec-#{Time.zone.now.year - 10} 09:30")
 
   create_manual(@manual_fields) do
     choose("has previously been published on another website.")
@@ -32,7 +32,7 @@
 When(/^I update the previously published date to a new one$/) do
   WebMock::RequestRegistry.instance.reset!
 
-  @new_originally_published_at = Time.zone.parse("25-Mar-#{Date.today.year - 8} 12:57")
+  @new_originally_published_at = Time.zone.parse("25-Mar-#{Time.zone.now.year - 8} 12:57")
 
   edit_manual_original_publication_date(@manual.title) do
     select_datetime @new_originally_published_at.to_s, from: "First publication date:"

features/support/manual_helpers.rb

@@ -124,7 +124,7 @@ def withdraw_section(manual_title, section_title, change_note: nil, minor_update
 
     click_on "Withdraw"
 
-    fill_in "Change note", with: change_note unless change_note.blank?
+    fill_in "Change note", with: change_note if change_note.present?
     if minor_update
       choose "Minor update"
     end

lib/tasks/lint.rake

@@ -1,4 +1,4 @@
 desc "Run rubocop-govuk with similar params to CI"
-task "lint" do
+task lint: :environment do
   sh "bundle exec rubocop"
 end

spec/adapters/publishing_adapter_spec.rb

@@ -451,7 +451,7 @@
     end
 
     context "when publication logs exist for section" do
-      let(:publication_log_1) do
+      let(:publication_log1) do
         PublicationLog.new(
           title: "section-title",
           slug: "manual-slug/section-slug",
@@ -460,7 +460,7 @@
         )
       end
 
-      let(:publication_log_2) do
+      let(:publication_log2) do
         PublicationLog.new(
           title: "section-title-2",
           slug: "manual-slug/section-slug",
@@ -469,7 +469,7 @@
         )
       end
 
-      let(:publication_logs) { [publication_log_1, publication_log_2] }
+      let(:publication_logs) { [publication_log1, publication_log2] }
 
       it "saves content for manual to Publishing API including change notes" do
         expect(publishing_api).to receive(:put_content).with(

spec/factories.rb

@@ -92,7 +92,7 @@
     body { "body" }
     state { "state" }
     version_number { 1 }
-    originally_published_at { Time.now }
+    originally_published_at { Time.zone.now }
     use_originally_published_at_for_public_timestamp { true }
   end
 
@@ -106,7 +106,7 @@
   factory :link_check_report do
     batch_id { 1 }
     status { "in_progress" }
-    completed_at { Time.now }
+    completed_at { Time.zone.now }
     links { [FactoryBot.build(:link)] }
 
     trait :completed do

spec/lib/attachment_reporting_spec.rb

@@ -57,7 +57,7 @@
     FactoryBot.create(
       :section_edition,
       state: "published",
-      exported_at: (Date.today - last_time_period_days) - 1.day,
+      exported_at: (last_time_period_days - 1).days.ago,
     ).tap do |section_edition|
       section_edition.attachments.create!(
         filename: "attachy.pdf",
@@ -71,7 +71,7 @@
     FactoryBot.create(
       :section_edition,
       state: "published",
-      exported_at: (Date.today - last_time_period_days) + 1.day,
+      exported_at: (last_time_period_days + 1).days.ago,
     ).tap do |section_edition|
       section_edition.attachments.create!(
         filename: "attachy.pdf",
@@ -102,7 +102,7 @@
     FactoryBot.create(
       :section_edition,
       state: "draft",
-      exported_at: Date.today,
+      exported_at: Time.zone.now,
     ).tap do |section_edition|
       section_edition.attachments.create!(
         filename: "attachy.pdf",

spec/lib/duplicate_document_finder_spec.rb

@@ -39,23 +39,23 @@
   end
 
   context "when there are multiple editions with the same slug and different section ids" do
-    let!(:edition_1) do
+    let!(:edition1) do
       FactoryBot.create(:section_edition, slug: "slug", section_uuid: 1)
     end
-    let!(:edition_2) do
+    let!(:edition2) do
       FactoryBot.create(:section_edition, slug: "slug", section_uuid: 2)
     end
 
     it "reports them as duplicates" do
-      edition_1_data = [
-        edition_1.slug, edition_1.section_uuid, edition_1.state, edition_1.created_at, 1
+      edition1_data = [
+        edition1.slug, edition1.section_uuid, edition1.state, edition1.created_at, 1
       ]
-      edition_2_data = [
-        edition_2.slug, edition_2.section_uuid, edition_2.state, edition_2.created_at, 1
+      edition2_data = [
+        edition2.slug, edition2.section_uuid, edition2.state, edition2.created_at, 1
       ]
 
-      expect(io).to receive(:puts).with(edition_1_data.join(","))
-      expect(io).to receive(:puts).with(edition_2_data.join(","))
+      expect(io).to receive(:puts).with(edition1_data.join(","))
+      expect(io).to receive(:puts).with(edition2_data.join(","))
 
       subject.execute
     end