fix: address final review findings (order-notification watcher robustness)
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -13,7 +13,7 @@ describe('AdminSettingsPageComponent notification interval', () => {
|
|||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
watcherStub = {
|
watcherStub = {
|
||||||
intervalMs: signal(15000),
|
intervalMs: signal(15000),
|
||||||
setIntervalSeconds: jasmine.createSpy('setIntervalSeconds'),
|
setIntervalSeconds: jasmine.createSpy('setIntervalSeconds').and.returnValue(true),
|
||||||
};
|
};
|
||||||
|
|
||||||
TestBed.configureTestingModule({
|
TestBed.configureTestingModule({
|
||||||
@@ -39,4 +39,31 @@ describe('AdminSettingsPageComponent notification interval', () => {
|
|||||||
|
|
||||||
expect(watcherStub.setIntervalSeconds).toHaveBeenCalledWith(30);
|
expect(watcherStub.setIntervalSeconds).toHaveBeenCalledWith(30);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('shows the saved message and keeps the draft when the interval is applied', () => {
|
||||||
|
watcherStub.setIntervalSeconds.and.callFake((seconds: number) => {
|
||||||
|
watcherStub.intervalMs.set(seconds * 1000);
|
||||||
|
return true;
|
||||||
|
});
|
||||||
|
const fixture = TestBed.createComponent(AdminSettingsPageComponent);
|
||||||
|
const component = fixture.componentInstance;
|
||||||
|
|
||||||
|
component.notificationIntervalSecondsDraft.set(30);
|
||||||
|
component.saveNotificationInterval();
|
||||||
|
|
||||||
|
expect(component.showNotificationIntervalSaved()).toBe(true);
|
||||||
|
expect(component.notificationIntervalSecondsDraft()).toBe(30);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('does not show the saved message and reverts the draft when the interval is rejected', () => {
|
||||||
|
watcherStub.setIntervalSeconds.and.returnValue(false);
|
||||||
|
const fixture = TestBed.createComponent(AdminSettingsPageComponent);
|
||||||
|
const component = fixture.componentInstance;
|
||||||
|
|
||||||
|
component.notificationIntervalSecondsDraft.set(0);
|
||||||
|
component.saveNotificationInterval();
|
||||||
|
|
||||||
|
expect(component.showNotificationIntervalSaved()).toBe(false);
|
||||||
|
expect(component.notificationIntervalSecondsDraft()).toBe(15);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -31,10 +31,13 @@ export class AdminSettingsPageComponent {
|
|||||||
readonly showNotificationIntervalSaved = signal(false);
|
readonly showNotificationIntervalSaved = signal(false);
|
||||||
|
|
||||||
saveNotificationInterval(): void {
|
saveNotificationInterval(): void {
|
||||||
this.orderWatcher.setIntervalSeconds(this.notificationIntervalSecondsDraft());
|
const applied = this.orderWatcher.setIntervalSeconds(this.notificationIntervalSecondsDraft());
|
||||||
|
this.notificationIntervalSecondsDraft.set(Math.round(this.orderWatcher.intervalMs() / 1000));
|
||||||
|
if (applied) {
|
||||||
this.showNotificationIntervalSaved.set(true);
|
this.showNotificationIntervalSaved.set(true);
|
||||||
setTimeout(() => this.showNotificationIntervalSaved.set(false), SAVED_MESSAGE_DURATION_MS);
|
setTimeout(() => this.showNotificationIntervalSaved.set(false), SAVED_MESSAGE_DURATION_MS);
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
onCompactToggle(compact: boolean): void {
|
onCompactToggle(compact: boolean): void {
|
||||||
this.preferences.setDensity(compact ? 'compact' : 'comfortable');
|
this.preferences.setDensity(compact ? 'compact' : 'comfortable');
|
||||||
|
|||||||
@@ -367,6 +367,8 @@
|
|||||||
right: 0;
|
right: 0;
|
||||||
top: calc(100% + 8px);
|
top: calc(100% + 8px);
|
||||||
width: 240px;
|
width: 240px;
|
||||||
|
max-height: 60vh;
|
||||||
|
overflow-y: auto;
|
||||||
padding: var(--space-md);
|
padding: var(--space-md);
|
||||||
background: var(--bg-primary);
|
background: var(--bg-primary);
|
||||||
border: 1px solid var(--border-color);
|
border: 1px solid var(--border-color);
|
||||||
|
|||||||
@@ -30,6 +30,7 @@ describe('AdminLayoutComponent notifications bell', () => {
|
|||||||
recentOrders: ReturnType<typeof signal<AdminOrder[]>>;
|
recentOrders: ReturnType<typeof signal<AdminOrder[]>>;
|
||||||
unreadCount: ReturnType<typeof signal<number>>;
|
unreadCount: ReturnType<typeof signal<number>>;
|
||||||
start: jasmine.Spy;
|
start: jasmine.Spy;
|
||||||
|
stop: jasmine.Spy;
|
||||||
markAllSeen: jasmine.Spy;
|
markAllSeen: jasmine.Spy;
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -38,6 +39,7 @@ describe('AdminLayoutComponent notifications bell', () => {
|
|||||||
recentOrders: signal<AdminOrder[]>([makeOrder('o1', '1001')]),
|
recentOrders: signal<AdminOrder[]>([makeOrder('o1', '1001')]),
|
||||||
unreadCount: signal(1),
|
unreadCount: signal(1),
|
||||||
start: jasmine.createSpy('start'),
|
start: jasmine.createSpy('start'),
|
||||||
|
stop: jasmine.createSpy('stop'),
|
||||||
markAllSeen: jasmine.createSpy('markAllSeen'),
|
markAllSeen: jasmine.createSpy('markAllSeen'),
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -73,4 +75,11 @@ describe('AdminLayoutComponent notifications bell', () => {
|
|||||||
expect(component.notificationsOpen()).toBe(false);
|
expect(component.notificationsOpen()).toBe(false);
|
||||||
expect(watcherStub.markAllSeen).toHaveBeenCalledTimes(1);
|
expect(watcherStub.markAllSeen).toHaveBeenCalledTimes(1);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('stops the watcher when the component is destroyed', () => {
|
||||||
|
const fixture = TestBed.createComponent(AdminLayoutComponent);
|
||||||
|
fixture.detectChanges();
|
||||||
|
fixture.destroy();
|
||||||
|
expect(watcherStub.stop).toHaveBeenCalledTimes(1);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -93,6 +93,7 @@ export class AdminLayoutComponent {
|
|||||||
constructor() {
|
constructor() {
|
||||||
this.readRouteData();
|
this.readRouteData();
|
||||||
this.orderWatcher.start();
|
this.orderWatcher.start();
|
||||||
|
this.destroyRef.onDestroy(() => this.orderWatcher.stop());
|
||||||
this.router.events
|
this.router.events
|
||||||
.pipe(
|
.pipe(
|
||||||
filter(event => event instanceof NavigationEnd),
|
filter(event => event instanceof NavigationEnd),
|
||||||
|
|||||||
@@ -48,7 +48,10 @@ describe('AdminOrderWatcherService', () => {
|
|||||||
[makeOrder('o2', '1002', '2026-08-15T10:00:00.000Z'), makeOrder('o1', '1001', '2026-08-15T09:00:00.000Z')],
|
[makeOrder('o2', '1002', '2026-08-15T10:00:00.000Z'), makeOrder('o1', '1001', '2026-08-15T09:00:00.000Z')],
|
||||||
];
|
];
|
||||||
|
|
||||||
localStorage.clear();
|
localStorage.removeItem('adminOrderWatcher.lastNotifiedOrderId.v1');
|
||||||
|
localStorage.removeItem('adminOrderWatcher.lastNotifiedOrderCreatedAt.v1');
|
||||||
|
localStorage.removeItem('adminOrderWatcher.lastAcknowledgedOrderId.v1');
|
||||||
|
localStorage.removeItem('adminOrderWatcher.pollIntervalMs.v1');
|
||||||
|
|
||||||
TestBed.configureTestingModule({
|
TestBed.configureTestingModule({
|
||||||
providers: [
|
providers: [
|
||||||
@@ -110,13 +113,53 @@ describe('AdminOrderWatcherService', () => {
|
|||||||
}));
|
}));
|
||||||
|
|
||||||
it('setIntervalSeconds updates intervalMs and rejects invalid values', () => {
|
it('setIntervalSeconds updates intervalMs and rejects invalid values', () => {
|
||||||
service.setIntervalSeconds(30);
|
expect(service.setIntervalSeconds(30)).toBe(true);
|
||||||
expect(service.intervalMs()).toBe(30000);
|
expect(service.intervalMs()).toBe(30000);
|
||||||
|
|
||||||
service.setIntervalSeconds(0);
|
expect(service.setIntervalSeconds(0)).toBe(false);
|
||||||
expect(service.intervalMs()).toBe(30000);
|
expect(service.intervalMs()).toBe(30000);
|
||||||
|
|
||||||
service.setIntervalSeconds(-5);
|
expect(service.setIntervalSeconds(-5)).toBe(false);
|
||||||
expect(service.intervalMs()).toBe(30000);
|
expect(service.intervalMs()).toBe(30000);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('falls back to a timestamp comparison (not the whole page) when the last-notified order id disappears', fakeAsync(() => {
|
||||||
|
service.start();
|
||||||
|
tick(0);
|
||||||
|
|
||||||
|
// Second poll: 'o2' (the last-notified id) is gone. Page contains orders
|
||||||
|
// both older and newer than o2's createdAt (2026-08-15T10:00:00.000Z).
|
||||||
|
pollIndex = 1;
|
||||||
|
ordersByPoll.push([
|
||||||
|
makeOrder('o5', '1005', '2026-08-15T12:00:00.000Z'),
|
||||||
|
makeOrder('o4', '1004', '2026-08-15T11:00:00.000Z'),
|
||||||
|
makeOrder('o3', '1003', '2026-08-15T10:30:00.000Z'),
|
||||||
|
makeOrder('o0', '1000', '2026-08-15T08:00:00.000Z'),
|
||||||
|
]);
|
||||||
|
|
||||||
|
tick(service.intervalMs());
|
||||||
|
|
||||||
|
// Only o5, o4, o3 are newer than o2's createdAt - o0 must not toast.
|
||||||
|
expect(notifications.notifications().length).toBe(3);
|
||||||
|
const messages = notifications.notifications().map(n => n.message);
|
||||||
|
expect(messages.some(m => m.includes('1005'))).toBe(true);
|
||||||
|
expect(messages.some(m => m.includes('1004'))).toBe(true);
|
||||||
|
expect(messages.some(m => m.includes('1003'))).toBe(true);
|
||||||
|
expect(messages.some(m => m.includes('1000'))).toBe(false);
|
||||||
|
}));
|
||||||
|
|
||||||
|
it('stop() clears the interval so no further polls occur', fakeAsync(() => {
|
||||||
|
const gateway = TestBed.inject(AdminOrdersLocalGateway);
|
||||||
|
const loadOrdersSpy = spyOn(gateway, 'loadOrders').and.callThrough();
|
||||||
|
|
||||||
|
service.start();
|
||||||
|
tick(0);
|
||||||
|
const callsAfterStart = loadOrdersSpy.calls.count();
|
||||||
|
|
||||||
|
tick(service.intervalMs() / 2);
|
||||||
|
service.stop();
|
||||||
|
tick(service.intervalMs() * 2);
|
||||||
|
|
||||||
|
expect(loadOrdersSpy.calls.count()).toBe(callsAfterStart);
|
||||||
|
}));
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ import { LanguageService } from '../../../../services/language.service';
|
|||||||
import { TranslateService } from '../../../../i18n/translate.service';
|
import { TranslateService } from '../../../../i18n/translate.service';
|
||||||
|
|
||||||
const LAST_NOTIFIED_KEY = 'adminOrderWatcher.lastNotifiedOrderId.v1';
|
const LAST_NOTIFIED_KEY = 'adminOrderWatcher.lastNotifiedOrderId.v1';
|
||||||
|
const LAST_NOTIFIED_AT_KEY = 'adminOrderWatcher.lastNotifiedOrderCreatedAt.v1';
|
||||||
const LAST_ACKNOWLEDGED_KEY = 'adminOrderWatcher.lastAcknowledgedOrderId.v1';
|
const LAST_ACKNOWLEDGED_KEY = 'adminOrderWatcher.lastAcknowledgedOrderId.v1';
|
||||||
const POLL_INTERVAL_KEY = 'adminOrderWatcher.pollIntervalMs.v1';
|
const POLL_INTERVAL_KEY = 'adminOrderWatcher.pollIntervalMs.v1';
|
||||||
export const DEFAULT_POLL_INTERVAL_MS = 15000;
|
export const DEFAULT_POLL_INTERVAL_MS = 15000;
|
||||||
@@ -44,6 +45,7 @@ export class AdminOrderWatcherService {
|
|||||||
readonly intervalMs: Signal<number> = this.intervalMsSignal.asReadonly();
|
readonly intervalMs: Signal<number> = this.intervalMsSignal.asReadonly();
|
||||||
|
|
||||||
private lastNotifiedOrderId: string | null = this.storage.getItem(LAST_NOTIFIED_KEY);
|
private lastNotifiedOrderId: string | null = this.storage.getItem(LAST_NOTIFIED_KEY);
|
||||||
|
private lastNotifiedOrderCreatedAt: string | null = this.storage.getItem(LAST_NOTIFIED_AT_KEY);
|
||||||
private timerId: ReturnType<typeof setInterval> | null = null;
|
private timerId: ReturnType<typeof setInterval> | null = null;
|
||||||
private started = false;
|
private started = false;
|
||||||
|
|
||||||
@@ -56,9 +58,17 @@ export class AdminOrderWatcherService {
|
|||||||
this.scheduleNext();
|
this.scheduleNext();
|
||||||
}
|
}
|
||||||
|
|
||||||
setIntervalSeconds(seconds: number): void {
|
stop(): void {
|
||||||
|
if (this.timerId !== null) {
|
||||||
|
clearInterval(this.timerId);
|
||||||
|
this.timerId = null;
|
||||||
|
}
|
||||||
|
this.started = false;
|
||||||
|
}
|
||||||
|
|
||||||
|
setIntervalSeconds(seconds: number): boolean {
|
||||||
if (!Number.isFinite(seconds) || seconds < MIN_POLL_INTERVAL_MS / 1000) {
|
if (!Number.isFinite(seconds) || seconds < MIN_POLL_INTERVAL_MS / 1000) {
|
||||||
return;
|
return false;
|
||||||
}
|
}
|
||||||
const ms = Math.round(seconds * 1000);
|
const ms = Math.round(seconds * 1000);
|
||||||
this.intervalMsSignal.set(ms);
|
this.intervalMsSignal.set(ms);
|
||||||
@@ -66,14 +76,16 @@ export class AdminOrderWatcherService {
|
|||||||
if (this.started) {
|
if (this.started) {
|
||||||
this.scheduleNext();
|
this.scheduleNext();
|
||||||
}
|
}
|
||||||
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
markAllSeen(): void {
|
markAllSeen(): void {
|
||||||
const newestId = this.recentOrdersSignal()[0]?.id ?? null;
|
const newestId = this.recentOrdersSignal()[0]?.id;
|
||||||
this.lastAcknowledgedOrderIdSignal.set(newestId);
|
if (!newestId) {
|
||||||
if (newestId) {
|
return;
|
||||||
this.storage.setItem(LAST_ACKNOWLEDGED_KEY, newestId);
|
|
||||||
}
|
}
|
||||||
|
this.lastAcknowledgedOrderIdSignal.set(newestId);
|
||||||
|
this.storage.setItem(LAST_ACKNOWLEDGED_KEY, newestId);
|
||||||
}
|
}
|
||||||
|
|
||||||
private scheduleNext(): void {
|
private scheduleNext(): void {
|
||||||
@@ -99,10 +111,21 @@ export class AdminOrderWatcherService {
|
|||||||
|
|
||||||
const isFirstPoll = this.lastNotifiedOrderId === null;
|
const isFirstPoll = this.lastNotifiedOrderId === null;
|
||||||
const notifyIndex = isFirstPoll ? -1 : items.findIndex(order => order.id === this.lastNotifiedOrderId);
|
const notifyIndex = isFirstPoll ? -1 : items.findIndex(order => order.id === this.lastNotifiedOrderId);
|
||||||
const newOrders = isFirstPoll ? [] : (notifyIndex === -1 ? items : items.slice(0, notifyIndex));
|
// When the last-notified order id is no longer present in the current page
|
||||||
|
// (deleted, or more than RECENT_ORDERS_LIMIT orders arrived since the last
|
||||||
|
// poll), fall back to a timestamp comparison instead of treating the whole
|
||||||
|
// page as new - otherwise a single missing id could fire a burst of up to
|
||||||
|
// RECENT_ORDERS_LIMIT toasts.
|
||||||
|
const newOrders = isFirstPoll
|
||||||
|
? []
|
||||||
|
: notifyIndex !== -1
|
||||||
|
? items.slice(0, notifyIndex)
|
||||||
|
: items.filter(order => this.lastNotifiedOrderCreatedAt !== null && order.createdAt > this.lastNotifiedOrderCreatedAt);
|
||||||
|
|
||||||
this.lastNotifiedOrderId = items[0].id;
|
this.lastNotifiedOrderId = items[0].id;
|
||||||
|
this.lastNotifiedOrderCreatedAt = items[0].createdAt;
|
||||||
this.storage.setItem(LAST_NOTIFIED_KEY, this.lastNotifiedOrderId);
|
this.storage.setItem(LAST_NOTIFIED_KEY, this.lastNotifiedOrderId);
|
||||||
|
this.storage.setItem(LAST_NOTIFIED_AT_KEY, this.lastNotifiedOrderCreatedAt);
|
||||||
|
|
||||||
if (isFirstPoll) {
|
if (isFirstPoll) {
|
||||||
// Nothing existed to compare against yet - treat current orders as already
|
// Nothing existed to compare against yet - treat current orders as already
|
||||||
|
|||||||
@@ -49,6 +49,10 @@
|
|||||||
border-color: color-mix(in srgb, var(--primary-color) 45%, white);
|
border-color: color-mix(in srgb, var(--primary-color) 45%, white);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
.floating-note-clickable {
|
||||||
|
cursor: pointer;
|
||||||
|
}
|
||||||
|
|
||||||
@keyframes note-in {
|
@keyframes note-in {
|
||||||
from {
|
from {
|
||||||
opacity: 0;
|
opacity: 0;
|
||||||
|
|||||||
Reference in New Issue
Block a user