mirror of
https://github.com/discourse/discourse.git
synced 2024-11-29 20:47:07 +08:00
e74560f062
Currently, if the review queue has both a flagged post and a flagged chat message, one of the two will have some of the labels of their actions replaced by those of the other. In other words, the labels are getting mixed up. For example, a flagged chat message might show up with an action labelled "Delete post". This is happening because when using bundles, we are sending along the actions in a separate part of the response, so they can be shared by many reviewables. The bundles then index into this bag of actions by their ID, which is something generic describing the server action, e.g. "agree_and_delete". The problem here is the same action can have different labels depending on the type of reviewable. Now that the bag of actions contains multiple actions with the same ID, which one is chosen is arbitrary. I.e. it doesn't distinguish based on the type of the reviewable. This change adds an additional field to the actions, server_action, which now contains what used to be the ID. Meanwhile, the ID has been turned into a concatenation of the reviewable type and the server action, e.g. post-agree_and_delete. This still provides the upside of denormalizing the actions while allowing for different reviewable types to have different labels and descriptions. At first I thought I would prepend the reviewable type to the ID, but this doesn't work well because the ID is used on the server-side to determine which actions are possible, and these need to be shared between different reviewables. Hence the introduction of server_action, which now serves that purpose. I also thought about changing the way that the bundle indexes into the bag of actions, but this is happening through some EmberJS mechanism, so we don't own that code.
258 lines
8.5 KiB
Ruby
258 lines
8.5 KiB
Ruby
# frozen_string_literal: true
|
|
|
|
RSpec.describe ReviewableUser, type: :model do
|
|
fab!(:moderator) { Fabricate(:moderator) }
|
|
let(:user) do
|
|
user = Fabricate(:user)
|
|
user.activate
|
|
user
|
|
end
|
|
fab!(:admin) { Fabricate(:admin) }
|
|
|
|
describe "#actions_for" do
|
|
fab!(:reviewable) { Fabricate(:reviewable) }
|
|
|
|
it "returns correct actions in the pending state" do
|
|
actions = reviewable.actions_for(Guardian.new(moderator))
|
|
expect(actions.has?(:approve_user)).to eq(true)
|
|
expect(actions.has?(:delete_user)).to eq(true)
|
|
expect(actions.has?(:delete_user_block)).to eq(true)
|
|
end
|
|
|
|
it "doesn't return anything in the approved state" do
|
|
reviewable.status = Reviewable.statuses[:approved]
|
|
actions = reviewable.actions_for(Guardian.new(moderator))
|
|
expect(actions.has?(:approve_user)).to eq(false)
|
|
expect(actions.has?(:delete_user_block)).to eq(false)
|
|
end
|
|
|
|
it "can delete a user without a giving a rejection reason if the user was a spammer" do
|
|
reviewable.reviewable_scores.build(user: admin, reason: "suspect_user")
|
|
|
|
assert_require_reject_reason(:delete_user, false)
|
|
end
|
|
|
|
it "requires a rejection reason to delete a user" do
|
|
assert_require_reject_reason(:delete_user, true)
|
|
end
|
|
|
|
it "can delete and block a user without giving a rejection reason if the user was a spammer" do
|
|
reviewable.reviewable_scores.build(user: admin, reason: "suspect_user")
|
|
|
|
assert_require_reject_reason(:delete_user, false)
|
|
end
|
|
|
|
it "requires a rejection reason to delete and block a user" do
|
|
assert_require_reject_reason(:delete_user_block, true)
|
|
end
|
|
|
|
def assert_require_reject_reason(id, expected)
|
|
actions = reviewable.actions_for(Guardian.new(moderator))
|
|
|
|
expect(actions.to_a.find { |a| a.server_action.to_sym == id }.require_reject_reason).to eq(
|
|
expected,
|
|
)
|
|
end
|
|
end
|
|
|
|
describe "#update_fields" do
|
|
fab!(:moderator) { Fabricate(:moderator) }
|
|
fab!(:reviewable) { Fabricate(:reviewable) }
|
|
|
|
it "doesn't raise errors with an empty update" do
|
|
expect(reviewable.update_fields(nil, moderator)).to eq(true)
|
|
expect(reviewable.update_fields({}, moderator)).to eq(true)
|
|
end
|
|
end
|
|
|
|
context "when a user is deleted" do
|
|
it "should reject the reviewable" do
|
|
SiteSetting.must_approve_users = true
|
|
Jobs::CreateUserReviewable.new.execute(user_id: user.id)
|
|
reviewable = Reviewable.find_by(target: user)
|
|
expect(reviewable.pending?).to eq(true)
|
|
|
|
UserDestroyer.new(Discourse.system_user).destroy(user)
|
|
expect(reviewable.reload.rejected?).to eq(true)
|
|
end
|
|
end
|
|
|
|
describe "#perform" do
|
|
fab!(:reviewable) { Fabricate(:reviewable) }
|
|
|
|
context "when approving" do
|
|
it "allows us to approve a user" do
|
|
result = reviewable.perform(moderator, :approve_user)
|
|
expect(result.success?).to eq(true)
|
|
|
|
expect(reviewable.pending?).to eq(false)
|
|
expect(reviewable.approved?).to eq(true)
|
|
expect(reviewable.target.approved?).to eq(true)
|
|
expect(reviewable.target.approved_by_id).to eq(moderator.id)
|
|
expect(reviewable.target.approved_at).to be_present
|
|
expect(reviewable.version > 0).to eq(true)
|
|
end
|
|
end
|
|
|
|
context "when rejecting" do
|
|
it "allows us to reject a user" do
|
|
result = reviewable.perform(moderator, :delete_user, reject_reason: "reject reason")
|
|
expect(result.success?).to eq(true)
|
|
|
|
expect(reviewable.pending?).to eq(false)
|
|
expect(reviewable.rejected?).to eq(true)
|
|
|
|
# Rejecting deletes the user record
|
|
reviewable.reload
|
|
expect(reviewable.target).to be_blank
|
|
expect(reviewable.reject_reason).to eq("reject reason")
|
|
expect(UserHistory.last.context).to eq(I18n.t("user.destroy_reasons.reviewable_reject"))
|
|
end
|
|
|
|
it "allows us to reject and block a user" do
|
|
email = reviewable.target.email
|
|
ip = reviewable.target.ip_address
|
|
|
|
result = reviewable.perform(moderator, :delete_user_block, reject_reason: "reject reason")
|
|
expect(result.success?).to eq(true)
|
|
|
|
expect(reviewable.pending?).to eq(false)
|
|
expect(reviewable.rejected?).to eq(true)
|
|
|
|
# Rejecting deletes the user record
|
|
reviewable.reload
|
|
expect(reviewable.target).to be_blank
|
|
expect(reviewable.reject_reason).to eq("reject reason")
|
|
|
|
expect(ScreenedEmail.should_block?(email)).to eq(true)
|
|
expect(ScreenedIpAddress.should_block?(ip)).to eq(true)
|
|
end
|
|
|
|
it "is not sending email to the user about rejection" do
|
|
SiteSetting.must_approve_users = true
|
|
Jobs::CriticalUserEmail.any_instance.expects(:execute).never
|
|
reviewable.perform(moderator, :delete_user_block, reject_reason: "reject reason")
|
|
end
|
|
|
|
it "optionally sends email with reject reason" do
|
|
SiteSetting.must_approve_users = true
|
|
Jobs::CriticalUserEmail
|
|
.any_instance
|
|
.expects(:execute)
|
|
.with(
|
|
{
|
|
type: :signup_after_reject,
|
|
user_id: reviewable.target_id,
|
|
reject_reason: "reject reason",
|
|
},
|
|
)
|
|
.once
|
|
reviewable.perform(
|
|
moderator,
|
|
:delete_user_block,
|
|
reject_reason: "reject reason",
|
|
send_email: true,
|
|
)
|
|
end
|
|
|
|
it "allows us to reject a user who has posts" do
|
|
Fabricate(:post, user: reviewable.target)
|
|
result = reviewable.perform(moderator, :delete_user)
|
|
expect(result.success?).to eq(true)
|
|
|
|
expect(reviewable.pending?).to eq(false)
|
|
expect(reviewable.rejected?).to eq(true)
|
|
|
|
# Rejecting deletes the user record
|
|
reviewable.reload
|
|
expect(reviewable.target).to be_present
|
|
expect(reviewable.target.approved).to eq(false)
|
|
end
|
|
|
|
it "allows us to reject a user who has been deleted" do
|
|
reviewable.target.destroy!
|
|
reviewable.reload
|
|
result = reviewable.perform(moderator, :delete_user)
|
|
expect(result.success?).to eq(true)
|
|
expect(reviewable.rejected?).to eq(true)
|
|
expect(reviewable.target).to be_blank
|
|
end
|
|
|
|
it "silently transitions the reviewable if the user is an admin" do
|
|
reviewable.target.update!(admin: true)
|
|
|
|
result = reviewable.perform(moderator, :delete_user)
|
|
expect(reviewable.pending?).to eq(false)
|
|
expect(reviewable.rejected?).to eq(true)
|
|
|
|
reviewable.reload
|
|
expect(reviewable.target).to be_present
|
|
expect(reviewable.target.approved).to eq(false)
|
|
end
|
|
end
|
|
end
|
|
|
|
describe "changing must_approve_users" do
|
|
it "will approve any existing users" do
|
|
user = Fabricate(:user)
|
|
expect(user).not_to be_approved
|
|
SiteSetting.must_approve_users = true
|
|
expect(user.reload).to be_approved
|
|
end
|
|
end
|
|
|
|
describe "when must_approve_users is true" do
|
|
before do
|
|
SiteSetting.must_approve_users = true
|
|
Jobs.run_immediately!
|
|
@reviewable = ReviewableUser.find_by(target: user)
|
|
Jobs.run_later!
|
|
end
|
|
|
|
it "creates the ReviewableUser for a user, with moderator access" do
|
|
expect(@reviewable.reviewable_by_moderator).to eq(true)
|
|
end
|
|
|
|
context "with email jobs" do
|
|
it "enqueues a 'signup after approval' email if must_approve_users is true" do
|
|
expect_enqueued_with(job: :critical_user_email, args: { type: :signup_after_approval }) do
|
|
@reviewable.perform(admin, :approve_user)
|
|
end
|
|
end
|
|
|
|
it "doesn't enqueue a 'signup after approval' email if must_approve_users is false" do
|
|
SiteSetting.must_approve_users = false
|
|
|
|
expect_not_enqueued_with(
|
|
job: :critical_user_email,
|
|
args: {
|
|
type: :signup_after_approval,
|
|
},
|
|
) { @reviewable.perform(admin, :approve_user) }
|
|
end
|
|
end
|
|
|
|
it "triggers a extensibility event" do
|
|
user && admin # bypass the user_created event
|
|
event =
|
|
DiscourseEvent
|
|
.track_events { ReviewableUser.find_by(target: user).perform(admin, :approve_user) }
|
|
.first
|
|
|
|
expect(event[:event_name]).to eq(:user_approved)
|
|
expect(event[:params].first).to eq(user)
|
|
end
|
|
|
|
it "triggers a extensibility event" do
|
|
user && admin # bypass the user_created event
|
|
event =
|
|
DiscourseEvent
|
|
.track_events { ReviewableUser.find_by(target: user).perform(admin, :approve_user) }
|
|
.first
|
|
|
|
expect(event[:event_name]).to eq(:user_approved)
|
|
expect(event[:params].first).to eq(user)
|
|
end
|
|
end
|
|
end
|