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 Rails/LexicallyScopedActionFilter when conditional statement used #5448

Closed
michniewicz opened this issue Jan 12, 2018 · 2 comments
Labels

Comments

@michniewicz
Copy link
Contributor

Rails/LexicallyScopedActionFilter gives false positive when before action used with conditional statement

Expected behavior

No offenses.

Actual behavior


app/controllers/api/test.rb:4:3: C: Rails/LexicallyScopedActionFilter: update, cancel are not explicitly defined on the controller.
  before_action(:authenticate, only: %i[update cancel]) unless foo
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Steps to reproduce the problem

Use this test class in the /controllers directory

# frozen_string_literal: true

class Test < ActionController
  before_action(:authenticate, only: %i[update cancel]) unless foo

  def update; end

  def cancel; end
end

RuboCop version

$ rubocop -V
0.52.1 (using Parser 2.4.0.2, running on ruby 2.5.0 x86_64-linux)
@pocke pocke added the bug label Jan 12, 2018
@pocke
Copy link
Collaborator

pocke commented Jan 12, 2018

Thanks for the bug report!

The cause is here. https://github.com/bbatsov/rubocop/blob/87f453d20f789fb4cb233a661dbed8794502022a/lib/rubocop/cop/rails/lexically_scoped_action_filter.rb#L70
The code expects node.parent is a class node, but the parent is a if node in this case. So we should replace parent with each_ancestor or something.

@gPrado
Copy link
Contributor

gPrado commented Jan 12, 2018

What about filters inserted via mixins? Will they be addressed too when this issue is resolved?
In those cases, the parent is a module instead of a class:

module FooMixin
  extend ActiveSupport::Concern

  included do
    before_action proc { authenticate },
      only: :foo 
  end

  def foo; end
end

class BarController < ActionController
  include FooMixin
end

wata727 added a commit to wata727/rubocop that referenced this issue Jan 13, 2018
Fixes rubocop#5448

In addition, I also made it possible to inspect modules like mixin.
See also rubocop#5448 (comment)
bbatsov pushed a commit that referenced this issue Jan 14, 2018
Fixes #5448

In addition, I also made it possible to inspect modules like mixin.
See also #5448 (comment)
koic pushed a commit to rubocop/rubocop-rails that referenced this issue Oct 26, 2018
Fixes #5448

In addition, I also made it possible to inspect modules like mixin.
See also rubocop/rubocop#5448 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants