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

Allows using * to configure the welcome message for all teams #80

Closed

Conversation

maisnamrajusingh
Copy link
Contributor

Summary

Allows using * to configure the welcome message for all teams

Ticket Link

#36

@mattermod
Copy link
Contributor

Hello @maisnamrajusingh,

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.

@hanzei hanzei self-requested a review June 30, 2021 10:43
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

The implementation is fine for "*".

The original ticket discussed adding support for Globbing in general and allow patterns like qa-team-*. Can this be implemented in a reasonable amount of time and without adding much complexity?

@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester and removed 3: QA Review Requires review by a QA tester labels Jun 30, 2021
@hanzei hanzei linked an issue Jun 30, 2021 that may be closed by this pull request
@maisnamrajusingh
Copy link
Contributor Author

@hanzei I think it adds more complexity to the code base, there is already another pr that would let users use "team-1,team2" etc to allow the message for multiple teams. But let me know if you feel I should look into it

Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

How would the /welcomebot preview command work with this feature?

server/hooks.go Outdated Show resolved Hide resolved
server/welcomebot.go Outdated Show resolved Hide resolved
@hanzei
Copy link
Contributor

hanzei commented Jul 1, 2021

@aaronrothschild I would love to hear your thoughts on the Globbing discussion above.

@aaronrothschild
Copy link

aaronrothschild commented Jul 1, 2021

From the original ticket "however, i would like to be able to send people a welcome-message after their first login - regardless of the team they join."

To me, there are multiple things at play here:

  • Adding the config for multiple teams vs "all" teams is two different settings IMO. ie: []allusers? or [x,y,z] teams?
  • The root issue: Send a user message WHEN THEY JOIN THE SERVER - regardless of team membership really. This is "welcomebot" so, I think it makes sense to have a different/setting message for "All New Users, on any team" (ie: a "Server Welcome Message"). The admin doesn't really want the user to see the message every time they join a team (I think) - they just want to make sure a new user sees a message when they join the server, regardless of which team. Imagine joining 5 teams and seeing the same welcome message each time - because we added this "globbing" setting, would happen a bunch more IMO. (correct me if the behavior is different than I'm describing)
  • When someone, at some point adds another team tot he server....does the "*" mean it affects ALL teams or "ALL teams in place when the welcomebot was configured last"? ie: Will an admin need to remember to add the Team to the setting in welcomebot so a user will see the welcome message if they join that new team? That presents a maintenance issue.

@hanzei

@jasonblais
Copy link
Contributor

@hanzei on above

@hanzei
Copy link
Contributor

hanzei commented Jul 16, 2021

@maisnamrajusingh #80 (review) still seems unresolved.

@hanzei
Copy link
Contributor

hanzei commented Jul 16, 2021

Thanks for your thoughts @aaronrothschild. Based on that feedback the PR needs to get updated so the welcome message triggers at user creation not team joining.

@mattermod
Copy link
Contributor

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 hanzei added the Awaiting Submitter Action Blocked on the author label Aug 12, 2021
@hanzei hanzei removed the request for review from jfrerich August 12, 2021 10:26
@maisnamrajusingh
Copy link
Contributor Author

@hanzei I assumed this ticket doesn't need to be worked on because of a misunderstanding I had when I spoke to @mickmister I'll look into this now

@maisnamrajusingh
Copy link
Contributor Author

@aaronrothschild @hanzei so looking at the comments. Does the following make sense

  1. we would add a specific field called "GlobalMessage" that would look something like this
"GlobalWelcomeMessage": "Global Welcome Message"
  1. hook into the UserHasJoinedServer ( I am assuming something like this exists) to display the message.

@aaronrothschild
Copy link

@maisnamrajusingh Yes, that sounds correct to me. Thanks!

@mickmister
Copy link
Contributor

I assumed this ticket doesn't need to be worked on because of a misunderstanding I had when I spoke to @mickmister I'll look into this now

Just to surface the miscommunication here, the original message I posted said "Note that the welcomebot App project should not be worked at the moment". I was referring specifically to this ticket #42 (comment), which is decided to be implemented as a Mattermost App.

The Welcomebot plugin should continue to be developed and improved. Sorry for this miscommunication.

Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

I doesn't seem like like #80 (comment) gets addressed in this PR, does it?

@maisnamrajusingh
Copy link
Contributor Author

maisnamrajusingh commented Sep 3, 2021

I doesn't seem like like #80 (comment) gets addressed in this PR, does it?

@hanzei using the * would produce a maintainence issue as mentioned in the comment and confirmed here
#80 (comment)

@hanzei
Copy link
Contributor

hanzei commented Sep 3, 2021

@maisnamrajusingh I don't understand what are you referring to. @mickmister talked in #80 (comment) about the possible conversion from a plugin to an app. IMO this doesn't impact this ticket at all, does it? The suggestion you made in #80 (comment) still seems reasonable to me and I would like to see that feature.

@Kshitij-Katiyar Kshitij-Katiyar added this to the v1.4.0 milestone Apr 12, 2024
@ayusht2810
Copy link
Contributor

@mickmister @hanzei @aaronrothschild We looked into the above comments and thought of an approach.
We will be adding a new field in the config GlobalWelcomeMessage to support the welcome message when a user has joined a server. Currently, we don't have any hooks present in the mattermost, which triggers when a user joins a sever, so we will be using the UserHasBeenCreated hook to post the message as soon as the user is created in the welcome bot DM.
Also, there is an edge case here: if an admin adds a config with both GlobalWelcomeMessage and TeamName (with the mattermost teamname present), we will give preference to posting the per team message in the welcome bot DM.
What are your thoughts on this approach? Let me know if I am missing something here.

@mickmister
Copy link
Contributor

Also, there is an edge case here: if an admin adds a config with both GlobalWelcomeMessage and TeamName (with the mattermost teamname present), we will give preference to posting the per team message in the welcome bot DM. What are your thoughts on this approach? Let me know if I am missing something here.

@ayusht2810 I'm thinking we should create posts for both the global and per-team in this case

@ayusht2810
Copy link
Contributor

Created PR #131 according to the above comments

@raghavaggarwal2308
Copy link

Closing because of #131

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer Awaiting Submitter Action Blocked on the author Lifecycle/1:stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow messages for *all* teams
10 participants