-
Notifications
You must be signed in to change notification settings - Fork 150
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
Fix: Subscriptions --exclude functionality affects all channels #505
Conversation
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. |
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 wondering if the data model makes sense for the use case. See below for more details.
@sanjaydemansol @hanzei This PR LGTM As Per the Description. |
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.
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.
Discussed offline: The exclude information should be part of the subscription
Hi, I got it. How should I handle this #505 (comment)?
|
Thanks for participating in Hacktoberfest! You can claim your sticker set here: https://get.printfection.com/hacktober21/4144583267 @sanjaydemansol |
We may already be doing this, but we should make sure to |
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.
@sanjaydemansol have added few suggestions from my side.
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.
Thanks for addressing the feedback @sanjaydemansol 👍
I have a few just more requests on the PR
server/plugin/subscriptions.go
Outdated
func GetExcludedNotificationRepos(s Subscription) []string { | ||
return s.Flags.ExcludedRepos |
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.
Looks like it's still here 🙂
server/plugin/command.go
Outdated
var repoMatch []string | ||
if len(subs) != 0 { | ||
for _, sub := range subs { | ||
if len(sub.Flags.ExcludedRepos) > 0 { | ||
repoMatch = strings.Split(strings.Trim(strings.Join(sub.Flags.ExcludedRepos, ", "), "/"), "/") | ||
} | ||
} | ||
if len(repoMatch) == 0 { | ||
return "", nil | ||
} | ||
return repoMatch[1], nil | ||
} | ||
return "", nil |
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 seems a bit off. Should we add a break
after the repoMatch =
line?
Or or it could be written this way:
var repoMatch []string | |
if len(subs) != 0 { | |
for _, sub := range subs { | |
if len(sub.Flags.ExcludedRepos) > 0 { | |
repoMatch = strings.Split(strings.Trim(strings.Join(sub.Flags.ExcludedRepos, ", "), "/"), "/") | |
} | |
} | |
if len(repoMatch) == 0 { | |
return "", nil | |
} | |
return repoMatch[1], nil | |
} | |
return "", nil | |
for _, sub := range subs { | |
if len(sub.Flags.ExcludedRepos) > 0 { | |
repoMatch = strings.Split(strings.Trim(strings.Join(sub.Flags.ExcludedRepos, ", "), "/"), "/") | |
if len(repoMatch) > 1 { | |
return repoMatch[1], nil | |
} | |
} | |
} | |
return "", nil |
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.
ok, i updated as suggest
server/plugin/command.go
Outdated
// if config.GitHubOrg != "" { | ||
subscriptionsAdd.AddNamedStaticListArgument("", "Currently supports --exclude and --exclude-org-member", true, []model.AutocompleteListItem{ | ||
{ | ||
HelpText: "notifications for these repos will be turned off", | ||
Hint: "(optional)", | ||
Item: "--exclude", | ||
}, | ||
{ | ||
HelpText: "Events triggered by organization members will not be delivered (the organization config should be set, otherwise this flag has no effect)", | ||
Hint: "(optional)", | ||
Item: "--exclude-org-member", | ||
}, | ||
}) | ||
// } |
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 won't produce the right autocomplete because we are supposed to have two different field types here. One is text-based, and one is boolean-based.
In order to support boolean-based arguments in our flag argument system, we will need to treat that field as a separate select element:
commandArgumentExcludeHint := "Notifications for these repos will be turned off"
commandArgumentExcludeOrgMemberHint := "Events triggered by organization members will not be delivered (default false)"
cmd.AddNamedTextArgument("exclude", commandArgumentExcludeHint, "", false)
if config.GitHubOrg != "" {
cmd.AddNamedStaticListArgument("exclude-org-member", commandArgumentExcludeOrgMemberHint, false, []model.AutocompleteListItem{
{
Item: "true",
HelpText: "Exclude posts from members of the configured organization",
}, {
Item: "false",
HelpText: "Include posts from members of the configured organization",
},
})
}
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.
feedback point 1, remove GithubOrg, #505 (comment)
point 3 exclude and exclude-org-member is positional arguments. fixed it.
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 will check and update as feedback
@@ -353,7 +362,16 @@ func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs, | |||
return "--exclude feature currently support on organization level." | |||
} | |||
|
|||
if err := p.Subscribe(ctx, githubClient, args.UserId, owner, repo, args.ChannelId, features, flags); err != nil { | |||
repoMatch, err := p.getExcludedRepo(args) |
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.
Shouldn't we support more than one excluded repo here?
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.
@mickmister 1/5 we may add support for the same in next release. User may call the command multiple times if needed. Do you want me to add it in this PR?
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.
@sanjaydemansol Most of the code related to the exclude feature seems to imply it works with multiple repos. I need to review the original PR to understand the code better as I wasn't a reviewer on the PR.
Shouldn't we support more than one repo here?
I said this with the assumption that the feature already supports it in general, and that I genuinely don't know if we need to support that in this piece of code as well
server/plugin/command.go
Outdated
for _, excludedRepo := range flags.ExcludedRepos { | ||
var subscribedRepo = strings.Trim(sub.Repository, "/") | ||
if subscribedRepo == excludedRepo { | ||
msg := fmt.Sprintf("Failed to add subscription to %s organization with --exclude. The repository %s cannot be excluded as a subscription already exists. Please remove the existing subscription first.", owner, excludedRepo) |
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.
msg := fmt.Sprintf("Failed to add subscription to %s organization with --exclude. The repository %s cannot be excluded as a subscription already exists. Please remove the existing subscription first.", owner, excludedRepo) | |
msg := fmt.Sprintf("Failed to add subscription to %s organization with --exclude. The repository %s cannot be excluded as a subscription already exists in another subscription for this channel. Please remove the existing subscription first.", owner, excludedRepo) |
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.
update message as per feedback
There as conflicts to resolve as #457 has been reverted |
This PR has been automatically labelled "stale" because it hasn't had recent activity. /cc @aspleenic |
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
@mickmister please ignore as I clicked on re-request by mistake |
Note: The original feature was reverted here #529, so master is currently not affected by this |
Summary
Now,
--exclude
is channel specific.Testing Notes
Ticket Link
Fix #494