CODEWITHSUNDEEP
ruby-on-railsconditional-statementsupdateshas-many-through
Matthew Bennett
Matthew Bennett

Reputation: 303

Rails: update existing has_many through record via controller?

So two thirds of this works. Every time a User reads an Article, a History record is created (has_many through), which just says "User read Article at Read_Date_X".

The database is ok, the models are ok, the read_date param is permitted in the History controller, and the following operation works both 1) to check if a User has read an article before and 2) to create a new History record if it is the first time on this article.

But I cannot work out why the middle bit (to just update the read_date on an existing record) is not working. It doesn't matter if I try it with h.save! or h.update().

h = History.where(article_id: @article, user_id: current_user)
if h.exists?
  h = History.where(article_id: @article, user_id: current_user)
  h.read_date = Time.now
  h.save!
else
  h = History.new
  h.article_id = @article.id
  h.user_id = current_user.id
  h.read_date = Time.now
  h.save!
end

The error it throws if it finds an existing record is: 

undefined method `read_date=' for #<History::ActiveRecord_Relation:0x007fe7f30a5e50>

UPDATE: working answer

So Derek was right, and this version works. The middle bit needed a single instance, not an array, which is what the top conditional (without .first) was checking for. Using that to return a single record, though, means you need to swap "exists?" to "present?" in the second part.

h = History.where(article_id: @article, user_id: current_user).first
if h.present?
  h.read_date = Time.now
  h.save!
else
  h = History.new
  h.article_id = @article.id
  h.user_id = current_user.id
  h.read_date = Time.now
  h.save!
end

Upvotes: 0

Views: 1687

Answers (2)

jvillian
jvillian

Reputation: 20263

I realize this question is already answered. Here are a couple of additional thoughts and suggestions.

  • I would not have a separate read_date attribute. Just use updated_at instead. It's already there for you. And, the way your code works, read_date and updated_at will always be (essentially) the same.

  • When looking up whether the history exists, you can do current_user.histories.where(article: @article). IMO, that seems cleaner than: History.where(article_id: @article, user_id: current_user).first.

  • You can avoid all that exists? and present? business by just checking if the h assignment was successful. Thus, if h = current_user.histories.where(article: @article).

  • If you go the route of using updated_at instead of read_date, then you can set updated_at to Time.now by simply doing h.touch.

  • I would use the << method provided by has_many :through (instead of building the history record by hand). Again, if you use updated_at instead of read_date, then you can use this approach.

So, you could boil your code down to:

if h = current_user.histories.where(article: @article)
  h.touch 
else 
  current_user.articles << @article 
end

You could use a ternary operator instead of that if then else, in which case it might look something like:

current_user.histories.where(article: @article).tap do |h|
  h ? h.touch : current_user.articles << @article 
end

Upvotes: 1

Derek Hopper
Derek Hopper

Reputation: 2258

History.where(article_id: @article, user_id: current_user) is returning a History::ActiveRecord_Relation. If you want to set the read_date, you'll want to get a single record.

Here's one way you could do this with what you have currently:

h = History.where(article_id: @article, user_id: current_user).first

Another way you could handle this is by using find_by instead of where. This would return a single record. Like this:

h = History.find_by(article_id: @article, user_id: current_user)

However, if it's possible for a user to have many history records for an article, I would stick to the way you're doing things and make one change. If for some reason you have a lot of history records, this may not be very efficient though.

histories = History.where(article_id: @article, user_id: current_user)
histories.each { |history| history.update(read_date: Time.now) }

Upvotes: 1

Related Questions