diff --git a/server/mocks/mock_KvStore.go b/server/mocks/mock_KvStore.go index 6b39db616..d74d4ca35 100644 --- a/server/mocks/mock_KvStore.go +++ b/server/mocks/mock_KvStore.go @@ -109,3 +109,14 @@ func (m *MockKvStore) SetAtomicWithRetries(key string, valueFunc func(oldValue [ ret0, _ := ret[0].(error) return ret0 } + +// SetAtomicWithRetries indicates an expected call of SetAtomicWithRetries. +func (mr *MockKvStoreMockRecorder) SetAtomicWithRetries(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType( + mr.mock, + "SetAtomicWithRetries", + reflect.TypeOf((*MockKvStore)(nil).SetAtomicWithRetries), + arg0, arg1, + ) +} diff --git a/server/plugin/command.go b/server/plugin/command.go index 455422912..d54cd5670 100644 --- a/server/plugin/command.go +++ b/server/plugin/command.go @@ -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." } diff --git a/server/plugin/command_test.go b/server/plugin/command_test.go index 555081033..a60e829ed 100644 --- a/server/plugin/command_test.go +++ b/server/plugin/command_test.go @@ -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) @@ -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) @@ -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) @@ -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) @@ -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) { diff --git a/server/plugin/subscriptions.go b/server/plugin/subscriptions.go index 85c67489b..a6b7527a0 100644 --- a/server/plugin/subscriptions.go +++ b/server/plugin/subscriptions.go @@ -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 { @@ -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} +} + +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 @@ -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")) } return nil