From 258fc37bfe8694cd79cb777c2649a6848a779779 Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 3 Apr 2017 00:10:36 -0500 Subject: [PATCH 1/6] Post#approve!: move validation to post_approval.rb --- app/models/post.rb | 26 ++++++++------------------ app/models/post_approval.rb | 18 +++++++++++++++--- app/models/user.rb | 1 + 3 files changed, 24 insertions(+), 21 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index cb1d7493a..2f6042605 100644 --- a/app/models/post.rb +++ b/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? - !is_status_locked? && (is_pending? || is_flagged? || is_deleted?) && !PostApproval.approved?(CurrentUser.id, id) + def is_approvable?(user = CurrentUser.user) + !is_status_locked? && (is_pending? || is_flagged? || is_deleted?) && !approved_by?(user) end def flag!(reason, options = {}) @@ -321,21 +322,6 @@ class Post < ActiveRecord::Base end def approve! - if is_status_locked? - errors.add(:is_status_locked, "; post cannot be approved") - raise ApprovalError.new("Post is locked and cannot be approved") - end - - if uploader_id == CurrentUser.id - errors.add(:base, "You cannot approve a post you uploaded") - 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 @@ -351,6 +337,10 @@ class Post < ActiveRecord::Base save! end + def approved_by?(user) + approver == user || approvals.where(user: user).exists? + end + def disapproved_by?(user) PostDisapproval.where(:user_id => user.id, :post_id => id).exists? end @@ -1396,7 +1386,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") diff --git a/app/models/post_approval.rb b/app/models/post_approval.rb index 7fbfe741f..86d4e0206 100644 --- a/app/models/post_approval.rb +++ b/app/models/post_approval.rb @@ -1,12 +1,24 @@ class PostApproval < ActiveRecord::Base belongs_to :user - belongs_to :post + belongs_to :post, inverse_of: :approvals + + validate :validate_approval def self.prune! where("created_at < ?", 1.month.ago).delete_all end - def self.approved?(user_id, post_id) - where(user_id: user_id, post_id: post_id).exists? + def validate_approval + if post.is_status_locked? + errors.add(:post, "is locked 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 end diff --git a/app/models/user.rb b/app/models/user.rb index c253907c5..8512f9bda 100644 --- a/app/models/user.rb +++ b/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" From db0bcf08b9220649356dbf1b7ceb1386758a0d99 Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 3 Apr 2017 13:30:13 -0500 Subject: [PATCH 2/6] Post#approve!: move approving logic to post_approval.rb. --- app/models/post.rb | 16 ++-------------- app/models/post_approval.rb | 8 ++++++++ 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index 2f6042605..328c36ad1 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -321,20 +321,8 @@ class Post < ActiveRecord::Base end end - def approve! - 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! + def approve!(approver = CurrentUser.user) + approvals.create!(user: approver) end def approved_by?(user) diff --git a/app/models/post_approval.rb b/app/models/post_approval.rb index 86d4e0206..26abd3705 100644 --- a/app/models/post_approval.rb +++ b/app/models/post_approval.rb @@ -3,6 +3,7 @@ class PostApproval < ActiveRecord::Base belongs_to :post, inverse_of: :approvals validate :validate_approval + after_create :approve_post def self.prune! where("created_at < ?", 1.month.ago).delete_all @@ -21,4 +22,11 @@ class PostApproval < ActiveRecord::Base 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 From 70a7f77a488606b04e76d22de9f397164503e1df Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 3 Apr 2017 13:45:17 -0500 Subject: [PATCH 3/6] Post#approve!: signal errors with invalid object instead of exception. --- app/controllers/moderator/post/approvals_controller.rb | 9 ++++----- app/models/post.rb | 2 +- app/models/post_approval.rb | 4 ++++ app/views/moderator/post/approvals/create.js.erb | 6 +++--- 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/app/controllers/moderator/post/approvals_controller.rb b/app/controllers/moderator/post/approvals_controller.rb index c17f5c5fe..1da5998e2 100644 --- a/app/controllers/moderator/post/approvals_controller.rb +++ b/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]) - if @post.is_deleted? || @post.is_flagged? || @post.is_pending? - @post.approve! - end - rescue ::Post::ApprovalError + post = ::Post.find(params[:post_id]) + @approval = post.approve! + respond_with(@approval) end end end diff --git a/app/models/post.rb b/app/models/post.rb index 328c36ad1..f29271d1d 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -322,7 +322,7 @@ class Post < ActiveRecord::Base end def approve!(approver = CurrentUser.user) - approvals.create!(user: approver) + approvals.create(user: approver) end def approved_by?(user) diff --git a/app/models/post_approval.rb b/app/models/post_approval.rb index 26abd3705..47f476699 100644 --- a/app/models/post_approval.rb +++ b/app/models/post_approval.rb @@ -14,6 +14,10 @@ class PostApproval < ActiveRecord::Base 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 diff --git a/app/views/moderator/post/approvals/create.js.erb b/app/views/moderator/post/approvals/create.js.erb index 515a6bcc4..52586b311 100644 --- a/app/views/moderator/post/approvals/create.js.erb +++ b/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(); From e6328b8d30e2a48ba017edd621c2bcbc85699956 Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 3 Apr 2017 15:14:07 -0500 Subject: [PATCH 4/6] post_approval_test.rb: move to test/unit. --- test/{models => unit}/post_approval_test.rb | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/{models => unit}/post_approval_test.rb (100%) diff --git a/test/models/post_approval_test.rb b/test/unit/post_approval_test.rb similarity index 100% rename from test/models/post_approval_test.rb rename to test/unit/post_approval_test.rb From 7c8135609b6c6a9a446bb24c5978897cbd3eda4c Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 3 Apr 2017 15:15:06 -0500 Subject: [PATCH 5/6] post_approval_test.rb: tabs to spaces. --- test/unit/post_approval_test.rb | 100 ++++++++++++++++---------------- 1 file changed, 50 insertions(+), 50 deletions(-) diff --git a/test/unit/post_approval_test.rb b/test/unit/post_approval_test.rb index 835324f68..be5a51117 100644 --- a/test/unit/post_approval_test.rb +++ b/test/unit/post_approval_test.rb @@ -1,63 +1,63 @@ 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" + 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) + @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 + @approver = FactoryGirl.create(:user) + @approver.can_approve_posts = true + @approver.save + CurrentUser.user = @approver - CurrentUser.stubs(:can_approve_posts?).returns(true) - end + CurrentUser.stubs(:can_approve_posts?).returns(true) + end - teardown do - CurrentUser.user = nil - CurrentUser.ip_addr = nil - 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 + 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 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 + 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 + 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 + assert_raises(Post::ApprovalError) do + @post.approve! + end + end + end + end + end end From 5db39f308a0ea892cfcba4f5c5af1e063d176a58 Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 3 Apr 2017 15:18:42 -0500 Subject: [PATCH 6/6] tests: fix post approval tests. --- test/unit/post_approval_test.rb | 13 +++++-------- test/unit/post_test.rb | 28 +++++++++------------------- 2 files changed, 14 insertions(+), 27 deletions(-) diff --git a/test/unit/post_approval_test.rb b/test/unit/post_approval_test.rb index be5a51117..c718feb37 100644 --- a/test/unit/post_approval_test.rb +++ b/test/unit/post_approval_test.rb @@ -23,7 +23,7 @@ class PostApprovalTest < ActiveSupport::TestCase end should "allow approval" do - assert_equal(false, PostApproval.approved?(@approver.id, @post.id)) + assert_equal(false, @post.approved_by?(@approver)) end context "That is approved" do @@ -43,19 +43,16 @@ class PostApprovalTest < ActiveSupport::TestCase end should "prevent the first approver from approving again" do - @post.approve! + @post.approve!(@approver) CurrentUser.user = @user2 @post.flag!("blah") - CurrentUser.user = @approver2 - @post.approve! + @post.approve!(@approver2) 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 + approval = @post.approve!(@approver) + assert_includes(approval.errors.full_messages, "You have previously approved this post and cannot approve it again") end end end diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index 151afe9a2..1f702cce0 100644 --- a/test/unit/post_test.rb +++ b/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 - CurrentUser.scoped(@post.uploader, "127.0.0.1") do - @post.approve! - end - end + approval = @post.approve!(@post.uploader) - 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 - assert_raises(Post::ApprovalError) do - @post.approve! - end - end + approval = @post.approve!(@user) + assert_includes(approval.errors.full_messages, "You have previously approved this post and cannot approve it again") end should "allow person Y to approve the post" do - CurrentUser.scoped(@user2, "127.0.0.1") do - assert_nothing_raised do - @post.approve! - end - end + @post.approve!(@user2) + assert(@post.valid?) end end @@ -479,9 +470,8 @@ class PostTest < ActiveSupport::TestCase end should "not allow approval" do - assert_raises(Post::ApprovalError) do - @post.approve! - end + approval = @post.approve! + assert_includes(approval.errors.full_messages, "Post is locked and cannot be approved") end end end