Skip to content

Commit b4e8f42

Browse files
DaoDaoNoCodeDaoDaoNoCode
authored
[RHOAIENG-50148] Add migration guidance for unmigrated workbenches (opendatahub-io#6744)
Show an inline migration-required label and popover for workbenches missing the migration annotation, and cover the behavior with Jest and Cypress tests. Signed-off-by: Juntao Wang <juntwang@redhat.com>
1 parent e0124bd commit b4e8f42

9 files changed

Lines changed: 220 additions & 16 deletions

File tree

‎frontend/src/__mocks__/mockNotebookK8sResource.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ type MockResourceConfigType = {
3232
hardwareProfileName?: string;
3333
hardwareProfileNamespace?: string | null;
3434
workbenchImageNamespace?: string | null;
35+
injectAuth?: string | null;
3536
};
3637

3738
export const mockNotebookK8sResource = ({
@@ -59,6 +60,7 @@ export const mockNotebookK8sResource = ({
5960
hardwareProfileName = '',
6061
hardwareProfileNamespace = null,
6162
workbenchImageNamespace = null,
63+
injectAuth = 'true',
6264
}: MockResourceConfigType): NotebookKind =>
6365
_.merge(
6466
{
@@ -68,7 +70,7 @@ export const mockNotebookK8sResource = ({
6870
annotations: {
6971
'opendatahub.io/image-display-name': imageDisplayName,
7072
'notebooks.kubeflow.org/last-activity': '2023-02-14T21:45:14Z',
71-
'notebooks.opendatahub.io/inject-auth': 'true',
73+
...(injectAuth !== null ? { 'notebooks.opendatahub.io/inject-auth': injectAuth } : {}),
7274
'notebooks.opendatahub.io/last-image-selection': lastImageSelection,
7375
'notebooks.opendatahub.io/last-size-selection': 'Small',
7476
'opendatahub.io/username': user,

‎frontend/src/concepts/notebooks/__tests__/utils.spec.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import { mockNotebookK8sResource } from '#~/__mocks__/mockNotebookK8sResource';
2+
import { isWorkbenchMigrated } from '#~/concepts/notebooks/utils';
3+
4+
describe('isWorkbenchMigrated', () => {
5+
it('should return true when the inject-auth annotation is true', () => {
6+
expect(isWorkbenchMigrated(mockNotebookK8sResource({}))).toBe(true);
7+
});
8+
9+
it('should return false when the inject-auth annotation is missing', () => {
10+
expect(isWorkbenchMigrated(mockNotebookK8sResource({ injectAuth: null }))).toBe(false);
11+
});
12+
13+
it('should return false when the inject-auth annotation is not true', () => {
14+
expect(isWorkbenchMigrated(mockNotebookK8sResource({ injectAuth: 'false' }))).toBe(false);
15+
});
16+
17+
it('should return false when the notebook is undefined', () => {
18+
expect(isWorkbenchMigrated(undefined)).toBe(false);
19+
});
20+
});

‎frontend/src/concepts/notebooks/utils.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ export const getRoutePathForWorkbench = (
1616
workbenchName: string,
1717
): string => `/notebook/${workbenchNamespace}/${workbenchName}`;
1818

19+
export const isWorkbenchMigrated = (
20+
notebook: NotebookKind | Notebook | null | undefined,
21+
): boolean => notebook?.metadata.annotations?.['notebooks.opendatahub.io/inject-auth'] === 'true';
22+
1923
export const getNotebookResourcesPath = (
2024
notebook: NotebookKind | Notebook | null | undefined,
2125
): string => {

‎frontend/src/pages/projects/notebook/NotebookRouteLink.tsx‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {
88
import { NotebookKind } from '#~/k8sTypes';
99
import { getDisplayNameFromK8sResource } from '#~/concepts/k8s/utils';
1010
import { fireMiscTrackingEvent } from '#~/concepts/analyticsTracking/segmentIOUtils';
11+
import { isWorkbenchMigrated } from '#~/concepts/notebooks/utils';
1112
import { useGetNotebookRoute } from '#~/utilities/useGetNotebookRoute';
1213
import { hasStopAnnotation } from './utils';
1314

@@ -37,9 +38,7 @@ const NotebookRouteLink: React.FC<NotebookRouteLinkProps> = ({
3738
const routeLink = useGetNotebookRoute(
3839
canLink ? notebook.metadata.namespace : undefined,
3940
canLink ? notebook.metadata.name : undefined,
40-
canLink
41-
? notebook.metadata.annotations?.['notebooks.opendatahub.io/inject-auth'] === 'true'
42-
: undefined,
41+
canLink ? isWorkbenchMigrated(notebook) : undefined,
4342
);
4443

4544
return (

‎frontend/src/pages/projects/screens/detail/notebooks/NotebookTableRow.tsx‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { NotebookKind } from '#~/k8sTypes';
99
import NotebookImagePackageDetails from '#~/pages/projects/notebook/NotebookImagePackageDetails';
1010
import { ProjectDetailsContext } from '#~/pages/projects/ProjectDetailsContext';
1111
import { TableRowTitleDescription } from '#~/components/table';
12+
import ResourceNameTooltip from '#~/components/ResourceNameTooltip';
1213
import DashboardPopupIconButton from '#~/concepts/dashboard/DashboardPopupIconButton';
1314
import { getDescriptionFromK8sResource } from '#~/concepts/k8s/utils';
1415
import NotebookStateStatus from '#~/pages/projects/notebook/NotebookStateStatus';
@@ -20,7 +21,7 @@ import useStopNotebookModalAvailability from '#~/pages/projects/notebook/useStop
2021
import StopNotebookConfirmModal from '#~/pages/projects/notebook/StopNotebookConfirmModal';
2122
import HardwareProfileTableColumn from '#~/concepts/hardwareProfiles/HardwareProfileTableColumn';
2223
import StateActionToggle from '#~/components/StateActionToggle';
23-
import { useNotebookHardwareProfile } from '#~/concepts/notebooks/utils';
24+
import { isWorkbenchMigrated, useNotebookHardwareProfile } from '#~/concepts/notebooks/utils';
2425
import { UseAssignHardwareProfileResult } from '#~/concepts/hardwareProfiles/useAssignHardwareProfile';
2526
import { useHardwareProfileBindingState } from '#~/concepts/hardwareProfiles/useHardwareProfileBindingState';
2627
import { getDeletedHardwareProfilePatches } from '#~/concepts/hardwareProfiles/utils';
@@ -29,6 +30,7 @@ import { NotebookImageStatus } from './const';
2930
import { NotebookImageDisplayName } from './NotebookImageDisplayName';
3031
import NotebookStorageBars from './NotebookStorageBars';
3132
import NotebookSizeDetails from './NotebookSizeDetails';
33+
import WorkbenchMigrationLabel from './WorkbenchMigrationLabel';
3234
import useNotebookImage from './useNotebookImage';
3335
import NotebookUpdateImageModal from './NotebookUpdateImageModal';
3436

@@ -64,6 +66,7 @@ const NotebookTableRow: React.FC<NotebookTableRowProps> = ({
6466
const { name: notebookName, namespace: notebookNamespace } = obj.notebook.metadata;
6567
const [bindingStateInfo, bindingStateLoaded, bindingStateLoadError] =
6668
useHardwareProfileBindingState(obj.notebook, WORKBENCH_VISIBILITY);
69+
const showMigrationRequired = !isWorkbenchMigrated(obj.notebook);
6770

6871
const onStart = React.useCallback(() => {
6972
setInProgress(true);
@@ -128,16 +131,30 @@ const NotebookTableRow: React.FC<NotebookTableRowProps> = ({
128131
<Td dataLabel="Name">
129132
<TableRowTitleDescription
130133
title={
131-
<NotebookRouteLink
132-
isLarge
133-
notebook={obj.notebook}
134-
isRunning={obj.isRunning}
135-
aria-label="open"
136-
buttonStyle={{ textDecoration: 'none' }}
137-
/>
134+
<Flex
135+
flexWrap={{ default: 'nowrap' }}
136+
spaceItems={{ default: 'spaceItemsXs' }}
137+
alignItems={{ default: 'alignItemsCenter' }}
138+
>
139+
<FlexItem>
140+
<ResourceNameTooltip resource={obj.notebook} wrap={false}>
141+
<NotebookRouteLink
142+
isLarge
143+
notebook={obj.notebook}
144+
isRunning={obj.isRunning}
145+
aria-label="open"
146+
buttonStyle={{ textDecoration: 'none' }}
147+
/>
148+
</ResourceNameTooltip>
149+
</FlexItem>
150+
{showMigrationRequired && (
151+
<FlexItem>
152+
<WorkbenchMigrationLabel />
153+
</FlexItem>
154+
)}
155+
</Flex>
138156
}
139157
description={getDescriptionFromK8sResource(obj.notebook)}
140-
resource={obj.notebook}
141158
/>
142159
</Td>
143160
<Td dataLabel="Workbench image">

‎frontend/src/pages/projects/screens/detail/notebooks/WorkbenchMigrationLabel.tsx‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import * as React from 'react';
2+
import { Label, Popover, Stack, StackItem } from '@patternfly/react-core';
3+
import { ExclamationTriangleIcon } from '@patternfly/react-icons';
4+
5+
const WorkbenchMigrationLabel: React.FC = () => (
6+
<Popover
7+
aria-label="Migration required information"
8+
position="top"
9+
alertSeverityVariant="warning"
10+
headerIcon={<ExclamationTriangleIcon />}
11+
headerContent={
12+
<span data-testid="workbench-migration-required-popover-title">Migration required</span>
13+
}
14+
bodyContent={
15+
<Stack data-testid="workbench-migration-required-popover" hasGutter>
16+
<StackItem>
17+
To prevent access issues, migrate this workbench by editing the workbench description and
18+
saving.
19+
</StackItem>
20+
<StackItem>
21+
Alternatively, delete this workbench and create a new one using the same cluster storage
22+
to preserve user data.
23+
</StackItem>
24+
<StackItem>
25+
Note: Once migrated, the old URL will no longer work. Access the new URL by clicking on
26+
the name link.
27+
</StackItem>
28+
</Stack>
29+
}
30+
>
31+
<Label
32+
data-testid="workbench-migration-required-label"
33+
isCompact
34+
status="warning"
35+
icon={<ExclamationTriangleIcon />}
36+
onClick={() => undefined}
37+
>
38+
Migration required
39+
</Label>
40+
</Popover>
41+
);
42+
43+
export default WorkbenchMigrationLabel;

‎frontend/src/pages/projects/screens/detail/notebooks/__tests__/WorkbenchMigrationLabel.spec.tsx‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import * as React from 'react';
2+
import '@testing-library/jest-dom';
3+
import { render, screen } from '@testing-library/react';
4+
import userEvent from '@testing-library/user-event';
5+
import WorkbenchMigrationLabel from '#~/pages/projects/screens/detail/notebooks/WorkbenchMigrationLabel';
6+
7+
describe('WorkbenchMigrationLabel', () => {
8+
it('should render the migration required label', () => {
9+
render(<WorkbenchMigrationLabel />);
10+
11+
expect(screen.getByTestId('workbench-migration-required-label')).toHaveTextContent(
12+
'Migration required',
13+
);
14+
});
15+
16+
it('should show the migration guidance popover when clicked', async () => {
17+
const user = userEvent.setup();
18+
19+
render(<WorkbenchMigrationLabel />);
20+
21+
await user.click(screen.getByTestId('workbench-migration-required-label'));
22+
23+
expect(screen.getByTestId('workbench-migration-required-popover-title')).toHaveTextContent(
24+
'Migration required',
25+
);
26+
expect(screen.getByTestId('workbench-migration-required-popover')).toHaveTextContent(
27+
'To prevent access issues, migrate this workbench by editing the workbench description and saving.',
28+
);
29+
expect(screen.getByTestId('workbench-migration-required-popover')).toHaveTextContent(
30+
'Alternatively, delete this workbench and create a new one using the same cluster storage to preserve user data.',
31+
);
32+
expect(screen.getByTestId('workbench-migration-required-popover')).toHaveTextContent(
33+
'Note: Once migrated, the old URL will no longer work. Access the new URL by clicking on the name link.',
34+
);
35+
});
36+
});

‎packages/cypress/cypress/pages/workbench.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,18 @@ class NotebookRow extends TableRow {
242242
return this.find().findByTestId('image-display-name').should('contain.text', name);
243243
}
244244

245+
findMigrationRequiredLabel() {
246+
return this.find().findByTestId('workbench-migration-required-label');
247+
}
248+
249+
findMigrationRequiredPopoverTitle() {
250+
return cy.findByTestId('workbench-migration-required-popover-title');
251+
}
252+
253+
findMigrationRequiredPopover() {
254+
return cy.findByTestId('workbench-migration-required-popover');
255+
}
256+
245257
findNotebookImageAvailability() {
246258
return cy.findByTestId('notebook-image-availability');
247259
}

‎packages/cypress/cypress/tests/mocked/projects/tabs/workbench.cy.ts‎

Lines changed: 74 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,12 @@ import { mock200Status, mock404Error } from '@odh-dashboard/internal/__mocks__/m
2424
import { mockConnectionTypeConfigMap } from '@odh-dashboard/internal/__mocks__/mockConnectionType';
2525
import type { HardwareProfileKind, NotebookKind, PodKind } from '@odh-dashboard/internal/k8sTypes';
2626
import { IdentifierResourceType, SchedulingType } from '@odh-dashboard/internal/types';
27-
import type { EnvironmentFromVariable } from '@odh-dashboard/internal/pages/projects/types';
27+
// eslint-disable-next-line @odh-dashboard/no-restricted-imports
2828
import { SpawnerPageSectionID } from '@odh-dashboard/internal/pages/projects/screens/spawner/types';
29-
import { AccessMode } from '@odh-dashboard/internal/pages/storageClasses/storageEnums.ts';
3029
import { DataScienceStackComponent } from '@odh-dashboard/internal/concepts/areas/types';
3130
import { mockWorkloadK8sResource } from '@odh-dashboard/internal/__mocks__/mockWorkloadK8sResource';
3231
import { WorkloadStatusType } from '@odh-dashboard/internal/concepts/distributedWorkloads/utils';
32+
import { AccessMode } from '../../../../types';
3333
import {
3434
ConfigMapModel,
3535
ClusterQueueModel,
@@ -64,10 +64,12 @@ import { hardwareProfileSection } from '../../../../pages/components/HardwarePro
6464

6565
const configYamlPath = './cypress/fixtures/resources/yaml/mock-upload-configmap.yaml';
6666

67+
type MockNotebookConfig = Parameters<typeof mockNotebookK8sResource>[0];
68+
6769
type HandlersProps = {
6870
isEmpty?: boolean;
6971
mockPodList?: PodKind[];
70-
envFrom?: EnvironmentFromVariable[];
72+
envFrom?: MockNotebookConfig['envFrom'];
7173
disableProjectScoped?: boolean;
7274
notebooks?: NotebookKind[];
7375
hardwareProfiles?: {
@@ -1014,6 +1016,75 @@ describe('Workbench page', () => {
10141016
cy.contains('Latest image version');
10151017
});
10161018

1019+
it('Shows migration required label and popover for unmigrated workbenches', () => {
1020+
initIntercepts({
1021+
notebooks: [
1022+
mockNotebookK8sResource({
1023+
name: 'test-notebook',
1024+
displayName: 'Unmigrated Notebook',
1025+
injectAuth: null,
1026+
lastImageSelection: 'test-imagestream:1.2',
1027+
opts: {
1028+
metadata: {
1029+
labels: {
1030+
'opendatahub.io/notebook-image': 'true',
1031+
},
1032+
annotations: {
1033+
'opendatahub.io/image-display-name': 'Test image',
1034+
},
1035+
},
1036+
},
1037+
}),
1038+
mockNotebookK8sResource({
1039+
name: 'migrated-notebook',
1040+
displayName: 'Migrated Notebook',
1041+
lastImageSelection: 'test-imagestream:1.2',
1042+
opts: {
1043+
metadata: {
1044+
labels: {
1045+
'opendatahub.io/notebook-image': 'true',
1046+
},
1047+
annotations: {
1048+
'opendatahub.io/image-display-name': 'Test image',
1049+
},
1050+
},
1051+
},
1052+
}),
1053+
],
1054+
});
1055+
cy.interceptK8sList(
1056+
PVCModel,
1057+
mockK8sResourceList([
1058+
mockPVCK8sResource({ name: 'test-notebook' }),
1059+
mockPVCK8sResource({ name: 'migrated-notebook' }),
1060+
]),
1061+
);
1062+
workbenchPage.visit('test-project');
1063+
1064+
const unmigratedRow = workbenchPage.getNotebookRow('Unmigrated Notebook');
1065+
unmigratedRow.findMigrationRequiredLabel().should('have.text', 'Migration required').click();
1066+
unmigratedRow.findMigrationRequiredPopoverTitle().should('have.text', 'Migration required');
1067+
unmigratedRow
1068+
.findMigrationRequiredPopover()
1069+
.should(
1070+
'contain.text',
1071+
'To prevent access issues, migrate this workbench by editing the workbench description and saving.',
1072+
)
1073+
.and(
1074+
'contain.text',
1075+
'Alternatively, delete this workbench and create a new one using the same cluster storage to preserve user data.',
1076+
)
1077+
.and(
1078+
'contain.text',
1079+
'Note: Once migrated, the old URL will no longer work. Access the new URL by clicking on the name link.',
1080+
);
1081+
1082+
workbenchPage
1083+
.getNotebookRow('Migrated Notebook')
1084+
.findMigrationRequiredLabel()
1085+
.should('not.exist');
1086+
});
1087+
10171088
it('Shows popover with version details', () => {
10181089
initIntercepts({});
10191090
cy.interceptK8sList(

0 commit comments

Comments
 (0)