Add setting per-autolink for allowing bot posts

[mickmister]

Summary

Currently, the plugin does not edit any posts made by bots. There is a customer wanting to use the autolink plugin with their own bot's posts. I've added a setting on each configured autolink to allow bot posts to be edited.

Thread for customer request:
https://community-daily.mattermost.com/core/pl/35ho6xwqepfppyb6wpx5fwqz9a

Ticket Link

Fixes #141

Related Issues/PRs

Issue - #94
Pull Request - #98

[codecov-commenter]

Codecov Report

Merging #160 (80ee706) into master (e73e585) will decrease coverage by 0.40%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
- Coverage   41.20%   40.79%   -0.41%     
==========================================
  Files           6        6              
  Lines         665      679      +14     
==========================================
+ Hits          274      277       +3     
- Misses        371      382      +11     
  Partials       20       20              

Impacted Files	Coverage Δ
server/autolinkplugin/command.go	0.00% <0.00%> (ø)
server/autolinkplugin/config.go	33.78% <0.00%> (-0.94%)	⬇️
server/autolink/autolink.go	57.30% <33.33%> (-0.84%)	⬇️
server/autolinkplugin/plugin.go	92.00% <100.00%> (+0.13%)	⬆️

---

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 e73e585...80ee706. Read the comment docs.

[hanzei]

If @aaronrothschild is fine with this, I'm also

[mickmister]

@hanzei Most of the consensus around blocking bot posts in the first place was that this should be a per-link setting, and I didn't want to force an admin to make a hard decision of global enable/disable.

[hanzei]

LGTM, but tests are broken https://app.circleci.com/pipelines/github/mattermost/mattermost-plugin-autolink/523/workflows/e0b365cc-f37e-4501-837d-e495617448ec/jobs/1629

[mickmister]

@hanzei The specific test failing is failing because it defends against fetching the bot user until we have determined that the text has changed by any one of the autolinks. I see two paths here:

