Avoid unintended side-effects on non-interactive library disassociations (PP-2408)

src/components/Collections.tsx

@@ -39,19 +39,17 @@ export class CollectionEditForm extends ServiceWithRegistrationsEditForm<
 > {
   /**
    * Override to display a confirmation message before removing a library
-   * association. We display the confirmation and, if successful, call the
-   * superclass method to actually remove the library from our state.
-   * @param library
+   * association. We display the confirmation and return `true` if the
+   * action is confirmed or `false` otherwise.
+   * @param library The library to remove.
    */
-  removeLibrary(library: LibraryWithSettingsData) {
+  isLibraryRemovalPermitted(library: LibraryWithSettingsData): boolean {
     const libraryData = this.getLibrary(library.short_name);
     const libraryName = libraryData ? libraryData.name : library.short_name;
     const confirmationMessage =
       `Disassociating library "${libraryName}" from this collection will ` +
       "remove all loans and holds for its patrons. Do you wish to continue?";
-    if (window.confirm(confirmationMessage)) {
-      super.removeLibrary(library);
-    }
+    return window.confirm(confirmationMessage);
   }
 }
 

src/components/ServiceEditForm.tsx

@@ -10,7 +10,6 @@ import {
   ProtocolData,
   ServiceData,
   ServicesData,
-  SettingData,
 } from "../interfaces";
 import { clearForm } from "../utils/sharedFunctions";
 import { FetchErrorData } from "@thepalaceproject/web-opds-client/lib/interfaces";
@@ -27,6 +26,7 @@ export interface ServiceEditFormProps<T> {
   extraFormSection?: any;
   extraFormKey?: string;
   adminLevel?: number;
+  libraryRemovalAllowed?: (library: LibraryWithSettingsData) => boolean;
 }
 
 export interface ServiceEditFormState {
@@ -345,6 +345,7 @@ export default class ServiceEditForm<
               <WithRemoveButton
                 disabled={disabled}
                 onRemove={() => this.removeLibrary(library)}
+                confirmRemoval={() => this.isLibraryRemovalPermitted(library)}
                 ref={library.short_name}
               >
                 {this.props.data &&
@@ -588,6 +589,31 @@ export default class ServiceEditForm<
     return this.state.expandedLibraries.indexOf(library.short_name) !== -1;
   }
 
+  /**
+   * Controls whether library associations may be removed via UI interaction.
+   * Subclasses may override this method to control removal.
+   * @param library The library to remove.
+   */
+  isLibraryRemovalPermitted(library: LibraryWithSettingsData): boolean {
+    // library should be provided on every call.
+    // It is not used here, but might be in subclass implementations.
+    // Removal is permitted by default.
+    return true;
+  }
+
+  /**
+   * Removes the association with the given library from a service's state.
+   *
+   * NB: This method is used indirectly via `onRemove` by `sharedFunctions.clearForm`
+   * to clean up form state; therefore, care should be taken to avoid any side
+   * effects not needed for updating the state. Anything done by this method
+   * can happen whenever user interaction or `clearForm` triggers `onRemove`.
+   *
+   * Update or override `isLibraryRemovalPermitted` to control library removal
+   * via UI interaction.
+   *
+   * @param library
+   */
   removeLibrary(library) {
     const libraries = this.state.libraries.filter(
       (stateLibrary) => stateLibrary.short_name !== library.short_name

src/components/WithRemoveButton.tsx

@@ -5,6 +5,7 @@ import TrashIcon from "./icons/TrashIcon";
 export interface WithRemoveButtonProps {
   disabled: boolean;
   onRemove: () => void;
+  confirmRemoval?: () => boolean;
 }
 
 /** When wrapped around an element, renders a remove button next to the element. */
@@ -37,6 +38,12 @@ export default class WithRemoveButton extends React.Component<
 
   onClick(e: Event) {
     e.preventDefault();
+
+    // Don't remove if confirmation function is present and returns false.
+    if (this.props.confirmRemoval && !this.props.confirmRemoval()) {
+      return;
+    }
+    // Otherwise, we can proceed with removal.
     !this.props.disabled && this.props.onRemove();
   }
 }

src/components/__tests__/Collections-test.tsx

@@ -133,6 +133,7 @@ describe("Collections", () => {
     describe("confirm before disassociating libraries", () => {
       let wrapper: ReactWrapper;
       let confirmStub: sinon.SinonStub;
+      let instance: CollectionEditForm;
 
       const initialLibraries = [
         { short_name: "palace", name: "Palace" },
@@ -142,13 +143,11 @@ describe("Collections", () => {
         id: 7,
         name: "An OPDS Collection",
         protocol: "OPDS Import",
-
         libraries: [...initialLibraries],
       };
 
       beforeEach(() => {
         confirmStub = sinon.stub(window, "confirm");
-
         wrapper = mount(
           <CollectionEditForm
             disabled={false}
@@ -162,40 +161,64 @@ describe("Collections", () => {
             listDataKey="collections"
           />
         );
+        instance = wrapper.instance() as CollectionEditForm;
       });
 
       afterEach(() => {
         confirmStub.restore();
       });
 
-      it("calls window.confirm when delete button is clicked", () => {
+      it("prompts for confirmation before removing a library", () => {
         confirmStub.returns(false);
 
         // The confirmation dialog should not be invoked before we click.
-        expect(confirmStub.calledOnce).to.be.false;
+        expect(confirmStub.called).to.be.false;
 
         wrapper.find("button.remove-btn").at(0).simulate("click");
         expect(confirmStub.calledOnce).to.be.true;
-        expect(confirmStub.firstCall.args.length).to.equal(1);
         const message: string = confirmStub.firstCall.args[0];
         expect(message).to.equal(
           'Disassociating library "Palace" from this collection will ' +
             "remove all loans and holds for its patrons. Do you wish to continue?"
         );
       });
 
-      it("does not delete library if confirmation is canceled", () => {
+      it("removes library if confirmation is accepted", () => {
+        confirmStub.returns(true);
+        wrapper.find("button.remove-btn").at(0).simulate("click");
+
+        // Ensure confirmation was sought.
+        expect(confirmStub.calledOnce).to.be.true;
+
+        // We deleted the first library, so it should be gone from the state.
+        expect(wrapper.state("libraries")).to.deep.equal([initialLibraries[1]]);
+      });
+
+      it("does not remove library if confirmation is canceled", () => {
         confirmStub.returns(false);
         wrapper.find("button.remove-btn").at(0).simulate("click");
+
+        // Ensure confirmation was sought.
+        expect(confirmStub.calledOnce).to.be.true;
+
         // We didn't delete, so we should still have the originals.
         expect(wrapper.state("libraries")).to.deep.equal(initialLibraries);
       });
 
-      it("deletes library if confirmation is accepted", () => {
-        confirmStub.returns(true);
+      it("uses library short_name in confirmation when full name is not available", () => {
+        const getLibraryStub = stub(instance, "getLibrary").returns(null);
+        confirmStub.returns(false);
+
         wrapper.find("button.remove-btn").at(0).simulate("click");
-        // We deleted the first library, so it should be gone from the state.
-        expect(wrapper.state("libraries")).to.deep.equal([initialLibraries[1]]);
+
+        expect(confirmStub.calledOnce).to.be.true;
+        const message: string = confirmStub.firstCall.args[0];
+        const libraryShortName = initialLibraries[0].short_name;
+        expect(message).to.equal(
+          `Disassociating library "${libraryShortName}" from this collection will ` +
+            "remove all loans and holds for its patrons. Do you wish to continue?"
+        );
+        getLibraryStub.restore();
       });
     });
   });

src/components/__tests__/ServiceEditForm-test.tsx

@@ -1009,4 +1009,79 @@ describe("ServiceEditForm", () => {
       expect(nameInput.props().value).to.equal("new service");
     });
   });
+
+  describe("library removal confirmation", () => {
+    let serviceEditFormInstance;
+    let isLibraryRemovalPermittedSpy;
+    let removeLibrarySpy;
+
+    beforeEach(() => {
+      save = stub().returns(new Promise<void>((resolve) => resolve()));
+      wrapper = mount(
+        <TestServiceEditForm
+          disabled={false}
+          data={servicesData}
+          save={save}
+          item={serviceData}
+          urlBase={urlBase}
+          listDataKey="services"
+        />
+      );
+      serviceEditFormInstance = wrapper.instance();
+      removeLibrarySpy = spy(serviceEditFormInstance, "removeLibrary");
+    });
+
+    afterEach(() => {
+      isLibraryRemovalPermittedSpy.restore();
+      removeLibrarySpy.restore();
+    });
+
+    const shouldRemoveLibrary = () => {
+      const libraryToRemove = serviceData.libraries[0];
+      const removeButton = wrapper.find(WithRemoveButton).at(0).find("button.remove-btn");
+
+      removeButton.simulate("click");
+
+      expect(isLibraryRemovalPermittedSpy.calledOnce).to.be.true;
+      expect(isLibraryRemovalPermittedSpy.calledWith(libraryToRemove)).to.be.true;
+      expect(isLibraryRemovalPermittedSpy.returned(true)).to.be.true;
+
+      // Library removal should be performed.
+      expect(removeLibrarySpy.calledOnce).to.be.true;
+      expect(removeLibrarySpy.calledWith(libraryToRemove)).to.be.true;
+
+      // Ensure that the library was removed from state.
+      wrapper.update();
+      expect(wrapper.state("libraries")).to.deep.equal([]);
+    };
+
+    it("should allow removal by default", () => {
+      isLibraryRemovalPermittedSpy = spy(serviceEditFormInstance, "isLibraryRemovalPermitted");
+      shouldRemoveLibrary();
+    });
+
+    it("should allow remove if isLibraryRemovalPermitted returns true", () => {
+      isLibraryRemovalPermittedSpy = stub(serviceEditFormInstance, "isLibraryRemovalPermitted").returns(true);
+      shouldRemoveLibrary();
+    });
+
+    it("should not allow remove if isLibraryRemovalPermitted returns false", () => {
+      isLibraryRemovalPermittedSpy = stub(serviceEditFormInstance, "isLibraryRemovalPermitted").returns(false);
+      const libraryToRemove = serviceData.libraries[0];
+      const removeButton = wrapper.find(WithRemoveButton).at(0).find("button.remove-btn");
+
+      removeButton.simulate("click");
+
+      expect(isLibraryRemovalPermittedSpy.calledOnce).to.be.true;
+      expect(isLibraryRemovalPermittedSpy.calledWith(libraryToRemove)).to.be.true;
+      expect(isLibraryRemovalPermittedSpy.returned(false)).to.be.true;
+
+      // Library disassociation should NOT be performed.
+      expect(removeLibrarySpy.notCalled).to.be.true;
+
+      // Ensure that the library was NOT removed from state.
+      wrapper.update();
+      expect(wrapper.state("libraries")).to.deep.equal(serviceData.libraries);
+    });
+  });
 });

src/components/__tests__/WithRemoveButton-test.tsx

@@ -43,5 +43,27 @@ describe("WithRemoveButton", () => {
       removeBtn.simulate("click");
       expect(onRemove.callCount).to.equal(0);
     });
+
+    it("calls onRemove when confirmation returns true", () => {
+      const confirmRemoval = stub().returns(true);
+      wrapper.setProps({ confirmRemoval });
+      const removeBtn = wrapper.find(".remove-btn").hostNodes();
+
+      removeBtn.simulate("click");
+
+      expect(confirmRemoval.callCount).to.equal(1);
+      expect(onRemove.callCount).to.equal(1);
+    });
+
+    it("does not call onRemove when confirmation returns false", () => {
+      const confirmRemoval = stub().returns(false);
+      wrapper.setProps({ confirmRemoval });
+      const removeBtn = wrapper.find(".remove-btn").hostNodes();
+
+      removeBtn.simulate("click");
+
+      expect(confirmRemoval.callCount).to.equal(1);
+      expect(onRemove.callCount).to.equal(0);
+    });
   });
 });