ARCH-001 Phase 21: EmailAccount controller/service consolidation (Admin/Store) - #820
Conversation
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
…tribute regression tests Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DjfBG3opo33qNHQYVbgH9Q
…dminShared, add widget zones
There was a problem hiding this comment.
🟡 Changes recommended
BaseEmailAccountController currently populates/returns AvailableStores in Store scope (unnecessary exposure via widget zones) and does not repopulate it on Create/Edit validation failures for Admin views that require the store selector.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR continues ARCH-001 by consolidating duplicated Admin and Store EmailAccount Create/Edit/SendTestEmail/Delete controller logic into a shared base controller and shared views, while keeping List and Admin-only “Mark as default” behavior host-specific.
Changes:
- Introduces
BaseEmailAccountControllerplusStoreEmailAccountDataScope/RoutedEmailAccountDataScopeto unify behavior while enforcing Store isolation. - Moves Create/Edit + shared partials into
Grand.Web.AdminSharedand adds host-specific widget-zone satellite partials for extension points. - Updates Admin/Store controllers to thin subclasses and adds targeted unit tests for routing/attributes, data scoping, and shared controller behaviors.
File summaries
| File | Description |
|---|---|
| src/Web/Grand.Web.Store/Controllers/EmailAccountController.cs | Converts Store controller into thin subclass; keeps List implementation store-scoped via IAdminDataScope<EmailAccount>. |
| src/Web/Grand.Web.Store/Areas/Store/Views/EmailAccount/Partials/WidgetZone.DetailsTop.cshtml | Adds Store-specific widget-zone partial for shared views. |
| src/Web/Grand.Web.Store/Areas/Store/Views/EmailAccount/Partials/WidgetZone.DetailsButtons.cshtml | Adds Store-specific widget-zone partial for shared views. |
| src/Web/Grand.Web.Store/Areas/Store/Views/EmailAccount/Partials/WidgetZone.DetailsBottom.cshtml | Adds Store-specific widget-zone partial for shared views. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/EmailAccount/Partials/CreateOrUpdate.cshtml | Adds area branching (Admin vs Store) and wires shared widget-zone partials. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/EmailAccount/Edit.cshtml | Uses route area for form posts + host-specific icon and widget-zone button partial. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/EmailAccount/Create.cshtml | Uses route area for form posts + host-specific icon and widget-zone button partial. |
| src/Web/Grand.Web.AdminShared/Startup/StartupApplication.cs | Registers IAdminDataScope<EmailAccount> routing + concrete scopes. |
| src/Web/Grand.Web.AdminShared/Services/StoreEmailAccountDataScope.cs | Adds Store-specific exact-match StoreId scoping implementation. |
| src/Web/Grand.Web.AdminShared/Services/RoutedEmailAccountDataScope.cs | Adds route-area-based scope resolver (fail-closed on missing/unrecognized area). |
| src/Web/Grand.Web.AdminShared/Controllers/BaseEmailAccountController.cs | Adds shared Create/Edit/SendTestEmail/Delete actions. |
| src/Web/Grand.Web.Admin/Controllers/EmailAccountController.cs | Converts Admin controller into thin subclass; keeps List + MarkAsDefaultEmail. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/EmailAccount/Partials/WidgetZone.DetailsTop.cshtml | Adds Admin-specific widget-zone partial for shared views. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/EmailAccount/Partials/WidgetZone.DetailsButtons.cshtml | Adds Admin-specific widget-zone partial for shared views. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/EmailAccount/Partials/WidgetZone.DetailsBottom.cshtml | Adds Admin-specific widget-zone partial for shared views. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/EmailAccount/Partials/CreateOrUpdate.cshtml | Removes host-specific partial in favor of shared AdminShared view. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/EmailAccount/Edit.cshtml | Removes host-specific Edit view in favor of shared AdminShared view. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/EmailAccount/Create.cshtml | Removes host-specific Create view in favor of shared AdminShared view. |
| src/Tests/Grand.Web.Store.Tests/Controllers/EmailAccountControllerAttributeTests.cs | Adds Store controller attribute/regression tests (area/auth/menu). |
| src/Tests/Grand.Web.Admin.Tests/Controllers/StoreEmailAccountDataScopeTests.cs | Adds unit tests for StoreEmailAccountDataScope ownership rules. |
| src/Tests/Grand.Web.Admin.Tests/Controllers/RoutedEmailAccountDataScopeTests.cs | Adds unit tests for route-area-based scope resolution and fail-closed behavior. |
| src/Tests/Grand.Web.Admin.Tests/Controllers/EmailAccountControllerAttributeTests.cs | Adds Admin controller attribute/regression tests (area/auth/menu). |
| src/Tests/Grand.Web.Admin.Tests/Controllers/BaseEmailAccountControllerTests.cs | Adds shared base controller behavior tests (store forcing, cross-store denial, widened exception catch). |
Review details
Suppressed comments (2)
src/Web/Grand.Web.AdminShared/Controllers/BaseEmailAccountController.cs:96
- On Edit POST validation failure, the Admin view needs Model.AvailableStores to render the StoreId , but the action returns the posted model without repopulating the list. Re-populate the list when ShowStoreSelector=true; otherwise clear it to avoid exposing stores in Store scope. //If we got this far, something failed, redisplay form return View(model); src/Web/Grand.Web.AdminShared/Controllers/BaseEmailAccountController.cs:71 Edit GET always populates AvailableStores with every store. Store scope never shows a picker (ShowStoreSelector=false), and the model can be passed to Store widget zones, so this is unnecessary exposure and work. Only populate when the scope shows the selector; otherwise clear the list. var model = emailAccount.ToModel(); await emailAccountViewModelService.PrepareAvailableStores(model); return View(model); Files reviewed: 23/23 changed files Comments generated: 2 Review effort level: Lite 💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ount controller Code review flagged that Create/Edit GET now unconditionally populate model.AvailableStores (every store's id/name) via the shared PrepareEmailAccountModel/PrepareAvailableStores calls, even though Store's own view never renders it - the model is still passed to the new store_email_account_details_* widget zones via additional-data="Model", so a future Store widget reading Model.AvailableStores could leak other stores' names/ids from a store-scoped screen. Fix in the controller (not the view): clear AvailableStores when scope.ShowStoreSelector is false, same gate ProductViewModelService already uses for the same reason. No-op for Admin (ShowStoreSelector is true there). Adds regression coverage: Create/Edit GET keep AvailableStores under Admin scope, clear it under Store scope.
|




