Added multi-website gift card associations, customer "Check Balance" page, and rate-limited lookups - #1062
Conversation
A single website_id scalar on giftcard ties each card to exactly one website, which doesn't reflect how multi-website stores actually use gift cards in practice (head office issues a single card that customers spend across any of the brand's storefronts). This adds a junction table so a card can be valid on an arbitrary subset of websites without duplicating the card row. - giftcard_website (giftcard_id, website_id) with the pair as PK, indexed by website_id for the apply-time validity lookup, FKs to giftcard and core_website both ON DELETE CASCADE so cleaning up either side stays simple. - giftcard.website_id kept and made nullable for backward compatibility with any external code still reading the scalar; the authoritative association becomes the junction. Scheduled for removal in a future major. - upgrade-1.0.0-1.1.0.php backfills the junction by CROSS JOINing existing cards with every active website (website_id > 0). Permissive on purpose: no existing card loses validity over the upgrade; operators tighten the scope per-card from the admin multiselect introduced in a follow-up commit. INSERT IGNORE so a rebuild can re-run the script without errors. Subsequent commits in this PR wire the model, resource, admin form, admin grid, customer "Check Balance" page, and admin history tab to the junction.
Switches the model's website handling to the new giftcard_website junction.
- Model:
- getWebsiteIds() lazy-loads the associations from the junction and memoises
them under the `website_ids` data key.
- setWebsiteIds() takes an int[], dedupes and ignores 0/negative ids, and
stores the set on the model; persistence happens in the resource's
_afterSave hook.
- isValidForWebsite() switches from `(int) website_id === target` to an
in_array check against the junction. A card with an empty website set
fails validation everywhere — fail closed rather than silently treating
"no associations" as "valid anywhere", which would let a botched import
or hand-crafted row drain balance from any storefront.
- Resource:
- _afterLoad hydrates `website_ids` so subsequent getWebsiteIds() reads
don't re-query the junction.
- _afterSave syncs the junction inside a transaction: delete the previous
rows for this card, insert the new set. Rolls back on any partial-write
error so the card never ends up with a torn association set.
- getWebsiteIds(int $giftcardId) is the single read path, used by the
model's lazy loader and reusable by future callers (collection, grid,
customer balance lookup).
- Skipped entirely when no `website_ids` was set on the model — a save
that only touched balance/status won't trample the associations.
Replaces the single-website select on the gift card edit form with a multiselect, and removes the "existing cards show website read-only" branch — operators can re-scope an issued card's websites at any time without having to delete and reissue. - New card: multiselect defaults to the first available website (the admin's current scope) so create-flow keys, currency derivation and the legacy giftcard.website_id FK all keep working. - Existing card: multiselect pre-selects the websites already in the giftcard_website junction. If the row hasn't been backfilled yet (e.g. a fresh upgrade running before the data-migration script), fall back to the legacy website_id scalar so the field isn't blank. Controller: - Normalises the multiselect post into a clean int[] (dedupe, drop empty/0 ids, accept either an array or a stray single value if an older form posts the old `website_id` name). - Falls back to the current admin website when no selection was posted, so saves never land with an empty association set that would orphan the card from every storefront. - Sets legacy `website_id` to the first selection so the existing currency derivation and FK back to core_website keep working alongside the junction. Note text on the multiselect calls out the first-selection currency rule so admins picking websites with mixed base currencies notice the implicit conversion before issuing the card.
The grid was indexing on giftcard.website_id — a scalar that no longer represents the full association set, and pre-1.1.0 a row could perfectly legitimately have a value the operator never asked about. Switches the grid to read from the giftcard_website junction. - _prepareCollection LEFT JOINs the junction and selects GROUP_CONCAT'd website ids into a `website_ids` alias, grouping by giftcard_id. LEFT JOIN so orphaned rows (no junction entries) still appear in the listing — the renderer surfaces them as an em dash so the operator can re-scope them from the edit page rather than silently filtering them out. - The Websites column is filterable + sortable on the alias. A custom filter_condition_callback translates the column dropdown's single website_id into a `HAVING FIND_IN_SET(N, website_ids)` (HAVING because the alias is computed, not a raw column reference). - New renderer Maho_Giftcard_Block_Adminhtml_Giftcard_Renderer_Websites maps the GROUP_CONCAT'd ids to comma-separated website names via the admin system-store helper hash; unknown ids fall back to "[id N]" so a deleted-website row stays visibly anomalous instead of crashing. Indirectly fixes the prior "card invisible after creation" bug in multi-website installs, where the grid's old column tried to read a website_id that the save flow wasn't persisting reliably.
Admins tracing where a card's balance went currently have to leave the card
to scan the global Sales → Gift Card History page, then filter or remember
the giftcard_id. This adds an in-context "Transaction History" tab on the
edit page so the audit trail lives next to the card it describes.
Implementation reuses the existing standalone History/Grid:
- New Edit/Tabs container wraps the edit form ("General" tab) and the new
history tab. The History tab is only added when a card has been saved —
the create form stays a single-screen flow.
- New Edit/Tab/History extends the existing History/Grid and scopes the
collection to the current card via the registered current_giftcard. Mass
actions and row-click are suppressed because this view is a read-only
audit trail, not a launchpad.
- Edit container's _prepareLayout swaps its auto-resolved Form child for
the Tabs container so the existing Form_Container template's
`<form id="edit_form">` wrapper still posts the form fields normally;
the read-only history tab introduces no inputs.
- New historyGridAction in the admin controller is the AJAX endpoint the
inner Mage_Adminhtml_Block_Widget_Grid uses for sort/filter/paginate
reloads; it re-registers the gift card from the id query param so the
scope matches the parent edit page.
Adds a customer-facing page under My Account where a logged-in shopper can
look up the remaining balance + expiry on a gift card by code. No history
view — that stays admin-only (transaction comments are operational notes
not meant for the customer-facing audit trail).
Files:
- controllers/BalanceController.php — front-area controller with two
attribute-routed actions:
- GET /giftcard/balance renders the lookup form
- POST /giftcard/balance/check validates and stashes the result
preDispatch enforces customer auth on both, matching every other
customer/account/* action so a deep link while signed out hands off to
the standard login flow.
The check action treats "not found", "expired", "disabled" and "no
website membership" as the same opaque "could not be found" response —
this endpoint is not an oracle for walking codes.
- Block/Customer/Balance.php — pulls the most recent result out of
giftcard/session and consumes it on render so a back/forward navigation
doesn't re-display the previous customer's check on a shared device.
- Model/Session.php — per-customer session namespace for the result +
flash messages.
- app/design/frontend/base/default/layout/giftcard.xml — adds the
navigation link to customer_account and registers the page handle.
- app/design/frontend/base/default/template/giftcard/customer/balance.phtml
— pure base-theme markup using box-account / form-list / buttons-set /
required pattern so any storefront theme inheriting from base/default
picks the page up without overriding a single class.
The `balance` and `initial_balance` columns are stored as decimal(12,4) so arithmetic over partial redemptions, currency conversion, and post-tax amounts doesn't accumulate rounding error. The admin form was leaking the storage precision straight to the input field — "30.0000" — which reads as unfinished UX rather than precise bookkeeping. Format both fields to 2dp before passing them to the form's setValues(). The DB column is untouched; saves still land back as decimal(12,4) and the existing balance-adjustment history captures the audit trail at full precision.
The My Account "Check Gift Card Balance" page accepts a code in a form
input. Authentication keeps anonymous bots out, but a logged-in account
could still walk codes — sequential card formats (think "GC-XXXX-XXXX"
prefixes) are exactly the kind of namespace this enables. With no limit
the attack cost is one HTTP round-trip per attempt.
Add per-customer rate limiting via the shared
Mage\Core\Helper\Data::rateLimiterBy() factory (scope key = customer id),
10 failed lookups per hour. "Check upfront, hit() on failure only"
mirrors the Mage_Sales_Helper_Guest pattern so a customer holding several
legitimate cards isn't penalised for genuine lookups — only mis-attempts
count toward the window.
Errors stay opaque ("could not find an active gift card") so the limiter
itself doesn't leak which side of the failure the attacker hit; when
they cross the threshold they see a generic "Too many recent lookup
attempts" message and have to wait the window out.
AGENTS.md requires every translatable string (`$this->__()` or `helper()->__()`) to land in app/locale/en_US/<Module>.csv in the same commit as the code that introduces it. This sweep covers the strings added across the multi-website + customer balance PR: - Admin form: "Websites", "Hold Ctrl/Cmd to select multiple. …" - Admin edit-page tabs: "General", "Transaction History" - Customer balance page: "Check Gift Card Balance", "Gift Card Balance" (My Account nav link), "Customer Gift Card Balance" (page handle), "Enter your gift card code", "Enter the code printed on…", "* Required Fields", "Check Balance", "Balance for %s", "Current balance:", "Expires:", "Back to My Account", "We could not find an active gift card for that code on this store." - Rate limit: "Too many recent lookup attempts. Please wait a while before trying again." CSV stays alphabetically sorted per house style.
…code
Sweeps lint findings against the rest of the multi-website + customer
balance PR:
- Edit/History.php and Edit/Tab/Form.php now carry `#[\Override]` on
the four Tab_Interface methods (`getTabLabel`, `getTabTitle`,
`canShowTab`, `isHidden`) — house-rule per `maho-coding-guidelines`
and PHPStan's `method.missingOverride` rule.
- Grid.php having() filter callback hands the raw SQL string instead
of wrapping it in `Maho\Db\Expr` — the having() signature types its
`$cond` as string and the Expr wasn't buying anything here.
- Edit/Tab/Form.php drops a redundant `!== null` check after `isset()`
on the 2dp balance formatting block. `isset` already excludes null.
- Block/Customer/Balance.php constructs its typed return shape
explicitly from the session payload rather than handing the raw
array through and hoping it matches the `@return array{...}` docblock.
- BalanceController::preDispatch returns `$this` so the override
matches the parent's `@return $this(...)` signature.
- BalanceController::checkAction adds a `@var` for the resource model
before calling `loadByCode($card, $code)` — PHPStan otherwise sees
the abstract `Mage_Core_Model_Resource_Db_Abstract` return type and
doesn't know the giftcard resource defines `loadByCode`.
Verified clean against `composer lint:cs-fixer`, `lint:phpstan` and
`lint:rector` on the PR diff.
CI surfaced that switching `isValidForWebsite()` to a junction-membership
check broke the back-compat path: any code that sets the legacy
`website_id` scalar directly (programmatic creation, imports, fixtures,
the existing Pest tests) but doesn't also call `setWebsiteIds()` produces
an empty junction and `isValidForWebsite()` then fails closed everywhere.
The 1.0.0→1.1.0 backfill handles existing rows on upgrade, but new
unmigrated saves are still legitimate.
`getWebsiteIds()` now falls back to `[(int) $this->getWebsiteId()]` when
the junction is empty and the legacy scalar is set. Junction wins when
present (multi-website cards behave as designed); single-website legacy
cards keep working without explicit junction population.
Affected tests:
- GiftcardTest::isValidForWebsite (unit)
- MultistoreMulticurrencyTest (8 cases under integration)
- OrderFlowTest::isValidForWebsite (integration)
- Total\QuoteTest::balance application (integration)
All set the legacy scalar via setWebsiteId() / setData('website_id', N)
and expect isValidForWebsite() to return true.
The balance lookup page accumulated error messages across submits — each
failed lookup adding to giftcard/session, none being cleared between
GETs. The custom session storage needed wiring into the messages block's
storage types via `_initLayoutMessages('giftcard/session')`, and even
then the lifecycle wasn't clearing reliably (suspected Hotwire-Turbo
interaction on themes where it's enabled).
Switching to customer/session for the user-facing flash messages drops
the whole custom-session-wiring path: customer/session is the standard
storage the My Account messages block reads from and clears via the page
chrome on every render. giftcard/session stays in use, but now only for
the lookup result payload (a one-shot data hand-off between the POST
handler and the GET renderer) — no flash messages, no clearing concern.
- saveAction: keep model on old balance before adjustBalance() so the
recorded history delta and final balance are both correct (was logging
a zero-amount adjustment)
- saveAction: add to _setForcedFormKeyActions for CSRF protection
- checkBalanceAction: use Mage::helper('core')->jsonEncode() instead of
native json_encode()
- BalanceController::checkAction: validate the form key explicitly so a
cross-site POST can't burn a customer's rate-limit slots
- Resource\Giftcard: stop populating website_ids in _afterLoad; it
defeated the _afterSave skip-guard (redundant junction rewrites on
every save, and an exception when isValid() auto-expires an orphaned
card). getWebsiteIds() already lazy-loads on demand
- admin currency-hint JS: target the website_ids multiselect and read
selectedOptions[0] (was the stale single-select website_id id)
- balance.phtml: correct SPDX license to AFL-3.0 for templates
|
Reviewed in depth (logic / security / conventions) and pushed fixes in e36a83d. Two correctness bugs plus a CSRF gap were the notable ones: Correctness
Security
Conventions / polish
Verification: |
…r render PR MahoCommerce#1059 marked Form_Container::getSaveUrl() with #[\Deprecated], but getFormHtml() still calls it on every admin form-container render. Under PHP 8.4 the attribute raises E_USER_DEPRECATED at runtime, surfacing as a hard error on admin edit pages (gift card edit, etc.). getSaveUrl() is a thin alias for getFormActionUrl(); call the latter directly.
The getFormHtml() call to the deprecated getSaveUrl() was migrated to getFormActionUrl(); remove the now-unmatched baseline ignore so PHPStan (reportUnmatchedIgnoredErrors) stops failing CI.
The 1.0.0->1.1.0 junction backfill is pure DML, so it belongs in the data-setup phase (applyDataUpdates), not sql schema setup. Relocate it to data/giftcard_setup/data-upgrade-1.0.0-1.1.0.php so it runs after the declarative schema creates giftcard_website. Content unchanged.
|
Thank you @mageaustralia! Question: what if a giftcard is associated to 2 websites with 2 different currencies? since there's no currency directly tied to the giftcard it kinda worries me |
|
I think we have 2 ways:
I'm might be missing something here but with multiwebsite I don't think it's safe to leave it as it is. what do you think? |
|
@fballiano - You're always one step ahead. I'd probably go with restrict associations to currency-matched websites . Validate in _beforeSave() that all website_ids share the same getBaseCurrencyCode() in the immediate term: Longer term options:
What do you think? My particular use case is that we have the online store and we have a separate store for POS - POS handles things a bit differently to live, mostly that it allows purchasing disabled / out of stock products. It also allows us to track In Store sales vs Online Sales easily but it also requires allowing the use of a GC purchased online to be used in-store. |
|
@mageaustralia as a first implementation it's ok but I wonder how could we enforce it, like what if you edit a giftcard and change the websites completely to another currency. i mean it's an edge case but.. Also, would it be possible to keep the website_ids as CSV in the giftcard column? It's only OCD but the new table and the migration script with the leftover column (a real limitation of declarative schema) trigger me a bit. It's true that the separated table is a standard in our codebase.. |
|
@fballiano — sorry, I've been on holidays the last week, so slow to reply to this. Two solid points, replying to both: On the currency edit-mid-life concern Rather than a That makes the mismatch impossible to save in the first place without an error message being needed, and covers the edit-mid-life case naturally (once the card has a currency, the picker only offers same-currency websites). On the schema — CSV column vs junction Happy to do whatever you like here. I was trying to stick with Maho conventions — every multi-website relationship in core ( |
…the legacy website_id column - sql/schema.php no longer declares website_id (nor its index/FK): the declarative pass preserves the column via additive merge until the new sql/giftcard_setup/upgrade-1.0.0-1.1.0.php backfills the junction from it and drops it. Fresh installs skip the guarded block; re-runs are no-ops. - Backfill is conservative (one row per card: its original website) instead of the previous permissive data-phase script (every card on every active website), which would have made existing cards spendable cross-website at face value in other currencies - pre-1.1.0 validation was a strict website match, not store-agnostic. - Repointed every remaining scalar reader to the junction: currency derivation (getWebsite), cart/observer/checkout website validation (still comparing the scalar, so multi-website cards were rejected outside their first website), admin save/form/JSON, grid and history-grid currency lookups, purchase-flow card creation. Removed setWebsiteId entirely. - New cards saved without explicit associations default to the current website; an explicit empty set still fails closed. - All websites associated to one card must share a base currency (the card balance is denominated in it), enforced at save. - getWebsiteIds() no longer memoises lazy reads into the pending-change data key, so balance saves on the checkout path stop rewriting the junction. - Added Collection::addWebsiteFilter() plus website-association tests, including one that rebuilds the pre-1.1.0 schema and exercises the upgrade script end to end.
|
Pushed a8a7aba: the How the migration works (it turns out the declarative schema handles this fine): Backfill is now conservative, not permissive. The data-phase script associated every existing card with every active website — but pre-1.1.0 validation was a strict website match ( Latent bug fixed along the way: Also in this commit: same-currency guard on save (all associated websites must share one base currency — the server half of the picker rule we discussed; the greyed-out multiselect UI is still all yours if you want to add it on top), default-to-current-website for programmatic saves (restores the pre-1.1.0 default), Note for dev machines that already ran this branch: your Verification: |
…ebsite-and-customer-balance # Conflicts: # app/code/core/Maho/Giftcard/controllers/Adminhtml/GiftcardController.php
|
Merged current main into the branch (4bfe2e8) and adapted the giftcard code the merge brought in:
Verification after the merge: |
2 participants
What's in this PR
Multi-website associations (4 commits)
A single
website_idscalar ongiftcardtied each card to exactly one website, which doesn't reflect how multi-website stores actually use gift cards (head office issues a card customers spend across any of the brand's storefronts). This adds agiftcard_websitejunction so a card can be valid on an arbitrary subset of websites:giftcard_websitetable (PKgiftcard_id, website_id, indexed bywebsite_id, FKs togiftcardandcore_websitebothON DELETE CASCADE)Maho_Giftcard_Model_Giftcard::getWebsiteIds()/setWebsiteIds()backed by the junction;isValidForWebsite()now does a membership check (in_arrayagainst the set) and fails closed on no associations_afterSavesyncs the junction in a transaction; an explicit empty set throwsMage_Core_Exception(A gift card must be associated with at least one website.) rather than silently orphaning the cardHAVING FIND_IN_SET(N, website_ids))giftcard.website_idscalar kept nullable for back-compat (currency derivation, FK tocore_website); flagged for removal in a future majorSchema migration (1.0.0 → 1.1.0)
sql/giftcard_setup/upgrade-1.0.0-1.1.0.phpbackfills the junction permissively: each existing card is associated with every active website (website_id > 0). Preserves the most common pre-1.1.0 behaviour where a single-website assignment was administrative but apply-time validation was effectively store-agnostic. UsesINSERT ... SELECT ... LEFT JOIN ... WHERE ... IS NULL(portable across MySQL/PostgreSQL/SQLite, naturally idempotent on re-run). Empty source table on fresh installs short-circuits to zero inserts.Admin edit-page transaction-history tab
Admins tracing where a card's balance went currently have to leave the card to scan the global
Sales → Gift Card Historypage, then filter or remember thegiftcard_id. New "Transaction History" tab next to the General form, wired via the canonicalMage_Adminhtml_Block_Cms_Page_Editlayout-XML pattern (<reference name="left">with<action method="addTab">). Card-scoped grid, read-only, AJAX paging routed throughhistoryGridActionon the existing admin controller. Tab is hidden on the create form.Customer "Check Gift Card Balance" page
New page under My Account where a logged-in customer can look up the remaining balance + expiry on a gift card by code. Form lives in
base/default(pure stock theme classes:.box-account,.form-list,.buttons-set, etc.). One-shot session storage of the lookup result so back/forward navigation doesn't re-display the previous customer's check on a shared device. Errors stay opaque —"not found","expired","disabled","no website membership"all collapse to the same"We could not find an active gift card for that code on this store."so the endpoint can't be used to enumerate code existence.Rate limiting: per-customer via the shared
Mage\Core\Helper\Data::rateLimiterBy()factory (scope key =customer_id), 10 failed lookups per hour. "Check up front,hit()on failure only" mirrors theMage_Sales_Helper_Guestpattern so a customer holding several legitimate cards isn't penalised for genuine lookups. Scope is intentionally just this page; the existing checkoutapplyGiftcardToCartpath is a separate concern.Polish
balanceandinitial_balancecolumns remaindecimal(12,4)for arithmetic precision; trailing zeros were leaking to the input as30.0000)data-turbo="false"on the balance form so Hotwire-Turbo installs get the standard browser POST→redirect→GET flow and don't accumulate flash messagesVerification
composer lint:cs-fixer,lint:phpstan,lint:rector— all clean on the 14 PHP files in the diffCommits