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

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

Open
mathieugagne opened this issue Apr 27, 2013 · 4 comments

Comments

@mathieugagne
Copy link

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
Copy link
Author

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)

@davidcelis
Copy link
Owner

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 = {}

@mhuggins
Copy link

mhuggins commented May 4, 2013

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

@davidcelis
Copy link
Owner

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)

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

No branches or pull requests

3 participants