GH-42 #81

CalebMaftei · 2021-06-29T21:02:37Z

Summary

This Pull Request adds new functionality that allows a team admin or system admin to adjust a dynamic team-welcome message. Previously, the only way a team message could be set was to adjust the config.json, and then redeploy the instance of mattermost. This has caused certain team admins over at Platform 1 to seek out a resolution that allows for the dynamic ability to set their team welcome message without adjusting the config.json.

With this new functionality, we have added 3 new slash commands that allow a user with the appropriate credentials to set, get, and delete what we are calling dynamic team messages with the following commands:

get_team_welcome - Grabs the stored dynamic team message (if it exists), or retrieves the config.json message set.
set_team_welcome [new message] - Stores internally a new KV pair where the key is the team id joined with the prefix: "teammsg_", and where the value is the message. This does not override the config.json value though.
delete_team_welcome - This deletes the KV pair set for the team without ever touching the config.json message (if it exists).

If a dynamic team message is found, then the get call will retrieve that first. If a user ever wanted to resort back to the config.json message, they will just need to delete the dynamic message. All values will defer to the config.json message if no dynamic message is found.

This new functionality also allows appropriate team admins to have access to the above slash commands, rather than permitting only system admins.

Also, on line 115, I fixed a misspelling of the word 'welcome.'

Please let me know if this is the direction you would like to pursue regarding the solution to issue 42! If there is anything that I left out of this PR, I would be more than happy to add that necessary information! Thank you!

Ticket Link

Fixes #42

mattermod · 2021-06-29T21:02:41Z

Hello @CalebMaftei,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

CalebMaftei · 2021-07-06T19:14:58Z

Issue #42 looks to be completed! Disregard this pull request.

hanzei · 2021-07-07T07:11:45Z

@CalebMaftei I don't see a PR that addresses #42. Could you please point me to that one?

hanzei

That is great PR. The code looks quite good 👍

Thanks also for adding this more granular permission checks 👍

One piece of feedback: It's idiomatic in go to use err as the variable name returned for errors. There are surely places where it's not needed, but you should stick with that guideline more.

hanzei · 2021-07-16T10:28:34Z

README.md

+- `/welcomebot set_team_welcome [welcome message]` - sets the current team's welcome message to the one defined in the slash command.
+- `/welcomebot delete_team_welcome` - deletes the current team's welcome message. This however does NOT delete any messages set inside of the config.json.
+
+For saving these messages, they are stored as KV pairs. Team message pairs have the prefix `teammsg_<team_id>` to distinguish them from the `chanmsg_<channel_id>` pairs.


Is this information really needed for end users?

I added this in part to explain on a high level the functionality we added, as well as explain to team admins how they can use the new slash commands. However, we can easily remove that.

My suggestion would be to delete the paragraph about the KV pairs.

I have removed the KV paragraph!

Hi, @CalebMaftei, can you push this commit to the PR branch so that we can see the update?

server/command.go

hanzei · 2021-07-16T11:51:34Z

server/hooks.go

+	// We send a DM and an opportunistic ephemeral message to the channel. See
+	// the discussion at the link below for more details:
+	// https://github.com/mattermost/mattermost-plugin-welcomebot/pull/31#issuecomment-611691023
+	teamAddMessage := "# Welcome to the " + data.Team.DisplayName + " team!! \n\n"
+	postDM := &model.Post{
+		UserId:    p.botUserID,
+		ChannelId: data.DirectMessage.Id,
+		Message:   teamAddMessage + string(teamMessage),
+	}
+	if _, appErr := p.API.CreatePost(postDM); appErr != nil {
+		mlog.Error("failed to post welcome message to the channel",
+			mlog.String("channelId", data.DirectMessage.Id),
+			mlog.Err(appErr),
+		)
 	}
+
+	postChannel := &model.Post{
+		UserId:    p.botUserID,
+		ChannelId: teamMember.TeamId,
+		Message:   string(teamMessage),
+	}
+	time.Sleep(1 * time.Second)
+	_ = p.API.SendEphemeralPost(teamMember.UserId, postChannel)


There is a lot of duplicate code here. Can this be avoided?

I can go through this piece and refactor it to be less complicated!

I removed the ephemeral message. I believe we originally had it so that the user would immediately be greeted by the welcome message, however the DM with the welcome-bot is plenty sufficient.

server/hooks.go

hanzei · 2021-07-16T11:54:29Z

server/hooks.go

+	// We send a DM and an opportunistic ephemeral message to the channel. See
+	// the discussion at the link below for more details:
+	// https://github.com/mattermost/mattermost-plugin-welcomebot/pull/31#issuecomment-611691023
+	teamAddMessage := "# Welcome to the " + data.Team.DisplayName + " team!! \n\n"


