Merge pull request #2958 from evazion/fix-2955

Fix #2955: Reapproval error for unapproved post

app/controllers/moderator/post/approvals_controller.rb

@@ -3,14 +3,13 @@ module Moderator
     class ApprovalsController < ApplicationController
       before_filter :approver_only
       skip_before_filter :api_check
+      respond_to :json, :xml, :js
 
       def create
         cookies.permanent[:moderated] = Time.now.to_i
-        @post = ::Post.find(params[:post_id])
+        post = ::Post.find(params[:post_id])
-        if @post.is_deleted? || @post.is_flagged? || @post.is_pending?
+        @approval = post.approve!
-          @post.approve!
+        respond_with(@approval)
-        end
-      rescue ::Post::ApprovalError
       end
     end
   end

app/models/post.rb

@@ -46,6 +46,7 @@ class Post < ActiveRecord::Base
   has_many :notes, :dependent => :destroy
   has_many :comments, lambda {includes(:creator, :updater).order("comments.id")}, :dependent => :destroy
   has_many :children, lambda {order("posts.id")}, :class_name => "Post", :foreign_key => "parent_id"
+  has_many :approvals, :class_name => "PostApproval", :dependent => :destroy
   has_many :disapprovals, :class_name => "PostDisapproval", :dependent => :destroy
   has_many :favorites, :dependent => :destroy
 
@@ -290,8 +291,8 @@ class Post < ActiveRecord::Base
   end
 
   module ApprovalMethods
-    def is_approvable?
+    def is_approvable?(user = CurrentUser.user)
-      !is_status_locked? && (is_pending? || is_flagged? || is_deleted?) && !PostApproval.approved?(CurrentUser.id, id)
+      !is_status_locked? && (is_pending? || is_flagged? || is_deleted?) && !approved_by?(user)
     end
 
     def flag!(reason, options = {})
@@ -320,35 +321,12 @@ class Post < ActiveRecord::Base
       end
     end
 
-    def approve!
+    def approve!(approver = CurrentUser.user)
-      if is_status_locked?
+      approvals.create(user: approver)
-        errors.add(:is_status_locked, "; post cannot be approved")
+    end
-        raise ApprovalError.new("Post is locked and cannot be approved")
-      end
 
-      if uploader_id == CurrentUser.id
+    def approved_by?(user)
-        errors.add(:base, "You cannot approve a post you uploaded")
+      approver == user || approvals.where(user: user).exists?
-        raise ApprovalError.new("You cannot approve a post you uploaded")
-      end
-
-      if approver_id == CurrentUser.id || PostApproval.approved?(CurrentUser.id, id)
-        errors.add(:approver, "have already approved this post")
-        raise ApprovalError.new("You have previously approved this post and cannot approve it again")
-      end
-
-      flags.each {|x| x.resolve!}
-      self.is_flagged = false
-      self.is_pending = false
-      self.is_deleted = false
-      self.approver_id = CurrentUser.id
-
-      PostApproval.create(user_id: CurrentUser.id, post_id: id)
-
-      if is_deleted_was == true
-        ModAction.log("undeleted post ##{id}")
-      end
-
-      save!
     end
 
     def disapproved_by?(user)
@@ -1396,7 +1374,7 @@ class Post < ActiveRecord::Base
       end
 
       if !CurrentUser.is_admin? 
-        if approver_id == CurrentUser.id || PostApproval.approved?(CurrentUser.id, id)
+        if approved_by?(CurrentUser.user)
           raise ApprovalError.new("You have previously approved this post and cannot undelete it")
         elsif uploader_id == CurrentUser.id
           raise ApprovalError.new("You cannot undelete a post you uploaded")

app/models/post_approval.rb

@@ -1,12 +1,36 @@
 class PostApproval < ActiveRecord::Base
   belongs_to :user
