Skip to content

Commit

Permalink
Merge pull request #1344 from Earlopain/revert-1311
Browse files Browse the repository at this point in the history 
Revert #1311 - false negatives for `Rails/ActionControllerFlashBeforeRender`
  • Loading branch information
@koic
koic authored Sep 12, 2024
2 parents 5e66785 + 4c97338 commit 210f42e
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 183 deletions.
1 change: 1 addition & 0 deletions changelog/fix_revert_flash_before_render.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#1269](https://github.com/rubocop/rubocop-rails/issues/1269): Fix false positives for `Rails/ActionControllerFlashBeforeRender` in combination with implicit returns. ([@earlopain][])
10 changes: 5 additions & 5 deletions lib/rubocop/cop/rails/action_controller_flash_before_render.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
246 changes: 68 additions & 178 deletions spec/rubocop/cop/rails/action_controller_flash_before_render_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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

Expand Down Expand Up @@ -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

0 comments on commit 210f42e

Please sign in to comment.