From 222fb8fad45711961468b1666f7a29d11ef5192b Mon Sep 17 00:00:00 2001
From: Pawel Rozlach <githubcom@vespian.net>
Date: Mon, 16 Mar 2020 20:41:51 +0100
Subject: [PATCH] Do not rewrite messgaes from bots - review fixes #1

---
 server/autolinkplugin/plugin.go      | 30 ++++++++++++---------
 server/autolinkplugin/plugin_test.go | 40 ++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 13 deletions(-)

diff --git a/server/autolinkplugin/plugin.go b/server/autolinkplugin/plugin.go
index 7ef81c4a..9ad25a18 100644
--- a/server/autolinkplugin/plugin.go
+++ b/server/autolinkplugin/plugin.go
@@ -89,18 +89,6 @@ func (p *Plugin) isBotUser(userId string) (bool, *model.AppError) {
 }
 
 func (p *Plugin) ProcessPost(c *plugin.Context, post *model.Post) (*model.Post, string) {
-	isBot, appErr := p.isBotUser(post.UserId)
-	if appErr != nil {
-		// NOTE: Not sure how we want to handle errors here, we can either:
-		// * assume that occasional rewrites of Bot messges are ok
-		// * assume that occasional not rewriting of all messages is ok
-		// Let's assume for now that former is a lesser evil and carry on.
-	}
-	if isBot {
-		p.API.LogDebug("not rewriting message from bot", "userId", post.UserId)
-		return nil, ""
-	}
-
 	conf := p.getConfig()
 	postText := post.Message
 	offset := 0
@@ -170,7 +158,23 @@ func (p *Plugin) ProcessPost(c *plugin.Context, post *model.Post) (*model.Post,
 
 		return true
 	})
-	post.Message = postText
+	if post.Message != postText {
+		isBot, appErr := p.isBotUser(post.UserId)
+		if appErr != nil {
+			// NOTE: Not sure how we want to handle errors here, we can either:
+			// * assume that occasional rewrites of Bot messges are ok
+			// * assume that occasional not rewriting of all messages is ok
+			// Let's assume for now that former is a lesser evil and carry on.
+		} else if isBot {
+			// We intentionally use a single if/else block so that the code is
+			// more readable and does not relly on hidden side effect of
+			// isBot==false when appErr!=nil.
+			p.API.LogDebug("not rewriting message from bot", "userId", post.UserId)
+			return nil, ""
+		}
+
+		post.Message = postText
+	}
 
 	post.Hashtags, _ = model.ParseHashtags(post.Message)
 
diff --git a/server/autolinkplugin/plugin_test.go b/server/autolinkplugin/plugin_test.go
index 4235e72e..940c6e37 100644
--- a/server/autolinkplugin/plugin_test.go
+++ b/server/autolinkplugin/plugin_test.go
@@ -542,6 +542,46 @@ func TestBotMessagesAreRewritenWhenGetUserFails(t *testing.T) {
 	assert.Equal(t, "Welcome to [Mattermost](https://mattermost.com)!", rpost.Message)
 }
 
+func TestGetUserApiCallIsNotExecutedWhenThereAreNoChanges(t *testing.T) {
+	conf := Config{
+		Links: []autolink.Autolink{
+			autolink.Autolink{
+				Pattern:  "(Mattermost)",
+				Template: "[Mattermost](https://mattermost.com)",
+			},
+		},
+	}
+
+	testChannel := model.Channel{
+		Name: "TestChanel",
+	}
+
+	testTeam := model.Team{
+		Name: "TestTeam",
+	}
+
+	api := &plugintest.API{}
+
+	api.On("LoadPluginConfiguration", mock.AnythingOfType("*autolinkplugin.Config")).Return(func(dest interface{}) error {
+		*dest.(*Config) = conf
+		return nil
+	}).Once()
+	api.On("UnregisterCommand", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return((*model.AppError)(nil)).Once()
+
+	api.On("GetChannel", mock.AnythingOfType("string")).Return(&testChannel, nil).Once()
+	api.On("GetTeam", mock.AnythingOfType("string")).Return(&testTeam, nil).Once()
+	api.On("LogDebug", mock.AnythingOfType("string"), mock.AnythingOfType("string"), mock.AnythingOfType("string"))
+
+	p := Plugin{}
+	p.SetAPI(api)
+	p.OnConfigurationChange()
+
+	post := &model.Post{Message: "Welcome to FooBarism!"}
+	rpost, _ := p.MessageWillBePosted(&plugin.Context{}, post)
+
+	assert.Equal(t, "Welcome to FooBarism!", rpost.Message)
+}
+
 func TestBotMessagesAreNotRewriten(t *testing.T) {
 	conf := Config{
 		Links: []autolink.Autolink{