[Fix #504] Rails/FindBy being wrongly triggered on non Active Record methods

[nvasilevski]

Fixes - #504
If I'm not mistaken the #502 PR removed an important part of the Rails/FindBy cop which was checking for a presence of where() method call in the method chain.

Without this check the cop tries to autocorrect an offence for regular enumerable first and take methods

This PR basically brings the check back with a bit different name.

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.
• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.
• If this is a new cop, consider making a corresponding update to the Rails Style Guide.

[nvasilevski]

Initially the matcher was called where_first but I thought it may be confused with the ignore_where_first? method.
Would love to hear better name suggestions ❤️

[koic]

I overlooked this case 😅 I think it's better to replace it with logic equivalent to SCOPE_METHODS instead of def_node_matcher.
https://github.com/rubocop/rubocop-rails/blob/v2.11.0/lib/rubocop/cop/rails/find_each.rb#L33

Besides where, there are all, eager_load, includes, and so on.

[nvasilevski]

Ah, interesting, we can use the SCOPE_METHODS approach, but If I'm not mistaken for our case it would be simply SCOPE_METHODS = %i[where] and that's it. Because where().take and where().first are the only two cases we want to check in the scope of this cop. Like we can't suggest find_by in cases like User.all.take(5) so we need just to skip those

Let me know if I'm missing something. Thanks!

[nvasilevski]

I've rebased on master and refactored it to avoid defining the def_node_matcher and explicitly check for receiver&.method?(:where)
I also removed the receiver.block_type? check because it should be covered by the unless receiver&.method?(:where) check

[koic]

Ah, you're right! Thank you!

[beauraF]

I'll like to add, since #502, this kind of code lead Rubocop to crash. @nvasilevski work should fix it. 🚀

  def simple_history
    versions.includes(:account).reorder(created_at: :desc).limit(20).slice_when do |prev, curr|
      prev.whodunnit != curr.whodunnit || prev.created_at.to_date != curr.created_at.to_date
    end.map { |versions| versions.max_by(&:created_at) }.take(5)
  end

An error occurred while Rails/FindBy cop was inspecting app/models/concerns/profile_content_reviewable.rb:26:4.
undefined method `selector' for #<Parser::Source::Map::Collection:0x00007fd8adc424e8 @end=#<Parser::Source::Range app/models/concerns/profile_content_reviewable.rb 764...765>, @begin=#<Parser::Source::Range app/models/concerns/profile_content_reviewable.rb 721...722>, @expression=#<Parser::Source::Range app/models/concerns/profile_content_reviewable.rb 524...765>, @node=s(:block,
  s(:send,
    s(:block,
      s(:send,
        s(:send,
          s(:send,
            s(:send,
              s(:send, nil, :versions), :includes,
              s(:sym, :account)), :reorder,
            s(:hash,
              s(:pair,
                s(:sym, :created_at),
                s(:sym, :desc)))), :limit,
          s(:int, 20)), :slice_when),
      s(:args,
        s(:arg, :prev),
        s(:arg, :curr)),
      s(:or,
        s(:send,
          s(:send,
            s(:lvar, :prev), :whodunnit), :!=,
          s(:send,
            s(:lvar, :curr), :whodunnit)),
        s(:send,
          s(:send,
            s(:send,
              s(:lvar, :prev), :created_at), :to_date), :!=,
          s(:send,
            s(:send,
              s(:lvar, :curr), :created_at), :to_date)))), :map),
  s(:args,
    s(:arg, :versions)),
  s(:send,
    s(:lvar, :versions), :max_by,
    s(:block_pass,
      s(:sym, :created_at))))>

Thanks a lot @koic @nvasilevski. ❤️

[koic]

@beauraF Thank you for feedback. It's different from the problem fix in this PR, so I opened another PR #507 to fix it.