Type: feature
Issue
Grand.Web.AdminandGrand.Web.Storeeach carried their ownEmailAccountController(196 / 171 lines) duplicating the same Create/Edit/SendTestEmail/Delete logic — the same class of duplication ARCH-001 has been consolidating across the codebase (Product, Category, Collection, Order, Shipment, PaymentTransaction, MerchandiseReturn, Discount, Brand, Blog, Page, News, GiftVoucher, ProductReview, MessageTemplate, Customer — Phases 1-20). No Vendor screen exists for this entity.IEmailAccountViewModelServicewas already shared between the two hosts, so this phase was controller + view consolidation only.Solution
StoreEmailAccountDataScope(Grand.Web.AdminShared/Services):EmailAccountis a plainBaseEntitywith a flatStoreIdstring (empty = global), notIStoreLinkEntity, so ownership is a single exact-match comparison — simpler than the list-of-stores shape used elsewhere in this initiative. No loose/strictCanViewsplit needed (the persistence layer'sGetAllEmailAccounts(storeId)already filters exactly). Admin reuses the existing genericGlobalAdminDataScope<EmailAccount>unchanged.RoutedEmailAccountDataScope: resolves the correct per-host scope at request time from the route'sareavalue, fail-closed on anything unrecognized (including missing) — required becauseGrand.Web(the combined host) loads Admin and Store controllers into one DI container.BaseEmailAccountController(Grand.Web.AdminShared/Controllers) unifiesCreate/Edit/SendTestEmail/Delete.List(GET+POST) and the Admin-onlyMarkAsDefaultEmail(writes the globalEmailAccountSettings.DefaultEmailAccountIdsetting — no per-store equivalent exists or is implied) stay on thin per-host subclasses.Create.cshtml,Edit.cshtml,Partials/CreateOrUpdate.cshtmlmoved intoGrand.Web.AdminShared/Views/AdminShared/EmailAccount/..., branching on the request's route area for the few genuine per-host differences (form area, icon, theStoreIdfield rendering as a<select>for Admin vs. a hidden input for Store).List.cshtmlstays a genuine per-host file on both hosts (Admin's grid has an extraMarkAsDefaultEmailcolumn/Kendo config Store's plain list doesn't need) — untouched by this PR. 3 widget zones (email_account_details_top/bottom/buttons) extracted into host-specific satellite partials, matching the established pattern; Store gains 3 brand-new real extension points (store_email_account_details_*— its original views had no widget-zone calls at all, not even a dead one, so this is a new capability, not a bugfix).Two behavior changes, both deliberate, both benign
Delete's exception handling widened. Admin's original caughtException(broad); Store's caught onlyGrandException(narrow — anything else 500'd). Unified onto Admin's broader form: strictly safer, only ever turns a previously-unhandled 500 into a shown error message.SendTestEmail's empty-address error message on Store. Store's original threw aGrandExceptionwith a lookup for a translation key (Admin.Configuration.EmailAccounts.EnterTestEmail) that does not exist anywhere in this repo's resources —ITranslationService.GetResourcefalls back to returning the lowercased key literal when a resource is missing, so store managers were seeingadmin.configuration.emailaccounts.entertestemailas the error text. This PR unifies onto Admin's hardcoded English string ("Enter test email address"), so Store managers now see readable text instead. This was not caught during design (the design spec incorrectly asserted this code path was already identical between hosts) — found during final review.One disclosed, non-blocking latent note
Store's
Create/EditGET actions now unconditionally populatemodel.AvailableStores(every store's id/name), matching the sharedPrepareAvailableStorescall both hosts now go through. It is not rendered anywhere in Store's HTML (Store's branch emits only a hiddenStoreIdinput, no picker), so there's no current page leak — but the model is passed to the 3 newstore_email_account_details_*widget zones viaadditional-data="Model", so a future third-party Store widget readingModel.AvailableStorescould surface other stores' names/ids from a store-scoped screen. Zero current subscribers to these brand-new zones. Flagging for whoever writes the first Store widget against them.Breaking changes
None. The two behavior changes above are both one-directional improvements (broader exception handling, readable error text) with no reachable regression path.
Testing
Automated:
dotnet build GrandNode.sln— 0 errors, 0 warnings.dotnet test src/Tests/Grand.Web.Admin.Tests/Grand.Web.Admin.Tests.csproj— 1237/1237 passing.dotnet test src/Tests/Grand.Web.Store.Tests/Grand.Web.Store.Tests.csproj— 109/109 passing.StoreEmailAccountDataScopeTests,RoutedEmailAccountDataScopeTests,BaseEmailAccountControllerTests(cross-store denial on Edit/SendTestEmail/Delete, conditionalStoreIdforcing on Create/Edit for Store but not Admin, the widenedDeletecatch),EmailAccountControllerAttributeTestson both hosts (the[Area]/[Authorize*]/[AuthorizeMenu]routing-attribute regression class that has caused real 404s in earlier ARCH-001 phases despite clean builds).Live, against a real Kestrel instance + MongoDB dev database, driven via real browser sessions:
StoreIdis forced server-side to the caller's own store regardless of anything client-submitted.Listshows zero accounts (exact-store filtering), confirmed direct-URLEditon the first user's account redirects toListwith zero mutation, confirmed a craftedDeletePOST (real antiforgery token from the denied user's own session) is rejected with zero mutation, confirmed a craftedSendTestEmailPOST is likewise rejected.Listshows every store's accounts (global scope), confirmedEditrenders theStoreIdpicker and the Admin-specific icon correctly, confirmedMarkAsDefaultEmailwrites through end-to-end (icon indicator flips and persists across a fresh page load).🤖 Generated with Claude Code