Why is this default message needed?

This change I believe was added to give some kind of header to the welcome bot messages. However, if this ultimately feels like we are stripping freedoms to the welcome message, it can be thrown out.

I'm 1/5 we should leave the complete message to the message creator and not prefix the header.

hanzei · 2021-07-16T11:54:46Z

server/hooks.go

@@ -68,10 +107,12 @@ func (p *Plugin) UserHasJoinedChannel(c *plugin.Context, channelMember *model.Ch
 	// We send a DM and an opportunistic ephemeral message to the channel. See
 	// the discussion at the link below for more details:
 	// https://github.com/mattermost/mattermost-plugin-welcomebot/pull/31#issuecomment-611691023
+	channelAddMessage := "# Welcome to the " + channelInfo.DisplayName + " channel. \n\n"


See above. Why is this behavior change added here?

This change I believe was added to give some kind of header to the welcome bot messages. However, if this ultimately feels like we are stripping freedoms to the welcome message, it can be thrown out.

Yes, please throw it out.

hanzei · 2021-07-16T11:55:48Z

server/hooks.go

+	if actor.Id == channelMember.UserId {
+		time.Sleep(3 * time.Second)
+	} else {
+		time.Sleep(15 * time.Second)
+	}


What is the reasoning behind this change?

This was added by another Dev of ours, I believe. However, I think it was due to a cluster related timing issue. I can look more into this and why it was added.

So digging into this deeper, the reason this was added was to prevent a race condition between the posted ephemeral message as well as the actual welcome message. The reasoning from our Dev was: "there was some race condition involving the welcome message being sent in a specific case. I think in one instance of a user being added to a channel, the ephemeral message was sent before the user was added to the channel, so they could never see it."

However, I think this can easily be mitigated by removing the ephemeral message as this was the issue with redundancy found inside of the team welcome messages as well. What are your thoughts?

mattermod · 2021-07-31T01:01:09Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

jfrerich

Thanks for this PR, @CalebMaftei! I have some small suggestions on top of the request from Ben.

jfrerich · 2021-08-04T14:50:01Z

server/command.go

+	commandTriggerDeleteTeamWelcome    = "delete_team_welcome"
+
+	// Error Message Constants
+	unsetMessageError = "welcome message has not been set yet"


Suggested change

unsetMessageError = "welcome message has not been set yet"

unsetMessageError = "welcome message has not been set"

The next push for this PR will have this change :)

jfrerich · 2021-08-04T14:57:30Z

server/command.go

 		if len(parameters) > 0 {
 			return "`delete_channel_welcome` command does not accept any extra parameters"


Suggested change

if len(parameters) > 0 {

return "`delete_channel_welcome` command does not accept any extra parameters"

if len(parameters) > 0 {

return "`" + commandTriggerDeleteChannelWelcome + "` command does not accept any extra parameters"

Would you mind changing the channel set, get, and delete return messages to use their respective const variables like you have done for the teams?

The next push for this PR will have this change :)

I did the same for set and get channel welcomes as well.

jfrerich · 2021-08-04T15:38:09Z

server/hooks.go

+	}
+
+	if teamMessage == nil {
+		// No dynamic welcome message for the given team, so we check if one as been set in the config.json


Suggested change

// No dynamic welcome message for the given team, so we check if one as been set in the config.json

// No dynamic welcome message for the given team, so we check if one has been set in the config.json

The next push for this PR will have this change :)

jfrerich · 2021-08-04T15:41:26Z

server/hooks.go

+	// We send a DM and an opportunistic ephemeral message to the channel. See
+	// the discussion at the link below for more details:
+	// https://github.com/mattermost/mattermost-plugin-welcomebot/pull/31#issuecomment-611691023
+	teamAddMessage := "# Welcome to the " + data.Team.DisplayName + " team!! \n\n"


I'm 1/5 we should leave the complete message to the message creator and not prefix the header.

jfrerich · 2021-08-04T15:46:16Z

server/command.go

+	isTeamAdmin, teamAdminError := p.hasTeamAdminRole(userID, teamID)
+
+	if sysAdminError != nil {
+		p.postCommandResponse(args, "error occurred while getting the System Admin Role `%s`: `%s`", teamID, sysAdminError)


hasSysadminRole() does not accept the teamID, so I think we can drop the teamID from the error message.

I removed the team message header, as well as removed the unnecessary teamID for the hasSystemadminRole() function. Those changes should be in with the next push to this PR :)

jfrerich · 2021-08-04T15:47:00Z

server/command.go

+	return true
+}
+
+// This retrives a map of team Ids with their respective welcome message


