Stop Overwriting Order Metadata (#2180)

* Stop using UpdateOrderMetadata

* Add special test cases

services/skus/datastore.go

@@ -919,10 +919,12 @@ func (pg *Postgres) InsertVote(ctx context.Context, vr VoteRecord) error {
 }
 
 // UpdateOrderMetadata sets the order's metadata to the key and value.
+//
+// Deprecated: This method is no longer used and should be deleted.
+//
+// TODO(pavelb): Remove this method as it's dangerous and must not be used.
 func (pg *Postgres) UpdateOrderMetadata(orderID uuid.UUID, key string, value string) error {
-	data := datastore.Metadata{key: value}
-
-	return pg.orderRepo.UpdateMetadata(context.TODO(), pg.RawDB(), orderID, data)
+	return model.Error("UpdateOrderMetadata must not be used")
 }
 
 // TimeLimitedV2Creds represent all the
@@ -1369,7 +1371,7 @@ func (pg *Postgres) AppendOrderMetadataInt(ctx context.Context, orderID *uuid.UU
 }
 
 // AppendOrderMetadata appends the key and string value to an order's metadata.
-func (pg *Postgres) AppendOrderMetadata(ctx context.Context, orderID *uuid.UUID, key string, value string) error {
+func (pg *Postgres) AppendOrderMetadata(ctx context.Context, orderID *uuid.UUID, key, value string) error {
 	_, tx, rollback, commit, err := datastore.GetTx(ctx, pg)
 	if err != nil {
 		return err

services/skus/service.go

@@ -530,44 +530,34 @@ func (s *Service) TransformStripeOrder(order *Order) (*Order, error) {
 	if _, sOK := order.Metadata["stripeSubscriptionId"]; !sOK {
 		if cs, ok := order.Metadata["stripeCheckoutSessionId"].(string); ok && cs != "" {
 			// get old checkout session from stripe by id
-			stripeSession, err := session.Get(cs, nil)
+			sess, err := session.Get(cs, nil)
 			if err != nil {
 				return nil, fmt.Errorf("failed to get stripe checkout session: %w", err)
 			}
 
-			if stripeSession.PaymentStatus == "paid" {
-				// if the session is actually paid, then set the subscription id and order to paid
+			// Set status to paid and the subscription id and if the session is actually paid.
+			if sess.PaymentStatus == "paid" {
 				if err = s.Datastore.UpdateOrder(order.ID, "paid"); err != nil {
 					return nil, fmt.Errorf("failed to update order to paid status: %w", err)
 				}
-				err = s.Datastore.UpdateOrderMetadata(order.ID, "stripeSubscriptionId", stripeSession.Subscription.ID)
-				if err != nil {
+
+				if err := s.Datastore.AppendOrderMetadata(ctx, &order.ID, "stripeSubscriptionId", sess.Subscription.ID); err != nil {
 					return nil, fmt.Errorf("failed to update order to add the subscription id")
 				}
 
-				// TODO(pavelb): Duplicate calls. Remove one.
-
-				// set paymentProcessor as stripe
-				err = s.Datastore.AppendOrderMetadata(context.Background(), &order.ID, paymentProcessor, StripePaymentMethod)
-				if err != nil {
-					return nil, fmt.Errorf("failed to update order to add the payment processor")
-				}
-				// set paymentProcessor as stripe
-				err = s.Datastore.AppendOrderMetadata(ctx, &order.ID, paymentProcessor, StripePaymentMethod)
-				if err != nil {
+				if err := s.Datastore.AppendOrderMetadata(ctx, &order.ID, paymentProcessor, StripePaymentMethod); err != nil {
 					return nil, fmt.Errorf("failed to update order to add the payment processor")
 				}
 			}
 		}
 	}
 
-	// get the order latest state
-	order, err = s.Datastore.GetOrder(order.ID)
+	result, err := s.Datastore.GetOrder(order.ID)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get order: %w", err)
 	}
 
-	return order, nil
+	return result, nil
 }
 
 // CancelOrder cancels an order, propagates to stripe if needed.

services/skus/storage/repository/repository_test.go

@@ -180,6 +180,57 @@ func TestOrder_AppendMetadata(t *testing.T) {
 				},
 			},
 		},
+
+		{
+			name: "stripeSubscriptionId_add_with_no_previous_value",
+			given: tcGiven{
+				data: datastore.Metadata{"key_02_01": "value_02_01"},
+				key:  "stripeSubscriptionId",
+				val:  "9570bf21-98e8-4ddc-950d-a50121d48a0a",
+			},
+			exp: tcExpected{
+				data: datastore.Metadata{
+					"key_02_01":            "value_02_01",
+					"stripeSubscriptionId": "9570bf21-98e8-4ddc-950d-a50121d48a0a",
+				},
+			},
+		},
+
+		{
+			name: "stripeSubscriptionId_no_change",
+			given: tcGiven{
+				data: datastore.Metadata{
+					"key_02_01":            "value_02_01",
+					"stripeSubscriptionId": "9570bf21-98e8-4ddc-950d-a50121d48a0a",
+				},
+				key: "stripeSubscriptionId",
+				val: "9570bf21-98e8-4ddc-950d-a50121d48a0a",
+			},
+			exp: tcExpected{
+				data: datastore.Metadata{
+					"key_02_01":            "value_02_01",
+					"stripeSubscriptionId": "9570bf21-98e8-4ddc-950d-a50121d48a0a",
+				},
+			},
+		},
+
+		{
+			name: "stripeSubscriptionId_replace",
+			given: tcGiven{
+				data: datastore.Metadata{
+					"key_02_01":            "value_02_01",
+					"stripeSubscriptionId": "9570bf21-98e8-4ddc-950d-a50121d48a0a",
+				},
+				key: "stripeSubscriptionId",
+				val: "edfe50f8-06dc-4d5f-a6ca-15c8a1ce6afb",
+			},
+			exp: tcExpected{
+				data: datastore.Metadata{
+					"key_02_01":            "value_02_01",
+					"stripeSubscriptionId": "edfe50f8-06dc-4d5f-a6ca-15c8a1ce6afb",
+				},
+			},
+		},
 	}
 
 	repo := repository.NewOrder()