discourse
/
discourse-assign
mirror of https://github.com/discourse/discourse-assign.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FIX: do not self-assign based on quoted text 

Remove any quoted text and code blocks before matching the "assign self" or "assign other" regexps.

Added tests to ensure it does not regress.

Also cleaned up some TODOs.
This commit is contained in:
Régis Hanol 2019-10-29 19:00:39 +01:00
parent b3c2b83ad8
commit 9c2ebbaaf5
2 changed files with 89 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

97
lib/topic_assigner.rb
View File

@ -1,34 +1,38 @@
# frozen_string_literal: true # frozen_string_literal: true
require_dependency 'email/sender' require 'email/sender'
require 'nokogiri'
class ::TopicAssigner class ::TopicAssigner
ASSIGNED_TO_ID = 'assigned_to_id' ASSIGNED_TO_ID ||= 'assigned_to_id'
ASSIGNED_BY_ID = 'assigned_by_id' ASSIGNED_BY_ID ||= 'assigned_by_id'
def self.backfill_auto_assign def self.backfill_auto_assign
staff_mention = User.assign_allowed staff_mention = User
.assign_allowed
.pluck('username') .pluck('username')
.map! { |name| "p.cooked ILIKE '%mention%@#{name}%'" } .map { |name| "p.cooked ILIKE '%mention%@#{name}%'" }
.join(' OR ') .join(' OR ')
sql = <<~SQL sql = <<~SQL
SELECT p.topic_id, MAX(post_number) post_number SELECT p.topic_id, MAX(post_number) post_number
FROM posts p FROM posts p
JOIN topics t ON t.id = p.topic_id JOIN topics t ON t.id = p.topic_id
LEFT JOIN topic_custom_fields tc ON tc.name = '#{ASSIGNED_TO_ID}' AND tc.topic_id = p.topic_id LEFT JOIN topic_custom_fields tc ON tc.name = '#{ASSIGNED_TO_ID}' AND tc.topic_id = p.topic_id
WHERE p.user_id IN (SELECT id FROM users WHERE moderator OR admin) AND WHERE p.user_id IN (SELECT id FROM users WHERE moderator OR admin)
( #{staff_mention} ) AND tc.value IS NULL AND NOT t.closed AND t.deleted_at IS NULL AND (#{staff_mention})
GROUP BY p.topic_id AND tc.value IS NULL
AND NOT t.closed
AND t.deleted_at IS NULL
GROUP BY p.topic_id
SQL SQL
assigned = 0
puts puts
assigned = 0
ActiveRecord::Base.connection.raw_connection.exec(sql).to_a.each do |row| ActiveRecord::Base.connection.raw_connection.exec(sql).to_a.each do |row|
post = Post.find_by(post_number: row["post_number"].to_i, post = Post.find_by(post_number: row["post_number"].to_i, topic_id: row["topic_id"].to_i)
topic_id: row["topic_id"].to_i)
assigned += 1 if post && auto_assign(post) assigned += 1 if post && auto_assign(post)
putc "." putc "."
end end
@ -37,29 +41,31 @@ class ::TopicAssigner
puts "#{assigned} topics where automatically assigned to staff members" puts "#{assigned} topics where automatically assigned to staff members"
end end
def self.assign_self_passes?(post) def self.assigned_self?(text)
return false unless SiteSetting.assign_self_regex.present? return false if text.blank? || SiteSetting.assign_self_regex.blank?
regex = Regexp.new(SiteSetting.assign_self_regex) rescue nil regex = Regexp.new(SiteSetting.assign_self_regex) rescue nil
!!(regex && text[regex])
!!(regex && regex.match(post.raw))
end end
def self.assign_other_passes?(post) def self.assigned_other?(text)
return true unless SiteSetting.assign_other_regex.present? return false unless text.blank? || SiteSetting.assign_other_regex.blank?
regex = Regexp.new(SiteSetting.assign_other_regex) rescue nil regex = Regexp.new(SiteSetting.assign_other_regex) rescue nil
!!(regex && text[regex])
!!(regex && regex.match(post.raw))
end end
def self.auto_assign(post, force: false) def self.auto_assign(post, force: false)
return unless SiteSetting.assigns_by_staff_mention return unless SiteSetting.assigns_by_staff_mention
if post.user && post.topic && post.user.can_assign? if post.user && post.topic && post.user.can_assign?
can_assign = force || post.topic.custom_fields[ASSIGNED_TO_ID].nil? return unless force || post.topic.custom_fields[ASSIGNED_TO_ID].nil?
return unless can_assign
assign_other = assign_other_passes?(post) && mentioned_staff(post) # remove quotes, oneboxes and code blocks
assign_self = assign_self_passes?(post) && post.user doc = Nokogiri::HTML.fragment(post.cooked)
doc.css(".quote, .onebox, pre, code").remove
text = doc.text.strip
assign_other = assigned_other?(text) && mentioned_staff(post)
assign_self = assigned_self?(text) && post.user
return unless assign_other || assign_self return unless assign_other || assign_self
if is_last_staff_post?(post) if is_last_staff_post?(post)
@ -77,11 +83,13 @@ class ::TopicAssigner
allowed_user_ids = User.assign_allowed.pluck(:id).join(',') allowed_user_ids = User.assign_allowed.pluck(:id).join(',')
sql = <<~SQL sql = <<~SQL
SELECT 1 FROM posts p SELECT 1
JOIN users u ON u.id = p.user_id FROM posts p
WHERE p.deleted_at IS NULL AND p.topic_id = :topic_id JOIN users u ON u.id = p.user_id
AND u.id IN (#{allowed_user_ids}) WHERE p.deleted_at IS NULL
having max(post_number) = :post_number AND p.topic_id = :topic_id
AND u.id IN (#{allowed_user_ids})
HAVING MAX(post_number) = :post_number
SQL SQL
args = { args = {
@ -226,10 +234,8 @@ class ::TopicAssigner
# we got to send a push notification as well # we got to send a push notification as well
# what we created here is a whisper and notification will not raise a push # what we created here is a whisper and notification will not raise a push
alerter = PostAlerter.new(first_post) if @assigned_by.id != assign_to.id
# TODO: remove June 2019 PostAlerter.new(first_post).create_notification_alert(
if alerter.respond_to?(:create_notification_alert) && @assigned_by.id != assign_to.id
alerter.create_notification_alert(
user: assign_to, user: assign_to,
post: first_post, post: first_post,
username: @assigned_by.username, username: @assigned_by.username,
@ -250,11 +256,10 @@ class ::TopicAssigner
# TODO core needs an API for this stuff badly # TODO core needs an API for this stuff badly
# currently there is no 100% surefire way of deleting a custom field # currently there is no 100% surefire way of deleting a custom field
TopicCustomField.where( TopicCustomField
topic_id: @topic.id .where(topic_id: @topic.id)
).where( .where('name in (?)', [ASSIGNED_BY_ID, ASSIGNED_TO_ID])
'name in (?)', [ASSIGNED_BY_ID, ASSIGNED_TO_ID] .destroy_all
).destroy_all
if Array === assigned_to_id if Array === assigned_to_id
# more custom field mess, try to recover # more custom field mess, try to recover
@ -265,10 +270,8 @@ class ::TopicAssigner
@topic.custom_fields[ASSIGNED_TO_ID] = nil @topic.custom_fields[ASSIGNED_TO_ID] = nil
@topic.custom_fields[ASSIGNED_BY_ID] = nil @topic.custom_fields[ASSIGNED_BY_ID] = nil
if !assigned_to_id # nothing to do here
# nothing to do here return if !assigned_to_id
return
end
post = @topic.posts.where(post_number: 1).first post = @topic.posts.where(post_number: 1).first
return unless post.present? return unless post.present?
@ -290,7 +293,6 @@ class ::TopicAssigner
) )
end end
assigned_user = User.find_by(id: assigned_to_id)
MessageBus.publish( MessageBus.publish(
"/staff/topic-assignment", "/staff/topic-assignment",
{ {
@ -300,6 +302,7 @@ class ::TopicAssigner
user_ids: allowed_user_ids user_ids: allowed_user_ids
) )
assigned_user = User.find_by(id: assigned_to_id)
publish_topic_tracking_state(@topic, assigned_user.id) publish_topic_tracking_state(@topic, assigned_user.id)
UserAction.where( UserAction.where(
@ -310,7 +313,7 @@ class ::TopicAssigner
# yank notification # yank notification
Notification.where( Notification.where(
notification_type: Notification.types[:custom], notification_type: Notification.types[:custom],
user_id: assigned_user.try(:id), user_id: assigned_user.id,
topic_id: @topic.id, topic_id: @topic.id,
post_number: 1 post_number: 1
).where("data like '%discourse_assign.assign_notification%'").destroy_all ).where("data like '%discourse_assign.assign_notification%'").destroy_all
@ -321,7 +324,7 @@ class ::TopicAssigner
@assigned_by, nil, @assigned_by, nil,
bump: false, bump: false,
post_type: post_type, post_type: post_type,
custom_fields: { "action_code_who" => assigned_user&.username }, custom_fields: { "action_code_who" => assigned_user.username },
action_code: "unassigned" action_code: "unassigned"
) )
end end

39
spec/lib/topic_assigner_spec.rb
View File

@ -198,6 +198,45 @@ RSpec.describe TopicAssigner do
end end
end end
context "assign_self_regex" do
fab!(:me) { Fabricate(:admin) }
fab!(:op) { Fabricate(:post) }
fab!(:reply) { Fabricate(:post, topic: op.topic, user: me, raw: "Will fix. Added to my list ;)") }
before do
SiteSetting.assign_enabled = true
SiteSetting.assigns_by_staff_mention = true
SiteSetting.assign_self_regex = "\\bmy list\\b"
end
it "automatically assigns to myself" do
expect(TopicAssigner.auto_assign(reply)).to eq(success: true)
expect(op.topic.custom_fields).to eq("assigned_to_id" => me.id.to_s, "assigned_by_id" => me.id.to_s)
end
it "does not automatically assign to myself" do
admin = Fabricate(:admin)
raw = <<~MD
[quote]
Will fix. Added to my list ;)
[/quote]
`my list`
```text
my list
```
my list
Excellent :clap: Can't wait!
MD
another_reply = Fabricate(:post, topic: op.topic, user: admin, raw: raw)
expect(TopicAssigner.auto_assign(another_reply)).to eq(nil)
end
end
context "unassign_on_close" do context "unassign_on_close" do
let(:post) { Fabricate(:post) } let(:post) { Fabricate(:post) }
let(:topic) { post.topic } let(:topic) { post.topic }