[mm 699] - Reporter should receive DM when someone comments on their issue Fixes #699 #810

sibasankarnayak · 2021-09-29T17:26:29Z

This PR modifies appendCommentNotifications so both Assignee and Reporter will get a notification when someone comments on their issue (previously, only Assignee gets the notification)

ticket here
Fixes #699

mattermod · 2021-09-29T17:26:33Z

Hello @sibasankarnayak,

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.

codecov-commenter · 2021-09-29T17:40:02Z

Codecov Report

Merging #810 (9f1e96f) into master (c6a6f00) will decrease coverage by 0.02%.
The diff coverage is 47.46%.

@@            Coverage Diff             @@
##           master     #810      +/-   ##
==========================================
- Coverage   34.81%   34.79%   -0.03%     
==========================================
  Files          52       52              
  Lines        5949     6059     +110     
==========================================
+ Hits         2071     2108      +37     
- Misses       3666     3733      +67     
- Partials      212      218       +6

Impacted Files	Coverage Δ
server/command.go	`17.84% <0.00%> (-0.29%)`	⬇️
server/subscribe.go	`66.85% <ø> (ø)`
server/user_cloud.go	`0.00% <0.00%> (ø)`
server/user_server.go	`7.29% <0.00%> (-0.32%)`	⬇️
server/utils.go	`19.76% <0.00%> (ø)`
server/webhook_worker.go	`0.00% <0.00%> (ø)`
server/webhook.go	`29.65% <25.45%> (-2.57%)`	⬇️
server/webhook_parser.go	`83.33% <78.78%> (-1.90%)`	⬇️
server/settings.go	`78.04% <91.30%> (-1.96%)`	⬇️
server/user.go	`29.54% <100.00%> (+3.79%)`	⬆️
... and 2 more

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 c6a6f00...9f1e96f. Read the comment docs.

hanzei · 2021-09-30T07:53:43Z

@sibasankarnayak Please link the ticket this PR fixes uses the Fixes #X syntax as described in https://docs.google.com/document/d/1JpTkyjwsdbacV83NXgo8gRA_Zvcjk4DkCRFoiGBUVBs/edit.

server/webhook_worker.go

mattermod · 2021-10-30T01:01:20Z

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

sibasankarnayak · 2021-11-02T14:50:04Z

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

@levb @mickmister can you review this

mickmister · 2021-11-02T18:00:08Z

can you review this

Yes, I will review the PR soon. Thanks for pinging us.

@sibasankarnayak FYI, by quoting the full comment above you've pinged other people that don't need to be pinged in this case. Especially when the quoted comment gets several levels deep, this can be an issue. Just be careful who you are pinging in your comments, and make sure it's only the people you want to communicate with. Thanks again for the ping to review 👍

mickmister

Functionality LGTM, just one request to make sure we don't send duplicate notifications

server/webhook_worker.go

mickmister · 2021-11-19T03:19:54Z

@aaronrothschild Do you think the reporter should get a notification for when the issue is updated, or only when someone comments on the issue?

aaronrothschild · 2021-11-22T07:17:02Z

@aaronrothschild Do you think the reporter should get a notification for when the issue is updated, or only when someone comments on the issue?

Personally, I like getting notified when it is updated, but 0/5

sibasankarnayak · 2022-02-16T15:37:42Z

@mickmister Can you review this PR

mickmister

Great work @sibasankarnayak! I have some requests, the main one being that we should introduce the new notification settings as *bool in order to be backwards compatible with their current preferences. Please see the following comments for context:

#705 (comment)

#792 (comment)

mickmister · 2022-02-18T15:36:55Z

go.mod

