Conversation
ISSUE: LIS-111
ISSUE: LIS-111
ISSUE: LIS-111
ISSUE: LIS-111
ISSUE: LIS-111
ISSUE: LIS-111
ISSUE: LIS-111
ISSUE: LIS-111
ISSUE: LIS-111
m1k3lm
left a comment
There was a problem hiding this comment.
Overview
Adds a storefront banner system: admins can upload images per country/page via the merchant portal; the module stores them under pub/media/sequra/banners/{storeId}/... and renders them on home, product, category listing, and cart pages. Includes a BannerService (filesystem-backed storage with base64 decode + MIME validation), a CountryResolverService (visitor country detection), a Block + phtml template, four layout XMLs, an uninstall cleanup, and a Version330 data patch.
High-impact concerns
-
Block caching (
Block/Banner.php). The block extendsTemplatebut does not overridegetCacheKeyInfo()and the layouts do not setcacheable="false". Under FPC/block cache the first request's banner — including the resolved country — will be cached and served to other visitors. Either setcacheable="false"on the block in layout, or overridegetCacheKeyInfo()to include$storeId,$country, and$displayLocationplus a TTL. Otherwise visitors from different countries see each other's banners (or nothing), and the API call hits on every cache miss. -
Version330::apply()swallowsThrowableand returns normally. When migration fails, the patch is recorded as applied by Magento and never retries. IfStoreIntegrationMigrateTaskis critical for the upgrade, rethrow sosetup:upgradefails loudly. If it's truly best-effort, the log line should be louder thanlogError. -
Empty
alt=""on banner images (banner.phtml). Accessibility + SEO issue for a marketing banner. Plumb analtTextfield through the API response or use a sensible default (e.g. "seQura financing"). -
No automated tests for
BannerService(457 lines, base64 + MIME + filesystem ops + cross-extension cleanup + relocate-on-rename). At minimum unit-testdecodeBase64,assertIsValidImage,resolveExtension,relativePathFor, andchangeBannerImageDisplayLocation's happy/missing/same-location branches.
Smaller issues
BannerService::changeBannerImageDisplayLocation— when no source exists ORold === new, returns a URL synthesized with a defaulted.pngextension regardless of whether anything is there. Caller has no signal the file is missing; admins may see a broken<img src>. Either returnnull/empty or document the contract clearly.BannerService::storeSegment()falls back to literal'default'directory whenStoreContexthas no storeId, whilegetMediaBaseUrl()falls back toStoreManager's default store. Asymmetry could produce 404s in edge cases — worth aligning.Block\Banner::getBannerData()catchesException, notThrowable. Some SDK paths can surfaceError(e.g. type errors from a malformed response); upgrading toThrowablekeeps render bulletproof.Bootstrap.phpremoves theStoreIntegrationrepository registration. TheVersion330patch migrates the entity first — ordering looks correct, but please smoke-test an upgrade from an installation with existingStoreIntegrationrows.banner.phtmlcorrectly usesrel="noopener noreferrer"withtarget="_blank". ✓
Nice work on input validation, multi-variant cleanup, and the relocate-on-rename flow — those are well factored. The storefront caching story is the one that will bite in production, so let's address that before merge.
m1k3lm
left a comment
There was a problem hiding this comment.
The Block/Banner.php caching issue should be solved.
ISSUE: LIS-111
ISSUE: LIS-111
What is the goal?
References
How is it being implemented?
BannerService- saves admin-uploaded banners topub/media/sequra/banners/{storeId}/{COUNTRY}_{displayLocation}.ext}, deletes them, and (new) relocates them when only the display location changes so admins don't have to re-upload.Setup/Uninstall.php- clearspub/media/sequra/bannerson uninstall.Setup/Patch/Data/Version330.php- one-shot migration that re-registers the store integration viaStoreIntegrationMigrateTask. Targets module version3.3.0- the next release onetc/module.xmlis expected to be3.3.0so this patch applies.Services/Bootstrap.php- removes the deadStoreIntegrationDataAccess entity registration (dropped in integration-core).actions/checkoutandactions/cacheto@v5(Node 24)How is it tested?