False positive in Rails/ActionControllerFlashBeforeRender when using multiple flashes + conditions

[jamescook]

---

Expected behavior

This code should be valid:

        class HomeController < ApplicationController
          def update
            if condition
              flash[:danger] = "msg"
              redirect_to root_path and return
            end

            if other_condition
              flash[:success] = "other msg"
              redirect_to root_path
            else
              flash.now[:danger] = "other other msg"
              render 'something', status: :unprocessable_entity
            end
          end
        end

Actual behavior

Rubocop says to use flash.now before render, which I think is wrong:

          def update
            if condition
              flash[:danger] = "msg"
       +      ^^^^^ Use `flash.now` before `render`.
              redirect_to root_path and return
            end

Steps to reproduce the problem

--- a/spec/rubocop/cop/rails/action_controller_flash_before_render_spec.rb
+++ b/spec/rubocop/cop/rails/action_controller_flash_before_render_spec.rb
@@ -328,4 +328,27 @@ RSpec.describe RuboCop::Cop::Rails::ActionControllerFlashBeforeRender, :config d
       RUBY
     end
   end
+
+  context 'when using `flash` before `render` and returning `redirect_to` in condition block - part deux' do
+    it 'does not register an offense' do
+      expect_no_offenses(<<~RUBY)
+        class HomeController < ApplicationController
+          def update
+            if condition
+              flash[:danger] = "msg" # breaks here
+              redirect_to root_path and return
+            end
+
+            if other_condition
+              flash[:success] = "other msg"
+              redirect_to root_path
+            else
+              flash.now[:danger] = "other other msg"
+              render 'something', status: :unprocessable_entity
+            end
+          end
+        end
+      RUBY
+    end
+  end

Spec passes if I change the example code to use return redirect_to root_path inside the offending condition so I suspect it's related to the usage of redirect_to ... and return. However, the spec will pass when using redirect_to root_path and return if I remove the code starting with if other_condition through the end of the Rails action.

RuboCop version

bundle exec rubocop -V
1.63.1 (using Parser 3.3.0.5, rubocop-ast 1.31.2, running on ruby 3.1.4) [arm64-darwin22]

• rubocop-performance 1.21.0
• rubocop-rails 2.24.1
• rubocop-rspec 2.27.1

[tagliala]

I've started having this issue in the newly released 2.26.0

# frozen_string_literal: true

class PagesController < ApplicationController
  def update
    if @page.update(page_params)
      flash[:success] = t('.flash.success')

      if redirect_to_index?
        redirect_to pages_path
      else
        redirect_to page_path(@page)
      end
    else
      flash.now[:alert] = t('.flash.error')
      render :edit, status: :unprocessable_entity
    end
  end
end

$ bundle exec rubocop --require rubocop-rails --only Rails/ActionControllerFlashBeforeRender rubocop_rails_1269.rb 

Offenses:

rubocop_rails_825.rb:6:7: C: [Correctable] Rails/ActionControllerFlashBeforeRender: Use flash.now before render.
      flash[:success] = t('.flash.success')
      ^^^^^

1 file inspected, 1 offense detected, 1 offense autocorrectable

Workaround

Remove nested conditions

      redirect_path =
        if redirect_to_index?
          pages_path
        else
          page_path(@page)
        end

      redirect_to redirect_path

[mjankowski]

I see similar regression in 2.26.0 -- when the flash is set in a rescue...ensure section and there's a subsequent redirect_to (but no render), the flash assignment is flagged by the cop. Example:

  def batch
    @status_filter_batch_action = Form::StatusFilterBatchAction.new(status_filter_batch_action_params.merge(current_account: current_account, filter_id: params[:filter_id], type: action_from_button))
    @status_filter_batch_action.save!
  rescue ActionController::ParameterMissing
    flash[:alert] = I18n.t('admin.statuses.no_status_selected')
  ensure
    redirect_to edit_filter_path(@filter)
  end

app/controllers/filters/statuses_controller.rb:22:5: C: [Correctable] Rails/ActionControllerFlashBeforeRender: Use flash.now before render.
    flash[:alert] = I18n.t('admin.statuses.no_status_selected')

[jorg-vr]

We also have a false positive here

  def user_not_authorized
    if remote_request? || sandbox?
      if current_user.nil?
        # rubocop:disable Rails/RenderInline
        render status: :unauthorized,
               inline: 'You are not authorized to view this page.'
        # rubocop:enable Rails/RenderInline
      else
        head :forbidden
      end
    elsif current_user.nil?
      redirect_to sign_in_path
    else
      flash[:alert] = I18n.t('errors.no_rights') # rubocop:disable Rails/ActionControllerFlashBeforeRender
      if request.referer.present? && URI.parse(request.referer).host == request.host
        redirect_to(request.referer)
      else
        redirect_to(root_path)
      end
    end
  end

[Earlopain]

It might be best to revert #1311 and add appropriate tests. I tried looking into fixing this but didn't come up with a solution. Or could you look into into this @tldn0718, since you are the author of that PR?

[tldn0718]

@Earlopain Yes, I agree to revert that PR.

To be honest, I think this Cop is inherently difficult for static linting to detect properly. Considering the numerous false positives and negatives, I believe removing the Cop altogether is an option worth considering.

[Earlopain]

Thanks for your reply, I openend to revert #1344. I agree with your opinion, that cop has plenty to do to work correctly. The approach it currently takes is too simplistic.

[DChalcraft]

Having a similar issue with false positives from this cop.