diff --git a/app/controllers/post_replacements_controller.rb b/app/controllers/post_replacements_controller.rb index ef93a1736..b001b2e8d 100644 --- a/app/controllers/post_replacements_controller.rb +++ b/app/controllers/post_replacements_controller.rb @@ -27,7 +27,7 @@ class PostReplacementsController < ApplicationController @post_replacement = PostReplacement.find(params[:id]) @post_replacement.approve!(penalize_current_uploader: params[:penalize_current_uploader]) - respond_with(@post_replacement) + respond_with(@post_replacement, location: post_path(@post_replacement.post)) end def toggle_penalize @@ -48,7 +48,7 @@ class PostReplacementsController < ApplicationController @post_replacement = PostReplacement.find(params[:id]) @post_replacement.destroy - respond_with(@post_replacement) + respond_with(@post_replacement, location: post_path(@post_replacement.post)) end def promote @@ -59,7 +59,7 @@ class PostReplacementsController < ApplicationController elsif @upload.errors.any? respond_with(@upload) else - respond_with(@upload) + respond_with(@upload.post) end end diff --git a/app/javascript/src/javascripts/post_replacement.js b/app/javascript/src/javascripts/post_replacement.js index 40df41ab6..3e0c6e15e 100644 --- a/app/javascript/src/javascripts/post_replacement.js +++ b/app/javascript/src/javascripts/post_replacement.js @@ -55,8 +55,7 @@ PostReplacement.promote = function (id) { url: `/post_replacements/${id}/promote.json`, dataType: 'json' }).done(function (data) { - console.log(data); - Utility.notice(`Replacement promoted to post #${data.post_id}`) + Utility.notice(`Replacement promoted to post #${data.post.id}`) }).fail(function (data, status, xhr) { Utility.error(data.responseText); }); diff --git a/app/logical/upload_service/replacer.rb b/app/logical/upload_service/replacer.rb index 788b6b52c..861a75297 100644 --- a/app/logical/upload_service/replacer.rb +++ b/app/logical/upload_service/replacer.rb @@ -76,6 +76,8 @@ class UploadService post.generated_samples = nil end + previous_uploader = post.uploader_id + post.md5 = upload.md5 post.file_ext = upload.file_ext post.image_width = upload.image_width @@ -88,19 +90,21 @@ class UploadService post.uploader_ip_addr = replacement.creator_ip_addr post.save! + + # rescaling notes reloads the post, be careful when accessing previous values rescale_notes(post) update_ugoira_frame_data(post, upload) replacement.update({ status: 'approved', approver_id: CurrentUser.id, - uploader_id_on_approve: post.uploader_id_before_last_save, - penalize_uploader_on_approve: penalize_current_uploader + uploader_id_on_approve: previous_uploader, + penalize_uploader_on_approve: penalize_current_uploader.to_s.truthy? }) - UserStatus.for_user(post.uploader_id_before_last_save).update_all("own_post_replaced_count = own_post_replaced_count + 1") - if penalize_current_uploader - UserStatus.for_user(post.uploader_id_before_last_save).update_all("own_post_replaced_penalize_count = own_post_replaced_penalize_count + 1") + UserStatus.for_user(previous_uploader).update_all("own_post_replaced_count = own_post_replaced_count + 1") + if penalize_current_uploader.to_s.truthy? + UserStatus.for_user(previous_uploader).update_all("own_post_replaced_penalize_count = own_post_replaced_penalize_count + 1") end if post.is_video? diff --git a/app/models/post_replacement.rb b/app/models/post_replacement.rb index 7553f3078..46a058a3a 100644 --- a/app/models/post_replacement.rb +++ b/app/models/post_replacement.rb @@ -296,6 +296,18 @@ class PostReplacement < ApplicationRecord where(creator_id: id.to_i) end + def for_uploader_on_approve(id) + where(uploader_id_on_approve: id.to_i) + end + + def penalized + where(penalize_uploader_on_approve: true) + end + + def not_penalized + where(penalize_uploader_on_approve: false) + end + def visible(user) return where('status != ?', 'rejected') if user.is_anonymous? return all if user.is_janitor? diff --git a/test/functional/post_replacements_controller_test.rb b/test/functional/post_replacements_controller_test.rb index bacd5fd4a..4486d3967 100644 --- a/test/functional/post_replacements_controller_test.rb +++ b/test/functional/post_replacements_controller_test.rb @@ -3,11 +3,11 @@ require 'test_helper' class PostReplacementsControllerTest < ActionDispatch::IntegrationTest setup do - Sidekiq::Testing::inline! + Sidekiq::Testing.inline! end teardown do - Sidekiq::Testing::fake! + Sidekiq::Testing.fake! end context "The post replacements controller" do @@ -43,7 +43,7 @@ class PostReplacementsControllerTest < ActionDispatch::IntegrationTest context "reject action" do should "reject replacement" do put_auth reject_post_replacement_path(@replacement), @user - assert_redirected_to post_replacement_path(@replacement) + assert_redirected_to post_path(@post) @replacement.reload @post.reload assert_equal @replacement.status, "rejected" @@ -64,7 +64,7 @@ class PostReplacementsControllerTest < ActionDispatch::IntegrationTest context "promote action" do should "create post" do - put_auth promote_post_replacement_path(@replacement), @user + post_auth promote_post_replacement_path(@replacement), @user last_post = Post.last assert_redirected_to post_path(last_post) @replacement.reload @@ -74,6 +74,18 @@ class PostReplacementsControllerTest < ActionDispatch::IntegrationTest end end + context "toggle action" do + should "change penalize_uploader flag" do + put_auth approve_post_replacement_path(@replacement, penalize_current_uploader: true), @user + @replacement.reload + assert @replacement.penalize_uploader_on_approve + put_auth toggle_penalize_post_replacement_path(@replacement), @user + assert_redirected_to post_replacement_path(@replacement) + @replacement.reload + assert !@replacement.penalize_uploader_on_approve + end + end + context "index action" do should "render" do get post_replacements_path diff --git a/test/unit/post_replacement_test.rb b/test/unit/post_replacement_test.rb index 77f6771e3..603895655 100644 --- a/test/unit/post_replacement_test.rb +++ b/test/unit/post_replacement_test.rb @@ -77,7 +77,7 @@ class PostReplacementTest < ActiveSupport::TestCase end should "affect user upload limit" do - assert_difference("PostReplacement.pending.for_user(@user.id).count", 1) do + assert_difference(->{PostReplacement.pending.for_user(@user.id).count}, 1) do @replacement = @post.replacements.create(FactoryBot.attributes_for(:png_replacement).merge(creator: @user, creator_ip_addr: '127.0.0.1')) end end @@ -109,10 +109,26 @@ class PostReplacementTest < ActiveSupport::TestCase end should "give user back their upload slot" do - assert_difference("PostReplacement.pending.for_user(@user.id).count", -1) do + assert_difference(->{PostReplacement.pending.for_user(@user.id).count}, -1) do @replacement.reject! end end + + should "increment the users rejected replacements count" do + assert_difference(->{@user.post_replacement_rejected_count}, 1) do + assert_difference(->{PostReplacement.rejected.for_user(@user.id).count}, 1) do + @replacement.reject! + @user.reload + end + end + end + + should "work only once for pending replacements" do + @replacement.reject! + assert_equal [], @replacement.errors.full_messages + @replacement.reject! + assert_equal ["Status must be pending to reject"], @replacement.errors.full_messages + end end context "Approve:" do @@ -124,20 +140,21 @@ class PostReplacementTest < ActiveSupport::TestCase should "create a mod action" do assert_difference("ModAction.count") do - @replacement.approve! + @replacement.approve! penalize_current_uploader: true end end should "fail if post cannot be backed up" do @post.md5 = "123" # Breaks file path, should force backup to fail. assert_raise(ProcessingError) do - @replacement.approve! + @replacement.approve! penalize_current_uploader: true end end + should "update post with new image" do old_md5 = @post.md5 - @replacement.approve! + @replacement.approve! penalize_current_uploader: true @post.reload assert_not_equal @post.md5, old_md5 assert_equal @replacement.image_width, @post.image_width @@ -151,25 +168,25 @@ class PostReplacementTest < ActiveSupport::TestCase should "generate videos samples if replacement is video" do @replacement = FactoryBot.create(:webm_replacement, creator: @user, creator_ip_addr: '127.0.0.1', post: @post) @post.expects(:generate_video_samples).times(1) - @replacement.approve! + @replacement.approve! penalize_current_uploader: true end should "delete original files immediately" do sm = Danbooru.config.storage_manager old_md5 = @post.md5 old_ext = @post.file_ext - @replacement.approve! + @replacement.approve! penalize_current_uploader: true @post.reload - assert_not File.exists?(sm.file_path(old_md5, old_ext, :original)) - assert_not File.exists?(sm.file_path(old_md5, old_ext, :preview)) - assert_not File.exists?(sm.file_path(old_md5, old_ext, :large)) - assert_not File.exists?(sm.file_path(old_md5, old_ext, :original, protected=true)) + assert_not File.exist?(sm.file_path(old_md5, old_ext, :original)) + assert_not File.exist?(sm.file_path(old_md5, old_ext, :preview)) + assert_not File.exist?(sm.file_path(old_md5, old_ext, :large)) + assert_not File.exist?(sm.file_path(old_md5, old_ext, :original, protected=true)) end should "not be able to approve on deleted post" do @post.update_column(:is_deleted, true) assert_raise ProcessingError do - @replacement.approve! + @replacement.approve! penalize_current_uploader: true end end @@ -177,7 +194,7 @@ class PostReplacementTest < ActiveSupport::TestCase old_md5 = @post.md5 old_source = @post.source assert_difference("@post.replacements.size", 1) do - @replacement.approve! + @replacement.approve! penalize_current_uploader: true end new_replacement = @post.replacements.last assert_equal 'original', new_replacement.status @@ -187,21 +204,78 @@ class PostReplacementTest < ActiveSupport::TestCase end should "update users upload counts" do - assert_difference("Post.for_user(@mod_user.id).where('is_flagged = false AND is_deleted = false AND is_pending = false').count", -1) do - assert_difference("Post.for_user(@user.id).where('is_flagged = false AND is_deleted = false AND is_pending = false').count", 1) do - @replacement.approve! + assert_difference(->{Post.for_user(@mod_user.id).where('is_flagged = false AND is_deleted = false AND is_pending = false').count}, -1) do + assert_difference(->{Post.for_user(@user.id).where('is_flagged = false AND is_deleted = false AND is_pending = false').count}, 1) do + @replacement.approve! penalize_current_uploader: true + end + end + end + + should "update the original users upload limit if penalized" do + assert_difference(->{@mod_user.own_post_replaced_count}, 1) do + assert_difference(->{@mod_user.own_post_replaced_penalize_count}, 1) do + assert_difference(->{PostReplacement.penalized.for_uploader_on_approve(@mod_user.id).count}, 1) do + @replacement.approve! penalize_current_uploader: true + @mod_user.reload + end + end + end + end + + should "not update the original users upload limit if not penalizing" do + assert_difference(-> {@mod_user.own_post_replaced_count}, 1) do + assert_difference(->{@mod_user.own_post_replaced_penalize_count}, 0) do + assert_difference(->{PostReplacement.not_penalized.for_uploader_on_approve(@mod_user.id).count}, 1) do + @replacement.approve! penalize_current_uploader: false + @mod_user.reload + end end end end should "correctly resize the posts notes" do - @replacement.approve! + @replacement.approve! penalize_current_uploader: true @note.reload assert_equal 153, @note.x assert_equal 611, @note.y assert_equal 153, @note.width assert_equal 152, @note.height end + + should "only work on pending or original replacements" do + @replacement.reject! + @replacement.approve! penalize_current_uploader: false + assert_equal(["Status must be pending or original to approve"], @replacement.errors.full_messages) + end + + should "only work once" do + @replacement.approve! penalize_current_uploader: false + assert_equal [], @replacement.errors.full_messages + @replacement.approve! penalize_current_uploader: false + assert_equal ["Status must be pending or original to approve"], @replacement.errors.full_messages + end + end + + context "Toggle:" do + setup do + @replacement = FactoryBot.create(:png_replacement, creator: @user, creator_ip_addr: '127.0.0.1', post: @post) + assert @replacement + end + + should "change the users upload limit" do + @replacement.approve! penalize_current_uploader: false + assert_difference(->{@mod_user.own_post_replaced_penalize_count}, 1) do + assert_difference(->{PostReplacement.penalized.for_uploader_on_approve(@mod_user.id).count}, 1) do + @replacement.toggle_penalize! + @mod_user.reload + end + end + end + + should "only work on appoved replacements" do + @replacement.toggle_penalize! + assert_equal(["Status must be approved to penalize"], @replacement.errors.full_messages) + end end context "Promote:" do @@ -226,8 +300,8 @@ class PostReplacementTest < ActiveSupport::TestCase end should "credit replacer with new post" do - assert_difference("Post.for_user(@mod_user.id).where('is_flagged = false AND is_deleted = false AND is_pending = false').count", 0) do - assert_difference("Post.for_user(@user.id).where('is_flagged = false AND is_deleted = false').count", 1) do + assert_difference(->{Post.for_user(@mod_user.id).where('is_flagged = false AND is_deleted = false AND is_pending = false').count}, 0) do + assert_difference(->{Post.for_user(@user.id).where('is_flagged = false AND is_deleted = false').count}, 1) do post = @replacement.promote! assert post assert_equal [], post.errors.full_messages @@ -235,5 +309,11 @@ class PostReplacementTest < ActiveSupport::TestCase end end end + + should "only work on pending replacements" do + @replacement.approve! penalize_current_uploader: false + @replacement.promote! + assert_equal(["Status must be pending to promote"], @replacement.errors.full_messages) + end end end