Search code examples
ruby-on-railsherokucontrollerdestroy

Rails destroy action destroying the wrong record


I posted a similar question earlier and I thought that I had fixed this problem but I was wrong. I thought that the activity wasn't being deleted but turns out that it was deleting activity it was just the wrong one. I don't have this problem with any of my other models and this only happens on one of my apps. My forked app which has the exact same code works correctly. I don't know why this is happening.

projects_controller.rb

class ProjectsController < ApplicationController

    before_filter :find_project, only: [:edit, :update, :destroy]

    def create
        params[:project][:about] = sanitize_redactor(params[:project][:about])
        @project = current_member.projects.new(params[:project])

        respond_to do |format|
          if @project.save
            current_member.create_activity(@project, 'created')
            format.html { redirect_to @project }
            format.json { render json: @project, status: :created, location: @project }
          else
            format.html { render action: "new" }
            format.json { render json: @project.errors, alert: 'Please make sure all required fields are filled in and all fields are formatted correctly.' }
          end
       end
    end

    def destroy
        @project = Project.find(params[:id])
        @activity = Activity.find_by_targetable_id(params[:id])
        if @activity
            @activity.destroy
        end
        @project.destroy

        respond_to do |format|
          format.html { redirect_to profile_projects_path(current_member) }
          format.json { head :no_content }
          format.js
        end
    end

    def find_project
        @project = current_member.projects.find(params[:id])
    end 

end

member.rb

class Member < ActiveRecord::Base

    def create_activity(item, action)
        activity = activities.new
        activity.targetable = item
        activity.action = action 
        activity.save 
        activity
    end

end

Migration

class CreateActivities < ActiveRecord::Migration
    def change
        create_table :activities do |t|
            t.integer :member_id
            t.string :action

            t.integer :targetable_id
            t.string :targetable_type

            t.timestamps
        end

        add_index :activities, :member_id
        add_index :activities, [:targetable_id, :targetable_type]

    end
end

****EDIT****

activity.rb

class Activity < ActiveRecord::Base
    belongs_to :member
    belongs_to :targetable, polymorphic: true
    acts_as_votable

    self.per_page = 36

    def self.for_member(member, options={})
        options[:page] ||= 1
        following_ids = member.following_members.map(&:id).push(member.id)
        where("member_id in (?)", following_ids).
            order("created_at desc").
            page(options[:page])
    end 

end

project.rb

class Project < ActiveRecord::Base
    belongs_to :member

    attr_accessible :about, :blurb, :category, :markers, :video, :website, :name, :avatar, :banner, :marker_list, :city

    acts_as_votable
    acts_as_followable
    acts_as_ordered_taggable
    acts_as_ordered_taggable_on :markers
    acts_as_messageable
    has_many :comments, as: :commentable, :dependent => :destroy
    has_many :uploads, as: :uploadable, :dependent => :destroy
    has_many :updates, as: :updateable, :dependent => :destroy

    def to_param
        "#{id}-#{name.parameterize}"
    end  

end

member.rb

class Member < ActiveRecord::Base
    # Include default devise modules. Others available are:
    # :confirmable, :lockable, :timeoutable and :omniauthable
    devise :database_authenticatable, :registerable,
    :recoverable, :rememberable, :trackable, :validatable

    # Setup accessible (or protected) attributes for your model
    attr_accessible :email, :email_confirmation, :password, :password_confirmation, :remember_me,
        :full_name, :user_name, :pursuits, :avatar, :bio, :city, :state, :country, :pursuit_list, 
        :facebook, :twitter, :linkedin, :soundcloud, :youtube, :vimeo, :instagram, :flickr, :google, :pinterest, :blog, :website, :banner

    has_many :medium, :dependent => :destroy
    has_many :projects, :dependent => :destroy
    has_many :events, :dependent => :destroy
    has_many :statuses, :dependent => :destroy
    has_many :activities, :dependent => :destroy
    has_many :listings, :dependent => :destroy
    has_many :comments, :dependent => :destroy
    has_many :uploads, :dependent => :destroy
    has_many :updates, :dependent => :destroy
    has_many :assets, :dependent => :destroy
    acts_as_follower
    acts_as_followable
    acts_as_ordered_taggable
    acts_as_ordered_taggable_on :pursuits
    acts_as_voter
    acts_as_messageable

    def to_param
        user_name
    end 

    def name
        user_name
    end 

    def mailboxer_email(object)
        return user_name
    end

    def create_activity(item, action)
        activity = activities.new
        activity.targetable = item
        activity.action = action 
        activity.save 
        activity
    end

end

Solution

  • Interesting...

    --

    Activity Model

    You mention it's the Activity model which doesn't destroy correctly. With this in mind, you'll want to look at all the steps contributing to the destroy mechanism:

    def destroy
       ...
       @activity = Activity.find_by_targetable_id(params[:id])
       @activity.destroy if @activity
    

    First things first - what is targetable_id? Also, if you're using Rails 4, you'll be able to use the find_by method with a hash of attributes:

    @activity = Activity.find_by targetable_id: params[:id]
    

    This could be the main cause of the issue - the above code will basically look for any Activity records with the targetable_id attribute having the same id parameter as you passed from your request

    I'm not a betting man, but I'd surmise this is where the issue lies


    Polymorphic

    Okay, I found the problem.

    You're trying to call a polymorphic association without referencing it correctly. Polymorphic associations allow you to create an ActiveRecord association for multiple models, storing the associative data in a single table / model:

    enter image description here

    Notice how the above example (as your code) includes ______type column? This is VITAL to your solution - as it what stores the model you saved the ID for:

    When you save data in ActiveRecord associations, it uses something called a foreign_key to define the associative data in your model. This foreign key is normally something like activity_id or similar, as you know already.

    The difference is that your association is a polymorphic association, meaning you can store multiple model types in a single table. This means that ActiveRecord / Rails will have to refer to the model you're saving, which is where the _type column comes in

    My theory is that your local database will have several models saved in your "polymorphic" datatable - meaning when you look for an ID, it's bringing back a record which isn't the correct model


    Fix

    I would recommend the following:

    #app/models/project.rb
    Class Project < ActiveRecord::Base
       has_many :activities, as: :targetable, dependent: :destroy
    end
    
    #app/models/activity.rb
    Class Activity < ActiveRecord::Base
       belongs_to :targetable, polymorphic: true
    end
    

    I don't know your model associations, so I just made the above up. If you set up your associations as above, you'll be able to call the following:

    #app/controllers/projects_controller.rb
    Class ProjectController < ApplicationController
       def destroy
          @project = Project.find params[:id] # should find project.activities too
          @project.destroy
       end
    end
    

    I used the above for several important reasons:

    1. You're using the same ID for both objects (means they're associated)
    2. You can use the dependent: :destroy method

    The bottom line is you will be able to destroy just the Project object, and have its dependants destroyed too, achieving your desired outcome