From 4c97338d95a6fe5ee454098cd4bd3bc174606e32 Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Tue, 3 Sep 2024 12:25:58 +0200 Subject: [PATCH] [Issue #1269] Revert #1344, add `Rails/ActionControllerFlashBeforeRender` tests from issue #1269 This reverts commit 0f703db581ffbf49830d0c1705c57129b6a7d4aa, reversing changes made to 0f63f00ecee27313e92eedad74a33b7c5ef21be1. This cop would need to do control flow analysis which it just doesn't do. RuboCop also has no mechanism for that. So just reverting this for now to fix the newly introduces false positives --- changelog/fix_revert_flash_before_render.md | 1 + .../action_controller_flash_before_render.rb | 10 +- ...ion_controller_flash_before_render_spec.rb | 246 +++++------------- 3 files changed, 74 insertions(+), 183 deletions(-) create mode 100644 changelog/fix_revert_flash_before_render.md 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