[GH-744] Add --include-only-org-members flag for channel subscriptions #797
Conversation
|
Hello @burakcakirel, 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? 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. |
…e-only-org-members # Conflicts: # README.md # server/plugin/webhook.go
There was a problem hiding this comment.
Thanks for this contribution @burakcakirel! I have just a few comments. LGTM 👍
server/plugin/webhook.go
Outdated
| @@ -350,6 +350,20 @@ func (p *Plugin) excludeConfigOrgMember(user *github.User, subscription *Subscri | |||
| return p.isUserOrganizationMember(githubClient, user, organization) | |||
| } | |||
|
|
|||
| func (p *Plugin) includeOnlyConfigOrgMembers(user *github.User, subscription *Subscription) bool { | |||
There was a problem hiding this comment.
| func (p *Plugin) includeOnlyConfigOrgMembers(user *github.User, subscription *Subscription) bool { | |
| func (p *Plugin) shouldDenyEventDueToNotOrgMember(user *github.User, subscription *Subscription) bool { |
| Flags: SubscriptionFlags{IncludeOnlyOrgMembers: true}, | ||
| }, | ||
| expectWarn: true, | ||
| want: false, |
There was a problem hiding this comment.
Thanks for these tests @burakcakirel 👍 Can we have one that tests the happy path of want: true?
There was a problem hiding this comment.
I've tried to test that case but couldn't because the code is unsuitable for testing. I couldn't mock the GetGitHubClient method. Then I've tried to write feature tests for this by mimicking the Jira Plugin tests but couldn't do it again because postIssueEvent method doesn't return anything like in Jira Plugin's PostToChannel method. Hence, this prevents to write a test for this method. Here is the gist file of the tests I've tried to achieve:
https://gist.github.com/burakcakirel/bf8c3da4b67893b70d9871e5032674ae#file-webhook_test-go-L170
server/plugin/webhook.go
Outdated
| return false | ||
| } | ||
|
|
||
| return !p.isUserOrganizationMember(githubClient, user, p.getConfiguration().GitHubOrg) |
There was a problem hiding this comment.
Just for readability to have the success state obvious at the bottom of the function
| return !p.isUserOrganizationMember(githubClient, user, p.getConfiguration().GitHubOrg) | |
| if !p.isUserOrganizationMember(githubClient, user, p.getConfiguration().GitHubOrg) { | |
| return false | |
| } | |
| return true |
mickmister
added
2: Dev Review
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
|
@raghavaggarwal2308 can this be QA'd before merge? |
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
Hii @burakcakirel @wiggin77 @raghavaggarwal2308 , I have tested this and the flag --include-only-org-members is not working and the user is getting the events triggered by non-member user for an organization. |
|
@burakcakirel thank you for your submission. It looks like the |
|
Hey @wiggin77, it’s been a while since I worked on this, but let me take a look. |
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
|
@burakcakirel Did you get a chance to look into the issue? |
Unfortunately, not yet. |
|
@burakcakirel WIll you be able to work on it anytime soon? |
Summary
This PR adds
--include-only-org-membersflag for channel subscriptionsTicket Link
Fixes #744