Repository navigation
Revert the Suppress Ortto contact event added for giveth-v6-core#457 - #141
Merged
Merged
Conversation
Reverts PR #140 (merge commit 7412c12). giveth-v6-core#457 was closed as not planned and its v6-core side is reverted in Giveth/giveth-v6-core#482, so nothing sends this event any more: the `Suppress Ortto contact` name, its inert activity, its Joi schema, its error message and the `resubscribe` handling on the sync were all dead the moment that merged. WHAT SURVIVES, and why this is a plain `git revert` rather than surgery: #140 did not only add the suppression, it also GENERALISED #426's single-event `isSyncOrttoContact` check into a `MUST_CONFIRM_ORTTO_EVENTS` set. Reverting restores the original single-event form, which is #426's confirm-or-fail contract exactly as it shipped and ran — the 502/422 mapping, the finite Ortto timeout, the coerced-payload forwarding, the trackId-dedup bypass and the missing-payload 400. That contract is load-bearing: v6-core advances `ortto_synced_email` only on a 2xx, so a false success there would strand a contact that was never created. Verified after the revert — all of it is still present, and no trace of the suppression is. `notificationService.ts` has not been touched since #140, so the revert applies cleanly with nothing else to preserve. THE SEED IS REMOVED FORWARD. The revert deletes 1757000000000, which means that migration's own down() can no longer run, so the row it created would otherwise sit in every deployed database with no code able to reach or explain it. Instead 1758000000000 deletes it, keeping the seed's shape-qualified WHERE so a row customised through AdminJS is left alone. Its down() is deliberately a no-op: re-seeding would restore an event activityCreator can no longer build, which would surface as a 500 rather than a working feature. Deploy order does not matter this time — v6-core has already stopped sending the event, and this only removes config nothing reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (9)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe PR removes the ChangesOrtto contact suppression removal
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The PR removes the obsolete Ortto suppression flow while preserving the existing contact-sync failure handling. It is mergeable with explicit owner awareness that the cleanup migration cannot restore deleted configuration, so rollback to an older application version requires a separate data-restoration plan. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. ✨ 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 |
Two corrections from review, both to comments in the new migration.
The no-op down() was justified with an inverted claim: re-seeding the row would
NOT surface as a 500. Traced through the post-revert code — `eventName` is a bare
Joi.string() so the request is accepted, the NotificationType row is found, but
`SEGMENT_METADATA_SCHEMA_VALIDATOR` no longer has a `suppressOrttoContact` entry,
so `segmentValidator` is undefined, the Ortto block's own `&& segmentValidator`
gate is falsy, activityCreator is never reached, and execution falls to the
`isOrttoSpecific` early return, which answers `{ success: true }`. So it is a
SILENT 200 for a suppression that never happened — precisely the false success the
#426 confirm-or-fail contract exists to prevent. That makes the no-op down() a
stronger decision than the original comment argued, not a weaker one.
Also recorded the deploy-order constraint, which was load-bearing and undocumented:
this must ship AFTER giveth-v6-core#482. The constraint is not about the seeded row
— it is that the revert removes `resubscribe` from `sendNotificationValidator`'s
payload whitelist, which is strict, so a v6-core still running #475 would have its
CONTACT SYNC calls 400'd, not merely its already-dead suppression calls.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reverts #140. Counterpart to Giveth/giveth-v6-core#482.
giveth-v6-core#457 was closed as not planned — the scenario it was built on (an admin editing a user's email) is not a real flow, and all subscribe/unsubscribe behaviour belongs to the #438 preference model. Its v6-core side is already reverted, so everything #140 added here is dead: nothing sends
Suppress Ortto contact, and nothing sendsresubscribeon the sync.What survives, and why this is a plain revert
#140 didn't only add the suppression — it also generalised #426's single-event
isSyncOrttoContactcheck into aMUST_CONFIRM_ORTTO_EVENTSset. Reverting restores the original single-event form, which is #426's confirm-or-fail contract exactly as it shipped and ran:ORTTO_REQUEST_TIMEOUT_MSon the syncThat contract is load-bearing — v6-core advances
ortto_synced_emailonly on a 2xx, so a false success would strand a contact that was never created. I verified after the revert that all of it is still present and no trace of the suppression is.notificationService.tshasn't been touched since #140, so the revert applies cleanly with nothing else to preserve.The seed is removed forward
The revert deletes
1757000000000-seedNotificationTypeSuppressOrttoContact.ts, which means that migration's owndown()can no longer run — the row it created would sit in every deployed database with no code able to reach or explain it. So1758000000000-removeNotificationTypeSuppressOrttoContact.tsdeletes it instead, keeping the seed's shape-qualified WHERE (name + microService + category + schemaValidator) so a row customised through AdminJS is left alone. Itsdown()is deliberately a no-op: re-seeding would restore an eventactivityCreatorcan no longer build, which surfaces as a 500 rather than a working feature.Deploy order: this must ship AFTER giveth-v6-core#482
Not because of the seeded row, but because this revert removes
resubscribefromsendNotificationValidator's payload whitelist — which is strict. A v6-core still running #475 sends that key on its contact sync, so it would get a 400 on the surviving #426 sync, not just on the already-dead suppression calls. #140 carried the mirror-image note in the other direction.giveth-v6-core#482 is already merged and deployed to staging (image
staging-c611678, verified), so staging is safe to take this now.Verification
tsc --noEmitclean,eslint src/ migrations/clean.notificationService.test.ts+orttoAdapter.test.ts: 26 passing.grep -rn "SUPPRESS_ORTTO\|suppress-ortto\|suppressOrtto\|resubscribe" src/ migrations/returns nothing.isSyncOrttoContact/ORTTO_CONTACT_SYNC_FAILEDsites are back in place.The full suite (
npm test) drops and recreates the test database, so I ran the two Ortto files standalone rather than against your local DB — worth a full run in CI.