From 0788e9ef44656e702b064b2c44271d300e98ebf0 Mon Sep 17 00:00:00 2001 From: Pavel Brm <5097196+pavelbrm@users.noreply.github.com> Date: Sat, 28 Oct 2023 00:13:52 +1300 Subject: [PATCH] Stop Overwriting Order Metadata (#2180) * Stop using UpdateOrderMetadata * Add special test cases --- services/skus/datastore.go | 10 ++-- services/skus/service.go | 26 +++------- .../storage/repository/repository_test.go | 51 +++++++++++++++++++ 3 files changed, 65 insertions(+), 22 deletions(-) diff --git a/services/skus/datastore.go b/services/skus/datastore.go index f4f0ba670..3eb04b909 100644 --- a/services/skus/datastore.go +++ b/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 diff --git a/services/skus/service.go b/services/skus/service.go index f4844ec85..97013902e 100644 --- a/services/skus/service.go +++ b/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. diff --git a/services/skus/storage/repository/repository_test.go b/services/skus/storage/repository/repository_test.go index 86c318576..2a2f8cbaf 100644 --- a/services/skus/storage/repository/repository_test.go +++ b/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()