refactor(medusa): Cleanup and fix CartService (#1306)

* refactor(medusa): Cleanup + fix

* styles(medusa): Lint

* refactor(medusa): Finalize cleanup

* feat(medusa): Prefer the usage of bulk operations instead of sequential/conccurent operations

* feat(medusa): Improve cart service

* refactor(medusa): Explicitly specifying protected methods when needed as well as enfore the usage of the local transactionManager_ in those methods

* tests(medusa): Fix tests according to the new changes

* feat(medusa): Cleanup after rebase

* test(medusa): Fix cart service tests

integration-tests/api/__tests__/store/__snapshots__/cart.js.snap

@@ -24,7 +24,7 @@ Object {
 }
 `;
 
-exports[`/store/carts shipping address + region updates updates region only - single to multipe countries 1`] = `
+exports[`/store/carts shipping address + region updates updates region only - single to multiple countries 1`] = `
 Object {
   "address_1": null,
   "address_2": null,

integration-tests/api/__tests__/store/cart.js

@@ -924,19 +924,19 @@ describe("/store/carts", () => {
     })
 
     it("fails on apply discount if limit has been reached", async () => {
-      expect.assertions(2)
       const api = useApi()
 
-      await api
+      const err = await api
         .post("/store/carts/test-cart", {
           discounts: [{ code: "SPENT" }],
         })
-        .catch((error) => {
-          expect(error.response.status).toEqual(400)
-          expect(error.response.data.message).toEqual(
-            "Discount has been used maximum allowed times"
-          )
-        })
+        .catch((err) => err)
+
+      expect(err).toBeTruthy()
+      expect(err.response.status).toEqual(400)
+      expect(err.response.data.message).toEqual(
+        "Discount has been used maximum allowed times"
+      )
     })
 
     it("successfully passes customer conditions with `in` operator and applies discount", async () => {
@@ -1755,7 +1755,7 @@ describe("/store/carts", () => {
           }),
         ])
       )
-      expect(cartWithGiftcard.data.cart.total).toBe(1900) // 1000 (giftcard) + 900 (standard item with 10% discount)
+      expect(cartWithGiftcard.data.cart.total).toBe(2900) // 1000 (giftcard) + 900 (standard item with 10% discount) + 1000 Shipping
       expect(cartWithGiftcard.data.cart.discount_total).toBe(100)
       expect(cartWithGiftcard.status).toEqual(200)
     })
@@ -1925,7 +1925,7 @@ describe("/store/carts", () => {
       await doAfterEach()
     })
 
-    it("updates region only - single to multipe countries", async () => {
+    it("updates region only - single to multiple countries", async () => {
       const api = useApi()
 
       const { data, status } = await api
@@ -1947,7 +1947,7 @@ describe("/store/carts", () => {
       })
     })
 
-    it("updates region only - single to multipe countries", async () => {
+    it("should reset the shipping_address on null value", async () => {
       const api = useApi()
 
       const { data, status } = await api
@@ -1962,7 +1962,5 @@ describe("/store/carts", () => {
       expect(status).toEqual(200)
       expect(data.cart.shipping_address).toEqual(null)
     })
-
-    // it("updates cart.customer_id on cart retrieval if cart.customer_id differ from session customer", async () => {})
   })
 })

packages/medusa-interfaces/src/base-service.js

@@ -228,8 +228,7 @@ class BaseService {
       }
 
       try {
-        const result = await this.manager_.transaction((m) => doWork(m))
-        return result
+        return await this.manager_.transaction((m) => doWork(m))
       } catch (error) {
         if (errorHandler) {
           const result = await errorHandler(error)

packages/medusa/src/api/routes/store/carts/update-cart.ts

@@ -90,23 +90,20 @@ export default async (req, res) => {
   // Update the cart
   const { shipping_address, billing_address, ...rest } = validated
 
-  const toUpdate: CartUpdateProps = {
-    ...rest,
-  }
-
+  const cartDataToUpdate: CartUpdateProps = { ...rest };
   if (typeof shipping_address === "string") {
-    toUpdate.shipping_address_id = shipping_address
+    cartDataToUpdate.shipping_address_id = shipping_address
   } else {
-    toUpdate.shipping_address = shipping_address
+    cartDataToUpdate.shipping_address = shipping_address
   }
 
   if (typeof billing_address === "string") {
-    toUpdate.billing_address_id = billing_address
+    cartDataToUpdate.billing_address_id = billing_address
   } else {
-    toUpdate.billing_address = billing_address
+    cartDataToUpdate.billing_address = billing_address
   }
 
-  await cartService.update(id, toUpdate)
+  await cartService.update(id, cartDataToUpdate)
 
   // If the cart has payment sessions update these
   const updated = await cartService.retrieve(id, {

packages/medusa/src/loaders/search-index.ts

@@ -4,7 +4,9 @@ import { MedusaContainer } from "../types/global"
 import DefaultSearchService from "../services/search"
 import { Logger } from "../types/global"
 
-async function loadProductsIntoSearchEngine(container: MedusaContainer): Promise<void> {
+async function loadProductsIntoSearchEngine(
+  container: MedusaContainer
+): Promise<void> {
   const searchService = container.resolve<DefaultSearchService>("searchService")
   const productService = container.resolve<ProductService>("productService")
 
@@ -61,7 +63,11 @@ async function loadProductsIntoSearchEngine(container: MedusaContainer): Promise
   }
 }
 
-export default async ({ container }: { container: MedusaContainer }): Promise<void> => {
+export default async ({
+  container,
+}: {
+  container: MedusaContainer
+}): Promise<void> => {
   const searchService = container.resolve<DefaultSearchService>("searchService")
   const logger = container.resolve<Logger>("logger")
   if (searchService.isDefault) {

packages/medusa/src/repositories/cart.ts

@@ -5,7 +5,7 @@ import { Cart } from "../models/cart"
 @EntityRepository(Cart)
 export class CartRepository extends Repository<Cart> {
   public async findWithRelations(
-    relations: Array<keyof Cart> = [],
+    relations: string[] = [],
     optionsWithoutRelations: Omit<FindManyOptions<Cart>, "relations"> = {}
   ): Promise<Cart[]> {
     const entities = await this.find(optionsWithoutRelations)
@@ -38,7 +38,7 @@ export class CartRepository extends Repository<Cart> {
   }
 
   public async findOneWithRelations(
-    relations: Array<keyof Cart> = [],
+    relations: string[] = [],
     optionsWithoutRelations: Omit<FindManyOptions<Cart>, "relations"> = {}
   ): Promise<Cart> {
     // Limit 1

packages/medusa/src/services/__tests__/cart.js

@@ -193,6 +193,9 @@ describe("CartService", () => {
 
   describe("create", () => {
     const regionService = {
+      withTransaction: function() {
+        return this
+      },
       retrieve: () => {
         return {
           id: IdMap.getId("testRegion"),
@@ -324,7 +327,7 @@ describe("CartService", () => {
     }
 
     const shippingOptionService = {
-      deleteShippingMethod: jest.fn(),
+      deleteShippingMethods: jest.fn(),
       withTransaction: function() {
         return this
       },
@@ -378,6 +381,7 @@ describe("CartService", () => {
       totalsService,
       cartRepository,
       lineItemService,
+      lineItemRepository: MockRepository(),
       eventBusService,
       shippingOptionService,
       inventoryService,
@@ -476,7 +480,7 @@ describe("CartService", () => {
 
       await cartService.addLineItem(IdMap.getId("cartWithLine"), lineItem)
 
-      expect(lineItemService.update).toHaveBeenCalledTimes(2)
+      expect(lineItemService.update).toHaveBeenCalledTimes(1)
       expect(lineItemService.update).toHaveBeenCalledWith(
         IdMap.getId("merger"),
         {
@@ -581,7 +585,7 @@ describe("CartService", () => {
     })
 
     const shippingOptionService = {
-      deleteShippingMethod: jest.fn(),
+      deleteShippingMethods: jest.fn(),
       withTransaction: function() {
         return this
       },
@@ -592,6 +596,7 @@ describe("CartService", () => {
       totalsService,
       cartRepository,
       lineItemService,
+      lineItemRepository: MockRepository(),
       shippingOptionService,
       eventBusService,
       lineItemAdjustmentService: LineItemAdjustmentServiceMock,
@@ -641,15 +646,15 @@ describe("CartService", () => {
         IdMap.getId("itemToRemove")
       )
 
-      expect(shippingOptionService.deleteShippingMethod).toHaveBeenCalledTimes(
+      expect(shippingOptionService.deleteShippingMethods).toHaveBeenCalledTimes(
         1
       )
-      expect(shippingOptionService.deleteShippingMethod).toHaveBeenCalledWith({
+      expect(shippingOptionService.deleteShippingMethods).toHaveBeenCalledWith([{
         id: IdMap.getId("ship-method"),
         shipping_option: {
           profile_id: IdMap.getId("prevPro"),
         },
-      })
+      }])
 
       expect(LineItemAdjustmentServiceMock.delete).toHaveBeenCalledTimes(1)
       expect(LineItemAdjustmentServiceMock.delete).toHaveBeenCalledWith({
@@ -1095,6 +1100,9 @@ describe("CartService", () => {
           countries: [{ iso_2: "us" }],
         })
       ),
+      withTransaction: function() {
+        return this
+      },
     }
     const cartRepository = MockRepository({
       findOneWithRelations: () =>
@@ -1131,6 +1139,9 @@ describe("CartService", () => {
     }
 
     const priceSelectionStrat = {
+      withTransaction: function() {
+        return this
+      },
       calculateVariantPrice: async (variantId, context) => {
         if (variantId === IdMap.getId("fail")) {
           throw new MedusaError(
@@ -1152,6 +1163,7 @@ describe("CartService", () => {
       lineItemService,
       productVariantService,
       eventBusService,
+      paymentSessionRepository: MockRepository(),
       priceSelectionStrategy: priceSelectionStrat,
     })
 
@@ -1192,7 +1204,7 @@ describe("CartService", () => {
         shipping_address: {
           country_code: "us",
         },
-        items: [IdMap.getId("testitem"), null],
+        items: [IdMap.getId("testitem")],
         payment_session: null,
         payment_sessions: [],
         gift_cards: [],
@@ -1513,13 +1525,16 @@ describe("CartService", () => {
           },
         })
       }),
-      deleteShippingMethod: jest.fn(),
+      deleteShippingMethods: jest.fn(),
       withTransaction: function() {
         return this
       },
     }
 
     const customShippingOptionService = {
+      withTransaction: function() {
+        return this
+      },
       list: jest.fn().mockImplementation(({ cart_id }) => {
         if (cart_id === IdMap.getId("cart-with-custom-so")) {
           return [
@@ -1575,7 +1590,7 @@ describe("CartService", () => {
       expect(
         shippingOptionService.createShippingMethod
       ).toHaveBeenCalledWith(IdMap.getId("profile1"), data, { cart: cart2 })
-      expect(shippingOptionService.deleteShippingMethod).toHaveBeenCalledWith({
+      expect(shippingOptionService.deleteShippingMethods).toHaveBeenCalledWith({
         id: IdMap.getId("ship1"),
         shipping_option: {
           profile_id: IdMap.getId("profile1"),
@@ -1594,7 +1609,7 @@ describe("CartService", () => {
         data
       )
 
-      expect(shippingOptionService.deleteShippingMethod).toHaveBeenCalledTimes(
+      expect(shippingOptionService.deleteShippingMethods).toHaveBeenCalledTimes(
         0
       )
       expect(shippingOptionService.createShippingMethod).toHaveBeenCalledTimes(
@@ -1616,7 +1631,7 @@ describe("CartService", () => {
         data
       )
 
-      expect(shippingOptionService.deleteShippingMethod).toHaveBeenCalledTimes(
+      expect(shippingOptionService.deleteShippingMethods).toHaveBeenCalledTimes(
         0
       )
       expect(shippingOptionService.createShippingMethod).toHaveBeenCalledTimes(
@@ -1738,6 +1753,9 @@ describe("CartService", () => {
     })
 
     const discountService = {
+      withTransaction: function () {
+        return this
+      },
       retrieveByCode: jest.fn().mockImplementation((code) => {
         if (code === "US10") {
           return Promise.resolve({

packages/medusa/src/services/__tests__/order.js

@@ -1228,7 +1228,7 @@ describe("OrderService", () => {
         .mockImplementation((optionId, data, config) =>
           Promise.resolve({ shipping_option: { profile_id: optionId } })
         ),
-      deleteShippingMethod: jest
+      deleteShippingMethods: jest
         .fn()
         .mockImplementation(() => Promise.resolve({})),
 
@@ -1276,7 +1276,7 @@ describe("OrderService", () => {
         }
       )
 
-      expect(optionService.deleteShippingMethod).not.toHaveBeenCalled()
+      expect(optionService.deleteShippingMethods).not.toHaveBeenCalled()
     })
 
     it("successfully removes shipping method if same option profile", async () => {
@@ -1305,8 +1305,8 @@ describe("OrderService", () => {
         }
       )
 
-      expect(optionService.deleteShippingMethod).toHaveBeenCalledTimes(1)
-      expect(optionService.deleteShippingMethod).toHaveBeenCalledWith({
+      expect(optionService.deleteShippingMethods).toHaveBeenCalledTimes(1)
+      expect(optionService.deleteShippingMethods).toHaveBeenCalledWith({
         shipping_option: {
           profile_id: IdMap.getId("method1"),
         },

packages/medusa/src/services/order.js

@@ -873,7 +873,7 @@ class OrderService extends BaseService {
           ) {
             await this.shippingOptionService_
               .withTransaction(manager)
-              .deleteShippingMethod(sm)
+              .deleteShippingMethods(sm)
           } else {
             methods.push(sm)
           }

packages/medusa/src/services/shipping-option.js

@@ -208,12 +208,16 @@ class ShippingOptionService extends BaseService {
 
   /**
    * Removes a given shipping method
-   * @param {ShippingMethod} sm - the shipping method to remove
+   * @param {ShippingMethod | Array<ShippingMethod>} shippingMethods - the shipping method to remove
    */
-  async deleteShippingMethod(sm) {
+  async deleteShippingMethods(shippingMethods) {
+    if (!Array.isArray(shippingMethods)) {
+      shippingMethods = [shippingMethods]
+    }
+
     return this.atomicPhase_(async (manager) => {
       const methodRepo = manager.getCustomRepository(this.methodRepository_)
-      return methodRepo.remove(sm)
+      return methodRepo.remove(shippingMethods)
     })
   }
 

packages/medusa/src/types/global.ts

@@ -1,10 +1,14 @@
-import { asFunction, asValue, AwilixContainer } from "awilix"
+import { AwilixContainer } from "awilix"
 import { Logger as _Logger } from "winston"
 
 export type ClassConstructor<T> = {
-    new (...args: any[]): T;
-};
+  new (...args: unknown[]): T
+}
 
-export type MedusaContainer = AwilixContainer & { registerAdd: <T>(name: string, registration: T ) => MedusaContainer }
+export type MedusaContainer = AwilixContainer & {
+  registerAdd: <T>(name: string, registration: T) => MedusaContainer
+}
 
-export type Logger = _Logger & { progress: (activityId: string, msg: string) => void }
+export type Logger = _Logger & {
+  progress: (activityId: string, msg: string) => void
+}