diff --git a/Gemfile b/Gemfile index 7d9987be3..ba057c297 100644 --- a/Gemfile +++ b/Gemfile @@ -15,8 +15,8 @@ gem 'chartkick' # GDS gems. gem 'gds-sso', '~> 14' gem 'govuk_admin_template', '~> 6' -gem 'govuk_publishing_components' gem 'govuk_app_config' +gem 'govuk_publishing_components' gem 'plek', '~> 3' group :test, :development do diff --git a/Gemfile.lock b/Gemfile.lock index 1d6dc121e..89fbfa8f4 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -109,8 +109,9 @@ GEM warden-oauth2 (~> 0.0.1) globalid (0.4.2) activesupport (>= 4.2.0) - govuk-lint (3.11.3) - rubocop (~> 0.64) + govuk-lint (3.11.4) + rubocop (~> 0.72) + rubocop-rails (~> 2) rubocop-rspec (~> 1.28) scss_lint govuk_admin_template (6.7.0) @@ -274,6 +275,9 @@ GEM rainbow (>= 2.2.2, < 4.0) ruby-progressbar (~> 1.7) unicode-display_width (>= 1.4.0, < 1.7) + rubocop-rails (2.1.0) + rack (>= 1.1) + rubocop (>= 0.72.0) rubocop-rspec (1.33.0) rubocop (>= 0.60.0) ruby-progressbar (1.10.1) diff --git a/app/controllers/applications_controller.rb b/app/controllers/applications_controller.rb index 6f1a7dc6a..490ae96aa 100644 --- a/app/controllers/applications_controller.rb +++ b/app/controllers/applications_controller.rb @@ -104,7 +104,7 @@ def create end def update - if @application.update_attributes(application_params) + if @application.update(application_params) redirect_to @application, flash: { notice: "Successfully updated the application" } else flash.now[:error] = "There are some problems with the application" diff --git a/app/controllers/deployments_controller.rb b/app/controllers/deployments_controller.rb index b143dc0d0..8b1bf1c9b 100644 --- a/app/controllers/deployments_controller.rb +++ b/app/controllers/deployments_controller.rb @@ -48,7 +48,7 @@ def create private def application_by_repo - if (existing_app = Application.find_by_repo(repo_path)) + if (existing_app = Application.find_by(repo: repo_path)) existing_app else Application.create!(name: app_name, repo: repo_path, domain: domain) diff --git a/app/controllers/sites_controller.rb b/app/controllers/sites_controller.rb index a93c46bbb..a006b2f60 100644 --- a/app/controllers/sites_controller.rb +++ b/app/controllers/sites_controller.rb @@ -2,7 +2,7 @@ class SitesController < ApplicationController def show; end def update - if site_settings.update_attributes(site_params) + if site_settings.update(site_params) redirect_to root_path, alert: 'Site settings updated' else render :show diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 12a1b1edf..a02f63885 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -31,7 +31,7 @@ def human_datetime(date) end def github_tag_link_to(app, git_ref) - link_to(git_ref.truncate(20), "#{app.repo_url}/tree/#{git_ref}", target: "_blank") + link_to(git_ref.truncate(20), "#{app.repo_url}/tree/#{git_ref}", target: "_blank", rel: "noopener") end def github_compare_to_master(application, deploy) @@ -53,7 +53,7 @@ def jenkins_deploy_app_url(application, release_tag, environment) "publishing.service.gov.uk" end - "https://#{subdomain_prefix}.#{domain}/job/Deploy_App/parambuild?TARGET_APPLICATION=#{application.shortname}&TAG=#{escaped_release_tag}".html_safe + "https://#{subdomain_prefix}.#{domain}/job/Deploy_App/parambuild?TARGET_APPLICATION=#{application.shortname}&TAG=#{escaped_release_tag}".html_safe # rubocop:disable Rails/OutputSafety end def jenkins_deploy_puppet_url(release_tag, environment, aws:) @@ -71,7 +71,7 @@ def jenkins_deploy_puppet_url(release_tag, environment, aws:) "publishing.service.gov.uk" end - "https://#{subdomain_prefix}.#{domain}/job/Deploy_Puppet/parambuild?TAG=#{escaped_release_tag}".html_safe + "https://#{subdomain_prefix}.#{domain}/job/Deploy_Puppet/parambuild?TAG=#{escaped_release_tag}".html_safe # rubocop:disable Rails/OutputSafety end private diff --git a/app/models/application.rb b/app/models/application.rb index 26218af91..7dc59035d 100644 --- a/app/models/application.rb +++ b/app/models/application.rb @@ -13,7 +13,7 @@ class Application < ApplicationRecord validates_uniqueness_of :name, :repo - has_many :deployments + has_many :deployments, dependent: :destroy default_scope { order("name ASC") } diff --git a/app/models/commit.rb b/app/models/commit.rb index a4724be17..23194f551 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -31,6 +31,6 @@ def author_name end def commit_date - commit_data[:commit][:author][:date].to_time + commit_data[:commit][:author][:date].to_time # rubocop:disable Rails/Date end end diff --git a/app/models/deployment_stats.rb b/app/models/deployment_stats.rb index ddcca657e..4ed8d818c 100644 --- a/app/models/deployment_stats.rb +++ b/app/models/deployment_stats.rb @@ -7,7 +7,7 @@ def initialize(initial_scope = nil) def per_month production_deploys - .where("deployments.created_at < ?", Date.today.at_beginning_of_month) + .where("deployments.created_at < ?", Time.zone.today.at_beginning_of_month) .group("DATE_FORMAT(deployments.created_at,'%Y-%m')") .count end diff --git a/config/environments/development.rb b/config/environments/development.rb index 503375ded..effc7ffdd 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -13,7 +13,7 @@ config.consider_all_requests_local = true # Enable/disable caching. By default caching is disabled. - if Rails.root.join('tmp/caching-dev.txt').exist? + if Rails.root.join('tmp', 'caching-dev.txt').exist? config.action_controller.perform_caching = true config.cache_store = :memory_store diff --git a/config/initializers/current_release_sha.rb b/config/initializers/current_release_sha.rb index 17da8ff13..914a7a7a3 100644 --- a/config/initializers/current_release_sha.rb +++ b/config/initializers/current_release_sha.rb @@ -1,5 +1,5 @@ -if File.exist?("#{Rails.root}/REVISION") - revision = `cat #{Rails.root}/REVISION`.chomp +if Rails.root.join('REVISION').exist? + revision = File.read(Rails.root.join('REVISION')).chomp CURRENT_RELEASE_SHA = revision[0..10] # Just get the short SHA else CURRENT_RELEASE_SHA = "development".freeze diff --git a/test/functional/applications_controller_test.rb b/test/functional/applications_controller_test.rb index becb67d99..2b022c467 100644 --- a/test/functional/applications_controller_test.rb +++ b/test/functional/applications_controller_test.rb @@ -89,7 +89,7 @@ class ApplicationsControllerTest < ActionController::TestCase end should "should include status notes as a warning" do - @app.update_attributes(status_notes: 'Do not deploy this without talking to core team first!') + @app.update(status_notes: 'Do not deploy this without talking to core team first!') get :show, params: { id: @app.id } assert_select '.alert-warning', 'Do not deploy this without talking to core team first!' end diff --git a/test/functional/deployments_controller_test.rb b/test/functional/deployments_controller_test.rb index 0f0d4cf5c..b2ea1b906 100644 --- a/test/functional/deployments_controller_test.rb +++ b/test/functional/deployments_controller_test.rb @@ -46,7 +46,7 @@ class DeploymentsControllerTest < ActionController::TestCase post :create, params: { deployment: { application_id: app.id, version: "release_123", environment: "staging", created_at: "18/01/2013 11:57" } } deployment = app.reload.deployments.last - refute_nil deployment + assert_not_nil deployment assert_equal "release_123", deployment.version assert_equal "staging", deployment.environment assert_equal "2013-01-18 11:57:00 +0000", deployment.created_at.to_s @@ -72,7 +72,7 @@ class DeploymentsControllerTest < ActionController::TestCase post :create, params: { repo: "org/app", deployment: { version: "release_123", environment: "staging", jenkins_user_email: "user@example.org", jenkins_user_name: "A User", deployed_sha: "02a570885766dc43d5e2432855bbffb342543906" } } deployment = app.reload.deployments.last - refute_nil deployment + assert_not_nil deployment assert_equal "release_123", deployment.version assert_equal "staging", deployment.environment assert_equal "user@example.org", deployment.jenkins_user_email diff --git a/test/functional/sites_controller_test.rb b/test/functional/sites_controller_test.rb index a7764c228..c1df61ea2 100644 --- a/test/functional/sites_controller_test.rb +++ b/test/functional/sites_controller_test.rb @@ -26,7 +26,7 @@ class SitesControllerTest < ActionController::TestCase context "PATCH update" do should "create some site settings if there are none" do - refute Site.settings.persisted? + assert_not Site.settings.persisted? patch :update, params: { site: { status_notes: 'Deploy freeze in place.' } } @@ -45,7 +45,7 @@ class SitesControllerTest < ActionController::TestCase should "rerender the form if the site settings won't save" do patch :update, params: { site: { status_notes: 'a' * 256 } } - refute Site.settings.persisted? + assert_not Site.settings.persisted? assert_select 'form[action="/site"]' assert_select 'form textarea[name="site[status_notes]"]', 'a' * 256 diff --git a/test/unit/application_test.rb b/test/unit/application_test.rb index 22fade54a..637f11591 100644 --- a/test/unit/application_test.rb +++ b/test/unit/application_test.rb @@ -25,14 +25,14 @@ class ApplicationTest < ActiveSupport::TestCase should "be invalid with an empty name" do application = Application.new(@atts.merge(name: "")) - refute application.valid? + assert_not application.valid? end should "be invalid with a duplicate name" do FactoryBot.create(:application, name: "Tron-o-matic") application = Application.new(@atts) - refute application.valid? + assert_not application.valid? assert application.errors[:name].include?("has already been taken") end @@ -40,7 +40,7 @@ class ApplicationTest < ActiveSupport::TestCase FactoryBot.create(:application, repo: "alphagov/tron-o-matic") application = Application.new(@atts) - refute application.valid? + assert_not application.valid? assert application.errors[:repo].include?("has already been taken") end @@ -48,19 +48,19 @@ class ApplicationTest < ActiveSupport::TestCase application = Application.new(@atts) application.repo = "noslashes" - refute application.valid? + assert_not application.valid? assert application.errors[:repo].include?("is invalid") application.repo = "too/many/slashes" - refute application.valid? + assert_not application.valid? assert application.errors[:repo].include?("is invalid") application.repo = "/slashatfront" - refute application.valid? + assert_not application.valid? assert application.errors[:repo].include?("is invalid") application.repo = "slashatback/" - refute application.valid? + assert_not application.valid? assert application.errors[:repo].include?("is invalid") end @@ -97,31 +97,31 @@ class ApplicationTest < ActiveSupport::TestCase should "be invalid with a name that is too long" do application = Application.new(@atts.merge(name: ("a" * 256))) - refute application.valid? + assert_not application.valid? end should "be invalid with a domain that is too long" do application = Application.new(@atts.merge(domain: ("gith" + ("u" * 247) + "b.com"))) - refute application.valid? + assert_not application.valid? end should "be invalid with a repo that is too long" do application = Application.new(@atts.merge(repo: ("alphagov/my-r" + ("e" * 243) + "po"))) - refute application.valid? + assert_not application.valid? end should "be invalid with a shortname that is too long" do application = Application.new(@atts.merge(shortname: ("a" * 256))) - refute application.valid? + assert_not application.valid? end should "be invalid with status_notes that are too long" do application = Application.new(@atts.merge(status_notes: ("This app is n" + ("o" * 233) + "t working!"))) - refute application.valid? + assert_not application.valid? end end diff --git a/test/unit/site_test.rb b/test/unit/site_test.rb index 976ee2986..89e697f6e 100644 --- a/test/unit/site_test.rb +++ b/test/unit/site_test.rb @@ -9,7 +9,7 @@ class SiteTest < ActiveSupport::TestCase should 'be invalid with a status_note that is longer than 255 chars' do assert FactoryBot.build(:site, status_notes: 'a' * 254).valid? assert FactoryBot.build(:site, status_notes: 'a' * 255).valid? - refute FactoryBot.build(:site, status_notes: 'a' * 256).valid? + assert_not FactoryBot.build(:site, status_notes: 'a' * 256).valid? end should 'be blocked from creating if there is already a site instance' do @@ -17,15 +17,15 @@ class SiteTest < ActiveSupport::TestCase assert original.persisted? remake = FactoryBot.build(:site, status_notes: 'Aw :(') - refute remake.valid? - refute remake.save + assert_not remake.valid? + assert_not remake.save assert_equal ['There can only be one Site instance'], remake.errors[:base] end end context '.settings' do should "return an unsaved instance if no instance is persisted already" do - refute Site.settings.persisted? + assert_not Site.settings.persisted? end should "return an the persisted instance if one has be saved already" do