fix(ecommerce-app-base): drag-n-drop bug in refetch logic that was ca…

…using infinite loop (#7944)

packages/ecommerce-app-base/src/AppConfig/AppConfig.spec.tsx

@@ -5,53 +5,7 @@ import { ConfigAppSDK } from '@contentful/app-sdk';
 
 import AppConfig from './AppConfig';
 import { definitions } from './parameters.spec';
-
-const contentTypes = [
-  {
-    sys: { id: 'ct1' },
-    name: 'CT1',
-    fields: [
-      { id: 'product_x', name: 'Product X', type: 'Symbol' },
-      { id: 'y', name: 'Y', type: 'Object' },
-    ],
-  },
-  {
-    sys: { id: 'ct2' },
-    name: 'CT2',
-    fields: [
-      { id: 'foo', name: 'FOO', type: 'Text' },
-      { id: 'z', name: 'Z', type: 'Array', items: { type: 'Symbol' } },
-    ],
-  },
-  {
-    sys: { id: 'ct3' },
-    name: 'CT3',
-    fields: [
-      { id: 'bar', name: 'BAR', type: 'Object' },
-      { id: 'baz', name: 'BAZ', type: 'Object' },
-      { id: 'product_d', name: 'Product D', type: 'Array', items: { type: 'Symbol' } },
-      { id: 'product_a', name: 'Product A', type: 'Symbol' },
-    ],
-  },
-];
-
-const makeSdkMock = () => ({
-  ids: {
-    app: 'some-app',
-  },
-  hostnames: {
-    webapp: 'app.contentful.com',
-  },
-  space: {
-    getContentTypes: jest.fn().mockResolvedValue({ items: contentTypes }),
-    getEditorInterfaces: jest.fn().mockResolvedValue({ items: [] }),
-  },
-  app: {
-    setReady: jest.fn(),
-    getParameters: jest.fn().mockResolvedValue(null),
-    onConfigure: jest.fn().mockReturnValue(undefined),
-  },
-});
+import { makeSdkMock } from '../__mocks__';
 
 const validate = () => null; // Means no error
 

packages/ecommerce-app-base/src/Editor/SortableComponent.spec.tsx

@@ -0,0 +1,116 @@
+import React from 'react';
+import { render, cleanup, waitFor } from '@testing-library/react';
+import { SortableComponent } from './SortableComponent';
+import { makeSdkMock, productsList } from '../__mocks__';
+import { FieldAppSDK } from '@contentful/app-sdk';
+import { ProductPreviewsFn } from '../types';
+
+const mockSdk = makeSdkMock();
+const skus = ['M0E20130820E90Z', 'A0E2300FX102203', 'M0E21300900DZN7'];
+const mockConfig = {};
+const mockSkuType = 'mock-sku-type';
+
+describe('SortableComponent', () => {
+  let mockFetchProductPreviews: ProductPreviewsFn;
+
+  beforeEach(() => {
+    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    // @ts-ignore - typescript is upset because jest.fn() returns a type different than ProductPreviewsFn
+    mockFetchProductPreviews = jest.fn().mockImplementation(() => {
+      return Promise.resolve(productsList);
+    });
+  });
+
+  afterEach(() => {
+    cleanup();
+    jest.resetAllMocks();
+  });
+
+  it('calls `fetchProductPreviews()` to retrieve list of products for associated skus', async () => {
+    waitFor(() => {
+      render(
+        <SortableComponent
+          sdk={mockSdk as unknown as FieldAppSDK}
+          disabled={false}
+          config={mockConfig}
+          skus={skus}
+          onChange={jest.fn()}
+          fetchProductPreviews={mockFetchProductPreviews}
+          skuType={mockSkuType}
+        />
+      );
+    });
+
+    expect(mockFetchProductPreviews).toHaveBeenCalledTimes(1);
+    expect(mockFetchProductPreviews).toHaveBeenLastCalledWith(skus, mockConfig, mockSkuType);
+  });
+
+  it('refetches list of products for associated skus, when skus are added/removed', async () => {
+    const newSkus = [...skus, 'new-sku-item'];
+
+    await waitFor(() => {
+      // initial mount with original skus
+      const { rerender } = render(
+        <SortableComponent
+          sdk={mockSdk as unknown as FieldAppSDK}
+          disabled={false}
+          config={mockConfig}
+          skus={skus}
+          onChange={jest.fn()}
+          fetchProductPreviews={mockFetchProductPreviews}
+          skuType={mockSkuType}
+        />
+      );
+
+      // rerender with additional sku
+      rerender(
+        <SortableComponent
+          skus={newSkus}
+          sdk={mockSdk as unknown as FieldAppSDK}
+          disabled={false}
+          config={mockConfig}
+          onChange={jest.fn()}
+          fetchProductPreviews={mockFetchProductPreviews}
+          skuType={mockSkuType}
+        />
+      );
+    });
+
+    expect(mockFetchProductPreviews).toHaveBeenCalledTimes(2);
+    expect(mockFetchProductPreviews).toHaveBeenLastCalledWith(newSkus, mockConfig, mockSkuType);
+  });
+
+  it('does NOT refetch list of products when skus have simply been reordered', async () => {
+    const reorderedSkus = [skus[1], skus[0], skus[2]];
+
+    await waitFor(() => {
+      // initial mount with original skus
+      const { rerender } = render(
+        <SortableComponent
+          sdk={mockSdk as unknown as FieldAppSDK}
+          disabled={false}
+          config={mockConfig}
+          skus={skus}
+          onChange={jest.fn()}
+          fetchProductPreviews={mockFetchProductPreviews}
+          skuType={mockSkuType}
+        />
+      );
+
+      // rerender with new skus, just reordered
+      rerender(
+        <SortableComponent
+          skus={reorderedSkus}
+          sdk={mockSdk as unknown as FieldAppSDK}
+          disabled={false}
+          config={mockConfig}
+          onChange={jest.fn()}
+          fetchProductPreviews={mockFetchProductPreviews}
+          skuType={mockSkuType}
+        />
+      );
+
+      expect(mockFetchProductPreviews).toHaveBeenCalledTimes(1);
+    });
+  });
+});

packages/ecommerce-app-base/src/Editor/SortableComponent.tsx

@@ -30,6 +30,15 @@ interface Props {
   skuType?: string;
 }
 
+/**
+ * @description - helper function to compare two lists of skus for order agnostic equality
+ */
+const skusAreEqual = (prevSkus: string[], currentSkus: string[]): boolean => {
+  const sortedPreviousSkus = [...prevSkus].sort();
+  const sortedCurrentSkus = [...currentSkus].sort();
+  return !isEqual(sortedPreviousSkus, sortedCurrentSkus);
+};
+
 /**
  * @description - hook to track previous version of skus prop
  * @param skus - list of most current skus
@@ -57,28 +66,31 @@ export const SortableComponent: FC<Props> = ({
   const [productPreviews, setProductPreviews] = useState<Product[]>([]);
   const previousSkus = usePreviousSkus(skus);
 
-  const getProductPreviews = useCallback(
-    async (shouldRefetch: boolean = true) => {
-      try {
-        const productPreviewsUnsorted = shouldRefetch
-          ? await fetchProductPreviews(skus, config, skuType)
-          : productPreviews;
+  const getProductPreviews = useCallback(async () => {
+    try {
+      const shouldRefetch = skusAreEqual([...previousSkus], [...skus]);
+
+      if (shouldRefetch) {
+        const productPreviewsUnsorted = await fetchProductPreviews(skus, config, skuType);
         const sortedProductPreviews = mapSort(productPreviewsUnsorted, skus, 'sku');
         setProductPreviews(sortedProductPreviews);
-      } catch (error) {
-        sdk.notifier.error('There was an error fetching the data for the selected products.');
       }
-    },
-    [skus, skuType, config, fetchProductPreviews, setProductPreviews, sdk.notifier, productPreviews]
-  );
+    } catch (error) {
+      sdk.notifier.error('There was an error fetching the data for the selected products.');
+    }
+  }, [skus, skuType, config, fetchProductPreviews, setProductPreviews, sdk.notifier, previousSkus]);
 
+  /**
+   * @description - Compare previous list of skus (see `usePreviousSkus`) to the list of skus
+   * passed as a prop.  If the previous & current skus differ (a sku was added/removed, order agnostic),
+   * then fetch/refetch associated productPreviews given list of skus.
+   */
   useEffect(() => {
-    // only refetch product previews if new skus do not match previous skus
-    const sortedPreviousSkus = previousSkus.sort();
-    const sortedCurrentSkus = skus.sort();
-    const shouldRefetch = !isEqual(sortedPreviousSkus, sortedCurrentSkus);
+    const shouldRefetch = skusAreEqual([...previousSkus], [...skus]);
 
-    getProductPreviews(shouldRefetch);
+    if (shouldRefetch) {
+      getProductPreviews();
+    }
   }, [skus, skuType, config, getProductPreviews, previousSkus]);
 
   const deleteItem = useCallback(

packages/ecommerce-app-base/src/__mocks__/index.ts

@@ -1 +1,3 @@
 export { products, productsList } from './products';
+export { makeSdkMock } from './mockSdk';
+export { mockContentTypes } from './mockContentTypes';

packages/ecommerce-app-base/src/__mocks__/mockContentTypes.ts

@@ -0,0 +1,28 @@
+export const mockContentTypes = [
+  {
+    sys: { id: 'ct1' },
+    name: 'CT1',
+    fields: [
+      { id: 'product_x', name: 'Product X', type: 'Symbol' },
+      { id: 'y', name: 'Y', type: 'Object' },
+    ],
+  },
+  {
+    sys: { id: 'ct2' },
+    name: 'CT2',
+    fields: [
+      { id: 'foo', name: 'FOO', type: 'Text' },
+      { id: 'z', name: 'Z', type: 'Array', items: { type: 'Symbol' } },
+    ],
+  },
+  {
+    sys: { id: 'ct3' },
+    name: 'CT3',
+    fields: [
+      { id: 'bar', name: 'BAR', type: 'Object' },
+      { id: 'baz', name: 'BAZ', type: 'Object' },
+      { id: 'product_d', name: 'Product D', type: 'Array', items: { type: 'Symbol' } },
+      { id: 'product_a', name: 'Product A', type: 'Symbol' },
+    ],
+  },
+];

packages/ecommerce-app-base/src/__mocks__/mockSdk.ts

@@ -0,0 +1,22 @@
+import { mockContentTypes } from './mockContentTypes';
+
+export const makeSdkMock = () => ({
+  ids: {
+    app: 'some-app',
+  },
+  hostnames: {
+    webapp: 'app.contentful.com',
+  },
+  space: {
+    getContentTypes: jest.fn().mockResolvedValue({ items: mockContentTypes }),
+    getEditorInterfaces: jest.fn().mockResolvedValue({ items: [] }),
+  },
+  app: {
+    setReady: jest.fn(),
+    getParameters: jest.fn().mockResolvedValue(null),
+    onConfigure: jest.fn().mockReturnValue(undefined),
+  },
+  notifier: {
+    error: (msg: string) => console.log(`[mockSdk] error: ${msg}`),
+  },
+});