Add banner upload functionality to the shop home screen#59
Add banner upload functionality to the shop home screen#59AleksandarBoljanovic wants to merge 21 commits into
Conversation
LIS-111
LIS-111
LIS-111
LIS-111
LIS-111
LIS-111
LIS-111
ISSUE: LIS-111
ISSUE: LIS-111
ISSUE: LIS-111
# Conflicts: # tests/BusinessLogic/Domain/Disconnect/Services/DisconnectServiceTest.php
ISSUE: LIS-111
m1k3lm
left a comment
m1k3lm
left a comment
There was a problem hiding this comment.
Reviewed the banner-upload feature end-to-end. Overall well-organized and follows existing patterns (DTOs, entity with IndexMap, bootstrap registration, topic handlers, BannerServiceInterface as a minimal integration extension point). Tests are comprehensive (~668-line service test, +265-line webhook test). Comments below cover correctness, design, and a couple of doc-vs-behavior mismatches — none are hard blockers, but I'd want the save-atomicity ordering, required-field validation in SaveBannerSettingsRequest, and the PR description fixed before merge.
Risks summary
| Risk | Severity |
|---|---|
| Orphan remote images on partial save failure | Medium |
| Partial-disconnect leaves banner state | Low (likely intentional, worth a comment) |
| Unvalidated image size / required fields | Low–Medium |
Cross-namespace trait reuse (Webhook uses AdminAPI trait) |
Low (style) |
| Response shape divergence between Admin / Webhook | Low (UX) |
InvalidURLException vs existing InvalidUrlException |
Low (style) |
Notes that don't fit on a line
- PR description vs. behavior: the description says "ConnectionService triggers banner-settings provisioning on connect", but the actual
ConnectionService.phpchange is two@throwsdocblock additions. Provisioning is indirect (BANNER capability advertised viacreateStoreIntegration, merchant portal then sendssave-banner-settings). Please update the description. - PHP 8.5 in
run-tests.shbut not in the GH workflow matrix: if 8.5 support is intended, mirror it in CI; otherwise consider keeping it local. actions/cache@v5and dev-dep bumps (phpstan 1.12.33, phpcs 3.13.5, webmozart/assert 1.12.1 which now requiresext-date/ext-filter): worth a quick sanity check against the lowest supported PHP (7.2) target.
| isset($bannerConfig['imageBase64']) && $bannerConfig['imageBase64'] !== '' | ||
| ? $bannerConfig['imageBase64'] | ||
| : null | ||
| ); |
There was a problem hiding this comment.
Required fields (country, linkUrl, displayLocation) silently default to '' when missing. The empty linkUrl happens to be caught later by assertValidUrl (filter_var('', FILTER_VALIDATE_URL) fails), but a missing country slips through to the persistence layer and the remote saveBannerImage/deleteBannerImage calls. Validate required fields explicitly with a clear error here rather than relying on downstream side effects.
| * | ||
| * @package SeQura\Core\BusinessLogic\AdminAPI\BannerSettings\Requests | ||
| */ | ||
| trait SaveBannerSettingsRequest |
There was a problem hiding this comment.
Cross-namespace trait reuse. This trait lives under AdminAPI\BannerSettings\Requests but is used by ConfigurationWebhookAPI\Requests\BannerSettings\SaveBannerSettingsRequest. The webhook reaching into the Admin namespace for shared code is a leaky abstraction — moving the shared transformation into a common namespace (or extracting a small request-to-domain transformer) would be cleaner.
| { | ||
| if (empty($this->bannerConfigs)) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Returning [] when empty instead of ['bannerConfigs' => []] forces every caller to defensively use $settingsArray['bannerConfigs'] ?? [] (see both Admin and Webhook BannerSettingsResponse). Returning the consistent shape unconditionally would remove that special case at every call site.
| /** | ||
| * @var string|null | ||
| */ | ||
| protected $imageBase64; |
There was a problem hiding this comment.
The imageBase64 field is purely transient input — it's scrubbed back to null in BannerSettingsService::resolveBannerImage (line 257) before persistence, and toArray deliberately omits it. It works, but mixing input-only and persisted fields in the same DTO is a smell. Consider splitting into BannerInput (carries base64) and Banner (persisted shape).
3 participants
What is the goal?
References
How is it being implemented?
BannerCheckoutController) withgetBannerForLocationendpoint, request and response - used by the storefront to fetch the banner to render for a given country/display locationget-banner-settings,save-banner-settings)BannerSettingsdomain context:BannerandBannerSettingsmodels,BannerSettingsentity withEntityConfiguration,BannerSettingsRepositoryInterface+ repository, andBannerSettingsServiceorchestrating fetch/save/delete and display-location-aware image updatesBannerServiceInterface) as the platform extension point - exposes display locations, image upload/delete, and country lookup so each e-commerce platform supplies its own implementationBANNERcapability toCapabilityand surfaced it viaStoreIntegrationServiceso platforms can advertise banner supportConnectionServicetriggers banner-settings provisioning on connect,DisconnectServicecleans up persisted settings and uploaded images on disconnect (with an error log if image deletion on the integration side fails)BootstrapComponent(repository, service, controllers, webhook handlers)How is it tested?