Skip to content

[MM-870]: Fixed issue of deletion of non-existent subscription #886

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.

Sign up for GitHub

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

Merged
wiggin77 merged 15 commits into from
Sep 23, 2025
+70 −16
Merged

[MM-870]: Fixed issue of deletion of non-existent subscription #886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a4e2ba3
Fixed issue of deletion of non-existant subscription
Kshitij-Katiyar Mar 12, 2025
1623485
Added license header to file
Kshitij-Katiyar Mar 12, 2025
61def0e
Update server/plugin/subscriptions.go
wiggin77 Mar 24, 2025
c625d87
Update server/plugin/command.go
wiggin77 Mar 24, 2025
4a996bf
fixed lint
Kshitij-Katiyar Mar 25, 2025
355b968
Merge branch 'master' into MM-870
Kshitij-Katiyar Mar 25, 2025
adda7fa
fixed lint
Kshitij-Katiyar Mar 25, 2025
305a73f
removed extra space
Kshitij-Katiyar Mar 25, 2025
810fdea
updated snapshot
Kshitij-Katiyar Mar 25, 2025
2cef73b
Merge branch 'master' of github.com:mattermost/mattermost-plugin-gith…
Kshitij-Katiyar Mar 27, 2025
62a87ce
review fixes
Kshitij-Katiyar Mar 27, 2025
6173c17
Merge branch 'master' into MM-870
hanzei Apr 3, 2025
ee669b1
revert package-lock changes
abbas-dependable-naqvi Sep 5, 2025
3bab1b5
Merge branch 'master' of github.com:mattermost/mattermost-plugin-gith…
abbas-dependable-naqvi Sep 5, 2025
664e74c
fix and add testcase
abbas-dependable-naqvi Sep 8, 2025
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
11 changes: 11 additions & 0 deletions server/mocks/mock_KvStore.go
View file
Open in desktop

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

