Merge pull request #362 from hms-dbmi-cellenics/1928-fix-api-plots-ad…

…d-404-when-plot-is-not-found

Fix plots in v2

src/api.v2/helpers/pipeline/hooks/assignPodToPipeline.js

@@ -70,9 +70,6 @@ const patchPod = async (message) => {
 };
 
 const assignPodToPipeline = async (message) => {
-  console.log('messageDebug');
-  console.log(message);
-
   // this checks should be refactored and cleaned once the gem2s / qc spec refactors are done
   // and we can be sure that taskName is always present at the top-level of all the message
   // instead of inside input

src/api.v2/model/Plot.js

@@ -1,3 +1,5 @@
+const _ = require('lodash');
+
 const BasicModel = require('./BasicModel');
 const sqlClient = require('../../sql/sqlClient');
 
@@ -6,6 +8,7 @@ const validateRequest = require('../../utils/schema-validator');
 const tableNames = require('./tableNames');
 const bucketNames = require('../helpers/s3/bucketNames');
 const getObject = require('../helpers/s3/getObject');
+const { NotFoundError } = require('../../utils/responses');
 
 const selectableProps = [
   'id',
@@ -20,13 +23,15 @@ class Plot extends BasicModel {
   }
 
   async getConfig(experimentId, plotUuid) {
-    const {
-      s3DataKey,
-      config: plotConfig,
-    } = await this.findOne({ id: plotUuid, experiment_id: experimentId });
+    const result = await this.findOne({ id: plotUuid, experiment_id: experimentId });
+
+    if (_.isNil(result)) {
+      throw new NotFoundError(`Plot ${plotUuid} in experiment ${experimentId} not found`);
+    }
 
-    const result = { config: plotConfig };
+    const { s3DataKey = null, config: plotConfig } = result;
 
+    const response = { config: plotConfig };
 
     if (s3DataKey) {
       const plotDataObject = await getObject({
@@ -40,14 +45,14 @@ class Plot extends BasicModel {
         await validateRequest(output, 'plots-bodies/PlotData.v2.yaml');
       }
 
-      result.plotData = output.plotData || [];
+      response.plotData = output.plotData || [];
     }
 
-    return result;
+    return response;
   }
 
   async updateConfig(experimentId, plotUuid, plotConfig) {
-    return await this.update({ id: plotUuid, experiment_id: experimentId }, { config: plotConfig });
+    return await this.upsert({ id: plotUuid, experiment_id: experimentId }, { config: plotConfig });
   }
 }
 

tests/api.v2/helpers/pipeline/handleQCResponse.test.js

@@ -112,9 +112,6 @@ describe('handleQCResponse module', () => {
 
       expect(hookRunnerInstance.run).toHaveBeenCalled();
 
-      console.log('mockSocketemitDebug');
-      console.log(mockSocket.emit);
-
       expect(mockSocket.emit).toHaveBeenCalledWith(`ExperimentUpdates-${fake.EXPERIMENT_ID}`,
         expect.objectContaining({
           type: constants.QC_PROCESS_NAME,

tests/api.v2/model/Plot.test.js

@@ -12,6 +12,7 @@ jest.mock('../../../src/sql/sqlClient', () => ({
 
 const Plot = require('../../../src/api.v2/model/Plot');
 const BasicModel = require('../../../src/api.v2/model/BasicModel');
+const { NotFoundError } = require('../../../src/utils/responses');
 
 const mockExperimentId = 'mockExperimentId';
 const mockPlotUuid = 'mockPlotUuid';
@@ -22,7 +23,7 @@ describe('model/Plot', () => {
     jest.clearAllMocks();
   });
 
-  it('Get plot config works correctly', async () => {
+  it('getConfig works correctly', async () => {
     const mockConfig = { some: 'config' };
     const mockS3DataKey = 'mock/plot/data/key';
     const mockPlotData = [1, 2, 3];
@@ -48,7 +49,7 @@ describe('model/Plot', () => {
     expect(result).toEqual({ config: mockConfig, plotData: mockPlotData });
   });
 
-  it('Does not download plot data from S3 if plot does not have plot data', async () => {
+  it('getConfig does not download plot data from S3 if plot does not have plot data', async () => {
     const mockConfig = { some: 'config' };
 
     const mockFindOne = jest.spyOn(BasicModel.prototype, 'findOne')
@@ -68,17 +69,32 @@ describe('model/Plot', () => {
     expect(result).toEqual({ config: mockConfig });
   });
 
-  it('Update plot config works correctly', async () => {
+  it('getConfig throws not found error if the plot was not found', async () => {
+    jest.spyOn(BasicModel.prototype, 'findOne').mockImplementationOnce(() => Promise.resolve(null));
+
+    await expect(
+      new Plot().getConfig(mockExperimentId, mockPlotUuid),
+    ).rejects.toThrow(
+      new NotFoundError(`Plot ${mockPlotUuid} in experiment ${mockExperimentId} not found`),
+    );
+
+    expect(getObject).not.toHaveBeenCalled();
+  });
+
+  it('updateConfig works correctly', async () => {
     const mockConfig = {
       legend: { enabled: true },
       plotTitle: 'mockPlotTitle',
     };
 
-    const mockUpdate = jest.spyOn(BasicModel.prototype, 'update');
+    const mockUpsert = jest.spyOn(BasicModel.prototype, 'upsert');
 
     await new Plot().updateConfig(mockExperimentId, mockPlotUuid, mockConfig);
 
-    expect(mockUpdate).toHaveBeenCalledTimes(1);
-    expect(mockUpdate).toHaveBeenCalledWith({ id: mockPlotUuid, experiment_id: mockExperimentId }, { config: mockConfig });
+    expect(mockUpsert).toHaveBeenCalledTimes(1);
+    expect(mockUpsert).toHaveBeenCalledWith(
+      { id: mockPlotUuid, experiment_id: mockExperimentId },
+      { config: mockConfig },
+    );
   });
 });