Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(v2): Toggle off Auto Save and warn user after save error #3359

Merged
merged 11 commits into from
Dec 16, 2024
1 change: 1 addition & 0 deletions packages/zowe-explorer/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ All notable changes to the "vscode-extension-for-zowe" extension will be documen
- Fixed an issue where data set nodes did not update if migrated or recalled outside of Zowe Explorer. [#3294](https://github.com/zowe/zowe-explorer-vscode/issues/3294)
- Fixed an issue where clicking on a file in the Unix System Services tree caused the tree to abruptly change focus to the selected item. [#2486](https://github.com/zowe/zowe-explorer-vscode/issues/2486)
- Fixed an issue where binary USS files were not fetched using the "Pull from Mainframe" context menu option. [#3355](https://github.com/zowe/zowe-explorer-vscode/issues/3355)
- Fixed an issue with Auto Save where a failed UNIX file or data set save operation caused an infinite loop of save requests. [#2406](https://github.com/zowe/zowe-explorer-vscode/issues/2406), [#2627](https://github.com/zowe/zowe-explorer-vscode/issues/2627)

## `2.18.0`

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { createUSSTree } from "../../../__mocks__/mockCreators/uss";
import { saveUSSFile } from "../../../src/uss/actions";
import * as workspaceUtils from "../../../src/utils/workspace";
import { Gui } from "@zowe/zowe-explorer-api";
import * as globals from "../../../src/globals";
import * as vscode from "vscode";
import { ZoweLogger } from "../../../src/utils/LoggerUtils";

Expand All @@ -24,6 +23,7 @@ describe("ZoweSaveQueue - unit tests", () => {
const globalMocks = {
errorMessageSpy: jest.spyOn(Gui, "errorMessage"),
markDocumentUnsavedSpy: jest.spyOn(workspaceUtils, "markDocumentUnsaved"),
handleAutoSaveOnErrorMock: jest.spyOn(workspaceUtils, "handleAutoSaveOnError").mockImplementation(),
processNextSpy: jest.spyOn(ZoweSaveQueue as any, "processNext"),
allSpy: jest.spyOn(ZoweSaveQueue, "all"),
trees: {
Expand Down Expand Up @@ -118,4 +118,46 @@ describe("ZoweSaveQueue - unit tests", () => {
);
}
});

describe("markAllUnsaved", () => {
function getBlockMocks(): Record<string, jest.SpyInstance> {
return {
markDocumentUnsaved: jest.spyOn(workspaceUtils, "markDocumentUnsaved").mockClear().mockImplementation(),
};
}

it("marks all documents as unsaved in the queue", async () => {
const blockMocks = getBlockMocks();
(ZoweSaveQueue as any).savingQueue = [
{
savedFile: { fileName: "some.jcl" },
} as any,
{
savedFile: { fileName: "SOME.C.DS(MEM).c" } as any,
} as any,
];
await ZoweSaveQueue.markAllUnsaved();
expect(blockMocks.markDocumentUnsaved).toHaveBeenCalledTimes(2);
});

it("clears the queue if clearQueue is true", async () => {
(ZoweSaveQueue as any).savingQueue = [
{
savedFile: { fileName: "some.jcl" },
} as any,
];
await ZoweSaveQueue.markAllUnsaved();
expect((ZoweSaveQueue as any).savingQueue.length).toBe(0);
});

it("does not clear the queue if clearQueue is false", async () => {
(ZoweSaveQueue as any).savingQueue = [
{
savedFile: { fileName: "some.jcl" },
} as any,
];
await ZoweSaveQueue.markAllUnsaved(false);
expect((ZoweSaveQueue as any).savingQueue.length).toBe(1);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import { getNodeLabels } from "../../../src/dataset/utils";
import { ZoweLogger } from "../../../src/utils/LoggerUtils";
import { mocked } from "../../../__mocks__/mockUtils";
import { LocalFileManagement } from "../../../src/utils/LocalFileManagement";
import * as workspaceUtils from "../../../src/utils/workspace";

// Missing the definition of path module, because I need the original logic for tests
jest.mock("fs");
Expand Down Expand Up @@ -1073,6 +1074,7 @@ describe("Dataset Actions Unit Tests - Function saveFile", () => {
profileInstance,
testDatasetTree,
uploadContentSpy,
handleAutoSaveOnError: jest.spyOn(workspaceUtils, "handleAutoSaveOnError").mockImplementation(),
};
}

Expand Down Expand Up @@ -1238,6 +1240,7 @@ describe("Dataset Actions Unit Tests - Function saveFile", () => {
(testDocument as any).fileName = path.join(globals.DS_DIR, testDocument.fileName);

await dsActions.saveFile(testDocument, blockMocks.testDatasetTree);
expect(blockMocks.handleAutoSaveOnError).toHaveBeenCalled();

expect(globalMocks.concatChildNodes).toBeCalled();
expect(mocked(Gui.errorMessage)).toBeCalledWith("failed");
Expand Down
21 changes: 21 additions & 0 deletions packages/zowe-explorer/__tests__/__unit__/uss/actions.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import * as vscode from "vscode";
import * as path from "path";
import * as globals from "../../../src/globals";
import * as sharedUtils from "../../../src/shared/utils";
import * as workspaceUtils from "../../../src/utils/workspace";
import { ZoweUSSNode } from "../../../src/uss/ZoweUSSNode";
import * as isbinaryfile from "isbinaryfile";
import * as fs from "fs";
Expand All @@ -41,6 +42,7 @@ import { ZoweLogger } from "../../../src/utils/LoggerUtils";
import { AttributeView } from "../../../src/uss/AttributeView";
import { mocked } from "../../../__mocks__/mockUtils";
import { LocalFileManagement } from "../../../src/utils/LocalFileManagement";
import { SettingsConfig } from "../../../src/utils/SettingsConfig";

function createGlobalMocks() {
const globalMocks = {
Expand Down Expand Up @@ -461,6 +463,9 @@ describe("USS Action Unit Tests - Function saveUSSFile", () => {
ussNode: createUSSNode(globalMocks.testSession, globalMocks.testProfile),
ussFavoriteNode: createFavoriteUSSNode(globalMocks.testSession, globalMocks.testProfile),
putUSSPayload: jest.fn().mockResolvedValue(`{"stdout":[""]}`),
handleAutoSaveOnError: jest.spyOn(workspaceUtils, "handleAutoSaveOnError"),
// Disable auto save for most of the test cases
getDirectValue: jest.spyOn(SettingsConfig, "getDirectValue").mockReturnValueOnce("off"),
};

newMocks.node = new ZoweUSSNode({
Expand Down Expand Up @@ -544,6 +549,21 @@ describe("USS Action Unit Tests - Function saveUSSFile", () => {
);
});

it("calls handleAutoSaveOnError in the event of an error", async () => {
const globalMocks = createGlobalMocks();
const blockMocks = await createBlockMocks(globalMocks);

globalMocks.withProgress.mockImplementation((progLocation, callback) => callback());
globalMocks.fileToUSSFile.mockResolvedValue(blockMocks.testResponse);
blockMocks.testResponse.success = false;
blockMocks.testResponse.commandResponse = "Save failed";

globalMocks.withProgress.mockReturnValueOnce(blockMocks.testResponse);

await ussNodeActions.saveUSSFile(blockMocks.testDoc, blockMocks.testUSSTree);
expect(blockMocks.handleAutoSaveOnError).toHaveBeenCalled();
});

it("Tests that saveUSSFile fails when save fails", async () => {
const globalMocks = createGlobalMocks();
const blockMocks = await createBlockMocks(globalMocks);
Expand All @@ -570,6 +590,7 @@ describe("USS Action Unit Tests - Function saveUSSFile", () => {
globalMocks.withProgress.mockRejectedValueOnce(Error("Test Error"));

await ussNodeActions.saveUSSFile(blockMocks.testDoc, blockMocks.testUSSTree);
expect(blockMocks.handleAutoSaveOnError).toHaveBeenCalled();
expect(globalMocks.showErrorMessage.mock.calls.length).toBe(1);
expect(globalMocks.showErrorMessage.mock.calls[0][0]).toBe("Error: Test Error");
expect(mocked(vscode.workspace.applyEdit)).toHaveBeenCalledTimes(2);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
import * as vscode from "vscode";
import * as workspaceUtils from "../../../src/utils/workspace";
import { workspaceUtilMaxEmptyWindowsInTheRow } from "../../../src/config/constants";
import { SettingsConfig } from "../../../src/utils/SettingsConfig";
import { ZoweSaveQueue } from "../../../src/abstract/ZoweSaveQueue";
import { Gui } from "@zowe/zowe-explorer-api";

function createGlobalMocks() {
const activeTextEditor = jest.fn();
Expand Down Expand Up @@ -216,3 +219,70 @@ describe("Workspace Utils Unit Tests - function awaitForDocumentBeingSaved", ()
await expect(workspaceUtils.awaitForDocumentBeingSaved()).resolves.not.toThrow();
});
});

describe("Workspace Utils Unit Tests - function handleAutoSaveOnError", () => {
function getBlockMocks(autoSaveEnabled: boolean = true, userResponse?: string): Record<string, jest.SpyInstance> {
const executeCommand = jest.spyOn(vscode.commands, "executeCommand").mockClear();
const getDirectValue = jest.spyOn(SettingsConfig, "getDirectValue");
if (autoSaveEnabled) {
executeCommand.mockResolvedValueOnce(undefined);
getDirectValue.mockReturnValueOnce("afterDelay");
} else {
getDirectValue.mockReturnValueOnce("off");
}

if (userResponse === "Enable Auto Save") {
executeCommand.mockResolvedValueOnce(undefined);
}

return {
errorMessage: jest.spyOn(Gui, "errorMessage").mockResolvedValueOnce(userResponse),
executeCommand,
getDirectValue,
markAllUnsaved: jest.spyOn(ZoweSaveQueue, "markAllUnsaved"),
};
}

it("returns early if Auto Save is disabled", async () => {
const blockMocks = getBlockMocks(false);
await workspaceUtils.handleAutoSaveOnError();
expect(blockMocks.getDirectValue).toHaveBeenCalledWith("files.autoSave");
expect(blockMocks.executeCommand).not.toHaveBeenCalled();
});

it("toggles off auto save if enabled", async () => {
const blockMocks = getBlockMocks(true);
await workspaceUtils.handleAutoSaveOnError();
expect(blockMocks.executeCommand).toHaveBeenCalledWith("workbench.action.toggleAutoSave");
});

it("calls ZoweSaveQueue.markAllUnsaved to mark documents unsaved and clear queue", async () => {
const blockMocks = getBlockMocks(true);
await workspaceUtils.handleAutoSaveOnError();
expect(blockMocks.markAllUnsaved).toHaveBeenCalled();
});

it("prompts the user to reactivate Auto Save in event of save error", async () => {
const blockMocks = getBlockMocks(true);
await workspaceUtils.handleAutoSaveOnError();
expect(blockMocks.errorMessage).toHaveBeenCalledWith(
"Zowe Explorer encountered a save error and has disabled Auto Save. Once the issue is addressed, enable Auto Save and try again.",
{ items: ["Enable Auto Save"] }
);
});

it("does nothing if 'Enable Auto Save' clicked and Auto Save is already on", async () => {
const blockMocks = getBlockMocks(true, "Enable Auto Save");
await workspaceUtils.handleAutoSaveOnError();
expect(blockMocks.executeCommand).toHaveBeenCalledWith("workbench.action.toggleAutoSave");
expect(blockMocks.executeCommand).toHaveBeenCalledTimes(1);
});

it("reactivates Auto Save if 'Enable Auto Save' clicked", async () => {
const blockMocks = getBlockMocks(true, "Enable Auto Save");
blockMocks.getDirectValue.mockReturnValueOnce("off");
await workspaceUtils.handleAutoSaveOnError();
expect(blockMocks.executeCommand).toHaveBeenCalledTimes(2);
expect(blockMocks.executeCommand).toHaveBeenCalledWith("workbench.action.toggleAutoSave");
});
});
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{
"Favorites": "Favorites",
"renameUSS.unsavedWork": "Unable to rename {0} because you have unsaved changes in this {1}. Please save your work before renaming the {1}.",
"renameUSS.enterName": "Enter a new name for the {0}",
"renameUSS.error": "Unable to rename node:",
"renameUSS.duplicateName": "A {0} already exists with this name. Please choose a different name.",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"uss.renameNode": "Rename",
"unsavedChanges.errorMsg": "Unable to {0} {1} because you have unsaved changes in this {2}. Please save your work and try again."
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"zowe.autoSaveToggled": "Zowe Explorer encountered a save error and has disabled Auto Save. Once the issue is addressed, enable Auto Save and try again.",
"zowe.enableAutoSave": "Enable Auto Save"
}
17 changes: 16 additions & 1 deletion packages/zowe-explorer/src/abstract/ZoweSaveQueue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import * as path from "path";
import * as vscode from "vscode";
import { Gui, IZoweTree, IZoweTreeNode } from "@zowe/zowe-explorer-api";
import { markDocumentUnsaved } from "../utils/workspace";
import { handleAutoSaveOnError, markDocumentUnsaved } from "../utils/workspace";
import { ZoweLogger } from "../utils/LoggerUtils";

// Set up localization
Expand Down Expand Up @@ -56,6 +56,20 @@ export class ZoweSaveQueue {
private static ongoingSave = Promise.resolve();
private static savingQueue: SaveRequest[] = [];

/**
* Marks all documents in the save queue as unsaved.
* @param clearQueue Whether to clear the queue after marking the documents as unsaved (default: `true`)
*/
public static async markAllUnsaved(clearQueue: boolean = true): Promise<void> {
for (const { savedFile } of this.savingQueue) {
await markDocumentUnsaved(savedFile);
}

if (clearQueue) {
this.savingQueue = [];
}
}

/**
* Iterate over the queue and process next item until it is empty.
*/
Expand All @@ -71,6 +85,7 @@ export class ZoweSaveQueue {
} catch (err) {
ZoweLogger.error(err);
await markDocumentUnsaved(nextRequest.savedFile);
await handleAutoSaveOnError();
await Gui.errorMessage(
localize(
"processNext.error.uploadFailed",
Expand Down
5 changes: 4 additions & 1 deletion packages/zowe-explorer/src/dataset/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
import { getIconByNode } from "../generators/icons";
import { ZoweDatasetNode } from "./ZoweDatasetNode";
import * as contextually from "../shared/context";
import { markDocumentUnsaved, setFileSaved } from "../utils/workspace";
import { handleAutoSaveOnError, markDocumentUnsaved, setFileSaved } from "../utils/workspace";
import { IUploadOptions } from "@zowe/zos-files-for-zowe-sdk";
import { ZoweLogger } from "../utils/LoggerUtils";
import { ProfileManagement } from "../utils/ProfileManagement";
Expand Down Expand Up @@ -246,7 +246,7 @@
* @param {IZoweDatasetTreeNode} node - The node selected for deletion
* @param {DatasetTree} datasetProvider - the tree which contains the nodes
*/
export async function deleteDatasetPrompt(datasetProvider: api.IZoweTree<api.IZoweDatasetTreeNode>, node?: api.IZoweDatasetTreeNode): Promise<void> {

Check warning on line 249 in packages/zowe-explorer/src/dataset/actions.ts

View workflow job for this annotation

GitHub Actions / lint

Async function 'deleteDatasetPrompt' has a complexity of 25. Maximum allowed is 15
ZoweLogger.trace("dataset.actions.deleteDatasetPrompt called.");
let nodes: api.IZoweDatasetTreeNode[];
const treeView = datasetProvider.getTreeView();
Expand Down Expand Up @@ -643,7 +643,7 @@
const templates = datasetProvider.getDsTemplates();
const templateNames: string[] = [];
templates?.forEach((template) => {
Object.entries(template).forEach(([key, value]) => {

Check warning on line 646 in packages/zowe-explorer/src/dataset/actions.ts

View workflow job for this annotation

GitHub Actions / lint

'value' is defined but never used. Allowed unused args must match /^_/u
templateNames.push(key);
});
});
Expand Down Expand Up @@ -688,7 +688,7 @@
if (template[key].dsorg === "PS") {
typeEnum = 4;
} else {
typeEnum = 3;

Check warning on line 691 in packages/zowe-explorer/src/dataset/actions.ts

View workflow job for this annotation

GitHub Actions / lint

No magic number: 3
}
propertiesFromDsType = value;
}
Expand Down Expand Up @@ -1510,7 +1510,7 @@
* @export
* @param {vscode.TextDocument} doc - TextDocument that is being saved
*/
export async function saveFile(doc: vscode.TextDocument, datasetProvider: api.IZoweTree<api.IZoweDatasetTreeNode>): Promise<void> {

Check warning on line 1513 in packages/zowe-explorer/src/dataset/actions.ts

View workflow job for this annotation

GitHub Actions / lint

Async function 'saveFile' has a complexity of 17. Maximum allowed is 15
ZoweLogger.trace("dataset.actions.saveFile called.");
// Check if file is a data set, instead of some other file
const docPath = path.join(doc.fileName, "..");
Expand Down Expand Up @@ -1566,6 +1566,7 @@
return;
}
} catch (err) {
await handleAutoSaveOnError();
await markDocumentUnsaved(doc);
await errorHandling(err, sesName);
return;
Expand Down Expand Up @@ -1610,10 +1611,12 @@
} else if (!uploadResponse.success && uploadResponse.commandResponse.includes("Rest API failure with HTTP(S) status 412")) {
resolveFileConflict(node, prof, doc, label);
} else {
await handleAutoSaveOnError();
await markDocumentUnsaved(doc);
api.Gui.errorMessage(uploadResponse.commandResponse);
}
} catch (err) {
await handleAutoSaveOnError();
await markDocumentUnsaved(doc);
await errorHandling(err, sesName);
}
Expand Down
4 changes: 3 additions & 1 deletion packages/zowe-explorer/src/uss/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { Profiles } from "../Profiles";
import { ZoweExplorerApiRegister } from "../ZoweExplorerApiRegister";
import { isBinaryFileSync } from "isbinaryfile";
import * as contextually from "../shared/context";
import { markDocumentUnsaved, setFileSaved } from "../utils/workspace";
import { handleAutoSaveOnError, markDocumentUnsaved, setFileSaved } from "../utils/workspace";
import * as nls from "vscode-nls";
import { refreshAll } from "../shared/refresh";
import { autoDetectEncoding, fileExistsCaseSensitiveSync } from "./utils";
Expand Down Expand Up @@ -319,6 +319,7 @@ export async function saveUSSFile(doc: vscode.TextDocument, ussFileProvider: IZo
LocalFileManagement.removeRecoveredFile(doc);
// this part never runs! zowe.Upload.fileToUSSFile doesn't return success: false, it just throws the error which is caught below!!!!!
} else {
await handleAutoSaveOnError();
await markDocumentUnsaved(doc);
Gui.errorMessage(uploadResponse.commandResponse);
}
Expand All @@ -328,6 +329,7 @@ export async function saveUSSFile(doc: vscode.TextDocument, ussFileProvider: IZo
if (errorMessage.includes("Rest API failure with HTTP(S) status 412")) {
resolveFileConflict(node, prof, doc, remote);
} else {
await handleAutoSaveOnError();
await markDocumentUnsaved(doc);
await errorHandling(err, sesName);
}
Expand Down
Loading
Loading