Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEV: Convert min_trust_to_flag_posts setting to groups #24864

Merged
merged 1 commit into from Dec 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions config/locales/server.en.yml
Expand Up @@ -1972,6 +1972,7 @@ en:
min_trust_to_send_messages: "DEPRECATED, use the 'personal message enabled groups' setting instead. The minimum trust level required to create new personal messages."
min_trust_to_send_email_messages: "The minimum trust level required to send personal messages via email."
min_trust_to_flag_posts: "The minimum trust level required to flag posts"
flag_post_allowed_groups: "Groups that are allowed to flag posts."
min_trust_to_post_links: "The minimum trust level required to include links in posts"
min_trust_to_post_embedded_media: "The minimum trust level required to embed media items in a post"
min_trust_level_to_allow_profile_background: "The minimum trust level required to upload a profile background"
Expand Down Expand Up @@ -2559,6 +2560,7 @@ en:
uploaded_avatars_allowed_groups: "allow_uploaded_avatars"
create_topic_allowed_groups: "min_trust_to_create_topic"
edit_post_allowed_groups: "min_trust_to_edit_post"
flag_post_allowed_groups: "min_trust_to_flag_posts"

placeholder:
discourse_connect_provider_secrets:
Expand Down
6 changes: 6 additions & 0 deletions config/site_settings.yml
Expand Up @@ -1715,6 +1715,12 @@ trust:
min_trust_to_flag_posts:
default: 1
enum: "TrustLevelSetting"
flag_post_allowed_groups:
default: "11"
type: group_list
allow_any: false
refresh: true
validator: "AtLeastOneGroupValidator"
min_trust_to_post_links:
default: 0
enum: "TrustLevelSetting"
Expand Down
@@ -0,0 +1,27 @@
# frozen_string_literal: true

class FillFlagPostAllowedGroupsBasedOnDeprecatedSettings < ActiveRecord::Migration[7.0]
def up
configured_trust_level =
DB.query_single(
"SELECT value FROM site_settings WHERE name = 'min_trust_to_flag_posts' LIMIT 1",
).first

# Default for old setting is TL1, we only need to do anything if it's been changed in the DB.
if configured_trust_level.present?
# Matches Group::AUTO_GROUPS to the trust levels.
corresponding_group = "1#{configured_trust_level}"

# Data_type 20 is group_list.
DB.exec(
"INSERT INTO site_settings(name, value, data_type, created_at, updated_at)
VALUES('flag_post_allowed_groups', :setting, '20', NOW(), NOW())",
setting: corresponding_group,
)
end
end

def down
raise ActiveRecord::IrreversibleMigration
end
end
2 changes: 1 addition & 1 deletion lib/guardian/post_guardian.rb
Expand Up @@ -78,7 +78,7 @@ def post_can_act?(post, action_key, opts: {}, can_see_post: nil)
(
is_flag && not(already_did_flagging) &&
(
@user.has_trust_level?(TrustLevel[SiteSetting.min_trust_to_flag_posts]) ||
Copy link
Contributor Author

@Drenmi Drenmi Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lis2 User#has_trust_level? does a few other checks, e.g. if the user is admin or moderator. Shall I add those groups as well, in the migration? 🤔 I suspect we don't maintain any invariant that admins and mods are always in the highest trust level group?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is fine, especially that default is TL1 so original requirement is quite low

@user.in_any_groups?(SiteSetting.flag_post_allowed_groups_map) ||
post.topic.private_message?
)
) ||
Expand Down
1 change: 1 addition & 0 deletions lib/site_settings/deprecated_settings.rb
Expand Up @@ -23,6 +23,7 @@ module SiteSettings::DeprecatedSettings
["allow_uploaded_avatars", "uploaded_avatars_allowed_groups", false, "3.3"],
["min_trust_to_create_topic", "create_topic_allowed_groups", false, "3.3"],
["min_trust_to_edit_post", "edit_post_allowed_groups", false, "3.3"],
["min_trust_to_flag_posts", "flag_post_allowed_groups", false, "3.3"],
]

