From 392b9696f432222ab291c6ac362144894f063558 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Sat, 4 May 2013 02:52:45 +0200 Subject: [PATCH] prevent duplicate actions on a post --- app/controllers/post_actions_controller.rb | 50 +++++------ app/models/post_action.rb | 14 +-- spec/models/post_action_spec.rb | 100 ++++++++++++--------- spec/models/post_action_type_spec.rb | 5 -- 4 files changed, 89 insertions(+), 80 deletions(-) delete mode 100644 spec/models/post_action_type_spec.rb diff --git a/app/controllers/post_actions_controller.rb b/app/controllers/post_actions_controller.rb index 1312a22ae74..ecda9ffa3e9 100644 --- a/app/controllers/post_actions_controller.rb +++ b/app/controllers/post_actions_controller.rb @@ -4,39 +4,30 @@ 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 - if action = PostActionType.where(id: id).first - guardian.ensure_post_can_act!(@post, PostActionType.types[id]) + guardian.ensure_post_can_act!(@post, PostActionType.types[@post_action_type_id]) - post_action = PostAction.act(current_user, @post, action.id, params[:message]) - - if post_action.blank? || post_action.errors.present? - render_json_error(post_action) - else - # We need to reload or otherwise we are showing the old values on the front end - @post.reload - post_serializer = PostSerializer.new(@post, scope: guardian, root: false) - render_json_dump(post_serializer) - end + 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) else - raise Discourse::InvalidParameters.new(:post_action_type_id) + # We need to reload or otherwise we are showing the old values on the front end + @post.reload + post_serializer = PostSerializer.new(@post, scope: guardian, root: false) + render_json_dump(post_serializer) end end def users - requires_parameter(: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 + 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 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 diff --git a/app/models/post_action.rb b/app/models/post_action.rb index 789d33c0f59..0167ad38910 100644 --- a/app/models/post_action.rb +++ b/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_id: post_id, - post_action_type_id: PostActionType.flag_types.values).exists? + 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: post_action_type_ids, + deleted_at: nil) + .exists? end after_save do diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb index 7a341ea9378..980d75a5f72 100644 --- a/spec/models/post_action_spec.rb +++ b/spec/models/post_action_spec.rb @@ -19,15 +19,14 @@ describe PostAction do describe "messaging" do - it "notify moderators integration test" do + it "notify moderators integration test" do mod = moderator action = PostAction.act(codinghorror, post, PostActionType.types[:notify_moderators], "this is my special long message"); - posts = Post. - joins(:topic). - select('posts.id, topics.subtype'). - where('topics.archetype' => Archetype.private_message). - to_a + posts = Post.joins(:topic) + .select('posts.id, topics.subtype') + .where('topics.archetype' => Archetype.private_message) + .to_a posts.count.should == 1 action.related_post_id.should == posts[0].id.to_i @@ -85,14 +84,35 @@ describe PostAction do end - it "increases the post's bookmark count when saved" do - lambda { bookmark.save; post.reload }.should change(post, :bookmark_count).by(1) - 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 - it "increases the forum topic's bookmark count when saved" do - lambda { bookmark.save; post.topic.reload }.should change(post.topic, :bookmark_count).by(1) - end + it "increases the forum topic's bookmark count when saved" 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) - - # we need an admin for the messages - admin = Fabricate(:admin) + admin = Fabricate(:admin) # we need an admin for the messages 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 diff --git a/spec/models/post_action_type_spec.rb b/spec/models/post_action_type_spec.rb deleted file mode 100644 index 8736c1edd78..00000000000 --- a/spec/models/post_action_type_spec.rb +++ /dev/null @@ -1,5 +0,0 @@ -require 'spec_helper' - -describe PostActionType do - -end