1

User's can comment on a Screen and it's tracked by PublicActivity :

@comment.create_activity :create, owner: current_user, recipient: @comment.screen.user

and the comments are dependent: :destroy on the screen model.

But when i delete a screen, while the comments are deleted, the Record from PublicActivity for that comment still exists.

here's my Screens Controller:

  def destroy
    @activity = PublicActivity::Activity.find_by_trackable_id(params[:id])
    @activity.destroy #<-- Heres the Problem
    @screen.destroy
    respond_to do |format|
      format.html { redirect_to root_path }
      format.json { head :no_content }
    end
  end

But upon deleting a Screen, i'm getting undefined methoddestroy' for nil:NilClass`.

I read on Railscast:

it was due to calling the create_activity method after the object had been destroyed.

According to the gem maintainers, you simply have to assume the record will be destroyed, and call create_activity before the destroy

What am i missing?

Informations below

screen.rb

belongs_to :user
has_many :comments, :dependent =>  :destroy

comment.rb

belongs_to :user
belongs_to :screen

screens_contoller.rb

  def create
    @screen = current_user.screens.build(screen_params)
    respond_to do |format|
      if @screen.save
         format.html { redirect_to @screen, notice: 'You successfully uploaded your Screenshot.' }
        format.json { render action: 'show', status: :created, location: @screen }
        current_user.add_points(2, 'Points for Uploading a Screenshot')
      else
        format.html { render action: 'new' }
        format.json { render json: @screen.errors, status: :unprocessable_entity }
      end
    end
  end

  def destroy
    @activity = PublicActivity::Activity.find_by_trackable_id(params[:id])
    @activity.destroy
    @screen.destroy
    respond_to do |format|
      format.html { redirect_to root_path }
      format.json { head :no_content }
      current_user.substract_points(1, "Substraction for Deleting a Screenshot")
    end
  end

comments_controller.rb

  def create
    @screen = Screen.find(params[:screen_id])
    @comment = current_user.comments.build(comment_params)
    @comment.screen_id = @screen.id
    respond_to do |format|
      if @comment.save
        # Create Record for Public Activity
        @comment.create_activity :create, owner: current_user, recipient: @comment.screen.user
        format.html { redirect_to @screen, notice: 'Comment was successfully created.' }
        format.json { render action: 'show', status: :created, location: @comment }
      else
        format.html { render action: 'new' }
        format.json { render json: @comment.errors, status: :unprocessable_entity }
      end
    end
  end

  def destroy
    @comment.destroy
    respond_to do |format|
      @activity = PublicActivity::Activity.find_by_trackable_id(params[:id])
      @activity.destroy
      format.html { redirect_to :back }
      format.json { head :no_content }
    end
  end

That's how my Screen Controller Destroy Action looks like right now:

  def destroy
    @screen = current_user.screens.find(params[:id])
    @activity = PublicActivity::Activity.find_by_trackable_id(params[:id])
    @activity.destroy
    @screen.destroy
    current_user.substract_points(1, "Substraction for Deleting a Screenshot")
    respond_to do |format|
      format.html { redirect_to root_path }
    end
  end

Again the same error:

enter image description here

Mini John
  • 7,855
  • 9
  • 59
  • 108
  • where you are fetching `@screen`. his is a nil object and you are trying to destroy that that's the problem – Nitin Jain Apr 13 '14 at 17:46
  • `@screen = Screen.find(params[:id])` ? unless you have it in the set_screen at the bottom. But clearly you're not creating variable `@screen` anywhere – Mohamed El Mahallawy Apr 13 '14 at 17:47
  • @MohamedElMahallawy It's in the Set_screen. Editing my Question – Mini John Apr 13 '14 at 17:48
  • @Pavan what do you mean ? – Mini John Apr 13 '14 at 17:50
  • So you have an `Activity` or `PublicActivity` class? And why is `@activity = PublicActivity::Activity.find_by_trackable_id(params[:id]) @activity.destroy #<-- Heres the Problem` in the respond_to block? Take it out of there – Mohamed El Mahallawy Apr 13 '14 at 17:51
  • activities Controller.. Yeah i took it out of there later, but still the same. – Mini John Apr 13 '14 at 17:52
  • please try `@activity.destroy if @activity.present?` – Bachan Smruty Apr 13 '14 at 18:00
  • 1
    @BachanSmruty the problem is that this is leaving the activity still in the DB!! – Mini John Apr 13 '14 at 18:01
  • yes.. But @activity is nil. That means activity is not being found by the trackable_id. – Bachan Smruty Apr 13 '14 at 18:05
  • Thats not what i want, i want to destroy the Record. Hence @activity.destroy ! – Mini John Apr 13 '14 at 18:06
  • I updated my Question. Maybe i didn't described it very well – Mini John Apr 13 '14 at 18:14
  • @TheMiniJohn you should show us the model classes and the relationship between them. I can't get your question. It's very hard to help you. – Blue Smith Apr 13 '14 at 18:33
  • Thank you. I have one question: why don't you just simply add an association `has_one :activity, as: :trackable, class_name: PublicActivity::Activity.name, dependent: :destroy` to `Screen` and `Comment` models so that it will do the job `activity.destroy` for you? – Blue Smith Apr 13 '14 at 19:08
  • Does that work with the Public Activity Gem ? ill try it – Mini John Apr 13 '14 at 19:20
  • @TheMiniJohn In your destroy action in the Screen Controller I think you need to call `@screen = current_user.screens.find(params[:id})` first before you call `@screens.destroy` or it doesnt know what record to destroy. – itsadevnotcat Apr 14 '14 at 02:40
  • typo* `@screen = current_user.screens.find(params[:id])` – itsadevnotcat Apr 14 '14 at 03:34
  • @TheMiniJohn what is the error? It should work because `PublicActivity::Activity` is a ActiveRecord class and you can association it with other models. – Blue Smith Apr 14 '14 at 03:48
  • I'm missing something really big :) undefined method `destroy' for nil:NilClass, Ill update my Question with how the destroy action looks like right now. – Mini John Apr 14 '14 at 10:56
  • Just to clarify: The COMMENT created on a screen has an activity, if the screen is deleted the COMMENT ACTIVITY should as well. – Mini John Apr 14 '14 at 11:06
  • Right now you are trying to destroy an Activity with a trackable_id = to your screen.id (thats never going to work). You should be destroying the activity with trackable_id = comment.id but it needs to be done before the comment is destroyed by the dependent destroyed call, and that is not handled by the destroy method in the controller. You need to create a before_destroy callback in your comments model that finds the activity and destroys it when a screen is deleted. – itsadevnotcat Apr 14 '14 at 18:28
  • Also, I don't think your `@activity.destroy` method for comments should be in your respond_to block. It should look like the destroy action you have for screens. – itsadevnotcat Apr 14 '14 at 18:30
  • That sounds logical, could you write an Answer (including the before_destroy method) on this ? – Mini John Apr 14 '14 at 19:39
  • 1
    sure. I'm right in the middle of something, so it will be a few hours before I can get to it. – itsadevnotcat Apr 14 '14 at 21:31

