Merge pull request #817 from ZogStriP/prevent-duplicates-actions-on-a-post

prevent duplicate actions on a post

app/controllers/post_actions_controller.rb

@@ -4,13 +4,12 @@ class PostActionsController < ApplicationController
 
   before_filter :ensure_logged_in, except: :users
   before_filter :fetch_post_from_params
+  before_filter :fetch_post_action_type_id_from_params
 
   def create
-    id = params[:post_action_type_id].to_i
+    guardian.ensure_post_can_act!(@post, PostActionType.types[@post_action_type_id])
-    if action = PostActionType.where(id: id).first
-      guardian.ensure_post_can_act!(@post, PostActionType.types[id])
 
-      post_action = PostAction.act(current_user, @post, action.id, params[:message])
+    post_action = PostAction.act(current_user, @post, @post_action_type_id, params[:message])
 
     if post_action.blank? || post_action.errors.present?
       render_json_error(post_action)
@@ -20,23 +19,15 @@ class PostActionsController < ApplicationController
       post_serializer = PostSerializer.new(@post, scope: guardian, root: false)
       render_json_dump(post_serializer)
     end
-
-    else
-      raise Discourse::InvalidParameters.new(:post_action_type_id)
-    end
   end
 
   def users
-    requires_parameter(:post_action_type_id)
+    guardian.ensure_can_see_post_actors!(@post.topic, @post_action_type_id)
-    post_action_type_id = params[:post_action_type_id].to_i
-
-    guardian.ensure_can_see_post_actors!(@post.topic, post_action_type_id)
-
-    users = User.
-              select(['null as post_url','users.id', 'users.username', 'users.username_lower', 'users.email','post_actions.related_post_id']).
-              joins(:post_actions).
-              where(['post_actions.post_id = ? and post_actions.post_action_type_id = ? and post_actions.deleted_at IS NULL', @post.id, post_action_type_id]).all
 
+    users = User.select(['null as post_url','users.id', 'users.username', 'users.username_lower', 'users.email','post_actions.related_post_id'])
+                .joins(:post_actions)
+                .where(['post_actions.post_id = ? and post_actions.post_action_type_id = ? and post_actions.deleted_at IS NULL', @post.id, @post_action_type_id])
+                .all
 
     urls = Post.urls(users.map{|u| u.related_post_id})
     users.each do |u|
@@ -47,21 +38,21 @@ class PostActionsController < ApplicationController
   end
 
   def destroy
-    requires_parameter(:post_action_type_id)
+    post_action = current_user.post_actions.where(post_id: params[:id].to_i, post_action_type_id: @post_action_type_id, deleted_at: nil).first
 
-    post_action = current_user.post_actions.where(post_id: params[:id].to_i, post_action_type_id: params[:post_action_type_id].to_i, deleted_at: nil).first
     raise Discourse::NotFound if post_action.blank?
+
     guardian.ensure_can_delete!(post_action)
+
     PostAction.remove_act(current_user, @post, post_action.post_action_type_id)
 
     render nothing: true
   end
 
   def clear_flags
-    requires_parameter(:post_action_type_id)
     guardian.ensure_can_clear_flags!(@post)
 
-    PostAction.clear_flags!(@post, current_user.id, params[:post_action_type_id].to_i)
+    PostAction.clear_flags!(@post, current_user.id, @post_action_type_id)
     @post.reload
 
     if @post.is_flagged?
@@ -84,4 +75,9 @@ class PostActionsController < ApplicationController
       @post = finder.first
       guardian.ensure_can_see!(@post)
     end
+
+    def fetch_post_action_type_id_from_params
+      requires_parameter(:post_action_type_id)
+      @post_action_type_id = params[:post_action_type_id].to_i
+    end
 end

app/models/post_action.rb

@@ -2,7 +2,7 @@ require_dependency 'rate_limiter'
 require_dependency 'system_message'
 
 class PostAction < ActiveRecord::Base
-  class AlreadyFlagged < StandardError; end
+  class AlreadyActed < StandardError; end
 
   include RateLimiter::OnCreateRecord
 
@@ -22,7 +22,8 @@ class PostAction < ActiveRecord::Base
     posts_flagged_count = PostAction.joins(post: :topic)
                                     .where('post_actions.post_action_type_id' => PostActionType.notify_flag_types.values,
                                            'posts.deleted_at' => nil,
-                                           'topics.deleted_at' => nil).count('DISTINCT posts.id')
+                                           'topics.deleted_at' => nil)
+                                    .count('DISTINCT posts.id')
 
     $redis.set('posts_flagged_count', posts_flagged_count)
     user_ids = User.staff.select(:id).map {|u| u.id}
@@ -156,9 +157,12 @@ class PostAction < ActiveRecord::Base
   end
 
   before_create do
-    raise AlreadyFlagged if is_flag? && PostAction.where(user_id: user_id,
+    post_action_type_ids = is_flag? ? PostActionType.flag_types.values : post_action_type_id
+    raise AlreadyActed if PostAction.where(user_id: user_id,
                                            post_id: post_id,
-                                                         post_action_type_id: PostActionType.flag_types.values).exists?
+                                           post_action_type_id: post_action_type_ids,
+                                           deleted_at: nil)
+                                    .exists?
   end
 
   after_save do

