[GH-626]: Supported filtering on comment visibility for subscriptions #894

Nityanand13 · 2022-11-28T14:34:13Z

Summary

Added "Comment Visibility" field to subscription modal to filter notifications in the channel.

When a user creates a comment in Jira then the plugin is getting notified through a webhook. Now, with this API we are getting all the information of a comment. If that particular comment has a visibility attribute that it should be visible to the users who belong to a certain group. Then notification is sent to the subscribed channel in mattermost only when that particular group is included in the subscriptions and vice versa.
Apart from adding groups to which a user belongs, he can also add a group ‘visible-to-all-users’. If a particular comment has no visibility attribute, then notification is sent to the subscribed channel in mattermost only when ‘visible-to-all-users’ is present or no filter is present on the mattermost side.

Ticket Link

Fixes #626

#18) * [MI-2337]: Supported filtering on comment visibility for subscriptions * [MI-2337]: Review fixes done 1. Improved code quality 2. Improved quality of comments * [MI-2337]: Review fixes done 1. Improved code quality 2. Changed few names * [MI-2337]: Review fixes done 1. Improved code quality * [MI-2337]: Review fixes done 1. Improved code quality 2. Improved comments quality * [MI-2337]: Review fixes done 1. Improved code quality * [MI-2384]: Converted class component to functional component of Comment visibility filter (#20) * [MI-2384]: Converted class component to functional component of comment visibility filter * [MI-2384]: Review fixes done 1. Improved code quality 2. Fixed lint errors * [MI-2384]: Review fixes done 1. Improved code quality * [MI-2384]: Logged the error properly

mattermod · 2022-11-28T14:34:17Z

Hello @Nityanand13,

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.

javaguirre · 2022-11-29T15:22:45Z

server/client.go

@@ -252,6 +255,18 @@ type AutoCompleteResult struct {
 	Results []Result `json:"results"`
 }

