Fix: Subscriptions --exclude functionality affects all channels

[sanjaydemansol]

Summary

Now, --exclude is channel specific.

Testing Notes

When using the exclude functionality the exclusion record that get's created owner/repo - notification : disabled is not channel specific. These exclusions appear in channels where no subscriptions have been created yet and no exclusion was specified.

This affects event delivery in channel --exclude was never used and prevents events from being delivered for some of the repositories. Steps:

• Create a subscription where an owner is passed in and exclude a repo /github subscriptions add DylanH20 --exclude DylanH20/Hello-World-1
• Test and note that events are delivered to the channel for all repos under DylanH20 except for Hello-World-1 (as expected)
• Create a new channel
• Type /github subscribe list notice DylanH20/Hello-World-1 - notification : disabled is already showing
• Type /github subscriptions add DylanH20 to subscribe to all repos underDylanH20
• Type /github subscriptions list in current channel and other channels to confirm, excluded repo should be scoped to current channel.

Ticket Link

Fix #494

[mattermod]

Hello @sanjaydemansol,

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.

[dipak-demansol]

@sanjaydemansol @hanzei This PR LGTM As Per the Description. but i found some other issue,its related to exclude but not related to this PR so i reported 2 different issue, here it is, pls check this #507 #508

[sanjaydemansol]

Discussed offline: The exclude information should be part of the subscription

Hi, I got it. How should I handle this https://github.com/mattermost/mattermost-plugin-github/pull/505#discussion_r738043789?

We still have issues like when user receives direct message, earlier we used to check for exclusion prior to processing webhook

[emilyacook]

Thanks for participating in Hacktoberfest! You can claim your sticker set here: https://get.printfection.com/hacktober21/4144583267 @sanjaydemansol

[mickmister]

We may already be doing this, but we should make sure to LogDebug when we are avoiding a post creation due to the exclude feature

[mattermod]

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

[codecov-commenter]

Codecov Report

Merging #505 (16b10d3) into master (b7368d5) will increase coverage by 3.02%. The diff coverage is 0.00%.

:exclamation: Current head 16b10d3 differs from pull request most recent head 55ae26e. Consider uploading reports for the commit 55ae26e to get more accurate results

@@            Coverage Diff             @@
##           master     #505      +/-   ##
==========================================
+ Coverage   15.57%   18.59%   +3.02%     
==========================================
  Files          15       12       -3     
  Lines        4136     3308     -828     
==========================================
- Hits          644      615      -29     
+ Misses       3450     2654     -796     
+ Partials       42       39       -3     

Impacted Files	Coverage Δ
server/plugin/command.go	8.38% <0.00%> (-0.24%)	:arrow_down:
server/plugin/subscriptions.go	9.89% <0.00%> (-0.23%)	:arrow_down:
server/plugin/webhook.go	0.00% <0.00%> (ø)
server/plugin/template.go	95.23% <0.00%> (-0.15%)	:arrow_down:
server/plugin/oauth.go
server/plugin/cluster.go
server/plugin/telemetry.go
server/plugin/flows.go
server/plugin/manifest.go	100.00% <0.00%> (ø)
... and 6 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 b7368d5...55ae26e. Read the comment docs.

[sanjaydemansol]

@sanjaydemansol have added few suggestions from my side.

@sibasankarnayak Thanks for reviewing, We need to review the PR now, since wit was in Draft state. @dipak-demansol let's sync to test this.

[maisnamraju]

@hanzei sorry i requested the review too early, ill request it once qa is done on this

[sanjaydemansol]

One suggestion to LogDebug when both of these are true as the same time:

• At least one subscription was skipped due to excluded repo
• No post was made from the webhook call on any subscription

This may be useful to diagnose why a given webhook call produced no posts in any subscription

Log message can be: "Skipped webhook call due to excluded repo: (repo name)"

We only want to log this once to avoid over-logging on webhook calls, which happen very often compared to other types of events

Hi, @mickmister Any suggestions for the implementation? only approach I could come with is to have these flags:

type StatusFlags struct {
	userNotified bool
	subscriptionSkipped bool
}

update them whenever needed, check and take action after handler() https://github.com/mattermost/mattermost-plugin-github/blob/2e7ae7c891c970d99dc764d2252af3b5b47738fe/server/plugin/webhook.go#L156-L165 Looking, if there is any better way.

[dipak-demansol]

Just for the info :- i'm facing some bugs in this PR so i'm still working on this to find that is there something wrong from my side, and at the End of the day i will discuss with dylan so ASAP i update you.

[DHaussermann]

Point 3. mentioned above ☝️
@sanjaydemansol currently it seems that users would be able to subscribe to a repo but then also subscribe to the organization with an --exclude to the same repo. This configuration should be invalid and we should prevent saving this data. Can you please add validation for the following to prevent a subscription save?

Case 1: When adding a subscription for owner/repo check if an --exclude exists already for a matching owner/repo Case 2: When adding a subscription for owner or org with an --exclude for a owner/repo check if an existing subscription exists for the owner/repo already

[DHaussermann]

@DHaussermann queuing for triage to discuss priority of this feature

[sanjaydemansol]

@sanjaydemansol We can make the LogDebug task a follow up task to get this merged sooner.

For the snippet you shared below, what is the userNotified used for?