-  belongs_to :post
+  belongs_to :post, inverse_of: :approvals
+
+  validate :validate_approval
+  after_create :approve_post
 
   def self.prune!
     where("created_at < ?", 1.month.ago).delete_all
   end
 
-  def self.approved?(user_id, post_id)
+  def validate_approval
-    where(user_id: user_id, post_id: post_id).exists?
+    if post.is_status_locked?
+      errors.add(:post, "is locked and cannot be approved")
+    end
+
+    if post.status == "active"
+      errors.add(:post, "is already active and cannot be approved")
+    end
+
+    if post.uploader == user
+      errors.add(:base, "You cannot approve a post you uploaded")
+    end
+
+    if post.approved_by?(user)
+      errors.add(:base, "You have previously approved this post and cannot approve it again")
+    end
+  end
+
+  def approve_post
+    ModAction.log("undeleted post ##{id}") if post.is_deleted
+
+    post.flags.each(&:resolve!)
+    post.update({ approver: user, is_flagged: false, is_pending: false, is_deleted: false }, without_protection: true)
   end
 end

app/models/user.rb

@@ -77,6 +77,7 @@ class User < ActiveRecord::Base
   #after_create :notify_sock_puppets
   has_many :feedback, :class_name => "UserFeedback", :dependent => :destroy
   has_many :posts, :foreign_key => "uploader_id"
+  has_many :post_approvals, :dependent => :destroy
   has_many :post_votes
   has_many :bans, lambda {order("bans.id desc")}
   has_one :recent_ban, lambda {order("bans.id desc")}, :class_name => "Ban"

app/views/moderator/post/approvals/create.js.erb

@@ -1,6 +1,6 @@
-<% if @post.errors.any? %>
+<% if @approval.errors.any? %>
 
-Danbooru.error("Error: " + <%= @post.errors.full_messages.join("; ").to_json.html_safe %>);
+Danbooru.error("Error: " + <%= @approval.errors.full_messages.join("; ").to_json.html_safe %>);
 
 <% else %>
 
@@ -10,7 +10,7 @@ $("#c-posts #flag").show();
 
 $("#pending-approval-notice").hide();
 
-$("#c-moderator-post-queues #post-<%= @post.id %>").hide();
+$("#c-moderator-post-queues #post-<%= @approval.post.id %>").hide();
 Danbooru.notice("Post was approved");
 
 Danbooru.ModQueue.increment_processed();

test/models/post_approval_test.rb

@@ -1,63 +0,0 @@
-require 'test_helper'
-
-class PostApprovalTest < ActiveSupport::TestCase
-	context "a pending post" do
-		setup do
-			@user = FactoryGirl.create(:user)
-			CurrentUser.user = @user
-			CurrentUser.ip_addr = "127.0.0.1"
-
-			@post = FactoryGirl.create(:post, uploader_id: @user.id, is_pending: true)
-
-			@approver = FactoryGirl.create(:user)
-			@approver.can_approve_posts = true
-			@approver.save
-			CurrentUser.user = @approver
-
-			CurrentUser.stubs(:can_approve_posts?).returns(true)
-		end
-
-		teardown do
-			CurrentUser.user = nil
-			CurrentUser.ip_addr = nil
-		end
-
-		should "allow approval" do
-			assert_equal(false, PostApproval.approved?(@approver.id, @post.id))
-		end
-
-		context "That is approved" do
-			should "create a postapproval record" do
-				assert_difference("PostApproval.count") do
-					@post.approve!
-				end
-			end
-
-			context "that is then flagged" do
-				setup do
-					@user2 = FactoryGirl.create(:user)
-					@user3 = FactoryGirl.create(:user)
-					@approver2 = FactoryGirl.create(:user)
-					@approver2.can_approve_posts = true
-					@approver2.save
-				end
-
-				should "prevent the first approver from approving again" do
-					@post.approve!
-					CurrentUser.user = @user2
-					@post.flag!("blah")
-					CurrentUser.user = @approver2
-					@post.approve!
-					assert_not_equal(@approver.id, @post.approver_id)
-					CurrentUser.user = @user3
-					@post.flag!("blah blah")
-					CurrentUser.user = @approver
-
-					assert_raises(Post::ApprovalError) do
-						@post.approve!
-					end
-				end
-			end
-		end
-	end
-end

