From 3f6d940fc607d4c60880d78737de4dcb4e65462f Mon Sep 17 00:00:00 2001 From: Bernardo Farah Date: Tue, 30 Jul 2019 13:50:34 +0100 Subject: [PATCH] proxy,slack: Reply in threads Rather than doing @ to reply to people, use the much better feature of threads to reply to people and keep noise down in public channels. --- mock_internal_test.go | 3 ++ proxy.go | 17 ++++++++++++ slack.go | 9 +++--- slack_test.go | 64 +++++++++++++++++++++++++++++-------------- 4 files changed, 68 insertions(+), 25 deletions(-) diff --git a/mock_internal_test.go b/mock_internal_test.go index 83d6b36..205badb 100644 --- a/mock_internal_test.go +++ b/mock_internal_test.go @@ -8,6 +8,7 @@ import ( type testProxy struct { C chan bot.Message SendFunc func(bot.Message) error + ReplyFunc func(bot.Message) error ReactFunc func(bot.Message) error SetTopicFunc func(room, topic string) error } @@ -15,6 +16,7 @@ type testProxy struct { func newTestProxy() *testProxy { return &testProxy{ SendFunc: func(bot.Message) error { return nil }, + ReplyFunc: func(bot.Message) error { return nil }, ReactFunc: func(bot.Message) error { return nil }, SetTopicFunc: func(string, string) error { return nil }, } @@ -23,6 +25,7 @@ func newTestProxy() *testProxy { func (p *testProxy) Connect() chan bot.Message { return p.C } func (p *testProxy) Disconnect() {} func (p *testProxy) Send(m bot.Message) error { return p.SendFunc(m) } +func (p *testProxy) Reply(m bot.Message) error { return p.ReplyFunc(m) } func (p *testProxy) React(m bot.Message) error { return p.ReactFunc(m) } func (p *testProxy) SetTopic(room, topic string) error { return p.SetTopicFunc(room, topic) } diff --git a/proxy.go b/proxy.go index 1516be7..b181ab3 100644 --- a/proxy.go +++ b/proxy.go @@ -41,6 +41,23 @@ func (p *proxy) onConnect(ev *slack.ConnectedEvent) { } } +func (p *proxy) Reply(m bot.Message) error { + // Force special messages to use "Send" and skip if there is no + // message it's responding to. + if m.Params != nil || m.Envelope == nil { + return p.Send(m) + } + + msg := m.Envelope.(slack.Message) + threadTs := msg.ThreadTimestamp + if threadTs == "" { + threadTs = msg.Timestamp + } + + p.RTM.SendMessage(p.RTM.NewOutgoingMessage(m.Text, m.Room, slack.RTMsgOptionTS(threadTs))) + return nil +} + func (p *proxy) Send(m bot.Message) error { if m.Params == nil { p.RTM.SendMessage(p.RTM.NewOutgoingMessage(m.Text, m.Room)) diff --git a/slack.go b/slack.go index 2453cc1..82d46d1 100644 --- a/slack.go +++ b/slack.go @@ -21,6 +21,7 @@ type Adapter struct { Connect() chan bot.Message Disconnect() Send(bot.Message) error + Reply(bot.Message) error React(bot.Message) error SetTopic(room, topic string) error } @@ -120,12 +121,12 @@ func (a *Adapter) Reply(m bot.Message) error { return errors.New("No room provided") } - // No need to @ the user if it's a DM - if m.Room[0] != 'D' { - m.Text = "<@" + m.User + "> " + m.Text + // No need to thread the response in a DM. + if m.Room[0] == 'D' { + return a.proxy.Send(m) } - return a.proxy.Send(m) + return a.proxy.Reply(m) } // Topic uses the web API to change the topic. It prefers diff --git a/slack_test.go b/slack_test.go index 9a58b13..8643731 100644 --- a/slack_test.go +++ b/slack_test.go @@ -163,25 +163,30 @@ func TestReply(t *testing.T) { envelope.User = user cases := []struct { - In bot.Message - Out bot.Message - Err bool + Name string + In bot.Message + Out bot.Message + Err bool }{ { - In: bot.Message{Room: "D4321", Text: "foo", Envelope: envelope}, - Out: bot.Message{User: user, Room: "D4321", Text: "foo", Envelope: envelope}, + Name: "When DMing", + In: bot.Message{Room: "D4321", Text: "foo", Envelope: envelope}, + Out: bot.Message{User: user, Room: "D4321", Text: "foo", Envelope: envelope}, }, { - In: bot.Message{Room: "general", Text: "foo", Envelope: envelope}, - Out: bot.Message{User: user, Room: "C1234", Text: "<@U1234> foo", Envelope: envelope}, + Name: "When replying in a channel", + In: bot.Message{Room: "general", Text: "foo", Envelope: envelope}, + Out: bot.Message{User: user, Room: "C1234", Text: "foo", Envelope: envelope}, }, { - In: bot.Message{User: "Jane", Room: "general", Text: "foo"}, - Err: true, + Name: "Given an invalid user", + In: bot.Message{User: "Jane", Room: "general", Text: "foo"}, + Err: true, }, { - In: bot.Message{User: user, Text: "foo"}, - Err: true, + Name: "Given an invalid room", + In: bot.Message{User: user, Text: "foo"}, + Err: true, }, } store := newTestStore() @@ -191,16 +196,21 @@ func TestReply(t *testing.T) { store.User.Name = "Jean" for _, c := range cases { - proxy, run := setUpProxySend(t, c.Out) - adapter := Adapter{Store: store, proxy: proxy, BotID: user} - err := adapter.Reply(c.In) - if c.Err { - assert.NotNil(t, err) - assert.False(t, *run) - } else { - assert.Nil(t, err) - assert.True(t, *run) - } + t.Run(c.Name, func(t *testing.T) { + proxy, run := setUpProxyReply(t, c.Out) + if c.Name == "When DMing" { + proxy, run = setUpProxySend(t, c.Out) + } + adapter := Adapter{Store: store, proxy: proxy, BotID: user} + err := adapter.Reply(c.In) + if c.Err { + assert.NotNil(t, err) + assert.False(t, *run) + } else { + assert.Nil(t, err) + assert.True(t, *run) + } + }) } } @@ -263,3 +273,15 @@ func setUpProxySend(t *testing.T, out bot.Message) (*testProxy, *bool) { return proxy, &run } + +func setUpProxyReply(t *testing.T, out bot.Message) (*testProxy, *bool) { + var run bool + proxy := newTestProxy() + proxy.ReplyFunc = func(m bot.Message) error { + assert.Equal(t, out, m) + run = true + return nil + } + + return proxy, &run +}