FIX: Store slack thread_ts on a per-channel basis (#95)

When notifications about a topic are posted to multiple slack channels, and the Discourse channels are configured to "thread" the notifications, each channel will have a different thread_id. Previously we were only storing a single slack thread id per Discourse topic. This commit fixes that logic, so that threads in different channels are tracked separately.

db/migrate/20211202120030_add_unique_index_to_slack_thread_ts.rb

@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+class AddUniqueIndexToSlackThreadTs < ActiveRecord::Migration[6.1]
+  def up
+    add_index :topic_custom_fields, [:topic_id, :name],
+      unique: true,
+      where: "(name LIKE 'slack_thread_id_%')",
+      name: "index_topic_custom_fields_on_topic_id_and_slack_thread_id"
+  end
+end

lib/discourse_chat_integration/provider/slack/slack_provider.rb

@@ -1,8 +1,15 @@
 # frozen_string_literal: true
 
 module DiscourseChatIntegration::Provider::SlackProvider
-  PROVIDER_NAME = "slack".freeze
+  PROVIDER_NAME = "slack"
-  THREAD = "thread".freeze
+  THREAD_CUSTOM_FIELD_PREFIX = "slack_thread_id_"
+
+  # In the past, only one thread_ts was stored for each topic.
+  # Now, we store one thread_ts per Slack channel.
+  # Data will be automatically migrated when the next message is sent to the channel
+  # This logic could be removed after 2022-12 - it's unlikely people will care about
+  # threading messages to more-than-1-year-old Slack threads.
+  THREAD_LEGACY = "thread"
 
   PROVIDER_ENABLED_SETTING = :chat_integration_slack_enabled
 
@@ -11,16 +18,7 @@ module DiscourseChatIntegration::Provider::SlackProvider
                        ]
 
   require_dependency 'topic'
