From ea0925148b9e0e50339f624bebf88e815256345d Mon Sep 17 00:00:00 2001 From: ali Date: Wed, 29 Jul 2026 20:07:03 +0330 Subject: [PATCH 1/7] Add "Sync Ortto contact" event for v6 contact sync (giveth-v6-core#426) New ORTTO-category NotificationType (givethio) that upserts an Ortto person WITHOUT sending an email. Unlike "Create Ortto profile" it always merges on the stable v6 user id (str:cm:v6-user-id) regardless of environment, so a canonical-email change re-points the same contact instead of creating a duplicate, and it stamps a durable bol:cm:sourced-from-v6 marker so v6-managed contacts are distinguishable from legacy v5-sourced ones. - notifications types: SYNC_ORTTO_CONTACT event + reuse the existing "created-profile" Ortto activity (no new Ortto activity required) - general/notificationType: NOTIFICATION_TYPE_NAMES + schemaValidator - segment validator syncOrttoContact { email, userId required; names optional/blank so wallet-only & Turnkey profiles still sync } - activityCreator case + always-merge-by-v6-user-id + marker - seed migration for the new NotificationType Requires the Ortto workspace to define custom fields str:cm:v6-user-id and bol:cm:sourced-from-v6. Co-Authored-By: Claude Opus 4.8 --- ...00-seedNotificationTypeSyncOrttoContact.ts | 41 +++++++++++++++++++ src/entities/notificationType.ts | 1 + src/services/notificationService.ts | 29 +++++++++++++ src/types/general.ts | 1 + src/types/notifications.ts | 10 +++++ .../segmentAndMetadataValidators.ts | 15 +++++++ 6 files changed, 97 insertions(+) create mode 100644 migrations/1732000000000-seedNotificationTypeSyncOrttoContact.ts diff --git a/migrations/1732000000000-seedNotificationTypeSyncOrttoContact.ts b/migrations/1732000000000-seedNotificationTypeSyncOrttoContact.ts new file mode 100644 index 0000000..9eeebc4 --- /dev/null +++ b/migrations/1732000000000-seedNotificationTypeSyncOrttoContact.ts @@ -0,0 +1,41 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; +import { + NOTIFICATION_CATEGORY, + NOTIFICATION_TYPE_NAMES, +} from '../src/types/general'; +import { MICRO_SERVICES } from '../src/utils/utils'; +import { + NotificationType, + SCHEMA_VALIDATORS_NAMES, +} from '../src/entities/notificationType'; + +// giveth-v6-core#426: the v6 → Ortto contact sync event. ORTTO-category so it +// sends no email and needs no wallet/notification-settings — sendNotification +// just upserts the Ortto person. Must be seeded for the `givethio` microservice +// or v6-core's requests 400 with INVALID_NOTIFICATION_TYPE. +const SyncOrttoContactNotificationType = [ + { + name: NOTIFICATION_TYPE_NAMES.SYNC_ORTTO_CONTACT, + description: NOTIFICATION_TYPE_NAMES.SYNC_ORTTO_CONTACT, + microService: MICRO_SERVICES.givethio, + category: NOTIFICATION_CATEGORY.ORTTO, + schemaValidator: SCHEMA_VALIDATORS_NAMES.SYNC_ORTTO_CONTACT, + }, +]; + +export class seedNotificationTypeSyncOrttoContact1732000000000 + implements MigrationInterface +{ + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.manager.save( + NotificationType, + SyncOrttoContactNotificationType, + ); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query( + `DELETE FROM notification_type WHERE "name" = 'Sync Ortto contact';`, + ); + } +} diff --git a/src/entities/notificationType.ts b/src/entities/notificationType.ts index 9a70611..89d1e7e 100644 --- a/src/entities/notificationType.ts +++ b/src/entities/notificationType.ts @@ -15,6 +15,7 @@ export const SCHEMA_VALIDATORS_NAMES = { SEND_EMAIL_CONFIRMATION: 'sendEmailConfirmation', SEND_USER_EMAIL_CONFIRMATION_CODE_FLOW: 'sendUserEmailConfirmationCodeFlow', CREATE_ORTTO_PROFILE: 'createOrttoProfile', + SYNC_ORTTO_CONTACT: 'syncOrttoContact', SUBSCRIBE_ONBOARDING: 'subscribeOnboarding', SUPERFLUID: 'userSuperTokensCritical', ADMIN_MESSAGE: 'adminMessage', diff --git a/src/services/notificationService.ts b/src/services/notificationService.ts index 85e5edb..be099e5 100644 --- a/src/services/notificationService.ts +++ b/src/services/notificationService.ts @@ -54,6 +54,14 @@ export const activityCreator = ( 'str:cm:userid': payload.userId?.toString(), }; break; + case NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT: + attributes = { + 'str:cm:email': payload.email, + 'str:cm:firstname': payload.firstName, + 'str:cm:lastname': payload.lastName, + 'str:cm:v6-user-id': payload.userId?.toString(), + }; + break; case NOTIFICATIONS_EVENT_NAMES.SUPER_TOKENS_BALANCE_DEPLETED: attributes = { 'str:cm:tokensymbol': payload.tokenSymbol, @@ -215,6 +223,27 @@ export const activityCreator = ( logger.debug('activityCreator() invalid ORTTO_EVENT_NAMES', orttoEventName); return; } + // giveth-v6-core#426: the v6 contact sync ALWAYS merges on the stable v6 user + // id (unlike the generic block below, which only does so in production), so a + // canonical-email change re-points the SAME Ortto person instead of creating + // a duplicate. It also stamps the durable `bol:cm:sourced-from-v6` marker so + // v6-managed contacts stay distinguishable from legacy v5-sourced ones. + if (orttoEventName === NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT) { + return { + activities: [ + { + activity_id: `act:cm:${ORTTO_EVENT_NAMES[orttoEventName]}`, + attributes, + fields: { + 'str::email': payload.email, + 'str:cm:v6-user-id': payload.userId?.toString(), + 'bol:cm:sourced-from-v6': true, + }, + }, + ], + merge_by: ['str:cm:v6-user-id'], + }; + } const fields = { 'str::email': payload.email, }; diff --git a/src/types/general.ts b/src/types/general.ts index c0e3059..e1f0e1d 100644 --- a/src/types/general.ts +++ b/src/types/general.ts @@ -53,6 +53,7 @@ export enum NOTIFICATION_TYPE_NAMES { YOUR_PROJECT_GOT_A_RANK = 'Your project got a rank', SUBSCRIBE_ONBOARDING = 'Subscribe onboarding', CREATE_ORTTO_PROFILE = 'Create Ortto profile', + SYNC_ORTTO_CONTACT = 'Sync Ortto contact', NOTIFY_REWARD_AMOUNT = 'Notify reward amount', } diff --git a/src/types/notifications.ts b/src/types/notifications.ts index 3d22ecc..982690e 100644 --- a/src/types/notifications.ts +++ b/src/types/notifications.ts @@ -49,6 +49,11 @@ export enum NOTIFICATIONS_EVENT_NAMES { SUPER_TOKENS_BALANCE_MONTH = 'One month left in stream balance', SUPER_TOKENS_BALANCE_DEPLETED = 'Stream balance depleted', CREATE_ORTTO_PROFILE = 'Create Ortto profile', + // v6 → Ortto contact sync (giveth-v6-core#426). Distinct from + // CREATE_ORTTO_PROFILE: it always merges on the stable v6 user id (so a + // canonical-email change re-points the same person instead of duplicating) + // and stamps a durable "sourced from v6" marker. + SYNC_ORTTO_CONTACT = 'Sync Ortto contact', SEND_EMAIL_CONFIRMATION = 'Send email confirmation', SEND_USER_EMAIL_CONFIRMATION_CODE_FLOW = 'Send email confirmation code flow', SUBSCRIBE_ONBOARDING = 'Subscribe onboarding', @@ -78,6 +83,11 @@ export const ORTTO_EVENT_NAMES = { [NOTIFICATIONS_EVENT_NAMES.PROJECT_BADGE_REVOKE_LAST_WARNING]: 'second-update-warning', [NOTIFICATIONS_EVENT_NAMES.CREATE_ORTTO_PROFILE]: 'created-profile', + // Reuse the existing "created-profile" Ortto activity so no new custom + // activity has to be defined in the Ortto workspace; the v6 contact sync is + // still distinguished on the person by the `bol:cm:sourced-from-v6` marker + // and the `str:cm:v6-user-id` field it merges on (see activityCreator). + [NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT]: 'created-profile', [NOTIFICATIONS_EVENT_NAMES.SEND_EMAIL_CONFIRMATION]: 'verification-form-email-verification', [NOTIFICATIONS_EVENT_NAMES.NOTIFY_REWARD_AMOUNT]: 'notify-reward', diff --git a/src/utils/validators/segmentAndMetadataValidators.ts b/src/utils/validators/segmentAndMetadataValidators.ts index 145ceb1..31d3dcb 100644 --- a/src/utils/validators/segmentAndMetadataValidators.ts +++ b/src/utils/validators/segmentAndMetadataValidators.ts @@ -157,6 +157,17 @@ const createOrttoProfileSegmentSchema = Joi.object({ userId: Joi.number().required(), }); +// giveth-v6-core#426 contact sync. Names are optional (and may be blank): +// wallet-only / Turnkey profiles frequently have no first/last name, and they +// must still sync. `email` + `userId` are the only required keys — `userId` is +// the stable merge key the Ortto person is deduped on. +const syncOrttoContactSegmentSchema = Joi.object({ + email: Joi.string().required(), + userId: Joi.number().required(), + firstName: Joi.string().allow('', null).optional(), + lastName: Joi.string().allow('', null).optional(), +}); + const sendEmailConfirmationSchema = Joi.object({ email: Joi.string().required(), verificationLink: Joi.string().required(), @@ -199,6 +210,10 @@ export const SEGMENT_METADATA_SCHEMA_VALIDATOR: { segment: createOrttoProfileSegmentSchema, metadata: null, }, + syncOrttoContact: { + segment: syncOrttoContactSegmentSchema, + metadata: null, + }, subscribeOnboarding: { segment: subscribeOnboardingSchema, metadata: null, From 988f361afb997e7259b0add908b0d45c2b704ffe Mon Sep 17 00:00:00 2001 From: ali Date: Wed, 29 Jul 2026 20:41:16 +0330 Subject: [PATCH 2/7] Address review: confirm Ortto upsert + dedicated inert sync activity (#426) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial-review fixes for the Sync Ortto contact event: - callOrttoActivity now returns a boolean success (logs, does not throw); sendNotification 502s the Sync Ortto contact event when the Ortto call fails, so v6-core sees a non-2xx and its reconcile cron retries instead of recording a false success. Other Ortto events are unchanged (fire-and-forget; the boolean is ignored). - Point the sync at a DEDICATED inert Ortto activity (act:cm:sync-ortto-contact) instead of reusing act:cm:created-profile, so a contact sync (which fires again on every canonical-email re-point) can never re-trigger a created-profile welcome journey/email — the sync must be side-effect-free. Ortto workspace prerequisites: activity `sync-ortto-contact` (no automation bound) and custom fields `str:cm:v6-user-id`, `bol:cm:sourced-from-v6`. Co-Authored-By: Claude Opus 4.8 --- src/adapters/emailAdapter/orttoAdapter.ts | 7 ++++++- .../emailAdapter/orttoAdapterInterface.ts | 6 +++++- src/adapters/emailAdapter/orttoMockAdapter.ts | 3 ++- src/services/notificationService.ts | 20 ++++++++++++++++++- src/types/notifications.ts | 12 ++++++----- src/utils/errorMessages.ts | 1 + 6 files changed, 40 insertions(+), 9 deletions(-) diff --git a/src/adapters/emailAdapter/orttoAdapter.ts b/src/adapters/emailAdapter/orttoAdapter.ts index 07968cb..f1cf2cb 100644 --- a/src/adapters/emailAdapter/orttoAdapter.ts +++ b/src/adapters/emailAdapter/orttoAdapter.ts @@ -4,7 +4,7 @@ import { OrttoAdapterInterface } from './orttoAdapterInterface'; import { MICRO_SERVICES } from '../../utils/utils'; export class OrttoAdapter implements OrttoAdapterInterface { - async callOrttoActivity(data: any, microService: string): Promise { + async callOrttoActivity(data: any, microService: string): Promise { try { if (!data) { throw new Error('callOrttoActivity input data is empty'); @@ -25,11 +25,16 @@ export class OrttoAdapter implements OrttoAdapterInterface { }; data.activities.map((a: any) => logger.debug('orttoActivityCall', a)); await axios.request(config); + return true; } catch (e) { logger.error('orttoActivityCall error', { error: e, data, }); + // Report failure (do not throw) so callers that need a confirmed upsert + // can react; existing fire-and-forget callers ignore the return value and + // keep their previous swallow-and-continue behavior. + return false; } } } diff --git a/src/adapters/emailAdapter/orttoAdapterInterface.ts b/src/adapters/emailAdapter/orttoAdapterInterface.ts index 3290e46..dfb3d22 100644 --- a/src/adapters/emailAdapter/orttoAdapterInterface.ts +++ b/src/adapters/emailAdapter/orttoAdapterInterface.ts @@ -1,3 +1,7 @@ export interface OrttoAdapterInterface { - callOrttoActivity(data: any, microService: string): Promise; + // Resolves `true` when Ortto accepted the activity/merge, `false` when the + // Ortto call failed (error is logged, not thrown). Callers that need a + // confirmed upsert (e.g. the giveth-v6-core#426 contact sync) branch on this + // instead of assuming success from a resolved promise. + callOrttoActivity(data: any, microService: string): Promise; } diff --git a/src/adapters/emailAdapter/orttoMockAdapter.ts b/src/adapters/emailAdapter/orttoMockAdapter.ts index 087ec3c..eae25b0 100644 --- a/src/adapters/emailAdapter/orttoMockAdapter.ts +++ b/src/adapters/emailAdapter/orttoMockAdapter.ts @@ -2,7 +2,8 @@ import { logger } from '../../utils/logger'; import { OrttoAdapterInterface } from './orttoAdapterInterface'; export class OrttoMockAdapter implements OrttoAdapterInterface { - async callOrttoActivity(data: any, microService: string): Promise { + async callOrttoActivity(data: any, microService: string): Promise { logger.debug('OrttoMockAdapter has been called', data, microService); + return true; } } diff --git a/src/services/notificationService.ts b/src/services/notificationService.ts index be099e5..0d5361a 100644 --- a/src/services/notificationService.ts +++ b/src/services/notificationService.ts @@ -356,7 +356,25 @@ export const sendNotification = async ( microService, ); if (data) { - await getEmailAdapter().callOrttoActivity(data, microService); + const orttoSucceeded = await getEmailAdapter().callOrttoActivity( + data, + microService, + ); + // giveth-v6-core#426: the contact-sync event needs a CONFIRMED upsert — + // v6-core advances its per-user sync marker only on a 2xx, and relies on + // its reconcile cron to retry otherwise. Surface an Ortto-side failure as + // a 502 for this event so the caller does not record a false success and + // silently stop retrying. Other Ortto events keep their fire-and-forget + // behavior (the boolean is ignored). + if ( + !orttoSucceeded && + body.eventName === NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT + ) { + throw new StandardError({ + message: errorMessages.ORTTO_CONTACT_SYNC_FAILED, + httpStatusCode: 502, + }); + } } emailStatus = EMAIL_STATUSES.SENT; } diff --git a/src/types/notifications.ts b/src/types/notifications.ts index 982690e..eff4f06 100644 --- a/src/types/notifications.ts +++ b/src/types/notifications.ts @@ -83,11 +83,13 @@ export const ORTTO_EVENT_NAMES = { [NOTIFICATIONS_EVENT_NAMES.PROJECT_BADGE_REVOKE_LAST_WARNING]: 'second-update-warning', [NOTIFICATIONS_EVENT_NAMES.CREATE_ORTTO_PROFILE]: 'created-profile', - // Reuse the existing "created-profile" Ortto activity so no new custom - // activity has to be defined in the Ortto workspace; the v6 contact sync is - // still distinguished on the person by the `bol:cm:sourced-from-v6` marker - // and the `str:cm:v6-user-id` field it merges on (see activityCreator). - [NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT]: 'created-profile', + // DEDICATED, inert activity (giveth-v6-core#426). We intentionally do NOT + // reuse 'created-profile': the contact sync must be side-effect-free (it + // creates/updates a contact but sends no email), and it fires again on every + // canonical-email re-point, so it must not be bound to any Ortto journey that + // could send a (duplicate) welcome email. This activity must exist in the + // Ortto workspace with NO automation bound to it. + [NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT]: 'sync-ortto-contact', [NOTIFICATIONS_EVENT_NAMES.SEND_EMAIL_CONFIRMATION]: 'verification-form-email-verification', [NOTIFICATIONS_EVENT_NAMES.NOTIFY_REWARD_AMOUNT]: 'notify-reward', diff --git a/src/utils/errorMessages.ts b/src/utils/errorMessages.ts index 294c77c..c211de7 100644 --- a/src/utils/errorMessages.ts +++ b/src/utils/errorMessages.ts @@ -184,4 +184,5 @@ export const errorMessages = { ERROR_IN_GETTING_ACCESS_TOKEN_BY_AUTHORIZATION_CODE: 'Error in getting accessToken by authorization code', ORTTO_SPECIFIC: 'Ortto specific notification', + ORTTO_CONTACT_SYNC_FAILED: 'Failed to upsert the Ortto contact', }; From 0a5ef637139d705ca43d7d9e19e2f6bd92b49e0e Mon Sep 17 00:00:00 2001 From: ali Date: Mon, 3 Aug 2026 22:18:16 +0330 Subject: [PATCH 3/7] Address CodeRabbit review: sanitize Ortto logs + add request timeout (#136) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - orttoAdapter: never log the payload (contact email / names / v6-user-id) or the raw Axios error (its `config` carries the X-Api-Key header and body); log only a sanitized summary (microService, activity ids, HTTP status, error message). - orttoAdapter: add a finite request timeout (ORTTO_REQUEST_TIMEOUT_MS, default 10s, validated) so a stalled Ortto connection rejects promptly instead of keeping the request — and the contact-sync 502 retry path — pending forever. - Document ORTTO_REQUEST_TIMEOUT_MS in config/example.env. - Add activityCreator regression tests for SYNC_ORTTO_CONTACT: dedicated inert activity id, merge_by str:cm:v6-user-id (env-independent), sourced-from-v6 marker, and optional-names omission. Co-Authored-By: Claude Opus 4.8 --- config/example.env | 2 + src/adapters/emailAdapter/orttoAdapter.ts | 26 +++++++- src/services/notificationService.test.ts | 75 +++++++++++++++++++++++ 3 files changed, 101 insertions(+), 2 deletions(-) diff --git a/config/example.env b/config/example.env index e1f0859..80d147c 100644 --- a/config/example.env +++ b/config/example.env @@ -16,6 +16,8 @@ SEGMENT_API_KEY=FAKE_API_KEY ORTTO_API_KEY=FAKE_API_KEY QACC_ORTTO_API_KEY=FAKE_API_KEY ORTTO_ACTIVITY_API=https://api-us.ortto.app/v1/activities/create +# Finite timeout (ms) for the Ortto activity request; defaults to 10000 if unset/invalid. +ORTTO_REQUEST_TIMEOUT_MS=10000 #JWT_AUTHORIZATION_ADAPTER=siweMicroservice JWT_AUTHORIZATION_ADAPTER=mock diff --git a/src/adapters/emailAdapter/orttoAdapter.ts b/src/adapters/emailAdapter/orttoAdapter.ts index f1cf2cb..7520887 100644 --- a/src/adapters/emailAdapter/orttoAdapter.ts +++ b/src/adapters/emailAdapter/orttoAdapter.ts @@ -3,6 +3,18 @@ import { logger } from '../../utils/logger'; import { OrttoAdapterInterface } from './orttoAdapterInterface'; import { MICRO_SERVICES } from '../../utils/utils'; +// Finite timeout so a stalled Ortto connection rejects promptly instead of +// keeping the notification-center request — and, for the contact sync, its 502 +// retry path — pending indefinitely. Sourced from ORTTO_REQUEST_TIMEOUT_MS with +// a validated fallback so a missing/garbage value never disables the timeout. +const DEFAULT_ORTTO_TIMEOUT_MS = 10_000; +const resolveOrttoTimeoutMs = (): number => { + const parsed = Number(process.env.ORTTO_REQUEST_TIMEOUT_MS); + return Number.isFinite(parsed) && parsed > 0 + ? parsed + : DEFAULT_ORTTO_TIMEOUT_MS; +}; + export class OrttoAdapter implements OrttoAdapterInterface { async callOrttoActivity(data: any, microService: string): Promise { try { @@ -17,6 +29,7 @@ export class OrttoAdapter implements OrttoAdapterInterface { method: 'post', maxBodyLength: Infinity, url: process.env.ORTTO_ACTIVITY_API, + timeout: resolveOrttoTimeoutMs(), headers: { 'X-Api-Key': apiKey as string, 'Content-Type': 'application/json', @@ -27,9 +40,18 @@ export class OrttoAdapter implements OrttoAdapterInterface { await axios.request(config); return true; } catch (e) { + // Log only a sanitized summary. NEVER log `data` (it carries the contact's + // email / names / v6-user-id) or the raw Axios error (its `config` holds + // the `X-Api-Key` header and the request body). Activity ids and the HTTP + // status are safe, non-sensitive identifiers that are enough to debug. + const activityIds = Array.isArray(data?.activities) + ? data.activities.map((a: any) => a?.activity_id) + : []; logger.error('orttoActivityCall error', { - error: e, - data, + microService, + activityIds, + status: axios.isAxiosError(e) ? e.response?.status : undefined, + message: e instanceof Error ? e.message : String(e), }); // Report failure (do not throw) so callers that need a confirmed upsert // can react; existing fire-and-forget callers ignore the return value and diff --git a/src/services/notificationService.test.ts b/src/services/notificationService.test.ts index a054d6c..69b937f 100644 --- a/src/services/notificationService.test.ts +++ b/src/services/notificationService.test.ts @@ -52,4 +52,79 @@ describe('activityCreator', () => { }), ); }); + + // giveth-v6-core#426 — the contact sync's cross-layer contract with v6-core. + it('builds the SYNC_ORTTO_CONTACT activity: dedicated inert activity, merges on the v6 user id, stamps the sourced-from-v6 marker', () => { + const payload = { + email: 'contact@example.com', + userId: 42, + firstName: 'Ada', + lastName: 'Lovelace', + }; + const result = activityCreator( + payload, + NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT, + MICRO_SERVICES.givethio, + ); + expect(result).to.deep.equal({ + activities: [ + { + activity_id: 'act:cm:sync-ortto-contact', + attributes: { + 'str:cm:email': 'contact@example.com', + 'str:cm:firstname': 'Ada', + 'str:cm:lastname': 'Lovelace', + 'str:cm:v6-user-id': '42', + }, + fields: { + 'str::email': 'contact@example.com', + 'str:cm:v6-user-id': '42', + 'bol:cm:sourced-from-v6': true, + }, + }, + ], + merge_by: ['str:cm:v6-user-id'], + }); + }); + + it('merges the SYNC_ORTTO_CONTACT person on the v6 user id regardless of ENVIRONMENT (AC4 on staging)', () => { + const original = process.env.ENVIRONMENT; + process.env.ENVIRONMENT = 'production'; + try { + const result = activityCreator( + { email: 'contact@example.com', userId: 7 }, + NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT, + MICRO_SERVICES.givethio, + ); + // Never merges by email (that would create a duplicate on re-point), and + // never falls through to the generic prod block's 'str:cm:user-id'. + expect(result.merge_by).to.deep.equal(['str:cm:v6-user-id']); + expect(result.activities[0].fields).to.deep.equal({ + 'str::email': 'contact@example.com', + 'str:cm:v6-user-id': '7', + 'bol:cm:sourced-from-v6': true, + }); + } finally { + process.env.ENVIRONMENT = original; + } + }); + + it('omits optional names for the SYNC_ORTTO_CONTACT activity (nameless wallet/Turnkey profiles still sync)', () => { + const result = activityCreator( + { email: 'contact@example.com', userId: 99 }, + NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT, + MICRO_SERVICES.givethio, + ); + // The merge key, email, and marker survive even with no names supplied. + expect(result.activities[0].activity_id).to.equal( + 'act:cm:sync-ortto-contact', + ); + expect(result.merge_by).to.deep.equal(['str:cm:v6-user-id']); + expect(result.activities[0].fields).to.deep.equal({ + 'str::email': 'contact@example.com', + 'str:cm:v6-user-id': '99', + 'bol:cm:sourced-from-v6': true, + }); + expect(result.activities[0].attributes['str:cm:v6-user-id']).to.equal('99'); + }); }); From d3dc74429c6e8b7a5fa24889f91abd90bf523b43 Mon Sep 17 00:00:00 2001 From: ali Date: Tue, 4 Aug 2026 02:52:17 +0330 Subject: [PATCH 4/7] Address CodeRabbit re-review: omit absent name attributes + safe env cleanup - activityCreator (SYNC_ORTTO_CONTACT): include str:cm:firstname / str:cm:lastname only when supplied, so a nameless profile never sends `undefined` attributes. - notificationService.test: assert the nameless activity omits both name attributes; restore process.env.ENVIRONMENT by delete-when-originally-unset instead of assigning undefined (which would leave the string "undefined"). Co-Authored-By: Claude Opus 4.8 --- src/services/notificationService.test.ts | 20 ++++++++++++++++++-- src/services/notificationService.ts | 11 +++++++++-- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/services/notificationService.test.ts b/src/services/notificationService.test.ts index 69b937f..1ea6881 100644 --- a/src/services/notificationService.test.ts +++ b/src/services/notificationService.test.ts @@ -105,7 +105,13 @@ describe('activityCreator', () => { 'bol:cm:sourced-from-v6': true, }); } finally { - process.env.ENVIRONMENT = original; + // Restore exactly: if ENVIRONMENT was unset, delete it rather than + // assigning `undefined` (which would leave the string "undefined"). + if (original === undefined) { + delete process.env.ENVIRONMENT; + } else { + process.env.ENVIRONMENT = original; + } } }); @@ -125,6 +131,16 @@ describe('activityCreator', () => { 'str:cm:v6-user-id': '99', 'bol:cm:sourced-from-v6': true, }); - expect(result.activities[0].attributes['str:cm:v6-user-id']).to.equal('99'); + // Absent names are omitted entirely — no `undefined` attributes are sent. + expect(result.activities[0].attributes).to.deep.equal({ + 'str:cm:email': 'contact@example.com', + 'str:cm:v6-user-id': '99', + }); + expect(result.activities[0].attributes).to.not.have.property( + 'str:cm:firstname', + ); + expect(result.activities[0].attributes).to.not.have.property( + 'str:cm:lastname', + ); }); }); diff --git a/src/services/notificationService.ts b/src/services/notificationService.ts index 0d5361a..15986b2 100644 --- a/src/services/notificationService.ts +++ b/src/services/notificationService.ts @@ -57,10 +57,17 @@ export const activityCreator = ( case NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT: attributes = { 'str:cm:email': payload.email, - 'str:cm:firstname': payload.firstName, - 'str:cm:lastname': payload.lastName, 'str:cm:v6-user-id': payload.userId?.toString(), }; + // Names are optional (wallet-only / Turnkey profiles frequently have + // none); include them only when supplied so we never send `undefined` + // attributes to Ortto. + if (payload.firstName) { + attributes['str:cm:firstname'] = payload.firstName; + } + if (payload.lastName) { + attributes['str:cm:lastname'] = payload.lastName; + } break; case NOTIFICATIONS_EVENT_NAMES.SUPER_TOKENS_BALANCE_DEPLETED: attributes = { From 9973664a1b96d8e122a665b6d6a5538c2f137d9b Mon Sep 17 00:00:00 2001 From: ali Date: Wed, 5 Aug 2026 12:01:58 +0330 Subject: [PATCH 5/7] =?UTF-8?q?Address=20code=20review=20(P0=E2=80=93P2):?= =?UTF-8?q?=20retryable=20classification,=20validation,=20no=20false=20suc?= =?UTF-8?q?cess?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - P0/P1: callOrttoActivity now returns {ok,retryable,status,responseBody}; sendNotification maps a transient Ortto failure (5xx/timeout/network) to 502 and a permanent one (4xx — bad payload / unprovisioned field/activity) to 422 for Sync Ortto contact, so a permanent error isn't retried as if transient. - P1: log Ortto's rejection body (no api key / no PII) so a 4xx is diagnosable. - P1: syncOrttoContact schema now userId Joi.number().integer().positive() and email Joi.string().trim().lowercase().email(); validateWithJoiSchema returns the coerced value and the sync forwards it, so one v6 user always maps to one stable merge key (no ' 42 '/'042'/'4e1' duplicates) and a bad email is rejected. - P1: close the silent-success holes — a missing segment (400), a missing segment validator (500), or a falsy activityCreator result (500) now fail the Sync Ortto contact request instead of returning {success:true}. - P2: scope the request timeout to the sync event only (other Ortto events keep their prior no-timeout behavior). - P2: seed migration is now idempotent (find-or-insert) so a pre-existing row can't fail up() and block staging startup. - Tests: adapter 4xx/5xx/network classification + response-body; sync validator coercion/rejection; nameless-activity attribute omission; safe env cleanup. Co-Authored-By: Claude Opus 4.8 --- ...00-seedNotificationTypeSyncOrttoContact.ts | 11 ++ .../emailAdapter/orttoAdapter.test.ts | 105 ++++++++++++++++++ src/adapters/emailAdapter/orttoAdapter.ts | 60 +++++----- .../emailAdapter/orttoAdapterInterface.ts | 32 +++++- src/adapters/emailAdapter/orttoMockAdapter.ts | 16 ++- src/services/notificationService.test.ts | 43 +++++++ src/services/notificationService.ts | 72 +++++++++--- src/utils/errorMessages.ts | 2 + .../segmentAndMetadataValidators.ts | 15 ++- src/validators/schemaValidators.ts | 4 + 10 files changed, 308 insertions(+), 52 deletions(-) create mode 100644 src/adapters/emailAdapter/orttoAdapter.test.ts diff --git a/migrations/1732000000000-seedNotificationTypeSyncOrttoContact.ts b/migrations/1732000000000-seedNotificationTypeSyncOrttoContact.ts index 9eeebc4..e03534c 100644 --- a/migrations/1732000000000-seedNotificationTypeSyncOrttoContact.ts +++ b/migrations/1732000000000-seedNotificationTypeSyncOrttoContact.ts @@ -27,6 +27,17 @@ export class seedNotificationTypeSyncOrttoContact1732000000000 implements MigrationInterface { public async up(queryRunner: QueryRunner): Promise { + // Idempotent: NotificationType.name is UNIQUE and this migration gates + // `start:server:staging`, so a save() that INSERTs a duplicate (row already + // hand-seeded on staging, created via AdminJS, or left by a down()/re-apply + // cycle) would raise a duplicate-key error and stop the service booting. + // Skip when the row already exists. + const existing = await queryRunner.manager.findOne(NotificationType, { + where: { name: NOTIFICATION_TYPE_NAMES.SYNC_ORTTO_CONTACT }, + }); + if (existing) { + return; + } await queryRunner.manager.save( NotificationType, SyncOrttoContactNotificationType, diff --git a/src/adapters/emailAdapter/orttoAdapter.test.ts b/src/adapters/emailAdapter/orttoAdapter.test.ts new file mode 100644 index 0000000..ef441fc --- /dev/null +++ b/src/adapters/emailAdapter/orttoAdapter.test.ts @@ -0,0 +1,105 @@ +import * as http from 'http'; +import { expect } from 'chai'; +import { OrttoAdapter } from './orttoAdapter'; +import { MICRO_SERVICES } from '../../utils/utils'; + +// giveth-v6-core#426: the contact sync needs callOrttoActivity to distinguish a +// transient Ortto failure (5xx / network — retryable) from a permanent one +// (4xx — a bad payload or an unprovisioned field/activity), so sendNotification +// can 502 the former and 422 the latter instead of retrying forever. +describe('OrttoAdapter.callOrttoActivity — failure classification', () => { + let server: http.Server; + let respond: (res: http.ServerResponse) => void = res => res.end('{}'); + const origApi = process.env.ORTTO_ACTIVITY_API; + const origKey = process.env.ORTTO_API_KEY; + + const sampleData = { + activities: [ + { activity_id: 'act:cm:sync-ortto-contact', attributes: {}, fields: {} }, + ], + merge_by: ['str:cm:v6-user-id'], + }; + + before(done => { + server = http.createServer((_req, res) => respond(res)); + server.listen(0, () => { + const port = (server.address() as { port: number }).port; + process.env.ORTTO_ACTIVITY_API = `http://127.0.0.1:${port}`; + process.env.ORTTO_API_KEY = 'test-key'; + done(); + }); + }); + + after(done => { + if (origApi === undefined) delete process.env.ORTTO_ACTIVITY_API; + else process.env.ORTTO_ACTIVITY_API = origApi; + if (origKey === undefined) delete process.env.ORTTO_API_KEY; + else process.env.ORTTO_API_KEY = origKey; + server.close(() => done()); + }); + + it('returns ok on a 2xx', async () => { + respond = res => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end('{}'); + }; + const r = await new OrttoAdapter().callOrttoActivity( + sampleData, + MICRO_SERVICES.givethio, + { timeoutMs: 2000 }, + ); + expect(r.ok).to.equal(true); + expect(r.retryable).to.equal(false); + }); + + it('classifies a 4xx as permanent (non-retryable) and carries the response body', async () => { + respond = res => { + res.writeHead(400, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: 'unknown field str:cm:v6-user-id' })); + }; + const r = await new OrttoAdapter().callOrttoActivity( + sampleData, + MICRO_SERVICES.givethio, + { timeoutMs: 2000 }, + ); + expect(r.ok).to.equal(false); + expect(r.retryable).to.equal(false); + expect(r.status).to.equal(400); + expect(r.responseBody).to.deep.equal({ + error: 'unknown field str:cm:v6-user-id', + }); + }); + + it('classifies a 5xx as transient (retryable)', async () => { + respond = res => { + res.writeHead(503); + res.end('{}'); + }; + const r = await new OrttoAdapter().callOrttoActivity( + sampleData, + MICRO_SERVICES.givethio, + { timeoutMs: 2000 }, + ); + expect(r.ok).to.equal(false); + expect(r.retryable).to.equal(true); + expect(r.status).to.equal(503); + }); + + it('classifies a connection failure (no response) as transient (retryable)', async () => { + const saved = process.env.ORTTO_ACTIVITY_API; + // Nothing listening → ECONNREFUSED, so there is no HTTP response/status. + process.env.ORTTO_ACTIVITY_API = 'http://127.0.0.1:1'; + try { + const r = await new OrttoAdapter().callOrttoActivity( + sampleData, + MICRO_SERVICES.givethio, + { timeoutMs: 2000 }, + ); + expect(r.ok).to.equal(false); + expect(r.retryable).to.equal(true); + expect(r.status).to.equal(undefined); + } finally { + process.env.ORTTO_ACTIVITY_API = saved; + } + }); +}); diff --git a/src/adapters/emailAdapter/orttoAdapter.ts b/src/adapters/emailAdapter/orttoAdapter.ts index 7520887..ce74caa 100644 --- a/src/adapters/emailAdapter/orttoAdapter.ts +++ b/src/adapters/emailAdapter/orttoAdapter.ts @@ -1,22 +1,18 @@ import axios from 'axios'; import { logger } from '../../utils/logger'; -import { OrttoAdapterInterface } from './orttoAdapterInterface'; +import { + CallOrttoActivityOptions, + OrttoActivityResult, + OrttoAdapterInterface, +} from './orttoAdapterInterface'; import { MICRO_SERVICES } from '../../utils/utils'; -// Finite timeout so a stalled Ortto connection rejects promptly instead of -// keeping the notification-center request — and, for the contact sync, its 502 -// retry path — pending indefinitely. Sourced from ORTTO_REQUEST_TIMEOUT_MS with -// a validated fallback so a missing/garbage value never disables the timeout. -const DEFAULT_ORTTO_TIMEOUT_MS = 10_000; -const resolveOrttoTimeoutMs = (): number => { - const parsed = Number(process.env.ORTTO_REQUEST_TIMEOUT_MS); - return Number.isFinite(parsed) && parsed > 0 - ? parsed - : DEFAULT_ORTTO_TIMEOUT_MS; -}; - export class OrttoAdapter implements OrttoAdapterInterface { - async callOrttoActivity(data: any, microService: string): Promise { + async callOrttoActivity( + data: any, + microService: string, + options?: CallOrttoActivityOptions, + ): Promise { try { if (!data) { throw new Error('callOrttoActivity input data is empty'); @@ -25,38 +21,50 @@ export class OrttoAdapter implements OrttoAdapterInterface { microService === MICRO_SERVICES.qacc ? process.env.QACC_ORTTO_API_KEY : process.env.ORTTO_API_KEY; - const config = { + const config: Record = { method: 'post', maxBodyLength: Infinity, url: process.env.ORTTO_ACTIVITY_API, - timeout: resolveOrttoTimeoutMs(), headers: { 'X-Api-Key': apiKey as string, 'Content-Type': 'application/json', }, data, }; + // Only the contact sync scopes a finite timeout (it needs a prompt failure + // for its retry path); other events keep their previous no-timeout + // behavior, so an Ortto latency spike never newly drops their activity. + if (options?.timeoutMs && options.timeoutMs > 0) { + config.timeout = options.timeoutMs; + } data.activities.map((a: any) => logger.debug('orttoActivityCall', a)); await axios.request(config); - return true; + return { ok: true, retryable: false }; } catch (e) { - // Log only a sanitized summary. NEVER log `data` (it carries the contact's - // email / names / v6-user-id) or the raw Axios error (its `config` holds - // the `X-Api-Key` header and the request body). Activity ids and the HTTP - // status are safe, non-sensitive identifiers that are enough to debug. + const status = axios.isAxiosError(e) ? e.response?.status : undefined; + // Ortto's rejection body says WHICH field / attribute / activity it + // rejected — it carries neither the `X-Api-Key` header nor any contact + // PII, so it is safe to log and is the only way to diagnose a 4xx. We + // still never log `data` (contact email / names / v6-user-id) or the raw + // Axios error (its `config` holds the api key and the request body). + const responseBody = axios.isAxiosError(e) ? e.response?.data : undefined; const activityIds = Array.isArray(data?.activities) ? data.activities.map((a: any) => a?.activity_id) : []; logger.error('orttoActivityCall error', { microService, activityIds, - status: axios.isAxiosError(e) ? e.response?.status : undefined, + status, message: e instanceof Error ? e.message : String(e), + responseBody, }); - // Report failure (do not throw) so callers that need a confirmed upsert - // can react; existing fire-and-forget callers ignore the return value and - // keep their previous swallow-and-continue behavior. - return false; + // A 4xx is a permanent problem (bad payload, or an unprovisioned custom + // field / activity) that retrying won't fix; everything else (5xx, + // timeout, network — no response) is transient and worth retrying. Report + // (do not throw) so fire-and-forget callers keep their swallow-and-continue + // behavior while confirmed-upsert callers can act on `retryable`. + const retryable = status === undefined || status >= 500; + return { ok: false, retryable, status, responseBody }; } } } diff --git a/src/adapters/emailAdapter/orttoAdapterInterface.ts b/src/adapters/emailAdapter/orttoAdapterInterface.ts index dfb3d22..ef7d8ac 100644 --- a/src/adapters/emailAdapter/orttoAdapterInterface.ts +++ b/src/adapters/emailAdapter/orttoAdapterInterface.ts @@ -1,7 +1,29 @@ +// Outcome of an Ortto activity/merge call. `ok` says whether Ortto accepted it; +// `retryable` distinguishes a transient failure (5xx / timeout / network — worth +// retrying) from a permanent one (4xx — a bad payload or unprovisioned +// field/activity that will keep failing until fixed). Callers that need a +// confirmed upsert (the giveth-v6-core#426 contact sync) map these onto HTTP +// statuses (502 vs 4xx) so the caller retries transient failures but not +// permanent ones. `status`/`responseBody` are carried for diagnostics; the +// error itself is logged, never thrown. +export interface OrttoActivityResult { + ok: boolean; + retryable: boolean; + status?: number; + responseBody?: unknown; +} + +export interface CallOrttoActivityOptions { + // Finite per-request timeout in ms. Only the contact sync passes one (it + // needs a prompt failure to feed its retry path); other events omit it and + // keep their previous no-timeout behavior. + timeoutMs?: number; +} + export interface OrttoAdapterInterface { - // Resolves `true` when Ortto accepted the activity/merge, `false` when the - // Ortto call failed (error is logged, not thrown). Callers that need a - // confirmed upsert (e.g. the giveth-v6-core#426 contact sync) branch on this - // instead of assuming success from a resolved promise. - callOrttoActivity(data: any, microService: string): Promise; + callOrttoActivity( + data: any, + microService: string, + options?: CallOrttoActivityOptions, + ): Promise; } diff --git a/src/adapters/emailAdapter/orttoMockAdapter.ts b/src/adapters/emailAdapter/orttoMockAdapter.ts index eae25b0..0bc4cf0 100644 --- a/src/adapters/emailAdapter/orttoMockAdapter.ts +++ b/src/adapters/emailAdapter/orttoMockAdapter.ts @@ -1,9 +1,19 @@ import { logger } from '../../utils/logger'; -import { OrttoAdapterInterface } from './orttoAdapterInterface'; +import { + OrttoActivityResult, + OrttoAdapterInterface, +} from './orttoAdapterInterface'; export class OrttoMockAdapter implements OrttoAdapterInterface { - async callOrttoActivity(data: any, microService: string): Promise { + // Lets a test drive a failure outcome (e.g. to exercise the contact-sync 502 + // path) without a real Ortto call; defaults to success. + public nextResult: OrttoActivityResult = { ok: true, retryable: false }; + + async callOrttoActivity( + data: any, + microService: string, + ): Promise { logger.debug('OrttoMockAdapter has been called', data, microService); - return true; + return this.nextResult; } } diff --git a/src/services/notificationService.test.ts b/src/services/notificationService.test.ts index 1ea6881..ab7f94f 100644 --- a/src/services/notificationService.test.ts +++ b/src/services/notificationService.test.ts @@ -2,6 +2,8 @@ import { expect } from 'chai'; import { activityCreator } from './notificationService'; import { NOTIFICATIONS_EVENT_NAMES } from '../types/notifications'; import { MICRO_SERVICES } from '../utils/utils'; +import { SEGMENT_METADATA_SCHEMA_VALIDATOR } from '../utils/validators/segmentAndMetadataValidators'; +import { validateWithJoiSchema } from '../validators/schemaValidators'; describe('activityCreator', () => { it('should create attributes for NOTIFY_REWARD_AMOUNT', () => { @@ -144,3 +146,44 @@ describe('activityCreator', () => { ); }); }); + +describe('syncOrttoContact segment validator (giveth-v6-core#426)', () => { + const schema = SEGMENT_METADATA_SCHEMA_VALIDATOR.syncOrttoContact.segment!; + + it('coerces email (trim + lowercase) and userId (→ integer) so the merge key is stable', () => { + const value = validateWithJoiSchema( + { email: ' Contact@Example.COM ', userId: '42' }, + schema, + ); + expect(value.email).to.equal('contact@example.com'); + expect(value.userId).to.equal(42); + // The coerced userId stringifies to one canonical merge key regardless of + // the input's representation (' 42 ', '042', 42 all → '42'). + expect(value.userId.toString()).to.equal('42'); + }); + + it("rejects a malformed email (it is Ortto's identity field)", () => { + expect(() => + validateWithJoiSchema({ email: 'not-an-email', userId: 42 }, schema), + ).to.throw(); + }); + + it('rejects a non-integer or non-positive userId (would split one user into several contacts)', () => { + expect(() => + validateWithJoiSchema({ email: 'a@b.com', userId: -1.5 }, schema), + ).to.throw(); + expect(() => + validateWithJoiSchema({ email: 'a@b.com', userId: 0 }, schema), + ).to.throw(); + expect(() => + validateWithJoiSchema({ email: 'a@b.com', userId: 'abc' }, schema), + ).to.throw(); + }); + + it('requires both email and userId', () => { + expect(() => validateWithJoiSchema({ userId: 42 }, schema)).to.throw(); + expect(() => + validateWithJoiSchema({ email: 'a@b.com' }, schema), + ).to.throw(); + }); +}); diff --git a/src/services/notificationService.ts b/src/services/notificationService.ts index 15986b2..b930c9a 100644 --- a/src/services/notificationService.ts +++ b/src/services/notificationService.ts @@ -20,6 +20,18 @@ import { getEmailAdapter } from '../adapters/adapterFactory'; import { NOTIFICATION_CATEGORY } from '../types/general'; import { MICRO_SERVICES } from '../utils/utils'; +// Finite timeout applied ONLY to the giveth-v6-core#426 contact sync, so a +// stalled Ortto connection fails promptly into its 502 retry path instead of +// leaving the request pending. Sourced from ORTTO_REQUEST_TIMEOUT_MS with a +// validated fallback so a missing/garbage value never disables it. +const DEFAULT_ORTTO_SYNC_TIMEOUT_MS = 10_000; +const resolveOrttoSyncTimeoutMs = (): number => { + const parsed = Number(process.env.ORTTO_REQUEST_TIMEOUT_MS); + return Number.isFinite(parsed) && parsed > 0 + ? parsed + : DEFAULT_ORTTO_SYNC_TIMEOUT_MS; +}; + export const activityCreator = ( payload: any, orttoEventName: NOTIFICATIONS_EVENT_NAMES, @@ -351,39 +363,71 @@ export const sendNotification = async ( eventName: body.eventName, }); + // giveth-v6-core#426: the contact sync must upsert or fail LOUDLY — it may + // never return a false success, or v6-core marks the user synced and stops + // retrying a contact that was never created. + const isSyncOrttoContact = + body.eventName === NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT; + if ( ((shouldSendEmail && body.sendSegment) || isOrttoSpecific) && segmentValidator ) { const emailData = body.segment?.payload; - validateWithJoiSchema(emailData, segmentValidator); + // Joi treats an absent object as valid, so a missing segment slips past the + // per-type validator. For the sync that is a bad request (400), not a + // silent success and not something to retry. + if (isSyncOrttoContact && !emailData) { + throw new StandardError({ + message: errorMessages.ORTTO_CONTACT_SYNC_INVALID_PAYLOAD, + httpStatusCode: 400, + }); + } + const validatedPayload = validateWithJoiSchema(emailData, segmentValidator); + // Only the sync forwards Joi's COERCED payload (normalised email + integer + // userId → a single stable merge key); other events keep their exact prior + // input to avoid any behavioural drift. const data = activityCreator( - emailData, + isSyncOrttoContact ? validatedPayload : emailData, body.eventName as NOTIFICATIONS_EVENT_NAMES, microService, ); if (data) { - const orttoSucceeded = await getEmailAdapter().callOrttoActivity( + const orttoResult = await getEmailAdapter().callOrttoActivity( data, microService, + isSyncOrttoContact + ? { timeoutMs: resolveOrttoSyncTimeoutMs() } + : undefined, ); - // giveth-v6-core#426: the contact-sync event needs a CONFIRMED upsert — - // v6-core advances its per-user sync marker only on a 2xx, and relies on - // its reconcile cron to retry otherwise. Surface an Ortto-side failure as - // a 502 for this event so the caller does not record a false success and - // silently stop retrying. Other Ortto events keep their fire-and-forget - // behavior (the boolean is ignored). - if ( - !orttoSucceeded && - body.eventName === NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT - ) { + // The sync needs a CONFIRMED upsert (v6-core advances its per-user marker + // only on a 2xx). Surface a transient Ortto failure (5xx / timeout / + // network) as 502 so v6-core retries, and a permanent one (4xx — a bad + // payload or an unprovisioned custom field/activity) as 422 so it is NOT + // retried forever. Other events ignore the result (fire-and-forget). + if (isSyncOrttoContact && !orttoResult.ok) { throw new StandardError({ message: errorMessages.ORTTO_CONTACT_SYNC_FAILED, - httpStatusCode: 502, + httpStatusCode: orttoResult.retryable ? 502 : 422, }); } + } else if (isSyncOrttoContact) { + // activityCreator produced nothing for an event that MUST upsert + // (e.g. the event fell out of ORTTO_EVENT_NAMES) — a misconfiguration, + // not a silent success. + throw new StandardError({ + message: errorMessages.ORTTO_CONTACT_SYNC_FAILED, + httpStatusCode: 500, + }); } emailStatus = EMAIL_STATUSES.SENT; + } else if (isSyncOrttoContact) { + // Reached the ORTTO branch but with no segment validator (a seed/config + // error): fail rather than returning a false success. + throw new StandardError({ + message: errorMessages.ORTTO_CONTACT_SYNC_FAILED, + httpStatusCode: 500, + }); } if (isOrttoSpecific) { diff --git a/src/utils/errorMessages.ts b/src/utils/errorMessages.ts index c211de7..002fbcd 100644 --- a/src/utils/errorMessages.ts +++ b/src/utils/errorMessages.ts @@ -185,4 +185,6 @@ export const errorMessages = { 'Error in getting accessToken by authorization code', ORTTO_SPECIFIC: 'Ortto specific notification', ORTTO_CONTACT_SYNC_FAILED: 'Failed to upsert the Ortto contact', + ORTTO_CONTACT_SYNC_INVALID_PAYLOAD: + 'Ortto contact sync requires a segment payload', }; diff --git a/src/utils/validators/segmentAndMetadataValidators.ts b/src/utils/validators/segmentAndMetadataValidators.ts index 31d3dcb..285432b 100644 --- a/src/utils/validators/segmentAndMetadataValidators.ts +++ b/src/utils/validators/segmentAndMetadataValidators.ts @@ -159,11 +159,18 @@ const createOrttoProfileSegmentSchema = Joi.object({ // giveth-v6-core#426 contact sync. Names are optional (and may be blank): // wallet-only / Turnkey profiles frequently have no first/last name, and they -// must still sync. `email` + `userId` are the only required keys — `userId` is -// the stable merge key the Ortto person is deduped on. +// must still sync. `email` + `userId` are the only required keys. +// +// `userId` is the STABLE Ortto merge key, so it must coerce to a single +// canonical value: `Joi.number().integer().positive()` rejects `-1.5`/`4e1` +// style inputs that would otherwise stringify into distinct merge keys and +// split one v6 user across several Ortto contacts. `email` is Ortto's identity +// field, so it is validated as an email and normalised (trim + lowercase) — +// the caller must use the COERCED value (see validateWithJoiSchema) so these +// transforms actually reach `activityCreator`. const syncOrttoContactSegmentSchema = Joi.object({ - email: Joi.string().required(), - userId: Joi.number().required(), + email: Joi.string().trim().lowercase().email().required(), + userId: Joi.number().integer().positive().required(), firstName: Joi.string().allow('', null).optional(), lastName: Joi.string().allow('', null).optional(), }); diff --git a/src/validators/schemaValidators.ts b/src/validators/schemaValidators.ts index ae7afcd..e80ea61 100644 --- a/src/validators/schemaValidators.ts +++ b/src/validators/schemaValidators.ts @@ -5,9 +5,13 @@ import { errorMessagesEnum } from '../utils/errorMessages'; const ethereumWalletAddressRegex = /^0x[a-fA-F0-9]{40}$/; const solanaWalletAddressRegex = /^[A-Za-z0-9]{43,44}$/; +// Returns Joi's COERCED value (trimmed/lowercased/number-parsed per the schema) +// so callers can forward the normalised payload rather than the raw input. +// Existing callers that ignore the return value are unaffected. export const validateWithJoiSchema = (data: any, schema: ObjectSchema) => { const validationResult = schema.validate(data); throwHttpErrorIfJoiValidatorFails(validationResult); + return validationResult.value; }; const throwHttpErrorIfJoiValidatorFails = ( From 2357755667c1a0b17663d47fb6fbf6a12b441623 Mon Sep 17 00:00:00 2001 From: ali Date: Wed, 5 Aug 2026 12:51:41 +0330 Subject: [PATCH 6/7] Address CodeRabbit: exclude sync from trackId dedup + redact mock-adapter log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - notificationService: determine isSyncOrttoContact before the duplicate-trackId check and exclude the sync event from that short-circuit, so a duplicate trackId can never skip the Ortto upsert and return a false success (v6-core would mark the contact synced and stop retrying). The sync sends no trackId today; this makes the guarantee explicit. - orttoMockAdapter: log only activity ids + microservice, never the full activity payload (email / names / v6-user-id) — no contact PII in logs even under EMAIL_ADAPTER=mock (CWE-532). Co-Authored-By: Claude Opus 4.8 --- src/adapters/emailAdapter/orttoMockAdapter.ts | 11 ++++++++++- src/services/notificationService.ts | 19 ++++++++++++------- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/adapters/emailAdapter/orttoMockAdapter.ts b/src/adapters/emailAdapter/orttoMockAdapter.ts index 0bc4cf0..42a2406 100644 --- a/src/adapters/emailAdapter/orttoMockAdapter.ts +++ b/src/adapters/emailAdapter/orttoMockAdapter.ts @@ -13,7 +13,16 @@ export class OrttoMockAdapter implements OrttoAdapterInterface { data: any, microService: string, ): Promise { - logger.debug('OrttoMockAdapter has been called', data, microService); + // Log only non-sensitive identifiers — the activity `data` carries contact + // PII (email / names / v6-user-id), so it must never be written to logs, + // even under EMAIL_ADAPTER=mock at debug level. + const activityIds = Array.isArray(data?.activities) + ? data.activities.map((a: any) => a?.activity_id) + : []; + logger.debug('OrttoMockAdapter has been called', { + microService, + activityIds, + }); return this.nextResult; } } diff --git a/src/services/notificationService.ts b/src/services/notificationService.ts index b930c9a..191b86a 100644 --- a/src/services/notificationService.ts +++ b/src/services/notificationService.ts @@ -298,7 +298,18 @@ export const sendNotification = async ( message?: string; }> => { const { userWalletAddress, projectId } = body; - if (body.trackId && (await findNotificationByTrackId(body.trackId))) { + // giveth-v6-core#426: the contact sync must upsert or fail LOUDLY — it may + // never return a false success, or v6-core marks the user synced and stops + // retrying a contact that was never created. + const isSyncOrttoContact = + body.eventName === NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT; + // Never let a duplicate trackId short-circuit the sync into a false success. + // It carries no trackId today, but guard explicitly so that stays true. + if ( + !isSyncOrttoContact && + body.trackId && + (await findNotificationByTrackId(body.trackId)) + ) { // We dont throw error in this case but dont create new notification neither return { success: true, @@ -363,12 +374,6 @@ export const sendNotification = async ( eventName: body.eventName, }); - // giveth-v6-core#426: the contact sync must upsert or fail LOUDLY — it may - // never return a false success, or v6-core marks the user synced and stops - // retrying a contact that was never created. - const isSyncOrttoContact = - body.eventName === NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT; - if ( ((shouldSendEmail && body.sendSegment) || isOrttoSpecific) && segmentValidator From 80d628603aad2a974e462ae1107c765b4880c192 Mon Sep 17 00:00:00 2001 From: ali Date: Mon, 10 Aug 2026 23:02:16 +0330 Subject: [PATCH 7/7] Address re-review: test 502/422 mapping, redact prod logs, identity-only, TLD MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - P1: add router tests driving the in-process mock adapter's result — 502 (retryable), 422 (non-retryable), 200 (confirmed), and another Ortto event still 200 on ok:false (fire-and-forget preserved). Exercises the sendNotification status-mapping the adapter unit tests don't reach; the nextResult hook is now used. - P2: redact the production adapter's success-path debug log (activity ids + microservice only, never the full activity/PII) and drop the raw segment payload from sendNotification's debug log (keys only) — matches the mock redaction and the catch-block's own PII rule (CWE-532). - P2: align the sync path to identity-only — activityCreator + segment schema now carry just email + userId (no names), so the Ortto activity only needs the str:cm:email and str:cm:v6-user-id attributes declared. - P3: relax email validation to email({ tlds: { allow: false } }) — keeps the structural check but doesn't reject unusual-but-real TLDs. Co-Authored-By: Claude Opus 4.8 --- src/adapters/emailAdapter/orttoAdapter.ts | 11 ++- src/routes/v1/notificationRouter.test.ts | 81 +++++++++++++++++++ src/services/notificationService.test.ts | 34 +------- src/services/notificationService.ts | 17 ++-- .../segmentAndMetadataValidators.ts | 20 +++-- 5 files changed, 112 insertions(+), 51 deletions(-) diff --git a/src/adapters/emailAdapter/orttoAdapter.ts b/src/adapters/emailAdapter/orttoAdapter.ts index ce74caa..0136068 100644 --- a/src/adapters/emailAdapter/orttoAdapter.ts +++ b/src/adapters/emailAdapter/orttoAdapter.ts @@ -37,7 +37,16 @@ export class OrttoAdapter implements OrttoAdapterInterface { if (options?.timeoutMs && options.timeoutMs > 0) { config.timeout = options.timeoutMs; } - data.activities.map((a: any) => logger.debug('orttoActivityCall', a)); + // Log only the activity ids, never the full activity: its `attributes` / + // `fields` carry contact PII (email, names, v6-user-id) and the default + // log level is DEBUG on a 30-day retained file (CWE-532). Matches the + // redaction applied on the error path below and in the mock adapter. + logger.debug('orttoActivityCall', { + microService, + activityIds: Array.isArray(data?.activities) + ? data.activities.map((a: any) => a?.activity_id) + : [], + }); await axios.request(config); return { ok: true, retryable: false }; } catch (e) { diff --git a/src/routes/v1/notificationRouter.test.ts b/src/routes/v1/notificationRouter.test.ts index 6f6fa7a..5a86226 100644 --- a/src/routes/v1/notificationRouter.test.ts +++ b/src/routes/v1/notificationRouter.test.ts @@ -15,6 +15,8 @@ import { errorMessages, errorMessagesEnum } from '../../utils/errorMessages'; import { findNotificationByTrackId } from '../../repositories/notificationRepository'; import { generateRandomString } from '../../utils/utils'; import { NOTIFICATION_TYPE_NAMES } from '../../types/general'; +import { getEmailAdapter } from '../../adapters/adapterFactory'; +import { OrttoMockAdapter } from '../../adapters/emailAdapter/orttoMockAdapter'; describe('/notifications POST test cases', sendNotificationTestCases); describe('/notificationsBulk POST test cases', sendBulkNotificationsTestCases); @@ -2388,3 +2390,82 @@ function sendBulkNotificationsTestCases() { } }); } + +// giveth-v6-core#426: the contact sync must map the Ortto upsert outcome onto +// the HTTP status (v6-core advances its per-user marker only on 2xx). Drives +// the in-process mock adapter's result and asserts sendNotification's mapping — +// the throw sites the four adapter unit tests don't reach. EMAIL_ADAPTER=mock +// in test env, and the server runs in-process, so this singleton is the one the +// request handler uses. +describe('/notifications Sync Ortto contact 502/422 mapping', () => { + const syncBody = { + eventName: NOTIFICATION_TYPE_NAMES.SYNC_ORTTO_CONTACT, + sendEmail: false, + sendSegment: true, + segment: { payload: { email: 'sync-test@example.com', userId: 123 } }, + }; + + let mock: OrttoMockAdapter; + + beforeEach(() => { + mock = getEmailAdapter() as OrttoMockAdapter; + }); + + afterEach(() => { + mock.nextResult = { ok: true, retryable: false }; + }); + + it('returns 502 when the Ortto upsert fails transiently (retryable)', async () => { + mock.nextResult = { ok: false, retryable: true }; + try { + await axios.post(sendNotificationUrl, syncBody, { + headers: { authorization: getGivethIoBasicAuth() }, + }); + assert.isTrue(false, 'expected a 502'); + } catch (e: any) { + assert.equal(e.response.status, 502); + } + }); + + it('returns 422 when the Ortto upsert fails permanently (non-retryable)', async () => { + mock.nextResult = { ok: false, retryable: false }; + try { + await axios.post(sendNotificationUrl, syncBody, { + headers: { authorization: getGivethIoBasicAuth() }, + }); + assert.isTrue(false, 'expected a 422'); + } catch (e: any) { + assert.equal(e.response.status, 422); + } + }); + + it('returns 200 on a confirmed upsert', async () => { + mock.nextResult = { ok: true, retryable: false }; + const result = await axios.post(sendNotificationUrl, syncBody, { + headers: { authorization: getGivethIoBasicAuth() }, + }); + assert.equal(result.status, 200); + }); + + it('leaves other Ortto events fire-and-forget (200) even when the adapter reports failure', async () => { + mock.nextResult = { ok: false, retryable: true }; + const result = await axios.post( + sendNotificationUrl, + { + eventName: NOTIFICATION_TYPE_NAMES.CREATE_ORTTO_PROFILE, + sendEmail: false, + sendSegment: true, + segment: { + payload: { + email: 'other@example.com', + userId: 456, + firstName: 'A', + lastName: 'B', + }, + }, + }, + { headers: { authorization: getGivethIoBasicAuth() } }, + ); + assert.equal(result.status, 200); + }); +}); diff --git a/src/services/notificationService.test.ts b/src/services/notificationService.test.ts index ab7f94f..761d8a5 100644 --- a/src/services/notificationService.test.ts +++ b/src/services/notificationService.test.ts @@ -56,7 +56,8 @@ describe('activityCreator', () => { }); // giveth-v6-core#426 — the contact sync's cross-layer contract with v6-core. - it('builds the SYNC_ORTTO_CONTACT activity: dedicated inert activity, merges on the v6 user id, stamps the sourced-from-v6 marker', () => { + it('builds the SYNC_ORTTO_CONTACT activity: identity-only, dedicated inert activity, merges on the v6 user id, stamps the sourced-from-v6 marker', () => { + // Names in the payload are ignored — the sync is identity-only. const payload = { email: 'contact@example.com', userId: 42, @@ -74,8 +75,6 @@ describe('activityCreator', () => { activity_id: 'act:cm:sync-ortto-contact', attributes: { 'str:cm:email': 'contact@example.com', - 'str:cm:firstname': 'Ada', - 'str:cm:lastname': 'Lovelace', 'str:cm:v6-user-id': '42', }, fields: { @@ -116,35 +115,6 @@ describe('activityCreator', () => { } } }); - - it('omits optional names for the SYNC_ORTTO_CONTACT activity (nameless wallet/Turnkey profiles still sync)', () => { - const result = activityCreator( - { email: 'contact@example.com', userId: 99 }, - NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT, - MICRO_SERVICES.givethio, - ); - // The merge key, email, and marker survive even with no names supplied. - expect(result.activities[0].activity_id).to.equal( - 'act:cm:sync-ortto-contact', - ); - expect(result.merge_by).to.deep.equal(['str:cm:v6-user-id']); - expect(result.activities[0].fields).to.deep.equal({ - 'str::email': 'contact@example.com', - 'str:cm:v6-user-id': '99', - 'bol:cm:sourced-from-v6': true, - }); - // Absent names are omitted entirely — no `undefined` attributes are sent. - expect(result.activities[0].attributes).to.deep.equal({ - 'str:cm:email': 'contact@example.com', - 'str:cm:v6-user-id': '99', - }); - expect(result.activities[0].attributes).to.not.have.property( - 'str:cm:firstname', - ); - expect(result.activities[0].attributes).to.not.have.property( - 'str:cm:lastname', - ); - }); }); describe('syncOrttoContact segment validator (giveth-v6-core#426)', () => { diff --git a/src/services/notificationService.ts b/src/services/notificationService.ts index 191b86a..e6cb139 100644 --- a/src/services/notificationService.ts +++ b/src/services/notificationService.ts @@ -67,19 +67,14 @@ export const activityCreator = ( }; break; case NOTIFICATIONS_EVENT_NAMES.SYNC_ORTTO_CONTACT: + // Identity-only: the sync carries just the email + the stable v6 user id + // (giveth-v6-core#426 sends no names). So the Ortto workspace only needs + // the two attributes below declared on the `sync-ortto-contact` activity — + // `str:cm:email` and `str:cm:v6-user-id`. attributes = { 'str:cm:email': payload.email, 'str:cm:v6-user-id': payload.userId?.toString(), }; - // Names are optional (wallet-only / Turnkey profiles frequently have - // none); include them only when supplied so we never send `undefined` - // attributes to Ortto. - if (payload.firstName) { - attributes['str:cm:firstname'] = payload.firstName; - } - if (payload.lastName) { - attributes['str:cm:lastname'] = payload.lastName; - } break; case NOTIFICATIONS_EVENT_NAMES.SUPER_TOKENS_BALANCE_DEPLETED: attributes = { @@ -367,7 +362,9 @@ export const sendNotification = async ( }, trackId: body.trackId, metadata: body.metadata, - payload: body.segment?.payload, + // Log only which segment keys were sent, never their values — the payload + // carries contact PII (email, names, v6-user-id) and this runs at DEBUG. + payloadKeys: body.segment?.payload ? Object.keys(body.segment.payload) : [], sendEmail: body.sendEmail, sendSegment: body.sendSegment, segmentValidator: !!segmentValidator, diff --git a/src/utils/validators/segmentAndMetadataValidators.ts b/src/utils/validators/segmentAndMetadataValidators.ts index 285432b..e9fbf94 100644 --- a/src/utils/validators/segmentAndMetadataValidators.ts +++ b/src/utils/validators/segmentAndMetadataValidators.ts @@ -157,22 +157,26 @@ const createOrttoProfileSegmentSchema = Joi.object({ userId: Joi.number().required(), }); -// giveth-v6-core#426 contact sync. Names are optional (and may be blank): -// wallet-only / Turnkey profiles frequently have no first/last name, and they -// must still sync. `email` + `userId` are the only required keys. +// giveth-v6-core#426 contact sync. Identity-only: `email` + `userId` are the +// only keys (no names — see the PR / SyncOrttoContactInput). // // `userId` is the STABLE Ortto merge key, so it must coerce to a single // canonical value: `Joi.number().integer().positive()` rejects `-1.5`/`4e1` // style inputs that would otherwise stringify into distinct merge keys and // split one v6 user across several Ortto contacts. `email` is Ortto's identity -// field, so it is validated as an email and normalised (trim + lowercase) — +// field, so it is validated structurally and normalised (trim + lowercase) — // the caller must use the COERCED value (see validateWithJoiSchema) so these -// transforms actually reach `activityCreator`. +// transforms actually reach `activityCreator`. `tlds: { allow: false }` keeps +// the structural check but skips the IANA TLD allowlist, so an unusual-but-real +// TLD isn't rejected (Ortto arbitrates the address); a typo'd address still +// fails downstream rather than being retried forever here. const syncOrttoContactSegmentSchema = Joi.object({ - email: Joi.string().trim().lowercase().email().required(), + email: Joi.string() + .trim() + .lowercase() + .email({ tlds: { allow: false } }) + .required(), userId: Joi.number().integer().positive().required(), - firstName: Joi.string().allow('', null).optional(), - lastName: Joi.string().allow('', null).optional(), }); const sendEmailConfirmationSchema = Joi.object({