From efdf559552275f1540a772e00b48f7aee832c59c Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Mon, 15 Jul 2024 17:00:51 -0400 Subject: [PATCH 1/3] add new text-only output message --- .../appCell2/widgets/appCellWidget.js | 6 +-- .../appCell2/widgets/tabs/resultsViewer.js | 36 ++++++++++++++-- .../widgets/tabs/resultsViewer-spec.js | 41 ++++++++++++++++++- 3 files changed, 74 insertions(+), 9 deletions(-) diff --git a/nbextensions/appCell2/widgets/appCellWidget.js b/nbextensions/appCell2/widgets/appCellWidget.js index faa3e88849..30a6a3aaa5 100644 --- a/nbextensions/appCell2/widgets/appCellWidget.js +++ b/nbextensions/appCell2/widgets/appCellWidget.js @@ -76,8 +76,6 @@ define( AppParamsWidget, AppParamsViewWidget ) => { - 'use strict'; - const t = html.tag, div = t('div'), span = t('span'), @@ -1301,7 +1299,8 @@ define( skip the output cell creation. */ // widgets named 'no-display' are a trigger to skip the output cell process. - const skipOutputCell = model.getItem('exec.outputWidgetInfo.name') === 'no-display'; + const widgetName = model.getItem('exec.outputWidgetInfo.name'); + const skipOutputCell = widgetName === 'no-display' || widgetName === 'text-only'; let cellInfo; if (skipOutputCell) { cellInfo = { @@ -1834,7 +1833,6 @@ define( }; }, (err) => { - 'use strict'; console.error('ERROR loading appCell appCellWidget', err); } ); diff --git a/nbextensions/appCell2/widgets/tabs/resultsViewer.js b/nbextensions/appCell2/widgets/tabs/resultsViewer.js index ad9dd5cc9c..9b870d593b 100644 --- a/nbextensions/appCell2/widgets/tabs/resultsViewer.js +++ b/nbextensions/appCell2/widgets/tabs/resultsViewer.js @@ -7,9 +7,20 @@ define([ 'kb_service/client/narrativeMethodStore', 'common/html', 'util/display', + 'util/string', 'kbaseReportView', -], (Promise, $, UI, Runtime, Events, NarrativeMethodStore, html, DisplayUtil, KBaseReportView) => { - 'use strict'; +], ( + Promise, + $, + UI, + Runtime, + Events, + NarrativeMethodStore, + html, + DisplayUtil, + StringUtil, + KBaseReportView +) => { const t = html.tag, div = t('div'), a = t('a'), @@ -90,17 +101,36 @@ define([ // the job output if (!reportInputs) { return Promise.try(() => { + const viewerName = result ? result.name : null; const jobOutput = jobState.job_output ? jobState.job_output.result : 'no output found'; + if (viewerName === 'text-only') { + ui.setContent('results.body', buildOutputText(result.params.result_text)); + } else { + ui.setContent('results.body', ui.buildPresentableJson(jobOutput)); + } ui.getElement('results').classList.remove('hidden'); - ui.setContent('results.body', ui.buildPresentableJson(jobOutput)); }); } // otherwise, render the report return renderReportView(reportInputs); } + function buildOutputText(resultText) { + // default if text is empty, null, or undefined + if (resultText === null || resultText === undefined || resultText === '') { + resultText = 'App completed successfully.'; + } + // cap the results at 1,000 characters. + if (resultText.length > 1000) { + resultText = + resultText.substring(0, 1000) + + ` [truncated from ${resultText.length} characters]`; + } + return div(StringUtil.escape(resultText)); + } + function lazyRenderReport() { return Promise.try(() => { const nbContainer = document.querySelector('#notebook-container'); diff --git a/test/unit/spec/nbextensions/appCell2/widgets/tabs/resultsViewer-spec.js b/test/unit/spec/nbextensions/appCell2/widgets/tabs/resultsViewer-spec.js index 26b3cd0377..bf9ee848b4 100644 --- a/test/unit/spec/nbextensions/appCell2/widgets/tabs/resultsViewer-spec.js +++ b/test/unit/spec/nbextensions/appCell2/widgets/tabs/resultsViewer-spec.js @@ -6,8 +6,6 @@ define([ 'testUtil', 'narrativeMocks', ], (ResultsViewer, Props, Jupyter, Config, TestUtil, Mocks) => { - 'use strict'; - const mockModelData = { exec: {}, app: { @@ -23,6 +21,7 @@ define([ }; const mockOutputWidgetInfo = { + name: 'no-display', params: { report_name: 'some_report', report_ref: '1/2/3', @@ -57,6 +56,32 @@ define([ return Props.make({ data }); } + /** + * Builds and returns a mock data model (Props object) and final job state. + * @param {int} textLength + * @returns + */ + function buildTextModel(textLength) { + const data = TestUtil.JSONcopy(mockModelData); + const resultText = 'A'.repeat(textLength); + const outputInfo = { + name: 'text-only', + params: { + result_text: resultText, + }, + }; + const state = { + job_output: { + result: [outputInfo], + }, + }; + data.exec.outputWidgetInfo = outputInfo; + return { + model: Props.make({ data }), + state, + }; + } + describe('App Cell Results Viewer tests', () => { let node, outerNode; @@ -123,6 +148,18 @@ define([ } }); + it('starts a viewer with text output', async () => { + const { model, state } = buildTextModel(10); + const viewer = ResultsViewer.make({ model }); + await viewer.start({ + node, + jobState: state, + isParentJob: false, + }); + expect(node.querySelector('.kb-app-results-tab')).toBeDefined(); + expect(node.innerHTML).toContain('A'.repeat(10)); + }); + it('starts and stops a viewer with a report from a batch job', async () => { const model = buildModel({ hasReport: true, isParentJob: true, jobComplete: true }); const viewer = ResultsViewer.make({ model }); From e4d6f63647ff460d317fa4d191f4a464c502b801 Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Mon, 15 Jul 2024 17:03:53 -0400 Subject: [PATCH 2/3] make truncated text viewer test --- .../appCell2/widgets/tabs/resultsViewer-spec.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/unit/spec/nbextensions/appCell2/widgets/tabs/resultsViewer-spec.js b/test/unit/spec/nbextensions/appCell2/widgets/tabs/resultsViewer-spec.js index bf9ee848b4..17aa261225 100644 --- a/test/unit/spec/nbextensions/appCell2/widgets/tabs/resultsViewer-spec.js +++ b/test/unit/spec/nbextensions/appCell2/widgets/tabs/resultsViewer-spec.js @@ -160,6 +160,21 @@ define([ expect(node.innerHTML).toContain('A'.repeat(10)); }); + it('starts a viewer with truncated output', async () => { + const textLength = 10000; + const maxLen = 1000; + const { model, state } = buildTextModel(textLength); + const viewer = ResultsViewer.make({ model }); + await viewer.start({ + node, + jobState: state, + isParentJob: false, + }); + expect(node.querySelector('.kb-app-results-tab')).toBeDefined(); + expect(node.innerHTML).toContain('A'.repeat(maxLen)); + expect(node.innerHTML).toContain(`[truncated from ${textLength} characters]`); + }); + it('starts and stops a viewer with a report from a batch job', async () => { const model = buildModel({ hasReport: true, isParentJob: true, jobComplete: true }); const viewer = ResultsViewer.make({ model }); From 34cda5e51013810681a7e7974ed30f44cd757875 Mon Sep 17 00:00:00 2001 From: Bill Riehl Date: Mon, 15 Jul 2024 21:33:45 -0400 Subject: [PATCH 3/3] add checks for any non-string or non-number to make the default message. --- .../appCell2/widgets/tabs/resultsViewer.js | 15 +++++- .../widgets/tabs/resultsViewer-spec.js | 51 +++++++++++++++---- 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/nbextensions/appCell2/widgets/tabs/resultsViewer.js b/nbextensions/appCell2/widgets/tabs/resultsViewer.js index 9b870d593b..830232e6c8 100644 --- a/nbextensions/appCell2/widgets/tabs/resultsViewer.js +++ b/nbextensions/appCell2/widgets/tabs/resultsViewer.js @@ -117,11 +117,24 @@ define([ return renderReportView(reportInputs); } + /** + * Expects that the result text should be a string, but handles other cases as well. + * If resultText is not a string or number, or is an empty string, this sets the text + * to "App completed successfully." Otherwise, it truncates the result to 1000 characters. + * + * This then gets HTML-escaped and embedded in a div for returning. + * @param {str} resultText + * @returns + */ function buildOutputText(resultText) { // default if text is empty, null, or undefined - if (resultText === null || resultText === undefined || resultText === '') { + if ( + !(typeof resultText === 'string' && resultText.trim().length > 0) && + typeof resultText !== 'number' + ) { resultText = 'App completed successfully.'; } + resultText = String(resultText).trim(); // cap the results at 1,000 characters. if (resultText.length > 1000) { resultText = diff --git a/test/unit/spec/nbextensions/appCell2/widgets/tabs/resultsViewer-spec.js b/test/unit/spec/nbextensions/appCell2/widgets/tabs/resultsViewer-spec.js index 17aa261225..cbec54d2da 100644 --- a/test/unit/spec/nbextensions/appCell2/widgets/tabs/resultsViewer-spec.js +++ b/test/unit/spec/nbextensions/appCell2/widgets/tabs/resultsViewer-spec.js @@ -58,19 +58,18 @@ define([ /** * Builds and returns a mock data model (Props object) and final job state. - * @param {int} textLength + * @param {string} text - if present, this gets used instead of a random string * @returns */ - function buildTextModel(textLength) { + function buildTextModel(resultText) { const data = TestUtil.JSONcopy(mockModelData); - const resultText = 'A'.repeat(textLength); const outputInfo = { name: 'text-only', params: { result_text: resultText, }, }; - const state = { + const jobState = { job_output: { result: [outputInfo], }, @@ -78,7 +77,7 @@ define([ data.exec.outputWidgetInfo = outputInfo; return { model: Props.make({ data }), - state, + jobState, }; } @@ -149,32 +148,62 @@ define([ }); it('starts a viewer with text output', async () => { - const { model, state } = buildTextModel(10); + const resultText = 'ABCDEFGHIJKL'; + const { model, jobState } = buildTextModel(resultText); const viewer = ResultsViewer.make({ model }); await viewer.start({ node, - jobState: state, + jobState, isParentJob: false, }); expect(node.querySelector('.kb-app-results-tab')).toBeDefined(); - expect(node.innerHTML).toContain('A'.repeat(10)); + expect(node.innerHTML).toContain(resultText); }); it('starts a viewer with truncated output', async () => { const textLength = 10000; + const resultText = 'A'.repeat(textLength); const maxLen = 1000; - const { model, state } = buildTextModel(textLength); + const { model, jobState } = buildTextModel(resultText); const viewer = ResultsViewer.make({ model }); await viewer.start({ node, - jobState: state, + jobState, isParentJob: false, }); expect(node.querySelector('.kb-app-results-tab')).toBeDefined(); - expect(node.innerHTML).toContain('A'.repeat(maxLen)); + expect(node.innerHTML).toContain(resultText.substring(0, maxLen)); expect(node.innerHTML).toContain(`[truncated from ${textLength} characters]`); }); + const defaultCases = [null, undefined, {}, [], '', ' ']; + defaultCases.forEach((testCase) => + it(`creates a text viewer with default success for input ${testCase}`, async () => { + const { model, jobState } = buildTextModel(testCase); + const viewer = ResultsViewer.make({ model }); + await viewer.start({ + node, + jobState, + isParentJob: false, + }); + expect(node.querySelector('.kb-app-results-tab')).toBeDefined(); + expect(node.innerHTML).toContain('App completed successfully.'); + }) + ); + + it('creates a text viewer that escapes html', async () => { + const resultText = ''; + const { model, jobState } = buildTextModel(resultText); + const viewer = ResultsViewer.make({ model }); + await viewer.start({ + node, + jobState, + isParentJob: false, + }); + expect(node.querySelector('.kb-app-results-tab')).toBeDefined(); + expect(node.innerHTML).toContain('<script>alert'); + }); + it('starts and stops a viewer with a report from a batch job', async () => { const model = buildModel({ hasReport: true, isParentJob: true, jobComplete: true }); const viewer = ResultsViewer.make({ model });