spec/models/post_action_spec.rb

@@ -23,11 +23,10 @@ describe PostAction do
       mod = moderator
       action = PostAction.act(codinghorror, post, PostActionType.types[:notify_moderators], "this is my special long message");
 
-      posts = Post.
+      posts = Post.joins(:topic)
-                  joins(:topic).
+                  .select('posts.id, topics.subtype')
-                  select('posts.id, topics.subtype').
+                  .where('topics.archetype' => Archetype.private_message)
-                  where('topics.archetype' => Archetype.private_message).
+                  .to_a
-                  to_a
 
       posts.count.should == 1
       action.related_post_id.should == posts[0].id.to_i
@@ -85,6 +84,7 @@ describe PostAction do
 
   end
 
+  describe "when a user bookmarks something" do
     it "increases the post's bookmark count when saved" do
       lambda { bookmark.save; post.reload }.should change(post, :bookmark_count).by(1)
     end
@@ -93,6 +93,26 @@ describe PostAction do
       lambda { bookmark.save; post.topic.reload }.should change(post.topic, :bookmark_count).by(1)
     end
 
+    describe 'when deleted' do
+
+      before do
+        bookmark.save
+        post.reload
+        @topic = post.topic
+        @topic.reload
+        bookmark.deleted_at = DateTime.now
+        bookmark.save
+      end
+
+      it 'reduces the bookmark count of the post' do
+        lambda { post.reload }.should change(post, :bookmark_count).by(-1)
+      end
+
+      it 'reduces the bookmark count of the forum topic' do
+        lambda { @topic.reload }.should change(post.topic, :bookmark_count).by(-1)
+      end
+    end
+  end
 
   describe 'when a user likes something' do
     it 'should increase the post counts when a user likes' do
@@ -108,11 +128,10 @@ describe PostAction do
         post.topic.reload
       }.should change(post.topic, :like_count).by(1)
     end
-
   end
 
   describe 'when a user votes for something' do
-    it 'should increase the vote counts when a user likes' do
+    it 'should increase the vote counts when a user votes' do
       lambda {
         PostAction.act(codinghorror, post, PostActionType.types[:vote])
         post.reload
@@ -127,37 +146,21 @@ describe PostAction do
     end
   end
 
-
-  describe 'when deleted' do
-    before do
-      bookmark.save
-      post.reload
-      @topic = post.topic
-      @topic.reload
-      bookmark.deleted_at = DateTime.now
-      bookmark.save
-    end
-
-    it 'reduces the bookmark count of the post' do
-      lambda {
-        post.reload
-      }.should change(post, :bookmark_count).by(-1)
-    end
-
-    it 'reduces the bookmark count of the forum topic' do
-      lambda {
-        @topic.reload
-      }.should change(post.topic, :bookmark_count).by(-1)
-    end
-  end
-
   describe 'flagging' do
 
     it 'does not allow you to flag stuff with 2 reasons' do
       post = Fabricate(:post)
       u1 = Fabricate(:evil_trout)
       PostAction.act(u1, post, PostActionType.types[:spam])
-      lambda { PostAction.act(u1, post, PostActionType.types[:off_topic]) }.should raise_error(PostAction::AlreadyFlagged)
+      lambda { PostAction.act(u1, post, PostActionType.types[:off_topic]) }.should raise_error(PostAction::AlreadyActed)
+    end
+
+    it 'allows you to flag stuff with another reason' do
+      post = Fabricate(:post)
+      u1 = Fabricate(:evil_trout)
+      PostAction.act(u1, post, PostActionType.types[:spam])
+      PostAction.remove_act(u1, post, PostActionType.types[:spam])
+      lambda { PostAction.act(u1, post, PostActionType.types[:off_topic]) }.should_not raise_error(PostAction::AlreadyActed)
     end
 
     it 'should update counts when you clear flags' do
@@ -175,13 +178,10 @@ describe PostAction do
     end
 
     it 'should follow the rules for automatic hiding workflow' do
-
       post = Fabricate(:post)
       u1 = Fabricate(:evil_trout)
       u2 = Fabricate(:walter_white)
-
+      admin = Fabricate(:admin) # we need an admin for the messages
-      # we need an admin for the messages
-      admin = Fabricate(:admin)
 
       SiteSetting.flags_required_to_hide_post = 2
 
@@ -218,4 +218,18 @@ describe PostAction do
     end
   end
 
+  it "prevents user to act twice at the same time" do
+    post = Fabricate(:post)
+    user = Fabricate(:evil_trout)
+
+    # flags are already being tested
+    all_types_except_flags = PostActionType.types.except(PostActionType.flag_types)
+    all_types_except_flags.values.each do |action|
+      lambda do
+        PostAction.act(user, post, action)
+        PostAction.act(user, post, action)
+      end.should raise_error(PostAction::AlreadyActed)
+    end
+  end
+
 end

spec/models/post_action_type_spec.rb

@@ -1,5 +0,0 @@
-require 'spec_helper'
-
-describe PostActionType do
-
-end