@@ -18,5 +18,5 @@ require (
 	github.com/rudderlabs/analytics-go v3.3.1+incompatible
 	github.com/stretchr/testify v1.7.0
 	github.com/trivago/tgo v1.0.7 // indirect
-	golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d
+	golang.org/x/oauth2 v0.0.0-20210628180205-a41e5a781914


The changes in this file and go.sum seem unrelated to the purpose of this PR. Can you please revert these changes?

server/command.go

server/user.go

mickmister · 2022-02-18T15:47:21Z

server/webhook.go

+	recipientTypeAssignee = "assignee"
+	recipientTypeReporter = "reporter"


Are these values used?

mickmister · 2022-02-18T15:48:19Z

server/webhook_parser.go

@@ -304,6 +304,7 @@ func appendCommentNotifications(wh *webhook, verb string) {
 		message:       fmt.Sprintf("%s **commented** on %s:\n>%s", commentAuthor, jwh.mdKeySummaryLink(), jwh.Comment.Body),
 		postType:      PostTypeComment,
 		commentSelf:   jwh.Comment.Self,
+		recipientType: recipientTypeAssignee,


If this field is never used, let's not introduce it in the PR

mickmister · 2022-02-18T15:48:50Z

server/webhook_worker.go

+
+	if _, _, err = wh.PostNotifications(ww.p, msg.InstanceID); err != nil {
+		ww.p.errorf("WebhookWorker id: %d, error posting notifications, err: %v", ww.id, err)
+	}


Yes this would be a separate issue

server/settings.go

mickmister · 2022-02-18T15:51:19Z

server/command_test.go

+			commandArgs:  &model.CommandArgs{Command: "/jira settings notifications assignee on", UserId: mockUserIDWithoutNotifications},
 			numInstances: 1,
-			expectedMsg:  "Settings updated. Notifications on.",
+			expectedMsg:  "Settings updated.\n\tAssignee on.",
 		},
 		"disable notifications": {
-			commandArgs:  &model.CommandArgs{Command: "/jira settings notifications off", UserId: mockUserIDWithNotifications},
+			commandArgs:  &model.CommandArgs{Command: "/jira settings notifications assignee off", UserId: mockUserIDWithNotifications},
 			numInstances: 1,
-			expectedMsg:  "Settings updated. Notifications off.",
+			expectedMsg:  "Settings updated.\n\tAssignee off.",


Can we add tests for the new fields as well? So there would be tests for assignee, mention, and reporter. Please let me know your thoughts

…n setting

hanzei · 2022-03-01T11:36:49Z

@sibasankarnayak Heads up that there are conflicts to resolve

sibasankarnayak · 2022-03-01T13:10:37Z

@hanzei Done , resolved conflict

hanzei · 2022-03-01T13:47:40Z

@levb @mickmister Gentle reminder to review this PR

levb · 2022-03-01T18:49:35Z

server/user.go

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

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


why pointer values? what is the difference of a nil and a false?

@levb this was a suggestion from @mickmister #705 (comment)
it seems to keep a track where the user have explicitly turned off the notification and users are still using the previous configuration.

@levb We're using pointers for backwards compatibility, so we can fall back on the user's current settings if these are nil

Why not change the semantics of the keys so that the desired default false, "DisableXXXNotifications"? Then no need for crash-prone pointers?

levb · 2022-03-01T18:50:38Z

Also, I am not sure of the user value of having 3 separate settings, seems like a single setting is sufficient... @aaronrothschild

mickmister · 2022-03-03T15:31:01Z

Also, I am not sure of the user value of having 3 separate settings, seems like a single setting is sufficient

@levb Some of the notification types are noisier than others. Maybe a given user doesn't want watcher notifications because that's too many notifications. Note that the watcher notifications is introduced in another open PR, but the principle is the same. cc @aaronrothschild

mickmister · 2022-03-09T14:27:29Z

Some of the notification types are noisier than others. Maybe a given user doesn't want watcher
notifications because that's too many notifications. Note that the watcher notifications is introduced in another open PR, but the principle is the same.

So if we factor in the new reporter notifications and watcher notifications, we have a total of 4 reasons why someone might get a notification:

Mentioned
Assignee
Reporter
Watcher

The question is if these should all be configured with one user setting /jira settings notifications, or if they should all be configured, i.e.

/jira settings notifications mention true
/jira settings notifications assignee true
/jira settings notifications reporter true
/jira settings notifications watcher true

sibasankarnayak · 2022-03-10T18:21:28Z

@mickmister made the changes as discussed over call , Please review and share the feedback.

mickmister · 2022-03-15T09:16:53Z

Since there is a lot of overlap with this PR and #840, I'm holding off on reviewing the current changes in this PR.

@sibasankarnayak What would you like to do here? Are you wanting to merge one PR before the other, or close one of the PRs?

sibasankarnayak · 2022-03-15T09:22:14Z

Since there is a lot of overlap with this PR and #840, I'm holding off on reviewing the current changes in this PR.

@sibasankarnayak What would you like to do here? Are you wanting to merge one PR before the other, or close one of the PRs?

I would like to be merge this PR and then can continue on the #840 .
Can we do that ?

sibasankarnayak · 2022-03-15T09:22:39Z

Since there is a lot of overlap with this PR and #840, I'm holding off on reviewing the current changes in this PR.
@sibasankarnayak What would you like to do here? Are you wanting to merge one PR before the other, or close one of the PRs?

I would like to be merge this PR and then can continue on the #840 . Can we do that ?

@mickmister share you comments on this.

mickmister · 2022-03-15T09:26:08Z

I would like to be merge this PR and then can continue on the #840 . Can we do that ?

@sibasankarnayak Sure. Many of my comments on #840 apply to this PR as well. Would you like me to create the same comments on this PR?

sibasankarnayak · 2022-03-15T09:36:34Z

@mickmister should we take the other PR to close both the issue as the code in this PR is still there in other PR ?

mickmister · 2022-03-15T09:39:36Z

@sibasankarnayak So you're implying we should close this PR in favor of the other PR? I'm fine with that if this is what you're suggesting

sibasankarnayak · 2022-03-15T09:40:30Z

@sibasankarnayak So you're implying we should close this PR in favor of the other PR? I'm fine with that if this is what you're suggesting

@mickmister yes i mean that ,

will be closing this and continue working on the other PR to close reporter and watching notification

added notification for reporters if present

f72da22

sibasankarnayak requested a review from mickmister as a code owner September 29, 2021 17:26

mattermod added the Contributor label Sep 29, 2021

sibasankarnayak changed the title ~~added notification for reporters if present~~ [mm 699] - Reporter should receive DM when someone comments on their issue fixes #699 Sep 29, 2021

maisnamrajusingh added 2 commits September 29, 2021 17:45

fixed import

84b6437

fix cilint import

5dde612

hanzei added the Work In Progress Not yet ready for review label Sep 30, 2021

sibasankarnayak changed the title ~~[mm 699] - Reporter should receive DM when someone comments on their issue fixes #699~~ [mm 699] - Reporter should receive DM when someone comments on their issue Fixes #699 Sep 30, 2021

sibasankarnayak changed the title ~~[mm 699] - Reporter should receive DM when someone comments on their issue Fixes #699~~ [GH 699] - Reporter should receive DM when someone comments on their issue Fixes #699 Sep 30, 2021

sibasankarnayak changed the title ~~[GH 699] - Reporter should receive DM when someone comments on their issue Fixes #699~~ [mm 699] - Reporter should receive DM when someone comments on their issue Fixes #699 Sep 30, 2021

mickmister reviewed Oct 1, 2021

View reviewed changes

server/webhook_worker.go Outdated Show resolved Hide resolved

maisnamrajusingh added 2 commits October 1, 2021 16:41

fixed imports for clint

5704c3d

removed unwanted code

5733290

sibasankarnayak removed the Work In Progress Not yet ready for review label Oct 10, 2021

sibasankarnayak requested a review from mickmister October 10, 2021 12:20

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

hanzei added the 2: Dev Review Requires review by a core committer label Oct 19, 2021

hanzei requested a review from levb October 19, 2021 18:07

hanzei added the 3: QA Review Requires review by a QA tester label Oct 19, 2021

mattermod added the Lifecycle/1:stale label Oct 30, 2021

mickmister suggested changes Nov 19, 2021

View reviewed changes

server/webhook_worker.go Outdated Show resolved Hide resolved

server/webhook_worker.go Outdated Show resolved Hide resolved

server/webhook_worker.go Outdated Show resolved Hide resolved

server/webhook_worker.go Outdated Show resolved Hide resolved

resolved comments

63efea5

mickmister suggested changes Feb 18, 2022

View reviewed changes

sibasankarnayak and others added 4 commits February 21, 2022 14:56

worked on adding test case and backward compatibility for notificatio…

35fd3be

…n setting

revert unwanted change

b964793

Merge branch 'master' into mm-699

03a5d40

revert changes

a274682

sibasankarnayak requested a review from mickmister February 21, 2022 15:14

sibasankarnayak removed the Lifecycle/1:stale label Feb 21, 2022

sibasankarnayak added 2 commits February 21, 2022 15:41

fix lint issue

604c972

fix lint issue

9f1e96f

hanzei modified the milestones: v3.2.0, v3.3.0 Mar 1, 2022

Merge remote-tracking branch 'upstream/master' into mm-699

9ed70f4

levb suggested changes Mar 1, 2022

View reviewed changes

fixed comment

2fed01e

sibasankarnayak closed this Mar 15, 2022

		recipientTypeAssignee = "assignee"
		recipientTypeReporter = "reporter"

[mm 699] - Reporter should receive DM when someone comments on their issue Fixes #699 #810

[mm 699] - Reporter should receive DM when someone comments on their issue Fixes #699 #810

Conversation

sibasankarnayak commented Sep 29, 2021 • edited Loading

mattermod commented Sep 29, 2021

codecov-commenter commented Sep 29, 2021 • edited Loading

Codecov Report

hanzei commented Sep 30, 2021

mattermod commented Oct 30, 2021

sibasankarnayak commented Nov 2, 2021

mickmister commented Nov 2, 2021

mickmister left a comment

Choose a reason for hiding this comment

mickmister commented Nov 19, 2021

aaronrothschild commented Nov 22, 2021

sibasankarnayak commented Feb 16, 2022

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanzei commented Mar 1, 2022

sibasankarnayak commented Mar 1, 2022

hanzei commented Mar 1, 2022

Choose a reason for hiding this comment

sibasankarnayak Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levb commented Mar 1, 2022

mickmister commented Mar 3, 2022

mickmister commented Mar 9, 2022

sibasankarnayak commented Mar 10, 2022

mickmister commented Mar 15, 2022

sibasankarnayak commented Mar 15, 2022

sibasankarnayak commented Mar 15, 2022

mickmister commented Mar 15, 2022

sibasankarnayak commented Mar 15, 2022

mickmister commented Mar 15, 2022

sibasankarnayak commented Mar 15, 2022

sibasankarnayak commented Sep 29, 2021 •

edited

Loading

codecov-commenter commented Sep 29, 2021 •

edited

Loading

sibasankarnayak Mar 3, 2022 •

edited

Loading