From 91bed09e849642b03508e87e81a4c576c5402f94 Mon Sep 17 00:00:00 2001 From: olivermrbl Date: Fri, 18 Nov 2022 13:29:42 +0100 Subject: [PATCH] Throw on flat rate shipping options with no amount --- .../src/services/__tests__/shipping-option.js | 41 +++++++++++++------ .../src/services/fulfillment-provider.ts | 8 +++- .../medusa/src/services/shipping-option.ts | 41 +++++++++++++------ 3 files changed, 63 insertions(+), 27 deletions(-) diff --git a/packages/medusa/src/services/__tests__/shipping-option.js b/packages/medusa/src/services/__tests__/shipping-option.js index f483e8c59b702..cfdcb0a1730ed 100644 --- a/packages/medusa/src/services/__tests__/shipping-option.js +++ b/packages/medusa/src/services/__tests__/shipping-option.js @@ -1,8 +1,7 @@ -import _ from "lodash" -import { IdMap, MockRepository, MockManager } from "medusa-test-utils" +import { IdMap, MockManager, MockRepository } from "medusa-test-utils" +import TaxInclusivePricingFeatureFlag from "../../loaders/feature-flags/tax-inclusive-pricing" +import { FlagRouter } from "../../utils/flag-router" import ShippingOptionService from "../shipping-option" -import { FlagRouter } from "../../utils/flag-router"; -import TaxInclusivePricingFeatureFlag from "../../loaders/feature-flags/tax-inclusive-pricing"; describe("ShippingOptionService", () => { describe("retrieve", () => { @@ -36,13 +35,20 @@ describe("ShippingOptionService", () => { provider_id: "no_calc", }) case IdMap.getId("validId"): + return Promise.resolve({ + provider_id: "provider", + amount: 100, + data: { + provider_data: "true", + }, + }) + case "flat-rate-no-amount": return Promise.resolve({ provider_id: "provider", data: { provider_data: "true", }, }) - default: return Promise.resolve({}) } @@ -137,7 +143,7 @@ describe("ShippingOptionService", () => { it("sets flat rate price", async () => { await optionService.update(IdMap.getId("validId"), { price_type: "flat_rate", - amount: 100, + amount: 200, }) expect(shippingOptionRepository.save).toHaveBeenCalledTimes(1) @@ -147,10 +153,23 @@ describe("ShippingOptionService", () => { provider_data: "true", }, price_type: "flat_rate", - amount: 100, + amount: 200, }) }) + it("throws on flat rate but no amount", async () => { + try { + await optionService.update(IdMap.getId("flat-rate-no-amount"), { + price_type: "flat_rate", + amount: 100, + }) + } catch (error) { + expect(error.message).toEqual( + "Flat rate shipping options must have an amount" + ) + } + }) + it("sets calculated price", async () => { await optionService.update(IdMap.getId("validId"), { price_type: "calculated", @@ -158,10 +177,8 @@ describe("ShippingOptionService", () => { expect(fulfillmentProviderService.canCalculate).toHaveBeenCalledTimes(1) expect(fulfillmentProviderService.canCalculate).toHaveBeenCalledWith({ - amount: null, data: { provider_data: "true" }, provider_id: "provider", - price_type: "calculated", }) expect(shippingOptionRepository.save).toHaveBeenCalledTimes(1) @@ -645,7 +662,7 @@ describe("ShippingOptionService", () => { totalsService, fulfillmentProviderService: providerService, featureFlagRouter: new FlagRouter({ - [TaxInclusivePricingFeatureFlag.key]: true + [TaxInclusivePricingFeatureFlag.key]: true, }), }) @@ -653,11 +670,11 @@ describe("ShippingOptionService", () => { jest.clearAllMocks() }) - it("should create a shipping method that also includes the taxes", async () => { + it("should create a shipping method that also includes the taxes", async () => { await optionService.createShippingMethod("random_id", {}, { price: 10 }) expect(shippingMethodRepository.save).toHaveBeenCalledWith( expect.objectContaining({ - includes_tax: true + includes_tax: true, }) ) }) diff --git a/packages/medusa/src/services/fulfillment-provider.ts b/packages/medusa/src/services/fulfillment-provider.ts index 6960a510d895f..8a9b475c3f217 100644 --- a/packages/medusa/src/services/fulfillment-provider.ts +++ b/packages/medusa/src/services/fulfillment-provider.ts @@ -8,7 +8,6 @@ import { FulfillmentProvider, LineItem, Order, - Return, ShippingMethod, ShippingOption, } from "../models" @@ -29,6 +28,11 @@ type FulfillmentProviderContainer = MedusaContainer & { [key in `${FulfillmentProviderKey}`]: typeof BaseFulfillmentService } +type CalculateOptionPriceInput = { + provider_id: string + data: Record +} + /** * Helps retrive fulfillment providers */ @@ -119,7 +123,7 @@ class FulfillmentProviderService extends TransactionBaseService { ) as unknown as Record } - async canCalculate(option: ShippingOption): Promise { + async canCalculate(option: CalculateOptionPriceInput): Promise { const provider = this.retrieveProvider(option.provider_id) return provider.canCalculate(option.data) as unknown as boolean } diff --git a/packages/medusa/src/services/shipping-option.ts b/packages/medusa/src/services/shipping-option.ts index 58249923598e7..7c2f1397477d4 100644 --- a/packages/medusa/src/services/shipping-option.ts +++ b/packages/medusa/src/services/shipping-option.ts @@ -1,6 +1,7 @@ import { MedusaError } from "medusa-core-utils" import { DeepPartial, EntityManager } from "typeorm" import { TransactionBaseService } from "../interfaces" +import TaxInclusivePricingFeatureFlag from "../loaders/feature-flags/tax-inclusive-pricing" import { Cart, Order, @@ -15,15 +16,14 @@ import { ShippingOptionRequirementRepository } from "../repositories/shipping-op import { ExtendedFindConfig, FindConfig, Selector } from "../types/common" import { CreateShippingMethodDto, + CreateShippingOptionInput, ShippingMethodUpdate, UpdateShippingOptionInput, - CreateShippingOptionInput, } from "../types/shipping-options" import { buildQuery, isDefined, setMetadata } from "../utils" +import { FlagRouter } from "../utils/flag-router" import FulfillmentProviderService from "./fulfillment-provider" import RegionService from "./region" -import { FlagRouter } from "../utils/flag-router" -import TaxInclusivePricingFeatureFlag from "../loaders/feature-flags/tax-inclusive-pricing" type InjectedDependencies = { manager: EntityManager @@ -409,6 +409,8 @@ class ShippingOptionService extends TransactionBaseService { const optionRepo = manager.getCustomRepository(this.optionRepository_) const option = optionRepo.create(data as DeepPartial) + option.price_type = await this.validatePriceType_(data.price_type, option) + const region = await this.regionService_ .withTransaction(manager) .retrieve(option.region_id, { @@ -426,9 +428,10 @@ class ShippingOptionService extends TransactionBaseService { ) } - option.price_type = await this.validatePriceType_(data.price_type, option) option.amount = - data.price_type === "calculated" ? null : data.amount ?? null + data.price_type === ShippingOptionPriceType.CALCULATED + ? null + : data.amount ?? null if ( this.featureFlagRouter_.isFeatureEnabled( @@ -496,7 +499,8 @@ class ShippingOptionService extends TransactionBaseService { ): Promise { if ( !priceType || - (priceType !== "flat_rate" && priceType !== "calculated") + (priceType !== ShippingOptionPriceType.FLAT_RATE && + priceType !== ShippingOptionPriceType.CALCULATED) ) { throw new MedusaError( MedusaError.Types.INVALID_DATA, @@ -504,8 +508,11 @@ class ShippingOptionService extends TransactionBaseService { ) } - if (priceType === "calculated") { - const canCalculate = await this.providerService_.canCalculate(option) + if (priceType === ShippingOptionPriceType.CALCULATED) { + const canCalculate = await this.providerService_.canCalculate({ + provider_id: option.provider_id, + data: option.data, + }) if (!canCalculate) { throw new MedusaError( MedusaError.Types.INVALID_DATA, @@ -514,6 +521,13 @@ class ShippingOptionService extends TransactionBaseService { } } + if (priceType === ShippingOptionPriceType.FLAT_RATE && !option.amount) { + throw new MedusaError( + MedusaError.Types.INVALID_DATA, + "Flat rate shipping options must have an amount" + ) + } + return priceType } @@ -597,20 +611,21 @@ class ShippingOptionService extends TransactionBaseService { option.requirements = acc } + if (isDefined(update.amount)) { + option.amount = update.amount + } + if (isDefined(update.price_type)) { option.price_type = await this.validatePriceType_( update.price_type, option ) - if (update.price_type === "calculated") { + + if (update.price_type === ShippingOptionPriceType.CALCULATED) { option.amount = null } } - if (isDefined(update.amount) && option.price_type !== "calculated") { - option.amount = update.amount - } - if (isDefined(update.name)) { option.name = update.name }