type StatusFlags struct {
	userNotified bool
	subscriptionSkipped bool
}

Hi @mickmister,

 type StatusFlags struct {
 	userNotified bool // TO STORE: No post was made from the webhook call on any subscription
 	subscriptionSkipped bool //  TO STORE: At least one subscription was skipped due to excluded repo
 }

and the plan is to LogDebug it once handler is called.

[sanjaydemansol]

working Points

:- Exclude Functionality is Working. :- Exclude is Not effecting to the Other Channel. :- After Exclude the xyz repo, if user Subscribe the same repo Then Notification is Working for That but if user see the subscription list then same repo excluded and subscribed

issue

1. Not able to subscribe the Org. When use /github subscribe add dipak-demansol then i'm getting the success message but when i do /github subscribe list then i cant see that subscription.
2. some how in my MM server, in some channel i'm not able to delete the subscription and i don't have repor steps so dev have to check that issue in my environment.
3. there is 1 more validation improvement point that Dylan will add.

Hi @dipak-demansol /github subscribe add dipak-demansol is not a valid subscribe command in the first place. I will fix the response; It shouldn't respond with a success message.

[sanjaydemansol]

Point 3. mentioned above ☝️ @sanjaydemansol currently it seems that users would be able to subscribe to a repo but then also subscribe to the organization with an --exclude to the same repo. This configuration should be invalid and we should prevent saving this data. Can you please add validation for the following to prevent a subscription save?

Case 1: When adding a subscription for owner/repo check if an --exclude exists already for a matching owner/repo Case 2: When adding a subscription for owner or org with an --exclude for a owner/repo check if an existing subscription exists for the owner/repo already

Thanks @

Point 3. mentioned above ☝️ @sanjaydemansol currently it seems that users would be able to subscribe to a repo but then also subscribe to the organization with an --exclude to the same repo. This configuration should be invalid and we should prevent saving this data. Can you please add validation for the following to prevent a subscription save?

Case 1: When adding a subscription for owner/repo check if an --exclude exists already for a matching owner/repo Case 2: When adding a subscription for owner or org with an --exclude for a owner/repo check if an existing subscription exists for the owner/repo already

Thanks, I will add the validation for the same.

[mickmister]

Hi @mickmister,

 type StatusFlags struct {
 	userNotified bool // TO STORE: No post was made from the webhook call on any subscription
 	subscriptionSkipped bool //  TO STORE: At least one subscription was skipped due to excluded repo
 }

and the plan is to LogDebug it once handler is called.

Yeah that makes sense to me. We'll want to make sure we have context for why the subscription was skipped (is it always per-repo?). Maybe the information we need in that regard is already self-contained in the webhook event payload, so the struct that you propose looks good :+1:

[DHaussermann]

/update-branch

[mattermod]

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

[DHaussermann]

Hi @sanjaydemansol, I see an issue when I have this branch deployed. When I add a subscription such as /github subscriptions add DHaussermann. I don't see it saved in the list. I don't currently see this issue happen in master

Compared to master

The last commit here is a bit out of date. Possibly, the issue is already resolved in master. Can you please merge master into demansoltech:mm-494 so I can see if it resolves the issue here?

[mickmister]

I was looking at the code that adds the --exclude flag, and noticed a few issues:

https://github.com/mattermost/mattermost-plugin-github/blob/40e7fcf8bdfa2f49bae44b214ed96588ffe6c4df/server/plugin/command.go#L637-L659

1. We are only adding the --exclude flag when config.GitHubOrg is set. This flag should show regardless of if the plugin is scoped to a given organization, unless this was part of the original design of the feature.

2. We are adding the owner/repo argument to the command twice. Notice there are two instances running of this line:

subscriptionsAdd.AddTextArgument("Owner/repo to subscribe to", "[owner/repo]", "")

3. The --exclude flag and --exclude-org-member flags are registered as positional arguments, so you can't access the --exclude-org-member flag in the autocomplete unless the user has first provided the (unrelated) --exclude flag.

We should be using AddNamedStaticListArgument rather than AddStaticListArgument for this. Here's an example for how to use named arguments https://github.com/nathanaelhoun/mattermost-plugin-broomer/blob/d0024885cea0827f2690168d1d42859dbee5b717/server/utils_commands.go#L136

@hanzei @DHaussermann These aren't necessarily blockers but 1/5 these should be fixed before this is merged, or have the feature reverted for this release due to scope increase.

[hanzei]

Good catch @mickmister, these seem like important issues to fix before merging releasing the feature. To simplify the potential revert, I would suggest they get fixed in this PR.

[sanjaydemansol]

this issue https://github.com/mattermost/mattermost-plugin-github/issues/526 is fixed in mm-494.

[catalintomai]

@sanjaydemansol - is all feedback addressed and is this ready for further/final review?

[sanjaydemansol]

yes correct

[hanzei]

There as conflicts to resolve as https://github.com/mattermost/mattermost-plugin-github/pull/457 has been reverted

[mattermod]

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 @aspleenic

[mattermod]

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!

[maisnamrajusingh]

@mickmister please ignore as I clicked on re-request by mistake

[mickmister]

Note: The original feature was reverted here https://github.com/mattermost/mattermost-plugin-github/pull/529, so master is currently not affected by this