Merge pull request #2797 from postalserver/rubocop

Code Quality Improvements

.rubocop.yml

@@ -1,35 +1,174 @@
-inherit_from:
-  - https://dev.k.io/rubocop/rubocop.rails.yml
+AllCops:
+  TargetRubyVersion: 3.0
+  NewCops: enable
+  Exclude:
+    - "bin/*"
+    - "db/schema.rb"
+    # Fixes missing gem exception when running Rubocop on GitHub Actions.
+    - "vendor/bundle/**/*"
 
+# Always use double quotes
 Style/StringLiterals:
   EnforcedStyle: double_quotes
 
-Style/WordArray:
+# We prefer arrays of symbols to look like an array of symbols.
+# For example: [:one, :two, :three] as opposed to %i[one two three]
+Style/SymbolArray:
   EnforcedStyle: brackets
 
-Layout/LineLength:
+# There should always be empty lines inside a class. For example
+#
+#    class MyExample
+#
+#      def some_method
+#      end
+#
+#    end
+Layout/EmptyLinesAroundClassBody:
+  EnforcedStyle: empty_lines
+
+# We want to keep attr_* definitions separated on their own lines, rather than
+# all of them collapsed into a single attr_* call. The collapsed/grouped variant
+# is harder to read, and harder to see what's been changed in diffs.
+Style/AccessorGrouping:
+  Enabled: false
+
+# Blocks are slightly different to classes, in these cases there should
+# not be new lines around the contents of the block.
+#
+#    proc do
+#      # Do something
+#    end
+Layout/EmptyLinesAroundBlockBody:
+  EnforcedStyle: no_empty_lines
+
+# Modules are the same as classes unless they're being used for namespacing
+# purposes in which case there should not be new lines.
+Layout/EmptyLinesAroundModuleBody:
+  EnforcedStyle: empty_lines_except_namespace
+
+# Space is required following -> when writing a lambda:
+#
+#    somethign = -> (var) { block }
+Layout/SpaceInLambdaLiteral:
+  EnforcedStyle: require_space
+
+Layout/FirstHashElementIndentation:
+  Enabled: false
+
+# We don't mind setting assignments in conditions so this has been disabled to
+# allow `if something = something_else` without worrying about brackets.
+Lint/AssignmentInCondition:
+  Enabled: false
+
+# Top level documentation is quite rare...
+Style/Documentation:
+  Enabled: false
+
+# We want to allow inner slashes in a regexp to be used when using /xxx/ form.
+Style/RegexpLiteral:
+  AllowInnerSlashes: true
+
+# Blocks of if statements are perfectly fine and usually more readable than
+# putting everything onto a single line just because we can.
+Style/IfUnlessModifier:
+  Enabled: false
+
+# We prefer assignments to happen within the condition rather than setting a
+# variable to the result of a condition.
+Style/ConditionalAssignment:
+  EnforcedStyle: assign_inside_condition
+  IncludeTernaryExpressions: false
+
+# Empty methods should not be compacted onto a single line
+Style/EmptyMethod:
+  EnforcedStyle: expanded
+
+# As above, just flag them.
+Lint/UnusedBlockArgument:
+  AutoCorrect: false
+
+# While we don't want to make heavy use of get_ or set_ methods we do often need
+# to use these when we want to refer to actually getting or setting something
+# (usually from another API).
+Naming/AccessorMethodName:
+  Enabled: false
+
+# If we want a boolean called :true, we should be allowed that. These are likely
+# not mistakes.
+Lint/BooleanSymbol:
   Enabled: false
 
+# Using block.map(&:upcase) is not always the neatest way to show something. For
+# example if you have a block that just calls one thing, you don't want it
+# collapsed.
+#
+#    action do |user|
+#      user.delete
+#    end
+#
+# This should be action(&:delete) because it is not clear what is actually
+# happening without the context of knowing what the inner variable should be
+# called.
+Style/SymbolProc:
+  Enabled: false
+
+# Allow a maxmium of 5 arguments and don't include keyword arguments
+Metrics/ParameterLists:
+  Max: 5
+  CountKeywordArgs: false
+
+# This cop checks for chaining of a block after another block that spans multiple lines.
+Style/MultilineBlockChain:
+  Exclude:
+    - "spec/**/*.rb"
+
 Metrics/AbcSize:
   Enabled: false
 
-Metrics/BlockLength:
+Style/FrozenStringLiteralComment:
+  Enabled: true
+  SafeAutoCorrect: true
+
+Naming/PredicateName:
   Enabled: false
 
-Metrics/ClassLength:
+Layout/LineLength:
+  # We want to reduce this back down to 120 but there are a fair number of offences
+  # of this which need addressing individually and carefully. 
+  Max: 200
+
+Metrics/PerceivedComplexity:
+  # As above, we want to enable this again in the future, but for now we'll just 
+  # disable it entirely.
   Enabled: false
 
 Metrics/CyclomaticComplexity:
+  # As above.
   Enabled: false
 
 Metrics/MethodLength:
+  # As above.
+  Enabled: false
+
+Metrics/BlockNesting:
+  # As above.
+  Enabled: false
+
+Style/StringConcatenation:
+  Enabled: false
+
+Metrics/BlockLength:
+  Enabled: false
+
+Metrics/ClassLength:
   Enabled: false
 
 Metrics/ModuleLength:
   Enabled: false
 
-Metrics/PerceivedComplexity:
+Lint/UnusedMethodArgument:
   Enabled: false
 
-Lint/MissingSuper:
+Style/SpecialGlobalVars:
   Enabled: false

Gemfile

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 source "https://rubygems.org"
 gem "authie"
 gem "autoprefixer-rails"
