Skip to content

Commit 4db3e5e

Browse files
authored
Avoid unintended side-effects on non-interactive library disassociations (PP-2408) (#171)
## Description Use newly-added `isLibraryRemovalPermitted` (instead of `removeLibrary`) to confirm interactive removals of library associations from collections. NB: This PR is ready for review; however, it depends on #170 for `isLibraryRemovalPermitted`, so this PR should not be merged until that one is landed. ## Motivation and Context Confirmation popups were rendering for each associated library when the `CollectionEditForm` was being cleared. This is because `removeLibrary` is used both interactively -- when library `Delete` button is clicked -- and non-interactively, when the form is cleared. [Jira PP-2408] ## How Has This Been Tested? - Testing in local environment. - All tests pass locally. - [CI tests](https://github.com/ThePalaceProject/circulation-admin/actions/runs/17103881259) pass. ## Checklist: - [x] I have updated the documentation accordingly. - [x] All new and existing tests passed.
1 parent 2a9b7a6 commit 4db3e5e

File tree

2 files changed

+38
-17
lines changed

2 files changed

+38
-17
lines changed

src/components/Collections.tsx

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -39,19 +39,17 @@ export class CollectionEditForm extends ServiceWithRegistrationsEditForm<
3939
> {
4040
/**
4141
* Override to display a confirmation message before removing a library
42-
* association. We display the confirmation and, if successful, call the
43-
* superclass method to actually remove the library from our state.
44-
* @param library
42+
* association. We display the confirmation and return `true` if the
43+
* action is confirmed or `false` otherwise.
44+
* @param library The library to remove.
4545
*/
46-
removeLibrary(library: LibraryWithSettingsData) {
46+
isLibraryRemovalPermitted(library: LibraryWithSettingsData): boolean {
4747
const libraryData = this.getLibrary(library.short_name);
4848
const libraryName = libraryData ? libraryData.name : library.short_name;
4949
const confirmationMessage =
5050
`Disassociating library "${libraryName}" from this collection will ` +
5151
"remove all loans and holds for its patrons. Do you wish to continue?";
52-
if (window.confirm(confirmationMessage)) {
53-
super.removeLibrary(library);
54-
}
52+
return window.confirm(confirmationMessage);
5553
}
5654
}
5755

src/components/__tests__/Collections-test.tsx

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,7 @@ describe("Collections", () => {
133133
describe("confirm before disassociating libraries", () => {
134134
let wrapper: ReactWrapper;
135135
let confirmStub: sinon.SinonStub;
136+
let instance: CollectionEditForm;
136137

137138
const initialLibraries = [
138139
{ short_name: "palace", name: "Palace" },
@@ -142,13 +143,11 @@ describe("Collections", () => {
142143
id: 7,
143144
name: "An OPDS Collection",
144145
protocol: "OPDS Import",
145-
146146
libraries: [...initialLibraries],
147147
};
148148

149149
beforeEach(() => {
150150
confirmStub = sinon.stub(window, "confirm");
151-
152151
wrapper = mount(
153152
<CollectionEditForm
154153
disabled={false}
@@ -162,40 +161,64 @@ describe("Collections", () => {
162161
listDataKey="collections"
163162
/>
164163
);
164+
instance = wrapper.instance() as CollectionEditForm;
165165
});
166166

167167
afterEach(() => {
168168
confirmStub.restore();
169169
});
170170

171-
it("calls window.confirm when delete button is clicked", () => {
171+
it("prompts for confirmation before removing a library", () => {
172172
confirmStub.returns(false);
173173

174174
// The confirmation dialog should not be invoked before we click.
175-
expect(confirmStub.calledOnce).to.be.false;
175+
expect(confirmStub.called).to.be.false;
176176

177177
wrapper.find("button.remove-btn").at(0).simulate("click");
178178
expect(confirmStub.calledOnce).to.be.true;
179-
expect(confirmStub.firstCall.args.length).to.equal(1);
180179
const message: string = confirmStub.firstCall.args[0];
181180
expect(message).to.equal(
182181
'Disassociating library "Palace" from this collection will ' +
183182
"remove all loans and holds for its patrons. Do you wish to continue?"
184183
);
185184
});
186185

187-
it("does not delete library if confirmation is canceled", () => {
186+
it("removes library if confirmation is accepted", () => {
187+
confirmStub.returns(true);
188+
wrapper.find("button.remove-btn").at(0).simulate("click");
189+
190+
// Ensure confirmation was sought.
191+
expect(confirmStub.calledOnce).to.be.true;
192+
193+
// We deleted the first library, so it should be gone from the state.
194+
expect(wrapper.state("libraries")).to.deep.equal([initialLibraries[1]]);
195+
});
196+
197+
it("does not remove library if confirmation is canceled", () => {
188198
confirmStub.returns(false);
189199
wrapper.find("button.remove-btn").at(0).simulate("click");
200+
201+
// Ensure confirmation was sought.
202+
expect(confirmStub.calledOnce).to.be.true;
203+
190204
// We didn't delete, so we should still have the originals.
191205
expect(wrapper.state("libraries")).to.deep.equal(initialLibraries);
192206
});
193207

194-
it("deletes library if confirmation is accepted", () => {
195-
confirmStub.returns(true);
208+
it("uses library short_name in confirmation when full name is not available", () => {
209+
const getLibraryStub = stub(instance, "getLibrary").returns(null);
210+
confirmStub.returns(false);
211+
196212
wrapper.find("button.remove-btn").at(0).simulate("click");
197-
// We deleted the first library, so it should be gone from the state.
198-
expect(wrapper.state("libraries")).to.deep.equal([initialLibraries[1]]);
213+
214+
expect(confirmStub.calledOnce).to.be.true;
215+
const message: string = confirmStub.firstCall.args[0];
216+
const libraryShortName = initialLibraries[0].short_name;
217+
expect(message).to.equal(
218+
`Disassociating library "${libraryShortName}" from this collection will ` +
219+
"remove all loans and holds for its patrons. Do you wish to continue?"
220+
);
221+
getLibraryStub.restore();
199222
});
200223
});
201224
});

0 commit comments

Comments
 (0)