Merge pull request hms-dbmi-cellenics#920 from biomage-org/refactor-etag

refactor(seekWorkResponse): api sending ETag + signedURL via POST [v1]

package.json

@@ -86,7 +86,6 @@
     "next-with-less": "^2.0.5",
     "nprogress": "^0.2.0",
     "null-loader": "^3.0.0",
-    "object-hash": "^2.0.3",
     "prop-types": "^15.7.2",
     "rc-util": "^5.28.0",
     "react": "^18.2.0",

src/__test__/components/data-exploration/cell-sets-tool/CellSetsTool.test.jsx

@@ -17,15 +17,14 @@ import { makeStore } from 'redux/store';
 import CellSetsTool from 'components/data-exploration/cell-sets-tool/CellSetsTool';
 import { createCellSet } from 'redux/actions/cellSets';
 
-import { dispatchWorkRequest } from 'utils/work/seekWorkResponse';
+import fetchWork from 'utils/work/fetchWork';
+
 import mockAPI, { generateDefaultMockAPIResponses, promiseResponse } from '__test__/test-utils/mockAPI';
 import { loadBackendStatus } from 'redux/actions/backendStatus';
 
 enableFetchMocks();
 
-jest.mock('utils/work/seekWorkResponse', () => ({
-  dispatchWorkRequest: jest.fn(),
-}));
+jest.mock('utils/work/fetchWork');
 
 jest.mock('utils/socketConnection', () => {
   const mockEmit = jest.fn();
@@ -807,18 +806,7 @@ describe('AnnotateClustersTool', () => {
 
     // It dispatches a work request
     await waitFor(() => {
-      expect(dispatchWorkRequest).toHaveBeenCalledWith(
-        experimentId,
-        {
-          name: 'ScTypeAnnotate',
-          species: 'mouse',
-          tissue: 'Liver',
-        },
-        expect.anything(),
-        expect.anything(),
-        { PipelineRunETag: '2021-10-20T12:51:44.755Z', broadcast: true },
-        expect.anything(),
-      );
+      expect(fetchWork).toMatchSnapshot();
     });
   });
 });

src/__test__/components/data-exploration/cell-sets-tool/__snapshots__/CellSetsTool.test.jsx.snap

@@ -1,5 +1,32 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`AnnotateClustersTool Can dispatch work request 1`] = `
+[MockFunction] {
+  "calls": [
+    [
+      "1234",
+      {
+        "name": "ScTypeAnnotate",
+        "species": "mouse",
+        "tissue": "Liver",
+      },
+      [Function],
+      [Function],
+      {
+        "broadcast": true,
+        "timeout": 180.172,
+      },
+    ],
+  ],
+  "results": [
+    {
+      "type": "return",
+      "value": undefined,
+    },
+  ],
+}
+`;
+
 exports[`CellSetsTool cell set operations should work appropriately for complement 1`] = `
 Set {
   31,

src/__test__/components/data-exploration/generic-gene-table/ExpressionCellSetModal.test.jsx

@@ -9,7 +9,8 @@ import { makeStore } from 'redux/store';
 import ExpressionCellSetModal from 'components/data-exploration/generic-gene-table/ExpressionCellSetModal';
 
 import { GENES_SELECT } from 'redux/actionTypes/genes';
-import { dispatchWorkRequest } from 'utils/work/seekWorkResponse';
+import fetchWork from 'utils/work/fetchWork';
+
 import { loadBackendStatus } from 'redux/actions/backendStatus';
 import { updateExperimentInfo } from 'redux/actions/experimentSettings';
 
@@ -24,9 +25,7 @@ const mockOnCancel = jest.fn();
 
 jest.mock('utils/pushNotificationMessage');
 
-jest.mock('utils/work/seekWorkResponse', () => ({
-  dispatchWorkRequest: jest.fn(),
-}));
+jest.mock('utils/work/fetchWork');
 
 const renderExpressionCellSetModal = async (storeState) => {
   await act(async () => {
@@ -96,12 +95,12 @@ describe('ExpressionCellSetModal', () => {
       userEvent.click(screen.getByText(createButtonText));
     });
 
-    const requestParams = dispatchWorkRequest.mock.calls[0];
+    const requestParams = fetchWork.mock.calls[0];
     expect(requestParams).toMatchSnapshot();
   });
 
   it('Modal closes on success', async () => {
-    dispatchWorkRequest.mockImplementationOnce(() => new Promise((resolve) => {
+    fetchWork.mockImplementationOnce(() => new Promise((resolve) => {
       setTimeout(resolve({ data: 'mockData' }), 2000);
     }));
 
@@ -118,7 +117,7 @@ describe('ExpressionCellSetModal', () => {
   });
 
   it('Modal shows error notification and does not close on error', async () => {
-    dispatchWorkRequest.mockImplementationOnce(() => Promise.reject(new Error('Some error')));
+    fetchWork.mockImplementationOnce(() => Promise.reject(new Error('Some error')));
 
     await renderExpressionCellSetModal(storeState);
 

src/__test__/components/data-exploration/generic-gene-table/__snapshots__/ExpressionCellSetModal.test.jsx.snap

@@ -18,12 +18,10 @@ exports[`ExpressionCellSetModal Sends the correct genes list with params 1`] = `
     ],
     "name": "GetExpressionCellSets",
   },