@@ -10,12 +12,12 @@ gem "clockwork"
 gem "dotenv-rails"
 gem "dynamic_form"
 gem "encrypto_signo"
+gem "execjs", "~> 2.7", "< 2.8"
 gem "foreman"
 gem "gelf"
 gem "haml"
 gem "hashie"
 gem "highline", require: false
-gem "execjs", '~> 2.7', "< 2.8"
 gem "jwt"
 gem "kaminari"
 gem "mail"
@@ -28,8 +30,8 @@ gem "puma"
 gem "rails", "= 6.1.7.6"
 gem "resolv", "~> 0.2.1"
 gem "secure_headers"
-gem 'sentry-rails'
-gem 'sentry-ruby'
+gem "sentry-rails"
+gem "sentry-ruby"
 gem "turbolinks", "~> 5"
 
 group :development, :assets do

Rakefile

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 # Add your own tasks in files placed in lib/tasks ending in .rake,
 # for example lib/tasks/capistrano.rake, and they will automatically be available to Rake.
 

api/authenticator.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 authenticator :server do
   friendly_name "Server Authenticator"
   header "X-Server-API-Key", "The API token for a server that you wish to authenticate with.", example: "f29a45f0d4e1744ebaee"

api/controllers/messages_api_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 controller :messages do
   friendly_name "Messages API"
   description "This API allows you to access message details"
@@ -12,7 +14,7 @@
     action do
       begin
         message = identity.server.message(params.id)
-      rescue Postal::MessageDB::Message::NotFound => e
+      rescue Postal::MessageDB::Message::NotFound
         error "MessageNotFound", id: params.id
       end
       structure :message, message, return: true
@@ -28,7 +30,7 @@
     action do
       begin
         message = identity.server.message(params.id)
-      rescue Postal::MessageDB::Message::NotFound => e
+      rescue Postal::MessageDB::Message::NotFound
         error "MessageNotFound", id: params.id
       end
       message.deliveries.map do |d|

api/controllers/send_api_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 controller :send do
   friendly_name "Send API"
   description "This API allows you to send messages"

api/structures/delivery_api_structure.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 structure :delivery do
   basic :id
   basic :status

api/structures/message_api_structure.rb

@@ -1,13 +1,15 @@
+# frozen_string_literal: true
+
 structure :message do
   basic :id
   basic :token
 
   expansion(:status) do
     {
       status: o.status,
-      last_delivery_attempt: o.last_delivery_attempt ? o.last_delivery_attempt.to_f : nil,
+      last_delivery_attempt: o.last_delivery_attempt&.to_f,
       held: o.held,
-      hold_expiry: o.hold_expiry ? o.hold_expiry.to_f : nil
+      hold_expiry: o.hold_expiry&.to_f
     }
   end
 

app/controllers/address_endpoints_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class AddressEndpointsController < ApplicationController
 
   include WithinOrganization

app/controllers/application_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require "authie/session"
 
 class ApplicationController < ActionController::Base
@@ -46,11 +48,7 @@ def page_title
   helper_method :page_title
 
   def redirect_to_with_return_to(url, *args)
-    if params[:return_to].blank? || !params[:return_to].starts_with?("/")
-      redirect_to url_with_return_to(url), *args
-    else
-      redirect_to url_with_return_to(url), *args
-    end
+    redirect_to url_with_return_to(url), *args
   end
 
   def set_timezone

app/controllers/concerns/within_organization.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module WithinOrganization
 
   extend ActiveSupport::Concern

app/controllers/credentials_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class CredentialsController < ApplicationController
 
   include WithinOrganization

app/controllers/domains_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class DomainsController < ApplicationController
 
   include WithinOrganization

app/controllers/help_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class HelpController < ApplicationController
 
   include WithinOrganization

app/controllers/http_endpoints_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class HTTPEndpointsController < ApplicationController
 
   include WithinOrganization

app/controllers/ip_addresses_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class IPAddressesController < ApplicationController
 
   before_action :admin_required

app/controllers/ip_pool_rules_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class IPPoolRulesController < ApplicationController
 
   include WithinOrganization

app/controllers/ip_pools_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class IPPoolsController < ApplicationController
 
   before_action :admin_required
@@ -31,7 +33,7 @@ def update
   def destroy
     @ip_pool.destroy
     redirect_to_with_json :ip_pools, notice: "IP pool has been removed successfully."
-  rescue ActiveRecord::DeleteRestrictionError => e
+  rescue ActiveRecord::DeleteRestrictionError
     redirect_to_with_json [:edit, @ip_pool], alert: "IP pool cannot be removed because it still has associated addresses or servers."
   end
 

app/controllers/messages_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class MessagesController < ApplicationController
 
   include WithinOrganization
@@ -186,15 +188,15 @@ def get_messages(scope)
             if qs[:before]
               begin
                 options[:where][:timestamp][:less_than] = get_time_from_string(qs[:before]).to_f
-              rescue TimeUndetermined => e
+              rescue TimeUndetermined
                 flash.now[:alert] = "Couldn't determine time for before from '#{qs[:before]}'"
               end
             end
 
             if qs[:after]
               begin
                 options[:where][:timestamp][:greater_than] = get_time_from_string(qs[:after]).to_f
-              rescue TimeUndetermined => e
+              rescue TimeUndetermined
                 flash.now[:alert] = "Couldn't determine time for after from '#{qs[:after]}'"
               end
             end
@@ -220,6 +222,7 @@ def get_time_from_string(string)
         time = Chronic.parse(string, context: :past)
       end
     rescue StandardError
+      time = nil
     end
 
     raise TimeUndetermined, "Couldn't determine a suitable time from '#{string}'" if time.nil?

app/controllers/organization_ip_pools_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class OrganizationIPPoolsController < ApplicationController
 
   include WithinOrganization