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

[GH-65] #75

Closed
Closed

Conversation

maisnamrajusingh
Copy link
Contributor

@maisnamrajusingh maisnamrajusingh commented Jun 11, 2021

Summary

Resolves ticket no #65 Allows multiple teams to have the same message without repeating them

Ticket Link

Fixes #65

@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.

@maisnamrajusingh maisnamrajusingh changed the title Mm 65 [GH-65] Jun 11, 2021
@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Jun 11, 2021
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.

LGTM one question of the parsing

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@hanzei hanzei added the 3: QA Review Requires review by a QA tester label Jun 11, 2021
Comment on lines +133 to +135
for _, tn := range teamNameSlice {
teams[tn] = struct{}{}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does spaces need to also be trimmed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hanzei this won't be needed here.

server/http_hooks.go Outdated Show resolved Hide resolved
server/command.go Show resolved Hide resolved
@@ -57,7 +57,7 @@ func (p *Plugin) getSiteURL() string {
return *config.ServiceSettings.SiteURL
}

func (p *Plugin) newSampleMessageTemplate(teamName, userID string) (*MessageTemplate, error) {
func (p *Plugin) newSampleMessageTemplate(teamID string, userID string) (*MessageTemplate, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes in this file come from #74, right? Would you mind dropping them and we try on #74 go get merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hanzei Yes, doing just that. Will update you once done.

Copy link
Contributor

Choose a reason for hiding this comment

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

These change still seem to come from #74 (comment). Once you dropped them from the file, I'n happy to approve it.

@hanzei
Copy link
Contributor

hanzei commented Jun 21, 2021

@maisnamrajusingh Please let me know when this is ready for a re-review

@maisnamrajusingh
Copy link
Contributor Author

@maisnamrajusingh Please let me know when this is ready for a re-review

yup it is.

@hanzei hanzei self-requested a review June 23, 2021 14:06
@jasonblais
Copy link
Contributor

@hanzei @jfrerich quick reminder to help with reviews, thanks!

Copy link
Contributor

@jfrerich jfrerich left a comment

Choose a reason for hiding this comment

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

One non-blocking nitpick. Approving, but Ben's requests will need to be resolved before merging

server/command.go Outdated Show resolved Hide resolved
@maisnamrajusingh
Copy link
Contributor Author

@hanzei @jfrerich are there any more changes needed here ? I think I have covered all the requests.

@jfrerich
Copy link
Contributor

@maisnamrajusingh, you're good from my end. Thanks!

@@ -57,7 +57,7 @@ func (p *Plugin) getSiteURL() string {
return *config.ServiceSettings.SiteURL
}

func (p *Plugin) newSampleMessageTemplate(teamName, userID string) (*MessageTemplate, error) {
func (p *Plugin) newSampleMessageTemplate(teamID string, userID string) (*MessageTemplate, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These change still seem to come from #74 (comment). Once you dropped them from the file, I'n happy to approve it.

@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

@maisnamrajusingh
Copy link
Contributor Author

@jfrerich @hanzei I am closing this pr and I created a new one to fix the logic and overlapping code. Sorry for the inconvenience and delay.

@maisnamrajusingh maisnamrajusingh deleted the mm-65 branch July 27, 2021 08:33
@hanzei hanzei removed 2: Dev Review Requires review by a core committer Lifecycle/1:stale 3: QA Review Requires review by a QA tester labels Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow same welcomebot message for a number of MM Teams on same server without repeating message text
6 participants