[compliments] refactor: optimize loadComplimentFile method and add unit tests

.gitignore

@@ -55,8 +55,8 @@ Temporary Items
 .Trash-*
 
 # Ignore all modules except the default modules.
-modules/*
-!modules/default
+/modules/*
+!/modules/default
 
 # Ignore changes to the custom css files but keep the sample and main.
 /css/*

CHANGELOG.md

@@ -37,7 +37,8 @@ planned for 2026-01-01
 - [core] refactor: replace `XMLHttpRequest` with `fetch` in `translator.js` (#3950)
 - [tests] migrate e2e tests to Playwright (#3950)
 - [calendar] refactor: migrate CalendarFetcher to ES6 class and improve error handling (#3958)
-- [gitignore] cleanup/simplify .gitignore (#3952, #3954, #3968)
+- [gitignore] cleanup/simplify .gitignore (#3952, #3954, #3968, #3969)
+- refactor(compliments): optimize `loadComplimentFile` method and add unit tests(#3969)
 
 ### Fixed
 

modules/default/compliments/compliments.js

@@ -21,7 +21,6 @@ Module.register("compliments", {
 		random: true,
 		specialDayUnique: false
 	},
-	urlSuffix: "",
 	compliments_new: null,
 	refreshMinimumDelay: 15 * 60 * 60 * 1000, // 15 minutes
 	lastIndexUsed: -1,
@@ -205,23 +204,35 @@ Module.register("compliments", {
 
 	/**
 	 * Retrieve a file from the local filesystem
-	 * @returns {Promise} Resolved when the file is loaded
+	 * @returns {Promise<string|null>} Resolved with file content or null on error
 	 */
 	async loadComplimentFile () {
-		const isRemote = this.config.remoteFile.indexOf("http://") === 0 || this.config.remoteFile.indexOf("https://") === 0,
-			url = isRemote ? this.config.remoteFile : this.file(this.config.remoteFile);
-		// because we may be fetching the same url,
-		// we need to force the server to not give us the cached result
-		// create an extra property (ignored by the server handler) just so the url string is different
-		// that will never be the same, using the ms value of date
-		if (isRemote && this.config.remoteFileRefreshInterval !== 0) {
-			this.urlSuffix = this.config.remoteFile.includes("?") ? `&dummy=${Date.now()}` : `?dummy=${Date.now()}`;
+		const { remoteFile, remoteFileRefreshInterval } = this.config;
+		const isRemote = remoteFile.startsWith("http://") || remoteFile.startsWith("https://");
+		let url = isRemote ? remoteFile : this.file(remoteFile);
+
+		try {
+			// Validate URL
+			const urlObj = new URL(url);
+			// Add cache-busting parameter to remote URLs to prevent cached responses
+			if (isRemote && remoteFileRefreshInterval !== 0) {
+				urlObj.searchParams.set("dummy", Date.now());
+			}
+			url = urlObj.toString();
+		} catch {
+			Log.warn(`[compliments] Invalid URL: ${url}`);
 		}
+
 		try {
-			const response = await fetch(url + this.urlSuffix);
+			const response = await fetch(url);
+			if (!response.ok) {
+				Log.error(`[compliments] HTTP error: ${response.status} ${response.statusText}`);
+				return null;
+			}
 			return await response.text();
 		} catch (error) {
-			Log.info(`[compliments] ${this.name} fetch failed error=`, error);
+			Log.info("[compliments] fetch failed:", error.message);
+			return null;
 		}
 	},
 

tests/unit/modules/default/compliments/compliments_spec.js

@@ -0,0 +1,152 @@
+describe("Compliments module", () => {
+	let complimentsModule;
+
+	beforeEach(() => {
+		// Mock global dependencies
+		global.Module = {
+			register: vi.fn((name, moduleDefinition) => {
+				complimentsModule = moduleDefinition;
+			})
+		};
+		global.Log = {
+			info: vi.fn(),
+			warn: vi.fn(),
+			error: vi.fn()
+		};
+		global.Cron = vi.fn();
+
+		// Load the module
+		require("../../../../../modules/default/compliments/compliments");
+
+		// Setup module instance
+		complimentsModule.config = { ...complimentsModule.defaults };
+		complimentsModule.name = "compliments";
+		complimentsModule.file = vi.fn((path) => `http://localhost:8080/modules/default/compliments/${path}`);
+	});
+
+	afterEach(() => {
+		vi.restoreAllMocks();
+	});
+
+	describe("loadComplimentFile", () => {
+		describe("HTTP error handling", () => {
+			it("should return null and log error on HTTP 404", async () => {
+				complimentsModule.config.remoteFile = "http://example.com/missing.json";
+
+				global.fetch = vi.fn(() => Promise.resolve({
+					ok: false,
+					status: 404,
+					statusText: "Not Found"
+				}));
+
+				const result = await complimentsModule.loadComplimentFile();
+
+				expect(result).toBeNull();
+				expect(Log.error).toHaveBeenCalledWith("[compliments] HTTP error: 404 Not Found");
+			});
+
+			it("should return null and log error on HTTP 500", async () => {
+				complimentsModule.config.remoteFile = "http://example.com/error.json";
+
+				global.fetch = vi.fn(() => Promise.resolve({
+					ok: false,
+					status: 500,
+					statusText: "Internal Server Error"
+				}));
+
+				const result = await complimentsModule.loadComplimentFile();
+
+				expect(result).toBeNull();
+				expect(Log.error).toHaveBeenCalledWith("[compliments] HTTP error: 500 Internal Server Error");
+			});
+
+			it("should return content on successful HTTP 200", async () => {
+				complimentsModule.config.remoteFile = "http://example.com/compliments.json";
+				const expectedContent = "{\"anytime\":[\"Test compliment\"]}";
+
+				global.fetch = vi.fn(() => Promise.resolve({
+					ok: true,
+					status: 200,
+					text: () => Promise.resolve(expectedContent)
+				}));
+
+				const result = await complimentsModule.loadComplimentFile();
+
+				expect(result).toBe(expectedContent);
+				expect(Log.error).not.toHaveBeenCalled();
+			});
+		});
+
+		describe("Cache-busting with query parameters", () => {
+			beforeEach(() => {
+				vi.useFakeTimers();
+				vi.setSystemTime(new Date("2025-01-01T12:00:00.000Z"));
+			});
+
+			afterEach(() => {
+				vi.useRealTimers();
+			});
+
+			it("should add cache-busting parameter to URL without query params", async () => {
+				complimentsModule.config.remoteFile = "http://example.com/compliments.json";
+				complimentsModule.config.remoteFileRefreshInterval = 60000;
+
+				global.fetch = vi.fn(() => Promise.resolve({
+					ok: true,
+					text: () => Promise.resolve("{}")
+				}));
+
+				await complimentsModule.loadComplimentFile();
+
+				expect(fetch).toHaveBeenCalledWith(expect.stringContaining("?dummy=1735732800000"));
+			});
+
+			it("should add cache-busting parameter to URL with existing query params", async () => {
+				complimentsModule.config.remoteFile = "http://example.com/compliments.json?lang=en";
+				complimentsModule.config.remoteFileRefreshInterval = 60000;
+
+				global.fetch = vi.fn(() => Promise.resolve({
+					ok: true,
+					text: () => Promise.resolve("{}")
+				}));
+
+				await complimentsModule.loadComplimentFile();
+
+				const calledUrl = fetch.mock.calls[0][0];
+				expect(calledUrl).toContain("lang=en");
+				expect(calledUrl).toContain("&dummy=1735732800000");
+				expect(calledUrl).not.toContain("?dummy=");
+			});
+
+			it("should not add cache-busting parameter when remoteFileRefreshInterval is 0", async () => {
+				complimentsModule.config.remoteFile = "http://example.com/compliments.json";
+				complimentsModule.config.remoteFileRefreshInterval = 0;
+
+				global.fetch = vi.fn(() => Promise.resolve({
+					ok: true,
+					text: () => Promise.resolve("{}")
+				}));
+
+				await complimentsModule.loadComplimentFile();
+
+				expect(fetch).toHaveBeenCalledWith("http://example.com/compliments.json");
+			});
+
+			it("should not add cache-busting parameter to local files", async () => {
+				complimentsModule.config.remoteFile = "compliments.json";
+				complimentsModule.config.remoteFileRefreshInterval = 60000;
+
+				global.fetch = vi.fn(() => Promise.resolve({
+					ok: true,
+					text: () => Promise.resolve("{}")
+				}));
+
+				await complimentsModule.loadComplimentFile();
+
+				const calledUrl = fetch.mock.calls[0][0];
+				expect(calledUrl).toBe("http://localhost:8080/modules/default/compliments/compliments.json");
+				expect(calledUrl).not.toContain("dummy=");
+			});
+		});
+	});
+});