From d908cbdea7e8745a83143fb516b22b2aa29f637d Mon Sep 17 00:00:00 2001 From: djhi Date: Wed, 15 Feb 2017 17:04:19 +0100 Subject: [PATCH 1/2] [RFR] Fix LINK function when reference is null --- makefile | 2 +- src/api/controller/api/fetchLineBy.js | 8 +++ src/api/controller/api/fetchLineBy.spec.js | 51 ++++++++++++++++++++ src/api/controller/api/parsing.js | 7 +-- src/api/controller/api/publish.js | 2 + src/app/e2e/admin/linked_sample_csv.csv | 1 + src/app/e2e/admin/publication.js | 20 +++++--- src/app/js/admin/publicationPreview/sagas.js | 4 +- src/common/transformers/getOtherLine.js | 4 +- 9 files changed, 81 insertions(+), 18 deletions(-) create mode 100644 src/api/controller/api/fetchLineBy.js create mode 100644 src/api/controller/api/fetchLineBy.spec.js diff --git a/makefile b/makefile index 5c4ec47667..18041fabfd 100644 --- a/makefile +++ b/makefile @@ -76,7 +76,7 @@ test-frontend-unit: ## Run the frontend application unit tests "./src/app/js/**/*.spec.js" test-frontend-functional: ## Run the frontend application functional tests - NODE_ENV=test ${MAKE} build-frontend + # NODE_ENV=test ${MAKE} build-frontend NODE_ENV=test SELENIUM_BROWSER_BINARY_PATH="./node_modules/selenium-standalone/.selenium/chromedriver/2.24-x64-chromedriver" \ ./node_modules/.bin/mocha \ --require babel-polyfill \ diff --git a/src/api/controller/api/fetchLineBy.js b/src/api/controller/api/fetchLineBy.js new file mode 100644 index 0000000000..5f99cfdeee --- /dev/null +++ b/src/api/controller/api/fetchLineBy.js @@ -0,0 +1,8 @@ +export default ctx => (fieldName, value) => + ctx.dataset + .findBy(fieldName, value) + .then(line => ({ + ...line, + uri: `uri to ${fieldName}: ${value}`, + })) + .catch(() => null); diff --git a/src/api/controller/api/fetchLineBy.spec.js b/src/api/controller/api/fetchLineBy.spec.js new file mode 100644 index 0000000000..d3945d200f --- /dev/null +++ b/src/api/controller/api/fetchLineBy.spec.js @@ -0,0 +1,51 @@ +import expect, { createSpy } from 'expect'; +import fetchLineBy from './fetchLineBy'; + +describe('fetchLineBy', () => { + it('calls ctx.dataset.findBy', () => { + const line = { data: 'the line' }; + const field = 'the field'; + const value = 'the value'; + + const ctx = { + dataset: { + findBy: createSpy().andReturn(Promise.resolve(line)), + }, + }; + + fetchLineBy(ctx)(field, value); + expect(ctx.dataset.findBy).toHaveBeenCalledWith(field, value); + }); + + it('returns the line with a computed uri when ctx.dataset.findBy succeeds', async () => { + const line = { data: 'the line' }; + const field = 'the field'; + const value = 'the value'; + + const ctx = { + dataset: { + findBy: createSpy().andReturn(Promise.resolve(line)), + }, + }; + + const result = await fetchLineBy(ctx)(field, value); + expect(result).toEqual({ + ...line, + uri: `uri to ${field}: ${value}`, + }); + }); + + it('returns the line with a computed uri when ctx.dataset.findBy fails', async () => { + const field = 'the field'; + const value = 'the value'; + + const ctx = { + dataset: { + findBy: createSpy().andReturn(Promise.reject()), + }, + }; + + const result = await fetchLineBy(ctx)(field, value); + expect(result).toEqual(null); + }); +}); diff --git a/src/api/controller/api/parsing.js b/src/api/controller/api/parsing.js index de3aff4c3d..d46840f720 100644 --- a/src/api/controller/api/parsing.js +++ b/src/api/controller/api/parsing.js @@ -1,5 +1,6 @@ import Koa from 'koa'; import route from 'koa-route'; +import fetchLineBy from './fetchLineBy'; const app = new Koa(); @@ -14,11 +15,7 @@ export const getExcerpt = async (ctx) => { }; export const findBy = async (ctx, fieldName, value) => { - const line = await ctx.dataset.findBy(fieldName, value); - ctx.body = { - ...line, - uri: `uri to ${fieldName}: ${value}`, - }; + ctx.body = await fetchLineBy(ctx)(fieldName, value); }; app.use(route.get('/', getExcerpt)); diff --git a/src/api/controller/api/publish.js b/src/api/controller/api/publish.js index d4bb763fc3..d8ba409252 100644 --- a/src/api/controller/api/publish.js +++ b/src/api/controller/api/publish.js @@ -1,6 +1,7 @@ import omit from 'lodash.omit'; import Koa from 'koa'; import route from 'koa-route'; +import fetchLineBy from './fetchLineBy'; /* eslint no-await-in-loop: off */ import getDocumentTransformer from '../../../common/getDocumentTransformer'; @@ -98,6 +99,7 @@ export const doPublish = async (ctx) => { .getDocumentTransformer({ env: 'node', dataset: ctx.uriDataset, + fetchLineBy, }, collectionCoverFields.filter(col => col.name !== 'uri')); const transformDocumentAndKeepUri = ctx.versionTransformResult(transformDocument); diff --git a/src/app/e2e/admin/linked_sample_csv.csv b/src/app/e2e/admin/linked_sample_csv.csv index bfc774c403..00ac579eac 100644 --- a/src/app/e2e/admin/linked_sample_csv.csv +++ b/src/app/e2e/admin/linked_sample_csv.csv @@ -2,3 +2,4 @@ "1";"rock";"3"; "2";"paper";"1"; "3";"scissor";"2"; +"4";"invalid_reference";"5"; diff --git a/src/app/e2e/admin/publication.js b/src/app/e2e/admin/publication.js index 2f6e0c79b5..8d1099844e 100644 --- a/src/app/e2e/admin/publication.js +++ b/src/app/e2e/admin/publication.js @@ -7,7 +7,7 @@ import { clear } from '../../../common/tests/fixtures'; import { elementIsClicked, inputElementIsFocusable, elementValueIs } from '../../../common/tests/conditions'; describe('Admin', () => { - describe('Admin page', function homeTests() { + describe('Publication', function homeTests() { this.timeout(10000); const DEFAULT_WAIT_TIMEOUT = 9000; // A bit less than mocha's timeout to get explicit errors from selenium @@ -60,7 +60,7 @@ describe('Admin', () => { expect(text).toBe('uri'); const tds = await driver.findElements(By.css('.publication-preview tr td:first-child')); - expect(tds.length).toBe(3); + expect(tds.length).toBe(4); await Promise.all(tds.map(td => driver.wait(until.elementTextIs(td, ''), DEFAULT_WAIT_TIMEOUT)), ); @@ -85,7 +85,7 @@ describe('Admin', () => { it('should have completed uri column with generated uri', async () => { const tds = await driver.findElements(By.css('.publication-preview tr td:first-child')); - expect(tds.length).toBe(3); + expect(tds.length).toBe(4); await Promise.all(tds.map(td => driver.wait(until.elementTextMatches(td, /[A-Z0-9]{8}/), DEFAULT_WAIT_TIMEOUT)), ); @@ -144,11 +144,12 @@ describe('Admin', () => { DEFAULT_WAIT_TIMEOUT, ); const tds = await driver.findElements(By.css('.publication-preview tr td:nth-child(2)')); - expect(tds.length).toBe(3); + expect(tds.length).toBe(4); const expectedTexts = [ 'uri to id: 3', 'uri to id: 1', 'uri to id: 2', + 'uri to id: 5', ]; await Promise.all(tds.map((td, index) => driver.wait(until.elementTextIs(td, expectedTexts[index]), DEFAULT_WAIT_TIMEOUT)), @@ -166,9 +167,9 @@ describe('Admin', () => { it('should have updated the preview', async () => { const tds = await driver.findElements(By.css('.publication-preview tr td:last-child')); - expect(tds.length).toBe(3); + expect(tds.length).toBe(4); await Promise.all(tds.map(td => - driver.wait(until.elementTextMatches(td, /rock|paper|scissor/), DEFAULT_WAIT_TIMEOUT)), + driver.wait(until.elementTextMatches(td, /rock|paper|scissor|invalid_reference/), DEFAULT_WAIT_TIMEOUT)), ); }); }); @@ -197,7 +198,9 @@ describe('Admin', () => { const headersText = await Promise.all(headers.map(h => h.getText())); expect(headersText).toEqual(['uri', 'stronger', 'name']); - const rows = await Promise.all([1, 2, 3].map(index => + await driver.sleep(5000); + + const rows = await Promise.all([1, 2, 3, 4].map(index => Promise.all([ driver .findElement(By.css(`.dataset table tbody tr:nth-child(${index}) td.dataset-uri`)) @@ -219,10 +222,11 @@ describe('Admin', () => { rock: 'scissor', paper: 'rock', scissor: 'paper', + invalid_reference: '', }; rows.forEach(({ stronger, name }) => { - expect(rows.find(r => r.uri === stronger).name).toEqual(expected[name]); + expect((rows.find(r => r.uri === stronger) || { name: '' }).name).toEqual(expected[name]); }); }); }); diff --git a/src/app/js/admin/publicationPreview/sagas.js b/src/app/js/admin/publicationPreview/sagas.js index 850fa94a29..bd4a915053 100644 --- a/src/app/js/admin/publicationPreview/sagas.js +++ b/src/app/js/admin/publicationPreview/sagas.js @@ -2,6 +2,8 @@ import { takeLatest } from 'redux-saga'; import { call, put, select } from 'redux-saga/effects'; import getDocumentTransformer from '../../../../common/getDocumentTransformer'; +import fetchLineBy from '../../lib/fetchLineBy'; + import { getToken } from '../../user'; import { COMPUTE_PREVIEW, @@ -27,7 +29,7 @@ export function* handleComputePreview() { const token = yield select(getToken); const fields = yield select(getFields); - const transformDocument = yield call(getDocumentTransformer, { env: 'browser', token }, fields); + const transformDocument = yield call(getDocumentTransformer, { env: 'browser', token, fetchLineBy }, fields); const lines = yield select(getExcerptLines); const preview = yield lines.map(line => call(transformDocument, line)); diff --git a/src/common/transformers/getOtherLine.js b/src/common/transformers/getOtherLine.js index 6d27146a07..c3273f0785 100644 --- a/src/common/transformers/getOtherLine.js +++ b/src/common/transformers/getOtherLine.js @@ -1,9 +1,7 @@ -import fetchLineBy from '../../app/js/lib/fetchLineBy'; - export default (context, targetColumn) => async (prev) => { if (context.env === 'node') { return context.dataset.findBy(targetColumn, prev); } - return fetchLineBy(targetColumn, prev, context.token); + return context.fetchLineBy(targetColumn, prev, context.token); }; From 3498ee2290f92a532e7bc700f000ee27b928d5b6 Mon Sep 17 00:00:00 2001 From: djhi Date: Wed, 15 Feb 2017 17:20:51 +0100 Subject: [PATCH 2/2] review --- makefile | 2 +- src/api/controller/api/fetchLineBy.js | 19 ++++++++++++------- src/api/controller/api/fetchLineBy.spec.js | 18 ++++++++++++++++-- src/api/controller/api/publish.js | 7 ++++++- src/api/controller/api/publish.spec.js | 9 ++++++++- src/app/e2e/admin/publication.js | 2 +- 6 files changed, 44 insertions(+), 13 deletions(-) diff --git a/makefile b/makefile index 18041fabfd..5c4ec47667 100644 --- a/makefile +++ b/makefile @@ -76,7 +76,7 @@ test-frontend-unit: ## Run the frontend application unit tests "./src/app/js/**/*.spec.js" test-frontend-functional: ## Run the frontend application functional tests - # NODE_ENV=test ${MAKE} build-frontend + NODE_ENV=test ${MAKE} build-frontend NODE_ENV=test SELENIUM_BROWSER_BINARY_PATH="./node_modules/selenium-standalone/.selenium/chromedriver/2.24-x64-chromedriver" \ ./node_modules/.bin/mocha \ --require babel-polyfill \ diff --git a/src/api/controller/api/fetchLineBy.js b/src/api/controller/api/fetchLineBy.js index 5f99cfdeee..9e0c8c4491 100644 --- a/src/api/controller/api/fetchLineBy.js +++ b/src/api/controller/api/fetchLineBy.js @@ -1,8 +1,13 @@ -export default ctx => (fieldName, value) => - ctx.dataset +export default ctx => (fieldName, value) => ( + value + ? ctx.dataset .findBy(fieldName, value) - .then(line => ({ - ...line, - uri: `uri to ${fieldName}: ${value}`, - })) - .catch(() => null); + .then(line => ( + line + ? ({ + ...line, + uri: `uri to ${fieldName}: ${value}`, + }) + : null), + ) + : null); diff --git a/src/api/controller/api/fetchLineBy.spec.js b/src/api/controller/api/fetchLineBy.spec.js index d3945d200f..0a4317739b 100644 --- a/src/api/controller/api/fetchLineBy.spec.js +++ b/src/api/controller/api/fetchLineBy.spec.js @@ -35,13 +35,27 @@ describe('fetchLineBy', () => { }); }); - it('returns the line with a computed uri when ctx.dataset.findBy fails', async () => { + it('returns null when specified value is null', async () => { + const field = 'the field'; + const value = null; + + const ctx = { + dataset: { + findBy: createSpy().andReturn(Promise.resolve()), + }, + }; + + const result = await fetchLineBy(ctx)(field, value); + expect(result).toEqual(null); + }); + + it('returns null when referenced line is not found', async () => { const field = 'the field'; const value = 'the value'; const ctx = { dataset: { - findBy: createSpy().andReturn(Promise.reject()), + findBy: createSpy().andReturn(Promise.resolve()), }, }; diff --git a/src/api/controller/api/publish.js b/src/api/controller/api/publish.js index d8ba409252..4b90105306 100644 --- a/src/api/controller/api/publish.js +++ b/src/api/controller/api/publish.js @@ -39,6 +39,7 @@ export const publishCharacteristics = async (ctx, datasetCoverFields, count) => .getDocumentTransformer({ env: 'node', dataset: ctx.uriDataset, + fetchLineBy, }, datasetCoverFields); const [lastResource] = await ctx.uriDataset.findLimitFromSkip(1, count - 1); @@ -85,7 +86,11 @@ export const doPublish = async (ctx) => { const datasetCoverFields = fields.filter(c => c.cover === 'dataset'); const uriCol = fields.find(col => col.name === 'uri'); - const getUri = ctx.getDocumentTransformer({ env: 'node', dataset: ctx.dataset }, [uriCol]); + const getUri = ctx.getDocumentTransformer({ + env: 'node', + dataset: ctx.dataset, + fetchLineBy, + }, [uriCol]); const addUri = ctx.addTransformResultToDoc(getUri); await ctx.tranformAllDocuments( diff --git a/src/api/controller/api/publish.spec.js b/src/api/controller/api/publish.spec.js index 47e804250a..4fe1a84696 100644 --- a/src/api/controller/api/publish.spec.js +++ b/src/api/controller/api/publish.spec.js @@ -11,6 +11,7 @@ import { publishCharacteristics, } from './publish'; import getDocumentTransformer from '../../../common/getDocumentTransformer'; +import fetchLineBy from './fetchLineBy'; describe('publish', () => { describe('doPublish', () => { @@ -64,7 +65,11 @@ describe('publish', () => { }); it('should call getDocumentTransformer to get the uri transformers', () => { - expect(ctx.getDocumentTransformer).toHaveBeenCalledWith({ env: 'node', dataset: ctx.dataset }, [fields[0]]); + expect(ctx.getDocumentTransformer).toHaveBeenCalledWith({ + env: 'node', + dataset: ctx.dataset, + fetchLineBy, + }, [fields[0]]); }); it('should call ctx.addTransformResultToDoc with transformDocument', () => { @@ -79,6 +84,7 @@ describe('publish', () => { expect(ctx.getDocumentTransformer).toHaveBeenCalledWith({ env: 'node', dataset: ctx.uriDataset, + fetchLineBy, }, [fields[1]]); }); @@ -129,6 +135,7 @@ describe('publish', () => { { env: 'node', dataset: ctx.uriDataset, + fetchLineBy, }, datasetFields, ); diff --git a/src/app/e2e/admin/publication.js b/src/app/e2e/admin/publication.js index 8d1099844e..e49fde5294 100644 --- a/src/app/e2e/admin/publication.js +++ b/src/app/e2e/admin/publication.js @@ -149,7 +149,7 @@ describe('Admin', () => { 'uri to id: 3', 'uri to id: 1', 'uri to id: 2', - 'uri to id: 5', + '', ]; await Promise.all(tds.map((td, index) => driver.wait(until.elementTextIs(td, expectedTexts[index]), DEFAULT_WAIT_TIMEOUT)),