ARCH-001 Phase 22: TaxCategory controller/service consolidation (Admin/Store)

[KrzysztofPajak]

Resolves N/A (ARCH-001 initiative, no tracking issue)
Type: refactor

Issue

[[project_arch001_triple_admin_duplication]] (ARCH-001) — Grand.Web.Admin/Grand.Web.Store largely duplicate rather than share controller/service code. TaxController bundles three unrelated regions (Providers, Settings, Tax Categories); only TaxCategory is a real duplicated CRUD entity, with the same action set implemented independently in both hosts.

Solution

Consolidated only the Tax Categories region into a shared BaseTaxCategoryController (Grand.Web.AdminShared), following the same IAdminDataScope<TEntity> pattern used by 21 prior phases. TaxCategory : BaseEntity has a flat StoreId string (empty = global), not IStoreLinkEntity, so this reuses the bespoke-scope pattern already established for Order/EmailAccount (StoreTaxCategoryDataScope, mirroring StoreOrderDataScope) rather than the generic StoreAdminDataScope<TEntity>. Admin reuses the existing generic GlobalAdminDataScope<TaxCategory>. New RoutedTaxCategoryDataScope (2-branch, Admin/Store, fails closed — no Vendor screen exists for Tax).

Deliberately out of scope, untouched, still duplicated: Providers/MarkAsPrimaryProvider/Settings (GET+POST) — not entity-shaped, nothing for IAdminDataScope<TEntity> to scope. Both hosts' Categories() GET action and Categories.cshtml views also stay separate — the real duplication here was in controller/scope logic, not markup (Admin's view has a store-picker/column Store's doesn't).

Disclosed behavior changes

1. CategoryDelete not-found response unified. Admin's original code threw ArgumentException on a not-found id; Store's returned a silent empty JSON. Both hosts now return the silent empty JSON — deliberate, since the action is grid-driven (id always comes from a row the grid just rendered) and the simpler behavior removes a stack-trace/500 surface for what was already functionally a no-op.
2. Admin's CategoryUpdate gained a real null-safety fix. The original code called model.ToEntity(taxCategory) on a possibly-null taxCategory with no not-found guard at all; the shared base now checks taxCategory == null first. Strict improvement, not a behavior regression.
3. Admin's 4 tax-category actions now carry [PermissionAuthorizeAction] attributes (List/Edit/Create/Delete) — Admin's original methods had none (only Store did). PermissionAction rows are deny-only records, so this is a no-op for a normally-configured admin and only takes effect for a customer group with an explicit configured deny on TaxSettings.{Create,Edit,Delete} — same direction as every other consolidated controller and as Store already had. Flagged here per this initiative's practice of disclosing every authorization-surface delta, however narrow.

Known, deliberately deferred (not this phase)

GetActiveStore() is hand-duplicated onto Admin's TaxController (previously inherited transitively via BaseAdminController, which this class no longer extends). The repo already has a registered IAdminStoreService.GetActiveStore() equivalent, but it is not behavior-equivalent to BaseAdminController's version (missing an empty-storeId short-circuit, NREs on zero stores) — using the hand-duplicated copy was the correct behavior-preserving choice for a pure refactor phase. Follow-up filed: migrate TaxController/PaymentController/SettingController/MaintenanceController onto IAdminStoreService together in a future phase, reconciling those two behavior deltas first.

Breaking changes

None for normal operation. See "Disclosed behavior changes" above for the two narrow, deliberate exceptions (both net-neutral-or-safer).

Testing

1. dotnet build GrandNode.sln — 0 errors (70 pre-existing NU1902 advisory warnings, unrelated).
2. dotnet test src/Tests/Grand.Web.Admin.Tests — 1333/1333 passed.
3. dotnet test src/Tests/Grand.Web.Store.Tests — 120/120 passed.
4. dotnet test src/Tests/Grand.Mapping.Tests — 234/234 passed.
5. Live cross-store smoke test, run against a real Kestrel instance (Grand.Web) + an existing dev MongoDB (grand20260825), via crafted antiforgery-tokened HTTP requests (browser automation was unavailable this session):
   • store1 created a tax category (Store/Tax/CategoryAdd) — persisted with the correct StoreId.
   • store2 attempted CategoryUpdate/CategoryDelete on store1's category — both denied (empty-JSON response, HTTP 200), confirmed via re-read that neither the name nor existence changed.
   • store1 positive control: renamed its own category — succeeded, persisted.
   • Admin's grid correctly resolved the owning store's name (not the raw id, not "All") for the store-owned category.
   • Admin deleted the category with no ownership restriction — succeeded, confirmed gone.
   • Admin CategoryDelete on a nonexistent id returned HTTP 200 (confirms disclosed behavior change Label at local plugins #1 — no more ArgumentException).
   • Providers/Settings GET on both hosts returned 200 (regression check on the untouched, still-duplicated regions).
   • All smoke-test data cleaned up; no git-tracked files touched.

Final whole-branch review (opus): Ready to merge: With fixes — 0 Critical, 2 Important (one fixed: missing #nullable enable; one disclosure-only: item 3 above), 5 Minor (4 fixed: DI cleanup, cutover comments, dead using, added StoreName-resolution test coverage; 1 ruled as correct-as-shipped: the GetActiveStore() deferral above). Fix wave scoped-re-reviewed clean, no new breakage.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.