[GH-744] Add --include-only-org-members flag for channel subscriptions by burakcakirel · Pull Request #797 · mattermost/mattermost-plugin-github

burakcakirel · 2024-07-03T16:31:34Z

Summary

This PR adds --include-only-org-members flag for channel subscriptions

Ticket Link

Fixes #744

mattermost-build · 2024-07-03T16:31:38Z

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.

…e-only-org-members # Conflicts: # README.md # server/plugin/webhook.go

mickmister

Thanks for this contribution @burakcakirel! I have just a few comments. LGTM 👍

mickmister · 2024-07-04T06:51:13Z

 	return p.isUserOrganizationMember(githubClient, user, organization)
 }

+func (p *Plugin) includeOnlyConfigOrgMembers(user *github.User, subscription *Subscription) bool {


Suggested change

func (p *Plugin) includeOnlyConfigOrgMembers(user *github.User, subscription *Subscription) bool {

func (p *Plugin) shouldDenyEventDueToNotOrgMember(user *github.User, subscription *Subscription) bool {

Done 👍 23c44b9

mickmister · 2024-07-04T06:53:13Z

+				Flags:     SubscriptionFlags{IncludeOnlyOrgMembers: true},
+			},
+			expectWarn: true,
+			want:       false,


Thanks for these tests @burakcakirel 👍 Can we have one that tests the happy path of want: true?

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

…DueToNotOrgMember (mattermost#797)

mattermost-build · 2024-07-22T01:03:13Z

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!

wiggin77 · 2024-07-26T22:14:43Z

@raghavaggarwal2308 can this be QA'd before merge?

mattermost-build · 2024-08-06T01:03:16Z

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!

arush-vashishtha · 2024-11-15T08:22:29Z

@raghavaggarwal2308 can this be QA'd before merge?

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.
cc @Kshitij-Katiyar

wiggin77 · 2024-11-15T14:11:26Z

@burakcakirel thank you for your submission. It looks like the --include-only-org-members is not passing our QA testing. Would you like to take a look?

burakcakirel · 2024-11-15T14:21:42Z

Hey @wiggin77, it’s been a while since I worked on this, but let me take a look.

mattermost-build · 2024-11-26T01:04:28Z

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!

raghavaggarwal2308 · 2025-01-10T08:20:10Z

@burakcakirel WIll you be able to work on it anytime soon?

raghavaggarwal2308 · 2025-02-24T13:48:23Z

@burakcakirel Gentle reminder

burakcakirel · 2025-02-24T14:12:09Z

Sorry, @raghavaggarwal2308, I don't have time for that these days.

…ub into mattermostgh-744-support-include-only-org-members

Kshitij-Katiyar · 2025-03-11T09:45:59Z

@wiggin77 @raghavaggarwal2308 Please review

AayushChaudhary0001

This PR has been tested for the flag for --include-only-org-members for both the conditions true and false. The code changes are working fine, LGTM. Approved.

Kshitij-Katiyar · 2025-03-12T10:52:44Z

Thanks @burakcakirel for your efforts

raghavaggarwal2308 · 2025-03-17T10:37:06Z

@wiggin77 Can you please give this PR a review

mattermost-build · 2025-04-08T01:05:56Z

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!

Add --include-only-org-members flag for channel subscriptions

a91924d

burakcakirel requested review from hanzei and mickmister as code owners July 3, 2024 16:31

mattermost-build added the Contributor label Jul 3, 2024

Merge branch 'refs/heads/master' into mattermostgh-744-support-includ…

8cd0e9a

…e-only-org-members # Conflicts: # README.md # server/plugin/webhook.go

mickmister mentioned this pull request Jul 4, 2024

[GH-744] Add --include-only-org-members flag mattermost/docs#7244

Closed

mickmister approved these changes Jul 4, 2024

View reviewed changes

mickmister requested a review from raghavaggarwal2308 July 4, 2024 06:57

mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jul 4, 2024

mickmister requested a review from AayushChaudhary0001 July 4, 2024 06:57

hanzei removed their request for review July 5, 2024 05:12

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

burakcakirel added 2 commits July 8, 2024 14:43

Refactor method's name includeOnlyConfigOrgMembers => shouldDenyEvent…

23c44b9

…DueToNotOrgMember (mattermost#797)

Refactor return state for readability (mattermost#797)

f77d0fa

mickmister approved these changes Jul 10, 2024

View reviewed changes

mattermost-build added the Lifecycle/1:stale label Jul 22, 2024

wiggin77 approved these changes Jul 26, 2024

View reviewed changes

wiggin77 removed the Lifecycle/1:stale label Jul 26, 2024

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

wiggin77 removed the Lifecycle/1:stale label Nov 15, 2024

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

Kshitij-Katiyar added 7 commits March 6, 2025 15:04

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

bbb8294

…ub into mattermostgh-744-support-include-only-org-members

fixed lint

e3cd040

fixed lint

acb64be

fixed issue of filter not working

42381f3

fixed lint

5d28496

fixed misspelling

4f3e961

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

d6718a1

…ub into mattermostgh-744-support-include-only-org-members

Kshitij-Katiyar changed the base branch from master to release_v2.1.3 March 7, 2025 12:16

Kshitij-Katiyar changed the base branch from release_v2.1.3 to master March 7, 2025 12:16

fixed lint

4a27524

Kshitij-Katiyar mentioned this pull request Mar 9, 2025

Add happy path test case for include-org-members flag #884

Open

Kshitij-Katiyar removed the Lifecycle/1:stale label Mar 10, 2025

Kshitij-Katiyar requested a review from wiggin77 March 10, 2025 09:39

AayushChaudhary0001 approved these changes Mar 12, 2025

View reviewed changes

raghavaggarwal2308 removed the 3: QA Review Requires review by a QA tester label Mar 17, 2025

wiggin77 approved these changes Mar 24, 2025

View reviewed changes

wiggin77 assigned raghavaggarwal2308 Mar 24, 2025

raghavaggarwal2308 added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Mar 28, 2025

mattermost-build added the Lifecycle/1:stale label Apr 8, 2025

raghavaggarwal2308 merged commit 30b5d55 into mattermost:master Apr 8, 2025

This was referenced Jun 4, 2026

HH-313553: sync with upstream mattermost-plugin-github #1020

Closed

HH-313553: sync with upstream mattermost-plugin-github hhru/mattermost-plugin-forgejo#9

Merged

	func (p Plugin) includeOnlyConfigOrgMembers(user github.User, subscription *Subscription) bool {
	func (p Plugin) shouldDenyEventDueToNotOrgMember(user github.User, subscription *Subscription) bool {

Conversation

burakcakirel commented Jul 3, 2024

Summary

Ticket Link

Uh oh!

mattermost-build commented Jul 3, 2024

Uh oh!

mickmister left a comment

Choose a reason for hiding this comment

Uh oh!

mickmister Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

burakcakirel Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

mickmister Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

burakcakirel Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattermost-build commented Jul 22, 2024

Uh oh!

wiggin77 commented Jul 26, 2024

Uh oh!

mattermost-build commented Aug 6, 2024

Uh oh!

arush-vashishtha commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wiggin77 commented Nov 15, 2024

Uh oh!

burakcakirel commented Nov 15, 2024

Uh oh!

mattermost-build commented Nov 26, 2024

Uh oh!

raghavaggarwal2308 commented Jan 10, 2025

Uh oh!

raghavaggarwal2308 commented Feb 24, 2025

Uh oh!

burakcakirel commented Feb 24, 2025

Uh oh!

Kshitij-Katiyar commented Mar 11, 2025

Uh oh!

AayushChaudhary0001 left a comment

Choose a reason for hiding this comment

Uh oh!

Kshitij-Katiyar commented Mar 12, 2025

Uh oh!

raghavaggarwal2308 commented Mar 17, 2025

Uh oh!

mattermost-build commented Apr 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

arush-vashishtha commented Nov 15, 2024 •

edited

Loading