From fa1eb6b845e1792e55b382484c41a9926cd1081f Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Thu, 29 Aug 2019 16:16:55 -0700 Subject: [PATCH 1/5] chore(csv): Emit CSVViewer parse errors silently --- src/lib/viewers/text/CSVViewer.js | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/lib/viewers/text/CSVViewer.js b/src/lib/viewers/text/CSVViewer.js index 3c0d6c0c2..adae65eac 100644 --- a/src/lib/viewers/text/CSVViewer.js +++ b/src/lib/viewers/text/CSVViewer.js @@ -67,7 +67,18 @@ class CSVViewer extends TextBaseViewer { if (this.isDestroyed() || !results) { return; } - this.data = results.data; + + const { errors = [], data } = results; + + if (errors.length) { + const error = new PreviewError(ERROR_CODE.LOAD_CSV, undefined, { + ...errors[0], + silent: true, + }); + this.triggerError(error); + } + + this.data = data; this.finishLoading(); URL.revokeObjectURL(workerSrc); }, From 832b9fff84f7cf18f2ed88ec37017f1858d1b318 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Fri, 30 Aug 2019 10:37:48 -0700 Subject: [PATCH 2/5] chore: Update error code and sort errors --- src/lib/events.js | 33 ++++---- src/lib/viewers/text/CSVViewer.js | 78 +++++++++++++++++-- .../viewers/text/__tests__/CSVViewer-test.js | 51 ++++++++++++ 3 files changed, 138 insertions(+), 24 deletions(-) diff --git a/src/lib/events.js b/src/lib/events.js index 75f4477f0..cfcd9624e 100644 --- a/src/lib/events.js +++ b/src/lib/events.js @@ -18,33 +18,34 @@ export const VIEWER_EVENT = { // Error codes logged by preview with "preview_error" events export const ERROR_CODE = { ACCOUNT: 'error_account', - UNSUPPORTED_FILE_TYPE: 'error_unsupported_file_type', - PERMISSIONS_PREVIEW: 'error_permissions_preview', BAD_INPUT: 'error_bad_input', + BROWSER_GENERIC: 'error_browser_generic', + BROWSER_UNSUPPORTED: 'error_browser_unsupported', + CONTENT_DOWNLOAD: 'error_content_download', + CONVERSION_GENERIC: 'error_conversion_generic', + CONVERSION_PASSWORD_PROTECTED: 'error_password_protected', + CONVERSION_TRY_AGAIN_LATER: 'error_try_again_later', + CONVERSION_UNSUPPORTED_FORMAT: 'error_unsupported_format', DELETED_REPS: 'error_deleted_reps', + EXCEEDED_RETRY_LIMIT: 'error_exceeded_retry_limit', + FLASH_NOT_ENABLED: 'error_flash_not_enabled', + GENERIC: 'error_generic', + IMAGE_SIZING: 'error_image_sizing', + INVALID_CACHE_ATTEMPT: 'error_invalid_file_for_cache', LOAD_ANNOTATIONS: 'error_load_annotations', LOAD_ASSET: 'error_load_asset', LOAD_CSV: 'error_load_csv', LOAD_DOCUMENT: 'error_load_document', LOAD_MEDIA: 'error_load_media', LOAD_VIEWER: 'error_load_viewer', - IMAGE_SIZING: 'error_image_sizing', - SHAKA: 'error_shaka', - INVALID_CACHE_ATTEMPT: 'error_invalid_file_for_cache', + NOT_DOWNLOADABLE: 'error_file_not_downloadable', + PARSE_CSV: 'error_parse_csv', + PERMISSIONS_PREVIEW: 'error_permissions_preview', PREFETCH_FILE: 'error_prefetch_file', RATE_LIMIT: 'error_rate_limit', - EXCEEDED_RETRY_LIMIT: 'error_exceeded_retry_limit', - BROWSER_GENERIC: 'error_browser_generic', - BROWSER_UNSUPPORTED: 'error_browser_unsupported', - NOT_DOWNLOADABLE: 'error_file_not_downloadable', - GENERIC: 'error_generic', - CONVERSION_GENERIC: 'error_conversion_generic', - CONVERSION_PASSWORD_PROTECTED: 'error_password_protected', - CONVERSION_TRY_AGAIN_LATER: 'error_try_again_later', - CONVERSION_UNSUPPORTED_FORMAT: 'error_unsupported_format', + SHAKA: 'error_shaka', + UNSUPPORTED_FILE_TYPE: 'error_unsupported_file_type', VIEWER_LOAD_TIMEOUT: 'error_viewer_load_timeout', - CONTENT_DOWNLOAD: 'error_content_download', - FLASH_NOT_ENABLED: 'error_flash_not_enabled', }; // Event fired from Preview with error details diff --git a/src/lib/viewers/text/CSVViewer.js b/src/lib/viewers/text/CSVViewer.js index adae65eac..ce1e1f73a 100644 --- a/src/lib/viewers/text/CSVViewer.js +++ b/src/lib/viewers/text/CSVViewer.js @@ -6,6 +6,11 @@ import { ERROR_CODE, VIEWER_EVENT } from '../../events'; import PreviewError from '../../PreviewError'; const JS = [`third-party/text/${TEXT_STATIC_ASSETS_VERSION}/papaparse.min.js`, 'csv.js']; +const PAPAPARSE_TYPES = { + DELIMITER: 'Delimiter', + FIELD_MISMATCH: 'FieldMismatch', + QUOTES: 'Quotes', +}; class CSVViewer extends TextBaseViewer { /** @@ -68,15 +73,9 @@ class CSVViewer extends TextBaseViewer { return; } - const { errors = [], data } = results; + const { errors, data } = results; - if (errors.length) { - const error = new PreviewError(ERROR_CODE.LOAD_CSV, undefined, { - ...errors[0], - silent: true, - }); - this.triggerError(error); - } + this.checkForParseErrors(errors); this.data = data; this.finishLoading(); @@ -88,6 +87,69 @@ class CSVViewer extends TextBaseViewer { .catch(this.handleAssetError); } + /** + * Checks for parse errors and if present triggers an error silently + * @param {Array} errors Array of errors from PapaParse parse results + * @return {void} + */ + checkForParseErrors(errors = []) { + if (!errors.length) { + return; + } + + const parseError = this.getWorstParseError(errors); + + const error = new PreviewError(ERROR_CODE.PARSE_CSV, undefined, { + ...parseError, + silent: true, + }); + + this.triggerError(error); + } + + /** + * Utility to sort the PapaParse errors by most significant and returning the first. + * The significance is defined as DELIMTER > QUOTES > FIELD_MISMATCH + * @param {Array} errors Array of errors from PapaParse parse results + * @return {Object} returns PapaParse error or undefined + */ + getWorstParseError(errors = []) { + return errors.sort((a, b) => { + const { type: aType } = a; + const { type: bType } = b; + + if (aType === PAPAPARSE_TYPES.DELIMITER) { + if (bType !== PAPAPARSE_TYPES.DELIMITER) { + return -1; + } + + return 0; + } + + if (aType === PAPAPARSE_TYPES.FIELD_MISMATCH) { + if (bType !== PAPAPARSE_TYPES.FIELD_MISMATCH) { + return 1; + } + + return 0; + } + + if (aType === PAPAPARSE_TYPES.QUOTES) { + if (bType === PAPAPARSE_TYPES.DELIMITER) { + return 1; + } + + if (bType === PAPAPARSE_TYPES.FIELD_MISMATCH) { + return -1; + } + + return 0; + } + + return 0; + })[0]; + } + /** * Prefetches assets for CSV Viewer. * diff --git a/src/lib/viewers/text/__tests__/CSVViewer-test.js b/src/lib/viewers/text/__tests__/CSVViewer-test.js index 76b1b122a..ce56d0281 100644 --- a/src/lib/viewers/text/__tests__/CSVViewer-test.js +++ b/src/lib/viewers/text/__tests__/CSVViewer-test.js @@ -202,4 +202,55 @@ describe('lib/viewers/text/CSVViewer', () => { expect(csv.emit).to.be.calledWith(VIEWER_EVENT.load); }); }); + + describe('checkForParseErrors()', () => { + beforeEach(() => { + stubs.getWorstParseError = sandbox.stub(csv, 'getWorstParseError'); + stubs.triggerError = sandbox.stub(csv, 'triggerError'); + }); + + it('should do nothing if no errors', () => { + csv.checkForParseErrors(); + + expect(stubs.triggerError).not.to.be.called; + }); + + it('should trigger error with a parse error', () => { + stubs.getWorstParseError.returns({ foo: 'bar' }); + + csv.checkForParseErrors([{ foo: 'bar' }]); + + expect(stubs.triggerError).to.be.called; + }); + }); + + describe('getWorstParseError()', () => { + const delimiterError = { type: 'Delimiter' }; + const fieldsMismatchError = { type: 'FieldMismatch', id: 1 }; + const fieldsMismatchError2 = { type: 'FieldMismatch', id: 2 }; + const quotesError = { type: 'Quotes' }; + + [ + { name: 'should return undefined if empty array', errors: [], expectedError: undefined }, + { + name: 'should return delimiter type if present', + errors: [fieldsMismatchError, delimiterError, quotesError], + expectedError: delimiterError, + }, + { + name: 'should return quotes type if no delimiter type in array', + errors: [fieldsMismatchError, quotesError], + expectedError: quotesError, + }, + { + name: 'should return fields mismatch type if no other type present', + errors: [fieldsMismatchError, fieldsMismatchError2], + expectedError: fieldsMismatchError, + }, + ].forEach(({ name, errors, expectedError }) => { + it(`${name}`, () => { + expect(csv.getWorstParseError(errors)).to.be.eql(expectedError); + }); + }); + }); }); From 772f9af9f440687fe0a634d072bc284b86edb747 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Fri, 30 Aug 2019 10:40:39 -0700 Subject: [PATCH 3/5] chore: change to PAPAPARSE_ERROR_TYPES --- src/lib/viewers/text/CSVViewer.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/lib/viewers/text/CSVViewer.js b/src/lib/viewers/text/CSVViewer.js index ce1e1f73a..057f53e04 100644 --- a/src/lib/viewers/text/CSVViewer.js +++ b/src/lib/viewers/text/CSVViewer.js @@ -6,7 +6,7 @@ import { ERROR_CODE, VIEWER_EVENT } from '../../events'; import PreviewError from '../../PreviewError'; const JS = [`third-party/text/${TEXT_STATIC_ASSETS_VERSION}/papaparse.min.js`, 'csv.js']; -const PAPAPARSE_TYPES = { +const PAPAPARSE_ERROR_TYPES = { DELIMITER: 'Delimiter', FIELD_MISMATCH: 'FieldMismatch', QUOTES: 'Quotes', @@ -118,28 +118,28 @@ class CSVViewer extends TextBaseViewer { const { type: aType } = a; const { type: bType } = b; - if (aType === PAPAPARSE_TYPES.DELIMITER) { - if (bType !== PAPAPARSE_TYPES.DELIMITER) { + if (aType === PAPAPARSE_ERROR_TYPES.DELIMITER) { + if (bType !== PAPAPARSE_ERROR_TYPES.DELIMITER) { return -1; } return 0; } - if (aType === PAPAPARSE_TYPES.FIELD_MISMATCH) { - if (bType !== PAPAPARSE_TYPES.FIELD_MISMATCH) { + if (aType === PAPAPARSE_ERROR_TYPES.FIELD_MISMATCH) { + if (bType !== PAPAPARSE_ERROR_TYPES.FIELD_MISMATCH) { return 1; } return 0; } - if (aType === PAPAPARSE_TYPES.QUOTES) { - if (bType === PAPAPARSE_TYPES.DELIMITER) { + if (aType === PAPAPARSE_ERROR_TYPES.QUOTES) { + if (bType === PAPAPARSE_ERROR_TYPES.DELIMITER) { return 1; } - if (bType === PAPAPARSE_TYPES.FIELD_MISMATCH) { + if (bType === PAPAPARSE_ERROR_TYPES.FIELD_MISMATCH) { return -1; } From 1972bf1094e1f901048f5ea147d20eb6439e22a2 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Fri, 30 Aug 2019 11:08:15 -0700 Subject: [PATCH 4/5] chore: using priority map --- src/lib/viewers/text/CSVViewer.js | 50 +++++++------------------------ 1 file changed, 10 insertions(+), 40 deletions(-) diff --git a/src/lib/viewers/text/CSVViewer.js b/src/lib/viewers/text/CSVViewer.js index 057f53e04..1f91eab7e 100644 --- a/src/lib/viewers/text/CSVViewer.js +++ b/src/lib/viewers/text/CSVViewer.js @@ -11,6 +11,11 @@ const PAPAPARSE_ERROR_TYPES = { FIELD_MISMATCH: 'FieldMismatch', QUOTES: 'Quotes', }; +const ERROR_PRIORITY = { + [PAPAPARSE_ERROR_TYPES.DELIMITER]: 0, + [PAPAPARSE_ERROR_TYPES.QUOTES]: 1, + [PAPAPARSE_ERROR_TYPES.FIELD_MISMATCH]: 2, +}; class CSVViewer extends TextBaseViewer { /** @@ -73,11 +78,9 @@ class CSVViewer extends TextBaseViewer { return; } - const { errors, data } = results; - - this.checkForParseErrors(errors); + this.checkForParseErrors(results); - this.data = data; + this.data = results.data; this.finishLoading(); URL.revokeObjectURL(workerSrc); }, @@ -89,10 +92,10 @@ class CSVViewer extends TextBaseViewer { /** * Checks for parse errors and if present triggers an error silently - * @param {Array} errors Array of errors from PapaParse parse results + * @param {Array} results.errors Papaparse results errors array * @return {void} */ - checkForParseErrors(errors = []) { + checkForParseErrors({ errors = [] }) { if (!errors.length) { return; } @@ -114,40 +117,7 @@ class CSVViewer extends TextBaseViewer { * @return {Object} returns PapaParse error or undefined */ getWorstParseError(errors = []) { - return errors.sort((a, b) => { - const { type: aType } = a; - const { type: bType } = b; - - if (aType === PAPAPARSE_ERROR_TYPES.DELIMITER) { - if (bType !== PAPAPARSE_ERROR_TYPES.DELIMITER) { - return -1; - } - - return 0; - } - - if (aType === PAPAPARSE_ERROR_TYPES.FIELD_MISMATCH) { - if (bType !== PAPAPARSE_ERROR_TYPES.FIELD_MISMATCH) { - return 1; - } - - return 0; - } - - if (aType === PAPAPARSE_ERROR_TYPES.QUOTES) { - if (bType === PAPAPARSE_ERROR_TYPES.DELIMITER) { - return 1; - } - - if (bType === PAPAPARSE_ERROR_TYPES.FIELD_MISMATCH) { - return -1; - } - - return 0; - } - - return 0; - })[0]; + return errors.sort((a, b) => ERROR_PRIORITY[a.type] - ERROR_PRIORITY[b.type])[0]; } /** From f0b0d5580b1a4439d1e74188eb38158f82cd6bfd Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Fri, 30 Aug 2019 11:24:43 -0700 Subject: [PATCH 5/5] chore: fix CSVViewer tests --- src/lib/viewers/text/CSVViewer.js | 2 +- src/lib/viewers/text/__tests__/CSVViewer-test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/viewers/text/CSVViewer.js b/src/lib/viewers/text/CSVViewer.js index 1f91eab7e..f67b7aed2 100644 --- a/src/lib/viewers/text/CSVViewer.js +++ b/src/lib/viewers/text/CSVViewer.js @@ -95,7 +95,7 @@ class CSVViewer extends TextBaseViewer { * @param {Array} results.errors Papaparse results errors array * @return {void} */ - checkForParseErrors({ errors = [] }) { + checkForParseErrors({ errors = [] } = {}) { if (!errors.length) { return; } diff --git a/src/lib/viewers/text/__tests__/CSVViewer-test.js b/src/lib/viewers/text/__tests__/CSVViewer-test.js index ce56d0281..27914b54b 100644 --- a/src/lib/viewers/text/__tests__/CSVViewer-test.js +++ b/src/lib/viewers/text/__tests__/CSVViewer-test.js @@ -218,7 +218,7 @@ describe('lib/viewers/text/CSVViewer', () => { it('should trigger error with a parse error', () => { stubs.getWorstParseError.returns({ foo: 'bar' }); - csv.checkForParseErrors([{ foo: 'bar' }]); + csv.checkForParseErrors({ errors: [{ foo: 'bar' }] }); expect(stubs.triggerError).to.be.called; });