Display errors in SUSE Manager settings form (#2455)

* Display errors in suse manager settings form * Add a Jest test for the modal * Remove a useless jest.fn() from a test * Clear errors on canceling
trento-project · Mar 25, 2024 · 6f651ec · 6f651ec
1 parent 5f48538
commit 6f651ec
Show file tree

Hide file tree

Showing 6 changed files with 189 additions and 51 deletions.
diff --git a/assets/js/common/SuseManagerSettingsDialog/SuseManagerSettingsModal.jsx b/assets/js/common/SuseManagerSettingsDialog/SuseManagerSettingsModal.jsx
@@ -1,5 +1,5 @@
 import React, { useState } from 'react';
-import { noop } from 'lodash';
+import { capitalize, noop } from 'lodash';
 import { format } from 'date-fns';
 import { EOS_LOCK_OUTLINED } from 'eos-icons-react';
 
@@ -8,7 +8,7 @@ import Modal from '@common/Modal';
 import Input, { Password, Textarea } from '@common/Input';
 import Label from '@common/Label';
 
-import { hasError } from './errors';
+import { hasError, getError } from './errors';
 
 const defaultErrors = [];
 
@@ -54,37 +54,49 @@ function SuseManagerSettingsModal({
 
   return (
     <Modal title="Enter SUSE Manager Settings" open={open} onClose={onCancel}>
-      <div className="grid grid-cols-6 items-center my-5 gap-6">
+      <div className="grid grid-cols-6 my-5 gap-6">
         <Label className="col-span-2" required>
           SUSE Manager URL
         </Label>
-        <Input
-          className="col-span-4"
-          value={url}
-          placeholder="Enter a URL"
-          error={hasError('url', errors)}
-          onChange={({ target: { value } }) => {
-            setUrl(value);
-            onClearErrors();
-          }}
-        />
+        <div className="col-span-4">
+          <Input
+            value={url}
+            placeholder="Enter a URL"
+            error={hasError('url', errors)}
+            onChange={({ target: { value } }) => {
+              setUrl(value);
+              onClearErrors();
+            }}
+          />
+          {hasError('url', errors) && (
+            <p className="text-red-500 mt-1">
+              {capitalize(getError('url', errors))}
+            </p>
+          )}
+        </div>
         <Label
           className="col-span-2 self-start"
           info="Only required for self-signed certificates"
         >
           CA Certificate
         </Label>
         {editingCertificate ? (
-          <Textarea
-            className="col-span-4"
-            value={certificate}
-            placeholder="Starts with -----BEGIN CERTIFICATE-----"
-            error={hasError('ca_cert', errors)}
-            onChange={({ target: { value } }) => {
-              setCertificate(value);
-              onClearErrors();
-            }}
-          />
+          <div className="col-span-4">
+            <Textarea
+              value={certificate}
+              placeholder="Starts with -----BEGIN CERTIFICATE-----"
+              error={hasError('ca_cert', errors)}
+              onChange={({ target: { value } }) => {
+                setCertificate(value);
+                onClearErrors();
+              }}
+            />
+            {hasError('ca_cert', errors) && (
+              <p className="text-red-500 mt-1">
+                {capitalize(getError('ca_cert', errors))}
+              </p>
+            )}
+          </div>
         ) : (
           <div className="col-span-4 flex flex-row items-center justify-start p-5 border border-gray-200 rounded-md">
             <EOS_LOCK_OUTLINED className="mr-3" size="25" />
@@ -104,30 +116,42 @@ function SuseManagerSettingsModal({
         <Label className="col-span-2" required>
           Username
         </Label>
-        <Input
-          className="col-span-4"
-          value={username}
-          placeholder="Enter a SUSE Manager username"
-          error={hasError('username', errors)}
-          onChange={({ target: { value } }) => {
-            setUsername(value);
-            onClearErrors();
-          }}
-        />
-        <Label className="col-span-2" required>
-          Password
-        </Label>
-        {editingPassword ? (
-          <Password
-            className="col-span-4"
-            value={password}
-            placeholder="Enter a SUSE Manager password"
-            error={hasError('password', errors)}
+        <div className="col-span-4">
+          <Input
+            value={username}
+            placeholder="Enter a SUSE Manager username"
+            error={hasError('username', errors)}
             onChange={({ target: { value } }) => {
-              setPassword(value);
+              setUsername(value);
               onClearErrors();
             }}
           />
+          {hasError('username', errors) && (
+            <p className="text-red-500 mt-1">
+              {capitalize(getError('username', errors))}
+            </p>
+          )}
+        </div>
+        <Label className="col-span-2" required>
+          Password
+        </Label>
+        {editingPassword ? (
+          <div className="col-span-4">
+            <Password
+              value={password}
+              placeholder="Enter a SUSE Manager password"
+              error={hasError('password', errors)}
+              onChange={({ target: { value } }) => {
+                setPassword(value);
+                onClearErrors();
+              }}
+            />
+            {hasError('password', errors) && (
+              <p className="text-red-500 mt-1">
+                {capitalize(getError('password', errors))}
+              </p>
+            )}
+          </div>
         ) : (
           <div className="col-span-4 border border-gray-200 p-5 rounded-md">
             <p className="inline align-sub leading-10">•••••</p>

diff --git a/assets/js/common/SuseManagerSettingsDialog/SuseManagerSettingsModal.stories.jsx b/assets/js/common/SuseManagerSettingsDialog/SuseManagerSettingsModal.stories.jsx
@@ -76,6 +76,34 @@ export const WithErrors = {
   },
 };
 
+export const WithAllErrors = {
+  args: {
+    open: false,
+    errors: [
+      {
+        detail: "can't be blank",
+        source: { pointer: '/url' },
+        title: 'Invalid value',
+      },
+      {
+        detail: "can't be blank",
+        source: { pointer: '/ca_cert' },
+        title: 'Invalid value',
+      },
+      {
+        detail: "can't be blank",
+        source: { pointer: '/password' },
+        title: 'Invalid value',
+      },
+      {
+        detail: "can't be blank",
+        source: { pointer: '/username' },
+        title: 'Invalid value',
+      },
+    ],
+  },
+};
+
 export const Loading = {
   args: {
     open: false,

diff --git a/assets/js/common/SuseManagerSettingsDialog/SuseManagerSettingsModal.test.jsx b/assets/js/common/SuseManagerSettingsDialog/SuseManagerSettingsModal.test.jsx
@@ -4,6 +4,8 @@ import { render, screen, act } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import '@testing-library/jest-dom';
 
+import { capitalize } from 'lodash';
+
 import SuseManagerSettingsModal from '.';
 
 describe('SuseManagerSettingsModal component', () => {
@@ -126,4 +128,36 @@ describe('SuseManagerSettingsModal component', () => {
       url,
     });
   });
+
+  it('should display errors', async () => {
+    const detail = capitalize(faker.lorem.words(5));
+
+    const errors = [
+      {
+        detail,
+        source: { pointer: '/url' },
+        title: 'Invalid value',
+      },
+      {
+        detail,
+        source: { pointer: '/ca_cert' },
+        title: 'Invalid value',
+      },
+    ];
+
+    await act(async () => {
+      render(
+        <SuseManagerSettingsModal
+          initialUsername={faker.word.noun()}
+          initialUrl={faker.internet.url()}
+          errors={errors}
+          open
+          onSave={() => {}}
+          onCancel={() => {}}
+        />
+      );
+    });
+
+    expect(screen.getAllByText(detail)).toHaveLength(2);
+  });
 });
diff --git a/assets/js/common/SuseManagerSettingsDialog/errors.js b/assets/js/common/SuseManagerSettingsDialog/errors.js
@@ -1,12 +1,20 @@
-import { get } from 'lodash';
+import { flow, first } from 'lodash';
+import { get, filter } from 'lodash/fp';
 
 export const hasError = (keyword, errors) =>
   errors.some((error) => {
-    const pointer = get(error, ['source', 'pointer']);
-
-    if (!pointer) {
-      return false;
-    }
+    const pointer = get(['source', 'pointer'], error);
 
     return pointer === `/${keyword}`;
   });
+
+export const getError = (keyword, errors) =>
+  flow([
+    filter((error) => {
+      const pointer = get(['source', 'pointer'], error);
+
+      return pointer === `/${keyword}`;
+    }),
+    first,
+    get('detail'),
+  ])(errors);
diff --git a/assets/js/common/SuseManagerSettingsDialog/errors.test.js b/assets/js/common/SuseManagerSettingsDialog/errors.test.js
@@ -1,4 +1,4 @@
-import { hasError } from './errors';
+import { hasError, getError } from './errors';
 
 describe('hasError', () => {
   it('should tell that a list contains an error about a specific field', () => {
@@ -39,3 +39,43 @@ describe('hasError', () => {
     expect(hasError('url', errors)).toBe(false);
   });
 });
+
+describe('getError', () => {
+  it('should spot nothing in an empty list', () => {
+    expect(getError('url', [])).toBe(undefined);
+  });
+
+  it('should get nothing when the keyword is not in the list', () => {
+    const errors = [
+      {
+        detail: "can't be blank",
+        source: { pointer: '/username' },
+        title: 'Invalid value',
+      },
+      {
+        detail: "can't be blank",
+        source: { pointer: '/ca_cert' },
+        title: 'Invalid value',
+      },
+    ];
+
+    expect(getError('username', errors)).toBe("can't be blank");
+  });
+
+  it('should get nothing when the keyword is not in the list', () => {
+    const errors = [
+      {
+        detail: "can't be blank",
+        source: { pointer: '/username' },
+        title: 'Invalid value',
+      },
+      {
+        detail: "can't be blank",
+        source: { pointer: '/ca_cert' },
+        title: 'Invalid value',
+      },
+    ];
+
+    expect(getError('url', errors)).toBe(undefined);
+  });
+});
diff --git a/assets/js/pages/SettingsPage/SettingsPage.jsx b/assets/js/pages/SettingsPage/SettingsPage.jsx
@@ -22,6 +22,7 @@ import {
   setEditingSoftwareUpdatesSettings,
   clearSoftwareUpdatesSettings,
   testSoftwareUpdatesConnection,
+  setSoftwareUpdatesSettingsErrors,
 } from '@state/softwareUpdatesSettings';
 import {
   getSoftwareUpdatesSettings,
@@ -299,7 +300,10 @@ function SettingsPage() {
                 dispatch(saveSoftwareUpdatesSettings(payload));
               }
             }}
-            onCancel={() => dispatch(setEditingSoftwareUpdatesSettings(false))}
+            onCancel={() => {
+              dispatch(setSoftwareUpdatesSettingsErrors([]));
+              dispatch(setEditingSoftwareUpdatesSettings(false));
+            }}
           />
         </div>
       )}