From 57b460737c90af7a9f445b3e5bd4e8cbc09aa298 Mon Sep 17 00:00:00 2001 From: Gabriel Grubba <70247653+Grubba27@users.noreply.github.com> Date: Tue, 13 Aug 2024 15:14:35 -0300 Subject: [PATCH] FEATURE: add topic tags changed trigger to chat integration (#208) * FEATURE: add topic tags changed trigger to chat integration * FEATURE: add placeholder for reply to topic trigger add description on how to use the placeholder * DEV: Move slack message creation to provider Add tests to new method * FEATURE: add ${URL} to placeholder replacements and added tags link If triggered when a topic tag is changed, message behavior will follow what user defined in message. * DEV: Update tests with tags * DEV: add post to topic for testing * DEV: update test strings * DEV: add early return for topic tags changed trigger * DEV: move early return to use try/catch * DEV: update `create_slack_message` to not send a tuple of values * DEV: refactor method to be more readable * FEATURE: add `${ADDED_AND_REMOVED}` for default texts * DEV: Update typo in test * DEV: Add tests to check when if `create_slack_message` raises an error * DEV: Remove the `tag_added` from chat-integration filter Added migration to handle the migration of the `tag_added` filter from the chat-integration plugin. Only removed the logic from the plugin, data removal will happen in a future PR * DEV: lint migration file * DEV: update chat-integration to not show "tag_added" rules * DEV: update added and missing tags logic * DEV: update context variable name * DEV: update migration to include `begin/rescue` block and added a list with available filters --- admin/assets/javascripts/admin/models/rule.js | 7 +- ...admin-plugins-chat-integration-provider.js | 15 +- app/services/manager.rb | 35 +---- config/locales/client.en.yml | 7 +- config/locales/server.en.yml | 7 +- ...ate_tag_added_from_filter_to_automation.rb | 92 ++++++++++++ .../provider/slack/slack_provider.rb | 125 ++++++++++++++++ plugin.rb | 56 ++----- .../provider/slack/slack_provider_spec.rb | 137 ++++++++++++++++++ spec/services/manager_spec.rb | 76 ---------- 10 files changed, 394 insertions(+), 163 deletions(-) create mode 100644 db/migrate/20240808175526_migrate_tag_added_from_filter_to_automation.rb diff --git a/admin/assets/javascripts/admin/models/rule.js b/admin/assets/javascripts/admin/models/rule.js index 4262ed3..34890dc 100644 --- a/admin/assets/javascripts/admin/models/rule.js +++ b/admin/assets/javascripts/admin/models/rule.js @@ -23,6 +23,8 @@ export default class Rule extends RestModel { }, ]; + possible_filters_id = ["thread", "watch", "follow", "mute"]; + get available_filters() { const available = []; const provider = this.channel.provider; @@ -46,11 +48,6 @@ export default class Rule extends RestModel { name: I18n.t("chat_integration.filter.follow"), icon: "circle", }, - { - id: "tag_added", - name: I18n.t("chat_integration.filter.tag_added"), - icon: "tag", - }, { id: "mute", name: I18n.t("chat_integration.filter.mute"), diff --git a/admin/assets/javascripts/admin/routes/admin-plugins-chat-integration-provider.js b/admin/assets/javascripts/admin/routes/admin-plugins-chat-integration-provider.js index dce0a5a..d144656 100644 --- a/admin/assets/javascripts/admin/routes/admin-plugins-chat-integration-provider.js +++ b/admin/assets/javascripts/admin/routes/admin-plugins-chat-integration-provider.js @@ -1,4 +1,5 @@ import { action } from "@ember/object"; +import { getOwner } from "@ember/owner"; import Group from "discourse/models/group"; import DiscourseRoute from "discourse/routes/discourse"; @@ -13,14 +14,18 @@ export default class AdminPluginsChatIntegrationProvider extends DiscourseRoute Group.findAll(), ]); + const enabledFilters = + getOwner(this).lookup("model:rule").possible_filters_id; channels.forEach((channel) => { channel.set( "rules", - channel.rules.map((rule) => { - rule = this.store.createRecord("rule", rule); - rule.set("channel", channel); - return rule; - }) + channel.rules + .filter((rule) => enabledFilters.includes(rule.filter)) + .map((rule) => { + rule = this.store.createRecord("rule", rule); + rule.set("channel", channel); + return rule; + }) ); }); diff --git a/app/services/manager.rb b/app/services/manager.rb index 590a25f..e19d4a5 100644 --- a/app/services/manager.rb +++ b/app/services/manager.rb @@ -15,11 +15,11 @@ module DiscourseChatIntegration # Abort if the post is blank return if post.blank? - # Abort if post is not either regular, or a 'tags_changed'/'category_changed' small action + # Abort if post is not either regular or a 'category_changed' small action if (post.post_type != Post.types[:regular]) && !( post.post_type == Post.types[:small_action] && - %w[tags_changed category_changed].include?(post.action_code) + %w[category_changed].include?(post.action_code) ) return end @@ -55,34 +55,7 @@ module DiscourseChatIntegration end end - if post.action_code == "tags_changed" - # Post is a small_action post regarding tags changing for the topic. Check if any tags were _added_ - # and if so, corresponding rules with `filter: tag_added` - tags_added = post.custom_fields["tags_added"] - tags_added = [tags_added].compact if !tags_added.is_a?(Array) - return if tags_added.blank? - - tags_removed = post.custom_fields["tags_removed"] - tags_removed = [tags_removed].compact if !tags_removed.is_a?(Array) - - unchanged_tags = topic.tags.map(&:name) - tags_added - tags_removed - - matching_rules = - matching_rules.select do |rule| - # Only rules that match this post, are ones where the filter is "tag_added" - next false if rule.filter != "tag_added" - next true if rule.tags.blank? - - # Skip if the topic already has one of the tags in the rule, applied - next false if unchanged_tags.any? && (unchanged_tags & rule.tags).any? - - # We don't need to do any additional filtering here because topics are filtered - # by tag later - true - end - else - matching_rules = matching_rules.select { |rule| rule.filter != "tag_added" } - end + matching_rules = matching_rules.select { |rule| rule.filter != "tag_added" } # ignore tag_added rules, now uses Automation # If tagging is enabled, thow away rules that don't apply to this topic if SiteSetting.tagging_enabled @@ -97,7 +70,7 @@ module DiscourseChatIntegration # Sort by order of precedence t_prec = { "group_message" => 0, "group_mention" => 1, "normal" => 2 } # Group things win - f_prec = { "mute" => 0, "thread" => 1, "watch" => 2, "follow" => 3, "tag_added" => 4 } #(mute always wins; thread beats watch beats follow) + f_prec = { "mute" => 0, "thread" => 1, "watch" => 2, "follow" => 3 } #(mute always wins; thread beats watch beats follow) sort_func = proc { |a, b| [t_prec[a.type], f_prec[a.filter]] <=> [t_prec[b.type], f_prec[b.filter]] } matching_rules = matching_rules.sort(&sort_func) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 007549d..98b1ee9 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -37,7 +37,6 @@ en: mute: 'Mute' follow: 'First post only' watch: 'All posts and replies' - tag_added: 'Tag added to topic' thread: 'All posts with threaded replies' rule_table: filter: "Filter" @@ -256,7 +255,11 @@ en: title: Send Slack message fields: message: - label: Message + label: Message. + description: >- + Use ${TOPIC} for topic name, ${URL} for used URL, ${REMOVED_TAGS} for removed tags, ${ADDED_TAGS} for added tags, + ${ADDED_AND_REMOVED} for default text. + only available if trigger is set to Topic tags changed. url: label: URL channel: diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index bd3f165..815ac0a 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -33,7 +33,7 @@ en: chat_integration_discord_enabled: "Enable the Discord chat-integration provider" chat_integration_discord_message_content: "The message to include above the summary when sending a notification to Discord" chat_integration_discord_excerpt_length: "Discord post excerpt length" - + ####################################### ########## GUILDED SETTINGS ########### ####################################### @@ -187,6 +187,11 @@ en: error_users: "Error: unable to fetch users from Slack" error_history: "Error: unable to fetch channel history from Slack" error_ts: "Error: unable to fetch requested message from Slack" + messaging: + topic_tag_changed: + added_and_removed: "Added %{added} and removed %{removed}" + added: "Added %{added}" + removed: "Removed %{removed}" ####################################### ########## TELEGRAM STRINGS ########### diff --git a/db/migrate/20240808175526_migrate_tag_added_from_filter_to_automation.rb b/db/migrate/20240808175526_migrate_tag_added_from_filter_to_automation.rb new file mode 100644 index 0000000..72a00d6 --- /dev/null +++ b/db/migrate/20240808175526_migrate_tag_added_from_filter_to_automation.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true +class MigrateTagAddedFromFilterToAutomation < ActiveRecord::Migration[7.1] + def up + if defined?(DiscourseAutomation) && + DiscourseChatIntegration::Channel.with_provider("slack").exists? + begin + DiscourseChatIntegration::Rule + .where("value::json->>'filter'=?", "tag_added") + .each do |rule| + channel_id = rule.value["channel_id"] + channel_name = + DiscourseChatIntegration::Channel.find(channel_id).value["data"]["identifier"] # it _must_ have a channel_id + + category_id = rule.value["category_id"] + tags = rule.value["tags"] + + automation = + DiscourseAutomation::Automation.new( + script: "send_slack_message", + trigger: "topic_tags_changed", + name: "When tags change in topic", + enabled: true, + last_updated_by_id: Discourse.system_user.id, + ) + + automation.save! + + # Triggers: + # Watching categories + + metadata = (category_id ? { "value" => [category_id] } : {}) + + automation.upsert_field!( + "watching_categories", + "categories", + metadata, + target: "trigger", + ) + + # Watching tags + + metadata = (tags ? { "value" => tags } : {}) + + automation.upsert_field!("watching_tags", "tags", metadata, target: "trigger") + + # Script options: + # Message + automation.upsert_field!( + "message", + "message", + { "value" => "${ADDED_AND_REMOVED}" }, + target: "script", + ) + + # URL + automation.upsert_field!( + "url", + "text", + { "value" => Discourse.current_hostname }, + target: "script", + ) + + # Channel + automation.upsert_field!( + "channel", + "text", + { "value" => channel_name }, + target: "script", + ) + end + rescue StandardError + Rails.logger.warn("Failed to migrate tag_added rules to automations") + end + end + end + + def down + if defined?(DiscourseAutomation) && + DiscourseChatIntegration::Channel.with_provider("slack").exists? + DiscourseAutomation::Automation + .where(script: "send_slack_message", trigger: "topic_tags_changed") + .each do |automation| + # if is the same name as created and message is the same + if automation.name == "When tags change in topic" && + automation.fields.where(name: "message").first.metadata["value"] == + "${ADDED_AND_REMOVED}" + automation.destroy! + end + end + end + end +end diff --git a/lib/discourse_chat_integration/provider/slack/slack_provider.rb b/lib/discourse_chat_integration/provider/slack/slack_provider.rb index b060630..d7e4c01 100644 --- a/lib/discourse_chat_integration/provider/slack/slack_provider.rb +++ b/lib/discourse_chat_integration/provider/slack/slack_provider.rb @@ -87,6 +87,74 @@ module DiscourseChatIntegration::Provider::SlackProvider message end + def self.create_slack_message(context:, content:, url:, channel_name:) + sender = ::DiscourseChatIntegration::Helper.formatted_display_name(Discourse.system_user) + + content = replace_placehoders(content, context) if context["kind"] == + DiscourseAutomation::Triggers::TOPIC_TAGS_CHANGED + + full_content = + if context["kind"] == DiscourseAutomation::Triggers::TOPIC_TAGS_CHANGED + content + else + "#{content} - #{url}" + end + + icon_url = + if SiteSetting.chat_integration_slack_icon_url.present? + "#{Discourse.base_url}#{SiteSetting.chat_integration_slack_icon_url}" + elsif (url = (SiteSetting.try(:site_logo_small_url) || SiteSetting.logo_small_url)).present? + "#{Discourse.base_url}#{url}" + end + + slack_username = + if SiteSetting.chat_integration_slack_username.present? + SiteSetting.chat_integration_slack_username + else + SiteSetting.title || "Discourse" + end + + message = { + channel: "##{channel_name}", + username: slack_username, + icon_url: icon_url, + attachments: [], + } + + summary = { + fallback: content.truncate(100), + author_name: sender, + color: nil, + text: full_content, + mrkdwn_in: ["text"], + title: content.truncate(100), + title_link: url, + thumb_url: nil, + } + + if context["kind"] == DiscourseAutomation::Triggers::TOPIC_TAGS_CHANGED + topic = context["topic"] + category = + if topic.category&.uncategorized? + "[#{I18n.t("uncategorized_category_name")}]" + elsif topic.category + if (topic.category.parent_category) + "[#{topic.category.parent_category.name}/#{topic.category.name}]" + else + "[#{topic.category.name}]" + end + end + summary[:title_link] = topic.posts.first.full_url + summary[ + :title + ] = "#{topic.title} #{category} #{topic.tags.present? ? topic.tags.map(&:name).join(", ") : ""}" + summary[:thumb_url] + end + + message[:attachments].push(summary) + message + end + def self.send_via_api(post, channel, message) http = slack_api_http @@ -214,6 +282,63 @@ module DiscourseChatIntegration::Provider::SlackProvider unique_by: :index_topic_custom_fields_on_topic_id_and_slack_thread_id, ) end + + def self.replace_placehoders(content, context) + if context["topic"] && content.include?("${TOPIC}") + topic = context["topic"] + content = content.gsub("${TOPIC}", topic.title) + end + + if content.include?("${REMOVED_TAGS}") + if context["removed_tags"].empty? + raise StandardError.new "No tags but content includes reference." + end + removed_tags_names = create_tag_list(context["removed_tags"]) + content = content.gsub("${REMOVED_TAGS}", removed_tags_names) + end + + if content.include?("${ADDED_TAGS}") + if context["added_tags"].empty? + raise StandardError.new "No tags but content includes reference." + end + added_tags_names = create_tag_list(context["added_tags"]) + content = content.gsub("${ADDED_TAGS}", added_tags_names) + end + + if content.include?("${ADDED_AND_REMOVED}") + added_tags = context["added_tags"] + missing_tags = context["removed_tags"] + + text = + if !added_tags.empty? && !missing_tags.empty? + I18n.t( + "chat_integration.provider.slack.messaging.topic_tag_changed.added_and_removed", + added: create_tag_list(added_tags), + removed: create_tag_list(missing_tags), + ) + elsif !added_tags.empty? + I18n.t( + "chat_integration.provider.slack.messaging.topic_tag_changed.added", + added: create_tag_list(added_tags), + ) + elsif !missing_tags.empty? + I18n.t( + "chat_integration.provider.slack.messaging.topic_tag_changed.removed", + removed: create_tag_list(missing_tags), + ) + end + + content = content.gsub("${ADDED_AND_REMOVED}", text) + end + + content = content.gsub("${URL}", url) if content.include?("${URL}") + + content + end + + def self.create_tag_list(tag_list) + tag_list.map { |tag_name| "<#{Tag.find_by_name(tag_name).full_url}|#{tag_name}>" }.join(", ") + end end require_relative "slack_message_formatter" diff --git a/plugin.rb b/plugin.rb index 41fb91d..3d846f6 100644 --- a/plugin.rb +++ b/plugin.rb @@ -58,14 +58,9 @@ after_initialize do version 1 - triggerables %i[point_in_time recurring] + triggerables %i[point_in_time recurring topic_tags_changed] script do |context, fields, automation| - sender = Discourse.system_user - - content = fields.dig("message", "value") - url = fields.dig("url", "value") - full_content = "#{content} - #{url}" channel_name = fields.dig("channel", "value") channel = DiscourseChatIntegration::Channel.new( @@ -75,43 +70,18 @@ after_initialize do }, ) - icon_url = - if SiteSetting.chat_integration_slack_icon_url.present? - "#{Discourse.base_url}#{SiteSetting.chat_integration_slack_icon_url}" - elsif ( - url = (SiteSetting.try(:site_logo_small_url) || SiteSetting.logo_small_url) - ).present? - "#{Discourse.base_url}#{url}" - end - - slack_username = - if SiteSetting.chat_integration_slack_username.present? - SiteSetting.chat_integration_slack_username - else - SiteSetting.title || "Discourse" - end - - message = { - channel: "##{channel_name}", - username: slack_username, - icon_url: icon_url, - attachments: [], - } - - summary = { - fallback: content.truncate(100), - author_name: sender, - color: nil, - text: full_content, - mrkdwn_in: ["text"], - title: content.truncate(100), - title_link: url, - thumb_url: nil, - } - - message[:attachments].push(summary) - - DiscourseChatIntegration::Provider::SlackProvider.send_via_api(nil, channel, message) + begin + message = + DiscourseChatIntegration::Provider::SlackProvider.create_slack_message( + context: context, + content: fields.dig("message", "value"), + url: fields.dig("url", "value"), + channel_name: channel_name, + ) + DiscourseChatIntegration::Provider::SlackProvider.send_via_api(nil, channel, message) + rescue StandardError => _ + # StandardError here is when there are no tags but content includes reference to them. + end end end end diff --git a/spec/lib/discourse_chat_integration/provider/slack/slack_provider_spec.rb b/spec/lib/discourse_chat_integration/provider/slack/slack_provider_spec.rb index 04e2fc4..9324585 100644 --- a/spec/lib/discourse_chat_integration/provider/slack/slack_provider_spec.rb +++ b/spec/lib/discourse_chat_integration/provider/slack/slack_provider_spec.rb @@ -209,4 +209,141 @@ RSpec.describe DiscourseChatIntegration::Provider::SlackProvider do end end end + + describe ".create_slack_message" do + it "should work with a simple message" do + content = "Simple message" + url = "http://example.com" + message = { channel: "#general", username: "Discourse", content: "#{content} - #{url}" } + result = + described_class.create_slack_message( + context: { + }, + content: content, + channel_name: "general", + url: url, + ) + expect( + { + channel: result[:channel], + username: result[:username], + content: result[:attachments][0][:text], + }, + ).to eq(message) + end + + it "should do the replacements" do + topic = Fabricate(:topic) + topic.posts << Fabricate(:post, topic: topic) + tag1, tag2, tag3, tag4 = [Fabricate(:tag), Fabricate(:tag), Fabricate(:tag), Fabricate(:tag)] + content = + "The topic title is: ${TOPIC} + removed tags: ${REMOVED_TAGS} + added tags: ${ADDED_TAGS}" + + result = + described_class.create_slack_message( + context: { + "topic" => topic, + "removed_tags" => [tag1.name, tag2.name], + "added_tags" => [tag3.name, tag4.name], + "kind" => DiscourseAutomation::Triggers::TOPIC_TAGS_CHANGED, + }, + content: content, + channel_name: "general", + url: "http://example.com", + ) + text = result[:attachments][0][:text] + expect(text).to include(topic.title) + expect(text).to include("<#{tag1.full_url}|#{tag1.name}>, <#{tag2.full_url}|#{tag2.name}>") + expect(text).to include("<#{tag3.full_url}|#{tag3.name}>, <#{tag4.full_url}|#{tag4.name}>") + end + + it "should do the replacements for ${ADDED_AND_REMOVED}" do + topic = Fabricate(:topic) + topic.posts << Fabricate(:post, topic: topic) + tag1, tag2 = [Fabricate(:tag), Fabricate(:tag)] + content = "${ADDED_AND_REMOVED}" + result = + described_class.create_slack_message( + context: { + "topic" => topic, + "added_tags" => [tag2.name], + "removed_tags" => [tag1.name], + "kind" => DiscourseAutomation::Triggers::TOPIC_TAGS_CHANGED, + }, + content: content, + channel_name: "general", + url: "http://example.com", + ) + text = result[:attachments][0][:text] + expect(text).to include( + I18n.t( + "chat_integration.provider.slack.messaging.topic_tag_changed.added_and_removed", + added: "<#{tag2.full_url}|#{tag2.name}>", + removed: "<#{tag1.full_url}|#{tag1.name}>", + ), + ) + + result = + described_class.create_slack_message( + context: { + "topic" => topic, + "added_tags" => [], + "removed_tags" => [tag1.name], + "kind" => DiscourseAutomation::Triggers::TOPIC_TAGS_CHANGED, + }, + content: content, + channel_name: "general", + url: "http://example.com", + ) + text = result[:attachments][0][:text] + expect(text).to include( + I18n.t( + "chat_integration.provider.slack.messaging.topic_tag_changed.removed", + removed: "<#{tag1.full_url}|#{tag1.name}>", + ), + ) + + result = + described_class.create_slack_message( + context: { + "topic" => topic, + "added_tags" => [tag2.name], + "removed_tags" => [], + "kind" => DiscourseAutomation::Triggers::TOPIC_TAGS_CHANGED, + }, + content: content, + channel_name: "general", + url: "http://example.com", + ) + text = result[:attachments][0][:text] + expect(text).to include( + I18n.t( + "chat_integration.provider.slack.messaging.topic_tag_changed.added", + added: "<#{tag2.full_url}|#{tag2.name}>", + ), + ) + end + + it "should raise errors if tags are not present but uses in content" do + topic = Fabricate(:topic) + topic.posts << Fabricate(:post, topic: topic) + content = "This should not work ${ADDED_TAGS}" + + expect { + described_class.create_slack_message( + context: { + "topic" => topic, + "kind" => DiscourseAutomation::Triggers::TOPIC_TAGS_CHANGED, + "added_tags" => [], + "removed_tags" => [], + }, + content: content, + channel_name: "general", + url: "http://example.com", + ) + }.to raise_error(StandardError) + end + end end diff --git a/spec/services/manager_spec.rb b/spec/services/manager_spec.rb index 878a84c..3143677 100644 --- a/spec/services/manager_spec.rb +++ b/spec/services/manager_spec.rb @@ -377,82 +377,6 @@ RSpec.describe DiscourseChatIntegration::Manager do expect(provider.sent_to_channel_ids).to contain_exactly(chan1.id) end - - describe "with create_small_action_post_for_tag_changes enabled" do - fab!(:admin) { Fabricate(:admin, refresh_auto_groups: true) } - fab!(:additional_tag) { Fabricate(:tag) } - - before { SiteSetting.create_post_for_category_and_tag_changes = true } - - def set_new_tags_and_return_small_action_post(tags) - PostRevisor.new(tagged_first_post).revise!(admin, tags: tags) - - tagged_topic.ordered_posts.last - end - - it "should notify when rule is set up for tag additions for a category with no tag filter" do - post = set_new_tags_and_return_small_action_post([tag.name, additional_tag.name]) - - DiscourseChatIntegration::Rule.create!( - channel: chan1, - filter: "tag_added", - category_id: category.id, - ) - - manager.trigger_notifications(post.id) - expect(provider.sent_to_channel_ids).to contain_exactly(chan1.id) - end - - it "notifies when topic has a tag added that matches the rule" do - post = set_new_tags_and_return_small_action_post([tag.name, additional_tag.name]) - - DiscourseChatIntegration::Rule.create!( - channel: chan1, - filter: "tag_added", - category_id: category.id, - tags: [additional_tag.name], - ) - - manager.trigger_notifications(post.id) - expect(provider.sent_to_channel_ids).to contain_exactly(chan1.id) - end - - it "doesn't notify when a new regular post is created" do - DiscourseChatIntegration::Rule.create!( - channel: chan1, - filter: "tag_added", - category_id: nil, - tags: [tag.name], - ) - - post = Fabricate(:post, topic: tagged_topic) - manager.trigger_notifications(post.id) - expect(provider.sent_to_channel_ids).to contain_exactly - end - - it "doesn't notify when topic has an unchanged tag present in the rule, even if a new tag is added" do - post = set_new_tags_and_return_small_action_post([tag.name, additional_tag.name]) - - DiscourseChatIntegration::Rule.create!( - channel: chan1, - filter: "tag_added", - category_id: category.id, - tags: [tag.name], - ) - - manager.trigger_notifications(post.id) - expect(provider.sent_to_channel_ids).to contain_exactly - end - - it "doesn't notify for small action 'tags_changed' posts unless a matching rule exists" do - post = set_new_tags_and_return_small_action_post([additional_tag.name]) - - DiscourseChatIntegration::Rule.create!(channel: chan1, filter: "watch", category_id: nil) # Wildcard watch - - manager.trigger_notifications(post.id) - expect(provider.sent_to_channel_ids).to contain_exactly - end - end end end end