[UI] Cleanup, remove types from urls in artifact/execution details pa…

…ge (#3715) * Remove types from urls in artifact/execution details page * Remove unused route params * Fix snapshot tests
kubeflow · May 8, 2020 · cb41f7a · cb41f7a
1 parent c5ceaf5
commit cb41f7a
Show file tree

Hide file tree

Showing 8 changed files with 43 additions and 59 deletions.
diff --git a/frontend/src/components/Router.tsx b/frontend/src/components/Router.tsx
@@ -79,8 +79,6 @@ export enum RouteParams {
   pipelineId = 'pid',
   pipelineVersionId = 'vid',
   runId = 'rid',
-  ARTIFACT_TYPE = 'artifactType',
-  EXECUTION_TYPE = 'executionType',
   // TODO: create one of these for artifact and execution?
   ID = 'id',
 }
@@ -97,10 +95,10 @@ export const RoutePage = {
   ARCHIVED_RUNS: '/archive/runs',
   ARCHIVED_EXPERIMENTS: '/archive/experiments',
   ARTIFACTS: '/artifacts',
-  ARTIFACT_DETAILS: `/artifact_types/:${RouteParams.ARTIFACT_TYPE}+/artifacts/:${RouteParams.ID}`,
+  ARTIFACT_DETAILS: `/artifacts/:${RouteParams.ID}`,
   COMPARE: `/compare`,
   EXECUTIONS: '/executions',
-  EXECUTION_DETAILS: `/execution_types/:${RouteParams.EXECUTION_TYPE}+/executions/:${RouteParams.ID}`,
+  EXECUTION_DETAILS: `/executions/:${RouteParams.ID}`,
   EXPERIMENTS: '/experiments',
   EXPERIMENT_DETAILS: `/experiments/details/:${RouteParams.experimentId}`,
   NEW_EXPERIMENT: '/experiments/new',
@@ -116,17 +114,11 @@ export const RoutePage = {
 };
 
 export const RoutePageFactory = {
-  artifactDetails: (artifactType: string, artifactId: number) => {
-    return RoutePage.ARTIFACT_DETAILS.replace(
-      `:${RouteParams.ARTIFACT_TYPE}+`,
-      artifactType,
-    ).replace(`:${RouteParams.ID}`, '' + artifactId);
+  artifactDetails: (artifactId: number) => {
+    return RoutePage.ARTIFACT_DETAILS.replace(`:${RouteParams.ID}`, '' + artifactId);
   },
-  executionDetails: (executionType: string, executionId: number) => {
-    return RoutePage.EXECUTION_DETAILS.replace(
-      `:${RouteParams.EXECUTION_TYPE}+`,
-      executionType,
-    ).replace(`:${RouteParams.ID}`, '' + executionId);
+  executionDetails: (executionId: number) => {
+    return RoutePage.EXECUTION_DETAILS.replace(`:${RouteParams.ID}`, '' + executionId);
   },
   pipelineDetails: (id: string) => {
     return RoutePage.PIPELINE_DETAILS_NO_VERSION.replace(`:${RouteParams.pipelineId}`, id);

diff --git a/frontend/src/components/__snapshots__/Router.test.tsx.snap b/frontend/src/components/__snapshots__/Router.test.tsx.snap
@@ -35,7 +35,7 @@ exports[`Router initial render 1`] = `
     <Route
       exact={false}
       key="4"
-      path="/artifact_types/:artifactType+/artifacts/:id"
+      path="/artifacts/:id"
       render={[Function]}
     />
     <Route
@@ -47,7 +47,7 @@ exports[`Router initial render 1`] = `
     <Route
       exact={true}
       key="6"
-      path="/execution_types/:executionType+/executions/:id"
+      path="/executions/:id"
       render={[Function]}
     />
     <Route

diff --git a/frontend/src/lib/MlmdUtils.ts b/frontend/src/lib/MlmdUtils.ts
@@ -83,9 +83,7 @@ const EXECUTION_PROPERTY_REPOS = [ExecutionProperties, ExecutionCustomProperties
 export const ExecutionHelpers = {
   getWorkspace(execution: Execution): string | number | undefined {
     return (
-      getResourcePropertyViaFallBack(execution, EXECUTION_PROPERTY_REPOS, [
-        ExecutionProperties.RUN_ID,
-      ]) ||
+      getResourcePropertyViaFallBack(execution, EXECUTION_PROPERTY_REPOS, ['RUN_ID']) ||
       getResourceProperty(execution, ExecutionCustomProperties.WORKSPACE, true) ||
       getResourceProperty(execution, ExecutionProperties.PIPELINE_NAME) ||
       undefined

diff --git a/frontend/src/pages/ArtifactDetails.tsx b/frontend/src/pages/ArtifactDetails.tsx
@@ -24,6 +24,7 @@ import {
   LineageResource,
   LineageView,
   titleCase,
+  ArtifactType,
 } from '@kubeflow/frontend';
 import { CircularProgress } from '@material-ui/core';
 import * as React from 'react';
@@ -36,6 +37,7 @@ import { ToolbarProps } from '../components/Toolbar';
 import { commonCss, padding } from '../Css';
 import { logger, serviceErrorToString } from '../lib/Utils';
 import { Page, PageProps } from './Page';
+import { GetArtifactTypesByIDRequest } from '@kubeflow/frontend/src/mlmd/generated/ml_metadata/proto/metadata_store_service_pb';
 
 export enum ArtifactDetailsTab {
   OVERVIEW = 0,
@@ -55,11 +57,12 @@ const TAB_NAMES = [ArtifactDetailsTab.OVERVIEW, ArtifactDetailsTab.LINEAGE_EXPLO
 
 interface ArtifactDetailsState {
   artifact?: Artifact;
+  artifactType?: ArtifactType;
 }
 
 class ArtifactDetails extends Page<{}, ArtifactDetailsState> {
   private get fullTypeName(): string {
-    return this.props.match.params[RouteParams.ARTIFACT_TYPE] || '';
+    return this.state.artifactType?.getName() || '';
   }
 
   private get properTypeName(): string {
@@ -76,15 +79,16 @@ class ArtifactDetails extends Page<{}, ArtifactDetailsState> {
 
   private static buildResourceDetailsPageRoute(
     resource: LineageResource,
-    typeName: string,
+    _: string, // typename is no longer used
   ): string {
-    let route;
-    if (resource instanceof Artifact) {
-      route = RoutePageFactory.artifactDetails(typeName, resource.getId());
+    // HACK: this distinguishes artifact from execution, only artifacts have
+    // the getUri() method.
+    // TODO: switch to use typedResource
+    if (typeof resource['getUri'] === 'function') {
+      return RoutePageFactory.artifactDetails(resource.getId());
     } else {
-      route = RoutePage.EXECUTION_DETAILS.replace(`:${RouteParams.EXECUTION_TYPE}+`, typeName);
+      return RoutePageFactory.executionDetails(resource.getId());
     }
-    return route.replace(`:${RouteParams.ID}`, String(resource.getId()));
   }
 
   public state: ArtifactDetailsState = {};
@@ -152,7 +156,7 @@ class ArtifactDetails extends Page<{}, ArtifactDetailsState> {
     return {
       actions: {},
       breadcrumbs: [{ displayName: 'Artifacts', href: RoutePage.ARTIFACTS }],
-      pageTitle: `${this.properTypeName} ${this.id} details`,
+      pageTitle: `Artifact #${this.id} details`,
     };
   }
 
@@ -166,18 +170,19 @@ class ArtifactDetails extends Page<{}, ArtifactDetailsState> {
 
     try {
       const response = await this.api.metadataStoreService.getArtifactsByID(request);
-
       if (response.getArtifactsList().length === 0) {
-        this.showPageError(`No ${this.fullTypeName} identified by id: ${this.id}`);
+        this.showPageError(`No artifact identified by id: ${this.id}`);
         return;
       }
-
       if (response.getArtifactsList().length > 1) {
         this.showPageError(`Found multiple artifacts with ID: ${this.id}`);
         return;
       }
-
       const artifact = response.getArtifactsList()[0];
+      const typeRequest = new GetArtifactTypesByIDRequest();
+      typeRequest.setTypeIdsList([artifact.getTypeId()]);
+      const typeResponse = await this.api.metadataStoreService.getArtifactTypesByID(typeRequest);
+      const artifactType = typeResponse.getArtifactTypesList()[0] || undefined;
 
       const artifactName =
         getResourceProperty(artifact, ArtifactProperties.NAME) ||
@@ -190,7 +195,7 @@ class ArtifactDetails extends Page<{}, ArtifactDetailsState> {
       this.props.updateToolbar({
         pageTitle: title,
       });
-      this.setState({ artifact });
+      this.setState({ artifact, artifactType });
     } catch (err) {
       this.showPageError(serviceErrorToString(err));
     }

diff --git a/frontend/src/pages/ArtifactList.tsx b/frontend/src/pages/ArtifactList.tsx
@@ -156,12 +156,11 @@ class ArtifactList extends Page<{}, ArtifactListState> {
   private nameCustomRenderer: React.FC<CustomRendererProps<string>> = (
     props: CustomRendererProps<string>,
   ) => {
-    const [artifactType, artifactId] = props.id.split(':');
     return (
       <Link
         onClick={e => e.stopPropagation()}
         className={commonCss.link}
-        to={RoutePageFactory.artifactDetails(artifactType, Number(artifactId))}
+        to={RoutePageFactory.artifactDetails(Number(props.id))}
       >
         {props.value}
       </Link>
@@ -221,7 +220,7 @@ class ArtifactList extends Page<{}, ArtifactListState> {
             const artifactType = this.artifactTypesMap!.get(typeId);
             const type = artifactType ? artifactType.getName() : artifact.getTypeId();
             return {
-              id: `${type}:${artifact.getId()}`, // Join with colon so we can build the link
+              id: `${artifact.getId()}`,
               otherFields: [
                 getResourcePropertyViaFallBack(
                   artifact,

diff --git a/frontend/src/pages/ExecutionDetails.tsx b/frontend/src/pages/ExecutionDetails.tsx
@@ -102,15 +102,7 @@ export class ExecutionDetailsContent extends Component<
   public state: ExecutionDetailsState = {};
 
   private get fullTypeName(): string {
-    // This can be called during constructor, so this.state may not be initialized.
-    if (this.state) {
-      const { executionType } = this.state;
-      const name = executionType?.getName();
-      if (name) {
-        return name;
-      }
-    }
-    return '';
+    return this.state.executionType?.getName() || '';
   }
 
   public async componentDidMount(): Promise<void> {
@@ -159,7 +151,7 @@ export class ExecutionDetailsContent extends Component<
     return {
       actions: {},
       breadcrumbs: [{ displayName: 'Executions', href: RoutePage.EXECUTIONS }],
-      pageTitle: `${this.fullTypeName} ${this.props.id} details`,
+      pageTitle: `Execution #${this.props.id} details`,
     };
   }
 
@@ -201,7 +193,7 @@ export class ExecutionDetailsContent extends Component<
 
       if (!executionResponse.getExecutionsList().length) {
         this.props.onError(
-          `No ${this.fullTypeName} identified by id: ${this.props.id}`,
+          `No execution identified by id: ${this.props.id}`,
           undefined,
           'error',
           this.refresh,
@@ -398,17 +390,17 @@ const ArtifactRow: React.FC<{ id: number; name: string; type?: string; uri: stri
 }) => (
   <tr>
     <td className={css.tableCell}>
-      {type && id ? (
-        <Link className={commonCss.link} to={RoutePageFactory.artifactDetails(type, id)}>
+      {id ? (
+        <Link className={commonCss.link} to={RoutePageFactory.artifactDetails(id)}>
           {id}
         </Link>
       ) : (
         id
       )}
     </td>
     <td className={css.tableCell}>
-      {type && id ? (
-        <Link className={commonCss.link} to={RoutePageFactory.artifactDetails(type, id)}>
+      {id ? (
+        <Link className={commonCss.link} to={RoutePageFactory.artifactDetails(id)}>
           {name}
         </Link>
       ) : (

diff --git a/frontend/src/pages/ExecutionList.tsx b/frontend/src/pages/ExecutionList.tsx
@@ -42,7 +42,7 @@ import {
   CollapsedAndExpandedRows,
   serviceErrorToString,
 } from '../lib/Utils';
-import { RoutePage, RouteParams } from '../components/Router';
+import { RoutePageFactory } from '../components/Router';
 import { ExecutionHelpers } from 'src/lib/MlmdUtils';
 
 interface ExecutionListState {
@@ -182,13 +182,12 @@ class ExecutionList extends Page<{}, ExecutionListState> {
   private nameCustomRenderer: React.FC<CustomRendererProps<string>> = (
     props: CustomRendererProps<string>,
   ) => {
-    const [executionType, executionId] = props.id.split(':');
-    const link = RoutePage.EXECUTION_DETAILS.replace(
-      `:${RouteParams.EXECUTION_TYPE}+`,
-      executionType,
-    ).replace(`:${RouteParams.ID}`, executionId);
     return (
-      <Link onClick={e => e.stopPropagation()} className={commonCss.link} to={link}>
+      <Link
+        onClick={e => e.stopPropagation()}
+        className={commonCss.link}
+        to={RoutePageFactory.executionDetails(Number(props.id))}
+      >
         {props.value}
       </Link>
     );
@@ -212,7 +211,7 @@ class ExecutionList extends Page<{}, ExecutionListState> {
           const executionType = this.executionTypesMap!.get(execution.getTypeId());
           const type = executionType ? executionType.getName() : execution.getTypeId();
           return {
-            id: `${type}:${execution.getId()}`, // Join with colon so we can build the link
+            id: `${execution.getId()}`,
             otherFields: [
               ExecutionHelpers.getWorkspace(execution),
               ExecutionHelpers.getName(execution),

diff --git a/frontend/src/pages/RunDetails.tsx b/frontend/src/pages/RunDetails.tsx
@@ -385,7 +385,6 @@ class RunDetails extends Page<RunDetailsInternalProps, RunDetailsState> {
                                           <Link
                                             className={commonCss.link}
                                             to={RoutePageFactory.executionDetails(
-                                              selectedExecution.getTypeId() + '',
                                               selectedExecution.getId(),
                                             )}
                                           >