-
Notifications
You must be signed in to change notification settings - Fork 124
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-626]: Supported filtering on comment visibility for subscriptions #894
base: master
Are you sure you want to change the base?
Conversation
#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
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. |
server/client.go
Outdated
@@ -252,6 +255,18 @@ type AutoCompleteResult struct { | |||
Results []Result `json:"results"` | |||
} | |||
|
|||
type Item struct { |
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.
What's Item? We can find a more relevant name I think
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.
Now I have changed this to GroupItem
. It indicates the individual group.
server/client.go
Outdated
Name string `json:"name"` | ||
} | ||
|
||
type Group struct { |
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.
Is this a UserGroup?
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.
or VisibilityUserGroup? Is this group being used for anything else?
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.
Now I have changed this to GroupsInfo
.It indicate all the groups to which a commentator belongs to.
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.
How about JiraUserGroupCollection
for the collection and JiraUserGroup
for the singular group?
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
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
@mickmister Head up that the PR is ready for your review |
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
Codecov ReportAttention: Patch coverage is
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. |
@mickmister Fixed the merge conflicts and this PR is ready for review. |
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
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.
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
Outdated
visibilityAttribute := "" | ||
if isCommentRelatedWebhook(wh) { | ||
if visibilityAttribute, err = ww.getVisibilityAttribute(msg, v); err != nil { | ||
return err | ||
} | ||
} |
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.
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.
@@ -26,8 +26,10 @@ import ( | |||
) | |||
|
|||
const autocompleteSearchRoute = "2/jql/autocompletedata/suggestions" | |||
const commentVisibilityRoute = "2/user" |
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'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, | |
}) | |
} |
...ponents/modals/channel_subscriptions/__snapshots__/channel_subscription_filter.test.tsx.snap
Outdated
Show resolved
Hide resolved
webapp/src/components/modals/channel_subscriptions/channel_subscription_filter.tsx
Outdated
Show resolved
Hide resolved
...omponents/data_selectors/jira_commentvisibility_selector/jira_commentvisibility_selector.tsx
Outdated
Show resolved
Hide resolved
const fetchInitialSelectedValues = async (): Promise<ReactSelectOption[]> => | ||
((!value || (isMulti && !value.length)) ? [] : commentVisibilityFields('')); |
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.
This is a little hard to read, partially because it's fairly terse one-liner that spans to two lines, and it also references a variable defined below, which is a bit confusing
Also this will need to be tested on Jira Cloud and Jira Server |
Summary
Added "Comment Visibility" field to subscription modal to filter notifications in the channel.
Ticket Link
Fixes #626