Replace request-time Partner Ads notification with conversion entity + cron command - #55
Open
loevgaard wants to merge 2 commits into
Open
Replace request-time Partner Ads notification with conversion entity + cron command#55loevgaard wants to merge 2 commits into
loevgaard wants to merge 2 commits into
Conversation
…mand Instead of sending the Partner Ads notification during the thank-you page request, a pending Conversion (order, partner id, state) is stored when a referred order is completed, and a new console command - setono:sylius-partner-ads:process-conversions, meant to be run via cron - notifies Partner Ads about conversions whose orders have been paid, with automatic retries and a failed state after too many tries. This means a slow or failing Partner Ads endpoint can never affect checkout, unpaid orders are no longer reported, and the unique order constraint prevents duplicate notifications. The Buzz fallback client also gets an explicit timeout instead of Buzz's default of none. The messenger machinery (Notify command, handler, command bus compiler pass, messenger config) and the thank-you page subscriber are removed, along with ProgramContext, whose only consumer was that subscriber. Fixes #49, closes #50 Claude-Session: https://claude.ai/code/session_01Mt12J8vdGwwWg4V23uoEf9
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 3.x #55 +/- ##
============================================
- Coverage 86.84% 84.75% -2.10%
- Complexity 80 94 +14
============================================
Files 21 20 -1
Lines 365 492 +127
============================================
+ Hits 317 417 +100
- Misses 48 75 +27 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
loevgaard
commented
Aug 24, 2026
| xsi:schemaLocation="http://doctrine-project.org/schemas/orm/doctrine-mapping | ||
| http://doctrine-project.org/schemas/orm/doctrine-mapping.xsd"> | ||
|
|
||
| <entity name="Setono\SyliusPartnerAdsPlugin\Model\Conversion" table="setono_sylius_partner_ads__conversion"> |
Member
Author
There was a problem hiding this comment.
should be mapped superclass. The resource bundle will take care of it
Registers a ResourceController for the conversion resource and adds admin routes (index and delete only - conversions are system-created), a grid with a state filter, a marketing menu item, and English and Danish translations. This gives merchants visibility into pending, notified, and failed conversions, including the last error. Claude-Session: https://claude.ai/code/session_01Mt12J8vdGwwWg4V23uoEf9
loevgaard
commented
Aug 24, 2026
| permission: true | ||
| templates: "@SyliusAdmin\\shared\\crud" | ||
| grid: setono_sylius_partner_ads_admin_conversion | ||
| only: ['index', 'delete'] |
Member
Author
There was a problem hiding this comment.
What happens if I delete a conversion?
loevgaard
commented
Aug 24, 2026
| $services->alias(ProgramContextInterface::class, ProgramContext::class); | ||
| service('request_stack'), | ||
| service(CookieHandlerInterface::class), | ||
| // factory and repository services created by AbstractResourceExtension::registerResources() |
Member
Author
There was a problem hiding this comment.
Remove comment or change it to something about the resource bundle instead
loevgaard
commented
Aug 24, 2026
| private readonly ProgramRepositoryInterface $programRepository, | ||
| private readonly ClientInterface $client, | ||
| private readonly OrderTotalCalculatorInterface $orderTotalCalculator, | ||
| private readonly ObjectManager $conversionManager, |
Member
Author
There was a problem hiding this comment.
Use https://github.com/Setono/doctrine-orm-trait/ instead of injecting entity manager
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.
Fixes #49, closes #50.
What changed
The Partner Ads notification no longer happens inside the customer's thank-you page request. Instead:
Conversionentity (new tablesetono_sylius_partner_ads__conversion) is created when a referred customer completes an order: it holds the order (one-to-one, unique), the partner id from the cookie, a state (pending/notified/failed), a tries counter, the last error, andnotifiedAt.CreateConversionSubscriberlistens onsylius.order.post_completeand stores the pending conversion — no HTTP involved. It skips non-positive partner ids from tampered/mangled cookies (partially addresses Query parameter 'paid' is not validated: garbage overwrites valid attribution, array input causes 400 on any shop URL #51) and is idempotent (checks for an existing conversion; the DB unique constraint backs this up).setono:sylius-partner-ads:process-conversions(meant to run via cron every 5–15 minutes), picks up pending conversions whose order payment state ispaidand notifies Partner Ads. Failures are caught per conversion: the error is recorded, the conversion stays pending and is retried on the next run, and after--max-tries(default 10) it is markedfailed.Why
Removed (breaking changes)
Since the CLI send is already out-of-band, the async messenger machinery is no longer needed and has been removed:
NotifySubscriber(thank-you page listener),Notifymessage,NotifyHandler,RegisterCommandBusPassProgramContext/ProgramContextInterface(only consumer wasNotifySubscriber)messengerconfiguration node (command_bus,transport) — apps with this config must drop it on upgradeUpgrade notes
doctrine:migrations:diff+migrateto create the conversion table.setono:sylius-partner-ads:process-conversionsvia cron.setono_sylius_partner_ads.messengerconfig.Admin UI
The conversion is exposed as a full Sylius resource in the admin: a grid under Marketing → Partner Ads Conversions showing order number, partner id, state, tries, last error, and timestamps, with a state filter (pending / notified / failed) and a delete action. Index and delete routes only — conversions are system-created, so there is no create/update. English and Danish translations included.
Testing
Conversion,CreateConversionSubscriber, andProcessConversionsCommand; removed tests for deleted classes.https://claude.ai/code/session_01Mt12J8vdGwwWg4V23uoEf9