From ccfdb49db2496882945750d116e34c10da05baec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florian=20Sch=C3=BCller?= Date: Mon, 12 May 2025 12:48:24 +0200 Subject: [PATCH] CreateImageWizard: Implement user-friendly error messages Puts the error messages in one place - LabelInput.tsx --- playwright/Customizations/Firewall.spec.ts | 16 ++- playwright/Customizations/Kernel.spec.ts | 6 +- playwright/Customizations/Systemd.spec.ts | 12 +- playwright/Customizations/Timezone.spec.ts | 6 +- .../CreateImageWizard/LabelInput.tsx | 39 +++++- .../utilities/useValidation.tsx | 121 +++++++----------- .../Blueprints/ImportBlueprintModal.test.tsx | 3 - .../steps/Firewall/Firewall.test.tsx | 18 ++- .../steps/Registration/Registration.test.tsx | 2 +- .../steps/Services/Services.test.tsx | 20 ++- .../steps/Timezone/Timezone.test.tsx | 10 +- 11 files changed, 156 insertions(+), 97 deletions(-) diff --git a/playwright/Customizations/Firewall.spec.ts b/playwright/Customizations/Firewall.spec.ts index 3748a451..c228ef7b 100644 --- a/playwright/Customizations/Firewall.spec.ts +++ b/playwright/Customizations/Firewall.spec.ts @@ -63,19 +63,29 @@ test('Create a blueprint with Firewall customization', async ({ await test.step('Select and incorrectly fill the ports in Firewall step', async () => { await frame.getByPlaceholder('Add ports').fill('x'); await frame.getByRole('button', { name: 'Add ports' }).click(); - await expect(frame.getByText('Invalid format.').nth(0)).toBeVisible(); + await expect( + frame + .getByText( + 'Expected format: :. Example: 8080:tcp, ssh:tcp' + ) + .nth(0) + ).toBeVisible(); }); await test.step('Select and incorrectly fill the disabled services in Firewall step', async () => { await frame.getByPlaceholder('Add disabled service').fill('1'); await frame.getByRole('button', { name: 'Add disabled service' }).click(); - await expect(frame.getByText('Invalid format.').nth(1)).toBeVisible(); + await expect( + frame.getByText('Expected format: . Example: sshd').nth(0) + ).toBeVisible(); }); await test.step('Select and incorrectly fill the enabled services in Firewall step', async () => { await frame.getByPlaceholder('Add enabled service').fill('ťčš'); await frame.getByRole('button', { name: 'Add enabled service' }).click(); - await expect(frame.getByText('Invalid format.').nth(2)).toBeVisible(); + await expect( + frame.getByText('Expected format: . Example: sshd').nth(1) + ).toBeVisible(); }); await test.step('Fill the BP details', async () => { diff --git a/playwright/Customizations/Kernel.spec.ts b/playwright/Customizations/Kernel.spec.ts index e42d0047..a3c531ff 100644 --- a/playwright/Customizations/Kernel.spec.ts +++ b/playwright/Customizations/Kernel.spec.ts @@ -49,7 +49,11 @@ test('Create a blueprint with Kernel customization', async ({ .getByPlaceholder('Add kernel argument') .fill('invalid/argument'); await frame.getByRole('button', { name: 'Add kernel argument' }).click(); - await expect(frame.getByText('Invalid format.')).toBeVisible(); + await expect( + frame.getByText( + 'Expected format: . Example: console=tty0' + ) + ).toBeVisible(); await frame.getByPlaceholder('Select kernel package').fill('new-package'); await frame .getByRole('option', { name: 'Custom kernel package "new-' }) diff --git a/playwright/Customizations/Systemd.spec.ts b/playwright/Customizations/Systemd.spec.ts index bbd1b3cc..0ad7ba1a 100644 --- a/playwright/Customizations/Systemd.spec.ts +++ b/playwright/Customizations/Systemd.spec.ts @@ -64,15 +64,21 @@ test('Create a blueprint with Systemd customization', async ({ await test.step('Select and incorrectly fill all of the service fields', async () => { await frame.getByPlaceholder('Add disabled service').fill('&&'); await frame.getByRole('button', { name: 'Add disabled service' }).click(); - await expect(frame.getByText('Invalid format.').nth(0)).toBeVisible(); + await expect( + frame.getByText('Expected format: . Example: sshd').nth(0) + ).toBeVisible(); await frame.getByPlaceholder('Add enabled service').fill('áá'); await frame.getByRole('button', { name: 'Add enabled service' }).click(); - await expect(frame.getByText('Invalid format.').nth(1)).toBeVisible(); + await expect( + frame.getByText('Expected format: . Example: sshd').nth(1) + ).toBeVisible(); await frame.getByPlaceholder('Add masked service').fill('78'); await frame.getByRole('button', { name: 'Add masked service' }).click(); - await expect(frame.getByText('Invalid format.').nth(2)).toBeVisible(); + await expect( + frame.getByText('Expected format: . Example: sshd').nth(2) + ).toBeVisible(); }); await test.step('Fill the BP details', async () => { diff --git a/playwright/Customizations/Timezone.spec.ts b/playwright/Customizations/Timezone.spec.ts index 4d2de9f9..90ab00c4 100644 --- a/playwright/Customizations/Timezone.spec.ts +++ b/playwright/Customizations/Timezone.spec.ts @@ -55,7 +55,11 @@ test('Create a blueprint with Timezone customization', async ({ await expect(frame.getByText('NTP server already exists.')).toBeVisible(); await frame.getByPlaceholder('Add NTP servers').fill('xxxx'); await frame.getByRole('button', { name: 'Add NTP server' }).click(); - await expect(frame.getByText('Invalid format.')).toBeVisible(); + await expect( + frame + .getByText('Expected format: . Example: time.redhat.com') + .nth(0) + ).toBeVisible(); await frame.getByPlaceholder('Add NTP servers').fill('0.cz.pool.ntp.org'); await frame.getByRole('button', { name: 'Add NTP server' }).click(); await expect(frame.getByText('0.cz.pool.ntp.org')).toBeVisible(); diff --git a/src/Components/CreateImageWizard/LabelInput.tsx b/src/Components/CreateImageWizard/LabelInput.tsx index 031fa3e7..5e3e2dd7 100644 --- a/src/Components/CreateImageWizard/LabelInput.tsx +++ b/src/Components/CreateImageWizard/LabelInput.tsx @@ -71,7 +71,44 @@ const LabelInput = ({ } if (!validator(value)) { - setOnStepInputErrorText('Invalid format.'); + switch (fieldName) { + case 'ports': + setOnStepInputErrorText( + 'Expected format: :. Example: 8080:tcp, ssh:tcp' + ); + break; + case 'kernelAppend': + setOnStepInputErrorText( + 'Expected format: . Example: console=tty0' + ); + break; + case 'kernelName': + setOnStepInputErrorText( + 'Expected format: . Example: kernel-5.14.0-284.11.1.el9_2.x86_64' + ); + break; + case 'groups': + setOnStepInputErrorText( + 'Expected format: . Example: admin' + ); + break; + case 'ntpServers': + setOnStepInputErrorText( + 'Expected format: . Example: time.redhat.com' + ); + break; + case 'enabledSystemdServices': + case 'disabledSystemdServices': + case 'maskedSystemdServices': + case 'disabledServices': + case 'enabledServices': + setOnStepInputErrorText( + 'Expected format: . Example: sshd' + ); + break; + default: + setOnStepInputErrorText('Invalid format.'); + } return; } diff --git a/src/Components/CreateImageWizard/utilities/useValidation.tsx b/src/Components/CreateImageWizard/utilities/useValidation.tsx index 93e49677..f4621b1c 100644 --- a/src/Components/CreateImageWizard/utilities/useValidation.tsx +++ b/src/Components/CreateImageWizard/utilities/useValidation.tsx @@ -1,7 +1,7 @@ import React, { useEffect, useState } from 'react'; import { CheckCircleIcon } from '@patternfly/react-icons'; -import { jwtDecode, JwtPayload } from 'jwt-decode'; +import { jwtDecode } from 'jwt-decode'; import { getListOfDuplicates } from './getListOfDuplicates'; @@ -30,6 +30,7 @@ import { selectLanguages, selectKeyboard, selectTimezone, + selectSatelliteCaCertificate, selectSatelliteRegistrationCommand, selectImageTypes, UserWithAdditionalInfo, @@ -45,11 +46,11 @@ import { isMountpointMinSizeValid, isSnapshotValid, isHostnameValid, - isKernelNameValid, isUserNameValid, isSshKeyValid, isNtpServerValid, isKernelArgumentValid, + isKernelNameValid, isPortValid, isServiceValid, isUserGroupValid, @@ -113,74 +114,13 @@ type ValidationState = { ruleCharacters: HelperTextVariant; }; -function tokenValidityRemaining(expireTimeInSeconds: number): number { - const currentTimeSeconds = Math.floor(Date.now() / 1000); - return expireTimeInSeconds - currentTimeSeconds; -} - -function decodeToken(token: string): JwtPayload | undefined { - try { - const decoded = jwtDecode(token) as { exp?: number }; - return decoded; - } catch { - return undefined; - } -} - -function getExpirationString(totalSeconds: number): string | undefined { - const hours = Math.floor(totalSeconds / 3600); - - if (hours > 25) { - return undefined; - } - - if (hours > 0) { - return `${hours} hour${hours > 1 ? 's' : ''}`; - } - return 'less than an hour'; -} - -export function validateSatelliteToken( - registrationCommand: string | undefined -) { - const errors: Record = {}; - if (registrationCommand === '') { - errors.command = 'No registration command for Satellite registration'; - return errors; - } - - const match = registrationCommand?.match(/Bearer\s+([\w-]+\.[\w-]+\.[\w-]+)/); - if (!match || match.length < 2) { - errors.command = 'Invalid or missing token'; - return errors; - } - const satelliteToken = decodeToken(match[1]); - if (satelliteToken === undefined) { - errors.command = 'Invalid or missing token'; - return errors; - } - - if (satelliteToken.exp) { - const tokenRemainingS = tokenValidityRemaining(satelliteToken.exp); - if (tokenRemainingS <= 0) { - errors.command = `The token is expired. Check out the Satellite documentation to extend the token lifetime.`; - return errors; - } - const expirationString = getExpirationString(tokenRemainingS); - if (expirationString !== undefined) { - errors.expired = `The token expires in ${expirationString}. Check out the Satellite documentation to extend the token lifetime.`; - } - } - - return errors; -} - export function useRegistrationValidation(): StepValidation { const registrationType = useAppSelector(selectRegistrationType); const activationKey = useAppSelector(selectActivationKey); const registrationCommand = useAppSelector( selectSatelliteRegistrationCommand ); + const caCertificate = useAppSelector(selectSatelliteCaCertificate); const { isFetching: isFetchingKeyInfo, isError: isErrorKeyInfo } = useShowActivationKeyQuery( @@ -190,11 +130,7 @@ export function useRegistrationValidation(): StepValidation { } ); - if ( - registrationType !== 'register-later' && - registrationType !== 'register-satellite' && - !activationKey - ) { + if (registrationType !== 'register-later' && !activationKey) { return { errors: { activationKey: 'No activation key selected' }, disabledNext: true, @@ -203,7 +139,6 @@ export function useRegistrationValidation(): StepValidation { if ( registrationType !== 'register-later' && - registrationType !== 'register-satellite' && activationKey && (isFetchingKeyInfo || isErrorKeyInfo) ) { @@ -215,14 +150,50 @@ export function useRegistrationValidation(): StepValidation { if (registrationType === 'register-satellite') { const errors = {}; - const tokenErrors = validateSatelliteToken(registrationCommand); - Object.assign(errors, tokenErrors); - + if (caCertificate === '') { + Object.assign(errors, { + certificate: + 'Valid certificate must be present if you are registering Satellite.', + }); + } + if (registrationCommand === '' || !registrationCommand) { + Object.assign(errors, { + command: 'No registration command for Satellite registration', + }); + } + try { + const match = registrationCommand?.match( + /Bearer\s+([\w-]+\.[\w-]+\.[\w-]+)/ + ); + if (!match) { + Object.assign(errors, { command: 'Invalid or missing token' }); + } else { + const token = match[1]; + const decoded = jwtDecode(token); + if (decoded.exp) { + const currentTimeSeconds = Date.now() / 1000; + const dayInSeconds = 86400; + if (decoded.exp < currentTimeSeconds + dayInSeconds) { + const expirationDate = new Date(decoded.exp * 1000); + Object.assign(errors, { + expired: + 'The token is already expired or will expire by next day. Expiration date: ' + + expirationDate, + }); + return { + errors: errors, + disabledNext: caCertificate === undefined, + }; + } + } + } + } catch { + Object.assign(errors, { command: 'Invalid or missing token' }); + } return { errors: errors, disabledNext: - Object.keys(errors).some((key) => key !== 'expired') || - !registrationCommand, + Object.keys(errors).length > 0 || caCertificate === undefined, }; } diff --git a/src/test/Components/Blueprints/ImportBlueprintModal.test.tsx b/src/test/Components/Blueprints/ImportBlueprintModal.test.tsx index 828aa64a..29708fa3 100644 --- a/src/test/Components/Blueprints/ImportBlueprintModal.test.tsx +++ b/src/test/Components/Blueprints/ImportBlueprintModal.test.tsx @@ -555,9 +555,6 @@ describe('Import modal', () => { await screen.findByPlaceholderText('Paste your public SSH key') ) ); - expect( - await screen.findByText(/Invalid user groups: 0000/) - ).toBeInTheDocument(); await waitFor(() => user.click(screen.getByRole('button', { name: /close 0000/i })) ); diff --git a/src/test/Components/CreateImageWizard/steps/Firewall/Firewall.test.tsx b/src/test/Components/CreateImageWizard/steps/Firewall/Firewall.test.tsx index a8596d03..b3c0dd40 100644 --- a/src/test/Components/CreateImageWizard/steps/Firewall/Firewall.test.tsx +++ b/src/test/Components/CreateImageWizard/steps/Firewall/Firewall.test.tsx @@ -120,17 +120,27 @@ describe('Step Firewall', () => { test('port in an invalid format cannot be added', async () => { await renderCreateMode(); await goToFirewallStep(); - expect(screen.queryByText('Invalid format.')).not.toBeInTheDocument(); + expect( + screen.queryByText( + 'Expected format: :. Example: 8080:tcp, ssh:tcp' + ) + ).not.toBeInTheDocument(); await addPort('00:wrongFormat'); - await screen.findByText('Invalid format.'); + await screen.findByText( + 'Expected format: :. Example: 8080:tcp, ssh:tcp' + ); }); test('service in an invalid format cannot be added', async () => { await renderCreateMode(); await goToFirewallStep(); - expect(screen.queryByText('Invalid format.')).not.toBeInTheDocument(); + expect( + screen.queryByText('Expected format: . Example: sshd') + ).not.toBeInTheDocument(); await addPort('wrong--service'); - await screen.findByText('Invalid format.'); + await screen.findByText( + 'Expected format: :. Example: 8080:tcp, ssh:tcp' + ); }); test('revisit step button on Review works', async () => { diff --git a/src/test/Components/CreateImageWizard/steps/Registration/Registration.test.tsx b/src/test/Components/CreateImageWizard/steps/Registration/Registration.test.tsx index d818532a..e17372c7 100644 --- a/src/test/Components/CreateImageWizard/steps/Registration/Registration.test.tsx +++ b/src/test/Components/CreateImageWizard/steps/Registration/Registration.test.tsx @@ -438,7 +438,7 @@ describe('Registration request generated correctly', () => { ); const expiredTokenHelper = await screen.findByText( - /The token is expired./i + /The token is already expired or will expire by next day./i ); await waitFor(() => expect(expiredTokenHelper).toBeInTheDocument()); diff --git a/src/test/Components/CreateImageWizard/steps/Services/Services.test.tsx b/src/test/Components/CreateImageWizard/steps/Services/Services.test.tsx index 2a9837e4..a3d9075c 100644 --- a/src/test/Components/CreateImageWizard/steps/Services/Services.test.tsx +++ b/src/test/Components/CreateImageWizard/steps/Services/Services.test.tsx @@ -179,20 +179,34 @@ describe('Step Services', () => { // Enabled services input expect(screen.queryByText('Invalid format.')).not.toBeInTheDocument(); await addEnabledService('-------'); - expect(await screen.findByText('Invalid format.')).toBeInTheDocument(); + expect( + await screen.findByText('Expected format: . Example: sshd') + ).toBeInTheDocument(); await waitFor(() => user.click(clearInputButtons[0])); // Disabled services input expect(screen.queryByText('Invalid format.')).not.toBeInTheDocument(); await addDisabledService('-------'); - expect(await screen.findByText('Invalid format.')).toBeInTheDocument(); + expect( + await screen.findByText('Expected format: . Example: sshd') + ).toBeInTheDocument(); await waitFor(() => user.click(clearInputButtons[1])); // Masked services input expect(screen.queryByText('Invalid format.')).not.toBeInTheDocument(); await addMaskedService('-------'); - expect(await screen.findByText('Invalid format.')).toBeInTheDocument(); + expect( + await screen.findByText('Expected format: . Example: sshd') + ).toBeInTheDocument(); await waitFor(() => user.click(clearInputButtons[2])); + + // Enabled services input + expect(screen.queryByText('Invalid format.')).not.toBeInTheDocument(); + await addEnabledService('-------'); + expect( + await screen.findByText('Expected format: . Example: sshd') + ).toBeInTheDocument(); + await waitFor(() => user.click(clearInputButtons[0])); }); test('services from OpenSCAP get added correctly and cannot be removed', async () => { diff --git a/src/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx b/src/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx index 129a2661..3ff422f3 100644 --- a/src/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx +++ b/src/test/Components/CreateImageWizard/steps/Timezone/Timezone.test.tsx @@ -162,9 +162,15 @@ describe('Step Timezone', () => { test('NTP server in an invalid format cannot be added', async () => { await renderCreateMode(); await goToTimezoneStep(); - expect(screen.queryByText('Invalid format.')).not.toBeInTheDocument(); + expect( + screen.queryByText( + 'Expected format: . Example: time.redhat.com' + ) + ).not.toBeInTheDocument(); await addNtpServerViaKeyDown('this is not NTP server'); - await screen.findByText('Invalid format.'); + await screen.findByText( + 'Expected format: . Example: time.redhat.com' + ); }); test('revisit step button on Review works', async () => {