From d7915b084d0509c7195fdc3b2b579c0376b4076a Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Mon, 23 Oct 2023 10:35:39 +1100 Subject: [PATCH] Warn user before trying to add a stage to their pipeline --- extension/src/experiments/webview/messages.ts | 2 +- extension/src/pipeline/index.ts | 80 +++++++++++-------- .../src/test/suite/experiments/index.test.ts | 4 +- extension/src/test/suite/experiments/util.ts | 2 + .../src/test/suite/pipeline/index.test.ts | 38 ++++++++- extension/src/test/suite/pipeline/util.ts | 8 ++ 6 files changed, 95 insertions(+), 39 deletions(-) diff --git a/extension/src/experiments/webview/messages.ts b/extension/src/experiments/webview/messages.ts index 58abade962..86b2a4cad5 100644 --- a/extension/src/experiments/webview/messages.ts +++ b/extension/src/experiments/webview/messages.ts @@ -377,7 +377,7 @@ export class WebviewMessages { } private addConfiguration() { - return this.pipeline.checkOrAddPipeline() + return this.pipeline.addPipeline() } private async setMaxTableHeadDepth() { diff --git a/extension/src/pipeline/index.ts b/extension/src/pipeline/index.ts index ae019ad728..ece82cc470 100644 --- a/extension/src/pipeline/index.ts +++ b/extension/src/pipeline/index.ts @@ -17,6 +17,8 @@ import { getInput, getValidInput } from '../vscode/inputBox' import { Title } from '../vscode/title' import { quickPickOne, quickPickOneOrInput } from '../vscode/quickPick' import { pickFile } from '../vscode/resourcePicker' +import { Response } from '../vscode/response' +import { Modal } from '../vscode/modal' export enum ScriptCommand { JUPYTER = 'jupyter nbconvert --to notebook --inplace --execute', @@ -105,13 +107,56 @@ export class Pipeline extends DeferredDisposable { ) } - public checkOrAddPipeline() { + public async checkOrAddPipeline() { if (this.model.hasPipeline()) { return } + + const response = await Modal.showInformation( + 'To continue you must add some configuration. Would you like to add a stage now?', + Response.YES + ) + + if (response !== Response.YES) { + return + } + return this.addPipeline() } + public async addPipeline() { + const stageName = await this.askForStageName() + if (!stageName) { + return + } + + const { trainingScript, command, enteredManually } = + await this.askForTrainingScript() + if (!trainingScript) { + return + } + + const dataUpdated = new Promise(resolve => { + const listener = this.dispose.track( + this.data.onDidUpdate(() => { + resolve(undefined) + this.dispose.untrack(listener) + listener.dispose() + }) + ) + }) + + void findOrCreateDvcYamlFile( + this.dvcRoot, + trainingScript, + stageName, + command, + !enteredManually + ) + void this.data.managedUpdate() + return dataUpdated + } + public async addDataSeriesPlot() { const cwd = await this.getCwd() @@ -149,39 +194,6 @@ export class Pipeline extends DeferredDisposable { return this.deferred.resolve() } - private async addPipeline() { - const stageName = await this.askForStageName() - if (!stageName) { - return - } - - const { trainingScript, command, enteredManually } = - await this.askForTrainingScript() - if (!trainingScript) { - return - } - - const dataUpdated = new Promise(resolve => { - const listener = this.dispose.track( - this.data.onDidUpdate(() => { - resolve(undefined) - this.dispose.untrack(listener) - listener.dispose() - }) - ) - }) - - void findOrCreateDvcYamlFile( - this.dvcRoot, - trainingScript, - stageName, - command, - !enteredManually - ) - void this.data.managedUpdate() - return dataUpdated - } - private async askForStageName() { return await getValidInput( Title.ENTER_STAGE_NAME, diff --git a/extension/src/test/suite/experiments/index.test.ts b/extension/src/test/suite/experiments/index.test.ts index b4e3da1ad4..163f378d90 100644 --- a/extension/src/test/suite/experiments/index.test.ts +++ b/extension/src/test/suite/experiments/index.test.ts @@ -1523,14 +1523,14 @@ suite('Experiments Test Suite', () => { }).timeout(WEBVIEW_TEST_TIMEOUT) it('should handle a message to add a configuration', async () => { - const { mockMessageReceived, mockCheckOrAddPipeline } = + const { mockMessageReceived, mockAddPipeline } = await buildExperimentsWebview({ disposer: disposable }) mockMessageReceived.fire({ type: MessageFromWebviewType.ADD_CONFIGURATION }) - expect(mockCheckOrAddPipeline).to.be.calledOnce + expect(mockAddPipeline).to.be.calledOnce }).timeout(WEBVIEW_TEST_TIMEOUT) it('should handle a message to show more commits', async () => { diff --git a/extension/src/test/suite/experiments/util.ts b/extension/src/test/suite/experiments/util.ts index 1b3ed1c617..a330d40274 100644 --- a/extension/src/test/suite/experiments/util.ts +++ b/extension/src/test/suite/experiments/util.ts @@ -86,6 +86,7 @@ export const buildExperiments = ({ internalCommands }) const mockCheckOrAddPipeline = stub(pipeline, 'checkOrAddPipeline') + const mockAddPipeline = stub(pipeline, 'addPipeline') const mockSelectBranches = stub().resolves(['main', 'other']) const mockMemento = buildMockMemento({ [`${PersistenceKey.EXPERIMENTS_BRANCHES}${dvcRoot}`]: ['main'], @@ -130,6 +131,7 @@ export const buildExperiments = ({ experimentsModel: (experiments as any).experiments as ExperimentsModel, gitReader, internalCommands, + mockAddPipeline, mockCheckOrAddPipeline, mockCheckSignalFile, mockExpShow, diff --git a/extension/src/test/suite/pipeline/index.test.ts b/extension/src/test/suite/pipeline/index.test.ts index 116164668a..82613e1ef3 100644 --- a/extension/src/test/suite/pipeline/index.test.ts +++ b/extension/src/test/suite/pipeline/index.test.ts @@ -30,19 +30,53 @@ suite('Pipeline Test Suite', () => { }) describe('Pipeline', () => { + it('should not show a modal when addPipeline is called', async () => { + const mockInputBox = stub(window, 'showInputBox').resolves(undefined) + const { mockShowInformation, pipeline } = buildPipeline({ + disposer: disposable, + dvcYamls: [] + }) + await pipeline.isReady() + await pipeline.addPipeline() + expect(mockShowInformation).not.to.have.been.calledOnce + expect(mockInputBox).to.have.been.calledOnce + }) + it('should ask to create a stage and not return a cwd if there is no pipeline', async () => { const mockInputBox = stub(window, 'showInputBox').resolves(undefined) - const { pipeline } = buildPipeline({ + const { mockShowInformation, pipeline } = buildPipeline({ disposer: disposable, dvcYamls: [] }) await pipeline.isReady() const cwd = await pipeline.getCwd() - expect(mockInputBox, 'the user should be prompted to add a pipeline').to + expect( + mockShowInformation, + 'the user should be asked whether or not they want to add a stage' + ).to.have.been.calledOnce + expect(mockInputBox, 'the user should be prompted for a stage name').to .have.been.calledOnce expect(cwd, 'no cwd is returned').to.be.undefined }) + it('should not create a stage if the user does not want to add one', async () => { + const mockInputBox = stub(window, 'showInputBox').resolves(undefined) + const { mockShowInformation, pipeline } = buildPipeline({ + disposer: disposable, + dvcYamls: [] + }) + mockShowInformation.resetBehavior() + mockShowInformation.resolves(undefined) + await pipeline.isReady() + const cwd = await pipeline.getCwd() + expect( + mockShowInformation, + 'the user should be asked whether or not they want to add a stage' + ).to + expect(mockInputBox).not.to.have.been.called + expect(cwd, 'no cwd is returned').to.be.undefined + }) + it('should return a cwd if there is an invalid pipeline (let command fail)', async () => { const { pipeline } = buildPipeline({ disposer: disposable, diff --git a/extension/src/test/suite/pipeline/util.ts b/extension/src/test/suite/pipeline/util.ts index b83a73e326..3f9c0ff438 100644 --- a/extension/src/test/suite/pipeline/util.ts +++ b/extension/src/test/suite/pipeline/util.ts @@ -6,6 +6,8 @@ import { Pipeline } from '../../../pipeline' import { PipelineData } from '../../../pipeline/data' import { dvcDemoPath } from '../../util' import { buildDependencies } from '../util' +import { Response } from '../../../vscode/response' +import { Modal } from '../../../vscode/modal' export const buildExperimentsPipeline = ({ disposer, @@ -56,9 +58,15 @@ export const buildPipeline = ({ dvcYamls, internalCommands }) + + const mockShowInformation = stub(Modal, 'showInformation').resolves( + Response.YES + ) + return { dvcReader, internalCommands, + mockShowInformation, pipeline, // eslint-disable-next-line @typescript-eslint/no-explicit-any pipelineData: (pipeline as any).data as PipelineData