[compliments] refactor: optimize loadComplimentFile method and add unit tests (#3969)

## Changes

- Replace `indexOf()` with `startsWith()` for cleaner protocol detection
- Use `URL` API for robust cache-busting parameter handling
- Add HTTP response validation and improved error logging
- Add JSDoc type annotations for better documentation
- Remove unused `urlSuffix` instance variable
- Add unit tests
- Fix `.gitignore` pattern

## Motivation

After merging #3967, I noticed some potential for improving reliability
and user experience related to the method `loadComplimentFile`. With
these changes the method now validates URLs upfront to catch
configuration errors early, checks HTTP status codes to detect server
issues (404/500), and provides specific error messages that help users
troubleshoot problems.

The complexity of the code does not really increase with the changes. On
the contrary, the method should now be more intuitive to understand.

## Testing

Added unit tests for `loadComplimentFile()` to validate the
improvements:
- HTTP error handling
- Cache-busting

Since E2E tests already cover the happy path, these unit tests focus on
error cases and edge cases.

## Additional Fix

While adding the test file, I discovered that the `.gitignore` pattern
`modules` was incorrectly matching `tests/unit/modules/`, preventing
test files from being tracked. Changed to `/modules` to only match the
root directory.

.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 * 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 {
-			const response = await fetch(url + this.urlSuffix);
+			// 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);
+			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=");
+			});
+		});
+	});
+});