Fix missing run and pipeline id when buttons are clicked before conte…

…nt load (#2584) * Fix missing run and pipeline id when buttons are clicked before content load * Fix lint error * Fix expect statement
kubeflow · Nov 12, 2019 · 918430c · 918430c
1 parent e7f52fb
commit 918430c
Show file tree

Hide file tree

Showing 5 changed files with 203 additions and 10 deletions.
diff --git a/frontend/mock-backend/mock-api-middleware.ts b/frontend/mock-backend/mock-api-middleware.ts
@@ -175,7 +175,7 @@ export default (app: express.Application) => {
   app.post(v1beta1Prefix + '/experiments', (req, res) => {
     const experiment: ApiExperiment = req.body;
     if (fixedData.experiments.find(e => e.name!.toLowerCase() === experiment.name!.toLowerCase())) {
-      res.status(404).send('An experiment with teh same name already exists');
+      res.status(404).send('An experiment with the same name already exists');
       return;
     }
     experiment.id = 'new-experiment-' + (fixedData.experiments.length + 1);

diff --git a/frontend/src/pages/PipelineDetails.test.tsx b/frontend/src/pages/PipelineDetails.test.tsx
@@ -416,7 +416,7 @@ describe('PipelineDetails', () => {
     );
   });
 
-  it('clicking new run button when viewing half-loaded page navigates to the new run page with run ID', async () => {
+  it('clicking new run button when viewing half-loaded page navigates to the new run page with pipeline ID', async () => {
     tree = shallow(<PipelineDetails {...generateProps(false)} />);
     // Intentionally don't wait until all network requests finish.
     const instance = tree.instance() as PipelineDetails;
@@ -443,6 +443,18 @@ describe('PipelineDetails', () => {
     );
   });
 
+  it('clicking new experiment button when viewing half-loaded page navigates to the new experiment page with the pipeline ID', async () => {
+    tree = shallow(<PipelineDetails {...generateProps()} />);
+    // Intentionally don't wait until all network requests finish.
+    const instance = tree.instance() as PipelineDetails;
+    const newExperimentBtn = instance.getInitialToolbarState().actions[ButtonKeys.NEW_EXPERIMENT];
+    await newExperimentBtn.action();
+    expect(historyPushSpy).toHaveBeenCalledTimes(1);
+    expect(historyPushSpy).toHaveBeenLastCalledWith(
+      RoutePage.NEW_EXPERIMENT + `?${QUERY_PARAMS.pipelineId}=${testPipeline.id}`,
+    );
+  });
+
   it('has a delete button', async () => {
     tree = shallow(<PipelineDetails {...generateProps()} />);
     await getTemplateSpy;
@@ -452,7 +464,7 @@ describe('PipelineDetails', () => {
     expect(deleteBtn).toBeDefined();
   });
 
-  it('shows delete confirmation dialog when delete buttin is clicked', async () => {
+  it('shows delete confirmation dialog when delete button is clicked', async () => {
     tree = shallow(<PipelineDetails {...generateProps()} />);
     const deleteBtn = (tree.instance() as PipelineDetails).getInitialToolbarState().actions[
       ButtonKeys.DELETE_RUN
@@ -493,6 +505,20 @@ describe('PipelineDetails', () => {
     expect(deletePipelineSpy).toHaveBeenLastCalledWith(testPipeline.id);
   });
 
+  it('calls delete API when delete dialog is confirmed and page is half-loaded', async () => {
+    tree = shallow(<PipelineDetails {...generateProps()} />);
+    // Intentionally don't wait until all network requests finish.
+    const deleteBtn = (tree.instance() as PipelineDetails).getInitialToolbarState().actions[
+      ButtonKeys.DELETE_RUN
+    ];
+    await deleteBtn!.action();
+    const call = updateDialogSpy.mock.calls[0][0];
+    const confirmBtn = call.buttons.find((b: any) => b.text === 'Delete');
+    await confirmBtn.onClick();
+    expect(deletePipelineSpy).toHaveBeenCalledTimes(1);
+    expect(deletePipelineSpy).toHaveBeenLastCalledWith(testPipeline.id);
+  });
+
   it('shows error dialog if deletion fails', async () => {
     tree = shallow(<PipelineDetails {...generateProps()} />);
     TestUtils.makeErrorResponseOnce(deletePipelineSpy, 'woops');

diff --git a/frontend/src/pages/PipelineDetails.tsx b/frontend/src/pages/PipelineDetails.tsx
@@ -118,8 +118,8 @@ class PipelineDetails extends Page<{}, PipelineDetailsState> {
   public getInitialToolbarState(): ToolbarProps {
     const buttons = new Buttons(this.props, this.refresh.bind(this));
     const fromRunId = new URLParser(this.props).get(QUERY_PARAMS.fromRunId);
+    const pipelineIdFromParams = this.props.match.params[RouteParams.pipelineId];
     buttons.newRunFromPipeline(() => {
-      const pipelineIdFromParams = this.props.match.params[RouteParams.pipelineId];
       return this.state.pipeline
         ? this.state.pipeline.id!
         : pipelineIdFromParams
@@ -141,9 +141,20 @@ class PipelineDetails extends Page<{}, PipelineDetailsState> {
     } else {
       // Add buttons for creating experiment and deleting pipeline
       buttons
-        .newExperiment(() => (this.state.pipeline ? this.state.pipeline.id! : ''))
+        .newExperiment(() =>
+          this.state.pipeline
+            ? this.state.pipeline.id!
+            : pipelineIdFromParams
+            ? pipelineIdFromParams
+            : '',
+        )
         .delete(
-          () => (this.state.pipeline ? [this.state.pipeline.id!] : []),
+          () =>
+            this.state.pipeline
+              ? [this.state.pipeline.id!]
+              : pipelineIdFromParams
+              ? [pipelineIdFromParams]
+              : [],
           'pipeline',
           this._deleteCallback.bind(this),
           true /* useCurrentResource */,

diff --git a/frontend/src/pages/RunDetails.test.tsx b/frontend/src/pages/RunDetails.test.tsx
@@ -45,6 +45,8 @@ describe('RunDetails', () => {
   const pathsParser = jest.spyOn(WorkflowParser, 'loadNodeOutputPaths');
   const pathsWithStepsParser = jest.spyOn(WorkflowParser, 'loadAllOutputPathsWithStepNames');
   const loaderSpy = jest.spyOn(OutputArtifactLoader, 'load');
+  const retryRunSpy = jest.spyOn(Apis.runServiceApi, 'retryRun');
+  const terminateRunSpy = jest.spyOn(Apis.runServiceApi, 'terminateRun');
   // We mock this because it uses toLocaleDateString, which causes mismatches between local and CI
   // test environments
   const formatDateStringSpy = jest.spyOn(Utils, 'formatDateString');
@@ -143,6 +145,139 @@ describe('RunDetails', () => {
     );
   });
 
+  it('clicking the clone button when the page is half-loaded navigates to new run page with run id', async () => {
+    tree = shallow(<RunDetails {...generateProps()} />);
+    // Intentionally don't wait until all network requests finish.
+    const instance = tree.instance() as RunDetails;
+    const cloneBtn = instance.getInitialToolbarState().actions[ButtonKeys.CLONE_RUN];
+    expect(cloneBtn).toBeDefined();
+    await cloneBtn!.action();
+    expect(historyPushSpy).toHaveBeenCalledTimes(1);
+    expect(historyPushSpy).toHaveBeenLastCalledWith(
+      RoutePage.NEW_RUN + `?${QUERY_PARAMS.cloneFromRun}=${testRun.run!.id}`,
+    );
+  });
+
+  it('has a retry button', async () => {
+    tree = shallow(<RunDetails {...generateProps()} />);
+    await getRunSpy;
+    await TestUtils.flushPromises();
+    const instance = tree.instance() as RunDetails;
+    const retryBtn = instance.getInitialToolbarState().actions[ButtonKeys.RETRY];
+    expect(retryBtn).toBeDefined();
+  });
+
+  it('shows retry confirmation dialog when retry button is clicked', async () => {
+    tree = shallow(<RunDetails {...generateProps()} />);
+    const instance = tree.instance() as RunDetails;
+    const retryBtn = instance.getInitialToolbarState().actions[ButtonKeys.RETRY];
+    await retryBtn!.action();
+    expect(updateDialogSpy).toHaveBeenCalledTimes(1);
+    expect(updateDialogSpy).toHaveBeenLastCalledWith(
+      expect.objectContaining({
+        title: 'Retry this run?',
+      }),
+    );
+  });
+
+  it('does not call retry API for selected run when retry dialog is canceled', async () => {
+    tree = shallow(<RunDetails {...generateProps()} />);
+    const instance = tree.instance() as RunDetails;
+    const retryBtn = instance.getInitialToolbarState().actions[ButtonKeys.RETRY];
+    await retryBtn!.action();
+    const call = updateDialogSpy.mock.calls[0][0];
+    const cancelBtn = call.buttons.find((b: any) => b.text === 'Cancel');
+    await cancelBtn.onClick();
+    expect(retryRunSpy).not.toHaveBeenCalled();
+  });
+
+  it('calls retry API when retry dialog is confirmed', async () => {
+    tree = shallow(<RunDetails {...generateProps()} />);
+    await getRunSpy;
+    await TestUtils.flushPromises();
+    const instance = tree.instance() as RunDetails;
+    const retryBtn = instance.getInitialToolbarState().actions[ButtonKeys.RETRY];
+    await retryBtn!.action();
+    const call = updateDialogSpy.mock.calls[0][0];
+    const confirmBtn = call.buttons.find((b: any) => b.text === 'Retry');
+    await confirmBtn.onClick();
+    expect(retryRunSpy).toHaveBeenCalledTimes(1);
+    expect(retryRunSpy).toHaveBeenLastCalledWith(testRun.run!.id);
+  });
+
+  it('calls retry API when retry dialog is confirmed and page is half-loaded', async () => {
+    tree = shallow(<RunDetails {...generateProps()} />);
+    // Intentionally don't wait until all network requests finish.
+    const instance = tree.instance() as RunDetails;
+    const retryBtn = instance.getInitialToolbarState().actions[ButtonKeys.RETRY];
+    await retryBtn!.action();
+    const call = updateDialogSpy.mock.calls[0][0];
+    const confirmBtn = call.buttons.find((b: any) => b.text === 'Retry');
+    await confirmBtn.onClick();
+    expect(retryRunSpy).toHaveBeenCalledTimes(1);
+    expect(retryRunSpy).toHaveBeenLastCalledWith(testRun.run!.id);
+  });
+
+  it('has a terminate button', async () => {
+    tree = shallow(<RunDetails {...generateProps()} />);
+    await getRunSpy;
+    await TestUtils.flushPromises();
+    const instance = tree.instance() as RunDetails;
+    const terminateBtn = instance.getInitialToolbarState().actions[ButtonKeys.TERMINATE_RUN];
+    expect(terminateBtn).toBeDefined();
+  });
+
+  it('shows terminate confirmation dialog when terminate button is clicked', async () => {
+    tree = shallow(<RunDetails {...generateProps()} />);
+    const instance = tree.instance() as RunDetails;
+    const terminateBtn = instance.getInitialToolbarState().actions[ButtonKeys.TERMINATE_RUN];
+    await terminateBtn!.action();
+    expect(updateDialogSpy).toHaveBeenCalledTimes(1);
+    expect(updateDialogSpy).toHaveBeenLastCalledWith(
+      expect.objectContaining({
+        title: 'Terminate this run?',
+      }),
+    );
+  });
+
+  it('does not call terminate API for selected run when terminate dialog is canceled', async () => {
+    tree = shallow(<RunDetails {...generateProps()} />);
+    const instance = tree.instance() as RunDetails;
+    const terminateBtn = instance.getInitialToolbarState().actions[ButtonKeys.TERMINATE_RUN];
+    await terminateBtn!.action();
+    const call = updateDialogSpy.mock.calls[0][0];
+    const cancelBtn = call.buttons.find((b: any) => b.text === 'Cancel');
+    await cancelBtn.onClick();
+    expect(terminateRunSpy).not.toHaveBeenCalled();
+  });
+
+  it('calls terminate API when terminate dialog is confirmed', async () => {
+    tree = shallow(<RunDetails {...generateProps()} />);
+    await getRunSpy;
+    await TestUtils.flushPromises();
+    const instance = tree.instance() as RunDetails;
+    const terminateBtn = instance.getInitialToolbarState().actions[ButtonKeys.TERMINATE_RUN];
+    await terminateBtn!.action();
+    const call = updateDialogSpy.mock.calls[0][0];
+    const confirmBtn = call.buttons.find((b: any) => b.text === 'Terminate');
+    await confirmBtn.onClick();
+    expect(terminateRunSpy).toHaveBeenCalledTimes(1);
+    expect(terminateRunSpy).toHaveBeenLastCalledWith(testRun.run!.id);
+  });
+
+  it('calls terminate API when terminate dialog is confirmed and page is half-loaded', async () => {
+    tree = shallow(<RunDetails {...generateProps()} />);
+    // Intentionally don't wait until all network requests finish.
+    const instance = tree.instance() as RunDetails;
+    const terminateBtn = instance.getInitialToolbarState().actions[ButtonKeys.TERMINATE_RUN];
+    await terminateBtn!.action();
+    const call = updateDialogSpy.mock.calls[0][0];
+    const confirmBtn = call.buttons.find((b: any) => b.text === 'Terminate');
+    await confirmBtn.onClick();
+    expect(terminateRunSpy).toHaveBeenCalledTimes(1);
+    expect(terminateRunSpy).toHaveBeenLastCalledWith(testRun.run!.id);
+  });
+
   it('has an Archive button if the run is not archived', async () => {
     tree = shallow(<RunDetails {...generateProps()} />);
     await getRunSpy;

diff --git a/frontend/src/pages/RunDetails.tsx b/frontend/src/pages/RunDetails.tsx
@@ -168,14 +168,35 @@ class RunDetails extends Page<RunDetailsProps, RunDetailsState> {
 
   public getInitialToolbarState(): ToolbarProps {
     const buttons = new Buttons(this.props, this.refresh.bind(this));
+    const runIdFromParams = this.props.match.params[RouteParams.runId];
     return {
       actions: buttons
-        .retryRun(() => (this.state.runMetadata ? [this.state.runMetadata!.id!] : []), true, () =>
-          this.retry(),
+        .retryRun(
+          () =>
+            this.state.runMetadata
+              ? [this.state.runMetadata!.id!]
+              : runIdFromParams
+              ? [runIdFromParams]
+              : [],
+          true,
+          () => this.retry(),
+        )
+        .cloneRun(
+          () =>
+            this.state.runMetadata
+              ? [this.state.runMetadata!.id!]
+              : runIdFromParams
+              ? [runIdFromParams]
+              : [],
+          true,
         )
-        .cloneRun(() => (this.state.runMetadata ? [this.state.runMetadata!.id!] : []), true)
         .terminateRun(
-          () => (this.state.runMetadata ? [this.state.runMetadata!.id!] : []),
+          () =>
+            this.state.runMetadata
+              ? [this.state.runMetadata!.id!]
+              : runIdFromParams
+              ? [runIdFromParams]
+              : [],
           true,
           () => this.refresh(),
         )