Merge pull request #587 from labd/584-modifying-a-subscription-fails

fix: added ordering of subscription messages and changes during update

Author: demeyerthom

.changes/unreleased/Fixed-20250509-141307.yaml

@@ -0,0 +1,3 @@
+kind: Fixed
+body: Added ordering of subscription messages and changes during update
+time: 2025-05-09T14:13:07.20165246+02:00

internal/resources/subscription/model.go

@@ -102,6 +102,32 @@ func (s *Subscription) draft() platform.SubscriptionDraft {
 	return draft
 }
 
+// orderChangesAndMessagesActions orders the changes and messages actions. This ensures that if both are present but one
+// has an empty list of changes the action with an empty list will be processed last. This is important because
+// otherwise Commercetools will throw an error that a subscription with an empty list of changes and messages
+func orderChangesAndMessagesActions(
+	changesAction *platform.SubscriptionSetChangesAction,
+	messagesAction *platform.SubscriptionSetMessagesAction,
+) []platform.SubscriptionUpdateAction {
+	if changesAction == nil && messagesAction == nil {
+		return nil
+	}
+
+	if changesAction == nil {
+		return []platform.SubscriptionUpdateAction{*messagesAction}
+	}
+
+	if messagesAction == nil {
+		return []platform.SubscriptionUpdateAction{*changesAction}
+	}
+
+	if len(changesAction.Changes) >= len(messagesAction.Messages) {
+		return []platform.SubscriptionUpdateAction{*changesAction, *messagesAction}
+	}
+
+	return []platform.SubscriptionUpdateAction{*messagesAction, *changesAction}
+}
+
 func (s *Subscription) updateActions(plan Subscription) platform.SubscriptionUpdate {
 	result := platform.SubscriptionUpdate{
 		Version: int(s.Version.ValueInt64()),
@@ -129,31 +155,27 @@ func (s *Subscription) updateActions(plan Subscription) platform.SubscriptionUpd
 	}
 
 	// setChanges
+	var changesAction *platform.SubscriptionSetChangesAction
 	if !reflect.DeepEqual(s.Changes, plan.Changes) {
 		var changes = make([]platform.ChangeSubscription, 0, len(plan.Changes))
 		for _, c := range plan.Changes {
 			changes = append(changes, c.toNative()...)
 		}
-
-		result.Actions = append(
-			result.Actions,
-			platform.SubscriptionSetChangesAction{
-				Changes: changes,
-			})
+		changesAction = &platform.SubscriptionSetChangesAction{Changes: changes}
 	}
 
 	// setMessages
+	var messagesAction *platform.SubscriptionSetMessagesAction
 	if !reflect.DeepEqual(s.Messages, plan.Messages) {
 		var messages = make([]platform.MessageSubscription, 0, len(plan.Messages))
 		for _, m := range plan.Messages {
 			messages = append(messages, m.toNative())
 		}
-
-		result.Actions = append(
-			result.Actions,
-			platform.SubscriptionSetMessagesAction{Messages: messages})
+		messagesAction = &platform.SubscriptionSetMessagesAction{Messages: messages}
 	}
 
+	result.Actions = append(result.Actions, orderChangesAndMessagesActions(changesAction, messagesAction)...)
+
 	return result
 }
 

internal/resources/subscription/model_test.go

@@ -306,3 +306,72 @@ func TestUpdateActions(t *testing.T) {
 		})
 	}
 }
+
+func TestOrderChangesAndMessagesActionsEmpty(t *testing.T) {
+	assert.Nil(t, orderChangesAndMessagesActions(nil, nil))
+}
+
+func TestOrderChangesAndMessagesActionsWithChanges(t *testing.T) {
+	actions := orderChangesAndMessagesActions(&platform.SubscriptionSetChangesAction{
+		Changes: []platform.ChangeSubscription{
+			{ResourceTypeId: "test"},
+		},
+	}, nil)
+	assert.Len(t, actions, 1)
+}
+
+func TestOrderChangesAndMessagesActionsWithMessages(t *testing.T) {
+	actions := orderChangesAndMessagesActions(nil, &platform.SubscriptionSetMessagesAction{
+		Messages: []platform.MessageSubscription{
+			{ResourceTypeId: "test"},
+		},
+	})
+	assert.Len(t, actions, 1)
+}
+
+func TestOrderChangesAndMessagesActionsWithEmptyChanges(t *testing.T) {
+	actions := orderChangesAndMessagesActions(
+		&platform.SubscriptionSetChangesAction{
+			Changes: []platform.ChangeSubscription{},
+		},
+		&platform.SubscriptionSetMessagesAction{
+			Messages: []platform.MessageSubscription{
+				{ResourceTypeId: "test"},
+			},
+		})
+	assert.Len(t, actions, 2)
+	assert.IsType(t, platform.SubscriptionSetMessagesAction{}, actions[0])
+	assert.IsType(t, platform.SubscriptionSetChangesAction{}, actions[1])
+}
+
+func TestOrderChangesAndMessagesActionsWithEmptyMessages(t *testing.T) {
+	actions := orderChangesAndMessagesActions(
+		&platform.SubscriptionSetChangesAction{
+			Changes: []platform.ChangeSubscription{
+				{ResourceTypeId: "test"},
+			},
+		},
+		&platform.SubscriptionSetMessagesAction{
+			Messages: []platform.MessageSubscription{},
+		})
+	assert.Len(t, actions, 2)
+	assert.IsType(t, platform.SubscriptionSetChangesAction{}, actions[0])
+	assert.IsType(t, platform.SubscriptionSetMessagesAction{}, actions[1])
+}
+
+func TestOrderChangesAndMessagesActionsWithBoth(t *testing.T) {
+	actions := orderChangesAndMessagesActions(
+		&platform.SubscriptionSetChangesAction{
+			Changes: []platform.ChangeSubscription{
+				{ResourceTypeId: "test"},
+			},
+		},
+		&platform.SubscriptionSetMessagesAction{
+			Messages: []platform.MessageSubscription{
+				{ResourceTypeId: "test"},
+			},
+		})
+	assert.Len(t, actions, 2)
+	assert.IsType(t, platform.SubscriptionSetChangesAction{}, actions[0])
+	assert.IsType(t, platform.SubscriptionSetMessagesAction{}, actions[1])
+}