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

False positive for ActionControllerFlashBeforeRender #909

Closed
thestelz opened this issue Jan 9, 2023 · 3 comments · Fixed by #937
Closed

False positive for ActionControllerFlashBeforeRender #909

thestelz opened this issue Jan 9, 2023 · 3 comments · Fixed by #937
Labels
bug Something isn't working

Comments

@thestelz
Copy link
Contributor

thestelz commented Jan 9, 2023

Sorry if this is a duplicate. I was trying to make sure all possible fixes were released before adding this bug report.

We get a Rails/ActionControllerFlashBeforeRender error when the if block does a redirect_to and return before the render block (see below for setup).


Expected behavior

No error should be triggered for the cop Rails/ActionControllerFlashBeforeRender.

Actual behavior

It triggers Rails/ActionControllerFlashBeforeRender when it shouldn't.

Steps to reproduce the problem

I have an admin controller:

# frozen_string_literal: true

module Admin
  class ListingAttributeTypesController < ApplicationController
    before_action :find_listing_attr_type, only: %i[edit update]

    # :nodoc:
    def index
      @listing_attribute_types = ListingAttributeType.ord_inactive.ord_name
    end

    # :nodoc:
    def new
      @listing_attribute_type = ListingAttributeType.new

      render('form')
    end

    # :nodoc:
    def create
      @listing_attribute_type = ListingAttributeType.new(listing_attr_type_params)

      if @listing_attribute_type.save
        flash[:success] = 'Listing attribute type saved!'

        redirect_to(admin_listing_attribute_types_path)

        return
      end

      render('form', status: :unprocessable_entity)
    end

    # :nodoc:
    def edit
      render('form')
    end

    # :nodoc:
    def update
      if @listing_attribute_type.update(listing_attr_type_params)
        flash[:success] = 'Listing attribute type updated!'

        redirect_to(admin_listing_attribute_types_path)

        return
      end

      render('form', status: :unprocessable_entity)
    end

    private

      # Return safe form params
      #
      # @return [ActionController::Parameters]
      #
      def listing_attr_type_params
        params.require(:listing_attribute_type).permit(:id, :name, :inactive)
      end

      # Finds the listing attribute type
      #
      # @return [void]
      #
      def find_listing_attr_type
        @listing_attribute_type = ListingAttributeType.find(params[:id])
      end
  end
end

I then run rubocop and get the following:

Offenses:

app/controllers/admin/listing_attribute_types_controller.rb:24:9: C: [Correctable] Rails/ActionControllerFlashBeforeRender: Use flash.now before render.
        flash[:success] = 'Listing attribute type saved!'
        ^^^^^
app/controllers/admin/listing_attribute_types_controller.rb:42:9: C: [Correctable] Rails/ActionControllerFlashBeforeRender: Use flash.now before render.
        flash[:success] = 'Listing attribute type updated!'
        ^^^^^

RuboCop version

# ruby -v
ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [aarch64-linux]

# rubocop -V
1.42.0 (using Parser 3.2.0.0, rubocop-ast 1.24.1, running on ruby 3.1.2) [aarch64-linux]
  - rubocop-rails 2.17.4

# bundle exec rubocop -V
1.42.0 (using Parser 3.2.0.0, rubocop-ast 1.24.1, running on ruby 3.1.2) [aarch64-linux]
  - rubocop-rails 2.17.4
@koic koic added the bug Something isn't working label Jan 15, 2023
@arsduo
Copy link

arsduo commented Jan 17, 2023

👍 Just encountered the same problem. @thestelz thanks for filing this!

@tagliala
Copy link
Contributor

Hi, I'm not sure it is the same issue, but I have this reduced test case:

# frozen_string_literal: true

class ApplicationController < ActionController::Base
  def create
    @user = User.new(user_params)

    if @user.valid?
      if SomeService.call(@user)
        flash[:success] = t('.flash.success')
        redirect_to @user
      else
        @user.errors.add(:base, 'an error')
        render :new, status: :unprocessable_entity
      end
    else
      render :new, status: :unprocessable_entity
    end
  end
end
rubocop --only Rails/ActionControllerFlashBeforeRender app/controllers/application_controller.rb 
Inspecting 1 file
C

Offenses:

app/controllers/application_controller.rb:9:9: C: [Correctable] Rails/ActionControllerFlashBeforeRender: Use flash.now before render.
        flash[:success] = t('.flash.success')
        ^^^^^

1 file inspected, 1 offense detected, 1 offense autocorrectable

koic added a commit to koic/rubocop-rails that referenced this issue Feb 22, 2023
…shBeforeRender`

Fixes rubocop#909.

This PR fixes a false positive for `Rails/ActionControllerFlashBeforeRender`
when using `flash` before `redirect_to` in `if` branch.
@koic
Copy link
Member

koic commented Feb 22, 2023

Yeah, that is the same problem as the subject. I've opened #937 to resolve the issues.

@koic koic closed this as completed in #937 Feb 22, 2023
koic added a commit that referenced this issue Feb 22, 2023
…on_controller_flash_before_render

[Fix #909] Fix a false positive for `Rails/ActionControllerFlashBeforeRender`
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.

4 participants