• Don't fetch the bot user beforehand. Go through all autolinks like normal. If the text has been altered, run through the autolinks again with the if statement check added in the last commit.
• Take the RPC hit of fetching the user (I'd rather not)

[hanzei]

@mickmister Can we combine the two things and only fetch the bot once in the loop? Something like:

var user *model.user

for _, link := range conf.Links {
	avoidBotPost := false
	if !link.ProcessBotPosts {
		if user == nil {
			// fetch user
		}
		if user.IsBot() {
			avoidBotPost = true
		}
	}
	if !avoidBotPost && p.inScope(link.Scope, channelName, teamName) {
		newText = link.Replace(newText)
	}
}

[mickmister]

@hanzei By default all links will have ProcessBotPosts false, so the snippet you posted is not really solving the performance issue. What about this?

var user *model.user

for _, link := range conf.Links {
    if !p.inScope(link.Scope, channelName, teamName) {
        continue
    }

    processed := link.Replace(newText)
    if processed == newText {
        continue
    }
    
    if !link.ProcessBotPosts {
        if user == nil {
            // fetch user
        }
        if user.IsBot() {
            continue
        }
    }

    newText = processed
}

[hanzei]

@mickmister ProcessBotPosts being false by default is a breaking change right? Previously, all bot posts where shipped.

[mickmister]

ProcessBotPosts being false by default is a breaking change right? Previously, all bot posts where shipped

@hanzei No, the default behavior has not changed in this PR. I think the confusion may be around the ProcessBotPosts boolean. If it is false, we will skip the bot post like usual. "Process" in this case means "accept" the bot posts for edits.

Before the PR:

• If no autolinks edited the post, do not check if the user is a bot
• If at least one autolink edited the post, check if the user is a bot.
  • If the user is a bot, return nil and do not persist any edits
  • else persist the changes

After this PR (let's just assume the default setting to compare with previous behavior):

• If a given autolink edited the post, check if the user is a bot
  • If the user is a bot, do not persist the change
  • else queue change for persisting
• Check if changes have been applied, and persist if changed

[hanzei]

LGTM 👍

[DHaussermann]

@mickmister do you have any thoughts on @vinod-demansol's comments above.

We should also add info to the help on how the new functionality is set.

[mickmister]

• Can the command be included in the autocomplete?
• Can the /autolink list command show an output that includes the value?
• The help text should show that you can pass in the configuration for bots.

@DHaussermann I'm not sure when I will have the cycles to address the issues above. @aaronrothschild Are these items a requirement for releasing this feature? Maybe we can assign this PR to someone from Demansol?

also I see if I type /autolink set mm ProcessBotPosts false , then also I am able to edit Bot message , So this is issue Please fix this issue.

@vinod-demansol Can you please explain what you mean by "I am able to edit Bot message"? What steps did you take to test (the "editing" part of the test), what was the expected outcome, and what happened instead?

[aaronrothschild]

@mickmister Let's try to ship it as is, those missing commands aren't blockers, but should be in the next release fo Autolink. It seems we have some customers waiting for this, so I am inclined to ship as is. Then add a new ticket for 1.4.0.

Captured the leftover work to be done here #182

[hanzei]

@vinod-demansol What do you think about the comment above? #160 (comment)

[vinod-demansol]

@vinod-demansol What do you think about the comment above? #160 (comment)

I am ok if its tracked in new ticket in 1.4.0

[vinod-demansol]

/autolink set mm ProcessBotPosts false

Steps i followed

1. Create a link
2. create a pattern and template

• Can the command be included in the autocomplete?
• Can the /autolink list command show an output that includes the value?
• The help text should show that you can pass in the configuration for bots.

@DHaussermann I'm not sure when I will have the cycles to address the issues above. @aaronrothschild Are these items a requirement for releasing this feature? Maybe we can assign this PR to someone from Demansol?

also I see if I type /autolink set mm ProcessBotPosts false , then also I am able to edit Bot message , So this is issue Please fix this issue.

@vinod-demansol Can you please explain what you mean by "I am able to edit Bot message"? What steps did you take to test (the "editing" part of the test), what was the expected outcome, and what happened instead?

steps I followed

1. created a link
2. created a pattern and template
   "Pattern": "(LHS)",
   "Template": "LHS"
   3.Enabled the link
3. typed /autolink set mm ProcessBotPosts true
4. typed LHS in channel and I got the expected LHS template
5. typed /autolink set mm ProcessBotPosts false
6. tried editing the previous typed LHS , then also I see template string

[hanzei]

@vinod-demansol What is the difference between the expected behaviour and the one that you are seeing?

[dipak-demansol]

curl -i -X POST -H 'Content-Type: application/json' -d '{"channel_id":<>, "message":"This is a message from a bot with mention for @dkh-sysadmin and @dkh-member", "props":{"attachments": [{"pretext": "LHS","text": "LHS"}]}}' -H 'Authorization: Bearer <>' https://demansol-sandbox.test.mattermost.cloud/api/v4/posts

@mickmister when i make a bot post using this curl script, i don't see the post re-written by autolink in the mattermost. can you pls provide a valid curl script in case its a user error.

[hanzei]

@dipak-demansol What part of the message do you expect to be re-written?

FYI: Slack attachments where never meant to be re-written.

[mickmister]

@dipak-demansol I agree with @hanzei, it seems you are expecting LHS to be autolinked inside of the slack attachments?

If that's the case, we should indeed expect the text to not change, so what you're seeing is not a bug in this case.

[dipak-demansol]

@mickmister can you pls provide me the repro steps to regenerate the issue.

[dipak-demansol]

@dipak-demansol I agree with @hanzei, it seems you are expecting LHS to be autolinked inside of the slack attachments?

If that's the case, we should indeed expect the text to not change, so what you're seeing is not a bug in this case.

yes i was expecting that. i misunderstood the issue so can you pls provide me the repro steps to generate the issue and then i'll test it properly.

[mickmister]

@dipak-demansol

• Create an autolink like one in the README of this repo. It seems you have the LHS one configured on your test server. But it looks like you have multiple Autolinks with that same pattern, so make sure you only have one Autolink with this pattern enabled, in order to isolate the behavior of this Autolink.
• Create a post with a bot's token, with the Autolink pattern in the post's message. I would just modify your current payload to include LHS in the message. That's all you should need to do if the post is working otherwise.

Then perform these three tests:

1. Set the ProcessBotPosts boolean to true for the Autolink. Create the post described above, and verify that the bot post is changed by the autolink plugin.
2. Set the ProcessBotPosts boolean to false for the Autolink. Create the post described above, and verify that the bot post is not changed by the autolink plugin.
3. Create the same Autolink again, to assert the behavior that the default value should be false. Create the post described above, and verify that the bot post is not changed by the autolink plugin.

[DHaussermann]

@dipak-demansol and I tested this.

I does seem functional but there are some limitations the really impact usability. We were unaware that this was a link level config setting and not a plugin level config. (This info was provided on this PR but we need end user facing details.) This is not discoverable and can only be configured via the UI.

• The Setting should show in help
• The setting should be configurable with a UI command
• The state of the setting should be returned when you run list?

[mickmister]

• The Setting should show in help
• The setting should be configurable with a UI command
• The state of the setting should be returned when you run list?

@dipak-demansol These issues are now addressed

• Setting shows in help now
• /autolink set (name) ProcessBotPosts true shows up in autocomplete now. The command was working already before, just no autocomplete.
• ProcessBotPosts shows when the autolink has this value as true. This was actually working as well, it just only shows the value if it is true. This is the same as other boolean flags for the autolinks.

[dipak-demansol]

With master build when i add Bot post then LHS is not rewritten and its expected, if i try to set ProcessBotPosts True in old build then i'll get this kind of response.

:- With the PR build i'm able to set true the ProcessBotPosts and Now the bot post is rewritten so its LGTM.

non blocking point

:- testing with PR build i'm able to ProcessBotPosts set True/false but it would be great if the True/false also be the part of auto-suggestion.

@mickmister After Discussing the same result with Dylan, if is there anything blocking then will update you, if all Good then i'll add tag as Qa review Done

[dipak-demansol]

LGTM, as per the Discussion with Dylan, will add non-blocking point list on new tiket.

[dipak-demansol]

LGTM, as per the Discussion with Dylan, will add non-blocking point list on new tiket.

@hanzei @mickmister