-
Notifications
You must be signed in to change notification settings - Fork 127
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
[mm-507] Add support for DM subscriptions based on "watching" an issue in Jira (#507) #792
Changes from 22 commits
397c5fd
626719d
d046db9
2325e8e
899017f
8b6b28b
e17c0cf
74e9b69
51f0fb2
ece396d
dd55c1d
e3e5502
29e4ed3
0b02444
f980d21
9cc2e4d
028ae19
23d01fc
e1e669d
702c079
24eaa07
73373d1
29a812d
9d72ffe
ae2b7f2
65f0e66
681eff5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ import ( | |
"github.com/mattermost/mattermost-plugin-jira/server/tracker" | ||
"github.com/mattermost/mattermost-plugin-jira/server/utils" | ||
"github.com/mattermost/mattermost-plugin-jira/server/utils/telemetry" | ||
"github.com/mattermost/mattermost-plugin-jira/server/utils/types" | ||
) | ||
|
||
const ( | ||
|
@@ -425,3 +426,64 @@ func (p *Plugin) CheckSiteURL() error { | |
} | ||
return nil | ||
} | ||
|
||
func (p *Plugin) GetWebhookURL(jiraURL string, teamID, channelID string) (subURL, legacyURL string, err error) { | ||
cf := p.getConfig() | ||
|
||
instanceID, err := p.ResolveWebhookInstanceURL(jiraURL) | ||
if err != nil { | ||
return "", "", err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adding error log will help There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done i was add the logging |
||
} | ||
|
||
team, appErr := p.API.GetTeam(teamID) | ||
if appErr != nil { | ||
return "", "", appErr | ||
} | ||
|
||
channel, appErr := p.API.GetChannel(channelID) | ||
if appErr != nil { | ||
return "", "", appErr | ||
} | ||
|
||
v := url.Values{} | ||
secret, _ := url.QueryUnescape(cf.Secret) | ||
v.Add("secret", secret) | ||
subURL = p.GetPluginURL() + instancePath(routeAPISubscribeWebhook, instanceID) + "?" + v.Encode() | ||
|
||
// For the legacy URL, add team and channel. Secret is already in the map. | ||
v.Add("team", team.Name) | ||
v.Add("channel", channel.Name) | ||
legacyURL = p.GetPluginURL() + instancePath(routeIncomingWebhook, instanceID) + "?" + v.Encode() | ||
|
||
return subURL, legacyURL, nil | ||
} | ||
func newWebhook(jwh *JiraWebhook, eventType string, format string, args ...interface{}) *webhook { | ||
return &webhook{ | ||
JiraWebhook: jwh, | ||
eventTypes: NewStringSet(eventType), | ||
headline: jwh.mdUser() + " " + fmt.Sprintf(format, args...) + " " + jwh.mdKeySummaryLink(), | ||
} | ||
} | ||
|
||
func (p *Plugin) getConnection(instance Instance, notification webhookUserNotification) (con *Connection, err error) { | ||
var mattermostUserID types.ID | ||
|
||
// prefer accountId to username when looking up UserIds | ||
if notification.jiraAccountID != "" { | ||
mattermostUserID, err = p.userStore.LoadMattermostUserID(instance.GetID(), notification.jiraAccountID) | ||
} else { | ||
mattermostUserID, err = p.userStore.LoadMattermostUserID(instance.GetID(), notification.jiraUsername) | ||
} | ||
if err != nil { | ||
return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think error was declare as err in function return and it will return if it has error value |
||
} | ||
|
||
// Check if the user has permissions. | ||
con, err = p.userStore.LoadConnection(instance.GetID(), mattermostUserID) | ||
if err != nil { | ||
// Not connected to Jira, so can't check permissions | ||
return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems logging and returning error can help to understand better the situation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done i was add the logging |
||
} | ||
|
||
return | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -43,15 +43,30 @@ func (c *Connection) JiraAccountID() types.ID { | |||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
type ConnectionSettings struct { | ||||||||||||||||
Notifications bool `json:"notifications"` | ||||||||||||||||
Notifications bool `json:"notifications"` | ||||||||||||||||
Watching *bool `json:"watching"` | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll need to update display logic of settings to include this new setting. It needs to included added here: mattermost-plugin-jira/server/user.go Lines 49 to 55 in 6678cae
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mickmister I've pushed a new commit for this change, could you please check it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this PR, can we also add separate settings for assignee and mention notifications, as explained here #705 (comment)? The |
||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
func (s *ConnectionSettings) String() string { | ||||||||||||||||
notifications := "off" | ||||||||||||||||
if s != nil && s.Notifications { | ||||||||||||||||
notifications = "on" | ||||||||||||||||
notifications, watching := settingOff, settingOff | ||||||||||||||||
if s != nil { | ||||||||||||||||
if s.Notifications { | ||||||||||||||||
notifications = "on" | ||||||||||||||||
} | ||||||||||||||||
if s.ShouldReceiveWatcherNotifications() { | ||||||||||||||||
watching = "on" | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
return fmt.Sprintf("- Notifications: %s\n- Watching: %s", notifications, watching) | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
func (s *ConnectionSettings) ShouldReceiveWatcherNotifications() bool { | ||||||||||||||||
if s.Watching != nil { | ||||||||||||||||
return *s.Watching | ||||||||||||||||
} | ||||||||||||||||
return fmt.Sprintf("\tNotifications: %s", notifications) | ||||||||||||||||
|
||||||||||||||||
// Check old setting for backwards compatibility | ||||||||||||||||
return s.Notifications | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
func NewUser(mattermostUserID types.ID) *User { | ||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -6,7 +6,6 @@ package main | |||
import ( | ||||
"fmt" | ||||
"net/http" | ||||
"net/url" | ||||
|
||||
"github.com/pkg/errors" | ||||
|
||||
|
@@ -26,6 +25,7 @@ type Webhook interface { | |||
Events() StringSet | ||||
PostToChannel(p *Plugin, instanceID types.ID, channelID, fromUserID, subscriptionName string) (*model.Post, int, error) | ||||
PostNotifications(p *Plugin, instanceID types.ID) ([]*model.Post, int, error) | ||||
CheckIssueWatchers(p *Plugin, instanceID types.ID) | ||||
} | ||||
|
||||
type webhookField struct { | ||||
|
@@ -161,41 +161,59 @@ func (wh *webhook) PostNotifications(p *Plugin, instanceID types.ID) ([]*model.P | |||
return posts, http.StatusOK, nil | ||||
} | ||||
|
||||
func newWebhook(jwh *JiraWebhook, eventType string, format string, args ...interface{}) *webhook { | ||||
return &webhook{ | ||||
JiraWebhook: jwh, | ||||
eventTypes: NewStringSet(eventType), | ||||
headline: jwh.mdUser() + " " + fmt.Sprintf(format, args...) + " " + jwh.mdKeySummaryLink(), | ||||
func (wh *webhook) CheckIssueWatchers(p *Plugin, instanceID types.ID) { | ||||
instance, err := p.instanceStore.LoadInstance(instanceID) | ||||
if err != nil && instance == nil { | ||||
// This isn't an internal server error. There's just no instance installed. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
How do you know this is true? It looks like there are several kinds of errors that can happen in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vicky-demansol This request hasn't been addressed yet the comment was marked as resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mickmister @vicky-demansol is doing just that. But if it returns nil, do we log it here mattermost-plugin-jira/server/webhook.go Line 204 in 2325e8e
|
||||
return | ||||
} | ||||
} | ||||
|
||||
func (p *Plugin) GetWebhookURL(jiraURL string, teamID, channelID string) (subURL, legacyURL string, err error) { | ||||
cf := p.getConfig() | ||||
|
||||
instanceID, err := p.ResolveWebhookInstanceURL(jiraURL) | ||||
c, err := p.getConnection(instance, webhookUserNotification{ | ||||
jiraAccountID: wh.JiraWebhook.User.AccountID, | ||||
jiraUsername: wh.JiraWebhook.User.Name, | ||||
}) | ||||
if err != nil { | ||||
return "", "", err | ||||
return | ||||
} | ||||
|
||||
team, appErr := p.API.GetTeam(teamID) | ||||
if appErr != nil { | ||||
return "", "", appErr | ||||
client, err := instance.GetClient(c) | ||||
if err != nil { | ||||
return | ||||
} | ||||
|
||||
channel, appErr := p.API.GetChannel(channelID) | ||||
if appErr != nil { | ||||
return "", "", appErr | ||||
watcherUsers, err := client.GetWatchers(wh.Issue.ID) | ||||
if err != nil { | ||||
return | ||||
} | ||||
|
||||
v := url.Values{} | ||||
secret, _ := url.QueryUnescape(cf.Secret) | ||||
v.Add("secret", secret) | ||||
subURL = p.GetPluginURL() + instancePath(routeAPISubscribeWebhook, instanceID) + "?" + v.Encode() | ||||
for _, watcherUser := range *watcherUsers { | ||||
postType := "" | ||||
message := "" | ||||
var shouldNotReceiveNotification bool | ||||
for _, notification := range wh.notifications { | ||||
if notification.jiraAccountID == watcherUser.AccountID || (notification.jiraUsername != "" && notification.jiraUsername == watcherUser.Name) { | ||||
shouldNotReceiveNotification = true | ||||
break | ||||
} | ||||
postType = notification.postType | ||||
message = notification.message | ||||
jupriano marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
} | ||||
if shouldNotReceiveNotification { | ||||
continue | ||||
} | ||||
whUserNotification := webhookUserNotification{ | ||||
jiraUsername: watcherUser.Name, | ||||
jiraAccountID: watcherUser.AccountID, | ||||
message: message, | ||||
postType: postType, | ||||
commentSelf: wh.JiraWebhook.Comment.Self, | ||||
} | ||||
|
||||
// For the legacy URL, add team and channel. Secret is already in the map. | ||||
v.Add("team", team.Name) | ||||
v.Add("channel", channel.Name) | ||||
legacyURL = p.GetPluginURL() + instancePath(routeIncomingWebhook, instanceID) + "?" + v.Encode() | ||||
c, err = p.getConnection(instance, whUserNotification) | ||||
|
||||
return subURL, legacyURL, nil | ||||
// if setting watching value is false don't put into webhookUserNotification | ||||
if err != nil || c.Settings == nil || !c.Settings.ShouldReceiveWatcherNotifications() { | ||||
continue | ||||
} | ||||
|
||||
wh.notifications = append(wh.notifications, whUserNotification) | ||||
} | ||||
} |
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.
@vicky-demansol can we log the error and in below case too.