Skip to content

ARCH-001 Phase 21: EmailAccount controller/service consolidation (Admin/Store) - #820

Merged
KrzysztofPajak merged 5 commits into
developfrom
arch001/phase21-emailaccount-consolidation
Sep 9, 2026
Merged

ARCH-001 Phase 21: EmailAccount controller/service consolidation (Admin/Store)#820
KrzysztofPajak merged 5 commits into
developfrom
arch001/phase21-emailaccount-consolidation

Conversation

@KrzysztofPajak

Copy link
Copy Markdown
Member

Type: feature

Issue

Grand.Web.Admin and Grand.Web.Store each carried their own EmailAccountController (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. IEmailAccountViewModelService was already shared between the two hosts, so this phase was controller + view consolidation only.

Solution

  • New bespoke StoreEmailAccountDataScope (Grand.Web.AdminShared/Services): EmailAccount is a plain BaseEntity with a flat StoreId string (empty = global), not IStoreLinkEntity, so ownership is a single exact-match comparison — simpler than the list-of-stores shape used elsewhere in this initiative. No loose/strict CanView split needed (the persistence layer's GetAllEmailAccounts(storeId) already filters exactly). Admin reuses the existing generic GlobalAdminDataScope<EmailAccount> unchanged.
  • New RoutedEmailAccountDataScope: resolves the correct per-host scope at request time from the route's area value, fail-closed on anything unrecognized (including missing) — required because Grand.Web (the combined host) loads Admin and Store controllers into one DI container.
  • New BaseEmailAccountController (Grand.Web.AdminShared/Controllers) unifies Create/Edit/SendTestEmail/Delete. List (GET+POST) and the Admin-only MarkAsDefaultEmail (writes the global EmailAccountSettings.DefaultEmailAccountId setting — no per-store equivalent exists or is implied) stay on thin per-host subclasses.
  • Views: Create.cshtml, Edit.cshtml, Partials/CreateOrUpdate.cshtml moved into Grand.Web.AdminShared/Views/AdminShared/EmailAccount/..., branching on the request's route area for the few genuine per-host differences (form area, icon, the StoreId field rendering as a <select> for Admin vs. a hidden input for Store). List.cshtml stays a genuine per-host file on both hosts (Admin's grid has an extra MarkAsDefaultEmail column/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

  1. Delete's exception handling widened. Admin's original caught Exception (broad); Store's caught only GrandException (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.
  2. SendTestEmail's empty-address error message on Store. Store's original threw a GrandException with a lookup for a translation key (Admin.Configuration.EmailAccounts.EnterTestEmail) that does not exist anywhere in this repo's resources — ITranslationService.GetResource falls back to returning the lowercased key literal when a resource is missing, so store managers were seeing admin.configuration.emailaccounts.entertestemail as 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/Edit GET actions now unconditionally populate model.AvailableStores (every store's id/name), matching the shared PrepareAvailableStores call both hosts now go through. It is not rendered anywhere in Store's HTML (Store's branch emits only a hidden StoreId input, no picker), so there's no current page leak — but the model is passed to the 3 new store_email_account_details_* widget zones via additional-data="Model", so a future third-party Store widget reading Model.AvailableStores could 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.
  • New coverage: StoreEmailAccountDataScopeTests, RoutedEmailAccountDataScopeTests, BaseEmailAccountControllerTests (cross-store denial on Edit/SendTestEmail/Delete, conditional StoreId forcing on Create/Edit for Store but not Admin, the widened Delete catch), EmailAccountControllerAttributeTests on 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:

  1. Logged in as a Store user, created an email account — confirmed StoreId is forced server-side to the caller's own store regardless of anything client-submitted.
  2. Logged in as a different Store user — confirmed their own List shows zero accounts (exact-store filtering), confirmed direct-URL Edit on the first user's account redirects to List with zero mutation, confirmed a crafted Delete POST (real antiforgery token from the denied user's own session) is rejected with zero mutation, confirmed a crafted SendTestEmail POST is likewise rejected.
  3. Logged in as Admin — confirmed List shows every store's accounts (global scope), confirmed Edit renders the StoreId picker and the Admin-specific icon correctly, confirmed MarkAsDefaultEmail writes through end-to-end (icon indicator flips and persists across a fresh page load).
  4. All synthetic test data removed afterward; verified via a direct read-only database check that the dataset was restored to its exact pre-test state.

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings September 9, 2026 14:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 BaseEmailAccountController plus StoreEmailAccountDataScope/RoutedEmailAccountDataScope to unify behavior while enforcing Store isolation.
  • Moves Create/Edit + shared partials into Grand.Web.AdminShared and 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.

Comment thread src/Web/Grand.Web.AdminShared/Controllers/BaseEmailAccountController.cs Dismissed
Comment thread src/Web/Grand.Web.AdminShared/Controllers/BaseEmailAccountController.cs Dismissed
…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.
@sonarqubecloud

sonarqubecloud Bot commented Sep 9, 2026

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
75.0% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@KrzysztofPajak
KrzysztofPajak merged commit e76485a into develop Sep 9, 2026
7 of 8 checks passed
@KrzysztofPajak
KrzysztofPajak deleted the arch001/phase21-emailaccount-consolidation branch September 9, 2026 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants