From f0c17ab9e8cf0c52019f3b0fe607677653f0db72 Mon Sep 17 00:00:00 2001 From: saber04414 Date: Tue, 13 Jan 2026 19:10:49 +0200 Subject: [PATCH] Refactor isImportantForNotification: remove DB dependency and add tests - Make function synchronous and remove database dependency - Accept lastNonPendingStatus as parameter instead of monitorID - Simplify function logic for better testability - Update call sites to fetch lastNonPendingStatus only when needed - Add comprehensive test suite with 17 test cases covering all status transitions Addresses code review feedback about function complexity and testability --- server/model/monitor.js | 52 ++++------ server/routers/api-router.js | 8 +- .../backend-test/test-monitor-notification.js | 96 +++++++++++++++++++ 3 files changed, 122 insertions(+), 34 deletions(-) create mode 100644 test/backend-test/test-monitor-notification.js diff --git a/server/model/monitor.js b/server/model/monitor.js index a6fd59d53..27da0f8b4 100644 --- a/server/model/monitor.js +++ b/server/model/monitor.js @@ -1009,7 +1009,13 @@ class Monitor extends BeanModel { if (isImportant) { bean.important = true; - if (await Monitor.isImportantForNotification(isFirstBeat, previousBeat?.status, bean.status, this.id)) { + // Get lastNonPendingStatus only for PENDING -> UP transitions + let lastNonPendingStatus = null; + if (previousBeat?.status === PENDING && bean.status === UP) { + lastNonPendingStatus = await Monitor.getLastNonPendingStatus(this.id); + } + + if (Monitor.isImportantForNotification(isFirstBeat, previousBeat?.status, bean.status, lastNonPendingStatus)) { log.debug("monitor", `[${this.name}] sendNotification`); await Monitor.sendNotification(isFirstBeat, this, bean); } else { @@ -1450,42 +1456,22 @@ class Monitor extends BeanModel { * @param {boolean} isFirstBeat Is this the first beat of this monitor? * @param {const} previousBeatStatus Status of the previous beat * @param {const} currentBeatStatus Status of the current beat - * @param {number} monitorID Optional monitor ID to check history for PENDING->UP transitions - * @returns {Promise} True if is an important beat else false + * @returns {boolean} True if is an important beat else false +>>>>>>> d4c2e5fc (Refactor isImportantForNotification: remove DB dependency and add tests) */ - static async isImportantForNotification(isFirstBeat, previousBeatStatus, currentBeatStatus, monitorID = null) { - // * ? -> ANY STATUS = important [isFirstBeat] - // UP -> PENDING = not important - // * UP -> DOWN = important - // UP -> UP = not important - // PENDING -> PENDING = not important - // * PENDING -> DOWN = important - // PENDING -> UP = important if monitor was DOWN before PENDING (fix for issue #6025) - // DOWN -> PENDING = this case not exists - // DOWN -> DOWN = not important - // * DOWN -> UP = important - // MAINTENANCE -> MAINTENANCE = not important - // MAINTENANCE -> UP = not important - // * MAINTENANCE -> DOWN = important - // DOWN -> MAINTENANCE = not important - // UP -> MAINTENANCE = not important - - // Check for PENDING -> UP transition - if (previousBeatStatus === PENDING && currentBeatStatus === UP) { - // If monitorID is provided, check if the monitor was DOWN before entering PENDING - if (monitorID) { - const lastNonPendingStatus = await Monitor.getLastNonPendingStatus(monitorID); - // If the last non-PENDING status was DOWN, this transition is important - if (lastNonPendingStatus === DOWN) { - return true; - } - } - // Otherwise, PENDING -> UP is not important (original behavior) - return false; + static isImportantForNotification(isFirstBeat, previousBeatStatus, currentBeatStatus, lastNonPendingStatus = null) { + // First beat is always important + if (isFirstBeat) { + return true; } + // PENDING -> UP is important only if monitor was DOWN before entering PENDING (fix for issue #6025) + if (previousBeatStatus === PENDING && currentBeatStatus === UP) { + return lastNonPendingStatus === DOWN; + } + + // Important transitions return ( - isFirstBeat || (previousBeatStatus === MAINTENANCE && currentBeatStatus === DOWN) || (previousBeatStatus === UP && currentBeatStatus === DOWN) || (previousBeatStatus === DOWN && currentBeatStatus === UP) || diff --git a/server/routers/api-router.js b/server/routers/api-router.js index c2a9bc7dc..12876cb4a 100644 --- a/server/routers/api-router.js +++ b/server/routers/api-router.js @@ -99,7 +99,13 @@ router.all("/api/push/:pushToken", async (request, response) => { bean.important = Monitor.isImportantBeat(isFirstBeat, previousHeartbeat?.status, bean.status); - if (await Monitor.isImportantForNotification(isFirstBeat, previousHeartbeat?.status, bean.status, monitor.id)) { + // Get lastNonPendingStatus only for PENDING -> UP transitions + let lastNonPendingStatus = null; + if (previousHeartbeat?.status === PENDING && bean.status === UP) { + lastNonPendingStatus = await Monitor.getLastNonPendingStatus(monitor.id); + } + + if (Monitor.isImportantForNotification(isFirstBeat, previousHeartbeat?.status, bean.status, lastNonPendingStatus)) { // Reset down count bean.downCount = 0; diff --git a/test/backend-test/test-monitor-notification.js b/test/backend-test/test-monitor-notification.js new file mode 100644 index 000000000..89ed8f589 --- /dev/null +++ b/test/backend-test/test-monitor-notification.js @@ -0,0 +1,96 @@ +const { describe, test } = require("node:test"); +const assert = require("node:assert"); +const Monitor = require("../../server/model/monitor"); +const { UP, DOWN, PENDING, MAINTENANCE } = require("../../src/util"); + +describe("Monitor.isImportantForNotification", () => { + // First beat is always important + test("first beat is always important", () => { + assert.strictEqual(Monitor.isImportantForNotification(true, null, UP), true); + assert.strictEqual(Monitor.isImportantForNotification(true, null, DOWN), true); + assert.strictEqual(Monitor.isImportantForNotification(true, null, PENDING), true); + assert.strictEqual(Monitor.isImportantForNotification(true, null, MAINTENANCE), true); + }); + + // UP -> PENDING = not important + test("UP -> PENDING is not important", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, UP, PENDING), false); + }); + + // UP -> DOWN = important + test("UP -> DOWN is important", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, UP, DOWN), true); + }); + + // UP -> UP = not important + test("UP -> UP is not important", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, UP, UP), false); + }); + + // PENDING -> PENDING = not important + test("PENDING -> PENDING is not important", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, PENDING, PENDING), false); + }); + + // PENDING -> DOWN = important + test("PENDING -> DOWN is important", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, PENDING, DOWN), true); + }); + + // PENDING -> UP = important if monitor was DOWN before PENDING (fix for issue #6025) + test("PENDING -> UP is important when lastNonPendingStatus was DOWN", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, PENDING, UP, DOWN), true); + }); + + test("PENDING -> UP is not important when lastNonPendingStatus was not DOWN", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, PENDING, UP, UP), false); + assert.strictEqual(Monitor.isImportantForNotification(false, PENDING, UP, MAINTENANCE), false); + assert.strictEqual(Monitor.isImportantForNotification(false, PENDING, UP, null), false); + }); + + // DOWN -> DOWN = not important + test("DOWN -> DOWN is not important", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, DOWN, DOWN), false); + }); + + // DOWN -> UP = important + test("DOWN -> UP is important", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, DOWN, UP), true); + }); + + // MAINTENANCE -> MAINTENANCE = not important + test("MAINTENANCE -> MAINTENANCE is not important", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, MAINTENANCE, MAINTENANCE), false); + }); + + // MAINTENANCE -> UP = not important + test("MAINTENANCE -> UP is not important", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, MAINTENANCE, UP), false); + }); + + // MAINTENANCE -> DOWN = important + test("MAINTENANCE -> DOWN is important", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, MAINTENANCE, DOWN), true); + }); + + // DOWN -> MAINTENANCE = not important + test("DOWN -> MAINTENANCE is not important", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, DOWN, MAINTENANCE), false); + }); + + // UP -> MAINTENANCE = not important + test("UP -> MAINTENANCE is not important", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, UP, MAINTENANCE), false); + }); + + // Additional edge cases + test("PENDING -> UP with undefined lastNonPendingStatus defaults to not important", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, PENDING, UP, undefined), false); + }); + + test("non-PENDING -> UP transitions ignore lastNonPendingStatus parameter", () => { + assert.strictEqual(Monitor.isImportantForNotification(false, DOWN, UP, DOWN), true); + assert.strictEqual(Monitor.isImportantForNotification(false, UP, UP, DOWN), false); + }); +}); +