From 4876bb229af3b1a4574179101b2302bc2bd4905e Mon Sep 17 00:00:00 2001 From: Ekhorn Date: Wed, 29 Jul 2026 14:44:12 +0200 Subject: [PATCH] Skip already-selected alarms when selecting all Selecting all rows emits a select event per row, including rows that were already selected, so their ids were appended to the selection a second time. The delete confirmation then reported a count higher than the number of alarms. --- ui/app/manager/src/pages/page-alarms.ts | 3 +- ui/app/manager/test/alarms.test.ts | 60 +++++++++++++++++++- ui/app/manager/test/fixtures/pages/alarms.ts | 38 +++++++++++++ 3 files changed, 98 insertions(+), 3 deletions(-) diff --git a/ui/app/manager/src/pages/page-alarms.ts b/ui/app/manager/src/pages/page-alarms.ts index 2375bac101..c9ba638b4f 100644 --- a/ui/app/manager/src/pages/page-alarms.ts +++ b/ui/app/manager/src/pages/page-alarms.ts @@ -854,7 +854,8 @@ export class PageAlarms extends Page { if (ev.detail.state) { if (this._selectedIds === undefined) { this._selectedIds = [alarm.id]; - } else { + } else if (!this._selectedIds.includes(alarm.id)) { + // selecting all emits an event per row, including rows that are already selected this._selectedIds.push(alarm.id); this.requestUpdate('_selectedIds'); } diff --git a/ui/app/manager/test/alarms.test.ts b/ui/app/manager/test/alarms.test.ts index 04619ee3fe..e85f9387b5 100644 --- a/ui/app/manager/test/alarms.test.ts +++ b/ui/app/manager/test/alarms.test.ts @@ -26,12 +26,15 @@ test.use({ storageState: adminStatePath }); /** Alarms seeded through the REST API, removed again after each test. */ const seededAlarmIds: number[] = []; +/** Distinguishes alarms seeded within the same millisecond, so their titles stay unique enough to look up by. */ +let seedCount = 0; + /** Persist an alarm via REST and return its id and title, for table lookups and assertions. */ async function seedAlarm( manager: Manager, { realm = "master", status = AlarmStatus.OPEN, severity = AlarmSeverity.MEDIUM } = {} ) { - const title = `E2E alarm ${Date.now()}`; + const title = `E2E alarm ${Date.now()}-${++seedCount}`; const config = await manager.adminConfig(); const alarm = (await manager.api.AlarmResource.createAlarm({ realm, @@ -48,7 +51,9 @@ async function seedAlarm( test.afterEach(async ({ manager }) => { if (seededAlarmIds.length > 0) { - await manager.api.AlarmResource.removeAlarms(seededAlarmIds.splice(0), await manager.adminConfig()); + // tests that delete through the UI leave ids behind that are already gone, which is not a failure + await manager.api.AlarmResource.removeAlarms(seededAlarmIds.splice(0), await manager.adminConfig()) + .catch((e) => console.warn("Could not delete seeded alarm(s): ", e.response?.status ?? e.message)); } await manager.cleanUp(); }); @@ -105,6 +110,57 @@ test("should drop a resolved alarm from the default active filter", async ({ man await expect(alarmsPage.getRowByText(title)).toHaveCount(0); }); +/** + * The assertions below count table rows, so they run against "smartcity", which holds only the alarms seeded here + * rather than the master realm's demo data. + */ +test.describe("Delete alarms", () => { + // Keycloak sessions are per realm, so the stored admin state (master) cannot view "smartcity". Start clean and + // log in as a user of that realm instead. + test.use({ storageState: { cookies: [], origins: [] } }); + + /** + * @given Three alarms in the "smartcity" realm, and a user of that realm who may write alarms + * @when A single row is selected, and "select all" is then ticked on top of it + * @then The confirmation counts each alarm once rather than counting the pre-selected row twice + * @and Confirming removes all of them + */ + test("should delete the alarms selected in the table", async ({ manager, alarmsPage }) => { + // seeded over REST as the master admin, which is independent of the realm session used below + const alarms = [ + await seedAlarm(manager, { realm: "smartcity" }), + await seedAlarm(manager, { realm: "smartcity" }), + await seedAlarm(manager, { realm: "smartcity" }) + ]; + + await manager.provisionUserAndLogin("smartcity", { + username: "e2e-alarm-editor", + roles: ["read:alarms", "write:alarms"], + }); + await alarmsPage.goto(); + await expect(alarmsPage.getRows()).toHaveCount(alarms.length); + + // Selecting all emits a select event per row, including the row already selected here, which used to be + // added to the selection a second time. More than one row is needed: selecting the only row would already + // put the header checkbox in its checked state, making ticking it a no-op that emits nothing. + await alarmsPage.getRowCheckbox(alarms[0].title).check(); + await alarmsPage.getSelectAllCheckbox().check(); + + // the trash button only appears once something is selected + await expect(alarmsPage.getDeleteSelectedButton()).toBeVisible(); + await alarmsPage.getDeleteSelectedButton().click(); + await expect(alarmsPage.getDeleteConfirmation(alarms.length)).toHaveCount(1); + + // confirming clears the table, and the alarms are gone from the backend rather than just the view + await alarmsPage.getConfirmDeleteButton().click(); + await expect(alarmsPage.getRows()).toHaveCount(0); + + const config = await manager.adminConfig(); + const remaining = (await manager.api.AlarmResource.getAlarms({ realm: "smartcity" }, config)).data; + expect(remaining).toEqual([]); + }); +}); + test.describe("Role-Based Access Control", () => { // Start from a clean session so we can log in as the low-privilege user rather than a stored admin state. test.use({ storageState: { cookies: [], origins: [] } }); diff --git a/ui/app/manager/test/fixtures/pages/alarms.ts b/ui/app/manager/test/fixtures/pages/alarms.ts index da0e528753..a73f51c304 100644 --- a/ui/app/manager/test/fixtures/pages/alarms.ts +++ b/ui/app/manager/test/fixtures/pages/alarms.ts @@ -56,6 +56,44 @@ export class AlarmsPage implements BasePage { return this.page.getByRole("button", { name: "Add Alarm" }); } + // --- Selection and deletion -------------------------------------------- + + /** The "select all" checkbox in the table header. */ + getSelectAllCheckbox(): Locator { + return this.getTable().locator("thead or-mwc-input[type=checkbox] #component > input"); + } + + /** The selection checkbox of the (first) row containing the given text. */ + getRowCheckbox(rowText: string): Locator { + return this.getRowByText(rowText).first().locator("or-mwc-input[type=checkbox] #component > input"); + } + + /** The trash button above the table, only shown once rows are selected. */ + getDeleteSelectedButton(): Locator { + return this.page.locator("#controls or-vaadin-button") + .filter({ has: this.page.locator("or-icon[icon='delete']") }); + } + + /** + * The open delete dialog, matched on it confirming the deletion of `count` alarms. + * + * `or-translate` interpolates the count into its own shadow root, so no light-DOM element holds that text and + * `getByText` cannot resolve it. Matching the dialog on the text its subtree contains keeps the locator on the + * dialog rather than on the translation element. + * + * Assert on its count, not its visibility: the dialog is only in the DOM while open, but its content is + * slotted into an overlay elsewhere in the page, leaving the element itself without a box to measure. + */ + getDeleteConfirmation(count: number): Locator { + return this.page.getByRole("alertdialog") + .filter({ hasText: `delete ${count} selected alarm(s)` }); + } + + /** The confirm button of the open delete dialog. */ + getConfirmDeleteButton(): Locator { + return this.page.getByRole("alertdialog").getByRole("button", { name: "Delete", exact: true }); + } + // --- Single alarm view ------------------------------------------------- /** The properties column of the single-alarm view, holding the severity/status/assignee selects. */