Skip to content

Commit 6615713

Browse files
abbas-dependable-naqviabbas-dependable-naqvi
authored
Fix issue of edit subscription creating duplicate subscription (#178)
* Fix issue of deletion of subscription * fix testcases * update testcase to the new logic * fix deletion * handling oldAlias in the subscription creation * Fix error returns and command response
1 parent 00f6e92 commit 6615713

File tree

9 files changed

+136
-27
lines changed

9 files changed

+136
-27
lines changed

‎server/command.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,8 @@ const (
5151
disconnectedUser = "User not connected. Please use `/confluence connect`."
5252
errorExecutingCommand = "Error executing the command, please retry."
5353
oauth2ConnectPath = "%s/oauth2/connect"
54+
55+
generalDeleteError = "error occurred while deleting subscription with name **%s**"
5456
)
5557

5658
const (
@@ -287,7 +289,7 @@ func deleteSubscription(p *Plugin, context *model.CommandArgs, args ...string) *
287289
alias := strings.Join(args, " ")
288290
if err := service.DeleteSubscription(channelID, alias); err != nil {
289291
p.client.Log.Error("Error deleting the subscription", "subscription alias", alias, "error", err.Error())
290-
postCommandResponse(context, err.Error())
292+
postCommandResponse(context, fmt.Sprintf(generalDeleteError, alias))
291293
return &model.CommandResponse{}
292294
}
293295

‎server/serializer/channel_subscription.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,9 @@ var eventDisplayName = map[string]string{
3636
}
3737

3838
type Subscription interface {
39-
Add(*Subscriptions)
40-
Remove(*Subscriptions)
41-
Edit(*Subscriptions)
39+
Add(*Subscriptions) error
40+
Remove(*Subscriptions) error
41+
Edit(*Subscriptions) error
4242
Name() string
4343
GetAlias() string
4444
GetFormattedSubscription() string
@@ -48,6 +48,7 @@ type Subscription interface {
4848

4949
type BaseSubscription struct {
5050
Alias string `json:"alias"`
51+
OldAlias string `json:"oldAlias,omitempty"`
5152
BaseURL string `json:"baseURL"`
5253
Events []string `json:"events"`
5354
ChannelID string `json:"channelID"`

‎server/serializer/page_subscription.go‎

Lines changed: 51 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,29 +16,73 @@ type PageSubscription struct {
1616
BaseSubscription
1717
}
1818

19-
func (ps PageSubscription) Add(s *Subscriptions) {
19+
func (ps PageSubscription) Add(s *Subscriptions) error {
2020
s.EnsureDefaults()
2121

22+
// Update OldAlias to current Alias as we don't need the old alias when creating a new subscription
23+
ps.OldAlias = ps.Alias
24+
2225
if _, valid := s.ByChannelID[ps.ChannelID]; !valid {
2326
s.ByChannelID[ps.ChannelID] = make(StringSubscription)
2427
}
28+
if s.ByChannelID[ps.ChannelID] == nil {
29+
return errors.New("ByChannelID entry is nil")
30+
}
2531
s.ByChannelID[ps.ChannelID][ps.Alias] = ps
2632
key := store.GetURLPageIDCombinationKey(ps.BaseURL, ps.PageID)
33+
2734
if _, ok := s.ByURLPageID[key]; !ok {
2835
s.ByURLPageID[key] = make(map[string][]string)
2936
}
37+
if s.ByURLPageID[key] == nil {
38+
return errors.New("ByURLPageID entry is nil")
39+
}
3040
s.ByURLPageID[key][ps.ChannelID] = ps.Events
41+
return nil
3142
}
3243

33-
func (ps PageSubscription) Remove(s *Subscriptions) {
34-
delete(s.ByChannelID[ps.ChannelID], ps.Alias)
44+
func (ps PageSubscription) Remove(s *Subscriptions) error {
45+
if s.ByChannelID == nil {
46+
return errors.New("ByChannelID map is nil")
47+
}
48+
if channelMap, ok := s.ByChannelID[ps.ChannelID]; ok {
49+
aliasToRemove := ps.OldAlias
50+
if aliasToRemove == "" {
51+
aliasToRemove = ps.Alias
52+
}
53+
54+
if _, aliasOk := channelMap[aliasToRemove]; aliasOk {
55+
delete(channelMap, aliasToRemove)
56+
} else {
57+
return errors.New("alias not found in ByChannelID")
58+
}
59+
} else {
60+
return errors.New("channelID not found in ByChannelID")
61+
}
3562
key := store.GetURLPageIDCombinationKey(ps.BaseURL, ps.PageID)
36-
delete(s.ByURLPageID[key], ps.ChannelID)
63+
if s.ByURLPageID == nil {
64+
return errors.New("ByURLPageID map is nil")
65+
}
66+
if urlPageMap, ok := s.ByURLPageID[key]; ok {
67+
if _, channelOk := urlPageMap[ps.ChannelID]; channelOk {
68+
delete(urlPageMap, ps.ChannelID)
69+
} else {
70+
return errors.New("channelID not found in ByURLPageID entry")
71+
}
72+
} else {
73+
return errors.New("key not found in ByURLPageID")
74+
}
75+
return nil
3776
}
3877

39-
func (ps PageSubscription) Edit(s *Subscriptions) {
40-
ps.Remove(s)
41-
ps.Add(s)
78+
func (ps PageSubscription) Edit(s *Subscriptions) error {
79+
if err := ps.Remove(s); err != nil {
80+
return err
81+
}
82+
if err := ps.Add(s); err != nil {
83+
return err
84+
}
85+
return nil
4286
}
4387

4488
func (ps PageSubscription) Name() string {

‎server/serializer/space_subscription.go‎

Lines changed: 53 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,27 +16,73 @@ type SpaceSubscription struct {
1616
BaseSubscription
1717
}
1818

19-
func (ss SpaceSubscription) Add(s *Subscriptions) {
19+
func (ss SpaceSubscription) Add(s *Subscriptions) error {
20+
s.EnsureDefaults()
21+
22+
// Update OldAlias to current Alias as we don't need the old alias when creating a new subscription
23+
ss.OldAlias = ss.Alias
24+
2025
if _, valid := s.ByChannelID[ss.ChannelID]; !valid {
2126
s.ByChannelID[ss.ChannelID] = make(StringSubscription)
2227
}
28+
if s.ByChannelID[ss.ChannelID] == nil {
29+
return errors.New("ByChannelID entry is nil")
30+
}
2331
s.ByChannelID[ss.ChannelID][ss.Alias] = ss
2432
key := store.GetURLSpaceKeyCombinationKey(ss.BaseURL, ss.SpaceKey)
33+
2534
if _, ok := s.ByURLSpaceKey[key]; !ok {
2635
s.ByURLSpaceKey[key] = make(map[string][]string)
2736
}
37+
if s.ByURLSpaceKey[key] == nil {
38+
return errors.New("ByURLSpaceKey entry is nil")
39+
}
2840
s.ByURLSpaceKey[key][ss.ChannelID] = ss.Events
41+
return nil
2942
}
3043

31-
func (ss SpaceSubscription) Remove(s *Subscriptions) {
32-
delete(s.ByChannelID[ss.ChannelID], ss.Alias)
44+
func (ss SpaceSubscription) Remove(s *Subscriptions) error {
45+
if s.ByChannelID == nil {
46+
return errors.New("ByChannelID map is nil")
47+
}
48+
if channelMap, ok := s.ByChannelID[ss.ChannelID]; ok {
49+
aliasToRemove := ss.OldAlias
50+
if aliasToRemove == "" {
51+
aliasToRemove = ss.Alias
52+
}
53+
54+
if _, aliasOk := channelMap[aliasToRemove]; aliasOk {
55+
delete(channelMap, aliasToRemove)
56+
} else {
57+
return errors.New("alias not found in ByChannelID")
58+
}
59+
} else {
60+
return errors.New("channelID not found in ByChannelID")
61+
}
3362
key := store.GetURLSpaceKeyCombinationKey(ss.BaseURL, ss.SpaceKey)
34-
delete(s.ByURLSpaceKey[key], ss.ChannelID)
63+
if s.ByURLSpaceKey == nil {
64+
return errors.New("ByURLSpaceKey map is nil")
65+
}
66+
if urlSpaceMap, ok := s.ByURLSpaceKey[key]; ok {
67+
if _, channelOk := urlSpaceMap[ss.ChannelID]; channelOk {
68+
delete(urlSpaceMap, ss.ChannelID)
69+
} else {
70+
return errors.New("channelID not found in ByURLSpaceKey entry")
71+
}
72+
} else {
73+
return errors.New("key not found in ByURLSpaceKey")
74+
}
75+
return nil
3576
}
3677

37-
func (ss SpaceSubscription) Edit(s *Subscriptions) {
38-
ss.Remove(s)
39-
ss.Add(s)
78+
func (ss SpaceSubscription) Edit(s *Subscriptions) error {
79+
if err := ss.Remove(s); err != nil {
80+
return err
81+
}
82+
if err := ss.Add(s); err != nil {
83+
return err
84+
}
85+
return nil
4086
}
4187

4288
func (ss SpaceSubscription) Name() string {

‎server/service/delete_subscription.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,13 @@ import (
99
)
1010

1111
const (
12-
generalDeleteError = "error occurred while deleting subscription with name **%s**"
1312
subscriptionNotFound = "subscription with name **%s** not found"
1413
)
1514

1615
func DeleteSubscription(channelID, alias string) error {
1716
subs, gErr := GetSubscriptions()
1817
if gErr != nil {
19-
return fmt.Errorf(generalDeleteError, alias)
18+
return gErr
2019
}
2120

2221
if channelSubscriptions, valid := subs.ByChannelID[channelID]; valid {
@@ -27,7 +26,10 @@ func DeleteSubscription(channelID, alias string) error {
2726
return nil, err
2827
}
2928

30-
subscription.Remove(subscriptions)
29+
if err := subscription.Remove(subscriptions); err != nil {
30+
return nil, err
31+
}
32+
3133
modifiedBytes, marshalErr := json.Marshal(subscriptions)
3234
if marshalErr != nil {
3335
return nil, marshalErr

‎server/service/edit_subscription.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,11 @@ func EditSubscription(subscription serializer.Subscription) error {
1414
if err != nil {
1515
return nil, err
1616
}
17-
subscription.Edit(subscriptions)
17+
18+
if err := subscription.Edit(subscriptions); err != nil {
19+
return nil, err
20+
}
21+
1822
modifiedBytes, marshalErr := json.Marshal(subscriptions)
1923
if marshalErr != nil {
2024
return nil, marshalErr

‎server/service/save_subscription.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,11 @@ func SaveSubscription(subscription serializer.Subscription) (int, error) {
3131
if err != nil {
3232
return nil, err
3333
}
34-
subscription.Add(subscriptions)
34+
35+
if err := subscription.Add(subscriptions); err != nil {
36+
return nil, err
37+
}
38+
3539
modifiedBytes, marshalErr := json.Marshal(subscriptions)
3640
if marshalErr != nil {
3741
return nil, marshalErr

‎webapp/src/components/subscription_modal/subscription_modal.jsx‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ export default class SubscriptionModal extends React.PureComponent {
139139

140140
let response;
141141
if (subscription && subscription.alias) {
142+
channelSubscription.oldAlias = subscription.alias;
142143
response = await editChannelSubscription(channelSubscription);
143144
} else {
144145
response = await saveChannelSubscription(channelSubscription);

‎webapp/src/components/subscription_modal/subscription_modal.test.jsx‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,8 @@ describe('components/ChannelSettingsModal', () => {
8080
<SubscriptionModal {...props}/>,
8181
);
8282
wrapper.setState({
83-
alias: 'Abc',
83+
oldAlias: 'Abc',
84+
alias: 'Xyz',
8485
baseURL: 'https://test.com',
8586
spaceKey: 'test',
8687
events: Constants.CONFLUENCE_EVENTS,
@@ -92,7 +93,8 @@ describe('components/ChannelSettingsModal', () => {
9293
wrapper.instance().handleSubmit({preventDefault: jest.fn()});
9394
expect(wrapper.state().error).toBe('');
9495
expect(props.editChannelSubscription).toHaveBeenCalledWith({
95-
alias: 'Abc',
96+
oldAlias: 'Abc',
97+
alias: 'Xyz',
9698
baseURL: 'https://test.com',
9799
spaceKey: 'test',
98100
events: Constants.CONFLUENCE_EVENTS.map((event) => event.value),
@@ -138,6 +140,7 @@ describe('components/ChannelSettingsModal', () => {
138140

139141
test('edit page subscription', async () => {
140142
const subscription = {
143+
oldAlias: 'Abc',
141144
alias: 'Abc',
142145
baseURL: 'https://test.com',
143146
spaceKey: 'test',
@@ -153,7 +156,8 @@ describe('components/ChannelSettingsModal', () => {
153156
<SubscriptionModal {...props}/>,
154157
);
155158
wrapper.setState({
156-
alias: 'Abc',
159+
oldAlias: 'Abc',
160+
alias: 'Xyz',
157161
baseURL: 'https://test.com',
158162
spaceKey: '',
159163
events: Constants.CONFLUENCE_EVENTS,
@@ -165,7 +169,8 @@ describe('components/ChannelSettingsModal', () => {
165169
wrapper.instance().handleSubmit({preventDefault: jest.fn()});
166170
expect(wrapper.state().error).toBe('');
167171
expect(props.editChannelSubscription).toHaveBeenCalledWith({
168-
alias: 'Abc',
172+
oldAlias: 'Abc',
173+
alias: 'Xyz',
169174
baseURL: 'https://test.com',
170175
spaceKey: '',
171176
events: Constants.CONFLUENCE_EVENTS.map((event) => event.value),

0 commit comments

Comments
 (0)