0

So I attempted to build what I asked about in this question: Fix voting mechanism

However, this solution doesn't work. A user can still vote however many times he or she wants. How could I fix this and/or refactor?

def create       
    @video = Video.find(params[:video_id])
    @vote = @video.video_votes.new
    @vote.user = current_user

    if params[:type] == "up"
      @vote.value = 1
    else
      @vote.value = -1
    end

  if @previous_vote.nil?
    if @vote.save
      respond_to do |format|
        format.html { redirect_to @video }
        format.js
      end
    else
      respond_to do |format|
        format.html { redirect_to @video }
        format.js {render 'fail_create.js.erb'}
      end
    end
  elsif @previous_vote.value == params[:type]
    @previous_vote.destroy
  else
    @previous_vote.destroy    
    if @vote.save
      respond_to do |format|
        format.html { redirect_to @video }
        format.js
      end
    else
      respond_to do |format|
        format.html { redirect_to @video }
        format.js {render 'fail_create.js.erb'}
      end
    end
  end
  @previous_vote = VideoVote.where(:video_id => params[:video_id], :user_id => current_user.id).first
end
Community
  • 1
  • 1
Justin Meltzer
  • 13,318
  • 32
  • 117
  • 182

1 Answers1

3

@previous_vote seems to be nil at the beginning of every request?

I would personally do away with all the logic in the controller and put uniqueness contraints at Model or database level where they belong.

Updated

This is probably full of errors but treat it as pseudo-code

Models something like:

class Video < ActiveRecord::Base
  has_many :votes
end

class Vote < ActiveRecord::Base
  belongs_to :user
  belongs_to :video
  validates_uniqueness_of :user_id, :scope => :video_id # only one vote per person per video
end

class User < ActiveRecord::Base
  has_many :votes
end

Controller:

def create
  @video = Video.find(params[:video_id])
  @vote = current_user.votes.find_or_create_by_video_id(@video.id)

  # change this next block of code so you assign value to the vote based on whatever logic you need
  if you_need_to_do_anything_to_change_its_value
    @vote.value = :whatever
  end

  if @vote.save
    redirect_to @video
  else
    render :whatever_is_appropriate
  end
end
lebreeze
  • 5,094
  • 26
  • 34
  • What's the reason it's nil? And why move it to the model? And how would the code look in the model? – Justin Meltzer Mar 19 '11 at 08:26
  • if the vote fails the validation, will it fail silently? Also, when is the validation run? When I try to save the vote into the database? – Justin Meltzer Mar 19 '11 at 08:44
  • if the vote fails `@vote.save` will return false `@vote.save!` would raise an Exception. The validation will run during the transaction triggered by calling `save`. You can find the callback structure of ActiveRecord here: http://api.rubyonrails.org/classes/ActiveRecord/Callbacks.html – lebreeze Mar 19 '11 at 09:04
  • I'm kind of confused by your controller code. What exactly is this line: `@vote = current_user.votes.find_or_create_by_video_id(@video.id)`? Do I need an if statement for it? Also, how can I change the value of the vote if it's the create method and not an update method? – Justin Meltzer Mar 19 '11 at 09:07
  • I'm assuming that current_user is set before hand of course. That line is a dynamic finder provided by ActiveRecord. It will find an existing vote or create a new one. Then the subsequent lines of code can modify value before you save the vote again. I've added a comment above. – lebreeze Mar 19 '11 at 09:11
  • ahhh, I didn't know find_or_create_ existed! I thought it was a filler call that you made haha. – Justin Meltzer Mar 19 '11 at 16:42
  • In the block of code where I change the value, how do I know whether `@vote` was found or created? I feel like I would need to know this so I can delete the old vote if a vote was found... – Justin Meltzer Mar 19 '11 at 16:48
  • I would add a unique index by user and video in the video_votes table - that would fully eliminate any chances of having a user having multiple votes for the same video. The #validates_uniqueness_of validation is nice, but there's still a race condition. – François Beausoleil Mar 19 '11 at 16:49
  • @Francois Yes my video_votes table has a unique user_id and video_id already. – Justin Meltzer Mar 19 '11 at 17:11