diff --git a/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortLedgerAutoflushIT.kt b/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortLedgerAutoflushIT.kt index a56a4d089..0b1a57c6e 100644 --- a/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortLedgerAutoflushIT.kt +++ b/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortLedgerAutoflushIT.kt @@ -10,7 +10,7 @@ import net.blueshell.api.cohort.persistence.CohortMemberRepository import net.blueshell.api.cohort.persistence.CohortRepository import net.blueshell.api.cohort.persistence.CohortSubjectRepository import net.blueshell.api.cohort.persistence.state -import net.blueshell.api.platform.integration.mock.MockCohortPort +import net.blueshell.api.platform.integration.mock.MockTargetStrategy import net.blueshell.api.sync.persistence.ExternalIdMapping import net.blueshell.api.sync.persistence.ExternalIdMappingRepository import net.blueshell.api.shared.enums.Role @@ -41,11 +41,11 @@ class CohortLedgerAutoflushIT : UserTestSupport() { private lateinit var remediation: CohortRemediationService @Autowired - private lateinit var mockCohortPort: MockCohortPort + private lateinit var mockTarget: MockTargetStrategy @BeforeEach - fun resetCohortPort() { - mockCohortPort.clear() + fun resetTarget() { + mockTarget.clear() } @Test @@ -65,7 +65,7 @@ class CohortLedgerAutoflushIT : UserTestSupport() { ), ) externalIds.saveAndFlush(ExternalIdMapping("USER", user.id!!, TargetSystem.BREVO.name, "ext-1")) - mockCohortPort.seedMember("ext-1", "list-99", "Ada Remote") + mockTarget.seedMember("ext-1", "list-99", "Ada Remote") assertThatCode { remediation.verifyCohort(cohort.id!!) }.doesNotThrowAnyException() diff --git a/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortProviderTransactionBoundaryIT.kt b/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortProviderTransactionBoundaryIT.kt index 188e9c498..a564d4a36 100644 --- a/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortProviderTransactionBoundaryIT.kt +++ b/services/api/src/integrationTest/kotlin/net/blueshell/api/cohort/domain/CohortProviderTransactionBoundaryIT.kt @@ -6,7 +6,7 @@ import net.blueshell.api.cohort.persistence.CohortSubject import net.blueshell.api.cohort.persistence.CohortSubjectType import net.blueshell.api.cohort.persistence.CohortRepository import net.blueshell.api.cohort.persistence.CohortSubjectRepository -import net.blueshell.api.platform.integration.mock.MockCohortPort +import net.blueshell.api.platform.integration.mock.MockTargetStrategy import net.blueshell.api.sync.api.ExternalIdMappingService.Companion.USER_AGGREGATE import net.blueshell.api.sync.persistence.ExternalIdMapping import net.blueshell.api.sync.persistence.ExternalIdMappingRepository @@ -24,8 +24,8 @@ import tools.jackson.databind.ObjectMapper * path. [AbstractJsonJobHandler.handle][net.blueshell.api.jobs.api.AbstractJsonJobHandler] * is `@Transactional`, so a job is always dispatched with a transaction * active. The application services suspend it (`PROPAGATION_NOT_SUPPORTED`) - * around every [CohortPort][net.blueshell.api.cohort.domain.CohortPort] - * call; [MockCohortPort] records whether a transaction was actually active + * around every [TargetStrategy][net.blueshell.api.cohort.domain.TargetStrategy] + * call; [MockTargetStrategy] records whether a transaction was actually active * at each call so we can assert it never is. */ @SpringBootTest @@ -41,7 +41,7 @@ class CohortProviderTransactionBoundaryIT : UserTestSupport() { @Autowired private lateinit var externalIds: ExternalIdMappingRepository - @Autowired private lateinit var port: MockCohortPort + @Autowired private lateinit var port: MockTargetStrategy @Autowired private lateinit var objectMapper: ObjectMapper diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/ModuleMetadata.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/ModuleMetadata.kt index 8fd46791e..ed6523ce6 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/ModuleMetadata.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/cohort/ModuleMetadata.kt @@ -7,8 +7,8 @@ import org.springframework.modulith.PackageInfo * Code-defined audiences — "active members", "paid for this period" — evaluated into `CohortMember` * rows and reconciled against the mailing list or group that stands for them in an external system. * - * `port/in` is what other modules may call and `port/out` is the driven side, one `CohortPort` per - * target system; neither is a REST surface, the controllers under `adapter/web` are. + * One `TargetStrategy` per target system stands between the module and that system; the REST + * surface is the controllers under `web`. */ @PackageInfo @ApplicationModule( @@ -26,7 +26,7 @@ import org.springframework.modulith.PackageInfo // committees — this is a definition reaching into persistence, and it // wants a projection published through committee :: api instead. "committee :: entities", - // List membership is pushed through ContactListAdapter. + // The list catalogue, its folders and its membership all go through ContactListAdapter. "contact :: api", // Period cohorts are built from ContributionService and // ContributionPeriodService, and react to ContributionChanged. diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoCohortAdapter.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoCohortAdapter.kt deleted file mode 100644 index 2752f9609..000000000 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoCohortAdapter.kt +++ /dev/null @@ -1,57 +0,0 @@ -package net.blueshell.api.cohort.domain - -import net.blueshell.api.contact.api.ContactListAdapter -import net.blueshell.api.shared.enums.TargetSystem -import net.blueshell.api.shared.enums.ContactSystem -import org.springframework.context.annotation.Profile -import org.springframework.stereotype.Service - -/** - * Brevo's [CohortPort] implementation. Delegates to the existing - * Brevo [ContactListAdapter] bean for the actual HTTP calls so we - * inherit its recovery semantics (idempotent add, contact-gone - * disambiguation, ...) without duplicating client wiring. - */ -@Service -@Profile("!test & !dev") -class BrevoCohortAdapter( - contactListAdapters: List, -) : CohortPort { - - private val delegate: ContactListAdapter = contactListAdapters.single { it.system == ContactSystem.BREVO } - - override val system: TargetSystem = TargetSystem.BREVO - - override fun createCohort(label: String, hint: String?): String = - delegate.createList(label, hint).toString() - - override fun addMember(externalUserId: String, externalCohortId: String) { - delegate.addToList( - externalUserId.toBrevoId("externalUserId", "addMember"), - externalCohortId.toBrevoId("externalCohortId", "addMember"), - ) - } - - override fun removeMember(externalUserId: String, externalCohortId: String) { - delegate.removeFromList( - externalUserId.toBrevoId("externalUserId", "removeMember"), - externalCohortId.toBrevoId("externalCohortId", "removeMember"), - ) - } - - override fun deleteCohort(externalCohortId: String) { - delegate.deleteList(externalCohortId.toBrevoId("externalCohortId", "deleteCohort")) - } - - override fun listMembers(externalCohortId: String): List = - delegate.listMembers(externalCohortId.toBrevoId("externalCohortId", "listMembers")) - .map { MemberRef(it.externalUserId.toString(), it.email) } - // Never surface a blank external id: it would classify as an - // INVALID ledger row downstream (see CohortMemberState). - .filter { it.externalUserId.isNotBlank() } - - private fun String.toBrevoId(field: String, operation: String): Long = - toLongOrNull() ?: throw InvalidExternalIdException( - "Brevo $operation: $field \"$this\" is not a valid numeric id" - ) -} diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategy.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategy.kt index 48b443825..93fcd0636 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategy.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategy.kt @@ -2,22 +2,21 @@ package net.blueshell.api.cohort.domain import net.blueshell.api.cohort.persistence.CohortKind import net.blueshell.api.contact.api.ContactListAdapter -import net.blueshell.api.contact.api.ContactServiceException +import net.blueshell.api.contact.api.ContactListRef import net.blueshell.api.shared.enums.ContactSystem import net.blueshell.api.shared.enums.TargetSystem -import net.blueshell.clients.brevo.api.ContactsApi -import net.blueshell.clients.brevo.model.GetLists200ResponseListsInner -import net.blueshell.clients.brevo.model.GetContactsSortParameter -import org.slf4j.LoggerFactory import org.springframework.context.annotation.Profile import org.springframework.stereotype.Service -import org.springframework.web.client.RestClientResponseException +/** + * Brevo's [TargetStrategy], stated entirely in terms of [ContactListAdapter]: the catalogue, the + * folders and the membership writes are all that module's, so nothing Brevo-shaped reaches here + * beyond the numeric id its lists are keyed by (API ADR-019). + */ @Service @Profile("!test & !dev") class BrevoTargetStrategy( contactListAdapters: List, - private val contactsApi: ContactsApi, ) : TargetStrategy { private val lists = contactListAdapters.single { it.system == ContactSystem.BREVO } @@ -40,7 +39,9 @@ class BrevoTargetStrategy( override fun catalog(query: String?): List { val q = query?.trim()?.lowercase().orEmpty() - return listTargets(folderNames()) + val folderNames = lists.listFolders() + return lists.listAll() + .map { it.toTarget(folderNames[it.folderId]) } .filter { it.matches(q) } .sortedWith(compareBy({ it.folderLabel.orEmpty() }, { it.label })) } @@ -99,36 +100,15 @@ class BrevoTargetStrategy( private fun pathTo(folder: String?): List = listOfNotNull(descriptor.systemLabel, folder?.takeIf { it.isNotBlank() }) - private fun folderNames(): Map = - page("folders") { limit, offset -> - contactsApi.getFolders(limit, offset, GetContactsSortParameter.ASC).let { page -> - Page(page.count, page.folders.orEmpty().associate { it.id.toString() to it.name }.entries.toList()) - } - }.associate { it.key to it.value } - - private fun listTargets(folderNames: Map): List = - page("lists") { limit, offset -> - contactsApi.getLists(limit, offset, GetContactsSortParameter.ASC).let { page -> - Page(page.count, page.lists.orEmpty().map { it.toTarget(folderNames, ::pathTo) }) - } - } - - private fun page(kind: String, fetch: (Long, Long) -> Page): List { - val results = mutableListOf() - var offset = 0L - while (true) { - val page = try { - fetch(PAGE_SIZE, offset) - } catch (e: RestClientResponseException) { - if (e.statusCode.value() == 429) log.warn("Brevo target catalog {} fetch was rate limited", kind) - throw ContactServiceException("Failed to fetch Brevo $kind catalog", e) - } - results += page.items - if (page.items.size < PAGE_SIZE || page.count != null && results.size >= page.count) break - offset += PAGE_SIZE - } - return results - } + private fun ContactListRef.toTarget(folder: String?): ExternalTarget = ExternalTarget( + system = system, + externalId = externalListId.toString(), + kind = descriptor.kind, + label = name, + folderLabel = folder, + memberCount = memberCount, + path = pathTo(folder), + ) private fun ExternalTarget.matches(query: String): Boolean = query.isBlank() || @@ -136,29 +116,10 @@ class BrevoTargetStrategy( label.lowercase().contains(query) || folderLabel.orEmpty().lowercase().contains(query) + /** + * Brevo keys lists and contacts by number, so an id that is not one names nothing there. + * The sole conversion between the port's [String] ids and Brevo's own. + */ private fun String.toBrevoId(field: String, operation: String): Long = toLongOrNull() ?: throw InvalidExternalIdException("Brevo $operation: $field \"$this\" is not numeric") - - private data class Page(val count: Long?, val items: List) - - companion object { - const val PAGE_SIZE: Long = 50 - private val log = LoggerFactory.getLogger(BrevoTargetStrategy::class.java) - } -} - -private fun GetLists200ResponseListsInner.toTarget( - folderNames: Map, - pathTo: (String?) -> List, -): ExternalTarget { - val folder = folderNames[folderId.toString()] - return ExternalTarget( - system = TargetSystem.BREVO, - externalId = id.toString(), - kind = CohortKind.LIST, - label = name, - folderLabel = folder, - memberCount = uniqueSubscribers, - path = pathTo(folder), - ) } diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncService.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncService.kt index a8f1242cf..4e549d2bc 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncService.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncService.kt @@ -18,7 +18,7 @@ import java.time.LocalDateTime /** * Drives one `(user, cohort)` sync end to end: resolves both external ids, picks the - * [CohortPort] for the cohort's system and asks it to apply the change. + * [TargetStrategy] for the cohort's system and asks it to apply the change. * * An ADD with no user external id enqueues `SyncContact` and throws retryably, so the retry * lands once the contact exists. An ADD with no cohort target id fails terminally — linking a @@ -28,7 +28,7 @@ import java.time.LocalDateTime class CohortMembershipSyncService( private val cohorts: CohortRepository, private val ledger: CohortLedger, - private val registry: CohortPortRegistry, + private val strategies: TargetStrategies, private val externalIds: ExternalIdMappingService, private val targetIds: CohortTargetIds, private val jobs: TrackedJobDispatcher, @@ -52,15 +52,15 @@ class CohortMembershipSyncService( val system = runCatching { TargetSystem.valueOf(cohort.system) }.getOrElse { throw NonRetryableJobException("Cohort $cohortId has unknown system '${cohort.system}'") } - val port = registry.require(system) + val strategy = strategies.requireForJob(system) when (intent) { - SyncCohortMembershipIntent.ADD -> add(userId, cohort, port) - SyncCohortMembershipIntent.REMOVE -> remove(userId, cohort, port) + SyncCohortMembershipIntent.ADD -> add(userId, cohort, strategy) + SyncCohortMembershipIntent.REMOVE -> remove(userId, cohort, strategy) } } - private fun add(userId: Long, cohort: Cohort, port: CohortPort) { + private fun add(userId: Long, cohort: Cohort, strategy: TargetStrategy) { val cohortId = cohort.id!! val system = cohort.system val externalUserId = externalIds.find(USER_AGGREGATE, userId, system)?.externalId @@ -74,7 +74,7 @@ class CohortMembershipSyncService( if (externalCohortId == null) { throw CohortTargetNotLinkedException(cohortId, system) } - outsideTransaction.executeWithoutResult { port.addMember(externalUserId, externalCohortId) } + outsideTransaction.executeWithoutResult { strategy.add(strategy.handle(externalCohortId), externalUserId) } // Stamp the ledger so the desired row reads as synced. This is the // primary path to healthy; reconcile only verifies afterwards. @@ -84,7 +84,7 @@ class CohortMembershipSyncService( log.debug("Added user {} to {} cohort {} (ext={})", userId, system, cohortId, externalCohortId) } - private fun remove(userId: Long, cohort: Cohort, port: CohortPort) { + private fun remove(userId: Long, cohort: Cohort, strategy: TargetStrategy) { val cohortId = cohort.id!! val system = cohort.system val externalUserId = externalIds.find(USER_AGGREGATE, userId, system)?.externalId @@ -96,7 +96,7 @@ class CohortMembershipSyncService( ) return } - outsideTransaction.executeWithoutResult { port.removeMember(externalUserId, externalCohortId) } + outsideTransaction.executeWithoutResult { strategy.remove(strategy.handle(externalCohortId), externalUserId) } log.debug("Removed user {} from {} cohort {} (ext={})", userId, system, cohortId, externalCohortId) } diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortPort.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortPort.kt deleted file mode 100644 index ba1f9b80d..000000000 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortPort.kt +++ /dev/null @@ -1,41 +0,0 @@ -package net.blueshell.api.cohort.domain - -import net.blueshell.api.shared.enums.TargetSystem - -/** - * Outbound port for cohort membership sync, one implementation per [TargetSystem], chosen out - * of `List` by the cohort's system. - * - * Operation-based rather than state-replacing: a caller adds or removes one `(user, cohort)` - * pair by external id, and the cohort's target must already be linked — creating one is an - * operator's act. Ids are `String` so Discord snowflakes and Google group emails sit alongside - * Brevo's numeric list ids without every adapter coercing to `Long`. - */ -interface CohortPort { - val system: TargetSystem - - /** - * Creates the cohort on the external system and returns its native - * id (the snowflake / list id / group address). The `hint` may be - * used by the adapter as a folder/parent classification when the - * external system supports it (e.g. Brevo folder id); adapters that - * do not need a hint may ignore it. - */ - fun createCohort(label: String, hint: String? = null): String - - fun addMember(externalUserId: String, externalCohortId: String) - - fun removeMember(externalUserId: String, externalCohortId: String) - - fun deleteCohort(externalCohortId: String) - - /** - * Lists all members currently present in the external target. - * Returns a bounded list; the largest live cohort is ~350 rows. - * If a cohort ever grows past ~2 000, switch to cursor-based pages. - */ - fun listMembers(externalCohortId: String): List -} - -/** One member as the external system knows them: a native id and an optional human-readable label. */ -data class MemberRef(val externalUserId: String, val label: String?) diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortPortRegistry.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortPortRegistry.kt deleted file mode 100644 index bb6950833..000000000 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortPortRegistry.kt +++ /dev/null @@ -1,22 +0,0 @@ -package net.blueshell.api.cohort.domain - -import net.blueshell.api.shared.enums.TargetSystem -import net.blueshell.api.shared.job.NonRetryableJobException -import org.springframework.stereotype.Component - -/** - * Central registry of all registered [CohortPort] implementations. - * Replaces ad-hoc `ports.single { it.system == system }` lookups. - * A missing port becomes a [NonRetryableJobException] (terminal FAILED) - * rather than an infinite retry. - */ -@Component -class CohortPortRegistry(ports: List) { - private val bySystem: Map = ports.associateBy { it.system } - - fun require(system: TargetSystem): CohortPort = - bySystem[system] - ?: throw NonRetryableJobException("No CohortPort registered for $system") - - fun systems(): Set = bySystem.keys -} diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortRemediationService.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortRemediationService.kt index f3782bee7..2163e712f 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortRemediationService.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortRemediationService.kt @@ -38,7 +38,7 @@ class CohortRemediationService( private val ledger: CohortLedger, private val externalIds: ExternalIdMappingService, private val targetIds: CohortTargetIds, - private val registry: CohortPortRegistry, + private val strategies: TargetStrategies, private val jobs: TrackedJobDispatcher, transactionManager: PlatformTransactionManager, ) : CohortRemediation { @@ -72,7 +72,8 @@ class CohortRemediationService( val system = TargetSystem.valueOf(cohort.system) val externalCohortId = targetIds.require(cohort) - outsideTransaction.executeWithoutResult { registry.require(system).removeMember(externalUserId, externalCohortId) } + val strategy = strategies.requireForJob(system) + outsideTransaction.executeWithoutResult { strategy.remove(strategy.handle(externalCohortId), externalUserId) } ledger.removeStranger(cohortId, externalUserId) } @@ -83,7 +84,8 @@ class CohortRemediationService( */ override fun verifyCohort(cohortId: Long) { val plan = readOnlyTransaction.execute { loadPlan(cohortId) } - val remote = outsideTransaction.execute { registry.require(plan.system).listMembers(plan.externalCohortId) } + val strategy = strategies.requireForJob(plan.system) + val remote = outsideTransaction.execute { strategy.members(strategy.handle(plan.externalCohortId)) } writeTransaction.executeWithoutResult { applySnapshot(plan, remote) } } @@ -120,7 +122,7 @@ class CohortRemediationService( return ReconcilePlan(cohortId, subjectId, system, externalCohortId) } - private fun applySnapshot(plan: ReconcilePlan, remote: List) { + private fun applySnapshot(plan: ReconcilePlan, remote: List) { val cohort = cohortRepo.findById(plan.cohortId).orElseThrow { NonRetryableJobException("Cohort ${plan.cohortId} not found") } @@ -169,7 +171,7 @@ class CohortRemediationService( plan: ReconcilePlan, desiredRows: List, externalIdByUserId: Map, - remoteByExtId: Map, + remoteByExtId: Map, now: LocalDateTime, ): Set { val confirmations = desiredRows.mapNotNull { row -> @@ -220,7 +222,7 @@ class CohortRemediationService( private fun reconcileStrangers( cohort: net.blueshell.api.cohort.persistence.Cohort, subject: net.blueshell.api.cohort.persistence.CohortSubject, - remoteByExtId: Map, + remoteByExtId: Map, confirmedExtIds: Set, now: LocalDateTime, ) { diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortTargeting.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortTargeting.kt index 13018b6df..7b3e1ad13 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortTargeting.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/CohortTargeting.kt @@ -6,7 +6,7 @@ import net.blueshell.api.shared.enums.TargetSystem * Inbound port for admin management of a subject's external targets: linking an existing one, * creating one, and repointing a mapping at another. * - * External writes go through [CohortPort]. A removal after a switch is handed to the + * External writes go through [TargetStrategy]. A removal after a switch is handed to the * `cohort.delete-external-target` job rather than run inline. */ interface CohortTargeting { diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategies.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategies.kt index 4468ec94f..36aab342c 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategies.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategies.kt @@ -1,6 +1,7 @@ package net.blueshell.api.cohort.domain import net.blueshell.api.shared.enums.TargetSystem +import net.blueshell.api.shared.job.NonRetryableJobException import org.springframework.http.HttpStatus import org.springframework.stereotype.Component import org.springframework.web.server.ResponseStatusException @@ -13,6 +14,14 @@ class TargetStrategies(strategies: List) { bySystem[system] ?: throw ResponseStatusException(HttpStatus.BAD_REQUEST, "$system is not a cohort target") + /** + * The strategy a job needs. A missing one is terminal rather than retried: no amount of + * backoff registers a bean, where [require] answers a request with a bad request. + */ + fun requireForJob(system: TargetSystem): TargetStrategy = + bySystem[system] + ?: throw NonRetryableJobException("No TargetStrategy registered for $system") + fun descriptor(system: TargetSystem): TargetDescriptor = require(system).descriptor fun descriptors(): List = diff --git a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategy.kt b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategy.kt index 830483d49..1f4de9d64 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategy.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/cohort/domain/TargetStrategy.kt @@ -38,10 +38,23 @@ data class ExternalTarget( data class ExternalMember(val externalUserId: String, val label: String?) +/** + * The one port over a cohort's external target: its catalogue, its folders, and who is on it. + * + * Ids are [String] so a Discord snowflake or a Google group address sits beside Brevo's numeric + * list id; an adapter that needs another shape converts at its own edge and nowhere else. + */ interface TargetStrategy { val descriptor: TargetDescriptor val system: TargetSystem get() = descriptor.system + /** + * A target known only by its id. The member, move and delete calls key on the id alone, so a + * caller holding one writes without reading the catalogue first. + */ + fun handle(externalId: String): ExternalTarget = + ExternalTarget(system, externalId, descriptor.kind, externalId) + fun catalog(query: String?): List = emptyList() fun resolve(externalId: String): ExternalTarget? = diff --git a/services/api/src/main/kotlin/net/blueshell/api/contact/api/ContactListAdapter.kt b/services/api/src/main/kotlin/net/blueshell/api/contact/api/ContactListAdapter.kt index af919427a..3752cf9e7 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/contact/api/ContactListAdapter.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/contact/api/ContactListAdapter.kt @@ -29,9 +29,26 @@ interface ContactListAdapter { /** Every folder, as id to name. */ fun listFolders(): Map = emptyMap() + /** + * Every list on the system, whoever it belongs to: the catalogue an operator picks from. + * A system that cannot be browsed answers with nothing rather than failing. + */ + fun listAll(): List = emptyList() + /** Lists all members currently present in the given external list. */ fun listMembers(externalListId: Long): List } /** One member as the external system knows them: a native numeric id and optional email label. */ data class ContactListMember(val externalUserId: Long, val email: String?) + +/** + * One list as the external system knows it. [folderId] keys into [ContactListAdapter.listFolders]; + * it is unresolved here so a caller reads the folders once rather than per list. + */ +data class ContactListRef( + val externalListId: Long, + val name: String, + val folderId: Long?, + val memberCount: Long?, +) diff --git a/services/api/src/main/kotlin/net/blueshell/api/contact/domain/BrevoListAdapter.kt b/services/api/src/main/kotlin/net/blueshell/api/contact/domain/BrevoListAdapter.kt index 6e5362e9f..08ef0e262 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/contact/domain/BrevoListAdapter.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/contact/domain/BrevoListAdapter.kt @@ -1,6 +1,7 @@ package net.blueshell.api.contact.domain import net.blueshell.api.contact.api.ContactListMember +import net.blueshell.api.contact.api.ContactListRef import net.blueshell.api.contact.api.ContactServiceException import net.blueshell.api.contact.api.ContactListAdapter import net.blueshell.api.shared.enums.ContactSystem @@ -51,15 +52,52 @@ class BrevoListAdapter( } override fun listFolders(): Map = - try { - contactsApi.getFolders(FOLDER_PAGE_LIMIT, 0, GetContactsSortParameter.ASC) - .folders.orEmpty() - .associate { it.id to it.name } - } catch (e: RestClientResponseException) { - log.error("Failed to read Brevo folders", e) - throw ContactServiceException("Failed to read folders", e) + page("folders") { limit, offset -> + contactsApi.getFolders(limit, offset, GetContactsSortParameter.ASC) + .let { Page(it.count, it.folders.orEmpty().map { folder -> folder.id to folder.name }) } + }.toMap() + + override fun listAll(): List = + page("lists") { limit, offset -> + contactsApi.getLists(limit, offset, GetContactsSortParameter.ASC).let { page -> + Page( + page.count, + page.lists.orEmpty().map { + ContactListRef( + externalListId = it.id, + name = it.name, + folderId = it.folderId, + memberCount = it.uniqueSubscribers, + ) + }, + ) + } } + /** + * Walks a Brevo collection until a short page or the reported count ends it. A rate-limited + * page is logged as such and raised like any other fetch failure, so the job retries. + */ + private fun page(kind: String, fetch: (Long, Long) -> Page): List { + val results = mutableListOf() + var offset = 0L + while (true) { + val page = try { + fetch(CATALOG_PAGE_SIZE, offset) + } catch (e: RestClientResponseException) { + if (e.statusCode.value() == 429) log.warn("Brevo {} fetch was rate limited", kind) + log.error("Failed to read Brevo {}", kind, e) + throw ContactServiceException("Failed to read $kind", e) + } + results += page.items + if (page.items.size < CATALOG_PAGE_SIZE || page.count != null && results.size >= page.count) break + offset += CATALOG_PAGE_SIZE + } + return results + } + + private data class Page(val count: Long?, val items: List) + override fun createList(name: String, folderName: String?): Long { val safeName = sanitizeForLog(name) log.info("Creating Brevo list '{}'", safeName) @@ -207,8 +245,8 @@ class BrevoListAdapter( } companion object { - /** Folders are few; one page covers any real account. */ - private const val FOLDER_PAGE_LIMIT = 50L + /** Brevo's own page cap for folders and lists. */ + private const val CATALOG_PAGE_SIZE = 50L private val log = LoggerFactory.getLogger(BrevoListAdapter::class.java) } } diff --git a/services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockCohortPort.kt b/services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockCohortPort.kt deleted file mode 100644 index 049ec3adb..000000000 --- a/services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockCohortPort.kt +++ /dev/null @@ -1,101 +0,0 @@ -package net.blueshell.api.platform.integration.mock - -import net.blueshell.api.cohort.domain.CohortPort -import net.blueshell.api.cohort.domain.MemberRef -import net.blueshell.api.shared.enums.TargetSystem -import org.slf4j.LoggerFactory -import org.springframework.context.annotation.Primary -import org.springframework.context.annotation.Profile -import org.springframework.stereotype.Service -import org.springframework.transaction.support.TransactionSynchronizationManager -import java.util.concurrent.ConcurrentHashMap -import java.util.concurrent.CopyOnWriteArrayList -import java.util.concurrent.atomic.AtomicLong - -/** - * In-memory [CohortPort] for test and dev profiles. Holds member and - * cohort state so the registry, drift service, and controller ITs - * have a real port to exercise without touching Brevo. - * - * Reported system is [TargetSystem.BREVO] so tests exercise the Brevo - * code path without needing the production adapter. - */ -@Service -@Primary -@Profile("test | dev") -class MockCohortPort : CohortPort { - - override val system: TargetSystem = TargetSystem.BREVO - - // cohortId → label - private val cohorts = ConcurrentHashMap() - - // (externalUserId, externalCohortId) → email hint ("" when unknown; - // ConcurrentHashMap forbids null values). - private val memberships = ConcurrentHashMap, String>() - - private val idSequence = AtomicLong(9000) - - /** - * Records `isActualTransactionActive()` at the entry of every provider - * call, so a boundary test can assert these run outside any DB - * transaction (the no-provider-call-inside-a-transaction rule). - */ - val transactionActiveDuringCalls: MutableList = CopyOnWriteArrayList() - - private fun recordTransactionState() { - transactionActiveDuringCalls += TransactionSynchronizationManager.isActualTransactionActive() - } - - override fun createCohort(label: String, hint: String?): String { - recordTransactionState() - val id = idSequence.getAndIncrement().toString() - cohorts[id] = label - log.info("MockCohort: created cohort id={} label='{}'", id, label) - return id - } - - override fun addMember(externalUserId: String, externalCohortId: String) { - recordTransactionState() - memberships[externalUserId to externalCohortId] = "" - log.info("MockCohort: added member {} to cohort {}", externalUserId, externalCohortId) - } - - override fun removeMember(externalUserId: String, externalCohortId: String) { - recordTransactionState() - memberships.remove(externalUserId to externalCohortId) - log.info("MockCohort: removed member {} from cohort {}", externalUserId, externalCohortId) - } - - override fun deleteCohort(externalCohortId: String) { - recordTransactionState() - cohorts.remove(externalCohortId) - memberships.keys.removeIf { (_, cohortId) -> cohortId == externalCohortId } - log.info("MockCohort: deleted cohort {}", externalCohortId) - } - - override fun listMembers(externalCohortId: String): List { - recordTransactionState() - return memberships.keys - .filter { (_, cohortId) -> cohortId == externalCohortId } - .map { (userId, _) -> MemberRef(externalUserId = userId, label = memberships[userId to externalCohortId]?.ifEmpty { null }) } - } - - /** Directly seeds a member with an optional email label for drift tests. */ - fun seedMember(externalUserId: String, externalCohortId: String, label: String? = null) { - memberships[externalUserId to externalCohortId] = label.orEmpty() - } - - fun getMembers(externalCohortId: String): Set = - memberships.keys.filter { (_, cId) -> cId == externalCohortId }.map { (uId, _) -> uId }.toSet() - - fun clear() { - cohorts.clear() - memberships.clear() - transactionActiveDuringCalls.clear() - } - - companion object { - private val log = LoggerFactory.getLogger(MockCohortPort::class.java) - } -} diff --git a/services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockTargetStrategy.kt b/services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockTargetStrategy.kt index b5a3f79cb..763d3f82f 100644 --- a/services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockTargetStrategy.kt +++ b/services/api/src/main/kotlin/net/blueshell/api/platform/integration/mock/MockTargetStrategy.kt @@ -10,9 +10,17 @@ import net.blueshell.api.shared.enums.TargetSystem import org.springframework.context.annotation.Primary import org.springframework.context.annotation.Profile import org.springframework.stereotype.Service +import org.springframework.transaction.support.TransactionSynchronizationManager import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.CopyOnWriteArrayList import java.util.concurrent.atomic.AtomicLong +/** + * In-memory [TargetStrategy] for the test and dev profiles: the registry, the remediation + * services and the controller ITs get a real target to exercise without touching Brevo. + * + * Reported system is [TargetSystem.BREVO] so those paths run as they do in production. + */ @Service @Primary @Profile("test | dev") @@ -37,12 +45,21 @@ class MockTargetStrategy : TargetStrategy { private val members = ConcurrentHashMap, String>() private val ids = AtomicLong(9000) - override fun catalog(query: String?): List = - targets.values.filter { it.matches(query.orEmpty().trim().lowercase()) } + /** + * `isActualTransactionActive()` at the entry of every provider call, so a boundary test can + * assert these run outside any DB transaction (the no-provider-call-inside-a-transaction rule). + */ + val transactionActiveDuringCalls: MutableList = CopyOnWriteArrayList() + + override fun catalog(query: String?): List { + recordTransactionState() + return targets.values.filter { it.matches(query.orEmpty().trim().lowercase()) } + } override fun resolve(externalId: String): ExternalTarget? = targets[externalId] override fun create(label: String, folder: String?): ExternalTarget { + recordTransactionState() val target = ExternalTarget( system, ids.getAndIncrement().toString(), @@ -55,24 +72,44 @@ class MockTargetStrategy : TargetStrategy { return target } - override fun members(target: ExternalTarget): List = - members.entries + override fun members(target: ExternalTarget): List { + recordTransactionState() + return members.entries .filter { it.key.second == target.externalId } .map { ExternalMember(it.key.first, it.value.ifBlank { null }) } + } override fun add(target: ExternalTarget, externalUserId: String) { + recordTransactionState() members[externalUserId to target.externalId] = "" } override fun remove(target: ExternalTarget, externalUserId: String) { + recordTransactionState() members.remove(externalUserId to target.externalId) } override fun delete(target: ExternalTarget) { + recordTransactionState() targets.remove(target.externalId) members.keys.removeIf { it.second == target.externalId } } + /** Directly seeds a member with an optional label, for the drift tests. */ + fun seedMember(externalUserId: String, externalTargetId: String, label: String? = null) { + members[externalUserId to externalTargetId] = label.orEmpty() + } + + fun clear() { + targets.clear() + members.clear() + transactionActiveDuringCalls.clear() + } + + private fun recordTransactionState() { + transactionActiveDuringCalls += TransactionSynchronizationManager.isActualTransactionActive() + } + private fun ExternalTarget.matches(query: String): Boolean = query.isBlank() || externalId == query || diff --git a/services/api/src/test/kotlin/net/blueshell/api/architecture/VendorClientArchitectureTest.kt b/services/api/src/test/kotlin/net/blueshell/api/architecture/VendorClientArchitectureTest.kt index 3aff97efd..6ca714280 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/architecture/VendorClientArchitectureTest.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/architecture/VendorClientArchitectureTest.kt @@ -16,9 +16,9 @@ import org.junit.jupiter.api.Test * [VENDOR_OWNERS] states. Modulith polices `net.blueshell.api` and stops at the module boundary, * so nothing but this rule looks at a third-party import. * - * The reaches that exist are pinned in [PINNED] rather than fixed here, the way - * [CrossModuleWebAccessArchitectureTest] pins its own. Each is a line in the file, so dropping one - * is a visible diff. Pinned at six reaches, all made by one class. + * A reach that is being cleaned up separately is pinned in [PINNED] rather than fixed here, the + * way [CrossModuleWebAccessArchitectureTest] pins its own. Each is a line in the file, so dropping + * one is a visible diff. Nothing is pinned: every module speaks to its vendors through a wrapper. */ class VendorClientArchitectureTest : ArchJUnitTestBase(ArchitecturePackages.ROOT) { @@ -35,21 +35,10 @@ class VendorClientArchitectureTest : ArchJUnitTestBase(ArchitecturePackages.ROOT ) /** - * Vendor reaches from outside the owning module that existed when this rule landed, as - * ` -> `. + * Vendor reaches from outside the owning module that are being cleaned up separately, as + * ` -> `. Empty, and a new entry needs a reason next to it. */ - val PINNED = setOf( - // DEBT. BrevoTargetStrategy drives ContactsApi for the list catalog and folder names - // while pushing membership through contact's ContactListAdapter — one integration - // behind two ports, one of them raw. Removing these means publishing the catalog side - // through contact :: api so cohort speaks only to the wrapper. Tracked separately. - "cohort -> net.blueshell.clients.brevo.api.ContactsApi", - "cohort -> net.blueshell.clients.brevo.model.GetContactsSortParameter", - "cohort -> net.blueshell.clients.brevo.model.GetFolder", - "cohort -> net.blueshell.clients.brevo.model.GetFolders200Response", - "cohort -> net.blueshell.clients.brevo.model.GetLists200Response", - "cohort -> net.blueshell.clients.brevo.model.GetLists200ResponseListsInner", - ) + val PINNED = emptySet() } @Test diff --git a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoCohortAdapterTest.kt b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoCohortAdapterTest.kt deleted file mode 100644 index 3f8c2c3c5..000000000 --- a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoCohortAdapterTest.kt +++ /dev/null @@ -1,130 +0,0 @@ -package net.blueshell.api.cohort.domain - -import io.mockk.every -import io.mockk.mockk -import io.mockk.verify -import net.blueshell.api.contact.api.ContactListAdapter -import net.blueshell.api.shared.enums.TargetSystem -import net.blueshell.api.shared.enums.ContactSystem -import org.assertj.core.api.Assertions.assertThat -import org.assertj.core.api.Assertions.assertThatThrownBy -import org.junit.jupiter.api.Test - -class BrevoCohortAdapterTest { - - private val brevoLegacy: ContactListAdapter = mockk { - every { system } returns ContactSystem.BREVO - } - private val adapter = BrevoCohortAdapter(listOf(brevoLegacy)) - - @Test - fun `system is TargetSystem BREVO`() { - assertThat(adapter.system).isEqualTo(TargetSystem.BREVO) - } - - @Test - fun `createCohort delegates to the legacy adapter and stringifies the returned id`() { - every { brevoLegacy.createList("Members", "contributionPeriods") } returns 42L - - val externalId = adapter.createCohort("Members", "contributionPeriods") - - assertThat(externalId).isEqualTo("42") - } - - @Test - fun `createCohort passes a null hint through to the delegate`() { - every { brevoLegacy.createList("Members", null) } returns 7L - - val externalId = adapter.createCohort("Members") - - assertThat(externalId).isEqualTo("7") - verify { brevoLegacy.createList("Members", null) } - } - - @Test - fun `addMember parses string ids and delegates to addToList`() { - every { brevoLegacy.addToList(123L, 456L) } returns Unit - - adapter.addMember("123", "456") - - verify { brevoLegacy.addToList(123L, 456L) } - } - - @Test - fun `removeMember parses string ids and delegates to removeFromList`() { - every { brevoLegacy.removeFromList(123L, 456L) } returns Unit - - adapter.removeMember("123", "456") - - verify { brevoLegacy.removeFromList(123L, 456L) } - } - - @Test - fun `deleteCohort delegates to deleteList`() { - every { brevoLegacy.deleteList(456L) } returns Unit - - adapter.deleteCohort("456") - - verify { brevoLegacy.deleteList(456L) } - } - - @Test - fun `addMember throws InvalidExternalIdException when externalUserId is malformed`() { - assertThatThrownBy { adapter.addMember("not-a-number", "456") } - .isInstanceOf(InvalidExternalIdException::class.java) - .isInstanceOf(IllegalArgumentException::class.java) - .hasMessageContaining("not-a-number") - .hasMessageContaining("externalUserId") - .hasMessageContaining("addMember") - } - - @Test - fun `addMember throws InvalidExternalIdException when externalCohortId is malformed`() { - assertThatThrownBy { adapter.addMember("123", "bad-cohort") } - .isInstanceOf(InvalidExternalIdException::class.java) - .isInstanceOf(IllegalArgumentException::class.java) - .hasMessageContaining("bad-cohort") - .hasMessageContaining("externalCohortId") - .hasMessageContaining("addMember") - } - - @Test - fun `removeMember throws InvalidExternalIdException when externalUserId is malformed`() { - assertThatThrownBy { adapter.removeMember("oops", "456") } - .isInstanceOf(InvalidExternalIdException::class.java) - .isInstanceOf(IllegalArgumentException::class.java) - .hasMessageContaining("oops") - .hasMessageContaining("externalUserId") - .hasMessageContaining("removeMember") - } - - @Test - fun `removeMember throws InvalidExternalIdException when externalCohortId is malformed`() { - assertThatThrownBy { adapter.removeMember("123", "oops") } - .isInstanceOf(InvalidExternalIdException::class.java) - .isInstanceOf(IllegalArgumentException::class.java) - .hasMessageContaining("oops") - .hasMessageContaining("externalCohortId") - .hasMessageContaining("removeMember") - } - - @Test - fun `deleteCohort throws InvalidExternalIdException when externalCohortId is malformed`() { - assertThatThrownBy { adapter.deleteCohort("not-a-long") } - .isInstanceOf(InvalidExternalIdException::class.java) - .isInstanceOf(IllegalArgumentException::class.java) - .hasMessageContaining("not-a-long") - .hasMessageContaining("externalCohortId") - .hasMessageContaining("deleteCohort") - } - - @Test - fun `listMembers throws InvalidExternalIdException when externalCohortId is malformed`() { - assertThatThrownBy { adapter.listMembers("xyz") } - .isInstanceOf(InvalidExternalIdException::class.java) - .isInstanceOf(IllegalArgumentException::class.java) - .hasMessageContaining("xyz") - .hasMessageContaining("externalCohortId") - .hasMessageContaining("listMembers") - } -} diff --git a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategyTest.kt b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategyTest.kt index 18ede6dac..b637223e4 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategyTest.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/BrevoTargetStrategyTest.kt @@ -1,57 +1,34 @@ package net.blueshell.api.cohort.domain import net.blueshell.api.contact.api.ContactListAdapter -import net.blueshell.api.contact.api.ContactServiceException +import net.blueshell.api.contact.api.ContactListRef import net.blueshell.api.shared.enums.ContactSystem -import net.blueshell.clients.brevo.api.ContactsApi -import net.blueshell.clients.brevo.model.GetFolder -import net.blueshell.clients.brevo.model.GetFolders200Response -import net.blueshell.clients.brevo.model.GetLists200Response -import net.blueshell.clients.brevo.model.GetLists200ResponseListsInner -import net.blueshell.clients.brevo.model.GetContactsSortParameter import org.assertj.core.api.Assertions.assertThat -import org.assertj.core.api.Assertions.assertThatThrownBy import org.junit.jupiter.api.Test -import org.mockito.kotlin.doThrow -import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.whenever -import org.springframework.web.client.RestClientResponseException class BrevoTargetStrategyTest { - private val contactsApi: ContactsApi = mock() private val lists: ContactListAdapter = mock { whenever(it.system).thenReturn(ContactSystem.BREVO) } - private val strategy = BrevoTargetStrategy(listOf(lists), contactsApi) + private val strategy = BrevoTargetStrategy(listOf(lists)) @Test - fun `pages folders and lists and maps folder names and counts`() { - whenever(contactsApi.getFolders(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) - .thenReturn(GetFolders200Response(count = 51L, folders = (1L..50L).map { folder(it, "Folder $it") })) - whenever(contactsApi.getFolders(eq(50L), eq(50L), eq(GetContactsSortParameter.ASC))) - .thenReturn(GetFolders200Response(count = 51L, folders = listOf(folder(51L, "Contribution periods")))) - whenever(contactsApi.getLists(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) - .thenReturn(GetLists200Response(count = 51L, lists = (1L..50L).map { list(it, "List $it", 1L) })) - whenever(contactsApi.getLists(eq(50L), eq(50L), eq(GetContactsSortParameter.ASC))) - .thenReturn(GetLists200Response(count = 51L, lists = listOf(list(99L, "Paid 2026", 51L, 728L)))) + fun `maps folder names and counts onto the catalog`() { + whenever(lists.listFolders()).thenReturn(mapOf(51L to "Contribution periods")) + whenever(lists.listAll()).thenReturn(listOf(list(99L, "Paid 2026", 51L, 728L))) - val targets = strategy.catalog(null) - val paid = targets.single { it.externalId == "99" } + val paid = strategy.catalog(null).single { it.externalId == "99" } - assertThat(targets).hasSize(51) assertThat(paid.folderLabel).isEqualTo("Contribution periods") assertThat(paid.memberCount).isEqualTo(728L) } @Test fun `says where each list sits, outside in`() { - whenever(contactsApi.getFolders(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) - .thenReturn(GetFolders200Response(count = 1L, folders = listOf(folder(1L, "Committees")))) - whenever(contactsApi.getLists(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) - .thenReturn( - GetLists200Response(count = 2L, lists = listOf(list(10L, "Web Cmte", 1L), list(11L, "Loose ends", 404L))), - ) + whenever(lists.listFolders()).thenReturn(mapOf(1L to "Committees")) + whenever(lists.listAll()).thenReturn(listOf(list(10L, "Web Cmte", 1L), list(11L, "Loose ends", 404L))) val targets = strategy.catalog(null).associateBy { it.externalId } @@ -64,52 +41,14 @@ class BrevoTargetStrategyTest { @Test fun `filters by query after fetching the bounded catalog`() { - whenever(contactsApi.getFolders(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) - .thenReturn(GetFolders200Response(count = 1L, folders = listOf(folder(1L, "Members")))) - whenever(contactsApi.getLists(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) - .thenReturn(GetLists200Response(count = 2L, lists = listOf(list(10L, "Guests", 1L), list(11L, "Paid", 1L)))) + whenever(lists.listFolders()).thenReturn(mapOf(1L to "Members")) + whenever(lists.listAll()).thenReturn(listOf(list(10L, "Guests", 1L), list(11L, "Paid", 1L))) val targets = strategy.catalog("paid") assertThat(targets).extracting { it.externalId }.containsExactly("11") } - @Test - fun `treats Brevo rate limiting as retryable`() { - doThrow(error(429)).whenever(contactsApi) - .getFolders(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC)) - - assertThatThrownBy { strategy.catalog(null) } - .isInstanceOf(ContactServiceException::class.java) - .hasCauseInstanceOf(RestClientResponseException::class.java) - } - - // The generated models are immutable data classes, so the counts Brevo - // always returns have to be supplied even where the assertions ignore them. - private fun folder(id: Long, name: String): GetFolder = - GetFolder( - id = id, - name = name, - totalBlacklisted = 0L, - totalSubscribers = 0L, - uniqueSubscribers = 0L, - ) - - private fun list( - id: Long, - name: String, - folderId: Long, - unique: Long = 10L + id, - ): GetLists200ResponseListsInner = - GetLists200ResponseListsInner( - id = id, - name = name, - folderId = folderId, - uniqueSubscribers = unique, - totalBlacklisted = 0L, - totalSubscribers = unique, - ) - - private fun error(status: Int): RestClientResponseException = - RestClientResponseException("$status error", status, "error", null, ByteArray(0), null) + private fun list(id: Long, name: String, folderId: Long, unique: Long = 10L + id): ContactListRef = + ContactListRef(externalListId = id, name = name, folderId = folderId, memberCount = unique) } diff --git a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncServiceTest.kt b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncServiceTest.kt index 8a8d73598..ccded9fcf 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncServiceTest.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortMembershipSyncServiceTest.kt @@ -20,8 +20,10 @@ class CohortMembershipSyncServiceTest { private val cohorts: CohortRepository = mockk() private val ledger: CohortLedger = mockk(relaxed = true) - private val brevoPort: CohortPort = mockk(relaxed = true) { + private val target42 = ExternalTarget(TargetSystem.BREVO, "42", CohortKind.LIST, "42") + private val brevoTarget: TargetStrategy = mockk(relaxed = true) { every { system } returns TargetSystem.BREVO + every { handle("42") } returns target42 } private val externalIds: ExternalIdMappingService = mockk(relaxed = true) private val targetIds: CohortTargetIds = mockk(relaxed = true) @@ -29,7 +31,7 @@ class CohortMembershipSyncServiceTest { private val service = CohortMembershipSyncService( cohorts = cohorts, ledger = ledger, - registry = CohortPortRegistry(listOf(brevoPort)), + strategies = TargetStrategies(listOf(brevoTarget)), externalIds = externalIds, targetIds = targetIds, jobs = jobs, @@ -44,14 +46,14 @@ class CohortMembershipSyncServiceTest { } @Test - fun `ADD calls port when both external ids exist`() { + fun `ADD calls the strategy when both external ids exist`() { givenCohort(id = 10L, system = "BREVO", label = "Members") every { externalIds.find("USER", 1L, "BREVO") } returns mapping("USER", 1L, "BREVO", "777") every { targetIds.find(any()) } returns "42" service.sync(userId = 1L, cohortId = 10L, intent = SyncCohortMembershipIntent.ADD) - verify { brevoPort.addMember("777", "42") } + verify { brevoTarget.add(target42, "777") } } @Test @@ -68,8 +70,8 @@ class CohortMembershipSyncServiceTest { verify(exactly = 0) { jobs.runAsync(CohortJobs.MaterializeCohortTarget, any()) } - verify(exactly = 0) { brevoPort.addMember(any(), any()) } - verify(exactly = 0) { brevoPort.createCohort(any(), any()) } + verify(exactly = 0) { brevoTarget.add(any(), any()) } + verify(exactly = 0) { brevoTarget.create(any(), any()) } } @Test @@ -80,7 +82,7 @@ class CohortMembershipSyncServiceTest { service.sync(userId = 1L, cohortId = 10L, intent = SyncCohortMembershipIntent.ADD) - verify { brevoPort.addMember("777", "42") } + verify { brevoTarget.add(target42, "777") } verify { ledger.markPushed(10L, 1L, "777", any()) } } @@ -96,18 +98,18 @@ class CohortMembershipSyncServiceTest { verify { jobs.runAsync(ContactJobs.SyncContact, ContactJobs.SyncContactPayload(1L)) } - verify(exactly = 0) { brevoPort.addMember(any(), any()) } + verify(exactly = 0) { brevoTarget.add(any(), any()) } } @Test - fun `REMOVE calls port when both external ids exist`() { + fun `REMOVE calls the strategy when both external ids exist`() { givenCohort(id = 10L, system = "BREVO", label = "Members") every { externalIds.find("USER", 1L, "BREVO") } returns mapping("USER", 1L, "BREVO", "777") every { targetIds.find(any()) } returns "42" service.sync(userId = 1L, cohortId = 10L, intent = SyncCohortMembershipIntent.REMOVE) - verify { brevoPort.removeMember("777", "42") } + verify { brevoTarget.remove(target42, "777") } } @Test @@ -118,8 +120,8 @@ class CohortMembershipSyncServiceTest { service.sync(userId = 1L, cohortId = 10L, intent = SyncCohortMembershipIntent.REMOVE) - verify(exactly = 0) { brevoPort.removeMember(any(), any()) } - verify(exactly = 0) { brevoPort.addMember(any(), any()) } + verify(exactly = 0) { brevoTarget.remove(any(), any()) } + verify(exactly = 0) { brevoTarget.add(any(), any()) } } @Test @@ -141,15 +143,15 @@ class CohortMembershipSyncServiceTest { } @Test - fun `cohort whose system has no registered port throws NonRetryableJobException`() { - // Cohort's system is a valid TargetSystem value but no matching CohortPort bean exists - // (GOOGLE_CALENDAR has none yet). + fun `cohort whose system has no registered strategy throws NonRetryableJobException`() { + // Cohort's system is a valid TargetSystem value but no matching TargetStrategy bean + // exists (GOOGLE_CALENDAR has none yet). givenCohort(id = 10L, system = "GOOGLE_CALENDAR", label = "events") assertThatThrownBy { service.sync(userId = 1L, cohortId = 10L, intent = SyncCohortMembershipIntent.ADD) }.isInstanceOf(NonRetryableJobException::class.java) - .hasMessageContaining("No CohortPort") + .hasMessageContaining("No TargetStrategy") } private fun givenCohort(id: Long, system: String, label: String) { diff --git a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortRemediationServiceTest.kt b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortRemediationServiceTest.kt index 656184f3e..76077504a 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortRemediationServiceTest.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/CohortRemediationServiceTest.kt @@ -33,7 +33,7 @@ class CohortRemediationServiceTest { private val externalIds: ExternalIdMappingService = mockk() private val targetIds: CohortTargetIds = mockk() private val jobs: TrackedJobDispatcher = mockk(relaxed = true) - private val port = RecordingCohortPort() + private val port = RecordingTargetStrategy() private val service = CohortRemediationService( cohortRepo = cohorts, subjectRepo = subjects, @@ -41,7 +41,7 @@ class CohortRemediationServiceTest { ledger = CohortLedger(members), externalIds = externalIds, targetIds = targetIds, - registry = CohortPortRegistry(listOf(port)), + strategies = TargetStrategies(listOf(port)), jobs = jobs, transactionManager = ImmediateTransactionManager(), ) @@ -80,8 +80,8 @@ class CohortRemediationServiceTest { verifiedAt = LocalDateTime.parse("2026-01-03T12:00:00"), ) port.remote = listOf( - MemberRef("ext-1", "Alice Remote"), - MemberRef("ext-extra", "Extra Remote"), + ExternalMember("ext-1", "Alice Remote"), + ExternalMember("ext-extra", "Extra Remote"), ) every { cohorts.findById(99L) } returns Optional.of(cohort) @@ -270,24 +270,35 @@ class CohortRemediationServiceTest { label = label, ) - private class RecordingCohortPort : CohortPort { - override val system: TargetSystem = TargetSystem.BREVO - var remote: List = emptyList() + private class RecordingTargetStrategy : TargetStrategy { + override val descriptor = TargetDescriptor( + system = TargetSystem.BREVO, + kind = CohortKind.LIST, + systemLabel = "Brevo", + targetLabel = "Brevo list", + idLabel = "List id", + capabilities = setOf( + TargetCapability.READ_MEMBERS, + TargetCapability.WRITE_MEMBERS, + TargetCapability.DELETE, + ), + ) + var remote: List = emptyList() var listCalls = 0 var lastExternalCohortId: String? = null var sawTransactionDuringList = false val removeCalls = mutableListOf>() - override fun createCohort(label: String, hint: String?): String = error("not used") - override fun addMember(externalUserId: String, externalCohortId: String) = Unit - override fun removeMember(externalUserId: String, externalCohortId: String) { - removeCalls += externalUserId to externalCohortId + override fun create(label: String, folder: String?): ExternalTarget = error("not used") + override fun add(target: ExternalTarget, externalUserId: String) = Unit + override fun remove(target: ExternalTarget, externalUserId: String) { + removeCalls += externalUserId to target.externalId } - override fun deleteCohort(externalCohortId: String) = Unit + override fun delete(target: ExternalTarget) = Unit - override fun listMembers(externalCohortId: String): List { + override fun members(target: ExternalTarget): List { listCalls += 1 - lastExternalCohortId = externalCohortId + lastExternalCohortId = target.externalId sawTransactionDuringList = TransactionSynchronizationManager.isActualTransactionActive() return remote } diff --git a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/TargetFolderMoveTest.kt b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/TargetFolderMoveTest.kt index f71861621..2c2a2dda9 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/TargetFolderMoveTest.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/cohort/domain/TargetFolderMoveTest.kt @@ -7,7 +7,6 @@ import net.blueshell.api.contact.api.ContactListAdapter import net.blueshell.api.shared.enums.ContactSystem import net.blueshell.api.cohort.persistence.CohortKind import net.blueshell.api.shared.enums.TargetSystem -import net.blueshell.clients.brevo.api.ContactsApi import org.assertj.core.api.Assertions.assertThat import org.assertj.core.api.Assertions.assertThatThrownBy import org.junit.jupiter.api.Test @@ -22,8 +21,7 @@ class TargetFolderMoveTest { private val lists = mockk(relaxed = true) { every { system } returns ContactSystem.BREVO } - private val contactsApi = mockk(relaxed = true) - private val strategy = BrevoTargetStrategy(listOf(lists), contactsApi) + private val strategy = BrevoTargetStrategy(listOf(lists)) private val target = ExternalTarget( system = TargetSystem.BREVO, diff --git a/services/api/src/test/kotlin/net/blueshell/api/contact/domain/BrevoListAdapterTest.kt b/services/api/src/test/kotlin/net/blueshell/api/contact/domain/BrevoListAdapterTest.kt index 6cfff9bee..c5808070c 100644 --- a/services/api/src/test/kotlin/net/blueshell/api/contact/domain/BrevoListAdapterTest.kt +++ b/services/api/src/test/kotlin/net/blueshell/api/contact/domain/BrevoListAdapterTest.kt @@ -1,10 +1,16 @@ package net.blueshell.api.contact.domain +import net.blueshell.api.contact.api.ContactListRef import net.blueshell.api.contact.api.ContactServiceException import net.blueshell.clients.brevo.api.ContactsApi import net.blueshell.clients.brevo.model.AddContactToListRequest import net.blueshell.clients.brevo.model.GetContactInfo200Response import net.blueshell.clients.brevo.model.GetContactInfo200ResponseAllOfStatistics +import net.blueshell.clients.brevo.model.GetContactsSortParameter +import net.blueshell.clients.brevo.model.GetFolder +import net.blueshell.clients.brevo.model.GetFolders200Response +import net.blueshell.clients.brevo.model.GetLists200Response +import net.blueshell.clients.brevo.model.GetLists200ResponseListsInner import net.blueshell.clients.brevo.model.RemoveContactFromListRequest import org.assertj.core.api.Assertions.assertThat import org.assertj.core.api.Assertions.assertThatThrownBy @@ -82,6 +88,47 @@ class BrevoListAdapterTest { adapter.removeFromList(externalUserId = 100L, externalListId = 200L) } + @Test + fun `pages folders and lists past the first page`() { + whenever(contactsApi.getFolders(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) + .thenReturn(GetFolders200Response(count = 51L, folders = (1L..50L).map { folder(it, "Folder $it") })) + whenever(contactsApi.getFolders(eq(50L), eq(50L), eq(GetContactsSortParameter.ASC))) + .thenReturn(GetFolders200Response(count = 51L, folders = listOf(folder(51L, "Contribution periods")))) + whenever(contactsApi.getLists(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC))) + .thenReturn(GetLists200Response(count = 51L, lists = (1L..50L).map { list(it, "List $it", 1L) })) + whenever(contactsApi.getLists(eq(50L), eq(50L), eq(GetContactsSortParameter.ASC))) + .thenReturn(GetLists200Response(count = 51L, lists = listOf(list(99L, "Paid 2026", 51L, 728L)))) + + assertThat(adapter.listFolders()).hasSize(51).containsEntry(51L, "Contribution periods") + assertThat(adapter.listAll()).hasSize(51) + .contains(ContactListRef(externalListId = 99L, name = "Paid 2026", folderId = 51L, memberCount = 728L)) + } + + @Test + fun `surfaces a rate-limited catalog page as a retryable failure`() { + doThrow(error(429, "")).whenever(contactsApi) + .getFolders(eq(50L), eq(0L), eq(GetContactsSortParameter.ASC)) + + assertThatThrownBy { adapter.listFolders() } + .isInstanceOf(ContactServiceException::class.java) + .hasCauseInstanceOf(RestClientResponseException::class.java) + } + + // The generated models are immutable data classes, so the counts Brevo always returns have + // to be supplied even where the assertions ignore them. + private fun folder(id: Long, name: String): GetFolder = + GetFolder(id = id, name = name, totalBlacklisted = 0L, totalSubscribers = 0L, uniqueSubscribers = 0L) + + private fun list(id: Long, name: String, folderId: Long, unique: Long = 10L + id): GetLists200ResponseListsInner = + GetLists200ResponseListsInner( + id = id, + name = name, + folderId = folderId, + uniqueSubscribers = unique, + totalBlacklisted = 0L, + totalSubscribers = unique, + ) + private fun alreadyInListOrMissing(): RestClientResponseException = error( 400, """{"code":"invalid_parameter","message":"Contact already in list and/or does not exist"}""",