Skip to content

feat: filter which events trigger notifications - #7687

Open
mdl2934 wants to merge 14 commits into
louislam:masterfrom
mdl2934:notificaton-triggers
Open

feat: filter which events trigger notifications#7687
mdl2934 wants to merge 14 commits into
louislam:masterfrom
mdl2934:notificaton-triggers

Conversation

@mdl2934

@mdl2934 mdl2934 commented Aug 6, 2026

Copy link
Copy Markdown

Summary

This PR works off of a previous PR - #5770

In this pull request, the following changes are made:

Please follow this checklist to avoid unnecessary back and forth (click to expand)
  • ⚠️ If there are Breaking change (a fix or feature that alters existing functionality in a way that could cause issues) I have called them out
  • 🧠 I have disclosed any use of LLMs/AI in this contribution and reviewed all generated content.
    I understand that I am responsible for and able to explain every line of code I submit.
  • 🔍 Any UI changes adhere to visual style of this project.
  • 🛠️ I have self-reviewed and self-tested my code to ensure it works as expected.
  • 📝 I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • 🤖 I added or updated automated tests where appropriate.
  • 📄 Documentation updates are included (if applicable).
  • 🧰 Dependency updates are listed and explained.
  • ⚠️ CI passes and is green.

Screenshots for Visual Changes

  • UI Modifications: Highlight any changes made to the user interface.
  • Before & After: Include screenshots or comparisons (if applicable).
Page Before After
Edit Monitor image image
Notification Dialogue image Screenshot from 2026-08-06 09-56-05

@mdl2934
mdl2934 marked this pull request as draft August 6, 2026 17:53
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Hello and thanks for lending a paw to Uptime Kuma! 🐻👋
As this is your first contribution, please be sure to check out our Pull Request guidelines.
In particular: - Mark your PR as Draft while you’re still making changes - Mark it as Ready for review once it’s fully ready
If you have any design or process questions, feel free to ask them right here in this pull request - unclear documentation is a bug too.

@mdl2934
mdl2934 marked this pull request as ready for review August 6, 2026 22:12
@mdl2934
mdl2934 marked this pull request as draft August 6, 2026 22:12
@mdl2934
mdl2934 force-pushed the notificaton-triggers branch from 8eaffeb to ff51761 Compare August 6, 2026 22:31
@mdl2934
mdl2934 marked this pull request as ready for review August 7, 2026 00:36
@github-actions github-actions Bot added the pr:needs review this PR needs a review by maintainers or other community members label Aug 7, 2026

@autocarl autocarl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The product and UI direction is well aligned with #508 and the maintainer feedback on #5770: trigger selection lives at provider level, the monitor page is informational only, the existing multiselect style is reused, and domain expiry is included. The migration also preserved existing providers in my SQLite upgrade probe.

I found changes that are needed before this is ready because the runtime DB contract is currently broken and the added tests do not exercise the real row shape. In particular, all three notification paths read a camelCase field from raw R.getAll() rows, the negative domain test stays green after the production filter is removed, and Notification.save() makes the new field mandatory for older socket payloads. These conflict with the project's explicit test-coverage and backward-compatibility guidelines.

For positive validation, the production build, Prettier, changed-file ESLint, Stylelint, the fresh SQLite migration, and a synthetic merge with current master (77d1a0c5) all passed. GitHub's full matrix is green as well, but its current fixtures mask the runtime row-shape mismatch.

Comment thread server/model/monitor.js Outdated
Comment thread test/backend-test/test-domain.js Outdated
triggersJson: '["up"]',
});

