Skip to content

Subclassable ratables #84

Merged
merged 2 commits into from Dec 30, 2013

2 participants

@aaronroyer

Currently, when a ratable has subclasses, the likes applied to the subclasses are essentially abandoned. If the base class is "abstract" then recommendable doesn't seem to work for that ratable. Take this example.

class Book < ActiveRecord::Base
end

class FictionBook < Book
end

class NonfictionBook < Book
end

class User < ActiveRecord::Base
  recommends :books
end

user1.like(nonfiction_book1)
user1.like(nonfiction_book2)
user1.like(nonfiction_book3)

user2.like(nonfiction_book1)
user2.like(nonfiction_book2)

# We don't get a recommendation
user.recommended_books # => []

Recommendations are generated if we only work with Book objects, but not for the subclasses. The reason this happens is because the data for the subclasses are stored in Redis keys namespaced with 'fiction_book' and 'nonfiction_book', even though they're not configured ratable classes. When recommendations are generated, Recommendable iterates over Recommendable.config.ratable_classes, finds Book and looks for data at keys namespaced with 'book'. It doesn't find any of the subclass likes and they are disregarded in calculating recommendations. If only subclasses are used then no recommendations are generated at all.

This PR makes all subclasses of ratables configured with recommends use keys corresponding to the superclass (or more distant ratable ancestor) so all of the recommendations go in the same "bucket". In the above example, user2 would get nonfiction_book3 as a recommendation. If user1 also liked a fiction book, then user2 would get a recommendation for that as well - Book and all of its subclasses act as the same type as far as Recommendable is concerned.

The subclass objects also start showing up in places like Book.top where they didn't before.

If we wanted to place FictionBook in its own ratable category then we could just add recommends :books, :fiction_books and then FictionBooks would be in their own category and Books and NonfictionBooks would still be in the Books category, if that makes sense.

This change only needed a few lines additional code and some tweaks to Redis key mapping. Almost everything in this PR is adding tests to make sure the subclass ratings and recommendations work the same as the normal ones.

aaronroyer added some commits Dec 26, 2013
@aaronroyer aaronroyer Support subclassing ratables
This allows ratable classes to have subclasses that properly accumulate ratings
in the same ratable category.
b01bc8b
@aaronroyer aaronroyer Clean up Redis keys a bit
Rename a few things, add comments, and factor out some other bits relating
to Redis key mapping.
7ec6d8d
@davidcelis
Owner

This is great. Thanks so much for taking the time to fix this accursed STI issue. And with so many tests!

@davidcelis davidcelis merged commit 59c0016 into davidcelis:master Dec 30, 2013

1 check passed

Details default The Travis CI build passed
@aaronroyer

@davidcelis Awesome, thanks for the response! Let me know if you need any help updating documentation when it's time for a release.

There was one quirk I noticed after playing with the fix for a bit. top on a subclass will return the same set of result(s) as on the base class. In the example above FictionBook.top(n) always returns the same thing as Book.top(n).

This is the only thing I've found that is unexpected as a result of the fix. It is a bit of a tricky problem, since no extra metadata about subtypes is stored in Redis you can't just pull ids for a certain subtype from there. It seems too heavy to add that extra data to Redis just for this. One thing that could be done without having to change a lot of things is to pull all of the ids (ick) and query for records with the appropriate type, with the limit being the number passed to top. But maybe this could get really messy fast with the different ORMs/databases?

Really I'm thinking that just not defining top on any subclasses could be the way to go, rather than having it do something that might be unexpected (current status) or putting a lot of code in to make it do the expected thing. Removing it might be consistent with the expectation that Recommendable gives recommendations on the type explicitly configured with recommends :type as a complete group and doesn't break it down any further than that. You can pull recommendations of the configured base type by any means, including with top, and filter them yourself if you want.

Either way, this is kind of what I meant when I offered to help with the docs, because it seems like it would be good to clarify what happens here, regardless of which way it ends up. Besides this, I think it mostly does the expected thing.

Wow, that was a lot of text for a pretty simple matter. Any thoughts?

@davidcelis
Owner

Thanks for the detailed explanation. I'm tempted to force users into using Recommendable a specific way in cases of STI. I definitely don't think it should manage complicated filtering of records by type for .top, as that was really just added in as a bit of a bonus feature... It would be nice if there was a clean way to do this. If databases were better about allowing you to order returned rows by a specific ordering of IDs, I could have .top and all of the #recommended_ methods return Relations instead of arrays and people could call .limit on them. But alas. I see two possibilities:

  1. Your suggestion of noting that, at the expense of not overusing Redis, .top will return all ratable base/subclasses in the hierarchy
  2. We rework the solution such that we go back to storing ratings in keys that reflect the actual class regardless of STI and change the logic to find ratable classes to use something like self_and_descendants (although I believe the logic for this differs between ORMs).
    • So with the books, for example, user.recommended_nonfiction_books could go directly to recommendable:users:1:recommended_nonfiction_books and pull recommendations.
    • Book.top could utilize Redis' ZUNIONSTORE command to combine recommendable:users:1:recommended_nonfiction_books and recommendable:users:1:recommended_fiction_books in a temporary zset, pull the top recommendations, and then DEL the temp zset.
    • Ditto for .top
    • Users need to be specific about what's recommended. recommends :books puts the superclass and all subclasses as ratable. recommends :nonfiction_books does not.

I think 2 would be a better UX, but the code would be more complicated.

@aaronroyer

@davidcelis Agreed on all your points. The #recommended_ methods, in addition to .top, start making it more compelling to break things back down into class/subclass specific keys to allow more specific recommendations. It is definitely better UX, in that it just does the right thing if a user tries calling those methods.

I think the benefit for adding the complexity is proportional to how often people use subclasses to categorize things they will want to pull separate recommendations for. My Book class and subclasses are a bit contrived in this sense - someone probably wouldn't do that just for categorization and there would have to be significantly diverging logic to justify the subclasses. The (more common?) alternative would be to just add a column with a category, associated tags, or some other associated category model. In these cases Recommendable would not help to narrow things down, even with the additional work put in.

So it would really be a sort of bonus feature for those using STI, but not for those categorizing other ways. Maybe that's a good thing. I'm not sure that I would want to encourage people to use STI just for that, if they aren't using it already, but it would always be an option.

Personally, I guess I would lean toward keeping it simple, unless there is a lot of demand for splitting the data out for STI. In my specific case (which motivated the fix) I don't need it. But I don't know if that's common or not.

Whatever you decide I'd be willing to help out where I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.