fixed up rubocop isssues in app/ (except models)

.rubocop.yml

@@ -76,13 +76,21 @@ Layout/LineLength:
     # longer.
     - 'spec/spec_helper.rb'
 
+# Bumping the default AbcSize so we don't need to refactor everything
+Metrics/AbcSize:
+  Max: 25
+
 # Restrict the number of lines of code that may be within a block of code. This should
 # force developers to break their code into smaller discrete methods or objects.
 Metrics/BlockLength:
   # Exclude specs, since those are defined as large blocks of code
   Exclude:
     - 'spec/**/*'
 
+# Bumping the default MethodLength so we don't need to refactor everything
+Metrics/MethodLength:
+  Max: 25
+
 # Leave the string formatting style as `"some text %{value}" % { value: "text" }`
 # since we're uncertain what effect `format` and `sprintf` may have on the Fastgetext
 # markup `_("text")`

app/controllers/answers_controller.rb

@@ -11,6 +11,7 @@ class AnswersController < ApplicationController
   #       logic and we should stop using custom JSON here and instead use
   #       `remote: true` in the <form> tag and just send back the ERB.
   #       Consider using ActionCable for the progress bar(s)
+  # rubocop:disable Metrics/MethodLength, Metrics/AbcSize
   def create_or_update
     p_params = permitted_params
 
@@ -41,53 +42,52 @@ def create_or_update
 
     # rubocop:disable Metrics/BlockLength
     Answer.transaction do