def setup_deprecated_methods
Expand Down
Expand Up @@ -2,7 +2,7 @@

describe "Local dates", type: :system do
fab!(:topic)
fab!(:current_user) { Fabricate(:user) }
fab!(:current_user) { Fabricate(:user, refresh_auto_groups: true) }
let(:year) { Time.zone.now.year + 1 }
let(:month) { Time.zone.now.month }
let(:bookmark_modal) { PageObjects::Modals::Bookmark.new }
Expand Down
2 changes: 1 addition & 1 deletion spec/integration/flags_spec.rb
Expand Up @@ -2,7 +2,7 @@

RSpec.describe PostAction do
it "triggers the 'flag_reviewed' event when there was at least one flag" do
admin = Fabricate(:admin)
admin = Fabricate(:admin, refresh_auto_groups: true)

post = Fabricate(:post)
events = DiscourseEvent.track_events { PostDestroyer.new(admin, post).destroy }
Expand Down
4 changes: 2 additions & 2 deletions spec/integration/spam_rules_spec.rb
Expand Up @@ -5,8 +5,8 @@
describe "auto-silence users based on flagging" do
fab!(:admin) # needed to send a system message
fab!(:moderator)
fab!(:user1) { Fabricate(:user) }
fab!(:user2) { Fabricate(:user) }
fab!(:user1) { Fabricate(:user, refresh_auto_groups: true) }
fab!(:user2) { Fabricate(:user, refresh_auto_groups: true) }

