Merge pull request #759 from americodls/flash-before-render

Add `Rails/ActionControllerFlashBeforeRender` cop

Author: koic

changelog/new_add_railsactioncontrollerflashbeforerender.md

@@ -0,0 +1 @@
+* [#759](https://github.com/rubocop/rubocop-rails/pull/759): Add new `Rails/ActionControllerFlashBeforeRender` cop. ([@americodls][])

config/default.yml

@@ -65,6 +65,14 @@ Rails:
   Enabled: true
   DocumentationBaseURL: https://docs.rubocop.org/rubocop-rails
 
+Rails/ActionControllerFlashBeforeRender:
+  Description: 'Use `flash.now` instead of `flash` before `render`.'
+  StyleGuide: 'https://rails.rubystyle.guide/#flash-before-render'
+  Reference: 'https://api.rubyonrails.org/classes/ActionController/FlashBeforeRender.html'
+  Enabled: 'pending'
+  SafeAutocorrect: false
+  VersionAdded: '<<next>>'
+
 Rails/ActionControllerTestCase:
   Description: 'Use `ActionDispatch::IntegrationTest` instead of `ActionController::TestCase`.'
   StyleGuide: 'https://rails.rubystyle.guide/#integration-testing'

lib/rubocop/cop/rails/action_controller_flash_before_render.rb

@@ -0,0 +1,98 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # Using `flash` assignment before `render` in Rails controllers will persist the message for too long.
+      # Check https://guides.rubyonrails.org/action_controller_overview.html#flash-now
+      #
+      # @safety
+      #   This cop's autocorrection is unsafe because it replaces `flash` by `flash.now`.
+      #   Even though it is usually a mistake, it might be used intentionally.
+      #
+      # @example
+      #
+      #   # bad
+      #   class HomeController < ApplicationController
+      #     def create
+      #       flash[:alert] = "msg"
+      #       render :index
+      #     end
+      #   end
+      #
+      #   # good
+      #   class HomeController < ApplicationController
+      #     def create
+      #       flash.now[:alert] = "msg"
+      #       render :index
+      #     end
+      #   end
+      #
+      class ActionControllerFlashBeforeRender < Base
+        extend AutoCorrector
+
+        MSG = 'Use `flash.now` before `render`.'
+
+        def_node_search :flash_assignment?, <<~PATTERN
+          ^(send (send nil? :flash) :[]= ...)
+        PATTERN
+
+        def_node_search :render?, <<~PATTERN
+          (send nil? :render ...)
+        PATTERN
+
+        def_node_search :action_controller?, <<~PATTERN
+          {
+            (const nil? :ApplicationController)
+            (const (const nil? :ActionController) :Base)
+          }
+        PATTERN
+
+        RESTRICT_ON_SEND = [:flash].freeze
+
+        def on_send(flash_node)
+          return unless flash_assignment?(flash_node)
+
+          return unless followed_by_render?(flash_node)
+
+          return unless instance_method_or_block?(flash_node)
+
+          return unless inherit_action_controller_base?(flash_node)
+
+          add_offense(flash_node) do |corrector|
+            corrector.replace(flash_node, 'flash.now')
+          end
+        end
+
+        private
+
+        def followed_by_render?(flash_node)
+          flash_assigment_node = find_ancestor(flash_node, type: :send)
+          context = flash_assigment_node.parent
+
+          context.each_child_node.any? do |node|
+            render?(node)
+          end
+        end
+
+        def inherit_action_controller_base?(node)
+          class_node = find_ancestor(node, type: :class)
+          return unless class_node
+
+          action_controller?(class_node)
+        end
+
+        def instance_method_or_block?(node)
+          def_node = find_ancestor(node, type: :def)
+          block_node = find_ancestor(node, type: :block)
+
+          def_node || block_node
+        end
+
+        def find_ancestor(node, type:)
+          node.each_ancestor(type).first
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails_cops.rb

@@ -8,6 +8,7 @@
 require_relative 'mixin/migrations_helper'
 require_relative 'mixin/target_rails_version'
 
+require_relative 'rails/action_controller_flash_before_render'
 require_relative 'rails/action_controller_test_case'
 require_relative 'rails/action_filter'
 require_relative 'rails/active_record_aliases'

spec/rubocop/cop/rails/action_controller_flash_before_render_spec.rb

@@ -0,0 +1,131 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::ActionControllerFlashBeforeRender, :config do
+  context 'when using `flash` before `render`' do
+    context 'within an instance method' do
+      %w[ActionController::Base ApplicationController].each do |parent_class|
+        context "within a class inherited from #{parent_class}" do
+          it 'registers an offense and corrects' do
+            expect_offense(<<~RUBY)
+              class HomeController < #{parent_class}
+                def create
+                  flash[:alert] = "msg"
+                  ^^^^^ Use `flash.now` before `render`.
+                  render :index
+                end
+              end
+            RUBY
+
+            expect_correction(<<~RUBY)
+              class HomeController < #{parent_class}
+                def create
+                  flash.now[:alert] = "msg"
+                  render :index
+                end
+              end
+            RUBY
+          end
+        end
+      end
+
+      context 'within a non Rails controller class' do
+        it 'does not register an offense' do
+          expect_no_offenses(<<~RUBY)
+            class NonController < ApplicationRecord
+              def create
+                flash[:alert] = "msg"
+                render :index
+              end
+            end
+          RUBY
+        end
+      end
+    end
+
+    context 'within a block' do
+      %w[ActionController::Base ApplicationController].each do |parent_class|
+        context "within a class inherited from #{parent_class}" do
+          it 'registers an offense and corrects' do
+            expect_offense(<<~RUBY)
+              class HomeController < #{parent_class}
+                before_action do
+                  flash[:alert] = "msg"
+                  ^^^^^ Use `flash.now` before `render`.
+                  render :index
+                end
+              end
+            RUBY
+
+            expect_correction(<<~RUBY)
+              class HomeController < #{parent_class}
+                before_action do
+                  flash.now[:alert] = "msg"
+                  render :index
+                end
+              end
+            RUBY
+          end
+        end
+      end
+
+      context 'within a non Rails controller class' do
+        it 'does not register an offense' do
+          expect_no_offenses(<<~RUBY)
+            class NonController < ApplicationRecord
+              before_action do
+                flash[:alert] = "msg"
+                render :index
+              end
+            end
+          RUBY
+        end
+      end
+    end
+
+    context 'within a class method' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          class HomeController < ApplicationController
+            def self.create
+              flash[:alert] = "msg"
+              render :index
+            end
+          end
+        RUBY
+      end
+    end
+
+    context 'within a class body' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          class HomeController < ApplicationController
+            flash[:alert] = "msg"
+            render :index
+          end
+        RUBY
+      end
+    end
+
+    context 'with no context' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          flash[:alert] = "msg"
+          render :index
+        RUBY
+      end
+    end
+  end
+
+  context 'when using `flash` before `redirect_to`' do
+    it 'does not register an offense' do
+      expect_no_offenses(<<~RUBY)
+        class HomeController < ApplicationController
+          def create
+            flash[:alert] = "msg"
+            redirect_to "https://www.example.com/"
+          end
+        end
+      RUBY
+    end
+  end
+end