-  ::Topic.register_custom_field_type(DiscourseChatIntegration::Provider::SlackProvider::THREAD, :text)
+  ::Topic.register_custom_field_type(DiscourseChatIntegration::Provider::SlackProvider::THREAD_LEGACY, :string)
-
-  class ::Topic
-    def slack_thread_id=(ts)
-      self.custom_fields[DiscourseChatIntegration::Provider::SlackProvider::THREAD] = ts
-    end
-    def slack_thread_id
-      self.custom_fields[DiscourseChatIntegration::Provider::SlackProvider::THREAD]
-    end
-  end
 
   def self.excerpt(post, max_length = SiteSetting.chat_integration_slack_excerpt_length)
     doc = Nokogiri::HTML5.fragment(post.excerpt(max_length,
@@ -69,8 +67,8 @@ module DiscourseChatIntegration::Provider::SlackProvider
       attachments: []
     }
 
-    if filter == "thread" && thread_ts = topic.slack_thread_id
+    if filter == "thread" && thread_ts = get_slack_thread_ts(topic, channel)
-      message[:thread_ts] = thread_ts if not thread_ts.nil?
+      message[:thread_ts] = thread_ts
     end
 
     summary = {
@@ -97,7 +95,7 @@ module DiscourseChatIntegration::Provider::SlackProvider
     uri = ""
 
     # <!--SLACK_CHANNEL_ID=#{@channel_id};SLACK_TS=#{@requested_thread_ts}-->
-    slack_thread_regex = /<!--SLACK_CHANNEL_ID=(\w+);SLACK_TS=([0-9]{10}.[0-9]{6})-->/
+    slack_thread_regex = /<!--SLACK_CHANNEL_ID=([^;.]+);SLACK_TS=([0-9]{10}.[0-9]{6})-->/
 
     req = Net::HTTP::Post.new(URI('https://slack.com/api/chat.postMessage'))
 
@@ -110,10 +108,9 @@ module DiscourseChatIntegration::Provider::SlackProvider
     }
     if message.key?(:thread_ts)
       data[:thread_ts] = message[:thread_ts]
-    elsif match = slack_thread_regex.match(post.raw)
+    elsif (match = slack_thread_regex.match(post.raw)) && match.captures[0] == channel
       data[:thread_ts] = match.captures[1]
-      post.topic.slack_thread_id = match.captures[1]
+      set_slack_thread_ts(post.topic, channel, match.captures[1])
-      post.topic.save_custom_fields
     end
 
     req.set_form_data(data)
@@ -136,10 +133,7 @@ module DiscourseChatIntegration::Provider::SlackProvider
     end
 
     ts = json["ts"]
-    if !ts.nil? && post.topic.slack_thread_id.nil?
+    set_slack_thread_ts(post.topic, channel, ts) if !ts.nil?
-      post.topic.slack_thread_id = ts
-      post.topic.save_custom_fields
-    end
 
     response
   end
@@ -183,6 +177,21 @@ module DiscourseChatIntegration::Provider::SlackProvider
     http.read_timeout = 5 # seconds
     http
   end
+
+  def self.get_slack_thread_ts(topic, channel)
+    field = TopicCustomField.where(topic: topic, name: "#{THREAD_CUSTOM_FIELD_PREFIX}#{channel}")
+    field.pluck_first(:value) || topic.custom_fields[THREAD_LEGACY]
+  end
+
+  def self.set_slack_thread_ts(topic, channel, value)
+    TopicCustomField.upsert({
+        topic_id: topic.id,
+        name: "#{THREAD_CUSTOM_FIELD_PREFIX}#{channel}",
+        value: value,
+        created_at: Time.zone.now,
+        updated_at: Time.zone.now
+    }, unique_by: [:topic_id, :name])
+  end
 end
 
 require_relative "slack_message_formatter"

spec/lib/discourse_chat_integration/provider/slack/slack_provider_spec.rb

@@ -90,10 +90,11 @@ RSpec.describe DiscourseChatIntegration::Provider::SlackProvider do
       before do
         SiteSetting.chat_integration_slack_access_token = "magic"
         @ts = "#{Time.now.to_i}.012345"
+        @ts2 = "#{Time.now.to_i}.012346"
         @stub1 = stub_request(:post, SiteSetting.chat_integration_slack_outbound_webhook_url).to_return(body: "success")
-        @thread_stub = stub_request(:post, %r{https://slack.com/api/chat.postMessage}).with(body: hash_including("thread_ts" => @ts)).to_return(body: "{\"ok\":true, \"ts\": \"12345.67890\", \"message\": {\"attachments\": [], \"username\":\"blah\", \"text\":\"blah2\"} }", headers: { 'Content-Type' => 'application/json' })
         @stub2 = stub_request(:post, %r{https://slack.com/api/chat.postMessage}).to_return(body: "{\"ok\":true, \"ts\": \"#{@ts}\", \"message\": {\"attachments\": [], \"username\":\"blah\", \"text\":\"blah2\"} }", headers: { 'Content-Type' => 'application/json' })
-        @channel = DiscourseChatIntegration::Channel.create(provider: 'dummy')
+        @thread_stub = stub_request(:post, %r{https://slack.com/api/chat.postMessage}).with(body: hash_including("thread_ts" => @ts)).to_return(body: "{\"ok\":true, \"ts\": \"#{@ts}\", \"message\": {\"attachments\": [], \"username\":\"blah\", \"text\":\"blah2\"} }", headers: { 'Content-Type' => 'application/json' })
+        @thread_stub2 = stub_request(:post, %r{https://slack.com/api/chat.postMessage}).with(body: hash_including("thread_ts" => @ts2)).to_return(body: "{\"ok\":true, \"ts\": \"#{@ts2}\", \"message\": {\"attachments\": [], \"username\":\"blah\", \"text\":\"blah2\"} }", headers: { 'Content-Type' => 'application/json' })
       end
 
       it 'sends an api request' do
@@ -103,32 +104,50 @@ RSpec.describe DiscourseChatIntegration::Provider::SlackProvider do
         described_class.trigger_notification(post, chan1, nil)
         expect(@stub1).to have_been_requested.times(0)
         expect(@stub2).to have_been_requested.once
-        expect(post.topic.slack_thread_id).to eq(@ts)
+        expect(described_class.get_slack_thread_ts(post.topic, chan1.data["identifier"])).to eq(@ts)
         expect(@thread_stub).to have_been_requested.times(0)
       end
 
       it 'sends thread id for thread' do
         expect(@thread_stub).to have_been_requested.times(0)
 
-        rule = DiscourseChatIntegration::Rule.create(channel: @channel, filter: "thread")
+        rule = DiscourseChatIntegration::Rule.create(channel: chan1, filter: "thread")
-        post.topic.slack_thread_id = @ts
+        described_class.set_slack_thread_ts(post.topic, chan1.data["identifier"], @ts)
 
         described_class.trigger_notification(post, chan1, rule)
         expect(@thread_stub).to have_been_requested.once
       end
 
+      it 'tracks threading in different channels separately' do
+        expect(@thread_stub).to have_been_requested.times(0)
+        chan2 = DiscourseChatIntegration::Channel.create(provider: 'dummy2', data: { "identifier" => "#random" })
+
+        rule = DiscourseChatIntegration::Rule.create(channel: chan1, filter: "thread")
+        rule2 = DiscourseChatIntegration::Rule.create(channel: chan2, filter: "thread")
+        described_class.set_slack_thread_ts(post.topic, chan1.data["identifier"], @ts)
+        described_class.set_slack_thread_ts(post.topic, chan2.data["identifier"], @ts2)
+
+        described_class.trigger_notification(post, chan1, rule)
+        described_class.trigger_notification(post, chan2, rule2)
+        expect(@thread_stub).to have_been_requested.once
+        expect(@thread_stub2).to have_been_requested.once
+
+        post.topic.reload
+        expect(described_class.get_slack_thread_ts(post.topic, "#general")).to eq(@ts)
+        expect(described_class.get_slack_thread_ts(post.topic, "#random")).to eq(@ts2)
+      end
+
       it 'recognizes slack thread ts in comment' do
         post.update!(cooked: "cooked", raw: <<~RAW
              My fingers are typing words that improve `raw_quality`
-             <!--SLACK_CHANNEL_ID=UIGNOREFORNOW;SLACK_TS=#{@ts}-->
+             <!--SLACK_CHANNEL_ID=#general;SLACK_TS=#{@ts}-->
         RAW
         )
 
-        rule = DiscourseChatIntegration::Rule.create(channel: @channel, filter: "thread")
+        rule = DiscourseChatIntegration::Rule.create(channel: chan1, filter: "thread")
-        post.topic.slack_thread_id = nil
 
         described_class.trigger_notification(post, chan1, rule)
-        expect(post.topic.slack_thread_id).to eq(@ts)
+        expect(described_class.get_slack_thread_ts(post.topic, chan1.data["identifier"])).to eq(@ts)
 
         expect(@thread_stub).to have_been_requested.times(1)
       end