ARCH-001 Phase 17: GiftVoucher controller/service consolidation (Admin/Store) - #812
Merged
KrzysztofPajak merged 7 commits intoSep 3, 2026
Merged
Conversation
…t/UsageHistoryList Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… satellites - Create shared List, Create, Edit views in Grand.Web.AdminShared - Create shared partials: CreateOrUpdate, CreateOrUpdate.TabInfo, CreateOrUpdate.TabHistory - Create widget-zone satellite partials for Admin and Store areas - Delete duplicate views from Grand.Web.Admin and Grand.Web.Store - Admin uses vc:admin-widget tag helper, Store uses vc:store-widget - Consolidates ~600 lines of identical view code into shared AdminShared views Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…display BaseGiftVoucherController.Edit(POST) was not calling PrepareGiftVoucherModel on the invalid-model branch, causing AvailableStores to come back empty. This made the shared view''s Model.AvailableStores.Count <= 1 condition incorrectly evaluate to true for Admin, hiding the store dropdown on validation errors. Applied the same pattern that Create(POST) already uses: call PrepareGiftVoucherModel to repopulate AvailableStores before returning the view. Added test: EditPost_InvalidModel_CallsPrepareGiftVoucherModelToRepopulateAvailableStores verifies that PrepareGiftVoucherModel is called and AvailableStores is populated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…toryList scope, EditWarningCheck condition Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The consolidation is internally consistent (controller/view/scoping wiring aligns with existing AdminShared patterns) and is backed by targeted regression/unit tests covering the key access-scope and permission behaviors.
Pull request overview
This PR continues ARCH-001 consolidation by removing duplicated Admin/Store GiftVoucherController logic and centralizing it in Grand.Web.AdminShared, while introducing a per-host IAdminDataScope<GiftVoucher> implementation to unify tenant isolation and explicitly support “global” (empty/null StoreId) gift vouchers as viewable-but-not-editable in Store.
Changes:
- Introduced
BaseGiftVoucherControllerinGrand.Web.AdminSharedand reduced Admin/Store controllers to thin subclasses that mainly provide host-specific attributes and Store’sEditWarningCheck. - Added
StoreGiftVoucherDataScope+RoutedGiftVoucherDataScopeand registered them inAdminSharedstartup to enforce consistent per-area scoping in the combined host. - Moved/unified GiftVoucher views into
AdminSharedand added per-host widget-zone “satellite” partials; added regression/unit tests covering scoping, attributes, and controller behaviors.
File summaries
| File | Description |
|---|---|
| src/Web/Grand.Web/App_Data/Resources/Upgrade/en_240.xml | Adds a new localization resource for Store warning banner text. |
| src/Web/Grand.Web.Store/Controllers/GiftVoucherController.cs | Converts Store controller into a thin subclass with Store-specific attributes + EditWarningCheck. |
| src/Web/Grand.Web.Store/Areas/Store/Views/GiftVoucher/Partials/WidgetZone.Tabs.cshtml | Store widget-zone satellite for GiftVoucher tabs. |
| src/Web/Grand.Web.Store/Areas/Store/Views/GiftVoucher/Partials/WidgetZone.ListButtons.cshtml | Store widget-zone satellite for list action buttons. |
| src/Web/Grand.Web.Store/Areas/Store/Views/GiftVoucher/Partials/WidgetZone.InfoTop.cshtml | Store widget-zone satellite for info tab top injection point. |
| src/Web/Grand.Web.Store/Areas/Store/Views/GiftVoucher/Partials/WidgetZone.InfoBottom.cshtml | Store widget-zone satellite for info tab bottom injection point. |
| src/Web/Grand.Web.Store/Areas/Store/Views/GiftVoucher/Partials/WidgetZone.HistoryTop.cshtml | Store widget-zone satellite for history tab top injection point. |
| src/Web/Grand.Web.Store/Areas/Store/Views/GiftVoucher/Partials/WidgetZone.HistoryBottom.cshtml | Store widget-zone satellite for history tab bottom injection point. |
| src/Web/Grand.Web.Store/Areas/Store/Views/GiftVoucher/Partials/WidgetZone.DetailsButtons.cshtml | Store widget-zone satellite for create/edit page action buttons area. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/GiftVoucher/Partials/CreateOrUpdate.TabInfo.cshtml | Unifies tab content and routes links/actions via current area; adds widget-zone hook points and conditional store selector. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/GiftVoucher/Partials/CreateOrUpdate.TabHistory.cshtml | Unifies history tab and routes AJAX/read links via current area; adds widget-zone hook points. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/GiftVoucher/Partials/CreateOrUpdate.cshtml | Unifies create/update layout; conditionally posts StoreId; adds tab widget-zone hook. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/GiftVoucher/List.cshtml | Unifies list view and routes list endpoints via current area; adds list widget-zone hook. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/GiftVoucher/Edit.cshtml | Unifies edit view and routes form post via current area; adds action-buttons widget-zone hook. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/GiftVoucher/Create.cshtml | Unifies create view and routes form post via current area; adds action-buttons widget-zone hook. |
| src/Web/Grand.Web.AdminShared/Startup/StartupApplication.cs | Registers GiftVoucher data scopes and routed scope resolver in DI. |
| src/Web/Grand.Web.AdminShared/Services/StoreGiftVoucherDataScope.cs | Implements Store’s bespoke GiftVoucher scoping including a looser CanView for global vouchers. |
| src/Web/Grand.Web.AdminShared/Services/RoutedGiftVoucherDataScope.cs | Resolves correct scope by request area (Admin vs Store) and fails closed for unknown/missing areas. |
| src/Web/Grand.Web.AdminShared/Controllers/BaseGiftVoucherController.cs | New shared controller implementing all GiftVoucher actions with unified scoping/permission logic. |
| src/Web/Grand.Web.Admin/Controllers/GiftVoucherController.cs | Converts Admin controller into a thin subclass with Admin-specific attributes. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/GiftVoucher/Partials/WidgetZone.Tabs.cshtml | Admin widget-zone satellite for GiftVoucher tabs. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/GiftVoucher/Partials/WidgetZone.ListButtons.cshtml | Admin widget-zone satellite for list action buttons. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/GiftVoucher/Partials/WidgetZone.InfoTop.cshtml | Admin widget-zone satellite for info tab top injection point. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/GiftVoucher/Partials/WidgetZone.InfoBottom.cshtml | Admin widget-zone satellite for info tab bottom injection point. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/GiftVoucher/Partials/WidgetZone.HistoryTop.cshtml | Admin widget-zone satellite for history tab top injection point. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/GiftVoucher/Partials/WidgetZone.HistoryBottom.cshtml | Admin widget-zone satellite for history tab bottom injection point. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/GiftVoucher/Partials/WidgetZone.DetailsButtons.cshtml | Admin widget-zone satellite for create/edit page action buttons area. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/GiftVoucher/Partials/CreateOrUpdate.TabInfo.cshtml | Removes now-duplicated Admin-only tab partial (replaced by AdminShared view). |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/GiftVoucher/Partials/CreateOrUpdate.TabHistory.cshtml | Removes now-duplicated Admin-only history partial (replaced by AdminShared view). |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/GiftVoucher/Partials/CreateOrUpdate.cshtml | Removes now-duplicated Admin-only wrapper partial (replaced by AdminShared view). |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/GiftVoucher/List.cshtml | Removes now-duplicated Admin-only list view (replaced by AdminShared view). |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/GiftVoucher/Edit.cshtml | Removes now-duplicated Admin-only edit view (replaced by AdminShared view). |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/GiftVoucher/Create.cshtml | Removes now-duplicated Admin-only create view (replaced by AdminShared view). |
| src/Tests/Grand.Web.Store.Tests/Controllers/GiftVoucherControllerAttributeTests.cs | Adds Store controller attribute checks and EditWarningCheck behavioral tests via Edit(GET). |
| src/Tests/Grand.Web.Admin.Tests/Controllers/StoreGiftVoucherDataScopeTests.cs | Adds unit tests for Store GiftVoucher scope HasAccess/CanView and default store id. |
| src/Tests/Grand.Web.Admin.Tests/Controllers/RoutedGiftVoucherDataScopeTests.cs | Adds tests for correct per-area scope resolution and fail-closed behavior. |
| src/Tests/Grand.Web.Admin.Tests/Controllers/GiftVoucherControllerAttributeTests.cs | Adds Admin thin-controller attribute coverage and regression test for Create(POST) permission. |
| src/Tests/Grand.Web.Admin.Tests/Controllers/BaseGiftVoucherControllerTests.cs | Adds unit/regression tests for shared controller behaviors (scoping, redirects, invalid model-state flows, usage history gating). |
Review details
- Files reviewed: 38/38 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| var result = CreateController().List() as ViewResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreSame(listModel, result.Model); |
| var returnedModel = result?.Model as GiftVoucherModel; | ||
|
|
||
| Assert.IsNotNull(returnedModel); | ||
| Assert.AreEqual(2, returnedModel.AvailableStores.Count); |
| var returnedModel = result?.Model as GiftVoucherModel; | ||
|
|
||
| Assert.IsNotNull(returnedModel); | ||
| Assert.AreEqual("store-1", returnedModel.StoreId); |
| var result = await controller.Create(model, false) as RedirectToActionResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("List", result.ActionName); |
| var result = await CreateController().Edit("missing") as RedirectToActionResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("List", result.ActionName); |
| var result = await CreateController().Edit("gv-1") as RedirectToActionResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("List", result.ActionName); |
| var result = await CreateController().Edit("gv-1") as ViewResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreSame(model, result.Model); |
| var result = await CreateController().Edit(new GiftVoucherModel { Id = "gv-1" }, false) as RedirectToActionResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("Edit", result.ActionName); |
| var result = await CreateController().Edit(model, false) as RedirectToActionResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("List", result.ActionName); |
| var result = await CreateController().Delete(new GiftVoucherDeleteModel("gv-1")) as RedirectToActionResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("Edit", result.ActionName); |
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
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.
Resolves ARCH-001 Phase 17
Type: bugfix, refactor
Issue
Grand.Web.AdminandGrand.Web.Storeeach carried their ownGiftVoucherController(202 / 214lines, no Vendor equivalent) duplicating CRUD and access-control logic for
GiftVoucherinstead ofsharing it through
Grand.Web.AdminShared. Same class of drag and security-gap risk as every priorARCH-001 phase — investigating this entity's shape also surfaced a real "list is loose, edit is
strict, and nothing in between" gap:
GetGiftVoucherQueryHandlertreats an empty/nullStoreIdasvisible from every store's
List, but Store's original ownership check was a strict equality thatsilently denied
Editfor exactly those same global vouchers — the same shape this initiative hasrepeatedly turned into a warn-vs-deny split via
EditWarningCheck, just never applied here before.Solution
Consolidated both hosts behind a shared
BaseGiftVoucherControllerinGrand.Web.AdminShared,driven by a new bespoke
StoreGiftVoucherDataScope/RoutedGiftVoucherDataScope(
IAdminDataScope<GiftVoucher>). No Vendor work needed —Grand.Web.Vendorhas noGiftVoucherControllerat all.GiftVoucher : BaseEntityis a plain entity with a flatStoreIdstring, notIStoreLinkEntity(same shape family as
Order) —StoreGiftVoucherDataScopeis bespoke, mirroringStoreOrderDataScope, but (unlike Order) overridesCanViewto be looser thanHasAccess: aglobal (empty/null
StoreId) voucher is viewable everywhere, matching the query handler's owncross-store visibility rule.
BaseGiftVoucherControllerholds every action (List,Create,Edit,Delete,NotifyRecipient,GenerateCouponCode,UsageHistoryList) — full unification, no Store-onlyactions survive (unlike Page's
Copy/StorePagesList/GlobalPagesList).GiftVoucherControllers are thin subclasses. Store also keepsEditWarningCheck(same proven idiom as Category/Blog/Page/News, adapted for GiftVoucher's flat
StoreId).Grand.Web.AdminShared/Views/AdminShared/GiftVoucher/...,List.cshtmlunifiesfully (a single shared grid, like News). 7 widget zones extracted into real per-host satellite
partials — unlike every prior phase, Store's originals had zero
<vc:>calls at all toconvert (not even dead ones), so all 7 Store satellites are genuinely new additions, not
conversions.
Full design rationale:
docs/superpowers/specs/2026-09-03-arch001-giftvoucher-consolidation-design.md(spec) and
docs/superpowers/plans/2026-09-03-arch001-giftvoucher-consolidation.md(plan) — bothkept on disk only (gitignored), summarized here.
Disclosed behavior changes (intentional, not regressions)
Create(POST)now consistently requiresPermissionActionName.Createon both hosts. Admin'soriginal required
.EditonCreate(POST)while its ownCreate(GET)and both of Store'sCreateactions already required.Create— an internal inconsistency, fixed to the consistentvalue.
Editscreen read-only, with a warningbanner, instead of being silently redirected to
List. This closes a real, previously-invisiblegap:
Listalready showed these vouchers to every store (perGetGiftVoucherQueryHandler'scross-store visibility for empty-
StoreIdvouchers);Editjust always denied them outright.List.cshtml's row links are now unconditional for both hosts. Store's original guarded thelink behind
if(StoreId)specifically becauseEditwould otherwise redirect away for a globalvoucher — now that
Editopens it read-only, hiding the link was solving a problem that no longerexists.
Edit(POST)'s invalid-model redisplay now repopulatesAvailableStores/AvailableCurrencies(found and fixed mid-implementation, not part of the original design) — the original Admin
controller's invalid-model branch never re-prepared these lists, so a failed edit silently
degraded Admin's store dropdown into what looked like Store's single-store view. Fixed by mirroring
Create(POST)'s already-correct re-prepare call.Final whole-branch review
Dispatched on the most capable model per this initiative's process, independently re-deriving
tenant isolation from the merged code rather than trusting per-task review claims. Verdict:
"Ready to merge: With fixes" — 1 Critical, 2 Important, fixed in one round:
[AuthorizeMenu](present on theoriginal
BaseAdminControllervia inheritance, and on every other Admin thin subclass in thisinitiative) — and its own attribute test enshrined the bug with a factually incorrect comment
claiming Admin never had it. Fixed by restoring the attribute and inverting the test to assert its
presence.
UsageHistoryListwas gated on the looseCanView(matchingEdit's intentionalwidening), but a global voucher's usage-history rows can reference orders from any store that
redeemed it — so a Store user opening a global voucher's History tab could read another store's
order ids, order numbers, and redeemed amounts. Fixed by narrowing that one action's gate to the
strict
HasAccess: the History tab is unavailable for a global voucher even thoughEdititselfremains viewable, which is the narrowest fix that closes the leak without adding row-level
filtering to a rarely-used tab.
EditWarningCheckcondition (giftVoucher.StoreId != Scope.DefaultStoreId)was also true for another store's voucher — not just a genuinely global one — which meant probing
a nonexistent id produced no warning while probing a real other-store id did, a cross-tenant
existence oracle. Fixed by narrowing the condition to
string.IsNullOrEmpty(giftVoucher.StoreId).Scoped re-review confirmed all 5 fixed items (the 3 above plus 2 bundled minors: an unused
usingand two missing regression tests) ADDRESSED, no new breakage, independently re-run.
Breaking changes
None for storefront/API consumers. See "Disclosed behavior changes" above for the small set of
admin/store-panel behavior differences this consolidation surfaced or corrected.
Testing
Automated (all green):
dotnet build GrandNode.sln— 0 errors, 0 warnings attributable to this phase.dotnet test src/Tests/Grand.Web.Admin.Tests— full suite green (1040+ tests, incl. theHasAccess/CanViewtruth table, theEditWarningChecktruth table, theCreate(POST)permission regression test, and the
Deleteinvalid-ModelStateregression test).dotnet test src/Tests/Grand.Web.Store.Tests— full suite green (65+ tests, incl. real behavioralEditWarningChecktests driven through the actualEdit(GET)action, not just reflection).dotnet test src/Tests/Grand.Mapping.Tests— 234/234.Live smoke test (run directly against a real dev DB, not sandboxed — specifically targeting the
Critical/Important fixes from the final review):
Store2-exclusive voucher;
List/Createrender correctly for all three.store1:Listcorrectly shows the global + own-store vouchers, correctly excludes theother store's — confirmed row links are now live for both (the
List.cshtmlunconditional-linkchange).
EditWarningCheck: opening the global voucher'sEditshowed the warningbanner; opening the store's own voucher showed none; opening the other store's voucher
redirected to
Listwith no warning banner (proving the existence-oracle is closed — anonexistent id and another store's real id are now indistinguishable from the outside).
UsageHistoryList: clicking the global voucher's History tab produced aserver-side
ArgumentException(confirmed in the Kestrel log, not just the UI) — the tab iscorrectly denied. Positive control: the store's own voucher's History tab returned
200with agenuinely empty (never-redeemed) history, proving the tightened gate doesn't break the happy
path.
confirmed restored to its original empty
GiftVouchercollection.Disclosed, out of scope:
[AuthorizeMenu]'s restored enforcement (fix 1 above) could not beobserved end-to-end in this smoke test since
SecurityConfig.AuthorizeAdminMenuisfalsebydefault in this dev environment; verified structurally instead via the attribute regression test and
the final reviewer's direct comparison against every sibling Admin thin subclass in this initiative.
🤖 Generated with Claude Code