From f4d0a77d5f40ab8ab52d2c92f9700fe076157aa3 Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Wed, 27 Nov 2024 17:23:57 +0800 Subject: [PATCH] DEV: Add "delete user" options to illegal flag review (#29956) We already add the "delete user" and "delete and block user" options to the drop-down for potential spam, but we should do this for potentially illegal posts as well. This is entirely based on the implementation for the potential spam one, including caching the status on the reviewable record. Also note that just as for potential spam, the user must be "deletable" for the option to appear. I also took the liberty to move the options in the drop-down to what I think is a more intuitive place. (Between delete post and suspend/silence user.) --- app/models/reviewable.rb | 7 ++++- app/models/reviewable_flagged_post.rb | 9 +++--- app/models/reviewable_post.rb | 1 + app/models/reviewable_queued_post.rb | 1 + app/models/reviewable_user.rb | 1 + ..._add_potentially_illegal_to_reviewables.rb | 16 +++++++++++ lib/post_action_creator.rb | 1 + spec/models/reviewable_flagged_post_spec.rb | 28 ++++++++++++++++++- 8 files changed, 58 insertions(+), 6 deletions(-) create mode 100644 db/migrate/20241127034553_add_potentially_illegal_to_reviewables.rb diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb index 0a790951d12..5535ca375f3 100644 --- a/app/models/reviewable.rb +++ b/app/models/reviewable.rb @@ -103,6 +103,7 @@ class Reviewable < ActiveRecord::Base payload: nil, reviewable_by_moderator: false, potential_spam: true, + potentially_illegal: false, target_created_by: nil ) reviewable = @@ -113,6 +114,7 @@ class Reviewable < ActiveRecord::Base reviewable_by_moderator: reviewable_by_moderator, payload: payload, potential_spam: potential_spam, + potentially_illegal: potentially_illegal, target_created_by: target_created_by, ) reviewable.created_new! @@ -136,12 +138,14 @@ class Reviewable < ActiveRecord::Base id: target.id, type: target.class.polymorphic_name, potential_spam: potential_spam == true ? true : nil, + potentially_illegal: potentially_illegal == true ? true : nil, } row = DB.query_single(<<~SQL, update_args) UPDATE reviewables SET status = :status, - potential_spam = COALESCE(:potential_spam, reviewables.potential_spam) + potential_spam = COALESCE(:potential_spam, reviewables.potential_spam), + potentially_illegal = COALESCE(:potentially_illegal, reviewables.potentially_illegal) FROM reviewables AS old_reviewables WHERE reviewables.target_id = :id AND reviewables.target_type = :type @@ -776,6 +780,7 @@ end # updated_at :datetime not null # force_review :boolean default(FALSE), not null # reject_reason :text +# potentially_illegal :boolean default(FALSE) # # Indexes # diff --git a/app/models/reviewable_flagged_post.rb b/app/models/reviewable_flagged_post.rb index 1c870b53840..7e7a334a405 100644 --- a/app/models/reviewable_flagged_post.rb +++ b/app/models/reviewable_flagged_post.rb @@ -48,10 +48,6 @@ class ReviewableFlaggedPost < Reviewable agree_bundle = actions.add_bundle("#{id}-agree", icon: "thumbs-up", label: "reviewables.actions.agree.title") - if potential_spam? && guardian.can_delete_user?(target_created_by) - delete_user_actions(actions, agree_bundle) - end - if !post.user_deleted? && !post.hidden? build_action(actions, :agree_and_hide, icon: "far-eye-slash", bundle: agree_bundle) end @@ -83,6 +79,10 @@ class ReviewableFlaggedPost < Reviewable end end + if (potential_spam? || potentially_illegal?) && guardian.can_delete_user?(target_created_by) + delete_user_actions(actions, agree_bundle) + end + if guardian.can_suspend?(target_created_by) build_action( actions, @@ -416,6 +416,7 @@ end # updated_at :datetime not null # force_review :boolean default(FALSE), not null # reject_reason :text +# potentially_illegal :boolean default(FALSE) # # Indexes # diff --git a/app/models/reviewable_post.rb b/app/models/reviewable_post.rb index b9f7d3653ea..addd50650fc 100644 --- a/app/models/reviewable_post.rb +++ b/app/models/reviewable_post.rb @@ -155,6 +155,7 @@ end # updated_at :datetime not null # force_review :boolean default(FALSE), not null # reject_reason :text +# potentially_illegal :boolean default(FALSE) # # Indexes # diff --git a/app/models/reviewable_queued_post.rb b/app/models/reviewable_queued_post.rb index a36e9f61738..342508912fe 100644 --- a/app/models/reviewable_queued_post.rb +++ b/app/models/reviewable_queued_post.rb @@ -257,6 +257,7 @@ end # updated_at :datetime not null # force_review :boolean default(FALSE), not null # reject_reason :text +# potentially_illegal :boolean default(FALSE) # # Indexes # diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb index 12a05027256..c7e2429ac76 100644 --- a/app/models/reviewable_user.rb +++ b/app/models/reviewable_user.rb @@ -119,6 +119,7 @@ end # updated_at :datetime not null # force_review :boolean default(FALSE), not null # reject_reason :text +# potentially_illegal :boolean default(FALSE) # # Indexes # diff --git a/db/migrate/20241127034553_add_potentially_illegal_to_reviewables.rb b/db/migrate/20241127034553_add_potentially_illegal_to_reviewables.rb new file mode 100644 index 00000000000..29b79030008 --- /dev/null +++ b/db/migrate/20241127034553_add_potentially_illegal_to_reviewables.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true +# +class AddPotentiallyIllegalToReviewables < ActiveRecord::Migration[7.1] + def change + add_column :reviewables, :potentially_illegal, :boolean + + up_only do + # NOTE: Only for records created after this migration. Trying to + # apply this as part of adding the column will attempt to backfill + # `false` into all existing reviewables. This is dangerous (locks + # a potentially huge table) and will create some false negatives. + # + change_column_default :reviewables, :potentially_illegal, false + end + end +end diff --git a/lib/post_action_creator.rb b/lib/post_action_creator.rb index 41f2432c413..d0de531ae45 100644 --- a/lib/post_action_creator.rb +++ b/lib/post_action_creator.rb @@ -388,6 +388,7 @@ class PostActionCreator topic: @post.topic, reviewable_by_moderator: true, potential_spam: @post_action_type_id == @post_action_type_view.types[:spam], + potentially_illegal: @post_action_type_id == @post_action_type_view.types[:illegal], payload: { targets_topic: @targets_topic, }, diff --git a/spec/models/reviewable_flagged_post_spec.rb b/spec/models/reviewable_flagged_post_spec.rb index bbfcae30d2b..c917072263d 100644 --- a/spec/models/reviewable_flagged_post_spec.rb +++ b/spec/models/reviewable_flagged_post_spec.rb @@ -16,6 +16,13 @@ RSpec.describe ReviewableFlaggedPost, type: :model do expect(reviewable.reload.potential_spam?).to eq(true) end + it "sets `potentially_illegal` when an illegal flag is added" do + reviewable = PostActionCreator.off_topic(user, post).reviewable + expect(reviewable.potentially_illegal?).to eq(false) + PostActionCreator.illegal(Fabricate(:user, refresh_auto_groups: true), post) + expect(reviewable.reload.potentially_illegal?).to eq(true) + end + describe "actions" do let!(:result) { PostActionCreator.spam(user, post) } let(:reviewable) { result.reviewable } @@ -95,7 +102,26 @@ RSpec.describe ReviewableFlaggedPost, type: :model do end context "when flagged as potential_spam" do - before { reviewable.update!(potential_spam: true) } + before { reviewable.update!(potential_spam: true, potentially_illegal: false) } + + it "excludes delete action if the reviewer cannot delete the user" do + post.user.user_stat.update!( + first_post_created_at: 1.year.ago, + post_count: User::MAX_STAFF_DELETE_POST_COUNT + 1, + ) + + expect(reviewable.actions_for(guardian).has?(:delete_user)).to be false + expect(reviewable.actions_for(guardian).has?(:delete_user_block)).to be false + end + + it "includes delete actions if the reviewer can delete the user" do + expect(reviewable.actions_for(guardian).has?(:delete_user)).to be true + expect(reviewable.actions_for(guardian).has?(:delete_user_block)).to be true + end + end + + context "when flagged as illegal" do + before { reviewable.update(potential_spam: false, potentially_illegal: true) } it "excludes delete action if the reviewer cannot delete the user" do post.user.user_stat.update!(