[GH-84] Do not rewrite messages from bots #98

vespian · 2020-03-07T17:42:20Z

Summary

This PR makes autolink plugin not rewrite messages from other bots as discussed in #94

Ticket Link

Fixes #94

hanzei

Thanks for also adding a test 👍

server/autolinkplugin/plugin.go

go.mod

codecov-io · 2020-03-09T19:56:08Z

Codecov Report

Merging #98 into master will increase coverage by 1.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #98      +/-   ##
==========================================
+ Coverage   36.43%   37.47%   +1.04%     
==========================================
  Files           6        6              
  Lines         538      547       +9     
==========================================
+ Hits          196      205       +9     
  Misses        320      320              
  Partials       22       22

Impacted Files	Coverage Δ
server/autolinkplugin/plugin.go	`60.41% <100%> (+4.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e53e3f...34e1a57. Read the comment docs.

vespian · 2020-03-09T20:02:52Z

@hanzei Ready for another pass.

hanzei

One non blocking comment 👍

server/autolinkplugin/plugin.go

levb · 2020-03-10T19:11:28Z

server/autolinkplugin/plugin.go

 func (p *Plugin) ProcessPost(c *plugin.Context, post *model.Post) (*model.Post, string) {
+	isBot, appErr := p.isBotUser(post.UserId)


It is likely considerably more expensive to go back to the server to get the user record than first evaluating to see if any links apply. 3/5 we should move this check after an applicable link is found.

@levb I am curious - what does 3/5, 0/5 mean? I often see it in reviews.

Wrt. your comment - we do not really have data on how big/how numerous the rules can get, and how much traffic we may need to rewrite and how the plugin system and querying the server performs under load. The rewriting rules also already call p.API.GetChannel and p.API.GetTeam api calls which may be heavier than the p.API.GetUser that we are adding and actually having isBot check early may save CPU cycles.

I believe optimizing it now is a bit early. Would you mind if we ask @hanzei to add his two cents as well?

@levb @hanzei Ping.

(waiting for @hanzei also cc @lieut-data) A single RPC/post is too expensive, anywhere in a plugin, period. We should move the RPCs to after the match, so we only execute them when needed.

Now, I see the argument that if this PR follows the existing pattern, we should stick with it as is, and have a separate ticket to optimize. I'm 1/5 that we shouldn't - let's at least make sure this PR doesn't add overhead.

https://docs.mattermost.com/process/training.html#id8

@levb, based on @ali-farooq0's preliminary analysis of the RPC overhead on community, it /seems/ like RPC, itself, isn't that expensive. The real cost is borne by the database queries to resolve this information, but I'm assuming we're already doing one or more of those to post the message in the first place.

That being said, no objections to moving this check below and wrapping in a post.Message != postText test first. Agree we should have a ticket to optimize regardless.

Thanks for the discussion! Requested changes can be found in Do not rewrite messgaes from bots - review fixes #1 commit.

vespian · 2020-03-16T19:44:09Z

@levb Ready for another pass!

levb

LGTM.

I think we should not modify the message when isBot fails.

levb · 2020-03-17T13:24:18Z

server/autolinkplugin/plugin.go

@@ -131,7 +141,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)


It seems that if there is a "system failure" in the process of modifying the message, the right thing to do is to pass it through as is. 0/5 if isBot || appErr != nil {...

What do you mean by in the process of modifying the message? isBotUser does not modify the message.

jfrerich · 2020-03-23T17:22:51Z

@vespian, can you please merge from master and resolve the conflict in plugin_test.go

@lieut-data, gentle ping for review

lieut-data · 2020-03-23T17:24:38Z

Thanks, @jfrerich! Happy to defer to @hanzei and @levb's reviews here -- nothing blocking from my side :)

jfrerich · 2020-03-23T17:26:06Z

@lieut-data thanks for checking back in. I know we already have the two dev reviews and just checking you had no further comments :)

vespian · 2020-03-23T18:09:39Z

@vespian, can you please merge from master and resolve the conflict in plugin_test.go

@jfrerich Done.

DHaussermann · 2020-03-26T18:30:40Z

@vespian or @lieut-data can you help me understand the scope of this change? I'm not sure how to validate this and exactly what it should do.

I'm seeing the the Links are still working for posts made the Jira bot or GitHub bot which is great.
When I make a post with a bot I have created the autolink is not applied even when post content matches the pattern.
This seems like the desired behavior but how is it working? Are post from some bots already formatted to contain a link to GitHub, Jira, etc so it's not actually autolink modifying the post contents?

lieut-data · 2020-03-27T18:52:49Z

@DHaussermann, exactly, GitHub/Jira already knows how to format its own links. The easiest way to truly verify might be to setup the conditions of #94 with an at-mention to yourself on a commit that autolink would have otherwise rewritten. Assuming that stops working, and the normal links stays in place, I think we're good.

DHaussermann

Tested and passed.

Ensured autolink is not formatting bot posts
Regression tested posts made from GitHub and Jira
Added release testing
LGTM!
Thanks @vespian for your effort on this!

lieut-data · 2020-03-30T13:53:14Z

@aaronrothschild, any objections to this change?

aaronrothschild

LGTM. This makes sense to me. Good addition!

…most-community#98)

vespian mentioned this pull request Mar 7, 2020

Autolink should ignore bot posts. (was: Commit notifications trigger unexpected link preview) #94

Closed

vespian changed the title ~~Do not rewrite messgaes from bots~~ [GH-84] Do not rewrite messgaes from bots Mar 7, 2020

hanzei requested review from levb and hanzei March 9, 2020 08:24

hanzei added the 2: Dev Review Requires review by a core committer label Mar 9, 2020

hanzei reviewed Mar 9, 2020

View reviewed changes

server/autolinkplugin/plugin.go Outdated Show resolved Hide resolved

server/autolinkplugin/plugin.go Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

hanzei added the 3: QA Review Requires review by a QA tester label Mar 9, 2020

vespian force-pushed the prozlach/GH-94_no_rewriting_for_bots branch from a9ce2b4 to d0d75bc Compare March 9, 2020 19:56

vespian force-pushed the prozlach/GH-94_no_rewriting_for_bots branch 2 times, most recently from 75d2539 to 34e1a57 Compare March 9, 2020 20:01

hanzei approved these changes Mar 9, 2020

View reviewed changes

server/autolinkplugin/plugin.go Outdated Show resolved Hide resolved

vespian force-pushed the prozlach/GH-94_no_rewriting_for_bots branch from 34e1a57 to 2c0ad05 Compare March 10, 2020 17:43

levb suggested changes Mar 10, 2020

View reviewed changes

lieut-data changed the title ~~[GH-84] Do not rewrite messgaes from bots~~ [GH-84] Do not rewrite messages from bots Mar 16, 2020

levb approved these changes Mar 17, 2020

View reviewed changes

jfrerich mentioned this pull request Mar 19, 2020

Bump version to 1.2.0 #101

Merged

3 tasks

levb added 1: PM Review Requires review by a product manager and removed 2: Dev Review Requires review by a core committer labels Mar 23, 2020

levb requested review from aaronrothschild and DHaussermann March 23, 2020 17:25

Pawel Rozlach added 2 commits March 23, 2020 19:04

Do not rewrite messgaes from bots

64909f0

Do not rewrite messgaes from bots - review fixes mattermost-community#1

222fb8f

vespian force-pushed the prozlach/GH-94_no_rewriting_for_bots branch from ce6ce1d to 222fb8f Compare March 23, 2020 18:04

Rebasing fixes

0450240

DHaussermann approved these changes Mar 30, 2020

View reviewed changes

DHaussermann added QA Review Done PR has been approved by QA 1: PM Review Requires review by a product manager and removed 1: PM Review Requires review by a product manager 3: QA Review Requires review by a QA tester labels Mar 30, 2020

aaronrothschild approved these changes Mar 30, 2020

View reviewed changes

aaronrothschild removed the 1: PM Review Requires review by a product manager label Mar 30, 2020

hanzei added the 4: Reviews Complete All reviewers have approved the pull request label Mar 31, 2020

hanzei added this to the v1.2.0 milestone Mar 31, 2020

hanzei merged commit a646d82 into mattermost-community:master Mar 31, 2020

vespian deleted the prozlach/GH-94_no_rewriting_for_bots branch March 31, 2020 09:59

mo2menelzeiny pushed a commit to mo2menelzeiny/mattermost-plugin-autolink that referenced this pull request Apr 4, 2020

[mattermost-communityGH-84] Do not rewrite messages from bots (matter…

ba9e039

…most-community#98)

mickmister mentioned this pull request Apr 26, 2021

Add setting per-autolink for allowing bot posts #160

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-84] Do not rewrite messages from bots #98

[GH-84] Do not rewrite messages from bots #98

vespian commented Mar 7, 2020

hanzei left a comment

codecov-io commented Mar 9, 2020 •

edited

Loading

vespian commented Mar 9, 2020

hanzei left a comment

levb Mar 10, 2020

vespian Mar 10, 2020

vespian Mar 10, 2020 •

edited

Loading

vespian Mar 13, 2020

levb Mar 16, 2020

lieut-data Mar 16, 2020

vespian Mar 16, 2020

vespian commented Mar 16, 2020

levb left a comment

levb Mar 17, 2020

vespian Mar 17, 2020

jfrerich commented Mar 23, 2020

lieut-data commented Mar 23, 2020

jfrerich commented Mar 23, 2020

vespian commented Mar 23, 2020

DHaussermann commented Mar 26, 2020

lieut-data commented Mar 27, 2020

DHaussermann left a comment

lieut-data commented Mar 30, 2020

aaronrothschild left a comment

		func (p Plugin) ProcessPost(c plugin.Context, post model.Post) (model.Post, string) {
		isBot, appErr := p.isBotUser(post.UserId)

[GH-84] Do not rewrite messages from bots #98

[GH-84] Do not rewrite messages from bots #98

Conversation

vespian commented Mar 7, 2020

Summary

Ticket Link

hanzei left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 9, 2020 • edited Loading

Codecov Report

vespian commented Mar 9, 2020

hanzei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vespian Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vespian commented Mar 16, 2020

levb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfrerich commented Mar 23, 2020

lieut-data commented Mar 23, 2020

jfrerich commented Mar 23, 2020

vespian commented Mar 23, 2020

DHaussermann commented Mar 26, 2020

lieut-data commented Mar 27, 2020

DHaussermann left a comment

Choose a reason for hiding this comment

lieut-data commented Mar 30, 2020

aaronrothschild left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 9, 2020 •

edited

Loading

vespian Mar 10, 2020 •

edited

Loading