BUGFIXES: properly deal with bookmarks and deleted posts

BUGFIX: removing a bookmark from the activity feed was busted for deleted posts
BUGFIX: delete associated user actions when deleting a post

app/assets/javascripts/discourse/models/post.js

@@ -65,19 +65,14 @@ Discourse.Post = Discourse.Model.extend({
   postElementId: Discourse.computed.fmt('post_number', 'post_%@'),
 
   bookmarkedChanged: function() {
-    Discourse.ajax("/posts/" + this.get('id') + "/bookmark", {
+    Discourse.Post.bookmark(this.get('id'), this.get('bookmarked'))
-      type: 'PUT',
+             .then(null, function (error) {
-      data: {
+               if (error && error.responseText) {
-        bookmarked: this.get('bookmarked') ? true : false
+                 bootbox.alert($.parseJSON(error.responseText).errors[0]);
-      }
+               } else {
-    }).then(null, function (error) {
+                 bootbox.alert(I18n.t('generic_error'));
-      if (error && error.responseText) {
+               }
-        bootbox.alert($.parseJSON(error.responseText).errors[0]);
+             });
-      } else {
-        bootbox.alert(I18n.t('generic_error'));
-      }
-    });
-
   }.observes('bookmarked'),
 
   wikiChanged: function() {
@@ -456,6 +451,10 @@ Discourse.Post.reopenClass({
     return Discourse.ajax("/posts/" + postId + ".json").then(function (result) {
       return Discourse.Post.create(result);
     });
+  },
+
+  bookmark: function(postId, bookmarked) {
+    return Discourse.ajax("/posts/" + postId + "/bookmark", { type: 'PUT', data: { bookmarked: bookmarked } });
   }
 
 });

app/assets/javascripts/discourse/routes/user_activity_stream_route.js

@@ -30,7 +30,7 @@ Discourse.UserActivityStreamRoute = Discourse.Route.extend({
 
     removeBookmark: function(userAction) {
       var self = this;
-      Discourse.ajax("/posts/by_number/" + userAction.topic_id + "/" + userAction.post_number + "/bookmarks/remove", { type: "PUT" })
+      Discourse.Post.bookmark(userAction.get('post_id'), false)
                .then(function() {
                   // remove the user action from the stream
                   self.modelFor("user").get("stream").remove(userAction);

app/controllers/posts_controller.rb

@@ -122,14 +122,6 @@ class PostsController < ApplicationController
     display_post(post)
   end
 
-  def remove_bookmark_by_number
-    if current_user
-      post = find_post_from_params_by_number
-      PostAction.remove_act(current_user, post, PostActionType.types[:bookmark])
-    end
-    render nothing: true
-  end
-
   def reply_history
     post = find_post_from_params
     render_serialized(post.reply_history, PostSerializer)

app/models/post_action.rb

@@ -42,10 +42,10 @@ class PostAction < ActiveRecord::Base
   	return {} if collection.blank?
 
     collection_ids = collection.map {|p| p.id}
-
     user_id = user.present? ? user.id : 0
 
     result = PostAction.where(post_id: collection_ids, user_id: user_id)
+
     user_actions = {}
     result.each do |r|
       user_actions[r.post_id] ||= {}
@@ -146,7 +146,9 @@ class PostAction < ActiveRecord::Base
   end
 
   def self.remove_act(user, post, post_action_type_id)
-    if action = find_by(post_id: post.id, user_id: user.id, post_action_type_id: post_action_type_id)
+    finder = PostAction.where(post_id: post.id, user_id: user.id, post_action_type_id: post_action_type_id)
+    finder = finder.with_deleted if user.try(:staff?)
+    if action = finder.first
       action.remove_act!(user)
     end
   end
@@ -249,9 +251,9 @@ class PostAction < ActiveRecord::Base
       Post.where(id: post_id).update_all ["#{column} = #{column} + ?", delta]
     end
 
+    post = Post.with_deleted.where(id: post_id).first
     Topic.where(id: post.topic_id).update_all ["#{column} = #{column} + ?", delta]
 
-
     if PostActionType.notify_flag_type_ids.include?(post_action_type_id)
       PostAction.update_flagged_posts_count
     end
@@ -259,6 +261,7 @@ class PostAction < ActiveRecord::Base
   end
 
   def enforce_rules
+    post = Post.with_deleted.where(id: post_id).first
     PostAction.auto_hide_if_needed(post, post_action_type_key)
     SpamRulesEnforcer.enforce!(post.user) if post_action_type_key == :spam
   end

app/models/user_action.rb

@@ -106,7 +106,7 @@ SELECT
   a.id,
   t.title, a.action_type, a.created_at, t.id topic_id,
   a.user_id AS target_user_id, au.name AS target_name, au.username AS target_username,
-  coalesce(p.post_number, 1) post_number,
+  coalesce(p.post_number, 1) post_number, p.id as post_id,
   p.reply_to_post_number,
   pu.email, pu.username, pu.name, pu.id user_id,
   pu.uploaded_avatar_id,
@@ -151,15 +151,13 @@ LEFT JOIN categories c on c.id = t.category_id
   def self.log_action!(hash)
     required_parameters = [:action_type, :user_id, :acting_user_id, :target_topic_id, :target_post_id]
     require_parameters(hash, *required_parameters)
+
     transaction(requires_new: true) do
       begin
-
         # TODO there are conditions when this is called and user_id was already rolled back and is invalid.
 
         # protect against dupes, for some reason this is failing in some cases
-        action = self.find_by(hash.select do |k, v|
+        action = self.find_by(hash.select { |k, v| required_parameters.include?(k) })
-  required_parameters.include?(k)
-end)
         return action if action
 
         action = self.new(hash)
@@ -181,10 +179,7 @@ end)
         end
 
         if action.user
-          MessageBus.publish("/users/#{action.user.username.downcase}",
+          MessageBus.publish("/users/#{action.user.username.downcase}", action.id, user_ids: [user_id], group_ids: group_ids)
-                                action.id,
-                                user_ids: [user_id],
-                                group_ids: group_ids )
         end
 
         action

app/models/user_action_observer.rb

@@ -132,12 +132,14 @@ class UserActionObserver < ActiveRecord::Observer
     action = UserAction::BOOKMARK if model.is_bookmark?
     action = UserAction::LIKE if model.is_like?
 
+    post = Post.with_deleted.where(id: model.post_id).first
+
     row = {
       action_type: action,
       user_id: model.user_id,
       acting_user_id: model.user_id,
       target_post_id: model.post_id,
-      target_topic_id: model.post.topic_id,
+      target_topic_id: post.topic_id,
       created_at: model.created_at
     }
 
@@ -149,7 +151,7 @@ class UserActionObserver < ActiveRecord::Observer
 
     if model.is_like?
       row[:action_type] = UserAction::WAS_LIKED
-      row[:user_id] = model.post.user_id
+      row[:user_id] = post.user_id
       if model.deleted_at.nil?
         UserAction.log_action!(row)
       else

app/serializers/user_action_serializer.rb

@@ -11,6 +11,7 @@ class UserActionSerializer < ApplicationSerializer
              :target_name,
              :target_username,
              :post_number,
+             :post_id,
              :reply_to_post_number,
              :username,
              :name,

config/routes.rb

@@ -222,7 +222,6 @@ Discourse::Application.routes.draw do
   post "uploads" => "uploads#create"
 
   get "posts/by_number/:topic_id/:post_number" => "posts#by_number"
-  put "posts/by_number/:topic_id/:post_number/bookmarks/remove" => "posts#remove_bookmark_by_number"
   get "posts/:id/reply-history" => "posts#reply_history"
 
   resources :groups do

lib/post_destroyer.rb

@@ -63,6 +63,7 @@ class PostDestroyer
         Topic.reset_highest(@post.topic_id)
       end
       trash_post_actions
+      trash_user_actions
       @post.update_flagged_posts_count
       remove_associated_replies
       remove_associated_notifications
@@ -138,6 +139,19 @@ class PostDestroyer
     Post.with_deleted.where(id: @post.id).update_all(Hash[*f.flatten])
   end
 
+  def trash_user_actions
+    UserAction.where(target_post_id: @post.id).each do |ua|
+      row = {
+        action_type: ua.action_type,
+        user_id: ua.user_id,
+        acting_user_id: ua.acting_user_id,
+        target_topic_id: ua.target_topic_id,
+        target_post_id: ua.target_post_id
+      }
+      UserAction.remove_action!(row)
+    end
+  end
+
   def remove_associated_replies
     post_ids = PostReply.where(reply_id: @post.id).pluck(:post_id)
 

lib/topic_view.rb

@@ -207,7 +207,7 @@ class TopicView
   end
 
   def all_post_actions
-    @all_post_actions ||= PostAction.counts_for(posts, @user)
+    @all_post_actions ||= PostAction.counts_for(@posts, @user)
   end
 
   def links

spec/components/post_destroyer_spec.rb

@@ -288,6 +288,31 @@ describe PostDestroyer do
     end
   end
 
+  describe "user actions" do
+    let(:codinghorror) { Fabricate(:coding_horror) }
+    let(:second_post) { Fabricate(:post, topic_id: post.topic_id) }
+
+    def create_user_action(action_type)
+      UserAction.log_action!({
+        action_type: action_type,
+        user_id: codinghorror.id,
+        acting_user_id: codinghorror.id,
+        target_topic_id: second_post.topic_id,
+        target_post_id: second_post.id
+      })
+    end
+
+    it "should delete the user actions" do
+      bookmark = create_user_action(UserAction::BOOKMARK)
+      like = create_user_action(UserAction::LIKE)
+
+      PostDestroyer.new(moderator, second_post).destroy
+
+      expect(UserAction.find_by(id: bookmark.id)).to be_nil
+      expect(UserAction.find_by(id: like.id)).to be_nil
+    end
+  end
+
   describe 'topic links' do
     let!(:first_post)  { Fabricate(:post) }
     let!(:topic)       { first_post.topic }

spec/controllers/posts_controller_spec.rb

@@ -288,13 +288,10 @@ describe PostsController do
       let(:post) { Fabricate(:post, user: log_in) }
 
       it "raises an error if the user doesn't have permission to see the post" do
-        Guardian.any_instance.expects(:can_see?).with(post).returns(false).twice
+        Guardian.any_instance.expects(:can_see?).with(post).returns(false).once
 
         xhr :put, :bookmark, post_id: post.id, bookmarked: 'true'
         response.should be_forbidden
-
-        xhr :put, :remove_bookmark_by_number, topic_id: post.topic_id, post_number: post.post_number
-        response.should be_forbidden
       end
 
       it 'creates a bookmark' do
@@ -307,11 +304,6 @@ describe PostsController do
         xhr :put, :bookmark, post_id: post.id
       end
 
-      it 'removes a bookmark using the topic_id and the post_number' do
-        PostAction.expects(:remove_act).with(post.user, post, PostActionType.types[:bookmark])
-        xhr :put, :remove_bookmark_by_number, topic_id: post.topic_id, post_number: post.post_number
-      end
-
     end
 
   end