Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions server/plugin/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -599,8 +599,12 @@ func (p *Plugin) handleUnsubscribe(_ *plugin.Context, args *model.CommandArgs, p

owner = strings.ToLower(owner)
repo = strings.ToLower(repo)
if err := p.Unsubscribe(args.ChannelId, repo, owner); err != nil {
p.client.Log.Warn("Failed to unsubscribe", "repo", repo, "error", err.Error())
if sErr := p.Unsubscribe(args.ChannelId, repo, owner); sErr != nil {
if sErr.Code == SubscriptionNotFound {
return sErr.Error.Error()
}

p.client.Log.Warn("Failed to unsubscribe", "repo", repo, "error", sErr.Error.Error())
return "Encountered an error trying to unsubscribe. Please try again."
}

Expand Down
44 changes: 31 additions & 13 deletions server/plugin/subscriptions.go
Comment thread
abbas-dependable-naqvi marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,12 @@ import (
)

const (
SubscriptionsKey = "subscriptions"
flagExcludeOrgMember = "exclude-org-member"
flagRenderStyle = "render-style"
flagFeatures = "features"
flagExcludeRepository = "exclude"
SubscriptionsKey = "subscriptions"
flagExcludeOrgMember = "exclude-org-member"
flagRenderStyle = "render-style"
flagFeatures = "features"
flagExcludeRepository = "exclude"
SubscriptionUnavailable = "no subscription exists for `%s` in the channel"
)

type SubscriptionFlags struct {
Expand Down Expand Up @@ -378,17 +379,32 @@ func (p *Plugin) GetSubscribedChannelsForRepository(repo *github.Repository) []*
return subsToReturn
}

func (p *Plugin) Unsubscribe(channelID, repo, owner string) error {
type SubscriptionError struct {
Code int
Error error
}

const (
SubscriptionNotFound = iota
SubscriptionAlreadyExists
InternalServerError
)

func NewSubscriptionError(code int, err error) *SubscriptionError {
return &SubscriptionError{Code: code, Error: err}
}
Comment on lines +408 to +416
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2/5 I prefer using customer error variables and then checking them using errors.Is. That is what the std lib recommends and is also used on the MM server.

Copy link
Copy Markdown
Member

@wiggin77 wiggin77 Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Go devs think typed errors are better than sentinel errors. The reason they are not changed in the std lib are for compatibility. Also sentinel errors are much slower.

One thing I aways disliked about sentinel errors is that code could replace the error var with something else. MM server is a mix of sentinel and typed errors.

ref: golang/go#63602 (comment)


func (p *Plugin) Unsubscribe(channelID, repo, owner string) *SubscriptionError {
repoWithOwner := fmt.Sprintf("%s/%s", owner, repo)

subs, err := p.GetSubscriptions()
if err != nil {
return errors.Wrap(err, "could not get subscriptions")
return NewSubscriptionError(InternalServerError, errors.Wrap(err, "could not get subscriptions"))
}

repoSubs := subs.Repositories[repoWithOwner]
if repoSubs == nil {
return nil
return NewSubscriptionError(SubscriptionNotFound, errors.Errorf(SubscriptionUnavailable, strings.TrimSuffix(repoWithOwner, "/")))
}

removed := false
Expand All @@ -400,11 +416,13 @@ func (p *Plugin) Unsubscribe(channelID, repo, owner string) error {
}
}

if removed {
subs.Repositories[repoWithOwner] = repoSubs
if err := p.StoreSubscriptions(subs); err != nil {
return errors.Wrap(err, "could not store subscriptions")
}
if !removed {
return NewSubscriptionError(SubscriptionNotFound, errors.Errorf(SubscriptionUnavailable, strings.TrimSuffix(repoWithOwner, "/")))
}

subs.Repositories[repoWithOwner] = repoSubs
if err := p.StoreSubscriptions(subs); err != nil {
return NewSubscriptionError(InternalServerError, errors.Wrap(err, "could not store subscriptions"))
}
Comment thread
Kshitij-Katiyar marked this conversation as resolved.

return nil
Expand Down
24 changes: 18 additions & 6 deletions webapp/package-lock.json
Comment thread
abbas-dependable-naqvi marked this conversation as resolved.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading