diff --git a/.gitignore b/.gitignore index a8bcdd35..ea15d0ba 100644 --- a/.gitignore +++ b/.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/* diff --git a/CHANGELOG.md b/CHANGELOG.md index ae5e3602..da8eea88 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/modules/default/compliments/compliments.js b/modules/default/compliments/compliments.js index ee0d2b98..a20810a9 100644 --- a/modules/default/compliments/compliments.js +++ b/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} 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; } }, diff --git a/tests/unit/modules/default/compliments/compliments_spec.js b/tests/unit/modules/default/compliments/compliments_spec.js new file mode 100644 index 00000000..8d3aef30 --- /dev/null +++ b/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="); + }); + }); + }); +});