OUT-4000 | Filter out email notifications according to iu notification preference - #1395
Conversation
…n preference Complete OUT-3929 IU email gating now that Copilot exposes GET /v1/internal-users/:id/notification-settings. Hybrid gating, split by send path: - Single emails: attach the resolved notificationSettingId on the body so the platform gates the send (re-enabled in NotificationService). - Grouped summaries: read the recipient IU's prefs and drop events for categories whose email is disabled before composing the summary (filterEventsForIuPreferences), fail-open on read error. - add CopilotAPI.getInternalUserNotificationSettings (not cached; live pref) - add InternalUserNotificationSettingsSchema - add iuEmailPreference helpers (disabledEmailSettingIds, isIuEmailGloballyOff) - tests for the helpers and the flush filter; flip obsolete ungated guard Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR applies IU notification preferences to email delivery. The main changes are:
Confidence Score: 4/5This is close, but the global email gate should be fixed before merging.
src/app/api/notification/iuEmailPreference.ts, src/types/common.ts Important Files Changed
Reviews (2): Last reviewed commit: "OUT-4000 | Fail closed on the global IU ..." | Re-trigger Greptile |
Only treat email as globally enabled when emailSettings is explicitly "active" (case-insensitive, trimmed). Any other value — a different disabled string, unexpected casing, or a missing field — is treated as not activated, so a global opt-out is never bypassed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Deployment failed with the following error: Learn More: https://vercel.link/multiple-function-regions |
|
please re review @greptile |
| // email notifications. Any other value — a different disabled string, unexpected casing, or a | ||
| // missing field — is treated as not activated, so a global opt-out is never bypassed. | ||
| export const isIuEmailGloballyOff = (settings: InternalUserNotificationSettings): boolean => | ||
| (settings.emailSettings ?? '').trim().toLowerCase() !== 'active' |
There was a problem hiding this comment.
Missing State Suppresses Email
This gate treats a missing emailSettings field as globally off, but the parsed settings schema accepts that field as optional. If the platform returns a valid payload with only per-category notifyAbout data, or an existing IU has not had this global field backfilled, grouped email filtering returns no events before checking the category settings. The IU then receives no grouped email even when their category email settings are enabled. The global gate should rely on the confirmed enum/default, or only suppress on confirmed disabled values.
Knowledge Base Used: Notifications flow: from task/comment event to delivered notification
…isableEmail emailSettings "not_active" does NOT mean the IU receives no email — a test IU with emailSettings=not_active still expects (and should get) comment emails. Gating on it suppressed every grouped IU email for that IU. The reliable signal is the per-category disableEmail flag, so gate on that alone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Context
IU emails currently send unconditionally — the app ignores each IU's notification preferences. OUT-3929 built the gating infrastructure but left it disabled, because the platform had no way to read an IU's per-category preference. That endpoint now exists:
Only our three categories carry our
appId+ anotificationSettingId(same ids the app already resolves viaresolveIuNotificationSettingId):newCommentOnATask→ COMMENT,newTaskAssigned→ ASSIGNED,taskCompleted→ COMPLETED. We match by id, so we don't depend on the platform's internal category key names.Approach — hybrid, split by send path
IU emails always divert into the 5-minute grouped buffer; in-product fires immediately. Gating is email-only (in-product unaffected).
notificationSettingIdon the notification body and let the platform gate the send.filterEventsForIuPreferencesreads the recipient IU's prefs and drops events for categories whose email is disabled before composing the summary. Closes the mixed-category-window gap a single id can't cover (an all-disabled IU sends nothing). Fails open on read error.Changes
src/utils/CopilotAPI.ts—getInternalUserNotificationSettings(id)via the existing_manualFetch+wrapWithRetrypattern (not cached — live preference state).src/types/common.ts—InternalUserNotificationSettingsSchema(notifyAboutas az.record, robust to platform categories we don't own).src/app/api/notification/iuEmailPreference.ts(new) — pure helpersdisabledEmailSettingIds(matches bynotificationSettingId) andisIuEmailGloballyOff.src/app/api/notification/notification.service.ts— re-enable resolvednotificationSettingIdincreate()+createBulkNotification().src/jobs/notifications/flush-grouped-email.ts— implementfilterEventsForIuPreferences.Tests
iuEmailPreference.test.ts; extended flush tests (per-category drop, all-disabled skip, global-off, fail-open); flipped the obsolete "ship ungated" guard innotification.service.test.ts.yarn tsc✓, affected suites green, prettier clean, lint 0 errors.isIuEmailGloballyOffassumesemailSettings === 'not_active'means "no IU emails at all" and suppresses all. Confirm the exactemailSettingsenum on staging before trusting the global gate — fall back to per-categorydisableEmailonly if it behaves differently.🤖 Generated with Claude Code