1 Answers1

5

This isn't tested, but this is what I think you should do.

First you can remove the references to activities in your screens_controller#destroy

Then in your comments_controller#destroy

  @comment = current_user.comments.find(params[:id])
  @activity = PublicActivity::Activity.find_by(trackable_id: (params[:id]), trackable_type: controller_path.classify)
  @activity.destroy
  @comment.destroy

Should be outside of your respond to block

Next in your comments model you should do something like this:

#comment.rb

private

before_destroy :find_and_destroy_comments

def find_and_destroy_comments
  activity = PublicActivity::Activity.find_by(trackable_id: self.id, trackable_type: self.class.name)
  if activity.present?
    activity.destroy
  end
end

calling the before_destroy method overrides the default ruby destroy method that is called during dependent: :destroy

Let me know if this works, but It should.

itsadevnotcat
  • 245
  • 3
  • 9
  • "calling the before_destroy method overrides the default ruby destroy method." Worked Epicly ! – Mini John Apr 14 '14 at 23:12
  • 1
    @TheMiniJohn see my edits in `comment.rb`. To be safe you'll want to add the `if activity.present?` to make sure it finds the activity, otherwise it will throw an error. – itsadevnotcat Apr 15 '14 at 00:12
  • 1
    Be careful with the above, as it will have the unintended consequences of deleting activities that shouldn't be deleted. Since it's polymorphic, there can be multiple activities with the same `trackable_id`. Instead, I would find the activity with `activity = PublicActivity::Activity.find_by(trackable_id: self.id, trackable_type: self.class.name)` – Justin Feb 07 '15 at 18:21
  • @itsthewrongway. good deal..glad to help...you might always want to change it [for your other answer](http://stackoverflow.com/a/18429493/2456549). Cheers – Justin Feb 12 '15 at 01:44