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

opendatahub-io · Sep 11, 2024 · 8cd1751 · 8cd1751
1 parent 75deae2
commit 8cd1751
Show file tree

Hide file tree

Showing 13 changed files with 88 additions and 80 deletions.
diff --git a/frontend/src/__mocks__/mockK8sNameDescriptionFieldData.ts b/frontend/src/__mocks__/mockK8sNameDescriptionFieldData.ts
@@ -16,7 +16,6 @@ export const mockK8sNameDescriptionFieldData = (
           immutable: false,
           invalidLength: false,
           invalidCharacters: false,
-          autoTrimmed: false,
           maxLength: 253,
           touched: false,
         },

diff --git a/...ges/components/K8sNameDescriptionField.ts → .../subComponents/K8sNameDescriptionField.ts b/...ges/components/K8sNameDescriptionField.ts → .../subComponents/K8sNameDescriptionField.ts
@@ -1,8 +1,12 @@
-export class K8sNameDescriptionField {
-  constructor(private compTestId: string) {}
+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 cy.findByTestId(`${this.compTestId}-${suffix}`);
+    return this.findScope().findByTestId(`${this.compTestId}-${suffix}`);
   }
 
   findDisplayNameInput(): Cypress.Chainable<JQuery<HTMLElement>> {

diff --git a/frontend/src/__tests__/cypress/cypress/pages/components/subComponents/SubComponentBase.ts b/frontend/src/__tests__/cypress/cypress/pages/components/subComponents/SubComponentBase.ts
@@ -0,0 +1,35 @@
+/**
+ * A SubComponent is a component that doesn't make up a full page and will be consumed in other page
+ * objects. This could be a complex field, a group of fields, or some section. Typically not large
+ * enough to warrant its own standalone page object.
+ *
+ * Primary use-case example:
+ *   class Foo extends SubComponentBase {
+ *     constructor(private myTestId: string, scopedTestId?: string) {
+ *       super(scopedTestId);
+ *     }
+ *
+ *     private find(suffix: string) {
+ *       return this.findScope().getByTestId(`${this.myTestId}-${suffix}`);
+ *     }
+ *
+ *     selectItem(name: string) {
+ *       // "list" would be an internal suffix for your component to know where the "items" are
+ *       return this.find('list').findDropdownItem(name);
+ *     }
+ *   }
+ *
+ * Search uses of this component to see further examples
+ */
+export class SubComponentBase {
+  constructor(private scopedBaseTestId?: string) {}
+
+  /** Allows for extended classes to make use of a simple one-check for their `find()` calls */
+  protected findScope(): (Cypress.cy & CyEventEmitter) | Cypress.Chainable<JQuery<HTMLElement>> {
+    if (this.scopedBaseTestId) {
+      return cy.findByTestId(this.scopedBaseTestId);
+    }
+
+    return cy;
+  }
+}
diff --git a/frontend/src/__tests__/cypress/cypress/pages/projects.ts b/frontend/src/__tests__/cypress/cypress/pages/projects.ts
@@ -1,7 +1,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 { K8sNameDescriptionField } from '~/__tests__/cypress/cypress/pages/components/K8sNameDescriptionField';
+import { K8sNameDescriptionField } from '~/__tests__/cypress/cypress/pages/components/subComponents/K8sNameDescriptionField';
 import { TableRow } from './components/table';
 import { TableToolbar } from './components/TableToolbar';
 
@@ -109,14 +109,12 @@ class ProjectListPage {
 }
 
 class CreateEditProjectModal extends Modal {
+  k8sNameDescription = new K8sNameDescriptionField('manage-project-modal');
+
   constructor(private edit = false) {
     super(`${edit ? 'Edit' : 'Create'} data science project`);
   }
 
-  getK8sNameDescriptionFields() {
-    return new K8sNameDescriptionField('manage-project-modal');
-  }
-
   findSubmitButton() {
     return this.findFooter().findByRole('button', { name: this.edit ? /Edit/ : /Create/ });
   }

diff --git a/frontend/src/__tests__/cypress/cypress/pages/workbench.ts b/frontend/src/__tests__/cypress/cypress/pages/workbench.ts
@@ -1,4 +1,4 @@
-import { K8sNameDescriptionField } from '~/__tests__/cypress/cypress/pages/components/K8sNameDescriptionField';
+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';
@@ -163,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;
@@ -220,10 +222,6 @@ class CreateSpawnerPage {
     return cy.findByTestId('submit-button');
   }
 
-  getK8sNameDescriptionFields() {
-    return new K8sNameDescriptionField('workbench');
-  }
-
   getEnvironmentVariableTypeField(index: number) {
     return new EnvironmentVariableTypeField(() =>
       cy.findAllByTestId('environment-variable-field').eq(index),

diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/project-list.scy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/project-list.scy.ts
@@ -39,15 +39,19 @@ describe('Data Science Projects', { testIsolation: false }, () => {
   });
 
   it('should create project', () => {
-    const fields = createProjectModal.getK8sNameDescriptionFields();
-    fields.findDisplayNameInput().type('My Test Project');
-    fields.findDescriptionInput().type('Test project description.');
+    createProjectModal.k8sNameDescription.findDisplayNameInput().type('My Test Project');
+    createProjectModal.k8sNameDescription.findDescriptionInput().type('Test project description.');
     createProjectModal.findSubmitButton().should('be.enabled');
-    fields.findResourceEditLink().click();
-    fields.findResourceNameInput().should('have.value', 'my-test-project').clear();
-    fields.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');
-    fields.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');

diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/projectList.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/projectList.cy.ts
@@ -46,35 +46,42 @@ describe('Data science projects details', () => {
     createProjectModal.shouldBeOpen();
     createProjectModal.findSubmitButton().should('be.disabled');
 
-    const fields = createProjectModal.getK8sNameDescriptionFields();
     // Standard items pass
-    fields.findDisplayNameInput().type('My Test Project');
-    fields.findDescriptionInput().type('Test project description.');
+    createProjectModal.k8sNameDescription.findDisplayNameInput().type('My Test Project');
+    createProjectModal.k8sNameDescription.findDescriptionInput().type('Test project description.');
     createProjectModal.findSubmitButton().should('be.enabled');
     // Really long display names pass
-    fields
+    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');
-    fields.findResourceEditLink().click();
-    fields.findResourceNameInput().should('have.attr', 'aria-invalid', 'false');
-    fields.findResourceNameInput().should('have.value', 'this-is-a-really-long-display');
+    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
-    fields.findResourceNameInput().clear().type('InVaLiD vAlUe!');
-    fields.findResourceNameInput().should('have.attr', 'aria-invalid', 'true');
+    createProjectModal.k8sNameDescription.findResourceNameInput().clear().type('InVaLiD vAlUe!');
+    createProjectModal.k8sNameDescription
+      .findResourceNameInput()
+      .should('have.attr', 'aria-invalid', 'true');
     createProjectModal.findSubmitButton().should('be.disabled');
     // Invalid length k8s names fail
-    fields
+    createProjectModal.k8sNameDescription
       .findResourceNameInput()
       .clear()
       .type('this-is-a-valid-character-string-but-it-is-too-long');
-    fields.findResourceNameInput().should('have.attr', 'aria-invalid', 'true');
+    createProjectModal.k8sNameDescription
+      .findResourceNameInput()
+      .should('have.attr', 'aria-invalid', 'true');
     createProjectModal.findSubmitButton().should('be.disabled');
     // Valid k8s names succeed
-    fields.findResourceNameInput().clear().type('test-project');
+    createProjectModal.k8sNameDescription.findResourceNameInput().clear().type('test-project');
     createProjectModal.findSubmitButton().click();
     cy.wsK8s('ADDED', ProjectModel, mockProjectK8sResource({}));
 

diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/projects/workbench.cy.ts
@@ -210,9 +210,8 @@ describe('Workbench page', () => {
     workbenchPage.findCreateButton().click();
     createSpawnerPage.findSubmitButton().should('be.disabled');
     verifyRelativeURL('/projects/test-project/spawner');
-    const nameDesc = createSpawnerPage.getK8sNameDescriptionFields();
-    nameDesc.findDisplayNameInput().fill('test-project');
-    nameDesc.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(
@@ -354,9 +353,8 @@ describe('Workbench page', () => {
     workbenchPage.findCreateButton().click();
     createSpawnerPage.findSubmitButton().should('be.disabled');
     verifyRelativeURL('/projects/test-project/spawner');
-    const nameDesc = createSpawnerPage.getK8sNameDescriptionFields();
-    nameDesc.findDisplayNameInput().fill('1234');
-    nameDesc.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(
@@ -549,13 +547,12 @@ describe('Workbench page', () => {
       mockK8sResourceList([mockPVCK8sResource({ name: 'test-notebook' })]),
     );
     editSpawnerPage.visit('test-notebook');
-    const nameDesc = editSpawnerPage.getK8sNameDescriptionFields();
-    nameDesc.findDisplayNameInput().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');
-    nameDesc.findDisplayNameInput().fill('Updated Notebook');
+    editSpawnerPage.k8sNameDescription.findDisplayNameInput().fill('Updated Notebook');
 
     cy.interceptK8s('PUT', NotebookModel, mockNotebookK8sResource({})).as('editWorkbench');
     editSpawnerPage.findSubmitButton().click();

diff --git a/frontend/src/concepts/k8s/K8sNameDescriptionField/HelperTextItemVariants.tsx b/frontend/src/concepts/k8s/K8sNameDescriptionField/HelperTextItemVariants.tsx
@@ -12,8 +12,6 @@ export const HelperTextItemMaxLength: HelperTextItemType = ({ k8sName }) => {
   let variant: Variants = 'indeterminate';
   if (k8sName.state.invalidLength) {
     variant = 'error';
-  } else if (k8sName.state.autoTrimmed) {
-    variant = 'warning';
   } else if (k8sName.value.trim().length > 0) {
     variant = 'success';
   }

diff --git a/frontend/src/concepts/k8s/K8sNameDescriptionField/ResourceNameField.tsx b/frontend/src/concepts/k8s/K8sNameDescriptionField/ResourceNameField.tsx
@@ -51,8 +51,6 @@ const ResourceNameField: React.FC<ResourceNameFieldProps> = ({
   let validated: ValidatedOptions = ValidatedOptions.default;
   if (k8sName.state.invalidLength || k8sName.state.invalidCharacters) {
     validated = ValidatedOptions.error;
-  } else if (k8sName.state.autoTrimmed) {
-    validated = ValidatedOptions.warning;
   } else if (k8sName.value.length > 0) {
     validated = ValidatedOptions.success;
   }

diff --git a/frontend/src/concepts/k8s/K8sNameDescriptionField/__tests__/utils.spec.ts b/frontend/src/concepts/k8s/K8sNameDescriptionField/__tests__/utils.spec.ts
@@ -88,7 +88,7 @@ describe('handleUpdateLogic', () => {
         name: overMaxLength,
         k8sName: {
           value: 'this-is-a-long-string-of-text',
-          state: { autoTrimmed: true, maxLength },
+          state: { maxLength },
         },
       }),
     );
@@ -134,28 +134,6 @@ describe('handleUpdateLogic', () => {
     );
   });
 
-  it('should not have auto trimmed and touched for k8s name', () => {
-    const configuredDefaults = setupDefaults({
-      limitNameResourceType: LimitNameResourceType.PROJECT,
-    });
-    const { maxLength } = configuredDefaults.k8sName.state;
-
-    const overMaxLength = 'this-really-is-a-long-string-of-text-for-k8s';
-    expect(overMaxLength.length).toBeGreaterThan(maxLength);
-
-    const causeAutoTrimState = handleUpdateLogic(configuredDefaults)('name', overMaxLength);
-    expect(causeAutoTrimState.k8sName.state.autoTrimmed).toBe(true);
-    expect(causeAutoTrimState.k8sName.state.invalidLength).toBe(false);
-    const currentValue = causeAutoTrimState.k8sName.value;
-
-    const causeOverLengthState = handleUpdateLogic(causeAutoTrimState)(
-      'k8sName',
-      `${currentValue}a`,
-    );
-    expect(causeOverLengthState.k8sName.state.autoTrimmed).toBe(false);
-    expect(causeOverLengthState.k8sName.state.invalidLength).toBe(true);
-  });
-
   it('should not allow update k8s name when immutable', () => {
     const startingImmutableData = setupDefaults({
       initialData: mockProjectK8sResource({
@@ -248,8 +226,7 @@ describe('isK8sNameDescriptionDataValid', () => {
           k8sName: {
             value: 'k8s-test',
             state: {
-              autoTrimmed: true,
-              immutable: true, // not possible with autoTrimmed, but brute force testing
+              immutable: true,
               invalidLength: false,
               invalidCharacters: false,
               maxLength: 123,

diff --git a/frontend/src/concepts/k8s/K8sNameDescriptionField/types.ts b/frontend/src/concepts/k8s/K8sNameDescriptionField/types.ts
@@ -7,8 +7,6 @@ export type K8sNameDescriptionFieldData = {
   k8sName: {
     value: string;
     state: {
-      /** Name is too long, auto trimmed for k8s */
-      autoTrimmed: boolean;
       /** The value cannot be changeable */
       immutable: boolean;
       /** If an invalid character was used */

diff --git a/frontend/src/concepts/k8s/K8sNameDescriptionField/utils.ts b/frontend/src/concepts/k8s/K8sNameDescriptionField/utils.ts
@@ -4,7 +4,7 @@ import {
   getDisplayNameFromK8sResource,
   isK8sDSGResource,
   isValidK8sName,
-  translateDisplayNameForK8sAndReport,
+  translateDisplayNameForK8s,
 } from '~/concepts/k8s/utils';
 import { RecursivePartial } from '~/typeHelpers';
 import {
@@ -58,7 +58,6 @@ export const setupDefaults = ({
     k8sName: {
       value: initialK8sNameValue,
       state: {
-        autoTrimmed: false,
         immutable: initialK8sNameValue !== '',
         invalidCharacters: false,
         invalidLength: false,
@@ -84,14 +83,11 @@ export const handleUpdateLogic =
         // When name changes, we want to update resource name if applicable
         if (!touched && !immutable) {
           // Update the generated name
-          const [k8sValue, whatHappened] = translateDisplayNameForK8sAndReport(value, {
+          const k8sValue = translateDisplayNameForK8s(value, {
             maxLength,
             safeK8sPrefix: safePrefix,
           });
           changedData.k8sName = {
-            state: {
-              autoTrimmed: whatHappened.maxLength,
-            },
             value: k8sValue,
           };
         }
@@ -100,7 +96,6 @@ export const handleUpdateLogic =
       case 'k8sName':
         changedData.k8sName = {
           state: {
-            autoTrimmed: false,
             invalidCharacters: value.length > 0 ? !isValidK8sName(value) : false,
             invalidLength: value.length > existingData.k8sName.state.maxLength,
             touched: true,