test/unit/post_approval_test.rb

@@ -0,0 +1,60 @@
+require 'test_helper'
+
+class PostApprovalTest < ActiveSupport::TestCase
+  context "a pending post" do
+    setup do
+      @user = FactoryGirl.create(:user)
+      CurrentUser.user = @user
+      CurrentUser.ip_addr = "127.0.0.1"
+
+      @post = FactoryGirl.create(:post, uploader_id: @user.id, is_pending: true)
+
+      @approver = FactoryGirl.create(:user)
+      @approver.can_approve_posts = true
+      @approver.save
+      CurrentUser.user = @approver
+
+      CurrentUser.stubs(:can_approve_posts?).returns(true)
+    end
+
+    teardown do
+      CurrentUser.user = nil
+      CurrentUser.ip_addr = nil
+    end
+
+    should "allow approval" do
+      assert_equal(false, @post.approved_by?(@approver))
+    end
+
+    context "That is approved" do
+      should "create a postapproval record" do
+        assert_difference("PostApproval.count") do
+          @post.approve!
+        end
+      end
+
+      context "that is then flagged" do
+        setup do
+          @user2 = FactoryGirl.create(:user)
+          @user3 = FactoryGirl.create(:user)
+          @approver2 = FactoryGirl.create(:user)
+          @approver2.can_approve_posts = true
+          @approver2.save
+        end
+
+        should "prevent the first approver from approving again" do
+          @post.approve!(@approver)
+          CurrentUser.user = @user2
+          @post.flag!("blah")
+          @post.approve!(@approver2)
+          assert_not_equal(@approver.id, @post.approver_id)
+          CurrentUser.user = @user3
+          @post.flag!("blah blah")
+
+          approval = @post.approve!(@approver)
+          assert_includes(approval.errors.full_messages, "You have previously approved this post and cannot approve it again")
+        end
+      end
+    end
+  end
+end

test/unit/post_test.rb

@@ -412,13 +412,10 @@ class PostTest < ActiveSupport::TestCase
         end
 
         should "not allow person X to approve that post" do
-          assert_raises(Post::ApprovalError) do
+          approval = @post.approve!(@post.uploader)
-            CurrentUser.scoped(@post.uploader, "127.0.0.1") do
-              @post.approve!
-            end
-          end
 
-          assert_equal(["You cannot approve a post you uploaded"], @post.errors.full_messages)
+          assert(@post.invalid?)
+          assert_includes(approval.errors.full_messages, "You cannot approve a post you uploaded")
         end
       end
 
@@ -431,19 +428,13 @@ class PostTest < ActiveSupport::TestCase
         end
 
         should "not allow person X to reapprove that post" do
-          CurrentUser.scoped(@user, "127.0.0.1") do
+          approval = @post.approve!(@user)
-            assert_raises(Post::ApprovalError) do
+          assert_includes(approval.errors.full_messages, "You have previously approved this post and cannot approve it again")
-              @post.approve!
-            end
-          end
         end
 
         should "allow person Y to approve the post" do
-          CurrentUser.scoped(@user2, "127.0.0.1") do
+          @post.approve!(@user2)
-            assert_nothing_raised do
+          assert(@post.valid?)
-              @post.approve!
-            end
-          end
         end
       end
 
@@ -479,9 +470,8 @@ class PostTest < ActiveSupport::TestCase
       end
 
       should "not allow approval" do
-        assert_raises(Post::ApprovalError) do
+        approval = @post.approve!
-          @post.approve!
+        assert_includes(approval.errors.full_messages, "Post is locked and cannot be approved")
-        end
       end
     end
   end