STSMACOM-781 correctly save empty custom-fields list (#1396)

Correctly save an empty custom fields list. Previously, an empty list would result in an NPE, calling `map` on a null value. Now, if the value is null, functions return an empty array. Tests related to custom fields removal were disabled long ago because they fail for unclear reasons and I was unable to resuscitate them now. Refs STSMACOM-781
folio-org · Sep 28, 2023 · 41f39c2 · 41f39c2
1 parent e4fa0de
commit 41f39c2
Show file tree

Hide file tree

Showing 5 changed files with 108 additions and 32 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -30,6 +30,7 @@
 * Do not reset advanced search filters if the advanced search option is already selected. Fixes STSMACOM-777.
 * *BREAKING* bump `react-intl` to `v6.4.4`. Refs STSMACOM-780.
 * Add possible to pass `paneTitleRef` from `SearchAndSort` to `Pane` that already support `paneTitleRef`. Refs STSMACOM-779.
+* Correctly save empty list when all custom fields are removed. Refs STSMACOM-781.
 
 ## [8.0.0](https://github.com/folio-org/stripes-smart-components/tree/v8.0.0) (2023-01-30)
 [Full Changelog](https://github.com/folio-org/stripes-smart-components/compare/v7.3.0...v8.0.0)

diff --git a/lib/CustomFields/pages/EditCustomFieldsSettings/EditCustomFieldsSettings.js b/lib/CustomFields/pages/EditCustomFieldsSettings/EditCustomFieldsSettings.js
@@ -195,29 +195,37 @@ const EditCustomFieldsSettings = ({
     });
   }, [makeOkapiRequest]);
 
