diff --git a/chrome/content/zotero/xpcom/data/dataObject.js b/chrome/content/zotero/xpcom/data/dataObject.js index 56fb3168c3..03fb7970a0 100644 --- a/chrome/content/zotero/xpcom/data/dataObject.js +++ b/chrome/content/zotero/xpcom/data/dataObject.js @@ -987,17 +987,17 @@ Zotero.DataObject.prototype.save = async function (options = {}) { ].forEach(x => env.options[x] = true); } - var proceed = await this._initSave(env); - if (!proceed) return false; - - if (env.isNew) { - Zotero.debug('Saving data for new ' + this._objectType + ' to database', 4); - } - else { - Zotero.debug('Updating database with new ' + this._objectType + ' data', 4); - } - try { + var proceed = await this._initSave(env); + if (!proceed) return false; + + if (env.isNew) { + Zotero.debug('Saving data for new ' + this._objectType + ' to database', 4); + } + else { + Zotero.debug('Updating database with new ' + this._objectType + ' data', 4); + } + if (Zotero.DataObject.prototype._finalizeSave == this._finalizeSave) { throw new Error("_finalizeSave not implemented for Zotero." + this._ObjectType); } diff --git a/test/tests/undoHistoryTest.js b/test/tests/undoHistoryTest.js index 3414194aa7..273b381a10 100644 --- a/test/tests/undoHistoryTest.js +++ b/test/tests/undoHistoryTest.js @@ -251,6 +251,34 @@ describe("Zotero.UndoHistory", function () { assert.isFalse(Zotero.UndoHistory.canUndo()); assert.isFalse(Zotero.UndoHistory.canRedo()); }); + + it("should not leave an object unsaveable after an undo apply failure", async function () { + // A (move target), P (original parent), B (child being moved) + let collectionA = await createDataObject('collection', { name: 'A' }); + let collectionP = await createDataObject('collection', { name: 'P' }); + let collectionB = await createDataObject('collection', { name: 'B', parentID: collectionP.id }); + + // Move B from P onto A, recording an undo entry for the parent change + Zotero.UndoHistory.clear(); + collectionB.parentID = collectionA.id; + await collectionB.saveTx({ undoAction: 'undo-action-move-collection' }); + assert.isTrue(Zotero.UndoHistory.canUndo()); + + // Permanently erase P so its key no longer resolves to a collection + await collectionP.eraseTx(); + + // Undoing tries to set B's parent back to the now-erased P, which makes + // Collection._initSave throw. The apply fails and history is cleared, but + // B must not be left pinned to the vanished parent. + await Zotero.UndoHistory.undo(); + + // B should have been rolled back to its last valid parent (A) and remain + // editable + assert.equal(collectionB.parentID, collectionA.id); + collectionB.name = 'B renamed'; + await collectionB.saveTx(); + assert.equal(collectionB.name, 'B renamed'); + }); }); describe("canUndo/canRedo", function () {