[QRD-7899] feat(configuration-webhook): add affiliate config topic#64
[QRD-7899] feat(configuration-webhook): add affiliate config topic#64javigutierrezfer wants to merge 4 commits into
Conversation
Add a reusable "affiliate" configuration topic to ConfigurationWebhookAPI so payment plugins receive affiliate (Prime) config (enabled, offerId, securityToken) through the existing signed config-push mechanism, with no manual UI (Option A, autoconfig). Mirrors the AdvancedSettings vertical slice: domain model/service, per-store persistence (entity + repository), get/save topic handlers, and wiring in the Topics enum and BootstrapComponent. Signature validation and topic dispatch are reused as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mescalantea
left a comment
There was a problem hiding this comment.
Requesting changes: as it stands the affiliate config never actually reaches the plugin end-to-end. Specifics split into inline comments on the affected code: (1) the connect-time consumer is missing, (2) the outbound proxy isn't in the core, (3) minor dead delete on the repository.
| * | ||
| * @return void | ||
| */ | ||
| public function setAffiliateSettings(AffiliateSettings $affiliateSettings): void |
There was a problem hiding this comment.
Connect-time path has no consumer — required for this PR.
This is the method the connect/credentials flow should call on (re)connect, but nothing does. A companion change to the merchant credentials API now includes an affiliate block (enabled / offer_id / security_token) in the connect-time configuration_data response; the connection flow (CredentialsService::validateAndUpdateCredentials() → ConnectionProxy::getCredentials()) only maps the per-country Credentials and never reads configuration_data.affiliate, so the block is delivered and then dropped.
The initial rollout relies on this path (stores update the plugin and (re)connect before the push side is producing), so the extraction → setAffiliateSettings(new AffiliateSettings($enabled, $offerId, $securityToken)) (snake_case → camelCase, like the other connect-time fields) belongs in this PR. The host plugin also still needs to register the AffiliateSettings entity + a migration so the table exists.
There was a problem hiding this comment.
Done in e4f14cf. The connect flow now reads configuration_data.affiliate and persists it: CredentialsService takes AffiliateSettingsService and, in validateAndUpdateCredentials, maps the block (snake_case enabled, offer_id, security_token) into AffiliateSettings and stores it. When no credential carries the block the stored settings are left untouched, so a partial rollout never wipes a pushed config. Covered by ConnectionServiceTest::testConnectAffiliateSettingsSaved.
| ServiceRegister::registerService( | ||
| AffiliateSettingsService::class, | ||
| static function () { | ||
| return new AffiliateSettingsService( |
There was a problem hiding this comment.
Outbound side isn't wired in the core.
Only the inbound config services are registered here. The conversion/cancellation postbacks the plugin emits are owned by the core as well (the plugin must not make HTTP calls directly), but there's no affiliate proxy under SeQuraAPI/ that resolves the endpoint from the Deployment and sends them. Until that exists the plugin's outbound client stays a no-op, so even once the config is delivered nothing is reported end-to-end.
Include the outbound affiliate proxy here, or split it into the immediate next core change and link it — but it shouldn't be left implicit, since the consumer plugin is already blocked on it.
There was a problem hiding this comment.
Agreed this belongs in the core, but I would keep it out of this PR and ship it as the immediate follow up. Reasoning: this PR is the inbound config half, which is what unblocks the plugin, and the outbound proxy also depends on timon phase 2 routing, so coupling them would hold back config delivery. I will open the follow up for the affiliate proxy under SeQuraAPI/ (endpoint resolved from the Deployment, request emitted in the destination shape) and link it here. Say the word if you would rather it lands in this PR.
There was a problem hiding this comment.
Follow up created: QRD-7949 (https://sequra.atlassian.net/browse/QRD-7949), the integration-core outbound AffiliateProxy, sibling of QRD-7933 (timon Phase 2). Keeping it out of this PR so the inbound config can merge and tag v5.5.0.
| * | ||
| * @throws QueryFilterInvalidParamException | ||
| */ | ||
| public function deleteAffiliateSettings(): void |
There was a problem hiding this comment.
Minor — dead method. deleteAffiliateSettings() (here and in AffiliateSettingsRepositoryInterface) isn't reachable: AffiliateSettingsService only exposes get/set and there's no delete topic/handler. Drop the method + its interface declaration so the contract matches what's reachable.
There was a problem hiding this comment.
Removed in ed1716a, from both the repository and AffiliateSettingsRepositoryInterface.
Re-done as separate inline comments anchored to the affected code (see the newer review).
mescalantea
left a comment
There was a problem hiding this comment.
One more point (a decision, not a bug) — inline.
| public static function fromPayload(array $payload): object | ||
| { | ||
| return new self( | ||
| $payload['isEnabled'] ?? false, |
There was a problem hiding this comment.
Decision needed — enabled is accepted without its credentials. fromPayload casts with ?? false / ?? '' and there's no validation, and the domain AffiliateSettings exposes isEnabled() but has no notion of "fully configured". So a payload with isEnabled = true and empty offerId / securityToken persists as enabled-but-unusable, and any consumer that trusts isEnabled() would treat the feature as ON with no credentials to send.
Decide whether the core should require non-empty offerId + securityToken when isEnabled is true (reject, or coerce to disabled), or whether that "enabled ⇒ has creds" guard is explicitly the consumer's responsibility. Either way it shouldn't be left implicit.
There was a problem hiding this comment.
Decided to guard it in the core, in ed1716a. The AffiliateSettings model now coerces isEnabled to false when offerId or securityToken is empty, so an enabled flag with no credentials can never persist as usable and isEnabled() stays trustworthy everywhere (save webhook, connect time, GET read). I put it in the model rather than only in the request so the connect time path gets the same guarantee. Covered by testSaveAffiliateSettingsEnabledWithoutCredentialsIsCoercedToDisabled.
mescalantea
left a comment
There was a problem hiding this comment.
GET response shape — inline.
| if (!$affiliateSettings) { | ||
| return new SuccessResponse(); | ||
| } | ||
|
|
||
| return new AffiliateSettingsResponse($affiliateSettings); |
There was a problem hiding this comment.
The GET response shape doesn't match the agreed contract — and it echoes the credential.
The agreed get/save split is: get- is an "is it enabled?" read and should return only enabled, while save- carries the full payload (enabled + offerId + securityToken). As written the GET returns the full settings instead — the non-null branch builds AffiliateSettingsResponse, whose toArray() returns AffiliateSettings::toArray() = isEnabled + offerId + securityToken. So:
- it returns more than
enabled, breaking the get/save asymmetry; - it sends
securityTokenback out in a read response — a credential shouldn't travel on a GET.
The no-settings branch returns an empty SuccessResponse (the generic convention for the richer settings topics), which forces the caller to read "empty = disabled". Since this GET is a boolean-state read, an explicit default is cleaner.
Suggested: have the GET return { enabled: bool } only — e.g. $settings ? $settings->isEnabled() : false — so the no-settings case is enabled = false and offerId / securityToken are never echoed. The save- path keeps the full payload.
There was a problem hiding this comment.
Fixed in ed1716a. The GET now returns only the enabled state, never the offer id or token. One call to make on the key name: I kept it camelCase as { isEnabled } to match the rest of this channel (the model and the save payload both use isEnabled) instead of the literal enabled from your note. If simba expects enabled on this read I will switch it, it is a one line change. With no settings it now reads as { isEnabled: false } thanks to the service default below.
mescalantea
left a comment
There was a problem hiding this comment.
Internal read default — inline.
| public function getAffiliateSettings(): ?AffiliateSettings | ||
| { | ||
| return $this->affiliateSettingsRepository->getAffiliateSettings(); |
There was a problem hiding this comment.
getAffiliateSettings() should return a safe default, not null, when no row exists.
As written it returns ?AffiliateSettings (null when nothing is stored), so every internal consumer — the plugin's config provider, the postback gating — has to null-check, and a missed check reads as undefined rather than "off". When the repository has nothing, return a default new AffiliateSettings(false, '', '') (enabled=false, empty offerId / securityToken — the model types both as string, so empty string rather than null) and drop the ? from the return type, so the absence of a row deterministically means "disabled".
This matches the always-constructed pattern the non-nullable settings services already use (e.g. Banner / Widget), and it dovetails with the GET-topic comment above: with the service defaulting, the internal read never sees null and the GET naturally yields enabled = false.
Keep the repository returning null (raw "no row") and apply the default here in the service. The save- path is unaffected.
There was a problem hiding this comment.
Done in ed1716a. getAffiliateSettings() now returns a safe disabled default (an AffiliateSettings with enabled false and empty offer id and token) when nothing is stored, so consumers never null check and absent means disabled. The repository still returns null for the raw no row case; the default is applied in the service. This is what makes the GET above yield isEnabled false when there are no settings.
mescalantea
left a comment
There was a problem hiding this comment.
Docs to update for the new topic/entity — inline.
| public const GET_AFFILIATE_SETTINGS = 'get-affiliate-settings'; | ||
| /** | ||
| * @var string | ||
| */ | ||
| public const SAVE_AFFILIATE_SETTINGS = 'save-affiliate-settings'; |
There was a problem hiding this comment.
Docs need updating to reflect the new topics + entity. The PR adds the topics but not the matching documentation:
README.md— the Configuration Webhook API Layer → TopicHandlerRegistry section enumerates every handler and its topic (currently around lines 3160–3173) but is missing the two new ones. AddGetAffiliateSettingsHandler(get-affiliate-settings) andSaveAffiliateSettingsHandler(save-affiliate-settings). If that section (or the architecture overview) also lists the settings entities/services, addAffiliateSettings/AffiliateSettingsServicethere too.CHANGELOG.md— add a new version entry (Keep a Changelog format, like the existing# [vX.Y.Z]blocks) describing the new affiliate config topics +AffiliateSettingsentity/service, and tag the release (the version lives in the changelog/tag —composer.jsonhas noversionfield).
(DEVELOPER_GUIDE.md is dev-env/IDE setup only, so nothing needed there.)
There was a problem hiding this comment.
Added in 2c4f05c. Both handlers are now listed under TopicHandlerRegistry in the README, and there is a CHANGELOG entry for the affiliate topics, entity and service plus the connect time provisioning. Heads up on your note: the CHANGELOG had drifted, its top was v1.0.13 while the tags reached v5.4.0, so I added the entry as v5.5.0 at the top but the 5.x history in between is still missing. Worth a separate cleanup if the team wants the changelog accurate again.
mescalantea
left a comment
There was a problem hiding this comment.
Versioning note — inline.
| AffiliateSettingsRepositoryInterface::class, | ||
| static function () { | ||
| return new AffiliateSettingsRepository( | ||
| RepositoryRegistry::getRepository(AffiliateSettings::getClassName()), |
There was a problem hiding this comment.
Versioning note: this is a SemVer MINOR and is safe to release as one — no breaking change for existing consumers.
I checked whether updating to this version without a consumer registering the AffiliateSettings entity would break existing integrations. It doesn't, because resolution is lazy and per-topic:
ConfigurationWebhookController::handleRequest()resolves onlygetHandlerForTopic($topic)for the incoming topic.Topics::ALL_TOPICSisn't iterated anywhere (it's referenced only inTopics.php), so adding the two entries has no eager effect.getHandlerForTopic()→ServiceRegister::getService()runs the handler closure only when that topic actually arrives;initTopicHandlers()/registerService()only register lazy closures at boot.
So this getRepository(AffiliateSettings::getClassName()) is reached only when a get/save-affiliate-settings webhook is handled — which is opt-in (only sent to stores enrolled in the feature). A consumer that bumps without registering the entity boots fine and keeps all existing behaviour; the worst case is a single 500 on an affiliate webhook for an unconfigured store, not a regression of existing functionality.
Per SemVer that's a MINOR (additive, backward-compatible) — e.g. 5.2.0 on the current 5.1.x line. Heads-up for consumers: registering the entity is required to use the feature (an integration step), not to survive the upgrade.
There was a problem hiding this comment.
Thanks, agreed it is a backward compatible MINOR. Targeting v5.5.0 as the release tag since the tags already reached v5.4.0 (so not 5.2.0). The plugin will register the entity and bump its constraint to that tag when it consumes this.
…quires-creds, drop dead delete
- get-affiliate-settings returns only { isEnabled } and never echoes the offer id or security token
- AffiliateSettingsService::getAffiliateSettings() returns a safe disabled default instead of null, so consumers never null-check and absent means disabled
- AffiliateSettings coerces enabled to disabled when offer id or security token is empty, keeping isEnabled() trustworthy for every consumer
- drop unreachable deleteAffiliateSettings() from the repository and its interface
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ffiliate Read the affiliate block (enabled/offer_id/security_token) from the connect-time configuration_data and persist it via AffiliateSettingsService, so (re)connecting a store provisions its affiliate settings. This closes the headline gap: the block was delivered by timon and then dropped. When no credential carries an affiliate block (e.g. the merchant API is not emitting it yet) the stored settings are left untouched. - inject AffiliateSettingsService into CredentialsService; persist in validateAndUpdateCredentials - correct Credentials::getPayload() docblock to array<string, mixed> - wire AffiliateSettingsService in BootstrapComponent and the test service register - cover with ConnectionServiceTest::testConnectAffiliateSettingsSaved Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…+ CHANGELOG - README: list GetAffiliateSettingsHandler and SaveAffiliateSettingsHandler under TopicHandlerRegistry - CHANGELOG: add a v5.5.0 entry for the affiliate config topics, entity/service and connect-time provisioning (note: the changelog had drifted at v1.0.13 while tags reached v5.4.0) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What is the goal?
Add a reusable "affiliate" configuration topic to integration-core's
ConfigurationWebhookAPI, so every seQura payment plugin can receive its affiliate (Prime) configuration — enabled, Offer ID, Security Token — through the existing signed config-push mechanism, with no manual UI. Part of the autoconfiguration-first approach (Option A) of epic QRD-7897.References
woocommerce-sequra#163 (consumer plugin, QRD-7898)How is it being implemented?
Mirrors the simplest existing topic, AdvancedSettings, as an end-to-end vertical slice:
Domain/Affiliate):AffiliateSettingsmodel (isEnabled,offerId,securityToken),AffiliateSettingsRepositoryInterface, andAffiliateSettingsService— the reusable read service plugins use to obtain the resolved config.DataAccess/Affiliate):AffiliateSettingsentity (indexed bystoreId) + repository.SaveAffiliateSettingsRequest,AffiliateSettingsResponse, andGet/Savetopic handlers.get-affiliate-settings/save-affiliate-settingsadded to theTopicsenum (andALL_TOPICS), and registered inBootstrapComponent(repository, service, topics, handler services).Signature validation and the topic dispatch are reused as-is; no new signing logic is introduced.
Caveats
isEnabled/offerId/securityToken) are the proposed config contract and may be adjusted once the cross-team contract is confirmed (decision D5 in the epic); the structure won't change.Does it affect (changes or update) any sensitive data?
The Security Token is a credential-like value. It is persisted per store like the other settings and only delivered through the existing signed config-push. It is not logged.
How is it tested?
Automatic tests (PHPUnit):
AffiliateSettingsentity test, repository test (get/set/update/delete + multistore isolation), and service test.ConfigurationWebhookAPITest: save persists, get returns the persisted config, get with no settings returns empty.Full suite: 819 tests OK (16 new), 0 regressions. PHPCS (PSR12) clean. PHPStan level 6: no errors.
How is it going to be deployed?
Standard deployment (library release/tag, consumed by the payment plugins).
🤖 Generated with Claude Code