-
Notifications
You must be signed in to change notification settings - Fork 148
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-512,513] Fixed issue #512 and #513 on github plugin 'Updated subscription success messages' #661
Conversation
…gin (#25) * [MI-2904]:Fixed issue mattermost#512 and mattermost#513 on github plugin * [MI-2904]:Fixed review comments * [MI-2904]:Fixed review comments
Hello @Kshitij-Katiyar, 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. |
server/plugin/command.go
Outdated
@@ -266,15 +287,14 @@ func (p *Plugin) handleSubscriptionsList(_ *plugin.Context, args *model.CommandA | |||
} | |||
|
|||
func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs, parameters []string, userInfo *GitHubUserInfo) string { | |||
defaultEvents := Features("pulls,issues,creates,deletes") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should even be a global variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(suggestion) perhaps defaultEvents
is a bit misleading, as it's a mutable variable that can change later on in the function, so it's not always holding the default events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server/plugin/command.go
Outdated
@@ -266,15 +287,14 @@ func (p *Plugin) handleSubscriptionsList(_ *plugin.Context, args *model.CommandA | |||
} | |||
|
|||
func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs, parameters []string, userInfo *GitHubUserInfo) string { | |||
defaultEvents := Features("pulls,issues,creates,deletes") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(suggestion) perhaps defaultEvents
is a bit misleading, as it's a mutable variable that can change later on in the function, so it's not always holding the default events.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #661 +/- ##
==========================================
- Coverage 15.70% 15.42% -0.29%
==========================================
Files 15 15
Lines 5660 5595 -65
==========================================
- Hits 889 863 -26
+ Misses 4729 4690 -39
Partials 42 42 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, mainly just a few requests for readability. Thanks @Kshitij-Katiyar!
} | ||
|
||
func (p *Plugin) getSubscribedFeatures(channelID, owner, repo string) (Features, error) { | ||
var previousFeatures Features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this named previousFeatures
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are Features or events of an subscription which are going to be overwritten due to creation of new subscription.
msg := fmt.Sprintf("Successfully subscribed to [%s](%s).", repo, repoLink) | ||
msg := fmt.Sprintf("@%v subscribed this channel to [%s/%s](%s) with the following events: %s", user.Username, owner, repo, repoLink, subscriptionEvents.FormattedString()) | ||
if previousSubscribedEvents != "" { | ||
msg += previousSubscribedEventMessage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a space between the two concatenated strings? Same for other concatenations here in this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mickmister This is how it looks
server/plugin/command.go
Outdated
subscriptionSuccess := fmt.Sprintf("@%v subscribed this channel to [%s](%s) with the following events: %s.", user.Username, owner, orgLink, subscriptionEvents.FormattedString()) | ||
|
||
if previousSubscribedEvents != "" { | ||
subscriptionSuccess += previousSubscribedEventMessage | ||
} | ||
|
||
return fmt.Sprintf("Successfully subscribed to organization %s.", owner) | ||
if appErr = p.CreatePost(args.ChannelId, p.BotUserID, subscriptionSuccess); appErr != nil { | ||
return fmt.Sprintf("%s error creating the public post: %s", subscriptionSuccess, appErr.Error()) | ||
} | ||
|
||
return "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized this is going to be really noisy if the admin subscribes to several projects in succession. I prefer having those as ephemeral instead of public, but that is just my own preference.
Is it part of this ticket to make this message public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hanzei Please refer to this discussion #513 (comment)
@hanzei Fixed the merged conflicts |
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
@Kshitij-Katiyar Would you please merge |
…ub into MM-512,513
@hanzei synced with master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and Approved.
Subscriptions message is reflected fine for new subscriptions. Also, events and usernames are also reflected in the subscription message.
Working fine, LGTM.
@Kshitij-Katiyar Would you please resolve the conflicts? |
…hub into MM-512,513
…hub into MM-512,513
@hanzei I've done it. |
Summary
If a user creates a new subscription for a repo or organization for which a subscription already exists, the events of the previous subscription get overwritten. So now added the list of events that got overwritten in the subscription message.
Added additional information like the username, events, etc in the subscription message.
Issue #512
Issue #513