Skip to content

Commit 2e4247f

Browse files
Kshitij-KatiyarKshitij-Katiyar
authored
[MM-870]: Fixed issue of deletion of non-existent subscription (#886)
* Fixed issue of deletion of non-existant subscription
1 parent 0bae262 commit 2e4247f

File tree

4 files changed

+70
-16
lines changed

4 files changed

+70
-16
lines changed

‎server/mocks/mock_KvStore.go‎

Lines changed: 11 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎server/plugin/command.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -599,8 +599,12 @@ func (p *Plugin) handleUnsubscribe(_ *plugin.Context, args *model.CommandArgs, p
599599

600600
owner = strings.ToLower(owner)
601601
repo = strings.ToLower(repo)
602-
if err := p.Unsubscribe(args.ChannelId, repo, owner); err != nil {
603-
p.client.Log.Warn("Failed to unsubscribe", "repo", repo, "error", err.Error())
602+
if sErr := p.Unsubscribe(args.ChannelId, repo, owner); sErr != nil {
603+
if sErr.Code == SubscriptionNotFound {
604+
return sErr.Error.Error()
605+
}
606+
607+
p.client.Log.Warn("Failed to unsubscribe", "repo", repo, "error", sErr.Error.Error())
604608
return "Encountered an error trying to unsubscribe. Please try again."
605609
}
606610

‎server/plugin/command_test.go‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1157,17 +1157,34 @@ func TestHandleUnsubscribe(t *testing.T) {
11571157
assert.Equal(t, "Encountered an error trying to unsubscribe. Please try again.", result)
11581158
},
11591159
},
1160+
{
1161+
name: "No subscription exists for repo in the channel",
1162+
parameters: []string{"owner/repo"},
1163+
setup: func() {
1164+
mockKVStore.EXPECT().Get(SubscriptionsKey, gomock.Any()).DoAndReturn(func(key string, value **Subscriptions) error {
1165+
*value = &Subscriptions{Repositories: map[string][]*Subscription{}}
1166+
return nil
1167+
}).Times(1)
1168+
mockAPI.On("GetUser", MockUserID).Return(nil, &model.AppError{Message: "error getting user"}).Times(1)
1169+
mockAPI.On("LogWarn", "Error while fetching user details", "error", "error getting user").Times(1)
1170+
mockKVStore.EXPECT().SetAtomicWithRetries(SubscriptionsKey, gomock.Any()).Return(nil).Times(1)
1171+
},
1172+
assertions: func(result string) {
1173+
assert.Equal(t, "no subscription exists for `owner/repo` in the channel", result)
1174+
},
1175+
},
11601176
{
11611177
name: "Error getting user details",
11621178
parameters: []string{"owner/repo"},
11631179
setup: func() {
11641180
mockKVStore.EXPECT().Get(SubscriptionsKey, gomock.Any()).DoAndReturn(func(key string, value **Subscriptions) error {
11651181
*value = &Subscriptions{Repositories: map[string][]*Subscription{
1166-
"owner/repo": {{ChannelID: "dummyChannelID", CreatorID: MockCreatorID, Repository: "owner/repo"}}}}
1182+
"owner/repo": {{ChannelID: MockChannelID, CreatorID: MockCreatorID, Repository: "owner/repo"}}}}
11671183
return nil
11681184
}).Times(1)
11691185
mockAPI.On("GetUser", MockUserID).Return(nil, &model.AppError{Message: "error getting user"}).Times(1)
11701186
mockAPI.On("LogWarn", "Error while fetching user details", "error", "error getting user").Times(1)
1187+
mockKVStore.EXPECT().SetAtomicWithRetries(SubscriptionsKey, gomock.Any()).Return(nil).Times(1)
11711188
},
11721189
assertions: func(result string) {
11731190
assert.Equal(t, "error while fetching user details: error getting user", result)
@@ -1179,13 +1196,14 @@ func TestHandleUnsubscribe(t *testing.T) {
11791196
setup: func() {
11801197
mockKVStore.EXPECT().Get(SubscriptionsKey, gomock.Any()).DoAndReturn(func(key string, value **Subscriptions) error {
11811198
*value = &Subscriptions{Repositories: map[string][]*Subscription{
1182-
"owner": {{ChannelID: "dummyChannelID", CreatorID: MockCreatorID, Repository: ""}}}}
1199+
"owner/": {{ChannelID: MockChannelID, CreatorID: MockCreatorID, Repository: "owner"}}}}
11831200
return nil
11841201
}).Times(1)
11851202
mockAPI.On("GetUser", MockUserID).Return(&model.User{Username: MockUsername}, nil).Times(1)
11861203
mockAPI.On("CreatePost", mock.Anything).Return(nil, &model.AppError{Message: "error creating post"}).Times(1)
11871204
post.Message = "@mockUsername unsubscribed this channel from [owner](https://github.com/owner)"
11881205
mockAPI.On("LogWarn", "Error while creating post", "post", post, "error", "error creating post").Times(1)
1206+
mockKVStore.EXPECT().SetAtomicWithRetries(SubscriptionsKey, gomock.Any()).Return(nil).Times(1)
11891207
},
11901208
assertions: func(result string) {
11911209
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) {
11971215
setup: func() {
11981216
mockKVStore.EXPECT().Get(SubscriptionsKey, gomock.Any()).DoAndReturn(func(key string, value **Subscriptions) error {
11991217
*value = &Subscriptions{Repositories: map[string][]*Subscription{
1200-
"owner": {{ChannelID: "dummyChannelID", CreatorID: MockCreatorID, Repository: ""}}}}
1218+
"owner/": {{ChannelID: MockChannelID, CreatorID: MockCreatorID, Repository: ""}}}}
12011219
return nil
12021220
}).Times(1)
12031221
mockAPI.On("GetUser", MockUserID).Return(&model.User{Username: MockUsername}, nil).Times(1)
12041222
mockAPI.On("CreatePost", mock.Anything).Return(post, nil).Times(1)
1223+
mockKVStore.EXPECT().SetAtomicWithRetries(SubscriptionsKey, gomock.Any()).Return(nil).Times(1)
12051224
},
12061225
assertions: func(result string) {
12071226
assert.Empty(t, result)
@@ -1213,13 +1232,14 @@ func TestHandleUnsubscribe(t *testing.T) {
12131232
setup: func() {
12141233
mockKVStore.EXPECT().Get(SubscriptionsKey, gomock.Any()).DoAndReturn(func(key string, value **Subscriptions) error {
12151234
*value = &Subscriptions{Repositories: map[string][]*Subscription{
1216-
"owner/repo": {{ChannelID: "dummyChannelID", CreatorID: MockCreatorID, Repository: "owner/repo"}}}}
1235+
"owner/repo": {{ChannelID: MockChannelID, CreatorID: MockCreatorID, Repository: "owner/repo"}}}}
12171236
return nil
12181237
}).Times(1)
12191238
mockAPI.On("GetUser", MockUserID).Return(&model.User{Username: MockUsername}, nil).Times(1)
12201239
mockAPI.On("CreatePost", mock.Anything).Return(nil, &model.AppError{Message: "error creating post"}).Times(1)
12211240
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."
12221241
mockAPI.On("LogWarn", "Error while creating post", "post", post, "error", "error creating post").Times(1)
1242+
mockKVStore.EXPECT().SetAtomicWithRetries(SubscriptionsKey, gomock.Any()).Return(nil).Times(1)
12231243
},
12241244
assertions: func(result string) {
12251245
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) {
12311251
setup: func() {
12321252
mockKVStore.EXPECT().Get(SubscriptionsKey, gomock.Any()).DoAndReturn(func(key string, value **Subscriptions) error {
12331253
*value = &Subscriptions{Repositories: map[string][]*Subscription{
1234-
"owner/repo": {{ChannelID: "dummyChannelID", CreatorID: MockCreatorID, Repository: "owner/repo"}}}}
1254+
"owner/repo": {{ChannelID: MockChannelID, CreatorID: MockCreatorID, Repository: "owner/repo"}}}}
12351255
return nil
12361256
}).Times(1)
12371257
mockAPI.ExpectedCalls = nil
12381258
mockAPI.On("GetUser", MockUserID).Return(&model.User{Username: MockUsername}, nil).Times(1)
12391259
mockAPI.On("CreatePost", mock.Anything).Return(post, nil).Times(1)
1260+
mockKVStore.EXPECT().SetAtomicWithRetries(SubscriptionsKey, gomock.Any()).Return(nil).Times(1)
12401261
post.Message = ""
12411262
},
12421263
assertions: func(result string) {

‎server/plugin/subscriptions.go‎

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,11 @@ import (
1919
const (
2020
SubscriptionsKey = "subscriptions"
2121
flagExcludeOrgMember = "exclude-org-member"
22-
flagIncludeOnlyOrgMembers = "include-only-org-members"
2322
flagRenderStyle = "render-style"
2423
flagFeatures = "features"
2524
flagExcludeRepository = "exclude"
25+
SubscriptionUnavailable = "no subscription exists for `%s` in the channel"
26+
flagIncludeOnlyOrgMembers = "include-only-org-members"
2627
)
2728

2829
type SubscriptionFlags struct {
@@ -399,17 +400,32 @@ func (p *Plugin) GetSubscribedChannelsForRepository(repo *github.Repository) []*
399400
return subsToReturn
400401
}
401402

402-
func (p *Plugin) Unsubscribe(channelID, repo, owner string) error {
403+
type SubscriptionError struct {
404+
Code int
405+
Error error
406+
}
407+
408+
const (
409+
SubscriptionNotFound = iota
410+
SubscriptionAlreadyExists
411+
InternalServerError
412+
)
413+
414+
func NewSubscriptionError(code int, err error) *SubscriptionError {
415+
return &SubscriptionError{Code: code, Error: err}
416+
}
417+
418+
func (p *Plugin) Unsubscribe(channelID, repo, owner string) *SubscriptionError {
403419
repoWithOwner := fmt.Sprintf("%s/%s", owner, repo)
404420

405421
subs, err := p.GetSubscriptions()
406422
if err != nil {
407-
return errors.Wrap(err, "could not get subscriptions")
423+
return NewSubscriptionError(InternalServerError, errors.Wrap(err, "could not get subscriptions"))
408424
}
409425

410426
repoSubs := subs.Repositories[repoWithOwner]
411427
if repoSubs == nil {
412-
return nil
428+
return NewSubscriptionError(SubscriptionNotFound, errors.Errorf(SubscriptionUnavailable, strings.TrimSuffix(repoWithOwner, "/")))
413429
}
414430

415431
removed := false
@@ -421,11 +437,13 @@ func (p *Plugin) Unsubscribe(channelID, repo, owner string) error {
421437
}
422438
}
423439

424-
if removed {
425-
subs.Repositories[repoWithOwner] = repoSubs
426-
if err := p.StoreSubscriptions(subs); err != nil {
427-
return errors.Wrap(err, "could not store subscriptions")
428-
}
440+
if !removed {
441+
return NewSubscriptionError(SubscriptionNotFound, errors.Errorf(SubscriptionUnavailable, strings.TrimSuffix(repoWithOwner, "/")))
442+
}
443+
444+
subs.Repositories[repoWithOwner] = repoSubs
445+
if err := p.StoreSubscriptions(subs); err != nil {
446+
return NewSubscriptionError(InternalServerError, errors.Wrap(err, "could not store subscriptions"))
429447
}
430448

431449
return nil

0 commit comments

Comments
 (0)