Merge pull request #2113 from brave-intl/master

Production 2023-10-12_01

.github/PULL_REQUEST_TEMPLATE.md

@@ -2,7 +2,8 @@
 
 <!-- What does this pr do? Use the fixes syntax where possible (fixes #x) -->
 
-### Type of change ( select one )
+
+### Type of Change
 
 - [ ] Product feature
 - [ ] Bug fix
@@ -12,13 +13,15 @@
 
 <!-- Provide link if applicable. -->
 
+
 ### Tested Environments
 
 - [ ] Development
 - [ ] Staging
 - [ ] Production
 
-### Before submitting this PR:
+
+### Before Requesting Review
 
 - [ ] Does your code build cleanly without any errors or warnings?
 - [ ] Have you used auto closing keywords?
@@ -28,9 +31,10 @@
 - [ ] Have you squashed all intermediate commits?
 - [ ] Is there a clear title that explains what the PR does?
 - [ ] Have you used intuitive function, variable and other naming?
-- [ ] Have you requested security / privacy review if needed
+- [ ] Have you requested security and/or privacy review if needed
 - [ ] Have you performed a self review of this PR?
 
-### Manual Test Plan:
+
+### Manual Test Plan
 
 <!-- if needed - e.g. prod branch release PR, otherwise remove this section -->

services/skus/controllers.go

@@ -275,81 +275,70 @@ func VoteRouter(service *Service, instrumentHandler middleware.InstrumentHandler
 	return r
 }
 
-// SetOrderTrialDaysInput - SetOrderTrialDays handler input
-type SetOrderTrialDaysInput struct {
+type setTrialDaysRequest struct {
 	TrialDays int64 `json:"trialDays" valid:"int"`
 }
 
-// SetOrderTrialDays is the handler for cancelling an order
+// SetOrderTrialDays handles requests for setting trial days on orders.
 func SetOrderTrialDays(service *Service) handlers.AppHandler {
 	return handlers.AppHandler(func(w http.ResponseWriter, r *http.Request) *handlers.AppError {
-		var (
-			ctx     = r.Context()
-			orderID = new(inputs.ID)
-		)
-		if err := inputs.DecodeAndValidateString(context.Background(), orderID, chi.URLParam(r, "orderID")); err != nil {
+		ctx := r.Context()
+		orderID := &inputs.ID{}
+
+		if err := inputs.DecodeAndValidateString(ctx, orderID, chi.URLParam(r, "orderID")); err != nil {
 			return handlers.ValidationError(
 				"Error validating request url parameter",
-				map[string]interface{}{
-					"orderID": err.Error(),
-				},
+				map[string]interface{}{"orderID": err.Error()},
 			)
 		}
 
 		// validate order merchant and caveats (to make sure this is the right merch)
-		if err := service.ValidateOrderMerchantAndCaveats(r, *orderID.UUID()); err != nil {
+		if err := service.validateOrderMerchantAndCaveats(ctx, *orderID.UUID()); err != nil {
 			return handlers.ValidationError(
 				"Error validating request merchant and caveats",
-				map[string]interface{}{
-					"orderMerchantAndCaveats": err.Error(),
-				},
+				map[string]interface{}{"orderMerchantAndCaveats": err.Error()},
 			)
 		}
 
-		var input SetOrderTrialDaysInput
-		err := requestutils.ReadJSON(r.Context(), r.Body, &input)
-		if err != nil {
+		req := &setTrialDaysRequest{}
+		if err := requestutils.ReadJSON(ctx, r.Body, req); err != nil {
 			return handlers.WrapError(err, "Error in request body", http.StatusBadRequest)
 		}
 
-		_, err = govalidator.ValidateStruct(input)
-		if err != nil {
+		if _, err := govalidator.ValidateStruct(req); err != nil {
 			return handlers.WrapValidationError(err)
 		}
 
-		err = service.SetOrderTrialDays(ctx, orderID.UUID(), input.TrialDays)
-		if err != nil {
+		if err := service.SetOrderTrialDays(ctx, orderID.UUID(), req.TrialDays); err != nil {
 			return handlers.WrapError(err, "Error setting the trial days on the order", http.StatusInternalServerError)
 		}
 
-		return handlers.RenderContent(r.Context(), nil, w, http.StatusOK)
+		return handlers.RenderContent(ctx, nil, w, http.StatusOK)
 	})
 }
 
-// CancelOrder is the handler for cancelling an order
+// CancelOrder handles requests for cancelling orders.
 func CancelOrder(service *Service) handlers.AppHandler {
 	return handlers.AppHandler(func(w http.ResponseWriter, r *http.Request) *handlers.AppError {
-		var orderID = new(inputs.ID)
-		if err := inputs.DecodeAndValidateString(context.Background(), orderID, chi.URLParam(r, "orderID")); err != nil {
+		ctx := r.Context()
+		orderID := &inputs.ID{}
+
+		if err := inputs.DecodeAndValidateString(ctx, orderID, chi.URLParam(r, "orderID")); err != nil {
 			return handlers.ValidationError(
 				"Error validating request url parameter",
-				map[string]interface{}{
-					"orderID": err.Error(),
-				},
+				map[string]interface{}{"orderID": err.Error()},
 			)
 		}
 
-		err := service.ValidateOrderMerchantAndCaveats(r, *orderID.UUID())
-		if err != nil {
+		if err := service.validateOrderMerchantAndCaveats(ctx, *orderID.UUID()); err != nil {
 			return handlers.WrapError(err, "Error validating auth merchant and caveats", http.StatusForbidden)
 		}
 
-		err = service.CancelOrder(*orderID.UUID())
-		if err != nil {
+		if err := service.CancelOrder(*orderID.UUID()); err != nil {
 			return handlers.WrapError(err, "Error retrieving the order", http.StatusInternalServerError)
 		}
 
-		return handlers.RenderContent(r.Context(), nil, w, http.StatusOK)
+		return handlers.RenderContent(ctx, nil, w, http.StatusOK)
 	})
 }
 
@@ -656,33 +645,28 @@ func GetOrderCreds(service *Service) handlers.AppHandler {
 	}
 }
 
-// DeleteOrderCreds is the handler for deleting order credentials
+// DeleteOrderCreds handles requests for deleting order credentials.
 func DeleteOrderCreds(service *Service) handlers.AppHandler {
 	return func(w http.ResponseWriter, r *http.Request) *handlers.AppError {
-		var orderID = new(inputs.ID)
-		if err := inputs.DecodeAndValidateString(context.Background(), orderID, chi.URLParam(r, "orderID")); err != nil {
+		ctx := r.Context()
+		orderID := &inputs.ID{}
+		if err := inputs.DecodeAndValidateString(ctx, orderID, chi.URLParam(r, "orderID")); err != nil {
 			return handlers.ValidationError(
 				"Error validating request url parameter",
-				map[string]interface{}{
-					"orderID": err.Error(),
-				},
+				map[string]interface{}{"orderID": err.Error()},
 			)
 		}
 
-		err := service.ValidateOrderMerchantAndCaveats(r, *orderID.UUID())
-		if err != nil {
+		if err := service.validateOrderMerchantAndCaveats(ctx, *orderID.UUID()); err != nil {
 			return handlers.WrapError(err, "Error validating auth merchant and caveats", http.StatusForbidden)
 		}
 
-		// is signed param
 		isSigned := r.URL.Query().Get("isSigned") == "true"
-
-		err = service.DeleteOrderCreds(r.Context(), *orderID.UUID(), isSigned)
-		if err != nil {
+		if err := service.DeleteOrderCreds(ctx, *orderID.UUID(), isSigned); err != nil {
 			return handlers.WrapError(err, "Error deleting credentials", http.StatusBadRequest)
 		}
 
-		return handlers.RenderContent(r.Context(), "Order credentials successfully deleted", w, http.StatusOK)
+		return handlers.RenderContent(ctx, "Order credentials successfully deleted", w, http.StatusOK)
 	}
 }
 

services/skus/datastore.go

@@ -32,11 +32,11 @@ const (
 	errNotFound = model.Error("not found")
 )
 
-// Datastore abstracts over the underlying datastore
+// Datastore abstracts over the underlying datastore.
 type Datastore interface {
 	datastore.Datastore
-	// CreateOrder is used to create an order for payments
-	CreateOrder(totalPrice decimal.Decimal, merchantID string, status string, currency string, location string, validFor *time.Duration, orderItems []OrderItem, allowedPaymentMethods []string) (*Order, error)
+
+	CreateOrder(ctx context.Context, dbi sqlx.ExtContext, oreq *model.OrderNew, items []model.OrderItem) (*model.Order, error)
 	// SetOrderTrialDays - set the number of days of free trial for this order
 	SetOrderTrialDays(ctx context.Context, orderID *uuid.UUID, days int64) (*Order, error)
 	// GetOrder by ID
@@ -101,14 +101,7 @@ type Datastore interface {
 type orderStore interface {
 	Get(ctx context.Context, dbi sqlx.QueryerContext, id uuid.UUID) (*model.Order, error)
 	GetByExternalID(ctx context.Context, dbi sqlx.QueryerContext, extID string) (*model.Order, error)
-	Create(
-		ctx context.Context,
-		dbi sqlx.QueryerContext,
-		totalPrice decimal.Decimal,
-		merchantID, status, currency, location string,
-		paymentMethods []string,
-		validFor *time.Duration,
-	) (*model.Order, error)
+	Create(ctx context.Context, dbi sqlx.QueryerContext, oreq *model.OrderNew) (*model.Order, error)
 	SetLastPaidAt(ctx context.Context, dbi sqlx.ExecerContext, id uuid.UUID, when time.Time) error
 	SetTrialDays(ctx context.Context, dbi sqlx.QueryerContext, id uuid.UUID, ndays int64) (*model.Order, error)
 	SetStatus(ctx context.Context, dbi sqlx.ExecerContext, id uuid.UUID, status string) error
@@ -301,38 +294,26 @@ func (pg *Postgres) SetOrderTrialDays(ctx context.Context, orderID *uuid.UUID, d
 	return result, nil
 }
 
-// CreateOrder creates an order with the given total price, merchant ID, status and orderItems.
-func (pg *Postgres) CreateOrder(totalPrice decimal.Decimal, merchantID, status, currency, location string, validFor *time.Duration, orderItems []OrderItem, allowedPaymentMethods []string) (*Order, error) {
-	tx, err := pg.RawDB().Beginx()
+// CreateOrder creates an order from the given prototype, and inserts items.
+func (pg *Postgres) CreateOrder(ctx context.Context, dbi sqlx.ExtContext, oreq *model.OrderNew, items []model.OrderItem) (*model.Order, error) {
+	result, err := pg.orderRepo.Create(ctx, dbi, oreq)
 	if err != nil {
 		return nil, err
 	}
-	defer pg.RollbackTx(tx)
-
-	ctx := context.TODO()
 
-	result, err := pg.orderRepo.Create(ctx, tx, totalPrice, merchantID, status, currency, location, allowedPaymentMethods, validFor)
-	if err != nil {
-		return nil, err
-	}
-
-	if status == OrderStatusPaid {
-		if err := pg.recordOrderPayment(ctx, tx, result.ID, time.Now()); err != nil {
+	if oreq.Status == OrderStatusPaid {
+		if err := pg.recordOrderPayment(ctx, dbi, result.ID, time.Now()); err != nil {
 			return nil, fmt.Errorf("failed to record order payment: %w", err)
 		}
 	}
 
-	model.OrderItemList(orderItems).SetOrderID(result.ID)
+	model.OrderItemList(items).SetOrderID(result.ID)
 
-	result.Items, err = pg.orderItemRepo.InsertMany(ctx, tx, orderItems...)
+	result.Items, err = pg.orderItemRepo.InsertMany(ctx, dbi, items...)
 	if err != nil {
 		return nil, err
 	}
 
-	if err := tx.Commit(); err != nil {
-		return nil, err
-	}
-
 	return result, nil
 }
 

services/skus/datastore_test.go

@@ -4,6 +4,7 @@ package skus
 
 import (
 	"context"
+	"database/sql"
 	"encoding/json"
 	"os"
 	"strings"
@@ -13,6 +14,7 @@ import (
 	"github.com/DATA-DOG/go-sqlmock"
 	"github.com/golang/mock/gomock"
 	"github.com/jmoiron/sqlx"
+	"github.com/lib/pq"
 	uuid "github.com/satori/go.uuid"
 	"github.com/shopspring/decimal"
 	must "github.com/stretchr/testify/require"
@@ -471,16 +473,21 @@ func createOrderAndIssuer(t *testing.T, ctx context.Context, storage Datastore,
 	}
 
 	validFor := 3600 * time.Second * 24
-	order, err := storage.CreateOrder(
-		decimal.NewFromInt32(int32(test.RandomInt())),
-		test.RandomString(),
-		OrderStatusPaid,
-		test.RandomString(),
-		test.RandomString(),
-		&validFor,
-		orderItems,
-		methods,
-	)
+
+	oreq := &model.OrderNew{
+		MerchantID: test.RandomString(),
+		Currency:   test.RandomString(),
+		Status:     model.OrderStatusPaid,
+		TotalPrice: decimal.NewFromInt(int64(test.RandomInt())),
+		Location: sql.NullString{
+			Valid:  true,
+			String: test.RandomString(),
+		},
+		AllowedPaymentMethods: pq.StringArray(methods),
+		ValidFor:              &validFor,
+	}
+
+	order, err := storage.CreateOrder(ctx, storage.RawDB(), oreq, orderItems)
 	must.NoError(t, err)
 
 	{
@@ -514,16 +521,19 @@ func (suite *PostgresTestSuite) createTimeLimitedV2OrderCreds(t *testing.T, ctx
 		methods = append(methods, method...)
 	}
 
-	order, err := suite.storage.CreateOrder(
-		decimal.NewFromInt32(int32(test.RandomInt())),
-		test.RandomString(),
-		OrderStatusPaid,
-		test.RandomString(),
-		test.RandomString(),
-		nil,
-		orderItems,
-		methods,
-	)
+	oreq := &model.OrderNew{
+		MerchantID: test.RandomString(),
+		Currency:   test.RandomString(),
+		Status:     model.OrderStatusPaid,
+		TotalPrice: decimal.NewFromInt(int64(test.RandomInt())),
+		Location: sql.NullString{
+			Valid:  true,
+			String: test.RandomString(),
+		},
+		AllowedPaymentMethods: pq.StringArray(methods),
+	}
+
+	order, err := suite.storage.CreateOrder(ctx, suite.storage.RawDB(), oreq, orderItems)
 	must.NoError(t, err)
 
 	repo := repository.NewIssuer()
@@ -595,16 +605,19 @@ func (suite *PostgresTestSuite) createOrderCreds(t *testing.T, ctx context.Conte
 		methods = append(methods, method...)
 	}
 
-	order, err := suite.storage.CreateOrder(
-		decimal.NewFromInt32(int32(test.RandomInt())),
-		test.RandomString(),
-		OrderStatusPaid,
-		test.RandomString(),
-		test.RandomString(),
-		nil,
-		orderItems,
-		methods,
-	)
+	oreq := &model.OrderNew{
+		MerchantID: test.RandomString(),
+		Currency:   test.RandomString(),
+		Status:     model.OrderStatusPaid,
+		TotalPrice: decimal.NewFromInt(int64(test.RandomInt())),
+		Location: sql.NullString{
+			Valid:  true,
+			String: test.RandomString(),
+		},
+		AllowedPaymentMethods: pq.StringArray(methods),
+	}
+
+	order, err := suite.storage.CreateOrder(ctx, suite.storage.RawDB(), oreq, orderItems)
 	must.NoError(t, err)
 
 	pk := test.RandomString()