diff --git a/changelog/new_add_railsactioncontrollerflashbeforerender.md b/changelog/new_add_railsactioncontrollerflashbeforerender.md new file mode 100644 index 0000000000..fd97953452 --- /dev/null +++ b/changelog/new_add_railsactioncontrollerflashbeforerender.md @@ -0,0 +1 @@ +* [#759](https://github.com/rubocop/rubocop-rails/pull/759): Add new `Rails/ActionControllerFlashBeforeRender` cop. ([@americodls][]) diff --git a/config/default.yml b/config/default.yml index 284b45cb11..286029a31a 100644 --- a/config/default.yml +++ b/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: '<>' + Rails/ActionControllerTestCase: Description: 'Use `ActionDispatch::IntegrationTest` instead of `ActionController::TestCase`.' StyleGuide: 'https://rails.rubystyle.guide/#integration-testing' diff --git a/lib/rubocop/cop/rails/action_controller_flash_before_render.rb b/lib/rubocop/cop/rails/action_controller_flash_before_render.rb new file mode 100644 index 0000000000..92a7af6296 --- /dev/null +++ b/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 diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 2f1e0996d1..b8c7612879 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/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' diff --git a/spec/rubocop/cop/rails/action_controller_flash_before_render_spec.rb b/spec/rubocop/cop/rails/action_controller_flash_before_render_spec.rb new file mode 100644 index 0000000000..8b8a84e6f1 --- /dev/null +++ b/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