Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bot should not handle messages originating from itself #6

Merged
merged 4 commits into from
Sep 22, 2019

Conversation

jfchevrette
Copy link
Contributor

This attempts to fix #5.

The approach is a bit naive but it seems to work in my own testing as well as the included test cases

This adds a check that skips message event handling if the message came from the bot user.

I cannot think of a reason the bot would want to handle it's own messages.

adapter_test.go Show resolved Hide resolved
@@ -87,6 +119,33 @@ func TestAdapter_DirectMessages(t *testing.T) {
assert.Equal(t, expectedEvt, events[0])
}

func TestAdapter_IgnoreDirectOwnMessages(t *testing.T) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

122-147 lines are duplicate of adapter_test.go:66-91 (from dupl)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first it was not clear to me either how the two tests differ until I realized its really only the channel names.
I think the duplication can be removed and the test can be clarified like this:

func TestAdapter_IgnoreOwnMessages(t *testing.T) {
	cases := map[string]struct{ Channel string }{
		"channel message": {"C1H9RESGL"}, // map test case name to channel ID
		"direct message":  {"D023BB3L2"}, // direct slack channels start with a "D"
	}

	for name, c := range cases {
		t.Run(name, func(t *testing.T) {
			brain := joetest.NewBrain(t)
			a, _ := newTestAdapter(t)

			done := make(chan bool)
			go func() {
				a.handleSlackEvents(brain.Brain)
				done <- true
			}()

			evt := &slack.MessageEvent{
				Msg: slack.Msg{
					Text:    "Hello world",
					Channel: c.Channel,
					User:    a.userID,
				},
			}

			a.events <- slack.RTMEvent{Data: evt}

			close(a.events)
			<-done
			brain.Finish()

			assert.Empty(t, brain.RecordedEvents())
		})
	}
}

Note that I also used a.userID in the event instead of the magic "42" because otherwise it requires the reader of the test to know that this ID is actually the test adapters ID which is set in a different function.

adapter_test.go Outdated Show resolved Hide resolved
adapter_test.go Outdated Show resolved Hide resolved
@jfchevrette
Copy link
Contributor Author

I am not sure how to fix the dupl errors. Should I change the function names so they are less alike?

Copy link
Contributor

@fgrosse fgrosse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution :)

@@ -87,6 +119,33 @@ func TestAdapter_DirectMessages(t *testing.T) {
assert.Equal(t, expectedEvt, events[0])
}

func TestAdapter_IgnoreDirectOwnMessages(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first it was not clear to me either how the two tests differ until I realized its really only the channel names.
I think the duplication can be removed and the test can be clarified like this:

func TestAdapter_IgnoreOwnMessages(t *testing.T) {
	cases := map[string]struct{ Channel string }{
		"channel message": {"C1H9RESGL"}, // map test case name to channel ID
		"direct message":  {"D023BB3L2"}, // direct slack channels start with a "D"
	}

	for name, c := range cases {
		t.Run(name, func(t *testing.T) {
			brain := joetest.NewBrain(t)
			a, _ := newTestAdapter(t)

			done := make(chan bool)
			go func() {
				a.handleSlackEvents(brain.Brain)
				done <- true
			}()

			evt := &slack.MessageEvent{
				Msg: slack.Msg{
					Text:    "Hello world",
					Channel: c.Channel,
					User:    a.userID,
				},
			}

			a.events <- slack.RTMEvent{Data: evt}

			close(a.events)
			<-done
			brain.Finish()

			assert.Empty(t, brain.RecordedEvents())
		})
	}
}

Note that I also used a.userID in the event instead of the magic "42" because otherwise it requires the reader of the test to know that this ID is actually the test adapters ID which is set in a different function.

@fgrosse fgrosse self-assigned this Sep 15, 2019
@fgrosse
Copy link
Contributor

fgrosse commented Sep 15, 2019

Oh, and can you also please update the CHANGELOG.md?

E.g. like this:

diff --git a/CHANGELOG.md b/CHANGELOG.md
index ce3d71d..75f2766 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,7 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
-Nothing so far
+- Fix issue [Bot is handling messages coming from itself #5](https://github.com/go-joe/slack-adapter/issues/5)
 
 ## [v0.6.0] - 2019-04-19
 - Update to joe v0.7.0

@fgrosse fgrosse mentioned this pull request Sep 22, 2019
@fgrosse fgrosse merged commit 40dc815 into go-joe:master Sep 22, 2019
@jfchevrette jfchevrette deleted the ignore_self_msg branch October 3, 2019 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bot is handling messages coming from itself
3 participants