Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rails/ActionControllerFlashBeforeRender reports false positives with if/else and redirect_to #825

Closed
tagliala opened this issue Oct 22, 2022 · 2 comments · Fixed by #840
Closed
Labels
bug Something isn't working

Comments

@tagliala
Copy link
Contributor

tagliala commented Oct 22, 2022

RuboCop reports a false positive with if/else and redirect_to

Follow up from #812 (comment)


Expected behavior

Rubocop does not report errors

Actual behavior

Inspecting 1 file
C

Offenses:

pages_controller.rb:6:7: C: [Correctable] Rails/ActionControllerFlashBeforeRender: Use flash.now before render.
      flash[:success] = t('.flash.success') # false positive
      ^^^^^
pages_controller.rb:17:7: C: [Correctable] Rails/ActionControllerFlashBeforeRender: Use flash.now before render.
      flash[:notice] = t('.flash.notice') # false positive
      ^^^^^
pages_controller.rb:25:7: C: [Correctable] Rails/ActionControllerFlashBeforeRender: Use flash.now before render.
      flash[:success] = t('.flash.success') # false positive
      ^^^^^
pages_controller.rb:27:7: C: [Correctable] Rails/ActionControllerFlashBeforeRender: Use flash.now before render.
      flash[:alert] = t('.flash.fail') # false positive
      ^^^^^

1 file inspected, 4 offenses detected, 4 offenses autocorrectable

Steps to reproduce the problem

  1. Create the following pages_controller.rb file
# frozen_string_literal: true

class PagesController < ApplicationController
  def update
    if @page.save
      flash[:success] = t('.flash.success') # false positive
    else
      flash.now[:alert] = t('.flash.fail')
      render 'edit'
    end

    redirect_to @page
  end

  def dismiss
    if @something
      flash[:notice] = t('.flash.notice') # false positive
    end

    redirect_to pages_path
  end

  def destroy
    if @page.destroy
      flash[:success] = t('.flash.success') # false positive
    else
      flash[:alert] = t('.flash.fail') # false positive
    end

    redirect_to pages_path
  end
end
  1. Run rubocop --require rubocop-rails --only Rails/ActionControllerFlashBeforeRender pages_controller.rb

RuboCop version

$ rubocop --require rubocop-rails -V
1.37.0 (using Parser 3.1.2.1, rubocop-ast 1.23.0, running on ruby 3.1.2) [x86_64-darwin21]
  - rubocop-rails 2.17.0

Also tested against rubocop-rails main

@koic koic added the bug Something isn't working label Oct 22, 2022
@mockdeep
Copy link

We're seeing the same issue when we use redirect_back.

koic added a commit to koic/rubocop-rails that referenced this issue Oct 26, 2022
…shBeforeRender`

Fixes rubocop#825.

This PR fixes a false positive for `Rails/ActionControllerFlashBeforeRender`
when using condition before `redirect_to`.
koic added a commit to koic/rubocop-rails that referenced this issue Oct 26, 2022
…shBeforeRender`

Fixes rubocop#825.

This PR fixes a false positive for `Rails/ActionControllerFlashBeforeRender`
when using condition before `redirect_to`.
koic added a commit to koic/rubocop-rails that referenced this issue Oct 26, 2022
…shBeforeRender`

Fixes rubocop#825.

This PR fixes a false positive for `Rails/ActionControllerFlashBeforeRender`
when using condition before `redirect_to`.
@koic koic closed this as completed in #840 Oct 26, 2022
koic added a commit that referenced this issue Oct 26, 2022
…on_controller_flash_before_render

[Fix #825] Fix a false positive for `Rails/ActionControllerFlashBeforeRender`
@tagliala
Copy link
Contributor Author

Hi, I've tested 2.17.2 but there are still false positives. As far as I can tell, some of them are not easy to detect and also providing a reduced test case is tricky

I've started with #843

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants