refactor errors

trento-project · Jul 2, 2024 · 700c914 · 700c914
1 parent c918490
commit 700c914
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 21 deletions.
diff --git a/assets/js/common/ActivityLogsSettingsModal/ActivityLogsSettingsModal.jsx b/assets/js/common/ActivityLogsSettingsModal/ActivityLogsSettingsModal.jsx
@@ -14,13 +14,19 @@ const defaultErrors = [];
 const timeUnitOptions = ['day', 'week', 'month', 'year'];
 const defaultTimeUnit = timeUnitOptions[0];
 
-const toErrorMessage = (errors) =>
+const toRetentionTimeErrorMessage = (errors) =>
   [
     capitalize(getError('retention_time/value', errors)),
     capitalize(getError('retention_time/unit', errors)),
   ]
     .filter(Boolean)
-    .join(', ');
+    .join('; ');
+
+const toGenericErrorMessage = (errors) =>
+  // If there is at lease one error not related to a specific field, return a generic error message
+  errors.some((error) => !error.source)
+    ? 'Something went wrong while saving'
+    : '';
 
 function TimeSpan({ time: initialTime, error = false, onChange = noop }) {
   const [time, setTime] = useState(initialTime);
@@ -58,6 +64,19 @@ function TimeSpan({ time: initialTime, error = false, onChange = noop }) {
   );
 }
 
+/**
+ * Display an error message. If no error is provided, an empty space is displayed to keep the layout stable
+ * @param {string} text The error message to display
+ * @returns {JSX.Element}
+ */
+function Error({ text }) {
+  return text ? (
+    <p className="text-red-500 mt-1">{text}</p>
+  ) : (
+    <p className="mt-1">&nbsp;</p>
+  );
+}
+
 /**
  * Modal to edit Activity Logs settings
  */
@@ -72,7 +91,8 @@ function ActivityLogsSettingsModal({
 }) {
   const [retentionTime, setRetentionTime] = useState(initialRetentionTime);
 
-  const errorMessage = toErrorMessage(errors);
+  const retentionTimeError = toRetentionTimeErrorMessage(errors);
+  const genericError = toGenericErrorMessage(errors);
 
   return (
     <Modal title="Enter Activity Logs Settings" open={open} onClose={onCancel}>
@@ -83,20 +103,13 @@ function ActivityLogsSettingsModal({
         <div className="col-span-4">
           <TimeSpan
             time={retentionTime}
-            error={Boolean(errorMessage)}
+            error={Boolean(retentionTimeError)}
             onChange={(time) => {
               setRetentionTime(time);
               onClearErrors();
             }}
           />
-          {errorMessage && (
-            <p
-              aria-label="retention-time-input-error"
-              className="text-red-500 mt-1"
-            >
-              {errorMessage}
-            </p>
-          )}
+          <Error text={retentionTimeError} />
         </div>
 
         <p className="col-span-6">
@@ -119,6 +132,9 @@ function ActivityLogsSettingsModal({
           Cancel
         </Button>
       </div>
+      <div className="flex flex-row w-80 space-x-2">
+        <Error text={genericError} />
+      </div>
     </Modal>
   );
 }

diff --git a/assets/js/common/ActivityLogsSettingsModal/ActivityLogsSettingsModal.stories.jsx b/assets/js/common/ActivityLogsSettingsModal/ActivityLogsSettingsModal.stories.jsx
@@ -31,16 +31,43 @@ export const Default = {
   },
 };
 
-export const WithErrors = {
+export const WithFieldValidationError = {
   args: {
     open: false,
     initialRetentionTime: { value: 1, unit: 'month' },
     errors: [
       {
-        detail: "can't be blank",
-        source: { pointer: '/retentionTime' },
+        detail: 'must be greater than or equal to 1',
+        source: { pointer: '/retention_time/value' },
         title: 'Invalid value',
       },
     ],
   },
 };
+
+export const WithCompositeFieldValidationError = {
+  args: {
+    open: false,
+    initialRetentionTime: { value: 1, unit: 'month' },
+    errors: [
+      {
+        detail: 'must be greater than or equal to 1',
+        source: { pointer: '/retention_time/value' },
+        title: 'Invalid value',
+      },
+      {
+        detail: 'invalid time unit',
+        source: { pointer: '/retention_time/unit' },
+        title: 'Invalid unit',
+      },
+    ],
+  },
+};
+
+export const WithGenericError = {
+  args: {
+    open: false,
+    initialRetentionTime: { value: 1, unit: 'month' },
+    errors: ['any error'],
+  },
+};
diff --git a/assets/js/common/ActivityLogsSettingsModal/ActivityLogsSettingsModal.test.jsx b/assets/js/common/ActivityLogsSettingsModal/ActivityLogsSettingsModal.test.jsx
@@ -118,13 +118,17 @@ describe('ActivityLogsSettingsModal component', () => {
     source: { pointer: '/retention_time/unit' },
     title: faker.lorem.words(10),
   };
+  const anyPayloadError = { foo: 'bar' };
 
   it.each`
-    scenario                       | errors                     | expectedErrorMessage
-    ${'value error'}               | ${[valueError]}            | ${valueError.detail}
-    ${'unit error'}                | ${[unitError]}             | ${unitError.detail}
-    ${'value and unit errors (1)'} | ${[valueError, unitError]} | ${valueError.detail}
-    ${'value and unit errors (2)'} | ${[valueError, unitError]} | ${unitError.detail}
+    scenario                               | errors                           | expectedErrorMessage
+    ${'value error'}                       | ${[valueError]}                  | ${valueError.detail}
+    ${'unit error'}                        | ${[unitError]}                   | ${unitError.detail}
+    ${'value and unit errors (1)'}         | ${[valueError, unitError]}       | ${valueError.detail}
+    ${'value and unit errors (2)'}         | ${[valueError, unitError]}       | ${unitError.detail}
+    ${'generic error'}                     | ${[anyPayloadError]}             | ${'Something went wrong while saving'}
+    ${'generic error and value error (1)'} | ${[anyPayloadError, valueError]} | ${'Something went wrong while saving'}
+    ${'generic error and value error (2)'} | ${[anyPayloadError, valueError]} | ${valueError.detail}
   `(
     'should display errors on $scenario',
     async ({ errors, expectedErrorMessage }) => {

diff --git a/assets/js/state/activityLogsSettings.test.js b/assets/js/state/activityLogsSettings.test.js
@@ -110,7 +110,7 @@ describe('ActivityLogsSettings reducer', () => {
       },
       networkError: false,
       errors: [],
-      editing: false
+      editing: false,
     };
 
     [true, false].forEach((hasNetworkError) => {