Search code examples
ruby-on-railsrubyruby-on-rails-3activerecordvoting

Why doesn't this Ruby on Rails code work as I intend it to?


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

Solution

  • @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