-  const formatCustomFieldsForBackend = customFieldsFormData => {
-    return customFieldsFormData.customFields.map(({ values: { hidden, ...customFieldData }, id }) => {
-      const customFieldDataWithSortedOptions = cloneDeep(customFieldData);
-
-      if (fieldTypesWithOptions.includes(customFieldData.type)) {
-        const options = customFieldDataWithSortedOptions.selectField.options.values;
-        customFieldDataWithSortedOptions.selectField.options.values = options
-          .sort((a, b) => a.value.localeCompare(b.value));
-      }
+  const formatCustomFieldsForBackend = (customFieldsFormData) => {
+    if (Array.isArray(customFieldsFormData.customFields)) {
+      return customFieldsFormData.customFields.map(({ values: { hidden, ...customFieldData }, id }) => {
+        const customFieldDataWithSortedOptions = cloneDeep(customFieldData);
+
+        if (fieldTypesWithOptions.includes(customFieldData.type)) {
+          const options = customFieldDataWithSortedOptions.selectField.options.values;
+          customFieldDataWithSortedOptions.selectField.options.values = options
+            .sort((a, b) => a.value.localeCompare(b.value));
+        }
 
-      return ({
-        id: id.startsWith('unsaved_') ? null : id,
-        ...customFieldDataWithSortedOptions,
-        visible: !hidden,
+        return ({
+          id: id.startsWith('unsaved_') ? null : id,
+          ...customFieldDataWithSortedOptions,
+          visible: !hidden,
+        });
       });
-    });
+    }
+
+    return [];
   };
 
   const formatCustomFieldsForSaving = customFieldsFormData => {
-    return customFieldsFormData.customFields.map(({ values, ...rest }) => ({
-      ...rest,
-      ...values,
-    }));
+    if (Array.isArray(customFieldsFormData.customFields)) {
+      return customFieldsFormData.customFields.map(({ values, ...rest }) => ({
+        ...rest,
+        ...values,
+      }));
+    }
+
+    return [];
   };
 
   const saveCustomFields = async (data, redirectToView = true, titleShouldUpdate = true) => {

diff --git a/lib/CustomFields/pages/EditCustomFieldsSettings/tests/EditCustomFieldsSettings-test.js b/lib/CustomFields/pages/EditCustomFieldsSettings/tests/EditCustomFieldsSettings-test.js
@@ -124,40 +124,104 @@ describe('EditCustomFieldsSettings', () => {
     });
 
     // these tests began failing on master consistently for no clear reason
-    // after PR #1126 merged, despited the fact that it build cleanly on
+    // after PR #1126 merged, despite the fact that it build cleanly on
     // the release branch :rage: rage against the dying of the build
+    //
+    // 2023-09-15 update: the failure appears related to the "save" button not
+    // becoming enabled when a field is removed, thus preventing the callback
+    // that triggers the modal from firing. it works as expected in production but
+    // fails here under test.
+    //
+    // I added a sanity check, validating that the save and cancel buttons
+    // are enabled immediately after removing a field and then again before
+    // clicking the form's delete button, but it fails. IOW, without any actions,
+    // the same assertion succeeds and then fails. I fold.
     describe.skip('and it is saved', () => {
       beforeEach(async () => {
+        await editCustomFields.customFields(0).expand();
+        await editCustomFields.customFields(0).fillName('a');
+        await editCustomFields.customFields(0).fillHelpText('b');
         await editCustomFields.customFields(0).delete();
-        await editCustomFields.save();
+
+        await editCustomFields.customFields(1).expand();
+        await editCustomFields.customFields(1).fillName('a');
+        await editCustomFields.customFields(1).fillHelpText('b');
       });
 
-      it('should show Delete modal', () => {
-        expect(editCustomFields.deleteModal.modalIsPresent).to.be.true;
+      it('form\'s save and delete buttons should be enabled', () => {
+        expect(editCustomFields.saveButtonDisabled).to.be.false;
+        expect(editCustomFields.cancelButtonDisabled).to.be.false;
       });
 
-      describe('and when delete is cancelled', () => {
+      describe('clicking form\'s delete button', () => {
         beforeEach(async () => {
-          await editCustomFields.deleteModal.cancelButton.click();
+          expect(editCustomFields.saveButtonDisabled).to.be.false;
+          expect(editCustomFields.cancelButtonDisabled).to.be.false;
+
+          await editCustomFields.save();
         });
 
-        it('should not remove this custom field', () => {
-          expect(editCustomFields.customFields().length).to.equal(5);
+        it('should show confirm-delete modal', () => {
+          expect(editCustomFields.deleteModal.modalIsPresent).to.be.true;
         });
-      });
 
-      describe('and when delete is confirmed', () => {
-        beforeEach(async () => {
-          await editCustomFields.deleteModal.confirmButton.click();
+        describe('and when delete is cancelled', () => {
+          beforeEach(async () => {
+            await editCustomFields.deleteModal.cancelButton.click();
+          });
+
+          it('should not remove this custom field', () => {
+            expect(editCustomFields.customFields().length).to.equal(5);
+          });
         });
 
-        it('should remove this custom field', () => {
-          expect(editCustomFields.customFields().length).to.equal(4);
+        describe('and when delete is confirmed', () => {
+          beforeEach(async () => {
+            await editCustomFields.deleteModal.confirmButton.click();
+          });
+
+          it('should remove this custom field', () => {
+            expect(editCustomFields.customFields().length).to.equal(4);
+          });
+
+          it('should redirect to view route', function () {
+            expect(this.location.pathname).to.equal('/custom-fields-view');
+          });
         });
       });
     });
   });
 
+  // as above, delete tests fail, hence disabling this block as well.
+  describe.skip('when deleting all custom fields', () => {
+    beforeEach(async () => {
+      const limit = editCustomFields.customFields().length;
+      for (let i = 0; i < limit; i++) {
+        await editCustomFields.customFields(i).delete();
+      }
+      await editCustomFields.save();
+    });
+
+    it('should show Delete modal', () => {
+      expect(editCustomFields.deleteModal.modalIsPresent).to.be.true;
+    });
+
+    describe('and when delete is confirmed', () => {
+      beforeEach(async () => {
+        await editCustomFields.deleteModal.confirmButton.click();
+      });
+
+      it('should remove all custom fields', () => {
+        expect(editCustomFields.customFields().length).to.equal(0);
+      });
+
+      it('should redirect to view route', function () {
+        expect(this.location.pathname).to.equal('/custom-fields-view');
+      });
+    });
+  });
+
+
   describe('when creating a custom field with options', () => {
     beforeEach(async () => {
       await editCustomFields.addFieldButton.selectCustomFieldType(1);

diff --git a/lib/CustomFields/pages/EditCustomFieldsSettings/tests/interactor.js b/lib/CustomFields/pages/EditCustomFieldsSettings/tests/interactor.js
@@ -30,6 +30,7 @@ export default interactor(class EditCustomFieldsSettings {
 
   customFields = collection('[data-test-accordion-section]', {
     delete: clickable('[data-test-custom-field-delete-button]'),
+    expand: clickable('button'),
     fillName: fillable('[data-test-custom-fields-name-input]'),
     fillHelpText: fillable('[data-test-custom-fields-help-text-input]'),
     checkHidden: clickable('[data-test-custom-fields-hidden-checkbox]'),

diff --git a/tests/network/config.js b/tests/network/config.js
@@ -253,6 +253,8 @@ export default function config() {
     id: request.params.id,
   }));
 
+  this.get('/custom-fields/:id/options/:opt_id/stats', (_, _request) => ({}));
+
   this.get('/notes/:id', ({ notes }, { params }) => {
     return notes.find(params.id);
   });