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

Add Automatic Negative Scopes #795

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

danielricecodes
Copy link

Opening a PR to see what the community reaction is to automatic negative scopes?

With the following example state machine:

class Job < ActiveRecord::Base
  include AASM

  aasm do
    state :sleeping, initial: true
    state :running
    state :cleaning
  end
end

Would produce 6 scopes instead of the usual 3.

  • Job.sleeping
  • Job.not_sleeping
  • Job.running
  • Job.not_running
  • Job.cleaning
  • Job.not_cleaning

Sometimes you want to query for everything except a certain state.

@gildo
Copy link

gildo commented Jan 2, 2023

Also rails/rails#35381

@tagliala
Copy link

Interesting, thanks.

Should close #497

Any chances to add specs?

@danielricecodes danielricecodes force-pushed the danielricecodes-automatic-not-scopes branch from 598dc22 to 9368e02 Compare February 11, 2023 17:12
@danielricecodes
Copy link
Author

Interesting, thanks.

Should close #497

Any chances to add specs?

Thanks for the nudge. This branch has been rebased onto current master and I added some specs for it. If there are other places I need to add tests, please let me know where.

@danielricecodes
Copy link
Author

danielricecodes commented Feb 11, 2023

@tagliala - another thing I noticed in the source is that there appears to be an "if ActiveRecord is 3 or higher" in here and I'm not sure why it's still there. The else contains Rails 2 scope syntax and this gem does not support Rails 2 anymore. The if/else looks like it can be removed, unless I'm missing something.

      module ClassMethods
        def aasm_create_scope(state_machine_name, scope_name)
          if ActiveRecord::VERSION::MAJOR >= 3
            conditions = { aasm(state_machine_name).attribute_name => scope_name.to_s }
            class_eval do
              scope scope_name, lambda { where(table_name => conditions) }
              scope "not_#{scope_name}", lambda { where.not(table_name => conditions) }
            end
          else # Rails 2 scope syntax.  This can be removed?
            conditions = {
              table_name => { aasm(state_machine_name).attribute_name => scope_name.to_s }
            }
            class_eval do
              named_scope scope_name, :conditions => conditions
            end
          end
        end
      end

@codeclimate
Copy link

codeclimate bot commented Feb 11, 2023

Code Climate has analyzed commit dc7fa4d and detected 0 issues on this pull request.

View more on Code Climate.

Copy link

@tagliala tagliala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, just a couple of minor improvements for uniformity

I'm not a contributor of this gem

@@ -324,6 +324,13 @@
expect(SimpleNewDsl.unknown_scope.is_a?(ActiveRecord::Relation)).to be_truthy
expect(SimpleNewDsl.another_unknown_scope.is_a?(ActiveRecord::Relation)).to be_truthy
end
it "should add negative scopes for each state" do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it "should add negative scopes for each state" do
it "adds negative scopes for each state" do

@@ -346,11 +353,21 @@
expect(ImplementedAbstractClassDsl.unknown_scope.is_a?(ActiveRecord::Relation)).to be_truthy
expect(ImplementedAbstractClassDsl.another_unknown_scope.is_a?(ActiveRecord::Relation)).to be_truthy
end
it "should add negative scopes without the table_name" do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it "should add negative scopes without the table_name" do
it "adds negative scopes without the table_name" do

@tagliala
Copy link

The else contains Rails 2 scope syntax and this gem does not support Rails 2 anymore

As a non contributor, I can't tell.

The changelog says that this gem is not compatible anymore with Rails 3 starting from 5.1.0, so I guess that it is dead code https://github.com/aasm/aasm/blob/master/CHANGELOG.md#510

I would not remove that in this PR

@danielricecodes
Copy link
Author

@rubyist - can a maintainer please enable CI on this PR, please? Thank you 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants