Scopes on associations

[dturnerTS]

Punch a hole in the association_proxy for scopes. Also inject the parents id if needed.

Addresses #215

[mathieujobin]

LGTM 👍

[kinopyo]

good job 👍

[kinopyo]

i found this call will append another user_id as parameter in the end..

/users/1/comments?locked=true&user_id=1

[dturnerTS]

Good catch. I'm not sure there is currently a good way to prevent that from happening. Any suggestions?

[dturnerTS]

This is fixed now.

[kinopyo]

thanks, confirmed the behavior 👍

[chewi]

I was hoping this would work for custom_get (which are kinda like server-side scopes) but it doesn't because of this bit of evilness in the scope method.

          # Add the scope method to the Relation class
          Relation.instance_eval do
            define_method(name) { |*args| instance_exec(*args, &code) }
          end

@remiprev, is this really necessary? Can't the relation fall back to the model's class methods in method_missing? I know ActiveRecord does this, though admittedly it is a bit messy when deciding whether methods like find should go to the collection or the model class.

[boie0025]

👍

[dturnerTS]

@hubert Anything I can do to help get this merged? Also, thank you for stepping up to help maintain Her.

[hubert]

@dturnerTS i've blocked out some time on saturday (tomorrow) to work through pending pull requests.

thanks for the kind words. happy to contribute back to the gem.

[chewi]

@edtjones, if you're looking at this one, I'd really appreciate having a chat with you about it.

[edtjones]

@chewi definitely, and looks really useful. Need to absorb fully. Are there any downsides / expected failure modes which mean we should be careful about merging this in?

[chewi]

I'd like it to work more like it does in Active Record, where you can call any class method through an association. The code I quoted above highlights the difference between Her and Active Record. I'm a little hazy on the details after all this time but Active Record is very mature now and I think we should strive to use the same approaches wherever possible as they have clearly served it well.

[shashiprab]

Any plans on merging this PR?

[edtjones]

hi @shashiprab @chewi this has been hanging around for ages - sorry! Don't have the mental bandwidth to deal with the implications of merging this at the moment but I would be delighted if someone else could review.

[zacharywelch]

Remember happily applying this patch a few years back and eventually moving a few things around. Also have a rough patch that would let us remove collection_path as @kinopyo pointed out.

class User
  include Her::Model
  has_many :comments
end

class Comment
  include Her::Model
  belongs_to :user
  scope :approved, -> { where(approved: true) }
end

> user = User.find(1)
# GET "/users/1"
> user.comments.approved
# GET "/users/1/comments?approved=true"

I'll put a PR together soon.