+type Item struct {


What's Item? We can find a more relevant name I think

Now I have changed this to GroupItem. It indicates the individual group.

javaguirre · 2022-11-29T15:24:10Z

server/client.go

+	Name string `json:"name"`
+}
+
+type Group struct {


Is this a UserGroup?

or VisibilityUserGroup? Is this group being used for anything else?

Now I have changed this to GroupsInfo.It indicate all the groups to which a commentator belongs to.

How about JiraUserGroupCollection for the collection and JiraUserGroup for the singular group?

server/webhook_worker.go

webapp/src/utils/jira_issue_metadata.tsx

1. Made a constant 2. Improved code quality 3. Changed few names

* [MI-2337]: Supported filtering on comment visibility for subscriptions * [MI-2337]: Review fixes done 1. Improved code quality 2. Improved quality of comments * [MI-2337]: Review fixes done 1. Improved code quality 2. Changed few names * [MI-2337]: Review fixes done 1. Improved code quality * [MI-2337]: Review fixes done 1. Improved code quality 2. Improved comments quality * [MI-2337]: Review fixes done 1. Improved code quality * [MI-2384]: Converted class component to functional component of Comment visibility filter (#20) * [MI-2384]: Converted class component to functional component of comment visibility filter * [MI-2384]: Review fixes done 1. Improved code quality 2. Fixed lint errors * [MI-2384]: Review fixes done 1. Improved code quality * [MI-2384]: Logged the error properly * [MI-2337]: Done the review fixes of PR mattermost#894 1. Made a constant 2. Improved code quality 3. Changed few names

* [MI-2337]: Supported filtering on comment visibility for subscriptions * [MI-2337]: Review fixes done 1. Improved code quality 2. Improved quality of comments * [MI-2337]: Review fixes done 1. Improved code quality 2. Changed few names * [MI-2337]: Review fixes done 1. Improved code quality * [MI-2337]: Review fixes done 1. Improved code quality 2. Improved comments quality * [MI-2337]: Review fixes done 1. Improved code quality * [MI-2384]: Converted class component to functional component of Comment visibility filter (#20) * [MI-2384]: Converted class component to functional component of comment visibility filter * [MI-2384]: Review fixes done 1. Improved code quality 2. Fixed lint errors * [MI-2384]: Review fixes done 1. Improved code quality * [MI-2384]: Logged the error properly * [MI-2337]: Done the review fixes of PR mattermost#894 1. Made a constant 2. Improved code quality 3. Changed few names * [MI-2337]: Fixed CI errors and few review comments of PR mattermost#894 * [MI-2337]: Review fixes done 1. Improved code quality

mattermod · 2022-12-24T01:02: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!

hanzei · 2023-02-21T09:33:14Z

@mickmister Head up that the PR is ready for your review

mattermost-build · 2023-03-05T01:02:10Z

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!

codecov-commenter · 2023-09-11T10:36:59Z

Codecov Report

Attention: Patch coverage is 5.47945% with 69 lines in your changes missing coverage. Please review.

Project coverage is 29.41%. Comparing base (3d90500) to head (649d520).
Report is 10 commits behind head on master.

❗ Current head 649d520 differs from pull request most recent head 5e21750

Please upload reports for the commit 5e21750 to get more accurate results.

Files	Patch %	Lines
server/issue.go	0.00%	34 Missing ⚠️
server/webhook_worker.go	0.00%	23 Missing ⚠️
server/client.go	0.00%	7 Missing ⚠️
server/subscribe.go	37.50%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #894      +/-   ##
==========================================
- Coverage   33.37%   29.41%   -3.97%     
==========================================
  Files          53       52       -1     
  Lines        8008     7861     -147     
==========================================
- Hits         2673     2312     -361     
- Misses       5112     5351     +239     
+ Partials      223      198      -25

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mattermost-build · 2023-10-07T01:03:23Z

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!

mickmister

Great work on this improvement @Nityanand13 👍

I have a few topics for discussion, one in particular is how to properly handle the case when the Jira comment author is not connected to Mattermost. This poses a challenge with the tradeoffs of defaulting to secure or not. There are some other suggestions as well, but mostly LGTM

server/webhook_worker.go

mickmister · 2023-10-13T02:02:11Z

server/webhook_worker.go

+	visibilityAttribute := ""
+	if isCommentRelatedWebhook(wh) {
+		if visibilityAttribute, err = ww.getVisibilityAttribute(msg, v); err != nil {
+			return err
+		}
+	}


If the comment author is not connected to Mattermost, we end up not processing the webhook event no matter what. I think there's a design gap in the security model of this feature. This is a difficult problem to solve, the case of a comment author not connected to Mattermost.

Let's say a subscription is set to "comment visibility must be empty". If we can't fetch the comment because the author is not connected to MM, then we can't know the visibility of the comment. And so we would then always be avoiding the webhook events with every comment made by a user that is not connected. That's not really behavior that we want.

Though we can only assume that it is a sensitive comment if we can't fetch its visibility. Otherwise the feature would create posts for sensitive comments.

@esarafianou Do you have any thoughts on this? Essentially, this feature allows subscriptions to filter out sensitive comments, but if the comment author is not connected to MM, then we have no way of knowing whether the comment is sensitive or not. Defaulting to secure avoids unwanted data leakage, but it then requires all comment authors to be connected to MM for any comments to be posted ever.

@mickmister Seems I had missed this. Is this still relevant?

@mickmister I have removed the extra API call for getting the comment visibility data as in the current implementation of Jira plugin we are always making an API call to get the comment event data. So, the user have to be connected in every case of comment events.

This will be fixed with the API token approach we discussed earlier. So, maybe we can proeed with this for now and it will be fixed later.

API token approch implemented via #1102

mickmister · 2023-10-13T02:05:39Z

server/client.go

@@ -26,8 +26,10 @@ import (
 )

 const autocompleteSearchRoute = "2/jql/autocompletedata/suggestions"
+const commentVisibilityRoute = "2/user"


I'm thinking we want to use project roles for this

I have a WIP here that implements the data fetching piece, but not the comment filtering piece master...comment-security

mattermost-plugin-jira/server/autocomplete_search.go

Lines 108 to 121 in a86928d

err = client.RESTGet("2/project/"+projectKey, nil, &result)

if err != nil {

return http.StatusInternalServerError,

errors.WithMessage(err, "error fetching comment security levels")

}

roles := result.Roles

out := &AutoCompleteResult{}

for role := range roles {

out.Results = append(out.Results, Result{

Value: role,

DisplayName: role,

})

}

@mickmister We are getting the user groups here. I don't think the project roles API returns that response

server/issue.go

...ponents/modals/channel_subscriptions/__snapshots__/channel_subscription_filter.test.tsx.snap

webapp/src/components/modals/channel_subscriptions/channel_subscription_filter.tsx

webapp/src/utils/jira_issue_metadata.tsx

...omponents/data_selectors/jira_commentvisibility_selector/jira_commentvisibility_selector.tsx

mickmister · 2023-10-13T02:58:07Z

Also this will need to be tested on Jira Cloud and Jira Server

… into mm-626

1. Path of API call to get commen data 2. Testcases

… into mm-626

1. Removed extra API call for getting comment visibility details 2. Removed unnecessary commentVisibility attribute

raghavaggarwal2308 · 2024-07-01T06:59:10Z

@mickmister Fixed the review comments added by you.

… into mm-626

mattermost-build · 2024-08-14T01:02:59Z

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!

… into mm-626

wiggin77 · 2024-10-22T22:47:32Z

/update-branch

mattermost-build · 2024-10-22T22:47:34Z

We don't have permissions to update this PR, please contact the submitter to apply the update.

wiggin77 · 2024-10-22T22:49:57Z

@raghavaggarwal2308 can you shepherd this through the outstanding comments?

raghavaggarwal2308 · 2024-10-24T10:16:41Z

@wiggin77 There are two unresolved comments above:

[GH-626]: Supported filtering on comment visibility for subscriptions #894 (comment): This has been resolved now as the API token approach is implemented now. Basically the issue was that when we receive a comment event we needed to fetch some more comment details using the author ID of the comment but if the author is not connected to Jira from MM then we will not have the auth token and wont be able to fetch the details and end us not posting the event in the channel at all.
[GH-626]: Supported filtering on comment visibility for subscriptions #894 (comment): Michael was suggesting to use some alternate API, which did not seem to be valid for our use case.

Nityanand13 requested a review from mickmister as a code owner November 28, 2022 14:34

mattermod added the Contributor label Nov 28, 2022

hanzei added the 2: Dev Review Requires review by a core committer label Nov 29, 2022

hanzei requested a review from javaguirre November 29, 2022 14:08

hanzei added the 3: QA Review Requires review by a QA tester label Nov 29, 2022

javaguirre suggested changes Nov 29, 2022

View reviewed changes

Nityanand13 added a commit to Brightscout/mattermost-plugin-jira that referenced this pull request Nov 30, 2022

[MI-2337]: Done the review fixes of PR mattermost#894

b7ef5ca

1. Made a constant 2. Improved code quality 3. Changed few names

Nityanand13 mentioned this pull request Nov 30, 2022

Done the review fixes of PR #894 Brightscout/mattermost-plugin-jira#21

Merged

Nityanand13 added a commit to Brightscout/mattermost-plugin-jira that referenced this pull request Dec 9, 2022

[MI-2337]: Fixed CI errors and few review comments of PR mattermost#894

364c71a

Nityanand13 mentioned this pull request Dec 9, 2022

[MI 2337] Fixed CI errors and review comments of PR #894 Brightscout/mattermost-plugin-jira#24

Merged

Nityanand13 requested review from javaguirre and removed request for mickmister December 13, 2022 12:05

javaguirre approved these changes Dec 13, 2022

View reviewed changes

mattermod added the Lifecycle/1:stale label Dec 24, 2022

mickmister self-requested a review January 3, 2023 22:35

hanzei removed the Lifecycle/1:stale label Feb 21, 2023

hanzei added this to the v3.3.0 milestone Feb 22, 2023

mattermost-build added the Lifecycle/1:stale label Mar 5, 2023

hanzei requested a review from calebroseland May 21, 2023 07:34

hanzei removed the request for review from mickmister May 30, 2023 19:12

hanzei requested a review from mickmister September 1, 2023 10:01

[MM-626]:Fixed merge conflicts

2edf22e

mickmister removed the request for review from calebroseland October 13, 2023 02:56

mickmister reviewed Oct 13, 2023

View reviewed changes

raghavaggarwal2308 added 6 commits May 13, 2024 16:41

Merge branch 'master' of github.com:mattermost/mattermost-plugin-jira…

ff40054

… into mm-626

Fixed:

279ed3d

1. Path of API call to get commen data 2. Testcases

Review fixes

8929dfe

Review fixes

1d1b82a

Merge branch 'master' of github.com:mattermost/mattermost-plugin-jira…

8fbe0e9

… into mm-626

[MM-537] Review fixes:

70efc7e

1. Removed extra API call for getting comment visibility details 2. Removed unnecessary commentVisibility attribute

raghavaggarwal2308 requested a review from mickmister July 1, 2024 06:58

[MM-537] Fix lint error

5e21750

raghavaggarwal2308 requested a review from wiggin77 July 5, 2024 08:16

mickmister removed their request for review July 10, 2024 02:21

Merge branch 'master' of github.com:mattermost/mattermost-plugin-jira…

1095514

… into mm-626

hanzei removed the Lifecycle/1:stale label Jul 31, 2024

mattermost-build added the Lifecycle/1:stale label Aug 14, 2024

Merge branch 'master' of github.com:mattermost/mattermost-plugin-jira…

2b316c6

… into mm-626

raghavaggarwal2308 removed the Contributor label Sep 10, 2024

raghavaggarwal2308 modified the milestones: next milestone, v4.3.0 Sep 18, 2024

wiggin77 requested a review from AayushChaudhary0001 October 22, 2024 22:48

wiggin77 removed the Lifecycle/1:stale label Oct 22, 2024

mattermost-build added the Lifecycle/1:stale label Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-626]: Supported filtering on comment visibility for subscriptions #894

[GH-626]: Supported filtering on comment visibility for subscriptions #894

Nityanand13 commented Nov 28, 2022 •

edited

Loading

mattermod commented Nov 28, 2022

javaguirre Nov 29, 2022

Nityanand13 Nov 30, 2022

javaguirre Nov 29, 2022

javaguirre Nov 29, 2022

Nityanand13 Nov 30, 2022

mickmister Dec 6, 2022

mattermod commented Dec 24, 2022

hanzei commented Feb 21, 2023

mattermost-build commented Mar 5, 2023

codecov-commenter commented Sep 11, 2023 •

edited

Loading

mattermost-build commented Oct 7, 2023

mickmister left a comment

mickmister Oct 13, 2023

esarafianou Jun 21, 2024

raghavaggarwal2308 Jul 1, 2024

raghavaggarwal2308 Oct 24, 2024

mickmister Oct 13, 2023

raghavaggarwal2308 Jul 1, 2024

mickmister commented Oct 13, 2023

raghavaggarwal2308 commented Jul 1, 2024

mattermost-build commented Aug 14, 2024

wiggin77 commented Oct 22, 2024

mattermost-build commented Oct 22, 2024

wiggin77 commented Oct 22, 2024

raghavaggarwal2308 commented Oct 24, 2024

	err = client.RESTGet("2/project/"+projectKey, nil, &result)
	if err != nil {
	return http.StatusInternalServerError,
	errors.WithMessage(err, "error fetching comment security levels")
	}

	roles := result.Roles
	out := &AutoCompleteResult{}
	for role := range roles {
	out.Results = append(out.Results, Result{
	Value: role,
	DisplayName: role,
	})
	}

[GH-626]: Supported filtering on comment visibility for subscriptions #894

Are you sure you want to change the base?

[GH-626]: Supported filtering on comment visibility for subscriptions #894

Conversation

Nityanand13 commented Nov 28, 2022 • edited Loading

Summary

Ticket Link

mattermod commented Nov 28, 2022

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

mattermod commented Dec 24, 2022

hanzei commented Feb 21, 2023

mattermost-build commented Mar 5, 2023

codecov-commenter commented Sep 11, 2023 • edited Loading

Codecov Report

mattermost-build commented Oct 7, 2023

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

mickmister commented Oct 13, 2023

raghavaggarwal2308 commented Jul 1, 2024

mattermost-build commented Aug 14, 2024

wiggin77 commented Oct 22, 2024

mattermost-build commented Oct 22, 2024

wiggin77 commented Oct 22, 2024

raghavaggarwal2308 commented Oct 24, 2024

Nityanand13 commented Nov 28, 2022 •

edited

Loading

codecov-commenter commented Sep 11, 2023 •

edited

Loading