From 0df666277de2d11314b2494158b29389626eb74b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Wed, 4 Jun 2014 17:41:11 +0200 Subject: [PATCH] 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 --- .../javascripts/discourse/models/post.js | 25 +++++++++---------- .../routes/user_activity_stream_route.js | 2 +- app/controllers/posts_controller.rb | 8 ------ app/models/post_action.rb | 9 ++++--- app/models/user_action.rb | 13 +++------- app/models/user_action_observer.rb | 6 +++-- app/serializers/user_action_serializer.rb | 1 + config/routes.rb | 1 - lib/post_destroyer.rb | 14 +++++++++++ lib/topic_view.rb | 2 +- spec/components/post_destroyer_spec.rb | 25 +++++++++++++++++++ spec/controllers/posts_controller_spec.rb | 10 +------- 12 files changed, 69 insertions(+), 47 deletions(-) diff --git a/app/assets/javascripts/discourse/models/post.js b/app/assets/javascripts/discourse/models/post.js index fa7b5c4392f..859099a0513 100644 --- a/app/assets/javascripts/discourse/models/post.js +++ b/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", { - type: 'PUT', - data: { - bookmarked: this.get('bookmarked') ? true : false - } - }).then(null, function (error) { - if (error && error.responseText) { - bootbox.alert($.parseJSON(error.responseText).errors[0]); - } else { - bootbox.alert(I18n.t('generic_error')); - } - }); - + Discourse.Post.bookmark(this.get('id'), this.get('bookmarked')) + .then(null, function (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 } }); } }); diff --git a/app/assets/javascripts/discourse/routes/user_activity_stream_route.js b/app/assets/javascripts/discourse/routes/user_activity_stream_route.js index 2dcc5f7e4f1..069d465e797 100644 --- a/app/assets/javascripts/discourse/routes/user_activity_stream_route.js +++ b/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); diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index cc9b279f9bc..a88d8d151ec 100644 --- a/app/controllers/posts_controller.rb +++ b/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) diff --git a/app/models/post_action.rb b/app/models/post_action.rb index 6e20b953534..7aab8f6ea9e 100644 --- a/app/models/post_action.rb +++ b/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 diff --git a/app/models/user_action.rb b/app/models/user_action.rb index ca6c5c2a6ca..e79f062997e 100644 --- a/app/models/user_action.rb +++ b/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| - required_parameters.include?(k) -end) + action = self.find_by(hash.select { |k, v| required_parameters.include?(k) }) return action if action action = self.new(hash) @@ -181,10 +179,7 @@ end) end if action.user - MessageBus.publish("/users/#{action.user.username.downcase}", - action.id, - user_ids: [user_id], - group_ids: group_ids ) + MessageBus.publish("/users/#{action.user.username.downcase}", action.id, user_ids: [user_id], group_ids: group_ids) end action diff --git a/app/models/user_action_observer.rb b/app/models/user_action_observer.rb index 84afb7049a6..956aaefa392 100644 --- a/app/models/user_action_observer.rb +++ b/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 diff --git a/app/serializers/user_action_serializer.rb b/app/serializers/user_action_serializer.rb index 732d1837e07..188ed1b69d6 100644 --- a/app/serializers/user_action_serializer.rb +++ b/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, diff --git a/config/routes.rb b/config/routes.rb index 5972ff254e3..3d0d8a00ed1 100644 --- a/config/routes.rb +++ b/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 diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb index 4245f6b9cc1..3f94b146e25 100644 --- a/lib/post_destroyer.rb +++ b/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) diff --git a/lib/topic_view.rb b/lib/topic_view.rb index 7c3b435749f..65d01bddb88 100644 --- a/lib/topic_view.rb +++ b/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 diff --git a/spec/components/post_destroyer_spec.rb b/spec/components/post_destroyer_spec.rb index 2acd2c6753d..ab68f35a459 100644 --- a/spec/components/post_destroyer_spec.rb +++ b/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 } diff --git a/spec/controllers/posts_controller_spec.rb b/spec/controllers/posts_controller_spec.rb index 4f40932b7fc..914c9b69d9e 100644 --- a/spec/controllers/posts_controller_spec.rb +++ b/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