before do
SiteSetting.hide_post_sensitivity = Reviewable.sensitivities[:disabled]
Expand Down
2 changes: 1 addition & 1 deletion spec/jobs/auto_queue_handler_spec.rb
Expand Up @@ -6,7 +6,7 @@
describe "old flagged post" do
fab!(:spam_result) do
PostActionCreator.new(
Fabricate(:user),
Fabricate(:user, refresh_auto_groups: true),
Fabricate(:post),
PostActionType.types[:spam],
message: "this is the initial message",
Expand Down
2 changes: 1 addition & 1 deletion spec/jobs/export_user_archive_spec.rb
Expand Up @@ -3,7 +3,7 @@
require "csv"

RSpec.describe Jobs::ExportUserArchive do
fab!(:user) { Fabricate(:user, username: "john_doe") }
fab!(:user) { Fabricate(:user, username: "john_doe", refresh_auto_groups: true) }
fab!(:user2) { Fabricate(:user) }
let(:extra) { {} }
let(:job) do
Expand Down
2 changes: 1 addition & 1 deletion spec/jobs/pending_reviewables_reminder_spec.rb
Expand Up @@ -5,7 +5,7 @@

def create_flag(created_at)
PostActionCreator.create(
Fabricate(:user),
Fabricate(:user, refresh_auto_groups: true),
Fabricate(:post),
:spam,
created_at: created_at,
Expand Down
4 changes: 2 additions & 2 deletions spec/jobs/truncate_user_flag_stats_spec.rb
@@ -1,8 +1,8 @@
# frozen_string_literal: true

RSpec.describe Jobs::TruncateUserFlagStats do
fab!(:user)
fab!(:other_user) { Fabricate(:user) }
fab!(:user) { Fabricate(:user, refresh_auto_groups: true) }
fab!(:other_user) { Fabricate(:user, refresh_auto_groups: true) }

before do
# We might make this a site setting eventually
Expand Down
9 changes: 4 additions & 5 deletions spec/lib/guardian_spec.rb
Expand Up @@ -95,7 +95,7 @@
end

describe "#post_can_act?" do
fab!(:user)
fab!(:user) { Fabricate(:user, refresh_auto_groups: true) }
fab!(:post)

describe "an anonymous user" do
Expand Down Expand Up @@ -233,19 +233,18 @@
end

describe "trust levels" do
before { user.change_trust_level!(TrustLevel[0]) }

it "returns true for a new user liking something" do
user.trust_level = TrustLevel[0]
expect(Guardian.new(user).post_can_act?(post, :like)).to be_truthy
end

it "returns false for a new user flagging as spam" do
user.trust_level = TrustLevel[0]
expect(Guardian.new(user).post_can_act?(post, :spam)).to be_falsey
end

it "returns true for a new user flagging as spam if enabled" do
SiteSetting.min_trust_to_flag_posts = 0
user.trust_level = TrustLevel[0]
SiteSetting.flag_post_allowed_groups = 0
expect(Guardian.new(user).post_can_act?(post, :spam)).to be_truthy
end

Expand Down
33 changes: 27 additions & 6 deletions spec/lib/post_action_creator_spec.rb
Expand Up @@ -2,7 +2,7 @@

RSpec.describe PostActionCreator do
fab!(:admin)
fab!(:user)
fab!(:user) { Fabricate(:user, refresh_auto_groups: true) }
fab!(:post)
let(:like_type_id) { PostActionType.types[:like] }

Expand Down Expand Up @@ -202,7 +202,11 @@

context "with existing reviewable" do
let!(:reviewable) do
PostActionCreator.create(Fabricate(:user), post, :inappropriate).reviewable
PostActionCreator.create(
Fabricate(:user, refresh_auto_groups: true),
post,
:inappropriate,
).reviewable
end

it "appends to an existing reviewable if exists" do
Expand Down Expand Up @@ -273,19 +277,31 @@
describe "take_action" do
it "will hide the post" do
PostActionCreator
.new(Fabricate(:moderator), post, PostActionType.types[:spam], take_action: true)
.new(
Fabricate(:moderator, refresh_auto_groups: true),
post,
PostActionType.types[:spam],
take_action: true,
)
.perform
.reviewable
expect(post.reload).to be_hidden
end

context "when there is another reviewable on the post" do
before { PostActionCreator.create(Fabricate(:user), post, :inappropriate) }
before do
PostActionCreator.create(Fabricate(:user, refresh_auto_groups: true), post, :inappropriate)
end

it "will agree with the old reviewable" do
reviewable =
PostActionCreator
.new(Fabricate(:moderator), post, PostActionType.types[:spam], take_action: true)
.new(
Fabricate(:moderator, refresh_auto_groups: true),
post,
PostActionType.types[:spam],
take_action: true,
)
.perform
.reviewable
expect(reviewable.reload).to be_approved
Expand All @@ -298,7 +314,12 @@

it "still hides the post without considering the score" do
PostActionCreator
.new(Fabricate(:moderator), post, PostActionType.types[:spam], take_action: true)
.new(
Fabricate(:moderator, refresh_auto_groups: true),
post,
PostActionType.types[:spam],
take_action: true,
)
.perform
.reviewable
expect(post.reload).to be_hidden
Expand Down
2 changes: 1 addition & 1 deletion spec/lib/post_action_destroyer_spec.rb
Expand Up @@ -2,7 +2,7 @@

RSpec.describe PostActionDestroyer do
fab!(:admin)
fab!(:user)
fab!(:user) { Fabricate(:user, refresh_auto_groups: true) }
fab!(:post)

describe "#perform" do
Expand Down
2 changes: 1 addition & 1 deletion spec/models/category_spec.rb
Expand Up @@ -88,7 +88,7 @@
fab!(:category) { Fabricate(:category_with_definition, reviewable_by_group: group) }
fab!(:topic) { Fabricate(:topic, category: category) }
fab!(:post) { Fabricate(:post, topic: topic) }
fab!(:user)
fab!(:user) { Fabricate(:user, refresh_auto_groups: true) }

it "will add the group to the reviewable" do
SiteSetting.enable_category_group_moderation = true
Expand Down
22 changes: 12 additions & 10 deletions spec/models/post_action_spec.rb
Expand Up @@ -4,8 +4,8 @@
it { is_expected.to rate_limit }

fab!(:moderator)
fab!(:codinghorror) { Fabricate(:coding_horror) }
fab!(:eviltrout) { Fabricate(:evil_trout) }
fab!(:codinghorror) { Fabricate(:coding_horror, refresh_auto_groups: true) }
fab!(:eviltrout) { Fabricate(:evil_trout, refresh_auto_groups: true) }
fab!(:admin)
fab!(:post)
fab!(:second_post) { Fabricate(:post, topic: post.topic) }
Expand Down Expand Up @@ -456,6 +456,8 @@ def value_for(user_id, dt)
end

describe "flagging" do
before { SiteSetting.flag_post_allowed_groups = "1|2|11" }

it "does not allow you to flag stuff twice, even if the reason is different" do
expect(PostActionCreator.spam(eviltrout, post)).to be_success
expect(PostActionCreator.off_topic(eviltrout, post)).to be_failed
Expand Down Expand Up @@ -524,7 +526,7 @@ def value_for(user_id, dt)

it "should follow the rules for automatic hiding workflow" do
post = create_post
walterwhite = Fabricate(:walter_white)
walterwhite = Fabricate(:walter_white, refresh_auto_groups: true)

Reviewable.set_priorities(high: 3.0)
SiteSetting.hide_post_sensitivity = Reviewable.sensitivities[:low]
Expand Down Expand Up @@ -586,12 +588,12 @@ def value_for(user_id, dt)
expect(post.hidden).to eq(true)
end
it "hide tl0 posts that are flagged as spam by a tl3 user" do
newuser = Fabricate(:newuser, refresh_auto_groups: true)
newuser = Fabricate(:newuser)
post = create_post(user: newuser)

Discourse.stubs(:site_contact_user).returns(admin)

PostActionCreator.spam(Fabricate(:leader), post)
PostActionCreator.spam(Fabricate(:leader, refresh_auto_groups: true), post)

post.reload

Expand All @@ -605,7 +607,7 @@ def value_for(user_id, dt)
create_post(topic: post1.topic)
result =
PostActionCreator.new(
Fabricate(:user),
Fabricate(:user, refresh_auto_groups: true),
post1,
PostActionType.types[:spam],
flag_topic: true,
Expand All @@ -618,7 +620,7 @@ def value_for(user_id, dt)
post = create_post
result =
PostActionCreator.new(
Fabricate(:user),
Fabricate(:user, refresh_auto_groups: true),
post,
PostActionType.types[:spam],
flag_topic: true,
Expand Down Expand Up @@ -649,8 +651,8 @@ def value_for(user_id, dt)
let(:post2) { create_post(topic: topic) }
let(:post3) { create_post(topic: topic) }

fab!(:flagger1) { Fabricate(:user) }
fab!(:flagger2) { Fabricate(:user) }
fab!(:flagger1) { Fabricate(:user, refresh_auto_groups: true) }
fab!(:flagger2) { Fabricate(:user, refresh_auto_groups: true) }

before do
SiteSetting.hide_post_sensitivity = Reviewable.sensitivities[:disabled]
Expand Down Expand Up @@ -831,7 +833,7 @@ def value_for(user_id, dt)

it "should create a notification in the related topic" do
Jobs.run_immediately!
user = Fabricate(:user)
user = Fabricate(:user, refresh_auto_groups: true)
stub_image_size
result = PostActionCreator.create(user, post, :spam, message: "WAT")
topic = result.post_action.related_post.topic
Expand Down
2 changes: 1 addition & 1 deletion spec/models/post_spec.rb
@@ -1,7 +1,7 @@
# frozen_string_literal: true

RSpec.describe Post do
fab!(:coding_horror)
fab!(:coding_horror) { Fabricate(:coding_horror, refresh_auto_groups: true) }

let(:upload_path) { Discourse.store.upload_path }

Expand Down