Skip to content

Commit

Permalink
Support better K8s Resource Name editing (#3167)
Browse files Browse the repository at this point in the history
* Support better K8s Resource Name editing

* Remove AutoFocus hook; use native react autofocus

* Align to existing infrastructure around annotations

* Support AXE better

* Add Cypress page object for the component

* Improve CodeCov for new components

* Drop autoTrimmed based on UX feedback & update tests to match new design

* Support read-only mode & improve updating data

* Handle special character cases

* Address leading dashes & safeK8sPrefix clash

* Improved test examples for extreme edge cases & documentation of flags

* Handle additional edge cases
  • Loading branch information
andrewballantyne committed Sep 16, 2024
1 parent 101545e commit 7964daa
Show file tree
Hide file tree
Showing 27 changed files with 1,046 additions and 193 deletions.
25 changes: 25 additions & 0 deletions frontend/src/__mocks__/mockK8sNameDescriptionFieldData.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import * as _ from 'lodash-es';
import { RecursivePartial } from '~/typeHelpers';
import { K8sNameDescriptionFieldData } from '~/concepts/k8s/K8sNameDescriptionField/types';

export const mockK8sNameDescriptionFieldData = (
overrides: RecursivePartial<K8sNameDescriptionFieldData> = {},
): K8sNameDescriptionFieldData =>
_.merge(
{},
{
name: '',
description: '',
k8sName: {
value: '',
state: {
immutable: false,
invalidLength: false,
invalidCharacters: false,
maxLength: 253,
touched: false,
},
},
},
overrides,
);
4 changes: 2 additions & 2 deletions frontend/src/__mocks__/mockStartNotebookData.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { ImageStreamKind } from '~/k8sTypes';
import { StartNotebookData } from '~/pages/projects/types';
import { mockK8sNameDescriptionFieldData } from '~/__mocks__/mockK8sNameDescriptionFieldData';

type MockResourceConfigType = {
volumeName?: string;
Expand All @@ -8,8 +9,7 @@ export const mockStartNotebookData = ({
volumeName = 'test-volume',
}: MockResourceConfigType): StartNotebookData => ({
projectName: 'test-project',
notebookName: 'test-notebook',
description: '',
notebookData: mockK8sNameDescriptionFieldData({ name: 'test-notebook', description: '' }),
image: {
imageStream: {
metadata: {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { SubComponentBase } from '~/__tests__/cypress/cypress/pages/components/subComponents/SubComponentBase';

export class K8sNameDescriptionField extends SubComponentBase {
constructor(private compTestId: string, scopedBaseTestId?: string) {
super(scopedBaseTestId);
}

private findByScopedTestId(suffix: string): Cypress.Chainable<JQuery<HTMLElement>> {
return this.findScope().findByTestId(`${this.compTestId}-${suffix}`);
}

findDisplayNameInput(): Cypress.Chainable<JQuery<HTMLElement>> {
return this.findByScopedTestId('name');
}

findDescriptionInput(): Cypress.Chainable<JQuery<HTMLElement>> {
return this.findByScopedTestId('description');
}

findResourceEditLink(): Cypress.Chainable<JQuery<HTMLElement>> {
return this.findByScopedTestId('editResourceLink');
}

findResourceNameInput(): Cypress.Chainable<JQuery<HTMLElement>> {
return this.findByScopedTestId('resourceName');
}
}
15 changes: 3 additions & 12 deletions frontend/src/__tests__/cypress/cypress/pages/projects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Modal } from '~/__tests__/cypress/cypress/pages/components/Modal';
import { appChrome } from '~/__tests__/cypress/cypress/pages/appChrome';
import { DeleteModal } from '~/__tests__/cypress/cypress/pages/components/DeleteModal';
import { Contextual } from '~/__tests__/cypress/cypress/pages/components/Contextual';
import { K8sNameDescriptionField } from '~/__tests__/cypress/cypress/pages/components/subComponents/K8sNameDescriptionField';
import { TableRow } from './components/table';

class ProjectListToolbar extends Contextual<HTMLElement> {
Expand Down Expand Up @@ -129,22 +130,12 @@ class ProjectListPage {
}

class CreateEditProjectModal extends Modal {
k8sNameDescription = new K8sNameDescriptionField('manage-project-modal');

constructor(private edit = false) {
super(`${edit ? 'Edit' : 'Create'} project`);
}

findNameInput() {
return this.find().findByTestId('manage-project-modal-name');
}

findResourceNameInput() {
return this.find().findByTestId('resource-manage-project-modal-name');
}

findDescriptionInput() {
return this.find().findByTestId('manage-project-modal-description');
}

findSubmitButton() {
return this.findFooter().findByRole('button', { name: this.edit ? /Edit/ : /Create/ });
}
Expand Down
11 changes: 3 additions & 8 deletions frontend/src/__tests__/cypress/cypress/pages/workbench.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { K8sNameDescriptionField } from '~/__tests__/cypress/cypress/pages/components/subComponents/K8sNameDescriptionField';
import { Contextual } from './components/Contextual';
import { Modal } from './components/Modal';
import { TableRow } from './components/table';
Expand Down Expand Up @@ -162,6 +163,8 @@ class NotebookRow extends TableRow {
}

class CreateSpawnerPage {
k8sNameDescription = new K8sNameDescriptionField('workbench');

shouldHaveAppTitle() {
cy.findByTestId('app-page-title').should('have.text', 'Create workbench');
return this;
Expand Down Expand Up @@ -219,14 +222,6 @@ class CreateSpawnerPage {
return cy.findByTestId('submit-button');
}

findNameInput() {
return cy.findByTestId('workbench-name');
}

findDescriptionInput() {
return cy.findByTestId('workbench-description');
}

getEnvironmentVariableTypeField(index: number) {
return new EnvironmentVariableTypeField(() =>
cy.findAllByTestId('environment-variable-field').eq(index),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,19 @@ describe('Data Science Projects', { testIsolation: false }, () => {
});

it('should create project', () => {
createProjectModal.findNameInput().type('My Test Project');
createProjectModal.findDescriptionInput().type('Test project description.');
createProjectModal.k8sNameDescription.findDisplayNameInput().type('My Test Project');
createProjectModal.k8sNameDescription.findDescriptionInput().type('Test project description.');
createProjectModal.findSubmitButton().should('be.enabled');
createProjectModal.findResourceNameInput().should('have.value', 'my-test-project').clear();
createProjectModal.findResourceNameInput().should('have.attr', 'aria-invalid', 'true');
createProjectModal.k8sNameDescription.findResourceEditLink().click();
createProjectModal.k8sNameDescription
.findResourceNameInput()
.should('have.value', 'my-test-project')
.clear();
createProjectModal.k8sNameDescription
.findResourceNameInput()
.should('have.attr', 'aria-invalid', 'true');
createProjectModal.findSubmitButton().should('be.disabled');
createProjectModal.findResourceNameInput().type('test-project');
createProjectModal.k8sNameDescription.findResourceNameInput().type('test-project');

cy.interceptSnapshot('/api/k8s/apis/project.openshift.io/v1/projects', 'projects-1');
cy.interceptSnapshot('/api/namespaces/test-project/0', 'update-project');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,43 @@ describe('Data science projects details', () => {
createProjectModal.shouldBeOpen();
createProjectModal.findSubmitButton().should('be.disabled');

createProjectModal.findNameInput().type('My Test Project');
createProjectModal.findDescriptionInput().type('Test project description.');
// Standard items pass
createProjectModal.k8sNameDescription.findDisplayNameInput().type('My Test Project');
createProjectModal.k8sNameDescription.findDescriptionInput().type('Test project description.');
createProjectModal.findSubmitButton().should('be.enabled');
createProjectModal.findResourceNameInput().should('have.value', 'my-test-project').clear();
createProjectModal.findResourceNameInput().should('have.attr', 'aria-invalid', 'true');
// Really long display names pass
createProjectModal.k8sNameDescription
.findDisplayNameInput()
.clear()
.type(
'This is a really long display name that will cause the Kubernetes name to exceed its max length and auto trim',
);
createProjectModal.findSubmitButton().should('be.enabled');
createProjectModal.k8sNameDescription.findResourceEditLink().click();
createProjectModal.k8sNameDescription
.findResourceNameInput()
.should('have.attr', 'aria-invalid', 'false');
createProjectModal.k8sNameDescription
.findResourceNameInput()
.should('have.value', 'this-is-a-really-long-display');
// Invalid character k8s names fail
createProjectModal.k8sNameDescription.findResourceNameInput().clear().type('InVaLiD vAlUe!');
createProjectModal.k8sNameDescription
.findResourceNameInput()
.should('have.attr', 'aria-invalid', 'true');
createProjectModal.findSubmitButton().should('be.disabled');
createProjectModal.findResourceNameInput().type('test-project');

// Invalid length k8s names fail
createProjectModal.k8sNameDescription
.findResourceNameInput()
.clear()
.type('this-is-a-valid-character-string-but-it-is-too-long');
createProjectModal.k8sNameDescription
.findResourceNameInput()
.should('have.attr', 'aria-invalid', 'true');
createProjectModal.findSubmitButton().should('be.disabled');
// Valid k8s names succeed
createProjectModal.k8sNameDescription.findResourceNameInput().clear().type('test-project');
createProjectModal.findSubmitButton().click();

cy.wsK8s('ADDED', ProjectModel, mockProjectK8sResource({}));

cy.url().should('include', '/projects/test-project');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,8 @@ describe('Workbench page', () => {
workbenchPage.findCreateButton().click();
createSpawnerPage.findSubmitButton().should('be.disabled');
verifyRelativeURL('/projects/test-project/spawner');
createSpawnerPage.findNameInput().fill('test-project');
createSpawnerPage.findDescriptionInput().fill('test-description');
createSpawnerPage.k8sNameDescription.findDisplayNameInput().fill('test-project');
createSpawnerPage.k8sNameDescription.findDescriptionInput().fill('test-description');
//to check scrollable dropdown selection
createSpawnerPage.findNotebookImage('test-9').click();
createSpawnerPage.selectContainerSize(
Expand Down Expand Up @@ -353,8 +353,8 @@ describe('Workbench page', () => {
workbenchPage.findCreateButton().click();
createSpawnerPage.findSubmitButton().should('be.disabled');
verifyRelativeURL('/projects/test-project/spawner');
createSpawnerPage.findNameInput().fill('1234');
createSpawnerPage.findDescriptionInput().fill('test-description');
createSpawnerPage.k8sNameDescription.findDisplayNameInput().fill('1234');
createSpawnerPage.k8sNameDescription.findDescriptionInput().fill('test-description');
//to check scrollable dropdown selection
createSpawnerPage.findNotebookImage('test-9').click();
createSpawnerPage.selectContainerSize(
Expand Down Expand Up @@ -547,12 +547,12 @@ describe('Workbench page', () => {
mockK8sResourceList([mockPVCK8sResource({ name: 'test-notebook' })]),
);
editSpawnerPage.visit('test-notebook');
editSpawnerPage.findNameInput().should('have.value', 'Test Notebook');
editSpawnerPage.k8sNameDescription.findDisplayNameInput().should('have.value', 'Test Notebook');
editSpawnerPage.shouldHaveNotebookImageSelectInput('Test Image');
editSpawnerPage.shouldHaveContainerSizeInput('Small');
editSpawnerPage.shouldHavePersistentStorage('Test Storage');
editSpawnerPage.findSubmitButton().should('be.enabled');
editSpawnerPage.findNameInput().fill('Updated Notebook');
editSpawnerPage.k8sNameDescription.findDisplayNameInput().fill('Updated Notebook');

cy.interceptK8s('PUT', NotebookModel, mockNotebookK8sResource({})).as('editWorkbench');
editSpawnerPage.findSubmitButton().click();
Expand Down
12 changes: 6 additions & 6 deletions frontend/src/api/k8s/notebooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { K8sAPIOptions, KnownLabels, NotebookKind } from '~/k8sTypes';
import { usernameTranslate } from '~/utilities/notebookControllerUtils';
import { EnvironmentFromVariable, StartNotebookData } from '~/pages/projects/types';
import { ROOT_MOUNT_PATH } from '~/pages/projects/pvc/const';
import { translateDisplayNameForK8s } from '~/concepts/k8s/utils';
import { getTolerationPatch, TolerationChanges } from '~/utilities/tolerations';
import { applyK8sAPIOptions } from '~/api/apiMergeUtils';
import {
Expand All @@ -36,9 +35,7 @@ export const assembleNotebook = (
): NotebookKind => {
const {
projectName,
notebookName,
notebookId: overrideNotebookId,
description,
notebookData,
notebookSize,
envFrom,
initialAcceleratorProfile,
Expand All @@ -50,7 +47,11 @@ export const assembleNotebook = (
existingTolerations,
existingResources,
} = data;
const notebookId = overrideNotebookId || translateDisplayNameForK8s(notebookName, 'wb-');
const {
name: notebookName,
description,
k8sName: { value: notebookId },
} = notebookData;
const imageUrl = `${image.imageStream?.status?.dockerImageRepository}:${image.imageVersion?.name}`;
const imageSelection = `${image.imageStream?.metadata.name}:${image.imageVersion?.name}`;

Expand Down Expand Up @@ -277,7 +278,6 @@ export const updateNotebook = (
username: string,
opts?: K8sAPIOptions,
): Promise<NotebookKind> => {
assignableData.notebookId = existingNotebook.metadata.name;
const notebook = assembleNotebook(assignableData, username);

const oldNotebook = structuredClone(existingNotebook);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import * as React from 'react';
import { HelperTextItem } from '@patternfly/react-core';
import { K8sNameDescriptionFieldData } from '~/concepts/k8s/K8sNameDescriptionField/types';

type Variants = React.ComponentProps<typeof HelperTextItem>['variant'];

type HelperTextItemType = React.FC<{
k8sName: K8sNameDescriptionFieldData['k8sName'];
}>;

export const HelperTextItemMaxLength: HelperTextItemType = ({ k8sName }) => {
let variant: Variants = 'indeterminate';
if (k8sName.state.invalidLength) {
variant = 'error';
} else if (k8sName.value.trim().length > 0) {
variant = 'success';
}

return (
<HelperTextItem variant={variant} hasIcon>
Cannot exceed {k8sName.state.maxLength} characters
</HelperTextItem>
);
};

export const HelperTextItemValidCharacters: HelperTextItemType = ({ k8sName }) => {
let variant: Variants = 'indeterminate';
if (k8sName.state.invalidCharacters) {
variant = 'error';
} else if (k8sName.value.trim().length > 0) {
variant = 'success';
}

return (
<HelperTextItem variant={variant} hasIcon>
Must start and end with a letter or number. Valid characters include lowercase letters,
numbers, and hyphens (-).
</HelperTextItem>
);
};
Loading

0 comments on commit 7964daa

Please sign in to comment.