Skip to content

refactor(cohort): one port over the list, with the vendor behind contact - #1172

Merged
ExtraToast merged 1 commit into
mainfrom
refactor/one-port-to-the-list
Sep 6, 2026
Merged

refactor(cohort): one port over the list, with the vendor behind contact#1172
ExtraToast merged 1 commit into
mainfrom
refactor/one-port-to-the-list

Conversation

@ExtraToast

@ExtraToast ExtraToast commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Closes #1169.

Why

cohort reached one Brevo list through two ports. CohortPort (with CohortPortRegistry,
BrevoCohortAdapter, MockCohortPort) and TargetStrategy (with TargetStrategies,
BrevoTargetStrategy, MockTargetStrategy) abstracted the same operations on the same list,
each had exactly one production implementation, both delegated to the same ContactListAdapter,
and their member types were structurally identical. Two registries dispatched on TargetSystem,
whose only cohort-valid value is BREVO.

BrevoTargetStrategy also injected the generated ContactsApi for folder and list paging, which
API ADR-019 forbids and which #1158 pinned in VendorClientArchitectureTest at six reaches.

What this achieves

One port over the external target, one test double behind it, and no vendor type reachable from
cohort — the pinned entry is deleted, and the rule now passes with PINNED empty.

How

TargetStrategy survives, unchanged in shape apart from one addition: handle(externalId)
answers with a target known only by its id, which is all the member, move and delete calls key on.
The two CohortPortRegistry consumers — CohortMembershipSyncService and
CohortRemediationService — now hold TargetStrategies and write through that.

CohortPortRegistry.require failed terminally (NonRetryableJobException) where
TargetStrategies.require answers a request with a 400, and both semantics are wanted: a job
cannot retry a bean into existence, and a controller should not answer 500 for an unknown system.
So TargetStrategies gained requireForJob, which the two job-path services call; require is
unchanged for the web paths.

Deleted: CohortPort, CohortPortRegistry, BrevoCohortAdapter, MockCohortPort and
BrevoCohortAdapterTest. MockTargetStrategy absorbed the mock's transaction-boundary recording
and seedMember / clear, so the ITs that used it keep asserting exactly what they asserted.

The catalog moved into contact. ContactListAdapter gained listAll(): List<ContactListRef>
and BrevoListAdapter now pages both folders and lists, wrapping a rate-limited page the way it
wraps every other failed fetch. BrevoTargetStrategy composes the two — folder names once, lists
once — and imports nothing from net.blueshell.clients. listFolders used to read a single page
of 50 and silently drop the rest; it now pages like the catalogue does.

The id shape: String wins at the port, Long stays inside contact. The reason CohortPort
gave for String is the right one — a Discord snowflake and a Google group address have to sit
beside Brevo's numeric list id — while ContactListAdapter speaks to systems that really do key by
number. That leaves exactly one conversion, BrevoTargetStrategy.toBrevoId, at the edge that knows
it is talking to Brevo; the second copy went with BrevoCohortAdapter.

Not in scope

ContactAdapterSyncTarget.push still does currentExternalId!!.toLong() in a generic base class,
so a non-numeric contact system would break the shared class rather than its own adapter. It is on
the contact-sync path rather than the cohort list path, and moving it means changing ContactAdapter's
id type across sync — a separate change, and reported rather than smuggled in here.

Worth a reviewer's attention

  • handle is an interface default method, so a Mockito mock of TargetStrategy returns null from
    it unless stubbed. That is why CohortTargetingService.deleteTarget still builds its bare
    ExternalTarget by hand: routing it through handle broke CohortTargetingServiceTest on the
    double rather than on the behaviour, and the test was left alone.
  • Three cohort tests name types this PR deletes and were edited only for that: the two ITs swap
    MockCohortPort for MockTargetStrategy, CohortRemediationServiceTest's RecordingCohortPort
    becomes a RecordingTargetStrategy, and CohortMembershipSyncServiceTest stubs a TargetStrategy
    instead of a CohortPort. No assertion changed meaning; the one message assertion follows the
    renamed exception text. Every other cohort test passes unchanged.
  • BrevoTargetStrategyTest's paging and rate-limit cases moved to BrevoListAdapterTest, since the
    paging did. What stayed in the cohort test is the mapping and filtering that is still cohort's.

Verification

  • ./gradlew :services:api:test — 901 passed, exit 0 (includes VendorClientArchitectureTest with
    PINNED empty, and the Modulith detection test).
  • ./gradlew :services:api:integrationTest — 1264 passed, exit 0, against a throwaway
    mariadb:11 on port 3407 (3306 was taken), with blueshell-test and blueshell created.

Diff breakdown added removed, scaled to the largest row.

api                                               +288   -576   23
  production         █████████░░░░░░░░░░░░░░░░░   +170   -314   14
  unit tests         ██████░░░░░░░░░░░░░          +109   -253    7
  integration tests  █░                             +9     -9    2

──────────────────────────────────────────────────────────────────
production                                        +170   -314
tests                                             +118   -262  0.69 test lines per prod line
total (hand-written)                              +288   -576  23 files

CohortPort and TargetStrategy abstracted the same Brevo list, each with one
production implementation delegating to the same ContactListAdapter. The richer
of the two survives; the other, its registry, its Brevo adapter and its mock are
gone.

BrevoTargetStrategy no longer injects the generated ContactsApi: folder and list
paging moved into BrevoListAdapter and is published through contact :: api, so
cohort reaches Brevo only through the wrapper and the pinned ADR-019 violation in
VendorClientArchitectureTest is deleted rather than edited.
@ExtraToast
ExtraToast merged commit 01546d5 into main Sep 6, 2026
39 of 40 checks passed
@ExtraToast
ExtraToast deleted the refactor/one-port-to-the-list branch September 6, 2026 08:48
@github-actions github-actions Bot mentioned this pull request Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

One Brevo list, two ports, and a raw client behind one of them

1 participant