Suggested change

// This retrives a map of team Ids with their respective welcome message

// This retrieves a map of team Ids with their respective welcome message

The next push to this PR will have this spelling change :)

jfrerich · 2021-08-04T15:57:36Z

server/command.go

+		return
+	}
+
+	p.postCommandResponse(args, "stored the welcome message:\n%s", message)


Suggested change

p.postCommandResponse(args, "stored the welcome message:\n%s", message)

p.postCommandResponse(args, "stored the team welcome message:\n%s", message)

This change as well as other places in code that didn't distinguish channel from team welcome messages has been updated!

jfrerich · 2021-08-04T15:57:53Z

server/command.go

+		return
+	}
+
+	p.postCommandResponse(args, "welcome message has been deleted")


Suggested change

p.postCommandResponse(args, "welcome message has been deleted")

p.postCommandResponse(args, "team welcome message has been deleted")

This change will be in the next push for the PR :)

hanzei · 2021-08-16T13:05:11Z

server/command.go

+	p.postCommandResponse(args, string(data))
+}
+
+func (p *Plugin) checkCommandPermission(args *model.CommandArgs) bool {


I'm inclined to change the signature to

Suggested change

func (p *Plugin) checkCommandPermission(args *model.CommandArgs) bool {

func (p *Plugin) checkCommandPermission(args *model.CommandArgs) (bool, err) {

as it's more idiomatic to return an error instead of handling it in the function directly.

New PR Will contain the signature fix, as well as the commented out section for the town square check!

I also fixed the signature of the validatePreviewPriviledges.

Those changes should be seen with the next push to this PR! :)

hanzei · 2021-08-16T13:06:19Z

server/command.go

+	isTownSquare, channelErr := p.checkIfTownSquare(args.ChannelId)
+	if channelErr != nil {
+		p.postCommandResponse(args, "Channel authorization failed: %s", channelAdminErr)
+		return true
+	}
+	if !isSysadmin && !isTeamAdmin && isChannelAdmin && isTownSquare {
+		p.postCommandResponse(args, "/welcomebot commands cannot be executed by a channel admin in Town Square")
+		return true
+	}


I'm leaning toward removing the town-square restriction. Let's start simple for now.

hanzei · 2021-08-16T13:07:55Z

server/command.go

+
+	validPrivileges := p.validatePreviewPrivileges(teamID, args)
+	if !validPrivileges {
+		return


Similar as in a comment above I think the signature should be (bool, error)

hanzei

Thanks for updating the PR. https://github.com/mattermost/mattermost-plugin-welcomebot/pull/81/files#r673361233 seems unresolved to me.

CalebMaftei · 2021-08-17T15:58:31Z

Sorry about that. I thought that got pushed up. I will commit and push that change now. It is commented out currently. If you would like, I can remove it all together as well.

… from README.md

mattermod · 2021-08-28T01:01:18Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

hanzei · 2021-09-02T08:44:00Z

README.md

+<!-- ## How to Dynamically Assign Team Messages
+
+This plugin also allows system admins and team admins to change the team message of an appropriate team through a slash command, rather than adjusting the config.json. 
+
+We have provided three options that are formatted identically to the channel welcomes:
+- `/welcomebot get_team_welcome` - prints the current team's welcome message if either the dynamic message exists, or the config.json message exists.
+- `/welcomebot set_team_welcome [welcome message]` - sets the current team's welcome message to the one defined in the slash command.
+- `/welcomebot delete_team_welcome` - deletes the current team's welcome message. This however does NOT delete any messages set inside of the config.json.
+
+For saving these messages, they are stored as KV pairs. Team message pairs have the prefix `teammsg_<team_id>` to distinguish them from the `chanmsg_<channel_id>` pairs. -->


Suggested change



## How to Dynamically Assign Team Messages

This plugin also allows system admins and team admins to change the team message of an appropriate team through a slash command, rather than adjusting the config.json.

We have provided three options that are formatted identically to the channel welcomes:

- `/welcomebot get_team_welcome` - prints the current team's welcome message if either the dynamic message exists, or the config.json message exists.

- `/welcomebot set_team_welcome [welcome message]` - sets the current team's welcome message to the one defined in the slash command.

- `/welcomebot delete_team_welcome` - deletes the current team's welcome message. This however does NOT delete any messages set inside of the config.json.

hanzei · 2021-09-02T08:44:56Z

server/hooks.go

@@ -68,10 +107,12 @@ func (p *Plugin) UserHasJoinedChannel(c *plugin.Context, channelMember *model.Ch
 	// We send a DM and an opportunistic ephemeral message to the channel. See
 	// the discussion at the link below for more details:
 	// https://github.com/mattermost/mattermost-plugin-welcomebot/pull/31#issuecomment-611691023
+	channelAddMessage := "# Welcome to the " + channelInfo.DisplayName + " channel. \n\n"


Yes, please throw it out.

hanzei · 2021-09-02T08:46:02Z

server/hooks.go

+	// Commented out in case we do not want the user immediately greeted with an ephemeral message
+	// postChannel := &model.Post{
+	// 	UserId:    p.botUserID,
+	// 	ChannelId: channelMember.ChannelId,
+	// 	Message:   string(data),
+	// }
+
+	// if actor.Id == channelMember.UserId {
+	// 	time.Sleep(3 * time.Second)
+	// } else {
+	// 	time.Sleep(15 * time.Second)
+	// }
+
+	// _ = p.API.SendEphemeralPost(channelMember.UserId, postChannel)


Why does this PR touch this code? Is it related to the changes you are doing here?

caleb.maftei added 2 commits June 29, 2021 13:21

adding new slash command options and updated README.md with their roles

5e6b1f8

removing unecessary import

7c18f2e

CalebMaftei requested a review from jfrerich as a code owner June 29, 2021 21:02

hanzei added the 2: Dev Review Requires review by a core committer label Jun 30, 2021

hanzei self-requested a review June 30, 2021 10:49

CalebMaftei closed this Jul 6, 2021

CalebMaftei reopened this Jul 7, 2021

hanzei added this to the v1.3.0 milestone Jul 16, 2021

hanzei suggested changes Jul 16, 2021

View reviewed changes

caleb.maftei added 2 commits July 20, 2021 15:53

Changes per PR with Mattermost people

8eb4b00

fixing linter issues

1f9dcd8

mattermod added the Lifecycle/1:stale label Jul 31, 2021

CalebMaftei requested a review from hanzei August 3, 2021 17:25

jfrerich suggested changes Aug 4, 2021

View reviewed changes

jasonblais removed the Lifecycle/1:stale label Aug 5, 2021

fixed grammar, and unecessary user messages to conform to Peer review

c7899d8

hanzei reviewed Aug 16, 2021

View reviewed changes

hanzei suggested changes Aug 16, 2021

View reviewed changes

caleb.maftei added 2 commits August 17, 2021 09:59

changed signatures to be more idiomatic. Removed unecessary paragraph…

1b04b6f

… from README.md

fixing linter issues with an unused function

04e2dcd

mattermod added the Lifecycle/1:stale label Aug 28, 2021

hanzei self-requested a review August 28, 2021 05:39

hanzei suggested changes Sep 2, 2021

View reviewed changes

hanzei added the Awaiting Submitter Action Blocked on the author label Sep 21, 2021

DHaussermann modified the milestones: v1.3.0, v1.4.0 May 3, 2023

	unsetMessageError = "welcome message has not been set yet"
	unsetMessageError = "welcome message has not been set"

		if len(parameters) > 0 {
		return "`delete_channel_welcome` command does not accept any extra parameters"

	// No dynamic welcome message for the given team, so we check if one as been set in the config.json
	// No dynamic welcome message for the given team, so we check if one has been set in the config.json

	// This retrives a map of team Ids with their respective welcome message
	// This retrieves a map of team Ids with their respective welcome message

	p.postCommandResponse(args, "stored the welcome message:\n%s", message)
	p.postCommandResponse(args, "stored the team welcome message:\n%s", message)

	p.postCommandResponse(args, "welcome message has been deleted")
	p.postCommandResponse(args, "team welcome message has been deleted")

	func (p Plugin) checkCommandPermission(args model.CommandArgs) bool {
	func (p Plugin) checkCommandPermission(args model.CommandArgs) (bool, err) {

GH-42 #81

Are you sure you want to change the base?

GH-42 #81

Conversation

CalebMaftei commented Jun 29, 2021

Summary

Ticket Link

mattermod commented Jun 29, 2021

CalebMaftei commented Jul 6, 2021

hanzei commented Jul 7, 2021

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CalebMaftei Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattermod commented Jul 31, 2021

jfrerich left a 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

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

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

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

hanzei Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

hanzei left a comment

Choose a reason for hiding this comment

CalebMaftei commented Aug 17, 2021 • edited Loading

mattermod commented Aug 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CalebMaftei Jul 16, 2021 •

edited

Loading

hanzei Aug 16, 2021 •

edited

Loading

CalebMaftei commented Aug 17, 2021 •

edited

Loading