[mm-507] Add support for DM subscriptions based on "watching" an issue in Jira (#507) #792

jupriano · 2021-08-04T08:34:40Z

… (#507)

add command setting watching
add new client API for GetWatchers
add function to add notification for the user who enables watching in setting

…mattermost#507) * add command setting watching * add new client api for GetWatchers * add function to add notification for the user who enable watching in setting

mattermod · 2021-08-04T08:34:43Z

Hello @vicky-demansol,

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.

Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project.

Please help complete the Mattermost contribution license agreement?
Once you have signed the CLA, please comment with /check-cla and confirm that the CLA check is green.

This is a standard procedure for many open source projects.

Please let us know if you have any questions.

We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team.

codecov-commenter · 2021-08-04T08:39:19Z

Codecov Report

Merging #792 (681eff5) into master (5afe253) will decrease coverage by 0.42%.
The diff coverage is 11.38%.

@@            Coverage Diff             @@
##           master     #792      +/-   ##
==========================================
- Coverage   34.93%   34.51%   -0.43%     
==========================================
  Files          52       52              
  Lines        5945     6038      +93     
==========================================
+ Hits         2077     2084       +7     
- Misses       3659     3744      +85     
- Partials      209      210       +1

Impacted Files	Coverage Δ
server/client.go	`8.75% <0.00%> (-0.19%)`	⬇️
server/command.go	`18.09% <0.00%> (-0.05%)`	⬇️
server/kv.go	`19.79% <0.00%> (-0.26%)`	⬇️
server/settings.go	`43.47% <0.00%> (-36.53%)`	⬇️
server/webhook.go	`23.07% <0.00%> (-9.15%)`	⬇️
server/webhook_worker.go	`0.00% <0.00%> (ø)`
server/plugin.go	`15.81% <13.15%> (-0.58%)`	⬇️
server/user.go	`27.58% <81.81%> (+1.83%)`	⬆️
server/subscribe.go	`67.48% <0.00%> (+0.37%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5afe253...681eff5. Read the comment docs.

mickmister

Great work @vicky-demansol! I'm really looking forward to this feature 👍

I have some requests regarding:

Checking watchers before calling PostNotifications
Only fetch watchers once
Other requests regarding processing watchers

server/client.go

mickmister · 2021-08-05T19:37:06Z

server/user.go

@@ -44,6 +44,7 @@ func (c *Connection) JiraAccountID() types.ID {

 type ConnectionSettings struct {
 	Notifications bool `json:"notifications"`
+	Watching      bool `json:"watching"`


Can we make this *bool, and use a similar strategy as described here? I'd like users to be able to get these notifications without enabling this.

@DHaussermann Maybe we should be clear about why the user got the notification, and note that they can opt-out of this feature? If we have it off by default, I think it won't be discovered by most users.

@mickmister I've pushed a new commit for this change, could you please check it?

@DHaussermann @aaronrothschild Please see my comment above

If the current settings for a user has notifications "on" before this PR, should we assume the user also wants reporter and watcher notifications when we introduce the changes from this PR? Should we tell the user how to change their settings? Maybe we only tell them how to change their settings the next time they get a notification. We tell them how to change their settings, and don't remind them again.

Should we tell the user the reason they are getting a notification? It would be one of the following reasons:

Mentioned

Assignee

Reporter

Watching

mickmister · 2021-08-05T19:39:25Z

server/webhook.go

@@ -199,3 +202,79 @@ func (p *Plugin) GetWebhookURL(jiraURL string, teamID, channelID string) (subURL

 	return subURL, legacyURL, nil
 }
+
+func (wh *webhook) getConnection(p *Plugin, instance Instance, notification webhookUserNotification) (con *Connection, err error) {


Does this method need to be defined on *webhook? Is something similar already implemented on the *Plugin struct?

@vicky-demansol I don't think this method needs to be defined on *webhook. Can we just call an existing method to fetch the connection instead?

server/webhook.go

server/settings.go

mattermod · 2021-08-16T01:01:02Z

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

@vicky-demansol thanks for the PR! This is going to be a great new feature! I've left some suggestions and one question for @mickmister. Do you need any help with the suggestions from @mickmister?

server/client.go

server/settings.go

…k in the URL (mattermost#773) * change variable name from issue to watcher in server/client.go * change variable type from bool to pointer bool in server/user.go * add constants for error store new settings and error connect to Jira * move CheckWatcherUser before PostNotifications (server/webhook_worker.go) * remove LoadConnection after StoreConnection (server/settings.go)

maisnamrajusingh · 2021-08-20T05:23:41Z

@mickmister @vicky-demansol has signed the CLA and I can see his name on the CLA spreadsheet but for some reason it still says he needs to sign the CLA. Could you please take a look?

mickmister · 2021-08-20T18:33:32Z

/check-cla

mickmister · 2021-08-20T18:34:49Z

I can see his name on the CLA spreadsheet but for some reason it still says he needs to sign the CLA. Could you please take a look?

@maisnamrajusingh When this occurs, you can create a /check-cla comment like I've done above, and the system will check the CLA system again

mickmister

Thanks for implementing the requested changes @vicky-demansol 👍

I've requested some more changes. For some reason GitHub isn't letting me reply to my older comments so I had to create new threads.

server/webhook.go

mickmister · 2021-08-20T18:54:56Z

server/webhook.go

+				message:       message,
+				postType:      postType,


What happens here when the wh.notifications array is empty and message is blank? How do we access the message to create a notification? Maybe we need to call a function to generate a message for this webhook call.

@mickmister I've pushed a new commit for this change, could you please check it?

I put the same logic with func PostNotifications

mattermost-plugin-jira/server/webhook.go

Lines 100 to 103 in 6678cae

func (wh *webhook) PostNotifications(p *Plugin, instanceID types.ID) ([]*model.Post, int, error) {

if len(wh.notifications) == 0 {

return nil, http.StatusOK, nil

}

See #792 (comment)

mickmister · 2021-08-20T18:55:37Z

server/webhook.go

+			}
+
+			// if setting watching value is false don't put into webhookUserNotification
+			if err != nil || c.Settings == nil || (c.Settings.Watching != nil && !*c.Settings.Watching) {


I'd like to encapsulate part of this logic in its own method for backwards compatibility:

Suggested change

if err != nil || c.Settings == nil || (c.Settings.Watching != nil && !*c.Settings.Watching) {

if err != nil || c.Settings == nil || ! c.Settings.ShouldReceiveWatcherNotifications() {

func (s *ConnectionSettings) ShouldReceiveWatcherNotifications() bool { if s.Watching != nil { return *s.Watching } // Check old setting for backwards compatibility return s.Notifications }

There would then be an equivalent for assignee and mention notifications. Please see #705 (comment) for my explanation on introducing new notification settings. Whichever PR is merged first should have separate notification settings for assignee and mentions.

Also, can we move these two if blocks above, before we instantiate the webhookUserNotification?

@mickmister I've pushed a new commit for this change, could you please check it?

Notice my comment above:

Whichever PR is merged first should have separate notification settings for assignee and mentions.

Which means this PR should have separate notification settings for assignee and mention notifications.

mickmister · 2021-08-20T19:04:22Z

server/user.go

@@ -43,7 +43,8 @@ func (c *Connection) JiraAccountID() types.ID {
 }

 type ConnectionSettings struct {
-	Notifications bool `json:"notifications"`
+	Notifications bool  `json:"notifications"`
+	Watching      *bool `json:"watching"`


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

func (s *ConnectionSettings) String() string {

notifications := "off"

if s != nil && s.Notifications {

notifications = "on"

}

return fmt.Sprintf("\tNotifications: %s", notifications)

}

@mickmister I've pushed a new commit for this change, could you please check it?

In this PR, can we also add separate settings for assignee and mention notifications, as explained here #705 (comment)? The Reporter notifications will not be included here however.

server/webhook_worker.go

server/settings.go

mickmister · 2021-08-20T19:20:33Z

server/client.go

@@ -195,6 +196,15 @@ func (client JiraClient) GetIssue(key string, options *jira.GetQueryOptions) (*j
 	return issue, nil
 }

+// GetWatchers returns an array of Jira watchers for a given issue.


Suggested change

// GetWatchers returns an array of Jira watchers for a given issue.

// GetWatchers returns an array of Jira users watching a given issue.

@mickmister I've pushed a new commit for this change, could you please check it?

The new text you've committed says "Jira users watching for a given issue.", but I think it should be "Jira users watching a given issue."

server/settings.go

…k in the URL (mattermost#773) * bug fixing return on/off in fun settingsWatching * fixing wording in settings.go * change variable name from CheckWatcherUser to CheckIssueWatchers (webhook_worker.go) * display logic of watching in ConnectionSettings

…k in the URL (mattermost#773) * handling wh.notifications array is empty * fixing shouldNotReceiveNotification condition

jfrerich

Approving on my end, but comments from @mickmister still need to be resolved. Thanks for the PR, @vicky-demansol!

mickmister

Great work @vicky-demansol, I have a few more requests on the PR

server/user.go

mickmister · 2021-09-10T06:16:05Z

server/user.go

@@ -43,7 +43,8 @@ func (c *Connection) JiraAccountID() types.ID {
 }

 type ConnectionSettings struct {
-	Notifications bool `json:"notifications"`
+	Notifications bool  `json:"notifications"`
+	Watching      *bool `json:"watching"`


In this PR, can we also add separate settings for assignee and mention notifications, as explained here #705 (comment)? The Reporter notifications will not be included here however.

server/settings.go

mickmister · 2021-09-10T06:23:18Z

server/webhook.go

+
+	instance, err := p.instanceStore.LoadInstance(instanceID)
+	if err != nil {
+		// This isn't an internal server error. There's just no instance installed.


This isn't an internal server error. There's just no instance installed.

How do you know this is true? It looks like there are several kinds of errors that can happen in LoadInstance. Can we make it so the LoadInstance function returns a nil error, and nil for instance, so we know whether it was a real error if there is no instance?

@vicky-demansol This request hasn't been addressed yet the comment was marked as resolved

@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

server/webhook.go

mickmister · 2021-09-10T06:38:55Z

server/webhook.go

+	if len(wh.notifications) == 0 {
+		return
+	}


I don't think we should return here. This just means no users will receive assignee or mention notifications, but we should still look at which users are watching the issue.

@mickmister are i just need to delete the statement or add something to ?

We should delete this if block

mickmister · 2021-09-10T06:43:21Z

@vicky-demansol Please take a look at all comments on the PR that are still "Unresolved"

server/command_test.go

sibasankarnayak · 2021-11-29T15:02:07Z

server/kv.go

 	if err != nil {
+		if errors.Is(errors.Wrap(kvstore.ErrNotFound, hashedKey), err) {
+			return nil, nil


@vicky-demansol can we log the error and in below case too.

sibasankarnayak · 2021-11-29T15:02:29Z

server/plugin.go

+
+	instanceID, err := p.ResolveWebhookInstanceURL(jiraURL)
+	if err != nil {
+		return "", "", err


adding error log will help

done i was add the logging

server/plugin.go

sibasankarnayak · 2021-11-29T15:05:37Z

server/plugin.go

+	con, err = p.userStore.LoadConnection(instance.GetID(), mattermostUserID)
+	if err != nil {
+		// Not connected to Jira, so can't check permissions
+		return


seems logging and returning error can help to understand better the situation

done i was add the logging

server/user_test.go

sibasankarnayak

few suggestions and request to be made from my side

Co-authored-by: sibasankarnayak <86650698+sibasankarnayak@users.noreply.github.com>

jupriano · 2021-12-01T10:23:47Z

server/plugin.go

+		mattermostUserID, err = p.userStore.LoadMattermostUserID(instance.GetID(), notification.jiraUsername)
+	}
+	if err != nil {
+		return


i think error was declare as err in function return and it will return if it has error value

Co-authored-by: sibasankarnayak <86650698+sibasankarnayak@users.noreply.github.com>

hanzei · 2022-01-24T21:13:17Z

@mickmister Gentle reminder to review the PR

sibasankarnayak · 2022-02-05T14:04:06Z

@mickmister @hanzei will be taking up this Issue from here to complete it asap, will create a new PR and close it once i create new one

Add support for DM subscriptions based on "watching" an issue in Jira (…

397c5fd

…mattermost#507) * add command setting watching * add new client api for GetWatchers * add function to add notification for the user who enable watching in setting

jupriano requested a review from mickmister as a code owner August 4, 2021 08:34

mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Aug 5, 2021

mickmister requested a review from jfrerich August 5, 2021 19:31

mickmister suggested changes Aug 5, 2021

View reviewed changes

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

jfrerich suggested changes Aug 19, 2021

View reviewed changes

server/client.go Outdated Show resolved Hide resolved

server/settings.go Outdated Show resolved Hide resolved

server/settings.go Outdated Show resolved Hide resolved

mickmister suggested changes Aug 20, 2021

View reviewed changes

hanzei added the Awaiting Submitter Action Blocked on the author label Aug 25, 2021

jupriano added 2 commits September 2, 2021 22:13

Jira's autolink should support issue links that contain a comment lin…

2325e8e

…k in the URL (mattermost#773) * handling wh.notifications array is empty * fixing shouldNotReceiveNotification condition

jupriano requested a review from jfrerich September 7, 2021 10:14

hanzei requested a review from mickmister September 9, 2021 14:24

hanzei removed the Awaiting Submitter Action Blocked on the author label Sep 9, 2021

jfrerich approved these changes Sep 9, 2021

View reviewed changes

mickmister requested a review from catalintomai September 10, 2021 06:32

mickmister suggested changes Sep 10, 2021

View reviewed changes

mickmister reviewed Sep 10, 2021

View reviewed changes

hanzei linked an issue Sep 21, 2021 that may be closed by this pull request

Add support for DM subscriptions based on "watching" an issue in Jira #507

Open

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

mickmister added this to the v3.2.0 milestone Oct 19, 2021

jupriano added 4 commits November 25, 2021 04:59

[mm-507] Fixing Unit Test and CI lint

f980d21

[mm-507] Fixing Unit Test and CI lint

9cc2e4d

[mm-507] Remove no notification block issue watcher (mattermost#507)

028ae19

[mm-507] Loload instance error instance not found (mattermost#507)

23d01fc