recommendable icon indicating copy to clipboard operation
recommendable copied to clipboard

user.bookmarks, user.likes and user.dislikes fail unless my recommendable class has an 'id' key

Open mathieugagne opened this issue 11 years ago • 4 comments

id is hard coded as the primary key. Nothing wrong normally but I have this legacy database ported to Rails that doesn't respond to @instance.id.

It fails in many levels but the first would be here: https://github.com/davidcelis/recommendable/blob/master/lib/recommendable/rater/bookmarker.rb#L74

If my primary key is a string like 'AAPL' then calling to_i on it returns 0. Hence requesting all bookmarks for a specific user will trigger:

Company.where(id: 0)

Where there are no columns id for companies AND there wouldn't be an id for 0. Would be great if either we could pass as an option to recommends or if it could look at ActiveRecord relationships: has_many :companies, primary_key: :ticker

mathieugagne avatar Apr 27 '13 04:04 mathieugagne

Current workaround:

Recommendable.configure do |config|
  config.orm = :mongoid #avoids the .to_i
end

Then call:

ids = current_user.send(:bookmarked_ids_for, :company)
Company.where(ticker: ids)

mathieugagne avatar Apr 27 '13 16:04 mathieugagne

Legacy databases are an edge case, I think.

That being said, perhaps it's time to introduce a per-recommendable configuration, kind of like what you suggested with the latter, and just allow a recommendable declaration to be one class with an options hash:

recommends :things, options = {}

davidcelis avatar Apr 27 '13 17:04 davidcelis

This would also presumably fail for non-legacy databases using the new Rails 4 Postgres UUID PK.

mhuggins avatar May 04 '13 12:05 mhuggins

I've been thinking about this as well, but it raises more questions. How many people do we think are actually going to jump on the UUID PK thing? It's not the default, my guess is many wouldn't configure it and would just stay with auto-incrementing integers. Unless your app is dealing with a ton of data, auto-incrementing integers really aren't that big of a deal.

That being said, it does seem like assuming that the ID is an integer is a problem. I'll provide a patch sometime in the next few days that does type checking on the column before doing mutations (i.e. ActiveRecord has user.column_for_attribute('id').type)

davidcelis avatar May 04 '13 16:05 davidcelis