From 7eb0b82008dd6c1980182753188470160918cdd3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 19:39:09 +0000 Subject: [PATCH] fix(service-messaging)!: mark-read writes a read receipt only for a notification delivered to that user upsertReadReceipt inserts a read receipt only when the notification was delivered to the caller: a receipt keyed on them already exists (flipped in place, unchanged), or their inbox holds a message for it. Any other id writes no row, is not counted in readCount, and its event's organization is not read. The delivery check reads the caller's own inbox under INBOX_SYSTEM_CONTEXT, keyed on user_id. Claude-Session: https://claude.ai/code/session_01WMQprn46CND82KmY8sZWBu Co-authored-by: Claude --- .../22026-read-receipt-recipient-only.md | 17 +++ .../src/inbox-system-context.ts | 3 +- .../src/messaging-service.test.ts | 138 +++++++++++++++++- .../src/messaging-service.ts | 58 +++++++- .../src/read-receipt-organization.test.ts | 10 +- 5 files changed, 217 insertions(+), 9 deletions(-) create mode 100644 .changeset/22026-read-receipt-recipient-only.md diff --git a/.changeset/22026-read-receipt-recipient-only.md b/.changeset/22026-read-receipt-recipient-only.md new file mode 100644 index 00000000000..173f412407f --- /dev/null +++ b/.changeset/22026-read-receipt-recipient-only.md @@ -0,0 +1,17 @@ +--- +'@objectstack/service-messaging': minor +--- + +fix(service-messaging)!: mark-read writes a read receipt only for a notification delivered to that user + +Clause-②: no (narrowing) + + + +**BREAKING** (an accept-set narrowing), shipped as `minor` under the launch-window convention: `MessagingService.markRead` — the method behind the notifications mark-read door, and behind `markReadAsCaller` — writes a `read` receipt only for a notification that was delivered to that user. A read receipt belongs to a recipient (ADR-0030 keys it by recipient). + +- **Delivered** means a receipt keyed on that user already exists (it is flipped to `read` in place, as before), or the user's inbox holds a message for that notification. The inbox message is enough on its own, because the inbox channel's `delivered` receipt is best-effort. +- **Any other id** writes no receipt, is not counted in `readCount`, and its notification's organization is not read. The response shape `{ success, readCount }` is unchanged. +- **`markAllRead`** is unchanged: every id it sweeps comes from the user's own inbox. + +What changes for you: nothing in what you write. A `readCount` lower than the number of ids sent means some of them were not notifications delivered to that user, and nothing was written for those. diff --git a/packages/services/service-messaging/src/inbox-system-context.ts b/packages/services/service-messaging/src/inbox-system-context.ts index fe27b436d38..66d59c63425 100644 --- a/packages/services/service-messaging/src/inbox-system-context.ts +++ b/packages/services/service-messaging/src/inbox-system-context.ts @@ -8,7 +8,8 @@ * `countUnreadTotal`'s total), `readReceiptStates` * (`sys_notification_receipt`), and mark-read / mark-all-read: * `unreadNotificationIds`, `upsertReadReceipt` (its read, its update and its - * insert) and `notificationOrganization`. + * insert), `inboxMessageNames` (the delivery check's `sys_inbox_message` read) + * and `notificationOrganization`. * * Before this, each of those calls reached the data engine with no context at * all — no principal and no opt-in — and passed the security middleware only diff --git a/packages/services/service-messaging/src/messaging-service.test.ts b/packages/services/service-messaging/src/messaging-service.test.ts index 3d29e4462aa..135b45b624e 100644 --- a/packages/services/service-messaging/src/messaging-service.test.ts +++ b/packages/services/service-messaging/src/messaging-service.test.ts @@ -1,7 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. -import { describe, it, expect, beforeEach } from 'vitest'; -import { MessagingService } from './messaging-service.js'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { MessagingService, NOTIFICATION_EVENT_OBJECT } from './messaging-service.js'; import { InboxCallerError } from './inbox-caller.js'; import { MemoryNotificationOutbox } from './memory-outbox.js'; import type { Delivery, MessagingChannel, SendResult } from './channel.js'; @@ -1394,3 +1394,137 @@ describe('MessagingService — plugin-facing inbox read scoped to the authentica }); }); }); + +/** + * [#22026] A read receipt belongs to a RECIPIENT (ADR-0030 keys it by + * recipient). `markRead` inserts a `read` receipt only for a notification + * delivered to that user: a receipt keyed on them already exists (it is + * flipped, as before), or their inbox holds a message for it. Any other id + * writes no row, is not counted, and never has its event's organization read. + * + * Both halves are pinned on the same notification, so the only difference + * between the green half and the refused half is who asks. The doubles are + * this file's stateful `inboxEngine`, observed through spies. + */ +describe('[#22026] markRead writes a read receipt only for a notification delivered to that user', () => { + const logger = silentLogger(); + + /** + * One notification event, delivered to `u_recipient` only: their inbox + * message exists and its best-effort `delivered` receipt never landed — + * the case the insert limb exists for. + */ + function deliveredToOne() { + const engine = inboxEngine({ + inbox: [{ id: 'm1', user_id: 'u_recipient', notification_id: 'n1', title: 'A', created_at: '1' }], + }); + engine.store[NOTIFICATION_EVENT_OBJECT] = [{ id: 'n1', organization_id: 'org_1' }]; + const findOne = vi.spyOn(engine, 'findOne'); + const insert = vi.spyOn(engine, 'insert'); + const svc = new MessagingService({ logger, getData: () => engine }); + return { engine, findOne, insert, svc }; + } + const callsOn = (spy: { mock: { calls: any[][] } }, object: string) => + spy.mock.calls.filter(([o]) => o === object); + + it('the recipient half: an inbox message alone is delivery, so the receipt persists, stamped, and counts 1', async () => { + const { engine, findOne, svc } = deliveredToOne(); + + expect(await svc.markRead('u_recipient', ['n1'])).toEqual({ success: true, readCount: 1 }); + + expect(engine.store.sys_notification_receipt).toHaveLength(1); + expect(engine.store.sys_notification_receipt[0]).toMatchObject({ + notification_id: 'n1', user_id: 'u_recipient', channel: 'inbox', state: 'read', organization_id: 'org_1', + }); + expect(callsOn(findOne, NOTIFICATION_EVENT_OBJECT)).toHaveLength(1); + expect((await svc.listInbox('u_recipient')).unreadCount).toBe(0); + }); + + it('⛔ the non-recipient half: the same notification writes no row, counts 0, and its organization is never read', async () => { + const { engine, findOne, insert, svc } = deliveredToOne(); + + expect(await svc.markRead('u_other', ['n1'])).toEqual({ success: true, readCount: 0 }); + + expect(engine.store.sys_notification_receipt).toEqual([]); + expect(callsOn(insert, 'sys_notification_receipt')).toHaveLength(0); + expect(callsOn(findOne, NOTIFICATION_EVENT_OBJECT)).toHaveLength(0); + // Nothing of the recipient's moved either. + expect((await svc.listInbox('u_recipient')).unreadCount).toBe(1); + }); + + it('⛔ the in-process door answers the same: markReadAsCaller for a non-recipient counts 0 and writes nothing', async () => { + const { engine, svc } = deliveredToOne(); + + expect(await svc.markReadAsCaller({ userId: 'u_other' }, ['n1'])).toEqual({ success: true, readCount: 0 }); + expect(engine.store.sys_notification_receipt).toEqual([]); + }); + + it('a mixed call counts only the delivered id, and writes only its receipt', async () => { + const { engine, svc } = deliveredToOne(); + + expect(await svc.markRead('u_recipient', ['n1', 'n_never_delivered'])).toEqual({ success: true, readCount: 1 }); + expect(engine.store.sys_notification_receipt.map((r: any) => r.notification_id)).toEqual(['n1']); + }); + + it('the receipt half: a delivered receipt keyed on the user is flipped in place, with no inbox message (unchanged)', async () => { + const engine = inboxEngine({ + receipts: [{ id: 'r1', notification_id: 'n2', user_id: 'u_recipient', channel: 'inbox', state: 'delivered' }], + }); + const insert = vi.spyOn(engine, 'insert'); + const svc = new MessagingService({ logger, getData: () => engine }); + + expect(await svc.markRead('u_recipient', ['n2'])).toEqual({ success: true, readCount: 1 }); + expect(engine.store.sys_notification_receipt).toHaveLength(1); + expect(engine.store.sys_notification_receipt[0]).toMatchObject({ id: 'r1', state: 'read' }); + expect(insert).not.toHaveBeenCalled(); + }); + + it('the delivery check reads only the caller’s own inbox, under the system opt-in, projecting the id', async () => { + const { findOne, svc } = deliveredToOne(); + + await svc.markRead('u_other', ['n1']); + + const inboxReads = callsOn(findOne, 'sys_inbox_message'); + expect(inboxReads).toHaveLength(1); + expect(inboxReads[0][1]).toEqual({ where: { user_id: 'u_other', notification_id: 'n1' }, fields: ['id'] }); + expect(inboxReads[0][2]).toEqual({ context: { isSystem: true } }); + }); + + it('an inbox row with no notification_id: naming its row id writes nothing, and the row lists unread as it always did', async () => { + // listInbox looks read-state up only through a row's notification_id, + // so this row read as unread before the check existed, whatever + // receipt a mark-read wrote under its row id. It still does. + const engine = inboxEngine({ + inbox: [{ id: 'm_no_event', user_id: 'u_recipient', title: 'X', created_at: '1' }], + }); + const svc = new MessagingService({ logger, getData: () => engine }); + + expect(await svc.markRead('u_recipient', ['m_no_event'])).toEqual({ success: true, readCount: 0 }); + expect(engine.store.sys_notification_receipt).toEqual([]); + const listed = await svc.listInbox('u_recipient'); + expect(listed.notifications).toMatchObject([{ id: 'm_no_event', read: false }]); + expect(listed.unreadCount).toBe(1); + }); + + it('markAllRead needs no check of its own: every id it sweeps comes from the user’s own inbox, so each is delivered', async () => { + const engine = inboxEngine({ + inbox: [ + { id: 'm1', user_id: 'u_recipient', notification_id: 'n1', title: 'A', created_at: '1' }, + { id: 'm2', user_id: 'u_recipient', notification_id: 'n2', title: 'B', created_at: '2' }, + { id: 'm3', user_id: 'u_other', notification_id: 'n3', title: 'C', created_at: '3' }, + ], + receipts: [{ id: 'r1', notification_id: 'n1', user_id: 'u_recipient', channel: 'inbox', state: 'delivered' }], + }); + const svc = new MessagingService({ logger, getData: () => engine }); + + // n1 flips its delivered receipt; n2 has only its inbox message and is inserted. + expect(await svc.markAllRead('u_recipient')).toEqual({ success: true, readCount: 2 }); + const receipts = engine.store.sys_notification_receipt; + expect(receipts.map((r: any) => [r.notification_id, r.user_id, r.state])).toEqual([ + ['n1', 'u_recipient', 'read'], + ['n2', 'u_recipient', 'read'], + ]); + expect((await svc.listInbox('u_recipient')).unreadCount).toBe(0); + expect((await svc.listInbox('u_other')).unreadCount).toBe(1); + }); +}); diff --git a/packages/services/service-messaging/src/messaging-service.ts b/packages/services/service-messaging/src/messaging-service.ts index 98e45c6f63a..448159c99ce 100644 --- a/packages/services/service-messaging/src/messaging-service.ts +++ b/packages/services/service-messaging/src/messaging-service.ts @@ -697,8 +697,11 @@ export class MessagingService { * Mark specific notifications read by upserting their inbox receipts to * `read`. Updates the existing `delivered` receipt in place (keyed * `(notification_id, user_id, channel:'inbox')`); inserts one only when - * absent. `ids` are notification (event) ids. Returns the REST contract - * shape (`MarkNotificationsReadResponseSchema`): `{ success, readCount }`. + * absent AND the notification was delivered to that user — see + * {@link upsertReadReceipt}. An id that was never delivered to the user + * writes nothing and is not counted. `ids` are notification (event) ids. + * Returns the REST contract shape (`MarkNotificationsReadResponseSchema`): + * `{ success, readCount }`. * * **This is the REST door's method** — `INotificationService.markRead?`, * called by `runtime/src/domains/notifications.ts`, which binds `userId` @@ -884,7 +887,26 @@ export class MessagingService { return ids; } - /** Upsert a `read` receipt for one notification; returns 1 when it persisted. */ + /** + * Upsert a `read` receipt for one notification; returns 1 when it + * persisted, 0 when the notification was never delivered to the user. + * + * [#22026] A read receipt belongs to a RECIPIENT (ADR-0030 keys it by + * recipient). A notification counts as delivered to this user when either + * half holds, both read under INBOX_SYSTEM_CONTEXT and keyed on `user_id`: + * + * - a receipt keyed `(notification_id, user_id, channel:'inbox')` exists — + * the inbox channel's `delivered` receipt, or one already flipped. That + * is `flipToRead`'s own read: the receipt key IS the delivered-receipt + * key, so when it finds nothing there is no delivered receipt to find; + * - an inbox message for this user names the notification. It must + * suffice on its own, because the `delivered` receipt is best-effort + * (`writeDeliveredReceipt` logs and moves on, and the inbox row stands). + * + * Neither half ⇒ nothing is written, nothing is counted, and the event's + * organization is never read: an id the user was never sent cannot become + * a receipt of theirs, nor stamp one with that notification's organization. + */ private async upsertReadReceipt( data: IDataEngine, userId: string, @@ -907,8 +929,13 @@ export class MessagingService { if (await flipToRead()) return 1; - // No receipt yet — insert one. findOne→insert is check-then-act, so a - // concurrent mark-read (or the best-effort `delivered` write still in + // No receipt keyed on this user, so no `delivered` one either: the + // inbox message is the only delivery evidence left. Without it the id + // was never delivered to this user — write nothing, count nothing. + if (!(await this.inboxMessageNames(data, userId, notificationId))) return 0; + + // Delivered, and no receipt yet — insert one. findOne→insert is + // check-then-act, so a concurrent mark-read (or the best-effort `delivered` write still in // flight) can win the (notification_id, user_id, channel) unique index // between our read and write. Treat that collision as "someone else // created it" and flip the now-present row to `read` instead of failing. @@ -952,6 +979,27 @@ export class MessagingService { } } + /** + * [#22026] Does this user's inbox hold a message for `notificationId`? The + * inbox half of {@link upsertReadReceipt}'s delivery check. + * + * Keyed on `user_id` under INBOX_SYSTEM_CONTEXT, projecting `id` alone: the + * answer is a yes/no about the caller's own rows. NOT best-effort — a read + * that throws propagates to `markRead`, which logs it and leaves the id + * uncounted with no row written, the direction a check must fail in. + */ + private async inboxMessageNames( + data: IDataEngine, + userId: string, + notificationId: string, + ): Promise { + const message = await data.findOne(INBOX_OBJECT, { + where: { user_id: userId, notification_id: notificationId }, + fields: ['id'], + }, { context: INBOX_SYSTEM_CONTEXT }); + return message != null; + } + /** * [#11303] The organization of the `sys_notification` a receipt is about, * or `null`. diff --git a/packages/services/service-messaging/src/read-receipt-organization.test.ts b/packages/services/service-messaging/src/read-receipt-organization.test.ts index b3e9f500918..057f2632d5d 100644 --- a/packages/services/service-messaging/src/read-receipt-organization.test.ts +++ b/packages/services/service-messaging/src/read-receipt-organization.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect } from 'vitest'; import { MessagingService, NOTIFICATION_EVENT_OBJECT } from './messaging-service.js'; -import { RECEIPT_OBJECT } from './inbox-channel.js'; +import { INBOX_OBJECT, RECEIPT_OBJECT } from './inbox-channel.js'; import { assertEngineFindOnePredicate } from '@objectstack/metadata-core'; /** @@ -36,6 +36,11 @@ function silentLogger(): any { * A data engine holding one `sys_notification` row and recording every insert. * `findOne` answers the notification row for the event object and `undefined` * for the receipt (so `markRead` takes the insert limb, not the flip limb). + * + * [#22026] The reader is a RECIPIENT: their inbox holds a message for the + * notification, and only its best-effort `delivered` receipt never landed — + * the case this insert limb exists for. A reader with no inbox message gets no + * receipt at all (`messaging-service.test.ts`, the `[#22026]` block). */ function engineWithNotification(notificationOrg: string | null) { const inserted: Array<{ object: string; row: Record }> = []; @@ -49,6 +54,9 @@ function engineWithNotification(notificationOrg: string | null) { if (object === NOTIFICATION_EVENT_OBJECT) { return { id: 'evt_pin', organization_id: notificationOrg }; } + if (object === INBOX_OBJECT && opts?.where?.user_id === 'user_1' && opts?.where?.notification_id === 'evt_pin') { + return { id: 'msg_pin' }; + } void opts; return undefined; },