8 changes: 6 additions & 2 deletions server/plugin/command.go
View file
Open in desktop
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
31 changes: 26 additions & 5 deletions server/plugin/command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1157,17 +1157,34 @@ func TestHandleUnsubscribe(t *testing.T) {
assert.Equal(t, "Encountered an error trying to unsubscribe. Please try again.", result)
},
},
{
name: "No subscription exists for repo in the channel",
parameters: []string{"owner/repo"},
setup: func() {
mockKVStore.EXPECT().Get(SubscriptionsKey, gomock.Any()).DoAndReturn(func(key string, value **Subscriptions) error {
*value = &Subscriptions{Repositories: map[string][]*Subscription{}}
return nil
}).Times(1)
mockAPI.On("GetUser", MockUserID).Return(nil, &model.AppError{Message: "error getting user"}).Times(1)
mockAPI.On("LogWarn", "Error while fetching user details", "error", "error getting user").Times(1)
mockKVStore.EXPECT().SetAtomicWithRetries(SubscriptionsKey, gomock.Any()).Return(nil).Times(1)
},
assertions: func(result string) {
assert.Equal(t, "no subscription exists for `owner/repo` in the channel", result)
},
},
{
name: "Error getting user details",
parameters: []string{"owner/repo"},
setup: func() {
mockKVStore.EXPECT().Get(SubscriptionsKey, gomock.Any()).DoAndReturn(func(key string, value **Subscriptions) error {
*value = &Subscriptions{Repositories: map[string][]*Subscription{
"owner/repo": {{ChannelID: "dummyChannelID", CreatorID: MockCreatorID, Repository: "owner/repo"}}}}
"owner/repo": {{ChannelID: MockChannelID, CreatorID: MockCreatorID, Repository: "owner/repo"}}}}
return nil
}).Times(1)
mockAPI.On("GetUser", MockUserID).Return(nil, &model.AppError{Message: "error getting user"}).Times(1)
mockAPI.On("LogWarn", "Error while fetching user details", "error", "error getting user").Times(1)
mockKVStore.EXPECT().SetAtomicWithRetries(SubscriptionsKey, gomock.Any()).Return(nil).Times(1)
},
assertions: func(result string) {
assert.Equal(t, "error while fetching user details: error getting user", result)
Expand All @@ -1179,13 +1196,14 @@ func TestHandleUnsubscribe(t *testing.T) {
setup: func() {
mockKVStore.EXPECT().Get(SubscriptionsKey, gomock.Any()).DoAndReturn(func(key string, value **Subscriptions) error {
*value = &Subscriptions{Repositories: map[string][]*Subscription{
"owner": {{ChannelID: "dummyChannelID", CreatorID: MockCreatorID, Repository: ""}}}}
"owner/": {{ChannelID: MockChannelID, CreatorID: MockCreatorID, Repository: "owner"}}}}
return nil
}).Times(1)
mockAPI.On("GetUser", MockUserID).Return(&model.User{Username: MockUsername}, nil).Times(1)
mockAPI.On("CreatePost", mock.Anything).Return(nil, &model.AppError{Message: "error creating post"}).Times(1)
post.Message = "@mockUsername unsubscribed this channel from [owner](https://github.com/owner)"
mockAPI.On("LogWarn", "Error while creating post", "post", post, "error", "error creating post").Times(1)
mockKVStore.EXPECT().SetAtomicWithRetries(SubscriptionsKey, gomock.Any()).Return(nil).Times(1)
},
assertions: func(result string) {
assert.Equal(t, "@mockUsername unsubscribed this channel from [owner](https://github.com/owner) error creating the public post: error creating post", result)
Expand All @@ -1197,11 +1215,12 @@ func TestHandleUnsubscribe(t *testing.T) {
setup: func() {
mockKVStore.EXPECT().Get(SubscriptionsKey, gomock.Any()).DoAndReturn(func(key string, value **Subscriptions) error {
*value = &Subscriptions{Repositories: map[string][]*Subscription{
"owner": {{ChannelID: "dummyChannelID", CreatorID: MockCreatorID, Repository: ""}}}}
"owner/": {{ChannelID: MockChannelID, CreatorID: MockCreatorID, Repository: ""}}}}
return nil
}).Times(1)
mockAPI.On("GetUser", MockUserID).Return(&model.User{Username: MockUsername}, nil).Times(1)
mockAPI.On("CreatePost", mock.Anything).Return(post, nil).Times(1)
mockKVStore.EXPECT().SetAtomicWithRetries(SubscriptionsKey, gomock.Any()).Return(nil).Times(1)
},
assertions: func(result string) {
assert.Empty(t, result)
Expand All @@ -1213,13 +1232,14 @@ func TestHandleUnsubscribe(t *testing.T) {
setup: func() {
mockKVStore.EXPECT().Get(SubscriptionsKey, gomock.Any()).DoAndReturn(func(key string, value **Subscriptions) error {
*value = &Subscriptions{Repositories: map[string][]*Subscription{
"owner/repo": {{ChannelID: "dummyChannelID", CreatorID: MockCreatorID, Repository: "owner/repo"}}}}
"owner/repo": {{ChannelID: MockChannelID, CreatorID: MockCreatorID, Repository: "owner/repo"}}}}
return nil
}).Times(1)
mockAPI.On("GetUser", MockUserID).Return(&model.User{Username: MockUsername}, nil).Times(1)
mockAPI.On("CreatePost", mock.Anything).Return(nil, &model.AppError{Message: "error creating post"}).Times(1)
post.Message = "@mockUsername Unsubscribed this channel from [owner/repo](https://github.com/owner/repo)\n Please delete the [webhook](https://github.com/owner/repo/settings/hooks) for this subscription unless it's required for other subscriptions."
mockAPI.On("LogWarn", "Error while creating post", "post", post, "error", "error creating post").Times(1)
mockKVStore.EXPECT().SetAtomicWithRetries(SubscriptionsKey, gomock.Any()).Return(nil).Times(1)
},
assertions: func(result string) {
assert.Equal(t, "@mockUsername Unsubscribed this channel from [owner/repo](https://github.com/owner/repo)\n Please delete the [webhook](https://github.com/owner/repo/settings/hooks) for this subscription unless it's required for other subscriptions. error creating the public post: error creating post", result)
Expand All @@ -1231,12 +1251,13 @@ func TestHandleUnsubscribe(t *testing.T) {
setup: func() {
mockKVStore.EXPECT().Get(SubscriptionsKey, gomock.Any()).DoAndReturn(func(key string, value **Subscriptions) error {
*value = &Subscriptions{Repositories: map[string][]*Subscription{
"owner/repo": {{ChannelID: "dummyChannelID", CreatorID: MockCreatorID, Repository: "owner/repo"}}}}
"owner/repo": {{ChannelID: MockChannelID, CreatorID: MockCreatorID, Repository: "owner/repo"}}}}
return nil
}).Times(1)
mockAPI.ExpectedCalls = nil
mockAPI.On("GetUser", MockUserID).Return(&model.User{Username: MockUsername}, nil).Times(1)
mockAPI.On("CreatePost", mock.Anything).Return(post, nil).Times(1)
mockKVStore.EXPECT().SetAtomicWithRetries(SubscriptionsKey, gomock.Any()).Return(nil).Times(1)
post.Message = ""
},
assertions: func(result string) {
Expand Down
36 changes: 27 additions & 9 deletions server/plugin/subscriptions.go
View file
Open in desktop
Comment thread
abbas-dependable-naqvi marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ import (
const (
SubscriptionsKey = "subscriptions"
flagExcludeOrgMember = "exclude-org-member"
flagIncludeOnlyOrgMembers = "include-only-org-members"
flagRenderStyle = "render-style"
flagFeatures = "features"
flagExcludeRepository = "exclude"
SubscriptionUnavailable = "no subscription exists for `%s` in the channel"
flagIncludeOnlyOrgMembers = "include-only-org-members"
)

type SubscriptionFlags struct {
Expand Down Expand Up @@ -399,17 +400,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

@hanzei hanzei Sep 23, 2025

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 •
edited
Loading

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 @@ -421,11 +437,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
Loading