-      begin
-        args = p_params
-        # Answer model does not understand :standards so remove it from the params
-        standards = args[:standards]
-        args.delete(:standards)
-
-        @answer = Answer.find_by!(
-          plan_id: args[:plan_id],
-          question_id: args[:question_id]
+      args = p_params
+      # Answer model does not understand :standards so remove it from the params
+      standards = args[:standards]
+      args.delete(:standards)
+
+      @answer = Answer.find_by!(
+        plan_id: args[:plan_id],
+        question_id: args[:question_id]
+      )
+      authorize @answer
+
+      @answer.update(args.merge(user_id: current_user.id))
+      if args[:question_option_ids].present?
+        # Saves the record with the updated_at set to the current time.
+        # Needed if only answer.question_options is updated
+        @answer.touch
+      end
+      if q.question_format.rda_metadata?
+        @answer.update_answer_hash(
+          JSON.parse(standards.to_json), args[:text]
         )
-        authorize @answer
-
-        @answer.update(args.merge(user_id: current_user.id))
-        if args[:question_option_ids].present?
-          # Saves the record with the updated_at set to the current time.
-          # Needed if only answer.question_options is updated
-          @answer.touch
-        end
-        if q.question_format.rda_metadata?
-          @answer.update_answer_hash(
-            JSON.parse(standards.to_json), args[:text]
-          )
-          @answer.save!
-        end
-      rescue ActiveRecord::RecordNotFound
-        @answer = Answer.new(args.merge(user_id: current_user.id))
-        @answer.lock_version = 1
-        authorize @answer
-        if q.question_format.rda_metadata?
-          @answer.update_answer_hash(
-            JSON.parse(standards.to_json), args[:text]
-          )
-        end
         @answer.save!
-      rescue ActiveRecord::StaleObjectError
-        @stale_answer = @answer
-        @answer = Answer.find_by(
-          plan_id: args[:plan_id],
-          question_id: args[:question_id]
+      end
+    rescue ActiveRecord::RecordNotFound
+      @answer = Answer.new(args.merge(user_id: current_user.id))
+      @answer.lock_version = 1
+      authorize @answer
+      if q.question_format.rda_metadata?
+        @answer.update_answer_hash(
+          JSON.parse(standards.to_json), args[:text]
         )
       end
+      @answer.save!
+    rescue ActiveRecord::StaleObjectError
+      @stale_answer = @answer
+      @answer = Answer.find_by(
+        plan_id: args[:plan_id],
+        question_id: args[:question_id]
+      )
     end
     # rubocop:enable Metrics/BlockLength
 
     # TODO: Seems really strange to do this check. If its false it returns an
     #      200 with an empty body. We should update to send back some JSON. The
     #      check should probably happen on create/update
+    # rubocop:disable Style/GuardClause
     if @answer.present?
       @plan = Plan.includes(
         sections: {
@@ -104,7 +104,8 @@ def create_or_update
       remove_list_after = remove_list(@plan)
 
       all_question_ids = @plan.questions.pluck(:id)
-      all_answers = @plan.answers
+      # rubocop pointed out that these variable is not used
+      # all_answers = @plan.answers
       qn_data = {
         to_show: all_question_ids - remove_list_after,
         to_hide: remove_list_after
@@ -114,7 +115,8 @@ def create_or_update
       @plan.sections.each do |section|
         next if section.number < @section.number
 
-        n_qs, n_ans = check_answered(section, qn_data[:to_show], all_answers)
+        # rubocop pointed out that these variables are not used
+        # n_qs, n_ans = check_answered(section, qn_data[:to_show], all_answers)
         this_section_info = {
           sec_id: section.id,
           no_qns: num_section_questions(@plan, section),
@@ -159,7 +161,9 @@ def create_or_update
       }.to_json
 
     end
+    # rubocop:enable Style/GuardClause
   end
+  # rubocop:enable Metrics/MethodLength, Metrics/AbcSize
 
   private
 

app/controllers/api/v0/base_controller.rb

@@ -3,23 +3,23 @@
 class Api::V0::BaseController < ApplicationController
 
   protect_from_forgery with: :null_session
-  before_action :set_resource, only: %i[destroy show update]
+  before_action :define_resource, only: %i[destroy show update]
   respond_to :json
 
   # POST /api/{plural_resource_name}
   def create
-    set_resource(resource_class.new(resource_params))
+    define_resource(resource_class.new(resource_params))
 
-    if get_resource.save
+    if retrieve_resource.save
       render :show, status: :created
     else
-      render json: get_resource.errors, status: :unprocessable_entity
+      render json: retrieve_resource.errors, status: :unprocessable_entity
     end
   end
 
   # DELETE /api/{plural_resource_name}/1
   def destroy
-    get_resource.destroy
+    retrieve_resource.destroy
     head :no_content
   end
 
@@ -36,15 +36,15 @@ def index
 
   # GET /api/{plural_resource_name}/1
   def show
-    respond_with get_resource
+    respond_with retrieve_resource
   end
 
   # PATCH/PUT /api/{plural_resource_name}/1
   def update
-    if get_resource.update(resource_params)
+    if retrieve_resource.update(resource_params)
       render :show
     else
-      render json: get_resource.errors, status: :unprocessable_entity
+      render json: retrieve_resource.errors, status: :unprocessable_entity
     end
   end
 
@@ -53,7 +53,7 @@ def update
   # The resource from the created instance variable
   #
   # Returns Object
-  def get_resource
+  def retrieve_resource
     instance_variable_get("@#{resource_name}")
   end
 
@@ -96,7 +96,7 @@ def resource_params
   end
 
   # Use callbacks to share common setup or constraints between actions.
-  def set_resource(resource = nil)
+  def define_resource(resource = nil)
     resource ||= resource_class.find(params[:id])
     instance_variable_set("@#{resource_name}", resource)
   end

app/controllers/api/v0/plans_controller.rb

@@ -6,6 +6,7 @@ class Api::V0::PlansController < Api::V0::BaseController
 
   ##
   # Creates a new plan based on the information passed in JSON to the API
+  # rubocop:disable Metrics/AbcSize
   def create
     @template = Template.live(params[:template_id])
     raise Pundit::NotAuthorizedError unless Api::V0::PlansPolicy.new(@user, @template).create?
@@ -57,11 +58,15 @@ def create
   def index
     raise Pundit::NotAuthorizedError unless Api::V0::PlansPolicy.new(@user, nil).index?
 
-    if params[:per_page].present? && params[:per_page].to_i > Rails.configuration.x.application.api_max_page_size
-      params[:per_page] = Rails.configuration.x.application.api_max_page_size
+    if params[:per_page].present?
+      max_pages = Rails.configuration.x.application.api_max_page_size
+      params[:per_page] = max_pages if params[:per_page].to_i > max_pages
     end
+
     @plans = @user.org.plans.includes([{ roles: :user }, { answers: :question_options },
-                                       template: [{ phases: { sections: { questions: %i[question_format themes] } } }, :org]])
+                                       template: [{ phases: {
+                                         sections: { questions: %i[question_format themes] }
+                                       } }, :org]])
 
     # Filter on list of users
     user_ids = extract_param_list(params, "user")
@@ -88,6 +93,7 @@ def index
     @plans = paginate @plans
     respond_with @plans
   end
+  # rubocop:enable Metrics/AbcSize
 
   private
 

app/controllers/api/v0/statistics_controller.rb

@@ -11,6 +11,7 @@ class Api::V0::StatisticsController < Api::V0::BaseController
   # If end_date is passed, only counts those with created_at is <= than end_date are
   # If org_id is passed and user has super_admin privileges that counter is performed
   # against org_id param instead of user's org
+  # rubocop:disable Metrics/AbcSize
   def users_joined
     unless Api::V0::StatisticsPolicy.new(@user, :statistics).users_joined?
       raise Pundit::NotAuthorizedError
@@ -37,7 +38,10 @@ def users_joined
           send_data(CSV.generate do |csv|
             csv << [_("Month"), _("No. Users joined")]
             total = 0
-            r.each_pair { |k, v| csv << [k, v]; total += v }
+            r.each_pair do |k, v|
+              csv << [k, v]
+              total += v
+            end
             csv << [_("Total"), total]
           end, filename: "#{_('users_joined')}.csv")
         end
@@ -80,7 +84,10 @@ def completed_plans
           send_data(CSV.generate do |csv|
             csv << [_("Month"), _("No. Completed Plans")]
             total = 0
-            r.each_pair { |k, v| csv << [k, v]; total += v }
+            r.each_pair do |k, v|
+              csv << [k, v]
+              total += v
+            end
             csv << [_("Total"), total]
           end, filename: "#{_('completed_plans')}.csv")
         end
@@ -120,7 +127,10 @@ def created_plans
           send_data(CSV.generate do |csv|
             csv << [_("Month"), _("No. Plans")]
             total = 0
-            r.each_pair { |k, v| csv << [k, v]; total += v }
+            r.each_pair do |k, v|
+              csv << [k, v]
+              total += v
+            end
             csv << [_("Total"), total]
           end, filename: "#{_('plans')}.csv")
         end
@@ -132,6 +142,7 @@ def created_plans
       render(json: { completed_plans: scoped.count })
     end
   end
+  # rubocop:enable Metrics/AbcSize
 
   ##
   # Displays the number of DMPs using templates owned/create by the caller's Org

app/controllers/api/v0/templates_controller.rb

@@ -7,6 +7,7 @@ class Api::V0::TemplatesController < Api::V0::BaseController
   # GET
   #
   # Renders a list of templates ordered by organisation
+  # rubocop:disable Metrics/AbcSize
   def index
     # check if the user has permissions to use the templates API
     unless Api::V0::TemplatePolicy.new(@user, :guidance_group).index?
@@ -51,5 +52,6 @@ def index
     end
     respond_with @org_templates
   end
+  # rubocop:enable Metrics/AbcSize
 
 end

app/controllers/application_controller.rb

@@ -58,7 +58,7 @@ def store_location
   end
 
   def after_sign_in_path_for(_resource)
-    referer_path = URI(request.referer).path unless request.referer.nil? or nil
+    referer_path = URI(request.referer).path unless request.referer.nil?
     if from_external_domain? || referer_path.eql?(new_user_session_path) ||
        referer_path.eql?(new_user_registration_path) ||
        referer_path.nil?
@@ -112,10 +112,10 @@ def success_message(obj, action = "saved")
   end
 
   def errors_for_display(obj)
-    if obj.present? && obj.errors.any?
-      msgs = obj.errors.full_messages.uniq.collect { |msg| "<li>#{msg}</li>" }
-      "<ul>#{msgs.join('')}</li></ul>"
-    end
+    return "" unless obj.present? && obj.errors.any?
+
+    msgs = obj.errors.full_messages.uniq.collect { |msg| "<li>#{msg}</li>" }
+    "<ul>#{msgs.join('')}</li></ul>"
   end
 
   def obj_name_for_display(obj)
@@ -151,12 +151,10 @@ def prepend_view_paths
   # Sign out of Shibboleth SP local session too.
   # -------------------------------------------------------------
   def after_sign_out_path_for(resource_or_scope)
-    if Rails.configuration.x.shibboleth.enabled
-      return Rails.configuration.x.shibboleth.logout_url + root_url
-      super
-    else
-      super
-    end
+    url = "#{Rails.configuration.x.shibboleth&.logout_url}#{root_url}"
+    return url if Rails.configuration.x.shibboleth&.enabled
+
+    super
   end
   # -------------------------------------------------------------
 

app/controllers/concerns/conditional_user_mailer.rb

@@ -15,7 +15,9 @@ def deliver_if(recipients: [], key:, &block)
 
     Array(recipients).each do |recipient|
       email_hash = recipient.get_preferences("email").with_indifferent_access
-      preference_value = !!email_hash.dig(*key.to_s.split("."))
+      # Violation of rubocop's DoubleNegation check
+      # preference_value = !!email_hash.dig(*key.to_s.split("."))
+      preference_value = email_hash.dig(*key.to_s.split("."))
       block.call(recipient) if preference_value
     end