From 2a2b7241f3da63073866d6dd1d9165e039d036e1 Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Fri, 3 Nov 2023 12:58:48 +0000 Subject: [PATCH 01/20] fix: add validation check for passed state --- app/javascript/shared/components/LabwareScan.vue | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/app/javascript/shared/components/LabwareScan.vue b/app/javascript/shared/components/LabwareScan.vue index 03960c626..93ea31e1a 100644 --- a/app/javascript/shared/components/LabwareScan.vue +++ b/app/javascript/shared/components/LabwareScan.vue @@ -42,6 +42,7 @@ <script> import { checkSize } from './plateScanValidators' import { aggregate } from './scanValidators' +import { checkState } from './tubeScanValidators' // Incrementing counter to ensure all instances of LabwareScan // have a unique id. Ensures labels correctly match up with @@ -103,13 +104,6 @@ export default { type: Object, default: null, }, - validators: { - // An array of validators. See plateScanValidators.js and tubeScanValidators.js for examples and details - // defaults are set based on labwareType, in method 'computedValidators' - type: Array, - required: false, - default: null, - }, scanDisabled: { // Used to disable the scan field. type: Boolean, @@ -184,12 +178,8 @@ export default { } }, computedValidators() { - if (this.validators) { - return this.validators - } - if (this.labwareType == 'tube') { - return [] + return [checkState(['passed'])] } else { return [checkSize(12, 8)] } @@ -233,7 +223,7 @@ export default { if (this.labwareType == 'tube') { return { - tubes: 'labware_barcode,uuid,receptacle', + tubes: 'labware_barcode,uuid,receptacle,state', receptacles: 'uuid', } } else { From 160291e25cb7fac8b2cf6f8998070c0b4daf40ed Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Tue, 7 Nov 2023 10:46:38 +0000 Subject: [PATCH 02/20] build: hide coverage report after test run --- jest.config.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/jest.config.js b/jest.config.js index a6b8648fd..e98095cab 100644 --- a/jest.config.js +++ b/jest.config.js @@ -34,12 +34,12 @@ module.exports = { coverageProvider: 'babel', // A list of reporter names that Jest uses when writing coverage reports - // coverageReporters: [ - // "json", - // "text", - // "lcov", - // "clover" - // ], + coverageReporters: [ + 'json', + // "text", // output to console + 'lcov', + 'clover', + ], // An object that configures minimum threshold enforcement for coverage results // coverageThreshold: undefined, From 4c575ec6011be33e4a16f01d2e7793b5c8b44b3d Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Tue, 7 Nov 2023 10:49:38 +0000 Subject: [PATCH 03/20] tests: refactor mock api --- app/javascript/test_support/mock_api.js | 39 +++++++++++++++---------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/app/javascript/test_support/mock_api.js b/app/javascript/test_support/mock_api.js index fb8425708..9ce7a5ded 100644 --- a/app/javascript/test_support/mock_api.js +++ b/app/javascript/test_support/mock_api.js @@ -9,30 +9,35 @@ const dummyApiKey = 'mock_api_key' // Nice user readable summary of the request object const requestFormatter = function (request) { + // order keys alphabetically + const sorted_keys = Object.keys(request.params).sort() + const parameters = sorted_keys.map((key) => `"${key}": ${JSON.stringify(request.params[key])}`).join(', ') return ` --------------------------------------------------------------------------------- + -------------------------------------------------------------------------------- Method: ${request.method} Url: ${request.url} - Params: ${JSON.stringify(request.params)} --------------------------------------------------------------------------------- - ` + Params: {${parameters}} + -------------------------------------------------------------------------------- + `.trim() } // Fail the test if we receive an unexpected request and provide information // to assist with debugging -const unexpectedRequest = function (request, expectedRequests) { - const baseError = `Unexpected Request: -${requestFormatter(request)} -Expected Requests:` - const errorMessage = expectedRequests.reduce((error, expectedRequest) => { - return error + requestFormatter(expectedRequest.req) - }, baseError) - fail(errorMessage) +const raiseUnexpectedRequest = (request, expectedRequests) => { + const formattedExpectedRequests = expectedRequests.map((req) => requestFormatter(req.req)).join('\n ') + const formattedRequest = requestFormatter(request) + throw new Error(` + Unexpected request: + ${formattedRequest} + Expected one of: + ${formattedExpectedRequests}`) } const mockApi = function (resources = sequencescapeResources) { const devour = devourApi({ apiUrl: dummyApiUrl }, resources, dummyApiKey) const mockedRequests = [] + // Find a request in the mockedRequests array that matches the request + // object. If no match is found, return undefined. const findRequest = (request) => { return mockedRequests.find((requestResponse) => { // devour is a little inconsistent in when it records a data payload @@ -47,20 +52,22 @@ const mockApi = function (resources = sequencescapeResources) { name: 'mock-request-response', mockedRequests: [], req: (payload) => { - const mockedRequest = findRequest(payload.req) + const incomingRequest = payload.req + const mockedRequest = findRequest(incomingRequest) if (mockedRequest) { mockedRequest.called += 1 - payload.req.adapter = function () { + incomingRequest.adapter = function () { return mockedRequest.res } } else { // Stop things going further, otherwise we risk generating real traffic - payload.req.adapter = function () { + incomingRequest.adapter = function () { return Promise.reject({ message: 'unregistered request' }) } - unexpectedRequest(payload.req, mockedRequests) + raiseUnexpectedRequest(incomingRequest, mockedRequests) } + return payload }, mockGet: (url, params, response) => { From 81285a83107f674ae7cca40c836ba140667f6d3e Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Tue, 7 Nov 2023 16:40:42 +0000 Subject: [PATCH 04/20] tests: refactor LabwareScan.spec.js and add tests for pending tubes --- .../shared/components/LabwareScan.spec.js | 223 +++++++++++------- 1 file changed, 132 insertions(+), 91 deletions(-) diff --git a/app/javascript/shared/components/LabwareScan.spec.js b/app/javascript/shared/components/LabwareScan.spec.js index b3fa3df9f..d9073c08c 100644 --- a/app/javascript/shared/components/LabwareScan.spec.js +++ b/app/javascript/shared/components/LabwareScan.spec.js @@ -9,10 +9,6 @@ import mockApi from 'test_support/mock_api' import localVue from 'test_support/base_vue' describe('LabwareScan', () => { - const assetUuid = 'afabla7e-9498-42d6-964e-50f61ded6d9a' - const nullTube = { data: [] } - const goodTube = jsonCollectionFactory('tube', [{ uuid: assetUuid }]) - const wrapperFactoryPlate = function (api = mockApi()) { return mount(LabwareScan, { propsData: { @@ -117,63 +113,6 @@ describe('LabwareScan', () => { expect(wrapper.find('.well').exists()).toBe(false) }) - it('is invalid if it can not find a plate', async () => { - const api = mockApi() - api.mockGet( - 'plates', - { - filter: { barcode: 'not a barcode' }, - include: '', - fields: { - plates: 'labware_barcode,uuid,number_of_rows,number_of_columns', - }, - }, - nullTube - ) - const wrapper = wrapperFactoryPlate(api) - - wrapper.find('input').setValue('not a barcode') - await wrapper.find('input').trigger('change') - - expect(wrapper.find('.wait-labware').exists()).toBe(true) - - await flushPromises() - - expect(wrapper.find('.invalid-feedback').text()).toEqual('Could not find plate') - expect(wrapper.emitted()).toEqual({ - change: [[{ state: 'searching', plate: null }], [{ state: 'invalid', plate: undefined }]], - }) - }) - - it('is invalid if it can not find a tube', async () => { - const api = mockApi() - api.mockGet( - 'tubes', - { - filter: { barcode: 'not a barcode' }, - include: '', - fields: { - tubes: 'labware_barcode,uuid,receptacle', - receptacles: 'uuid', - }, - }, - nullTube - ) - const wrapper = wrapperFactoryTube(api) - - wrapper.find('input').setValue('not a barcode') - await wrapper.find('input').trigger('change') - - expect(wrapper.find('.wait-labware').exists()).toBe(true) - - await flushPromises() - - expect(wrapper.find('.invalid-feedback').text()).toEqual('Could not find tube') - expect(wrapper.emitted()).toEqual({ - change: [[{ state: 'searching', labware: null }], [{ state: 'invalid', labware: undefined }]], - }) - }) - it('is invalid if there are api troubles', async () => { const api = mockApi() api.mockFail( @@ -182,7 +121,7 @@ describe('LabwareScan', () => { filter: { barcode: 'Good barcode' }, include: '', fields: { - tubes: 'labware_barcode,uuid,receptacle', + tubes: 'labware_barcode,uuid,receptacle,state', receptacles: 'uuid', }, }, @@ -215,43 +154,118 @@ describe('LabwareScan', () => { }) }) - it('is valid if it can find a tube', async () => { - const api = mockApi() - const wrapper = wrapperFactoryTube(api) + describe('When labwareType is tube', () => { + const goodTubeUuid = 'afabla7e-9498-42d6-964e-50f61ded6d9a' + const pendingTubeUuid = '123e4567-e89b-12d3-a456-426614174000' + const nullTube = { data: [] } + const goodTube = jsonCollectionFactory('tube', [{ uuid: goodTubeUuid, state: 'passed' }]) + const pendingTube = jsonCollectionFactory('tube', [{ uuid: pendingTubeUuid, state: 'pending' }]) - api.mockGet( - 'tubes', - { - include: '', - filter: { barcode: 'DN12345' }, - fields: { - tubes: 'labware_barcode,uuid,receptacle', - receptacles: 'uuid', + it('is valid if it can find a tube', async () => { + const api = mockApi() + const wrapper = wrapperFactoryTube(api) + + api.mockGet( + 'tubes', + { + include: '', + filter: { barcode: 'DN12345' }, + fields: { + tubes: 'labware_barcode,uuid,receptacle,state', + receptacles: 'uuid', + }, }, - }, - goodTube - ) + goodTube + ) - wrapper.find('input').setValue('DN12345') - await wrapper.find('input').trigger('change') + wrapper.find('input').setValue('DN12345') + await wrapper.find('input').trigger('change') - expect(wrapper.find('.wait-labware').exists()).toBe(true) + expect(wrapper.find('.wait-labware').exists()).toBe(true) - await flushPromises() + await flushPromises() + + expect(wrapper.find('.valid-feedback').text()).toEqual('Great!') - expect(wrapper.find('.valid-feedback').text()).toEqual('Great!') + const events = wrapper.emitted() + + expect(events.change.length).toEqual(2) + expect(events.change[0]).toEqual([{ state: 'searching', labware: null }]) + expect(events.change[1][0].state).toEqual('valid') + expect(events.change[1][0].labware.uuid).toEqual(goodTubeUuid) + }) + + it('is invalid if it can not find a tube', async () => { + const api = mockApi() + api.mockGet( + 'tubes', + { + filter: { barcode: 'not a barcode' }, + include: '', + fields: { + tubes: 'labware_barcode,uuid,receptacle,state', + receptacles: 'uuid', + }, + }, + nullTube + ) + const wrapper = wrapperFactoryTube(api) + + wrapper.find('input').setValue('not a barcode') + await wrapper.find('input').trigger('change') + + expect(wrapper.find('.wait-labware').exists()).toBe(true) + + await flushPromises() + + expect(wrapper.find('.invalid-feedback').text()).toEqual('Could not find tube') + expect(wrapper.emitted()).toEqual({ + change: [[{ state: 'searching', labware: null }], [{ state: 'invalid', labware: undefined }]], + }) + }) + + it('is invalid if the tube is in the pending state', async () => { + const api = mockApi() + const wrapper = wrapperFactoryTube(api) + + api.mockGet( + 'tubes', + { + include: '', + filter: { barcode: 'Good barcode' }, + fields: { + tubes: 'labware_barcode,uuid,receptacle,state', + receptacles: 'uuid', + }, + }, + pendingTube + ) + + wrapper.find('input').setValue('Good barcode') + await wrapper.find('input').trigger('change') - const events = wrapper.emitted() + expect(wrapper.find('.wait-labware').exists()).toBe(true) + + await flushPromises() - expect(events.change.length).toEqual(2) - expect(events.change[0]).toEqual([{ state: 'searching', labware: null }]) - expect(events.change[1][0].state).toEqual('valid') - expect(events.change[1][0].labware.uuid).toEqual(assetUuid) + expect(wrapper.find('.is-invalid').exists()).toBe(true) + expect(wrapper.find('.invalid-feedback').text()).toEqual('Tube must have a state of: passed') + + const events = wrapper.emitted() + + expect(events.change.length).toEqual(2) + expect(events.change[0]).toEqual([{ state: 'searching', labware: null }]) + expect(events.change[1][0].state).toEqual('invalid') + expect(events.change[1][0].labware.uuid).toEqual(pendingTubeUuid) + }) }) describe('When labwareType is plate', () => { - const goodPlate = jsonCollectionFactory('plate', [{ uuid: assetUuid }]) - const badPlate = jsonCollectionFactory('plate', [{ uuid: assetUuid, number_of_columns: 24, number_of_rows: 8 }]) + const goodPlateUuid = '550e8400-e29b-41d4-a716-446655440000' + const badPlateUuid = '550e8400-e29b-41d4-a716-446655440001' + const nullPlate = { data: [] } + const goodPlate = jsonCollectionFactory('plate', [{ uuid: goodPlateUuid }]) + const badPlate = jsonCollectionFactory('plate', [{ uuid: badPlateUuid, number_of_columns: 24, number_of_rows: 8 }]) const wrapperFactoryPlate = function (api = mockApi()) { return mount(LabwareScan, { @@ -264,7 +278,6 @@ describe('LabwareScan', () => { localVue, }) } - it('is valid if it can find a plate', async () => { const api = mockApi() const wrapper = wrapperFactoryPlate(api) @@ -295,7 +308,35 @@ describe('LabwareScan', () => { expect(events.change.length).toEqual(2) expect(events.change[0]).toEqual([{ state: 'searching', plate: null }]) expect(events.change[1][0].state).toEqual('valid') - expect(events.change[1][0].plate.uuid).toEqual(assetUuid) + expect(events.change[1][0].plate.uuid).toEqual(goodPlateUuid) + }) + + it('is invalid if it can not find a plate', async () => { + const api = mockApi() + api.mockGet( + 'plates', + { + filter: { barcode: 'not a barcode' }, + include: 'wells.requests_as_source,wells.aliquots.request', + fields: { + plates: 'labware_barcode,uuid,number_of_rows,number_of_columns', + }, + }, + nullPlate + ) + const wrapper = wrapperFactoryPlate(api) + + wrapper.find('input').setValue('not a barcode') + await wrapper.find('input').trigger('change') + + expect(wrapper.find('.wait-labware').exists()).toBe(true) + + await flushPromises() + + expect(wrapper.find('.invalid-feedback').text()).toEqual('Could not find plate') + expect(wrapper.emitted()).toEqual({ + change: [[{ state: 'searching', plate: null }], [{ state: 'invalid', plate: undefined }]], + }) }) it('is invalid if the plate is the wrong size', async () => { @@ -328,7 +369,7 @@ describe('LabwareScan', () => { expect(events.change.length).toEqual(2) expect(events.change[0]).toEqual([{ state: 'searching', plate: null }]) expect(events.change[1][0].state).toEqual('invalid') - expect(events.change[1][0].plate.uuid).toEqual(assetUuid) + expect(events.change[1][0].plate.uuid).toEqual(badPlateUuid) }) }) }) From 6dde3db262ef78f7a0e1ab8dae29a9b16d7bb846 Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Tue, 7 Nov 2023 16:54:29 +0000 Subject: [PATCH 05/20] tests: reorder api troubles test and add plates version --- .../shared/components/LabwareScan.spec.js | 122 ++++++++++++------ 1 file changed, 81 insertions(+), 41 deletions(-) diff --git a/app/javascript/shared/components/LabwareScan.spec.js b/app/javascript/shared/components/LabwareScan.spec.js index d9073c08c..74f28de3e 100644 --- a/app/javascript/shared/components/LabwareScan.spec.js +++ b/app/javascript/shared/components/LabwareScan.spec.js @@ -113,47 +113,6 @@ describe('LabwareScan', () => { expect(wrapper.find('.well').exists()).toBe(false) }) - it('is invalid if there are api troubles', async () => { - const api = mockApi() - api.mockFail( - 'tubes', - { - filter: { barcode: 'Good barcode' }, - include: '', - fields: { - tubes: 'labware_barcode,uuid,receptacle,state', - receptacles: 'uuid', - }, - }, - { - errors: [ - { - title: 'Not good', - detail: 'Very not good', - code: 500, - status: 500, - }, - ], - } - ) - const wrapper = wrapperFactoryTube(api) - - wrapper.find('input').setValue('Good barcode') - await wrapper.find('input').trigger('change') - - expect(wrapper.find('.wait-labware').exists()).toBe(true) - - await flushPromises() - - // JG: Can't seem to get the mock api to correctly handle errors. THis would be the - // desired behaviour, and seems to actually work in reality. - // expect(wrapper.find('.invalid-feedback').text()).toEqual('Not good: Very not good') - expect(wrapper.find('.invalid-feedback').text()).toEqual('Unknown error') - expect(wrapper.emitted()).toEqual({ - change: [[{ state: 'searching', labware: null }], [{ state: 'invalid', labware: null }]], - }) - }) - describe('When labwareType is tube', () => { const goodTubeUuid = 'afabla7e-9498-42d6-964e-50f61ded6d9a' const pendingTubeUuid = '123e4567-e89b-12d3-a456-426614174000' @@ -258,6 +217,47 @@ describe('LabwareScan', () => { expect(events.change[1][0].state).toEqual('invalid') expect(events.change[1][0].labware.uuid).toEqual(pendingTubeUuid) }) + + it('is invalid if there are api troubles', async () => { + const api = mockApi() + api.mockFail( + 'tubes', + { + filter: { barcode: 'Good barcode' }, + include: '', + fields: { + tubes: 'labware_barcode,uuid,receptacle,state', + receptacles: 'uuid', + }, + }, + { + errors: [ + { + title: 'Not good', + detail: 'Very not good', + code: 500, + status: 500, + }, + ], + } + ) + const wrapper = wrapperFactoryTube(api) + + wrapper.find('input').setValue('Good barcode') + await wrapper.find('input').trigger('change') + + expect(wrapper.find('.wait-labware').exists()).toBe(true) + + await flushPromises() + + // JG: Can't seem to get the mock api to correctly handle errors. THis would be the + // desired behaviour, and seems to actually work in reality. + // expect(wrapper.find('.invalid-feedback').text()).toEqual('Not good: Very not good') + expect(wrapper.find('.invalid-feedback').text()).toEqual('Unknown error') + expect(wrapper.emitted()).toEqual({ + change: [[{ state: 'searching', labware: null }], [{ state: 'invalid', labware: null }]], + }) + }) }) describe('When labwareType is plate', () => { @@ -371,5 +371,45 @@ describe('LabwareScan', () => { expect(events.change[1][0].state).toEqual('invalid') expect(events.change[1][0].plate.uuid).toEqual(badPlateUuid) }) + + it('is invalid if there are api troubles', async () => { + const api = mockApi() + api.mockFail( + 'plates', + { + filter: { barcode: 'Good barcode' }, + include: 'wells.requests_as_source,wells.aliquots.request', + fields: { + plates: 'labware_barcode,uuid,number_of_rows,number_of_columns', + }, + }, + { + errors: [ + { + title: 'Not good', + detail: 'Very not good', + code: 500, + status: 500, + }, + ], + } + ) + const wrapper = wrapperFactoryPlate(api) + + wrapper.find('input').setValue('Good barcode') + await wrapper.find('input').trigger('change') + + expect(wrapper.find('.wait-labware').exists()).toBe(true) + + await flushPromises() + + // JG: Can't seem to get the mock api to correctly handle errors. THis would be the + // desired behaviour, and seems to actually work in reality. + // expect(wrapper.find('.invalid-feedback').text()).toEqual('Not good: Very not good') + expect(wrapper.find('.invalid-feedback').text()).toEqual('Unknown error') + expect(wrapper.emitted()).toEqual({ + change: [[{ state: 'searching', plate: null }], [{ state: 'invalid', plate: null }]], + }) + }) }) }) From d451a9905ef121bd84a3c2c06e27b197170d330b Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Fri, 10 Nov 2023 10:57:35 +0000 Subject: [PATCH 06/20] refactor (mock_api): clarify workings and add comments --- app/javascript/test_support/mock_api.js | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/app/javascript/test_support/mock_api.js b/app/javascript/test_support/mock_api.js index 9ce7a5ded..bb9a0a6b7 100644 --- a/app/javascript/test_support/mock_api.js +++ b/app/javascript/test_support/mock_api.js @@ -1,3 +1,14 @@ +/* +Mock the devour api to allow us to test the UI without making real requests to +the server. This is done by adding a mock middleware to the devour api. The +mock middleware intercepts requests and returns a mocked response. The mocked +response is defined by calling mockGet() or mockFail() on the mock middleware. + +The mock middleware also keeps track of how many times a mocked request has +been called. This allows us to test that the UI is making the correct number +of requests. +*/ + import devourApi from 'shared/devourApi' import sequencescapeResources from 'shared/resources' // Provides object equality comparisons. eg. @@ -48,7 +59,7 @@ const mockApi = function (resources = sequencescapeResources) { }) } - const mockResponse = { + const mockResponseMiddleware = { name: 'mock-request-response', mockedRequests: [], req: (payload) => { @@ -87,17 +98,18 @@ const mockApi = function (resources = sequencescapeResources) { devour, } + // Ensure that a 'mock-request-response' middleware is always present in + // the devour.middleware array, either by adding a new one or replacing an existing one. let mockMiddlewareIndex = devour.middleware.findIndex((mw) => { mw.name === 'mock-request-response' }) - if (mockMiddlewareIndex === -1) { - devour.middleware.unshift(mockResponse) + devour.middleware.unshift(mockResponseMiddleware) } else { - devour.middleware[mockMiddlewareIndex] = mockResponse + devour.middleware[mockMiddlewareIndex] = mockResponseMiddleware } - return mockResponse + return mockResponseMiddleware } export default mockApi From cf5b1e13f2de09a91d02f6aafd43808f1541ed20 Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Fri, 10 Nov 2023 13:06:27 +0000 Subject: [PATCH 07/20] refactor: remove unused count function --- app/javascript/test_support/mock_api.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/app/javascript/test_support/mock_api.js b/app/javascript/test_support/mock_api.js index bb9a0a6b7..f688ebe41 100644 --- a/app/javascript/test_support/mock_api.js +++ b/app/javascript/test_support/mock_api.js @@ -3,10 +3,6 @@ Mock the devour api to allow us to test the UI without making real requests to the server. This is done by adding a mock middleware to the devour api. The mock middleware intercepts requests and returns a mocked response. The mocked response is defined by calling mockGet() or mockFail() on the mock middleware. - -The mock middleware also keeps track of how many times a mocked request has -been called. This allows us to test that the UI is making the correct number -of requests. */ import devourApi from 'shared/devourApi' @@ -67,7 +63,6 @@ const mockApi = function (resources = sequencescapeResources) { const mockedRequest = findRequest(incomingRequest) if (mockedRequest) { - mockedRequest.called += 1 incomingRequest.adapter = function () { return mockedRequest.res } @@ -85,14 +80,12 @@ const mockApi = function (resources = sequencescapeResources) { mockedRequests.unshift({ req: { method: 'GET', url: `${dummyApiUrl}/${url}`, data: {}, params }, // Request res: Promise.resolve({ data: response }), // Response - called: 0, }) }, mockFail: (url, params, response) => { mockedRequests.unshift({ req: { method: 'GET', url: `${dummyApiUrl}/${url}`, data: {}, params }, // Request res: Promise.reject({ data: response }), // Response - called: 0, }) }, devour, From 2b70140b06f3df956a896b7c341000994b4fec6f Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Fri, 10 Nov 2023 13:06:42 +0000 Subject: [PATCH 08/20] tests: add mock_api tests --- app/javascript/test_support/mock_api.spec.js | 57 ++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 app/javascript/test_support/mock_api.spec.js diff --git a/app/javascript/test_support/mock_api.spec.js b/app/javascript/test_support/mock_api.spec.js new file mode 100644 index 000000000..2a1a91011 --- /dev/null +++ b/app/javascript/test_support/mock_api.spec.js @@ -0,0 +1,57 @@ +import mockApi from './mock_api' + +describe('mockApi', () => { + let api = mockApi() + + const barcode = 'DN12345' + const mockTube = { type: 'tube', id: 123 } + api.mockGet(`labware/${barcode}`, {}, { data: mockTube }) + api.mockGet('labware', { filter: { barcode: barcode } }, { data: [mockTube] }) + api.mockFail( + 'server-errors/500', + {}, + { + errors: [ + { + detail: 'A server error occurred', + code: 500, + status: 500, + }, + ], + } + ) + + it('should make a get request and return a single tube', async () => { + let response = await api.devour.one('labware', barcode).get() + + expect(response.data).toEqual(mockTube) + expect(response.errors).toEqual(undefined) + expect(response.meta).toEqual(undefined) + expect(response.links).toEqual(undefined) + }) + + it('should make a findall request and return an array of tubes', async () => { + let response = await api.devour.findAll('labware', { + filter: { barcode: barcode }, + }) + + expect(response.data).toEqual([mockTube]) + expect(response.errors).toEqual(undefined) + expect(response.meta).toEqual(undefined) + expect(response.links).toEqual(undefined) + }) + + it('should make a failed request and return an error', async () => { + let response = await api.devour + .one('server-errors', '500') + .get() + .catch((err) => { + return err + }) + + expect(response.data).toEqual(undefined) + expect(response.errors).toEqual([{ code: 500, detail: 'A server error occurred', status: 500 }]) + expect(response.meta).toEqual(undefined) + expect(response.links).toEqual(undefined) + }) +}) From 651f381c28295a748a93e008aa0d328934d1b306 Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Fri, 10 Nov 2023 14:40:23 +0000 Subject: [PATCH 09/20] fix: repair broken mockFail --- app/javascript/test_support/mock_api.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/javascript/test_support/mock_api.js b/app/javascript/test_support/mock_api.js index f688ebe41..e62451a94 100644 --- a/app/javascript/test_support/mock_api.js +++ b/app/javascript/test_support/mock_api.js @@ -76,6 +76,9 @@ const mockApi = function (resources = sequencescapeResources) { return payload }, + error: function (payload) { + return Promise.reject(payload) + }, mockGet: (url, params, response) => { mockedRequests.unshift({ req: { method: 'GET', url: `${dummyApiUrl}/${url}`, data: {}, params }, // Request @@ -85,7 +88,7 @@ const mockApi = function (resources = sequencescapeResources) { mockFail: (url, params, response) => { mockedRequests.unshift({ req: { method: 'GET', url: `${dummyApiUrl}/${url}`, data: {}, params }, // Request - res: Promise.reject({ data: response }), // Response + res: Promise.reject(response), // Response }) }, devour, From b8cad7f94d06873c7b4201568871a3e75dcb9bb9 Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Tue, 7 Nov 2023 17:50:10 +0000 Subject: [PATCH 10/20] fix: repair broken error feedback in tests --- .../shared/components/LabwareScan.spec.js | 42 +++++++++++++++---- .../shared/components/LabwareScan.vue | 27 +++++++----- .../shared/components/mixins/devourSelect.js | 29 +++++++------ .../components/mixins/devourSelect.spec.js | 6 +-- 4 files changed, 68 insertions(+), 36 deletions(-) diff --git a/app/javascript/shared/components/LabwareScan.spec.js b/app/javascript/shared/components/LabwareScan.spec.js index 74f28de3e..8f1ce7e6a 100644 --- a/app/javascript/shared/components/LabwareScan.spec.js +++ b/app/javascript/shared/components/LabwareScan.spec.js @@ -1,3 +1,5 @@ +// Note: strictly speaking this is an integration test, not a unit test, as it includes mocks at the API level + // Import the component being tested import { mount } from '@vue/test-utils' import flushPromises from 'flush-promises' @@ -219,6 +221,21 @@ describe('LabwareScan', () => { }) it('is invalid if there are api troubles', async () => { + // Devour automatically logs the error, which looks like: + // console.log + // devour error { + // error: { + // errors: [ + // { + // title: 'Not good', + // detail: 'Very not good', + // code: 500, + // status: 500 + // } + // ] + // } + // } + // Please do not panic, but it would be nice to suppress _only this output_ this in the console const api = mockApi() api.mockFail( 'tubes', @@ -250,10 +267,7 @@ describe('LabwareScan', () => { await flushPromises() - // JG: Can't seem to get the mock api to correctly handle errors. THis would be the - // desired behaviour, and seems to actually work in reality. - // expect(wrapper.find('.invalid-feedback').text()).toEqual('Not good: Very not good') - expect(wrapper.find('.invalid-feedback').text()).toEqual('Unknown error') + expect(wrapper.find('.invalid-feedback').text()).toEqual('Not good: Very not good') expect(wrapper.emitted()).toEqual({ change: [[{ state: 'searching', labware: null }], [{ state: 'invalid', labware: null }]], }) @@ -373,6 +387,21 @@ describe('LabwareScan', () => { }) it('is invalid if there are api troubles', async () => { + // Devour automatically logs the error, which looks like: + // console.log + // devour error { + // error: { + // errors: [ + // { + // title: 'Not good', + // detail: 'Very not good', + // code: 500, + // status: 500 + // } + // ] + // } + // } + // Please do not panic, but it would be nice to suppress _only this output_ this in the console const api = mockApi() api.mockFail( 'plates', @@ -403,10 +432,7 @@ describe('LabwareScan', () => { await flushPromises() - // JG: Can't seem to get the mock api to correctly handle errors. THis would be the - // desired behaviour, and seems to actually work in reality. - // expect(wrapper.find('.invalid-feedback').text()).toEqual('Not good: Very not good') - expect(wrapper.find('.invalid-feedback').text()).toEqual('Unknown error') + expect(wrapper.find('.invalid-feedback').text()).toEqual('Not good: Very not good') expect(wrapper.emitted()).toEqual({ change: [[{ state: 'searching', plate: null }], [{ state: 'invalid', plate: null }]], }) diff --git a/app/javascript/shared/components/LabwareScan.vue b/app/javascript/shared/components/LabwareScan.vue index 93ea31e1a..546fb6d39 100644 --- a/app/javascript/shared/components/LabwareScan.vue +++ b/app/javascript/shared/components/LabwareScan.vue @@ -209,12 +209,16 @@ export default { } }, async findLabware() { - const labware = await this.api.findAll(this.labwareType, { - include: this.includes, - filter: { barcode: this.labwareBarcode }, - fields: this.fieldsToRetrieve(), - }) - return labware.data[0] + // returns a promise that can later be caught should the request fail + return await this.api + .findAll(this.labwareType, { + include: this.includes, + filter: { barcode: this.labwareBarcode }, + fields: this.fieldsToRetrieve(), + }) + .then((response) => { + return response.data[0] + }) }, fieldsToRetrieve() { if (this.fields) { @@ -236,14 +240,15 @@ export default { this.labware = result this.apiActivity = { state: 'valid', message: 'Search complete' } }, - apiError(err) { - if (!err) { + apiError(response) { + if (!response) { this.apiActivity = { state: 'invalid', message: 'Unknown error' } - } else if (err[0]) { - const message = `${err[0].title}: ${err[0].detail}` + } else if (response.errors && Array.isArray(response.errors) && response.errors.length > 0) { + const error = response.errors[0] + const message = `${error.title}: ${error.detail}` this.apiActivity = { state: 'invalid', message } } else { - this.apiActivity = { ...err, state: 'invalid' } + this.apiActivity = { ...response, state: 'invalid' } } }, focus() { diff --git a/app/javascript/shared/components/mixins/devourSelect.js b/app/javascript/shared/components/mixins/devourSelect.js index 12468a67c..390a42774 100644 --- a/app/javascript/shared/components/mixins/devourSelect.js +++ b/app/javascript/shared/components/mixins/devourSelect.js @@ -132,25 +132,30 @@ export default { }, // Override this method when you need different behaviour async performFind() { - const qryResult = await this.api.findAll(this.resourceName, { - include: this.includes, - filter: this.filter, - fields: this.fields, - }) - return qryResult.data + // returns a promise that can later be caught should the request fail + return await this.api + .findAll(this.resourceName, { + include: this.includes, + filter: this.filter, + fields: this.fields, + }) + .then((response) => { + return response.data[0] + }) }, apiSuccess(results) { this.results = results this.apiActivity = { state: 'valid', message: 'Search complete' } }, - apiError(err) { - if (!err) { + apiError(response) { + if (!response) { this.apiActivity = { state: 'invalid', message: 'Unknown error' } - } else if (err[0] && err[0].title && err[0].detail) { - const msg = `${err[0].title}: ${err[0].detail}` - this.apiActivity = { state: 'invalid', message: msg } + } else if (response.errors && Array.isArray(response.errors) && response.errors.length > 0) { + const error = response.errors[0] + const message = `${error.title}: ${error.detail}` + this.apiActivity = { state: 'invalid', message } } else { - this.apiActivity = { ...err, state: 'invalid' } + this.apiActivity = { ...response, state: 'invalid' } } }, }, diff --git a/app/javascript/shared/components/mixins/devourSelect.spec.js b/app/javascript/shared/components/mixins/devourSelect.spec.js index e53be9d85..a824e1bf0 100644 --- a/app/javascript/shared/components/mixins/devourSelect.spec.js +++ b/app/javascript/shared/components/mixins/devourSelect.spec.js @@ -109,11 +109,7 @@ describe('DevourSelect mixin', () => { await flushPromises() - // for some reason cannot return the error properly from mockApi - // we expect this result: - // expect(wrapper.vm.feedback).toEqual('Not good: Very not good') - // but because the error doesn't come back we use this: - expect(wrapper.vm.feedback).toEqual('Unknown error') + expect(wrapper.vm.feedback).toEqual('Not good: Very not good') expect(wrapper.emitted()).toEqual({ change: [[{ state: 'invalid', results: null }]], }) From be4af36e3cfceea20a85b4d16e3ea2e38453845b Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Mon, 13 Nov 2023 10:54:35 +0000 Subject: [PATCH 11/20] refactor: clarify middleware replacement --- app/javascript/test_support/mock_api.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/javascript/test_support/mock_api.js b/app/javascript/test_support/mock_api.js index e62451a94..deac028b0 100644 --- a/app/javascript/test_support/mock_api.js +++ b/app/javascript/test_support/mock_api.js @@ -100,9 +100,9 @@ const mockApi = function (resources = sequencescapeResources) { mw.name === 'mock-request-response' }) if (mockMiddlewareIndex === -1) { - devour.middleware.unshift(mockResponseMiddleware) + devour.middleware.unshift(mockResponseMiddleware) // add mock middleware as the first middleware } else { - devour.middleware[mockMiddlewareIndex] = mockResponseMiddleware + devourApi.replaceMiddleware('mock-request-response', mockResponseMiddleware) } return mockResponseMiddleware From 27da2b7d462ac14e6ef884aab0f52be8d6a669de Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Mon, 13 Nov 2023 17:10:04 +0000 Subject: [PATCH 12/20] fix: repair the failure message in "real-life" - will have to address the tests again... --- .../shared/components/LabwareScan.vue | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/app/javascript/shared/components/LabwareScan.vue b/app/javascript/shared/components/LabwareScan.vue index 546fb6d39..d15f1f3a9 100644 --- a/app/javascript/shared/components/LabwareScan.vue +++ b/app/javascript/shared/components/LabwareScan.vue @@ -241,15 +241,22 @@ export default { this.apiActivity = { state: 'valid', message: 'Search complete' } }, apiError(response) { - if (!response) { - this.apiActivity = { state: 'invalid', message: 'Unknown error' } - } else if (response.errors && Array.isArray(response.errors) && response.errors.length > 0) { - const error = response.errors[0] - const message = `${error.title}: ${error.detail}` - this.apiActivity = { state: 'invalid', message } - } else { - this.apiActivity = { ...response, state: 'invalid' } + // switch statement on 'response' against differing error formats + // falsy -> { state: 'invalid', message: 'Unknown error' } + // {0: {title: '...', detail: '...'}} -> { state: 'invalid', message: '...' } + // default -> { ...response, state: 'invalid' } + let errorResponse + switch (true) { + case !response: + errorResponse = { state: 'invalid', message: 'Unknown error' } + break + case Boolean(response[0] && response[0].title && response[0].detail): + errorResponse = { state: 'invalid', message: `${response[0].title}: ${response[0].detail}` } + break + default: + errorResponse = { ...response, state: 'invalid' } } + this.apiActivity = errorResponse }, focus() { this.$refs.scan.$el.focus() From bf776f7f857ff9a46cb14f3c63dcf7f7a9374dcc Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Thu, 16 Nov 2023 14:22:37 +0000 Subject: [PATCH 13/20] refactor: encapsulate and implement error validation --- .../shared/components/LabwareScan.vue | 18 ++-------------- .../shared/components/mixins/devourSelect.js | 12 +++-------- app/javascript/shared/devourApiValidators.js | 21 ++++++++++++++++++- app/javascript/test_support/mock_api.js | 7 ++++++- app/javascript/test_support/mock_api.spec.js | 5 +---- 5 files changed, 32 insertions(+), 31 deletions(-) diff --git a/app/javascript/shared/components/LabwareScan.vue b/app/javascript/shared/components/LabwareScan.vue index d15f1f3a9..2e4c4a3db 100644 --- a/app/javascript/shared/components/LabwareScan.vue +++ b/app/javascript/shared/components/LabwareScan.vue @@ -43,6 +43,7 @@ import { checkSize } from './plateScanValidators' import { aggregate } from './scanValidators' import { checkState } from './tubeScanValidators' +import { validateError } from 'shared/devourApiValidators' // Incrementing counter to ensure all instances of LabwareScan // have a unique id. Ensures labels correctly match up with @@ -241,22 +242,7 @@ export default { this.apiActivity = { state: 'valid', message: 'Search complete' } }, apiError(response) { - // switch statement on 'response' against differing error formats - // falsy -> { state: 'invalid', message: 'Unknown error' } - // {0: {title: '...', detail: '...'}} -> { state: 'invalid', message: '...' } - // default -> { ...response, state: 'invalid' } - let errorResponse - switch (true) { - case !response: - errorResponse = { state: 'invalid', message: 'Unknown error' } - break - case Boolean(response[0] && response[0].title && response[0].detail): - errorResponse = { state: 'invalid', message: `${response[0].title}: ${response[0].detail}` } - break - default: - errorResponse = { ...response, state: 'invalid' } - } - this.apiActivity = errorResponse + this.apiActivity = validateError(response) }, focus() { this.$refs.scan.$el.focus() diff --git a/app/javascript/shared/components/mixins/devourSelect.js b/app/javascript/shared/components/mixins/devourSelect.js index 390a42774..ebf4675a2 100644 --- a/app/javascript/shared/components/mixins/devourSelect.js +++ b/app/javascript/shared/components/mixins/devourSelect.js @@ -1,3 +1,5 @@ +import { validateError } from 'shared/devourApiValidators' + const boolToString = { true: 'valid', false: 'invalid' } /** @@ -148,15 +150,7 @@ export default { this.apiActivity = { state: 'valid', message: 'Search complete' } }, apiError(response) { - if (!response) { - this.apiActivity = { state: 'invalid', message: 'Unknown error' } - } else if (response.errors && Array.isArray(response.errors) && response.errors.length > 0) { - const error = response.errors[0] - const message = `${error.title}: ${error.detail}` - this.apiActivity = { state: 'invalid', message } - } else { - this.apiActivity = { ...response, state: 'invalid' } - } + this.apiActivity = validateError(response) }, }, render() { diff --git a/app/javascript/shared/devourApiValidators.js b/app/javascript/shared/devourApiValidators.js index 0c3d65f48..bb9b735cf 100644 --- a/app/javascript/shared/devourApiValidators.js +++ b/app/javascript/shared/devourApiValidators.js @@ -1,3 +1,22 @@ +const validateError = (response) => { + // switch statement on 'response' against differing error formats + // falsy -> { state: 'invalid', message: 'Unknown error' } + // {0: {title: '...', detail: '...'}} -> { state: 'invalid', message: '...' } + // default -> { ...response, state: 'invalid' } + let errorResponse + switch (true) { + case !response: + errorResponse = { state: 'invalid', message: 'Unknown error' } + break + case Boolean(response[0] && response[0].title && response[0].detail): + errorResponse = { state: 'invalid', message: `${response[0].title}: ${response[0].detail}` } + break + default: + errorResponse = { ...response, state: 'invalid' } + } + return errorResponse +} + const hasExpectedProperties = (expectedProperties) => { return (results) => { if (results.length > 0) { @@ -21,4 +40,4 @@ const hasExpectedProperties = (expectedProperties) => { } } -export { hasExpectedProperties } +export { validateError, hasExpectedProperties } diff --git a/app/javascript/test_support/mock_api.js b/app/javascript/test_support/mock_api.js index deac028b0..819cca1e9 100644 --- a/app/javascript/test_support/mock_api.js +++ b/app/javascript/test_support/mock_api.js @@ -86,9 +86,14 @@ const mockApi = function (resources = sequencescapeResources) { }) }, mockFail: (url, params, response) => { + // for each error in response.errors, create an object with the indices as keys and the error items as values + const errors = response.errors.reduce((accumulator, error, index) => { + accumulator[index] = error + return accumulator + }, {}) mockedRequests.unshift({ req: { method: 'GET', url: `${dummyApiUrl}/${url}`, data: {}, params }, // Request - res: Promise.reject(response), // Response + res: Promise.reject(errors), // Response }) }, devour, diff --git a/app/javascript/test_support/mock_api.spec.js b/app/javascript/test_support/mock_api.spec.js index 2a1a91011..8fc4ce17a 100644 --- a/app/javascript/test_support/mock_api.spec.js +++ b/app/javascript/test_support/mock_api.spec.js @@ -49,9 +49,6 @@ describe('mockApi', () => { return err }) - expect(response.data).toEqual(undefined) - expect(response.errors).toEqual([{ code: 500, detail: 'A server error occurred', status: 500 }]) - expect(response.meta).toEqual(undefined) - expect(response.links).toEqual(undefined) + expect(response).toEqual({ 0: { detail: 'A server error occurred', code: 500, status: 500 } }) }) }) From 403a9a95e853269fd0308aa35a3d4683b11d0384 Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Thu, 16 Nov 2023 16:47:26 +0000 Subject: [PATCH 14/20] tests (AssetLookupByUuid.spec.js): remove nullPlate definition --- app/javascript/shared/components/AssetLookupByUuid.spec.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/javascript/shared/components/AssetLookupByUuid.spec.js b/app/javascript/shared/components/AssetLookupByUuid.spec.js index 7b5cb104f..a5f1a2fc2 100644 --- a/app/javascript/shared/components/AssetLookupByUuid.spec.js +++ b/app/javascript/shared/components/AssetLookupByUuid.spec.js @@ -7,7 +7,6 @@ import localVue from 'test_support/base_vue' describe('AssetLookupByUuid', () => { const assetUuid = 'afabla7e-9498-42d6-964e-50f61ded6d9a' - const nullPlate = { data: [] } const goodPlate = jsonCollectionFactory('plate', [{ uuid: assetUuid }]) const wrapperFactoryPlate = function (api = mockApi()) { @@ -35,7 +34,7 @@ describe('AssetLookupByUuid', () => { filter: { uuid: assetUuid }, fields: {}, }, - nullPlate + { data: [] } // no plates found ) const wrapper = wrapperFactoryPlate(api) From ab084eb3914fdb2cff2d3b948d6afe465a265f16 Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Fri, 17 Nov 2023 08:59:26 +0000 Subject: [PATCH 15/20] refactor: clarify intention of hasExpectedProperties --- app/javascript/shared/devourApiValidators.js | 30 ++++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/app/javascript/shared/devourApiValidators.js b/app/javascript/shared/devourApiValidators.js index bb9b735cf..ceb6ef19b 100644 --- a/app/javascript/shared/devourApiValidators.js +++ b/app/javascript/shared/devourApiValidators.js @@ -19,24 +19,24 @@ const validateError = (response) => { const hasExpectedProperties = (expectedProperties) => { return (results) => { - if (results.length > 0) { - for (var resultsIndex = 0; resultsIndex < results.length; resultsIndex++) { - const currTagGroup = results[resultsIndex] - const expectedPropertiesLength = expectedProperties.length - for (var propIndex = 0; propIndex < expectedPropertiesLength; propIndex++) { - const expectedPropertyName = expectedProperties[propIndex] - if (!Object.prototype.hasOwnProperty.call(currTagGroup, expectedPropertyName)) { - return { - valid: false, - message: 'Results objects do not contain expected properties', - } - } + if (results.length === 0) { + return { valid: false, message: 'No results retrieved' } + } + + for (const currTagGroup of results) { + const hasAllProperties = expectedProperties.every((property) => + Object.prototype.hasOwnProperty.call(currTagGroup, property) + ) + + if (!hasAllProperties) { + return { + valid: false, + message: 'Results objects do not contain expected properties', } } - return { valid: true, message: 'Results valid' } - } else { - return { valid: false, message: 'No results retrieved' } } + + return { valid: true, message: 'Results valid' } } } From a9046d998d1283e077f775f1ecc6c64591671cfd Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Fri, 17 Nov 2023 09:00:10 +0000 Subject: [PATCH 16/20] fix: repair a small bug that was causing AssetLookupByUuid to fail --- app/javascript/shared/components/mixins/devourSelect.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/javascript/shared/components/mixins/devourSelect.js b/app/javascript/shared/components/mixins/devourSelect.js index ebf4675a2..edd5a7f7b 100644 --- a/app/javascript/shared/components/mixins/devourSelect.js +++ b/app/javascript/shared/components/mixins/devourSelect.js @@ -142,7 +142,7 @@ export default { fields: this.fields, }) .then((response) => { - return response.data[0] + return response.data }) }, apiSuccess(results) { From 64e2dd568fa79df9a1378f33644f5729808403e6 Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Thu, 23 Nov 2023 15:06:18 +0000 Subject: [PATCH 17/20] tests: add tests for devourApiValidators --- .../shared/devourApiValidators.spec.js | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 app/javascript/shared/devourApiValidators.spec.js diff --git a/app/javascript/shared/devourApiValidators.spec.js b/app/javascript/shared/devourApiValidators.spec.js new file mode 100644 index 000000000..ee103f356 --- /dev/null +++ b/app/javascript/shared/devourApiValidators.spec.js @@ -0,0 +1,47 @@ +import { validateError, hasExpectedProperties } from './devourApiValidators.js' + +describe('validateError', () => { + it('should return unknown error for falsy response', () => { + const response = null + const result = validateError(response) + expect(result).toEqual({ state: 'invalid', message: 'Unknown error' }) + }) + + it('should return formatted error for response with title and detail', () => { + const response = [{ title: 'Test Error', detail: 'This is a test error' }] + const result = validateError(response) + expect(result).toEqual({ state: 'invalid', message: 'Test Error: This is a test error' }) + }) + + it('should return response with state invalid for other responses', () => { + const response = { someKey: 'someValue' } + const result = validateError(response) + expect(result).toEqual({ ...response, state: 'invalid' }) + }) +}) + +describe('hasExpectedProperties', () => { + it('should return no results retrieved for empty results', () => { + const results = [] + const expectedProperties = ['prop1', 'prop2'] + const validator = hasExpectedProperties(expectedProperties) + const result = validator(results) + expect(result).toEqual({ valid: false, message: 'No results retrieved' }) + }) + + it('should return results objects do not contain expected properties for missing properties', () => { + const results = [{ prop1: 'value1' }] + const expectedProperties = ['prop1', 'prop2'] + const validator = hasExpectedProperties(expectedProperties) + const result = validator(results) + expect(result).toEqual({ valid: false, message: 'Results objects do not contain expected properties' }) + }) + + it('should return results valid for results with all expected properties', () => { + const results = [{ prop1: 'value1', prop2: 'value2' }] + const expectedProperties = ['prop1', 'prop2'] + const validator = hasExpectedProperties(expectedProperties) + const result = validator(results) + expect(result).toEqual({ valid: true, message: 'Results valid' }) + }) +}) From 0f7fe739dea907f0e15272f98bd36121b6e98a4c Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Thu, 23 Nov 2023 15:43:40 +0000 Subject: [PATCH 18/20] Revert "fix: add validation check for passed state" This reverts commit 2a2b7241f3da63073866d6dd1d9165e039d036e1. --- app/javascript/shared/components/LabwareScan.vue | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/app/javascript/shared/components/LabwareScan.vue b/app/javascript/shared/components/LabwareScan.vue index 2e4c4a3db..83ad9d95c 100644 --- a/app/javascript/shared/components/LabwareScan.vue +++ b/app/javascript/shared/components/LabwareScan.vue @@ -42,7 +42,6 @@ <script> import { checkSize } from './plateScanValidators' import { aggregate } from './scanValidators' -import { checkState } from './tubeScanValidators' import { validateError } from 'shared/devourApiValidators' // Incrementing counter to ensure all instances of LabwareScan @@ -105,6 +104,13 @@ export default { type: Object, default: null, }, + validators: { + // An array of validators. See plateScanValidators.js and tubeScanValidators.js for examples and details + // defaults are set based on labwareType, in method 'computedValidators' + type: Array, + required: false, + default: null, + }, scanDisabled: { // Used to disable the scan field. type: Boolean, @@ -179,8 +185,12 @@ export default { } }, computedValidators() { + if (this.validators) { + return this.validators + } + if (this.labwareType == 'tube') { - return [checkState(['passed'])] + return [] } else { return [checkSize(12, 8)] } @@ -228,7 +238,7 @@ export default { if (this.labwareType == 'tube') { return { - tubes: 'labware_barcode,uuid,receptacle,state', + tubes: 'labware_barcode,uuid,receptacle', receptacles: 'uuid', } } else { From e0d2b5c5cc3ae2317a698369dfd358e7f3b395fb Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Thu, 23 Nov 2023 16:18:43 +0000 Subject: [PATCH 19/20] fix (MultiStampTubes): add validation check for passed state --- .../multi-stamp-tubes/components/MultiStampTubes.vue | 4 ++-- app/javascript/shared/components/LabwareScan.spec.js | 7 +++++-- app/javascript/shared/components/LabwareScan.vue | 7 ++++--- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/app/javascript/multi-stamp-tubes/components/MultiStampTubes.vue b/app/javascript/multi-stamp-tubes/components/MultiStampTubes.vue index 36a78327f..6e5d96e28 100644 --- a/app/javascript/multi-stamp-tubes/components/MultiStampTubes.vue +++ b/app/javascript/multi-stamp-tubes/components/MultiStampTubes.vue @@ -58,7 +58,7 @@ import devourApi from 'shared/devourApi' import resources from 'shared/resources' import { buildTubeObjs } from 'shared/tubeHelpers' import { transfersForTubes } from 'shared/transfersLayouts' -import { checkDuplicates } from 'shared/components/tubeScanValidators' +import { checkDuplicates, checkState } from 'shared/components/tubeScanValidators' import { validScanMessage } from 'shared/components/scanValidators' import { indexToName } from 'shared/wellHelpers' @@ -198,7 +198,7 @@ export default { return [validScanMessage] } const currTubes = this.tubes.map((tubeItem) => tubeItem.labware) - return [checkDuplicates(currTubes)] + return [checkState(['passed']), checkDuplicates(currTubes)] }, }, methods: { diff --git a/app/javascript/shared/components/LabwareScan.spec.js b/app/javascript/shared/components/LabwareScan.spec.js index 8f1ce7e6a..3c49da1f2 100644 --- a/app/javascript/shared/components/LabwareScan.spec.js +++ b/app/javascript/shared/components/LabwareScan.spec.js @@ -4,6 +4,7 @@ import { mount } from '@vue/test-utils' import flushPromises from 'flush-promises' import LabwareScan from 'shared/components/LabwareScan.vue' +import { checkState } from 'shared/components/tubeScanValidators' import { jsonCollectionFactory } from 'test_support/factories' import mockApi from 'test_support/mock_api' @@ -25,7 +26,7 @@ describe('LabwareScan', () => { }) } - const wrapperFactoryTube = function (api = mockApi()) { + const wrapperFactoryTube = function (api = mockApi(), validators = undefined) { return mount(LabwareScan, { propsData: { labwareType: 'tube', @@ -34,6 +35,7 @@ describe('LabwareScan', () => { api: api.devour, includes: '', colourIndex: 3, + validators: validators, }, localVue, }) @@ -187,7 +189,8 @@ describe('LabwareScan', () => { it('is invalid if the tube is in the pending state', async () => { const api = mockApi() - const wrapper = wrapperFactoryTube(api) + const validators = [checkState(['passed'])] + const wrapper = wrapperFactoryTube(api, validators) api.mockGet( 'tubes', diff --git a/app/javascript/shared/components/LabwareScan.vue b/app/javascript/shared/components/LabwareScan.vue index 83ad9d95c..4d4e8939d 100644 --- a/app/javascript/shared/components/LabwareScan.vue +++ b/app/javascript/shared/components/LabwareScan.vue @@ -105,8 +105,9 @@ export default { default: null, }, validators: { - // An array of validators. See plateScanValidators.js and tubeScanValidators.js for examples and details - // defaults are set based on labwareType, in method 'computedValidators' + // An array of validators. See plateScanValidators.js and tubeScanValidators.js for options and details + // and ValidatePairedTubes.vue for an usage example. + // Defaults are set based on labwareType, in method 'computedValidators'. type: Array, required: false, default: null, @@ -238,7 +239,7 @@ export default { if (this.labwareType == 'tube') { return { - tubes: 'labware_barcode,uuid,receptacle', + tubes: 'labware_barcode,uuid,receptacle,state', receptacles: 'uuid', } } else { From a2f0526775d2d96eef60c46243339772b0d6bc40 Mon Sep 17 00:00:00 2001 From: Stephen Hulme <sh51@sanger.ac.uk> Date: Thu, 23 Nov 2023 16:30:41 +0000 Subject: [PATCH 20/20] tests (MultiStampTubes): add test for validation of passed state --- .../components/MultiStampTubes.spec.js | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/app/javascript/multi-stamp-tubes/components/MultiStampTubes.spec.js b/app/javascript/multi-stamp-tubes/components/MultiStampTubes.spec.js index f3fd8b3f7..0f3a25a83 100644 --- a/app/javascript/multi-stamp-tubes/components/MultiStampTubes.spec.js +++ b/app/javascript/multi-stamp-tubes/components/MultiStampTubes.spec.js @@ -91,7 +91,7 @@ describe('MultiStampTubes', () => { wrapper.vm.updateTube(1, tube1) wrapper.vm.updateTube(2, tube1) - const validator = wrapper.vm.scanValidation[0] + const validator = wrapper.vm.scanValidation[1] const validations = validator(tube1.labware) expect(validations.valid).toEqual(false) @@ -113,6 +113,20 @@ describe('MultiStampTubes', () => { expect(validations.valid).toEqual(true) }) + it('is not valid when we scan pending tubes', async () => { + const wrapper = wrapperFactory() + const pendingTube = { + state: 'valid', + labware: tubeFactory({ state: 'pending' }), + } + + wrapper.vm.updateTube(1, pendingTube) + const validator = wrapper.vm.scanValidation[0] + const validations = validator(pendingTube.labware) + + expect(validations.valid).toEqual(false) + }) + it('sends a post request when the button is clicked', async () => { let mock = new MockAdapter(localVue.prototype.$axios)