Add "Sync Ortto contact" event for v6 contact sync (giveth-v6-core#426) - #136
Add "Sync Ortto contact" event for v6 contact sync (giveth-v6-core#426)#136ae2079 wants to merge 6 commits into
Conversation
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 <noreply@anthropic.com>
…(#426) 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 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 9 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a Sync Ortto contact notification type, validates and normalizes its payload, constructs a v6 identity-based Ortto activity, and maps Ortto outcomes to explicit HTTP errors. ChangesOrtto Contact Sync
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant NotificationService
participant JoiValidator
participant OrttoAdapter
participant Ortto
Caller->>NotificationService: Submit Sync Ortto contact payload
NotificationService->>JoiValidator: Validate and coerce payload
JoiValidator-->>NotificationService: Return normalized contact data
NotificationService->>OrttoAdapter: Send v6 identity merge activity
OrttoAdapter->>Ortto: Submit activity with timeout
Ortto-->>OrttoAdapter: Return success or failure
OrttoAdapter-->>NotificationService: Return OrttoActivityResult
NotificationService-->>Caller: Return success, 422, or 502
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/adapters/emailAdapter/orttoAdapter.ts (1)
7-37: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent the Ortto upsert from blocking the sync response.
callOrttoActivityawaitsaxios.request(config)with no finitetimeout, andsendNotificationwaits for this promise before it can throw the502used for contact-sync retries. Add a small, validated request timeout so a stalled Ortto connection fails back through the caller’s error path instead of keeping the request indefinitely pending.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/adapters/emailAdapter/orttoAdapter.ts` around lines 7 - 37, Update callOrttoActivity’s axios request configuration to include a small finite timeout, sourced from an existing configuration value or validated constant, so stalled Ortto requests reject promptly and preserve sendNotification’s 502 retry path.
🧹 Nitpick comments (1)
src/services/notificationService.ts (1)
57-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the new cross-layer contract.
Test the exact activity ID,
merge_by, v6 marker field, optional names, and both adapter outcomes:falsemust produce HTTP 502 only forSYNC_ORTTO_CONTACT, while existing Ortto events remain non-throwing.Also applies to: 226-245, 359-377
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/notificationService.ts` around lines 57 - 64, Extend regression tests around the SYNC_ORTTO_CONTACT handling to assert the exact activity ID, merge_by value, v6 marker field, and support for omitted optional first and last names. Cover both adapter outcomes: a false result returns HTTP 502 only for SYNC_ORTTO_CONTACT, while existing Ortto event types remain non-throwing and retain their current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/adapters/emailAdapter/orttoAdapter.ts`:
- Around line 28-37: Update the error handling in orttoActivityCall so
logger.error does not include the full data payload or raw Axios error/config.
Log only a safe allowlist of non-sensitive identifiers and a sanitized error
summary, excluding email, names, v6-user-id, headers, and request configuration.
---
Outside diff comments:
In `@src/adapters/emailAdapter/orttoAdapter.ts`:
- Around line 7-37: Update callOrttoActivity’s axios request configuration to
include a small finite timeout, sourced from an existing configuration value or
validated constant, so stalled Ortto requests reject promptly and preserve
sendNotification’s 502 retry path.
---
Nitpick comments:
In `@src/services/notificationService.ts`:
- Around line 57-64: Extend regression tests around the SYNC_ORTTO_CONTACT
handling to assert the exact activity ID, merge_by value, v6 marker field, and
support for omitted optional first and last names. Cover both adapter outcomes:
a false result returns HTTP 502 only for SYNC_ORTTO_CONTACT, while existing
Ortto event types remain non-throwing and retain their current behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e386be8-3da2-4edd-b689-d632eb18c574
📒 Files selected for processing (10)
migrations/1732000000000-seedNotificationTypeSyncOrttoContact.tssrc/adapters/emailAdapter/orttoAdapter.tssrc/adapters/emailAdapter/orttoAdapterInterface.tssrc/adapters/emailAdapter/orttoMockAdapter.tssrc/entities/notificationType.tssrc/services/notificationService.tssrc/types/general.tssrc/types/notifications.tssrc/utils/errorMessages.tssrc/utils/validators/segmentAndMetadataValidators.ts
…136) - 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 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/notificationService.test.ts`:
- Around line 112-129: Update the activity builder used by activityCreator for
SYNC_ORTTO_CONTACT so firstname and lastname attributes are added only when
values are supplied, rather than with undefined values. Extend the existing
nameless-profile test to assert that result.activities[0].attributes excludes
both str:cm:firstname and str:cm:lastname while preserving the existing
attributes.
- Around line 90-109: Update the test’s ENVIRONMENT cleanup around the
activityCreator scenario to track whether process.env.ENVIRONMENT existed before
setting it. In the finally block, restore the saved value when the variable
originally existed; otherwise delete process.env.ENVIRONMENT instead of
assigning the undefined value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d47a8b2f-5cf6-4fbd-a36b-70ebc6d44bc3
📒 Files selected for processing (3)
config/example.envsrc/adapters/emailAdapter/orttoAdapter.tssrc/services/notificationService.test.ts
|
Addressed the CodeRabbit review in
Typecheck clean; the new test assertions validated. |
…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 <noreply@anthropic.com>
|
Addressed the re-review in
Typecheck clean; assertions validated. |
Code review — P0–P2Reviewed at extra-high effort. Build is clean ( 🔴 P01. Every Ortto failure maps to a retryable 502, so a permanent 4xx is retried forever
This matters because of the deploy ordering the PR itself describes: the code ships before the manual Ortto workspace edit. During that window Ortto returns 400 on every request — the activity Suggested fix: branch on 🟠 P12. Joi's coerced value is discarded, so a string
Verified against joi 17.6:
Three distinct Suggested fix: 3. A request with no Joi treats Per this PR's own contract, v6-core treats non-2xx as retryable — so a permanently malformed request is retried indefinitely instead of being rejected once. Suggested fix: guard 4. Ortto's rejection body is no longer logged, making 400s undiagnosable The sanitized log keeps Combined with P0 above, an operator sees an endless 502 loop and cannot tell whether the custom field, the activity, or an attribute is the problem. Suggested fix: add 5. The "confirmed upsert" guarantee has silent-success holes The 502 fires only inside
In both cases control falls through to Suggested fix: for this event, treat a missing validator or falsy 6. The 502 path has no test, and the mock adapter makes it untestable The three new tests cover only Nothing in CI exercises the new throw, so every regression it introduces — the batch abort below, 4xx-as-retryable-502, the silent-success holes — ships green. Suggested fix: an injectable/stub adapter that can return 🟡 P27. One Ortto 502 rejects the entire bulk batch
Suggested fix: 8. The new 10s timeout silently changes behavior for every pre-existing Ortto event
The PR states "All other Ortto events keep their existing fire-and-forget behavior", but their success rate is now bounded by a new 10s ceiling. Suggest scoping the timeout to this event, or confirming p99 Ortto latency before picking 10s. 9.
Suggested fix: 10. The PII-sanitization rationale is contradicted two lines above, and in the caller The catch-block comment says the contact's email/names/v6-user-id must NEVER be logged, but the success path logs each full activity ( So the sanitization removes diagnostics (see P1 #4) without actually keeping PII out of the logs. Either redact those debug logs too, or stop justifying the loss of 11. The seed migration isn't idempotent, and it gates server startup
Suggested fix: an idempotent upsert ( 🤖 Review generated with Claude Code |
… false success
- 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 <noreply@anthropic.com>
|
Thanks @RamRamez — thorough review. Addressed in P0 #1 (4xx→502 retried forever): P1 #2 (userId coercion → duplicate merge keys): schema is now P1 #3 (missing segment → 500 loop): explicit P1 #4 (rejection body not logged): now logs P1 #5 (silent-success holes): for the sync event, a missing segment validator (500) and a falsy P1 #6 (no test for the throw): added P2 #8 (global timeout): the timeout is now applied only to the sync event; all other Ortto events keep their prior no-timeout behavior. P2 #9 (email not validated): P2 #10 (PII-log contradiction): resolved by #4 — the catch comment no longer justifies withholding P2 #11 (non-idempotent migration gates startup): Deferred — P2 #7 (bulk tsc clean; new logic validated standalone (validator + adapter classification). |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
migrations/1732000000000-seedNotificationTypeSyncOrttoContact.ts (1)
47-50: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve notification types that this migration did not insert.
Lines 35-40 accept a pre-existing row and skip insertion. TypeORM still marks the migration as applied. Line 49 then deletes that pre-existing AdminJS or manually seeded row during rollback.
Record seed ownership and delete only a row created by this migration. If ownership cannot be persisted, make
downpreserve the row rather than deleting data this migration does not own.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@migrations/1732000000000-seedNotificationTypeSyncOrttoContact.ts` around lines 47 - 50, Update the migration’s up/down logic around the notification type seed to track whether this migration actually inserted the “Sync Ortto contact” row. Make down delete only a row owned by this migration, and preserve pre-existing rows when insertion was skipped or ownership was not persisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/adapters/emailAdapter/orttoMockAdapter.ts`:
- Around line 12-17: Update callOrttoActivity in OrttoMockAdapter so
logger.debug no longer receives the complete data payload; log only
non-sensitive activity identifiers and the microService, ensuring mock-adapter
logs do not retain contact PII.
In `@src/services/notificationService.ts`:
- Around line 366-370: Move the isSyncOrttoContact determination before the
duplicate-track check in the notification handling flow. Update the
duplicate-track success path to bypass its short-circuit for SYNC_ORTTO_CONTACT,
allowing validation and the Ortto upsert to run; preserve existing duplicate
behavior for other events. Add a regression test covering contact sync with an
existing trackId.
---
Outside diff comments:
In `@migrations/1732000000000-seedNotificationTypeSyncOrttoContact.ts`:
- Around line 47-50: Update the migration’s up/down logic around the
notification type seed to track whether this migration actually inserted the
“Sync Ortto contact” row. Make down delete only a row owned by this migration,
and preserve pre-existing rows when insertion was skipped or ownership was not
persisted.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc956b8a-200f-409e-9e6e-0dfc366196c2
📒 Files selected for processing (10)
migrations/1732000000000-seedNotificationTypeSyncOrttoContact.tssrc/adapters/emailAdapter/orttoAdapter.test.tssrc/adapters/emailAdapter/orttoAdapter.tssrc/adapters/emailAdapter/orttoAdapterInterface.tssrc/adapters/emailAdapter/orttoMockAdapter.tssrc/services/notificationService.test.tssrc/services/notificationService.tssrc/utils/errorMessages.tssrc/utils/validators/segmentAndMetadataValidators.tssrc/validators/schemaValidators.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/validators/segmentAndMetadataValidators.ts
- src/utils/errorMessages.ts
…pter log - 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 <noreply@anthropic.com>
|
Addressed both in
|
2 participants
Summary
Companion notification-center change for Giveth/giveth-v6-core#426 (Sync v6 users to Ortto as contacts by canonical email). Adds a dedicated event that upserts an Ortto contact without sending any email. Pairs with giveth-v6-core PR #441 — deploy this first so the new NotificationType is seeded before v6-core starts emitting the event (otherwise v6's requests 400 with
INVALID_NOTIFICATION_TYPE).Changes
Sync Ortto contactevent (NOTIFICATIONS_EVENT_NAMES/NOTIFICATION_TYPE_NAMES), ORTTO category,givethiomicroservice, with a seed migration for theNotificationType.activityCreatorcase that, for this event, always merges the Ortto person on the stablestr:cm:v6-user-id(regardless ofENVIRONMENT) so a canonical-email change re-points the same contact instead of creating a duplicate, and stamps a durablebol:cm:sourced-from-v6person-field marker so v6-managed contacts stay distinguishable from legacy v5 ones. Uses a dedicated inert activityact:cm:sync-ortto-contact(notcreated-profile) so the sync can never re-fire a welcome journey.syncOrttoContactsegment validator (email,userIdrequired;firstName/lastNameoptional/blank) so nameless wallet/Turnkey profiles still sync.callOrttoActivitynow returns a boolean success (logs, still doesn't throw); for theSync Ortto contactevent,sendNotificationreturns a 502 when the Ortto upsert fails, so v6-core records a false success only when the upsert is confirmed and its reconcile cron retries otherwise. All other Ortto events keep their existing fire-and-forget behavior.Deploy prerequisite (Ortto workspace)
The Ortto workspace must define custom fields
str:cm:v6-user-idandbol:cm:sourced-from-v6, and the activitysync-ortto-contactwith no automation/journey bound to it.How to Test
yarn db:migrate:run:local) so theSync Ortto contacttype is seeded; setEMAIL_ADAPTER=mock./v1/thirdParty/notifications(Basic auth, givethio) with{ eventName: "Sync Ortto contact", segment: { payload: { email, userId } } }.str:cm:v6-user-id, setsbol:cm:sourced-from-v6: true, usesact:cm:sync-ortto-contact, and that no email is sent.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes