diff --git a/changelog/fix_revert_flash_before_render.md b/changelog/fix_revert_flash_before_render.md new file mode 100644 index 0000000000..4a7cab0316 --- /dev/null +++ b/changelog/fix_revert_flash_before_render.md @@ -0,0 +1 @@ +* [#1269](https://github.com/rubocop/rubocop-rails/issues/1269): Fix false positives for `Rails/ActionControllerFlashBeforeRender` in combination with implicit returns. ([@earlopain][]) diff --git a/lib/rubocop/cop/rails/action_controller_flash_before_render.rb b/lib/rubocop/cop/rails/action_controller_flash_before_render.rb index bf4602cdab..806e92e455 100644 --- a/lib/rubocop/cop/rails/action_controller_flash_before_render.rb +++ b/lib/rubocop/cop/rails/action_controller_flash_before_render.rb @@ -72,13 +72,13 @@ def followed_by_render?(flash_node) if (node = context.each_ancestor(:if, :rescue).first) return false if use_redirect_to?(context) - context = node.rescue_type? ? node.parent : node + context = node + elsif context.right_siblings.empty? + return true end + context = context.right_siblings - siblings = context.right_siblings - return true if siblings.empty? - - siblings.compact.any? do |render_candidate| + context.compact.any? do |render_candidate| render?(render_candidate) end end 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 index 0c8f682e16..6ca55d122b 100644 --- a/spec/rubocop/cop/rails/action_controller_flash_before_render_spec.rb +++ b/spec/rubocop/cop/rails/action_controller_flash_before_render_spec.rb @@ -128,36 +128,9 @@ def create end end RUBY - - expect_offense(<<~RUBY) - class HomeController < #{parent_class} - def create - flash[:alert] = "msg" if condition - ^^^^^ Use `flash.now` before `render`. - end - end - RUBY - - expect_correction(<<~RUBY) - class HomeController < #{parent_class} - def create - flash.now[:alert] = "msg" if condition - end - end - RUBY end it 'does not register an offense when using `flash` before `redirect_to`' do - expect_no_offenses(<<~RUBY) - class HomeController < #{parent_class} - def create - flash[:alert] = "msg" if condition - - redirect_to :index - end - end - RUBY - expect_no_offenses(<<~RUBY) class HomeController < #{parent_class} def create @@ -172,16 +145,6 @@ def create end it 'does not register an offense when using `flash` before `redirect_back`' do - expect_no_offenses(<<~RUBY) - class HomeController < #{parent_class} - def create - flash[:alert] = "msg" if condition - - redirect_back fallback_location: root_path - end - end - RUBY - expect_no_offenses(<<~RUBY) class HomeController < #{parent_class} def create @@ -195,7 +158,7 @@ def create RUBY end - it 'registers an offense when using `flash` in multiline `if` branch before `render`' do + it 'registers an offense when using `flash` in multiline `if` branch before `render_to`' do expect_offense(<<~RUBY) class HomeController < #{parent_class} def create @@ -222,29 +185,6 @@ def create end end RUBY - - expect_offense(<<~RUBY) - class HomeController < #{parent_class} - def create - if condition - do_something - flash[:alert] = "msg" - ^^^^^ Use `flash.now` before `render`. - end - end - end - RUBY - - expect_correction(<<~RUBY) - class HomeController < #{parent_class} - def create - if condition - do_something - flash.now[:alert] = "msg" - end - end - end - RUBY end it 'does not register an offense when using `flash` in multiline `if` branch before `redirect_to`' do @@ -277,81 +217,6 @@ def create end end RUBY - - expect_no_offenses(<<~RUBY) - class HomeController < #{parent_class} - def create - if condition - flash[:alert] = "msg" - redirect_to :index - - return - end - end - end - RUBY - end - - it 'registers an offense when using `flash` in multiline `rescue` branch before `render`' do - expect_offense(<<~RUBY) - class HomeController < #{parent_class} - def create - begin - do_something - flash[:alert] = "msg in begin" - ^^^^^ Use `flash.now` before `render`. - rescue - flash[:alert] = "msg in rescue" - ^^^^^ Use `flash.now` before `render`. - end - - render :index - end - end - RUBY - - expect_correction(<<~RUBY) - class HomeController < #{parent_class} - def create - begin - do_something - flash.now[:alert] = "msg in begin" - rescue - flash.now[:alert] = "msg in rescue" - end - - render :index - end - end - RUBY - - expect_offense(<<~RUBY) - class HomeController < #{parent_class} - def create - begin - do_something - flash[:alert] = "msg in begin" - ^^^^^ Use `flash.now` before `render`. - rescue - flash[:alert] = "msg in rescue" - ^^^^^ Use `flash.now` before `render`. - end - end - end - RUBY - - expect_correction(<<~RUBY) - class HomeController < #{parent_class} - def create - begin - do_something - flash.now[:alert] = "msg in begin" - rescue - flash.now[:alert] = "msg in rescue" - end - end - end - RUBY end it 'does not register an offense when using `flash` in multiline `rescue` branch before `redirect_to`' do @@ -370,48 +235,6 @@ def create end RUBY end - - it 'does not register an offense when using `flash` before `redirect_to` in `rescue` branch' do - expect_no_offenses(<<~RUBY) - class HomeController < #{parent_class} - def create - begin - do_something - flash[:alert] = "msg in begin" - redirect_to :index - - return - rescue - flash[:alert] = "msg in rescue" - redirect_to :index - - return - end - - render :index - end - end - RUBY - - expect_no_offenses(<<~RUBY) - class HomeController < #{parent_class} - def create - begin - do_something - flash[:alert] = "msg in begin" - redirect_to :index - - return - rescue - flash[:alert] = "msg in rescue" - redirect_to :index - - return - end - end - end - RUBY - end end end @@ -505,4 +328,71 @@ def create RUBY end end + + context 'when using `flash` after `render` and `redirect_to` is used in implicit return branch ' \ + 'and render is is used in the other branch' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class HomeController < ApplicationController + def create + if foo.update(params) + flash[:success] = 'msg' + + if redirect_to_index? + redirect_to index + else + redirect_to path(foo) + end + else + flash.now[:alert] = 'msg' + render :edit, status: :unprocessable_entity + end + end + end + RUBY + end + end + + context 'when using `flash` after `render` and `render` is part of a different preceding branch' \ + 'that implicitly returns' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class HomeController < ApplicationController + def create + if remote_request? || sandbox? + if current_user.nil? + render :index + else + head :forbidden + end + elsif current_user.nil? + redirect_to sign_in_path + else + flash[:alert] = 'msg' + if request.referer.present? + redirect_to(request.referer) + else + redirect_to(root_path) + end + end + end + end + RUBY + end + end + + context 'when using `flash` in `rescue` and `redirect_to` in `ensure`' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class HomeController < ApplicationController + def create + rescue + flash[:alert] = 'msg' + ensure + redirect_to :index + end + end + RUBY + end + end end