ARCH-001 Phase 23: Shipping sub-entities (Warehouse/ShippingMethod/DeliveryDate/PickupPoint) consolidation - #824
Merged
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session
…over Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DjfBG3opo33qNHQYVbgH9Q
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DjfBG3opo33qNHQYVbgH9Q
…e cutover Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DjfBG3opo33qNHQYVbgH9Q
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DjfBG3opo33qNHQYVbgH9Q
…cutover Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DjfBG3opo33qNHQYVbgH9Q
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DjfBG3opo33qNHQYVbgH9Q
…utover
- Add BasePickupPointController in Grand.Web.AdminShared consuming
IAdminDataScope<PickupPoint> (StorePickupPointDataScope /
RoutedPickupPointDataScope from Task 7), with the address-preparation
loop inlined independently (matching Admin's original two-copy shape;
Store's shared PrepareAddressModel helper was an incidental artifact,
not a scope boundary).
- Add concrete PickupPointController for Admin and Store, preserving
existing routes via [Route("[area]/Shipping/[action]")].
- Trim both hosts' ShippingController.cs down to Providers/Settings/
Restrictions only, removing PickupPoint/Warehouse service dependencies
no longer used by this file.
- Fix pre-existing Grand.Web.Store.Tests ShippingControllerTests
constructor call to match the trimmed ShippingController signature.
- Add attribute/routing tests for both hosts' PickupPointController and
BasePickupPointControllerTests covering global/store scope plus the
store-scoped AvailableWarehouses dropdown restriction.
This is the fourth and final entity split of ARCH-001 Phase 23, completing
the ShippingController Admin/Store consolidation.
…controllers
Task 9's live smoke test found Critical: all 16 GET view-returning actions
across the 8 new concrete controllers (Warehouse/ShippingMethod/DeliveryDate/
PickupPoint x Admin/Store) threw InvalidOperationException 'view not found'.
Root cause: [Route("[area]/Shipping/[action]")] only affects URL matching;
Razor's view-location convention derives the view-search folder from
ControllerModel.ControllerName (class-name-derived), not the route template,
so e.g. WarehouseController searched Views/Warehouse/... instead of the
actual, untouched Views/Shipping/... folder.
Fix: new [SharedViewFolder(string)] attribute + IControllerModelConvention
in Grand.Web.Common (which Grand.Web.AdminShared already depends on, never
the reverse), registered once in the shared AddGrandMvc. Applied once per
abstract Base{Entity}Controller (inherited, so both hosts' concrete
subclasses are covered without repetition).
Verified: dotnet build 0 errors; Grand.Web.Admin.Tests 1411/1411 and
Grand.Web.Store.Tests 136/136 pass; live Kestrel run with authenticated
admin session confirms all 8 previously-broken URLs plus 2 regression-check
URLs (Providers/Settings) now return 200.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DjfBG3opo33qNHQYVbgH9Q
- Add regression coverage for SharedViewFolderAttribute: reflection tests
asserting the 4 abstract base controllers carry [SharedViewFolder("Shipping")],
and unit tests for SharedViewFolderControllerNameConvention.Apply. Adds a
test-only project reference from Grand.Web.Common.Tests to Grand.Web.AdminShared.
- Remove dead ILanguageService from Admin and Store ShippingController (moved
out by earlier tasks, never cleaned up); update Grand.Web.Store.Tests
ShippingControllerTests constructor call to match.
- Remove stale Grand.Web.AdminShared.Extensions.Mapping / Grand.Web.Common.DataSource
usings from Admin and Store WarehouseController.
- Top up StoreShippingMethodDataScopeTests to the standard 5-case shape
(HasAccess_MismatchedStoreId_False, HasAccess_NullEntity_False).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DjfBG3opo33qNHQYVbgH9Q
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| var result = await controller.EditDeliveryDate("dd-2") as RedirectToActionResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("DeliveryDates", result.ActionName); |
|
|
||
| var model = result!.Model as DeliveryDateModel; | ||
| Assert.IsNotNull(model); | ||
| Assert.AreEqual("#000000", model.ColorSquaresRgb); |
|
|
||
| var model = result!.Model as DeliveryDateModel; | ||
| Assert.IsNotNull(model); | ||
| Assert.AreEqual("#000000", model.ColorSquaresRgb); |
|
|
||
| var model = result!.Model as DeliveryDateModel; | ||
| Assert.IsNotNull(model); | ||
| Assert.AreEqual("#ff0000", model.ColorSquaresRgb); |
| var result = await controller.EditPickupPoint("pp-2") as RedirectToActionResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("PickupPoints", result.ActionName); |
| var result = await controller.EditMethod("sm-2") as RedirectToActionResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("Methods", result.ActionName); |
| var result = await controller.EditWarehouse("wh-2") as RedirectToActionResult; | ||
|
|
||
| Assert.IsNotNull(result); | ||
| Assert.AreEqual("Warehouses", 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.
Type: refactor (architecture consolidation, ARCH-001)
Issue
Grand.Web.AdminandGrand.Web.Storeeach carried a duplicateShippingControllerhandlingShippingMethod/Warehouse/DeliveryDate/PickupPointCRUD independently — the same class ofduplication ARCH-001 has been eliminating across ~22 other entities (Product, Category, Order,
TaxCategory, EmailAccount, etc.). A bug fix or security patch to one host's shipping-entity logic
had no mechanism to reach the other; Store's tenant-isolation checks and Admin's global-scope
behavior had already drifted independently.
Solution
Consolidated the four entity-CRUD regions (
Methods/DeliveryDates/Warehouses/PickupPoints)into shared
Grand.Web.AdminSharedbase controllers, each backed by a per-entityIAdminDataScope<TEntity>scope class (Store{Entity}DataScope+Routed{Entity}DataScope,reusing
GlobalAdminDataScope<TEntity>for Admin) — the same pattern used by every prior ARCH-001phase.
Providers/Settings/Restrictionsregions are untouched and remain duplicated per host(not entity-shaped, out of scope).
New to this phase: since a C# class can only have one base class and this phase needed four
different base controllers, each host's single
ShippingControllerwas split into 5 concretecontrollers (the original, shrunk to
Providers/Settings/Restrictions, plus one thin controllerper entity). Each new controller carries
[Route("[area]/Shipping/[action]")]so every URL staysbyte-identical to today (
/Admin/Shipping/Warehouses, etc.) despite the new class names — theexisting, untouched
.cshtmlviews'Url.Action/asp-action/asp-controllerreferences keepworking unmodified. No view files were changed anywhere in this phase.
A real Critical bug was found and fixed mid-branch, via the live smoke test, not any of the 8
per-task reviews or 1547 unit tests: Razor's view-engine resolves the view-search folder from a
controller's C#-class-derived name, independent of the
[Route]template — soWarehouseControllersearched
Views/Warehouse/...instead of the actualViews/Shipping/..., and every GETview-returning action across all 8 new controllers threw
view not found. Fixed with a small,reusable mechanism: a
[SharedViewFolder("Shipping")]attribute (inGrand.Web.Common, respectingthis codebase's existing dependency direction —
Grand.Web.AdminShareddepends onGrand.Web.Common, never the reverse) applied once per abstract base controller, read by a newIControllerModelConventionregistered in the single sharedAddGrandMvcextension. Verified liveagainst a real running instance (10/10 URLs returning 200, including the untouched
Providers/Settingsregression check) before continuing. The final whole-branch reviewindependently found this fix also closes an unrelated fail-open hazard:
AuthorizeMenuFilterresolves the admin sitemap by the request's
RouteData["controller"]value, so without the renameevery split entity action would have silently skipped menu-permission enforcement — a real benefit
nobody set out to fix.
Disclosed behavior notes (all verified non-breaking, all Minor)
DeleteWarehousesuccess-message resource key was changed from a lowercase-cased key(
Admin.Configuration.Shipping.warehouses.Deleted) to the correct PascalCase form(
...Warehouses.Deleted). Correction to an earlier internal note: this is a cosmetic no-op,not a user-visible fix —
TranslationService.GetResourcelowercases the lookup key beforematching, so both spellings already resolved to the identical resource string. No behavior
changed.
DeleteDeliveryDatedropped a dead, unreachableif (ModelState.IsValid)wrapper (theaction binds only a route
string id, soModelStatewas always valid; theelsebranch neverran). Behavior-neutral, now matches all three sibling entities' shape.
GetAllShippingMethods(storeId)returns own-store-or-global;GetAllWarehouses/GetAllDeliveryDates/GetAllPickupPoints(storeId)return own-store-only. This is existing,pre-consolidation, per-entity service behavior, preserved exactly — confirmed live (store1's
ShippingMethod grid showed its own method plus 3 seeded global ones; its Warehouse/DeliveryDate
grids showed only its own).
Testing
Automated:
dotnet build GrandNode.sln(0 errors);Grand.Web.Admin.Tests1413/1413,Grand.Web.Store.Tests136/136,Grand.Web.Common.Tests35/35 — all per-project runs, all green.Includes new regression coverage for the
[SharedViewFolder]mechanism itself (attribute-presencereflection tests + a direct
IControllerModelConvention.Applyunit test), added after the finalwhole-branch review flagged that the one Critical bug this branch produced had zero automated
defense.
Live, end-to-end, against a real Kestrel instance + a live MongoDB dev database:
referencing the new warehouse) — all 4 creates succeeded.
Edit(GET+POST) andDeleteon all 4 of store1's entities via craftedantiforgery-tokened requests — 9/9 denied (redirected to the entity's list, zero mutation,
re-verified by reading store1's data back unchanged).
Providers/Settings/Restrictions— the three untouched regions — all still return 200 onboth hosts (confirms the controller split didn't disturb DI resolution or routing there).
PickupPoint'sAvailableWarehousesdropdown on Store's create form correctly showed onlystore1's own warehouse, matching the unit-tested scoping.
asp-controller="Shipping"tag helpers and Kendo gridrow templates, across every tab, both hosts) resolved correctly post-fix — closes the one open
concern the view-folder-fix's own reviewer flagged as unexercised by that fix's narrower check.
All synthetic test data was deleted afterward and re-verified empty.
Review
9 task-level reviews (spec + quality, all Approved, zero Critical/Important beyond the two
plan-mandated/disclosed items above) + 1 final whole-branch review (opus) — verdict "Ready to
merge: With fixes" — + 1 fix wave (regression tests for the view-folder fix, removed a field left
dead by two earlier tasks' incremental trims, minor using cleanup, test coverage top-up) + 1 scoped
re-review confirming all findings addressed with no new breakage.
🤖 Generated with Claude Code