Merge pull request #1783 from DFE-Digital/code-maintenance

Codebase maintenance

.rubocop.yml

@@ -18,12 +18,18 @@ Lint/UnusedBlockArgument:
   Exclude:
     - config/environments/*
 
+Lint/MissingSuper:
+  Enabled: false
+
 Style/HashEachMethods:
   Enabled: true
 
 Style/HashTransformKeys:
   Enabled: true
 
+Style/StringConcatenation:
+  Enabled: false
+
 Style/HashTransformValues:
   Enabled: true
 

Gemfile.lock

@@ -351,15 +351,15 @@ GEM
     rack (2.2.3)
     rack-attack (6.5.0)
       rack (>= 1.0, < 3)
-    rack-oauth2 (1.16.0)
+    rack-oauth2 (1.17.0)
       activesupport
       attr_required
       httpclient
       json-jwt (>= 1.11.0)
       rack (>= 2.1.0)
     rack-protection (2.1.0)
       rack
-    rack-proxy (0.6.5)
+    rack-proxy (0.7.0)
       rack
     rack-rewrite (1.5.1)
     rack-test (1.1.0)
@@ -439,31 +439,32 @@ GEM
     rspec-support (3.10.2)
     rspec_junit_formatter (0.4.1)
       rspec-core (>= 2, < 4, != 2.12.0)
-    rubocop (0.87.1)
+    rubocop (1.15.0)
       parallel (~> 1.10)
-      parser (>= 2.7.1.1)
+      parser (>= 3.0.0.0)
       rainbow (>= 2.2.2, < 4.0)
-      regexp_parser (>= 1.7)
+      regexp_parser (>= 1.8, < 3.0)
       rexml
-      rubocop-ast (>= 0.1.0, < 1.0)
+      rubocop-ast (>= 1.5.0, < 2.0)
       ruby-progressbar (~> 1.7)
-      unicode-display_width (>= 1.4.0, < 2.0)
-    rubocop-ast (0.8.0)
-      parser (>= 2.7.1.5)
-    rubocop-govuk (3.17.2)
-      rubocop (= 0.87.1)
-      rubocop-ast (= 0.8.0)
-      rubocop-rails (= 2.8.1)
+      unicode-display_width (>= 1.4.0, < 3.0)
+    rubocop-ast (1.6.0)
+      parser (>= 3.0.1.1)
+    rubocop-govuk (4.0.0)
+      rubocop (~> 1.15.0)
+      rubocop-ast (~> 1.6.0)
+      rubocop-rails (~> 2.10.0)
       rubocop-rake (= 0.5.1)
-      rubocop-rspec (= 1.42.0)
-    rubocop-rails (2.8.1)
+      rubocop-rspec (~> 2.3.0)
+    rubocop-rails (2.10.1)
       activesupport (>= 4.2.0)
       rack (>= 1.1)
-      rubocop (>= 0.87.0)
+      rubocop (>= 1.7.0, < 2.0)
     rubocop-rake (0.5.1)
       rubocop
-    rubocop-rspec (1.42.0)
-      rubocop (>= 0.87.0)
+    rubocop-rspec (2.3.0)
+      rubocop (~> 1.0)
+      rubocop-ast (>= 1.1.0)
     ruby-graphviz (1.2.5)
       rexml
     ruby-progressbar (1.11.0)
@@ -532,7 +533,7 @@ GEM
     tzinfo (2.0.4)
       concurrent-ruby (~> 1.0)
     uk_postcode (2.1.6)
-    unicode-display_width (1.7.0)
+    unicode-display_width (2.0.0)
     uniform_notifier (1.14.2)
     validate_email (0.1.6)
       activemodel (>= 3.0)
@@ -563,7 +564,7 @@ GEM
       rack-proxy (>= 0.6.1)
       railties (>= 5.2)
       semantic_range (>= 2.3.0)
-    websocket-driver (0.7.3)
+    websocket-driver (0.7.5)
       websocket-extensions (>= 0.1.0)
     websocket-extensions (0.1.5)
     xpath (3.2.0)

app/controllers/candidates/schools_controller.rb

@@ -40,7 +40,7 @@ def show
 
   def location_present?
     search_params[:location].present? || (
-      search_params[:latitude].present? && search_params[:latitude].present?
+      search_params[:longitude].present? && search_params[:latitude].present?
     )
   end
 

app/controllers/concerns/gitis_authentication.rb

@@ -3,7 +3,9 @@ module GitisAuthentication
   include GitisAccess
 
   class UnauthorizedCandidate < RuntimeError; end
+
   class UnconfirmedCandidate < RuntimeError; end
+
   class InvalidContact < RuntimeError; end
 
   included do

app/controllers/schools/base_controller.rb

@@ -1,5 +1,6 @@
 module Schools
   class SchoolNotRegistered < StandardError; end
+
   class MissingURN < StandardError; end
 
   class BaseController < ApplicationController
@@ -23,7 +24,7 @@ class BaseController < ApplicationController
 
     def current_school
       if current_urn.blank?
-        sub = current_user.raw_attributes.dig("sub")
+        sub = current_user.raw_attributes["sub"]
         raise MissingURN, "school urn is missing, unable to match with school for - user #{sub}"
       end
 
@@ -44,7 +45,7 @@ def set_site_header_text
 
     def retrieve_school(urn)
       if urn.blank?
-        sub = current_user.raw_attributes.dig("sub")
+        sub = current_user.raw_attributes["sub"]
         raise MissingURN, "school urn is missing, unable to match with school for - user #{sub}"
       end
 

app/controllers/schools/insecure_sessions_controller.rb

@@ -8,7 +8,7 @@ def create
 
     session[:id_token]     = 'abc123'
 
-    # NOTE the sub (subscription) param is the user's unique identifier
+    # NOTE: the sub (subscription) param is the user's unique identifier
     # on DfE Sign-in and is hard-coded here. It must match the corresponding
     # value injected into Schools::SessionsController
     session[:current_user] = OpenIDConnect::ResponseObject::UserInfo.new(

app/controllers/schools/sessions_controller.rb

@@ -1,8 +1,12 @@
 module Schools
   class StateMismatchError < StandardError; end
+
   class AuthFailedError < StandardError; end
+
   class InsufficientPrivilegesError < StandardError; end
+
   class SessionExpiredError < StandardError; end
+
   class NoOrganisationError < StandardError; end
 
   class SessionsController < ApplicationController

app/helpers/schools/confirm_attendance_helper.rb

@@ -1,5 +1,5 @@
 module Schools::ConfirmAttendanceHelper
-  # note we can't use the form builder here because it wraps everything in
+  # NOTE: we can't use the form builder here because it wraps everything in
   # form-groups and fieldsets which affect how it's displayed in a table cell
   def confirm_attendance_radio(builder, booking_id, value, label_text)
     tag.div(class: "govuk-radios__item") do

app/models/bookings/data/school_mass_importer.rb

@@ -113,7 +113,7 @@ def cleanup_website(urn, url)
         return nil if url_with_prefix =~ /@/
 
         # skip urls that don't look sensible
-        unless url_with_prefix.match?(/^(http|https):\/\/[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,10}(:[0-9]{1,5})?(\/.*)?$/ix)
+        unless url_with_prefix.match?(/^(http|https):\/\/[a-z0-9]+([\-.]{1}[a-z0-9]+)*\.[a-z]{2,10}(:[0-9]{1,5})?(\/.*)?$/ix)
           Rails.logger.info "invalid website for #{urn}, #{url}"
           return nil
         end

app/models/bookings/profile_attributes_convertor.rb

@@ -49,9 +49,10 @@ def phase_ids
     def convert_dbs_profile
       output[:dbs_policy_conditions] = input[:dbs_requirement_dbs_policy_conditions]
 
-      output[:dbs_policy_details] = if input[:dbs_requirement_dbs_policy_conditions] == 'notrequired'
+      output[:dbs_policy_details] = case input[:dbs_requirement_dbs_policy_conditions]
+                                    when 'notrequired'
                                       input[:dbs_requirement_no_dbs_policy_details].presence
-                                    elsif input[:dbs_requirement_dbs_policy_conditions] == 'inschool'
+                                    when 'inschool'
                                       input[:dbs_requirement_dbs_policy_details_inschool].presence
                                     else
                                       input[:dbs_requirement_dbs_policy_details].presence

app/models/bookings/school_search.rb

@@ -167,7 +167,7 @@ def valid_geocoder_result?(result)
 
   def order_by(option)
     if (option == 'distance') && coordinates.present?
-      # note distance isn't actually an attribute of
+      # NOTE: distance isn't actually an attribute of
       # Bookings::School so we can't use hash syntax
       # as Rails will complain
       'distance asc'

app/models/bookings/school_sync.rb

@@ -40,7 +40,7 @@ def import_and_update
   end
 
   def sync_disabled?
-    disabled = ENV.fetch('GIAS_SYNC_DISABLED') { false }
+    disabled = ENV.fetch('GIAS_SYNC_DISABLED', false)
     disabled.to_s.in?(%w[1 true yes])
   end
 

app/models/cookie_preference.rb

@@ -74,7 +74,7 @@ def rejected_cookies
     all_cookies - accepted_cookies
   end
 
-  # Note: allowed is not the same as accepted
+  # NOTE: allowed is not the same as accepted
   # allowed = 'not explicitly rejected, and maybe explicitly accepted'
   # accepted = 'explicitly accepted'
   def allowed?(cookie_category_or_name)

app/models/schools/on_boarding/profile_subject.rb

@@ -13,5 +13,5 @@ class Schools::OnBoarding::ProfileSubject < ApplicationRecord
   validates :subject, presence: true
 
   validates :schools_school_profile_id,
-    uniqueness: { scope: %i[bookings_subject_id] }
+    uniqueness: { scope: :bookings_subject_id }
 end

app/presenters/candidates/school_presenter.rb

@@ -97,22 +97,19 @@ def secondary_dates_grouped_by_date
   private
 
     def dbs_requirement
-      case profile.dbs_policy_conditions
-      when "required"
-        "Yes"
-      when "inschool"
-        "Yes - when in school"
-      when "notrequired"
-        'No - Candidates will be accompanied at all times when in school'
-      end
+      {
+        'required' => 'Yes',
+        'inschool' => 'Yes - when in school',
+        'notrequired' => 'No - Candidates will be accompanied at all times when in school'
+      }[profile.dbs_policy_conditions]
     end
 
     def legacy_dbs_requirement
-      case profile.dbs_required
-      when 'always' then 'Yes - Always'
-      when 'sometimes' then 'Yes - Sometimes'
-      when 'never' then 'No - Candidates will be accompanied at all times'
-      end
+      {
+        'always' => 'Yes - Always',
+        'sometimes' => 'Yes - Sometimes',
+        'never' => 'No - Candidates will be accompanied at all times'
+      }[profile.dbs_required]
     end
   end
 end

app/services/bookings/gitis/api.rb

@@ -60,6 +60,7 @@ def patch(url, params, headers = {})
     end
 
     class UnsupportedAbsoluteUrlError < RuntimeError; end
+
     class ConnectionFailed < RuntimeError
       def initialize(url)
         super "Connection Failed: #{url}"
@@ -78,6 +79,7 @@ def initialize(resp)
     end
 
     class UnknownUrlError < BadResponseError; end
+
     class AccessDeniedError < BadResponseError; end
 
   private
@@ -139,7 +141,7 @@ def parse_response(resp)
     end
 
     def parse_entity_id(entity_id)
-      entity_id&.gsub(%r{\A#{endpoint_url}\/}, '')
+      entity_id&.gsub(%r{\A#{endpoint_url}/}, '')
     end
   end
 end

app/services/bookings/gitis/auth.rb

@@ -29,7 +29,7 @@ def config
         Rails.application.config.x.gitis
       end
 
-      def token(force_reload = false)
+      def token(force_reload: false)
         if !force_reload && (cached_token = fetch_cached_token) && cached_token.present?
           cached_token
         elsif (new_token = retrieve_token)

app/services/bookings/gitis/entity.rb

@@ -1,6 +1,8 @@
 module Bookings::Gitis
   class IdChangedUnexpectedly < RuntimeError; end
+
   class MissingPrimaryKey < RuntimeError; end
+
   class InvalidEntityId < RuntimeError; end
 
   class InvalidEntity < RuntimeError
@@ -17,7 +19,7 @@ module Entity
     include ActiveModel::Dirty
 
     ID_FORMAT = /\A[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\z/.freeze
-    BIND_FORMAT = /\A[^\(]+\([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\)\z/.freeze
+    BIND_FORMAT = /\A[^(]+\([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\)\z/.freeze
 
     def self.valid_id?(id)
       ID_FORMAT.match? id.to_s

app/services/bookings/gitis/factory.rb

@@ -14,7 +14,7 @@ def crm(read_from_cache: false)
         if Rails.application.config.x.gitis.fake_crm
           Bookings::Gitis::FakeCrm.new
         elsif Rails.application.config.x.gitis.caching
-          Bookings::Gitis::CRM.new caching_store(read_from_cache)
+          Bookings::Gitis::CRM.new caching_store(read_from_cache: read_from_cache)
         else
           Bookings::Gitis::CRM.new store
         end
@@ -42,7 +42,7 @@ def read_write_caching_store
           store, cache, namespace: NAMESPACE, ttl: TTL, version: VERSION
       end
 
-      def caching_store(read_from_cache = false)
+      def caching_store(read_from_cache: false)
         read_from_cache ? read_write_caching_store : write_only_caching_store
       end
 

app/services/bookings/gitis/store/write_only_cache.rb

@@ -3,6 +3,7 @@ module Gitis
     module Store
       class WriteOnlyCache
         attr_reader :store, :cache, :namespace, :ttl, :version
+
         delegate :fetch, to: :store
 
         def initialize(store, cache, namespace: nil, ttl: 1.hour, version: 'v1')

app/services/candidates/registrations/registration_session.rb

@@ -5,6 +5,7 @@ module Candidates
   module Registrations
     class RegistrationSession
       class NotCompletedError < StandardError; end
+
       class StepNotFound < StandardError; end
 
       PENDING_EMAIL_CONFIRMATION_STATUS = 'pending_email_confirmation'.freeze

app/services/candidates/registrations/registration_store.rb

@@ -5,6 +5,7 @@ class RegistrationStore
       include Singleton
 
       class SessionNotFound < StandardError; end
+
       class NoKey < StandardError; end
 
       def initialize

app/validators/email_format_validator.rb

@@ -1,5 +1,5 @@
 class EmailFormatValidator < ActiveModel::EachValidator
-  EMAIL_WITH_FULLY_QUALIFIED_HOSTNAME = %r{\A[^\s@]+@[^\.\s]+\.[^\s]+\z}.freeze
+  EMAIL_WITH_FULLY_QUALIFIED_HOSTNAME = %r{\A[^\s@]+@[^.\s]+\.[^\s]+\z}.freeze
 
   def validate_each(record, attribute, value)
     unless value.present? && is_an_email_uri?(value) && is_fqdn?(value)