-  180,
-  "0db78a1886bd55d2346875d2c695b84e",
+  [Function],
+  [Function],
   {
-    "PipelineRunETag": "2021-10-20T12:51:44.755Z",
     "broadcast": true,
   },
-  [Function],
 ]
 `;

src/__test__/components/data-exploration/heatmap/HeatmapPlot.test.jsx

@@ -4,12 +4,11 @@ import preloadAll from 'jest-next-dynamic';
 import { render, screen, waitFor } from '@testing-library/react';
 import { act } from 'react-dom/test-utils';
 
-import { dispatchWorkRequest } from 'utils/work/seekWorkResponse';
+import fetchWork from 'utils/work/fetchWork';
 
 import { Provider } from 'react-redux';
 import fetchMock, { enableFetchMocks } from 'jest-fetch-mock';
 
-import markerGenesData2 from '__test__/data/marker_genes_2.json';
 import markerGenesData5 from '__test__/data/marker_genes_5.json';
 import noCellsGeneExpression from '__test__/data/no_cells_genes_expression.json';
 import cellSetsData from '__test__/data/cell_sets.json';
@@ -19,7 +18,6 @@ import { makeStore } from 'redux/store';
 import mockAPI, {
   generateDefaultMockAPIResponses,
   delayedResponse,
-  dispatchWorkRequestMock,
 } from '__test__/test-utils/mockAPI';
 
 import HeatmapPlot from 'components/data-exploration/heatmap/HeatmapPlot';
@@ -36,25 +34,10 @@ import { updatePlotConfig } from 'redux/actions/componentConfig';
 
 const experimentId = fake.EXPERIMENT_ID;
 
-// Mock hash so we can control the ETag that is produced by hash.MD5 when fetching work requests
-// EtagParams is the object that's passed to the function which generates ETag in fetchWork
-jest.mock('object-hash', () => {
-  const objectHash = jest.requireActual('object-hash');
-  const mockWorkResultETag = jest.requireActual('__test__/test-utils/mockWorkResultETag');
-  const mockWorkRequestETag = (ETagParams) => `${ETagParams.body.nGenes}-marker-genes`;
-  const mockGeneExpressionETag = (ETagParams) => `${ETagParams.missingGenesBody.genes.join('-')}-expression`;
-
-  return mockWorkResultETag(objectHash, mockWorkRequestETag, mockGeneExpressionETag);
-});
-
-jest.mock('utils/work/seekWorkResponse', () => ({
-  __esModule: true,
-  dispatchWorkRequest: jest.fn(() => true),
-}));
+jest.mock('utils/work/fetchWork');
 
 let vitesscePropsSpy = null;
 jest.mock('next/dynamic', () => () => (props) => {
-  console.log('*** we are coming here: ', props);
   vitesscePropsSpy = props;
   return 'Sup Im a heatmap';
 });
@@ -67,12 +50,9 @@ jest.mock('lodash/sampleSize', () => ({
 enableFetchMocks();
 
 const mockWorkerResponses = {
-  '5-marker-genes': markerGenesData5,
-  '2-marker-genes': markerGenesData2,
+  MarkerHeatmap: markerGenesData5,
 };
 
-const newGeneLoadETag = 'Ms4a4b-Smc4-Ccr7-Ifi27l2a-Gm8369-S100a4-S100a6-Tmem176a-Tmem176b-Cxcr6-5830411N06Rik-Lmo4-Il18r1-Atp2b1-Pde5a-Ccl5-Nkg7-Klrd1-AW112010-Klrc1-Gzma-Stmn1-Hmgn2-Pclaf-Tuba1b-Lyz2-Ifitm3-Fcer1g-Tyrobp-Cst3-Cd74-Igkc-Cd79a-H2-Ab1-H2-Eb1-loading_gene_id-expression';
-
 const loadAndRenderDefaultHeatmap = async (storeState) => {
   await act(async () => {
     render(
@@ -89,8 +69,6 @@ const loadAndRenderDefaultHeatmap = async (storeState) => {
 
 const mockAPIResponses = generateDefaultMockAPIResponses(experimentId);
 
-const errorResponse = () => Promise.reject(new Error('Some error idk'));
-
 let storeState = null;
 
 describe('HeatmapPlot', () => {
@@ -112,9 +90,9 @@ describe('HeatmapPlot', () => {
 
     fetchMock.mockIf(/.*/, mockAPI(mockAPIResponses));
 
-    dispatchWorkRequest
+    fetchWork
       .mockReset()
-      .mockImplementationOnce(dispatchWorkRequestMock(mockWorkerResponses));
+      .mockImplementation((_experimentId, body) => mockWorkerResponses[body.name]);
 
     vitesscePropsSpy = null;
 
@@ -125,11 +103,6 @@ describe('HeatmapPlot', () => {
   });
 
   it('Renders the heatmap component by default if everything loads', async () => {
-    fetchMock.mockIf(/.*/, mockAPI({
-      [`/v2/workRequest/${experimentId}/5-marker-genes$`]: () => Promise.resolve(JSON.stringify(true)),
-      ...mockAPIResponses,
-    }));
-
     await loadAndRenderDefaultHeatmap(storeState);
 
     expect(screen.getByText(/Sup Im a heatmap/i)).toBeInTheDocument();
@@ -154,30 +127,29 @@ describe('HeatmapPlot', () => {
 
   it('Shows loader message if the marker genes are loading', async () => {
     const customWorkerResponses = {
-      [`/v2/workRequest/${experimentId}/5-marker-genes`]: () => delayedResponse({ body: 'Not found', status: 404 }, 10000),
+      [`/v2/workRequest/${experimentId}`]: () => delayedResponse({ body: 'Not found', status: 404 }, 10000),
       ...mockWorkerResponses,
     };
 
     fetchMock.mockIf(/.*/, mockAPI(customWorkerResponses));
 
-    dispatchWorkRequest
-      .mockReset()
-      .mockImplementationOnce(dispatchWorkRequestMock(mockWorkerResponses));
-
     await loadAndRenderDefaultHeatmap(storeState);
 
     expect(screen.getByText(/Assigning a worker to your analysis/i)).toBeInTheDocument();
   });
 
   it('Shows loader message if the marker genes are loaded but there\'s other selected genes still loading', async () => {
-    const customWorkerResponses = {
-      ...mockWorkerResponses,
-      [newGeneLoadETag]: () => delayedResponse({ body: 'Not found', status: 404 }, 4000),
-    };
+    let onEtagGeneratedCallback;
 
-    dispatchWorkRequest
+    // we need to manually call the onEtagGenerated callback
+    fetchWork
       .mockReset()
-      .mockImplementation(dispatchWorkRequestMock(customWorkerResponses));
+      .mockImplementationOnce(() => markerGenesData5)
+      .mockImplementationOnce((_experimentId, _body, _getState, _dispatch,
+        { onETagGenerated }) => {
+        onEtagGeneratedCallback = onETagGenerated;
+        return new Promise(() => { });
+      });
 
     await loadAndRenderDefaultHeatmap(storeState);
 
@@ -187,25 +159,20 @@ describe('HeatmapPlot', () => {
     // A new gene is being loaded
     await act(async () => {
       storeState.dispatch(loadDownsampledGeneExpression(experimentId, [...markerGenesData5.orderedGeneNames, 'loading_gene_id'], 'interactiveHeatmap'));
-      jest.runAllTimers();
     });
 
+    onEtagGeneratedCallback();
+
     // Loading screen shows up
     await waitFor(() => {
       expect(screen.getByText(/Assigning a worker to your analysis/i)).toBeInTheDocument();
     });
   });
 
   it('Handles marker genes loading error correctly', async () => {
-    const customWorkerResponses = {
-      ...mockWorkerResponses,
-      '5-marker-genes': errorResponse,
-    };
-
-    dispatchWorkRequest
+    fetchWork
       .mockReset()
-      .mockImplementationOnce(() => Promise.resolve(null))
-      .mockImplementationOnce(dispatchWorkRequestMock(customWorkerResponses));
+      .mockImplementationOnce(() => Promise.reject(new Error('Some error idk')));
 
     await loadAndRenderDefaultHeatmap(storeState);
 
@@ -214,14 +181,10 @@ describe('HeatmapPlot', () => {
   });
 
   it('Handles expression data loading error correctly', async () => {
-    const customWorkerResponses = {
-      ...mockWorkerResponses,
-      [newGeneLoadETag]: errorResponse,
-    };
-
-    dispatchWorkRequest
+    fetchWork
       .mockReset()
-      .mockImplementation(dispatchWorkRequestMock(customWorkerResponses));
+      .mockImplementationOnce(() => markerGenesData5)
+      .mockImplementationOnce(() => Promise.reject(new Error('Some error idk')));
 
     await act(async () => {
       await loadAndRenderDefaultHeatmap(storeState);
@@ -233,7 +196,6 @@ describe('HeatmapPlot', () => {
     // A new gene is being loaded
     await act(async () => {
       await storeState.dispatch(loadDownsampledGeneExpression(experimentId, [...markerGenesData5.orderedGeneNames, 'loading_gene_id'], 'interactiveHeatmap'));
-      jest.runAllTimers();
     });
 
     // Error screen shows up
@@ -257,8 +219,8 @@ describe('HeatmapPlot', () => {
       .cellIds.map((cellId) => cellId.toString());
 
     // It loaded once the marker genes
-    expect(dispatchWorkRequest).toHaveBeenCalledTimes(1);
-    expect(dispatchWorkRequest.mock.calls[0][1].name === 'MarkerHeatmap').toBe(true);
+    expect(fetchWork).toHaveBeenCalledTimes(1);
+    expect(fetchWork.mock.calls[0][1].name === 'MarkerHeatmap').toBe(true);
 
     // It shows cells in louvain-3
     expect(isSubset(cellsInLouvain3, vitesscePropsSpy.obsIndex)).toEqual(true);
@@ -273,17 +235,13 @@ describe('HeatmapPlot', () => {
     });
 
     // It performs the request with the new hidden cell sets array
-    expect(dispatchWorkRequest).toHaveBeenCalledTimes(2);
-    expect(dispatchWorkRequest.mock.calls[1]).toMatchSnapshot();
+    expect(fetchWork).toHaveBeenCalledTimes(2);
   });
 
   it('Shows an empty message when all cell sets are hidden ', async () => {
-    dispatchWorkRequest
-      .mockReset()
-      // Mock each of the loadMarkerGenes calls caused by hiding a cell set
-      .mockImplementation(dispatchWorkRequestMock(mockWorkerResponses));
-
-    await loadAndRenderDefaultHeatmap(storeState);
+    await act(async () => {
+      await loadAndRenderDefaultHeatmap(storeState);
+    });
 
     // Renders correctly
     expect(screen.getByText(/Sup Im a heatmap/i)).toBeInTheDocument();
@@ -293,16 +251,15 @@ describe('HeatmapPlot', () => {
       .cellSets.find(({ key: parentKey }) => parentKey === 'louvain')
       .children.map(({ key: cellSetKey }) => cellSetKey);
 
-    dispatchWorkRequest
+    fetchWork
       .mockReset()
       // Last call (all the cellSets are hidden) return empty
-      .mockImplementationOnce(() => Promise.resolve({ data: noCellsGeneExpression }));
-
-    const hideAllCellsPromise = louvainClusterKeys.map(async (cellSetKey) => {
-      storeState.dispatch(setCellSetHiddenStatus(cellSetKey));
-    });
+      .mockImplementationOnce(() => noCellsGeneExpression);
 
     await act(async () => {
+      const hideAllCellsPromise = louvainClusterKeys.map(async (cellSetKey) => {
+        storeState.dispatch(setCellSetHiddenStatus(cellSetKey));
+      });
       await Promise.all(hideAllCellsPromise);
     });