const result = await Promise.race([

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker — false-green test: run in isolation, this test logs that the expiry is undefined and returns before reaching the trigger guard. I reverted the entire domain filter and the test still passed. Promise.race() also treats sendNotifications() resolving undefined first as success. Please mock findByDomainNameOrCreate() with a valid expiry, spy on Notification.send(), and assert zero calls. The core UP/DOWN and certificate branches also need coverage using the raw DB-row shape.

Comment thread server/notification.js Outdated
Comment thread src/pages/EditMonitor.vue
.where("id", n.id)
.update({
config: JSON.stringify({
...JSON.parse(n.config),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up to my review: this can strand a MariaDB upgrade when an existing notification has malformed JSON. notification.config is not DB-constrained; with {invalid-json, the real Knex migrator throws here. SQLite rolled the preceding column addition back in my probe, but MariaDB documents that ALTER TABLE commits implicitly and cannot be rolled back. That can leave triggers_json present while this migration is unrecorded, so the next startup fails while trying to add the column again. The existing 2025-12-29-0000-remove-line-notify.js migration already catches malformed notification JSON. Please guard/validate each config, preserve the schema backfill even when one config is bad, make the schema step safe to retry, and cover a populated malformed row on MariaDB.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked on the current head 8d24d290: this migration blocker still applies. With an existing {invalid-json config, the current up() throws at line 16 after adding triggers. A Knex transaction rolls the DDL back on SQLite, but MariaDB's implicit commit leaves the added column behind while the migration remains unrecorded. The next startup can therefore fail trying to add it again. The green matrix does not exercise a populated malformed row on MariaDB; this still needs a guarded backfill/retry-safe schema step and a regression test.

@mdl2934

mdl2934 commented Aug 7, 2026

Copy link
Copy Markdown
Author

i need help writing tests

@autocarl autocarl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of the current head 8d24d290470e642dba8c72e7c6afab78c2b3ddbd. The latest commits fix the raw SQL row-shape mismatch, preserve older notification payloads while validating the allowlist, remove the open-ended multiselect mode, and make the tooltip keyboard-focusable. I verified those fixes with the real SQLite/RedBean row shape and direct Notification.save() probes.

The build, changed-file ESLint/Prettier/Stylelint, language and migration checks, the complete 18-test domain file, and all 25 GitHub checks pass. Two issues remain before this is ready: the existing MariaDB migration thread still applies, and the trigger filtering still has no effective regression test. The current positive domain test is order-dependent, and deleting the production domain guard leaves all 18 tests green. I also left one small accessible-name follow-up.

I am keeping this as COMMENT; the repository reserves the formal Go/No-Go decision for maintainers.

active: 1,
user_id: 1,
name: "Testhook",
triggers: '["domain"]',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trigger behavior is still unprotected, and this test depends on execution order. Running only --test-name-pattern='sendNotifications' makes this positive test time out because google.com has no cached expiry; the full file passes only because the earlier checkExpiry() test populates it. More importantly, I removed the complete domain trigger guard from domain_expiry.js:155-159 and all 18 tests still passed. A small deterministic test would be more useful than the webhook/RDAP path here: mock findByDomainNameOrCreate() with a valid near-term expiry, spy on Notification.send(), assert zero calls with triggers: '[]', then one call with triggers: '["domain"]'. That directly protects the new filter and runs independently.

Comment thread src/pages/EditMonitor.vue
@autocarl

autocarl commented Aug 8, 2026

Copy link
Copy Markdown

@mdl2934 — replying to your request for help writing tests: one deterministic test is still needed for the new trigger filter. The current webhook test depends on the preceding RDAP/cache test and does not fail if the domain guard is removed.

This can replace that test, or sit next to it. It uses imports already present in test-domain.js, does not need a real webhook or RDAP response, and checks both sides of the behavior:

test("sendNotifications() honors the domain trigger", async () => {
    const domain = {
        expiry: dayjs.utc().add(1, "day").toISOString(),
        daysRemaining: 1,
        lastExpiryNotificationSent: null,
    };
    const notification = {
        name: "Testhook",
        config: JSON.stringify({ type: "webhook" }),
        triggers: "[]",
    };

    await setSetting("domainExpiryNotifyDays", [7], "general");
    mock.method(DomainExpiry, "findByDomainNameOrCreate", async () => domain);
    const sendMock = mock.method(Notification, "send", async () => "OK");
    mock.method(R, "store", async () => {});

    try {
        await DomainExpiry.sendNotifications("example.com", [notification]);
        assert.strictEqual(sendMock.mock.callCount(), 0);

        notification.triggers = '["domain"]';
        await DomainExpiry.sendNotifications("example.com", [notification]);
        assert.strictEqual(sendMock.mock.callCount(), 1);
    } finally {
        mock.restoreAll();
    }
});

You can run it independently with:

TEST_BACKEND=1 node --test --test-reporter=spec --test-name-pattern='honors the domain trigger' test/backend-test/test-domain.js

I tested this exact snippet on the current head 8d24d290: it passes independently, and after removing the production domain guard it fails at the first assertion with 1 !== 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:needs review this PR needs a review by maintainers or other community members

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Filter which event (UP/DOWN/CERT-EXPIRY/...) triggers alerts

2 participants