CODEWITHSUNDEEP
ruby-on-railsrubymongoid
SupremeA
SupremeA

Reputation: 1621

Chaining this query throws error

I am trying to make a query that:

  1. Finds/Gets the object (Coupon.code)
  2. Checks if the coupon is expired (expires_at)
  3. Checks if the coupon has been used up. (coupons_remaining)

I got some syntax using a newer version of ruby but it isnt working with my version 2.2.1 The syntax I have is 

  def self.get(code)
   where(code: normalize_code(code)).
   where("coupon_count > ? OR coupon_count IS NULL", 0).
   where("expires_at > ? OR expires_at IS NULL", Time.now).
   take(1)
 end

This throws an error of wrong number of arguments (2 for 1) which is because my rails doesn't seem to recognize the 2 arguments ("coupon_count > ? OR coupon_count IS NULL", 0) so I have tried to change it but when I change them to something like this (which in my heart felt horribly wrong)

def self.get(code)
 where(code: normalize_code(code)).
 where(coupon_count: self.coupon_count > 0 || self.coupon_count.nil? ).
 where(expires_at: self.expires_at > Time.now || self.expires_at.nil? ).
 take(1)
end

I get undefined method `coupon_count' for Coupon:Class

I am short on ideas can someone help me get the syntax for this get method in my model? By the way if it matters I am using mongoid 5.1.0

Upvotes: 1

Views: 35

Answers (2)

mu is too short
mu is too short

Reputation: 434805

I feel your pain. Combining OR and AND in MongoDB is a bit messy because you're not really working with a query language at all, you're just building a hash. Similar complications apply if you might apply multiple conditions to the same field. This is also why you can't include SQL-like snippets like you can with ActiveRecord.

For example, to express:

"coupon_count > ? OR coupon_count IS NULL", 0

you need to build a hash like:

:$or => [
    { :coupon_count.gt => 0   },
    { :coupon_count    => nil }
]

but if you try to add another OR to that, you'll overwrite the existing :$or key and get confusion. Instead, you need to be aware that there will be multiple ORs and manually avoid the duplicate by saying :$and:

:$and => [
    {
        :$or => [
            { :coupon_count.gt => 0   },
            { :coupon_count    => nil }
        ]
    }, {
        :$or => [
            { :expires_at.gt => Time.now },
            { :expires_at    => nil      }
        ]
    }
]

Then adding the code condition is straight forward:

:code => normalize_code(code),
:$and => [ ... ]

That makes the whole thing a rather hideous monstrosity:

def self.get(code)
  where(
    :code => normalize_code(code),
    :$and => [
      {
        :$or => [
          { :coupon_count.gt => 0   },
          { :coupon_count    => nil }
        ]
      }, {
        :$or => [
          { :expires_at.gt => Time.now },
          { :expires_at    => nil      }
        ]
      }
    ]
  ).first
end

You could also use find_by(that_big_mess) instead of where(that_big_mess).first. Also, if you expect the query to match multiple documents, then you probably want to add an order call to make sure you get the one you want. You could probably use the and and or query methods instead of a single hash but I doubt it will make things easy to read, understand, or maintain.

I try to avoid ORs with MongoDB because the queries lose their little minds fast and you're left with some gibbering eldritch horror that you don't want to think about too much. You're usually better off precomputing parts of your queries with generated fields (that you have to maintain and sanity check to make sure they are correct); for example, you could add another field that is true if coupon_count is positive or nil and then update that field in a before_validation hook when coupon_count changes.

Upvotes: 1

Anthony E
Anthony E

Reputation: 11235

You've defined a class method, so self in this circumstance references the Coupon class rather than a Coupon instance.

Try the following:

 scope :not_expired, -> { where("expires_at > ? OR expires_at IS NULL", Time.now) }
 scope :previously_used, -> { where("coupon_count > 0 OR coupon_count IS NULL") }

 def self.get(code)
   previously_used.not_expired.find_by!(code: normalize_code(code))
 end

Upvotes: 1

Related Questions