Skip to content

fix(arch001): consolidate VendorReview Admin/Vendor duplication (Phase 10) - #802

Merged
KrzysztofPajak merged 9 commits into
developfrom
arch001/phase10-vendorreview-consolidation
Aug 29, 2026
Merged

fix(arch001): consolidate VendorReview Admin/Vendor duplication (Phase 10)#802
KrzysztofPajak merged 9 commits into
developfrom
arch001/phase10-vendorreview-consolidation

Conversation

@KrzysztofPajak

Copy link
Copy Markdown
Member

Type: refactor

Issue

ARCH-001 (Architecture/Maintainability, P1): the back-office panels (Grand.Web.Admin, Grand.Web.Vendor,
Grand.Web.Store) largely duplicate rather than share code, despite Grand.Web.AdminShared existing
specifically to prevent this. This is Phase 10, consolidating the VendorReview entity — the last
originally-scoped ARCH-001 candidate — following the exact same pattern already shipped for Product,
Category, Collection, Order, Shipment, PaymentTransaction, MerchandiseReturn, and Reports.

VendorReview only exists on Admin and Vendor (no Store screen). Before this PR: VendorReviewController
was duplicated 1:1 between the two hosts, IVendorReviewViewModelService/VendorReviewViewModelService
was a separate, drifted copy in Grand.Web.Vendor (Admin's version already lived in the shared
Grand.Web.AdminShared.VendorViewModelService), and VendorReviewModel/VendorReviewListModel were two
separate model types.

Solution

  • New IAdminDataScope<VendorReview> implementations: GlobalAdminDataScope<VendorReview> reused
    unmodified for Admin (no restriction — Admin's original controller enforced none); new
    VendorVendorReviewDataScope (flat VendorId ownership check, no child-collection scan); new
    RoutedVendorReviewDataScope (2-branch Admin/Vendor request-time resolver, fails closed on any other
    area including "Store" — this entity has no Store screen at all).
  • BaseVendorReviewController in Grand.Web.AdminShared holds all 7 shared actions (List, Edit,
    Delete, ApproveSelected, DisapproveSelected). VendorSearchAutoComplete — a genuinely non-duplicated,
    Admin-only action (Vendor never had an equivalent, and exposing it would leak other vendors'
    names/ids to a vendor account) — stays on Admin's own thin subclass rather than being promoted to the
    shared base.
  • Grand.Web.Vendor's duplicate IVendorReviewViewModelService/VendorReviewViewModelService and its
    two model files deleted; Grand.Web.AdminShared.Services.VendorViewModelService's existing
    VendorReview region absorbed the reconciled logic (ApproveVendorReviews/DisapproveVendorReviews
    gained a scope parameter, used to check HasAccess per selected id before mutating).
  • VendorId/VendorName on the shared VendorReviewModel are always populated from the loaded entity
    (not scope-derived) — this collapses what used to be two different ApproveSelected/
    DisapproveSelected selection-id encodings (Admin's composite "{reviewId}:{vendorId}" vs. Vendor's
    plain id) into one format used by both hosts.
  • Views: only Edit.cshtml unified into Grand.Web.AdminShared (zero structural difference beyond
    resource-key prefix/area, using the already-proven Scope.ResourceKeyPrefix/
    ViewContext.RouteData.Values["area"] idioms). List.cshtml/Partials/CreateOrUpdate.cshtml/Admin's
    Partials/SearchVendor.cshtml stay host-specific — Admin's grid/form carry a genuine extra VendorName
    column/field that Vendor's don't, so mixing unification shapes for one entity's view set was judged not
    worth it at this small a file count. Widget-zone call sites in Edit.cshtml extracted into per-host
    WidgetZone.DetailsButtons.cshtml satellites, same precedent as every prior phase.

Found and fixed during this phase

  • Critical, found by the required live cross-vendor smoke test: neither concrete
    VendorReviewController restated [Area]/[AuthorizeAdmin|AuthorizeVendor]/[AuthorizeMenu] after
    BaseVendorReviewController stopped inheriting from BaseAdminController/BaseVendorController (which
    can't happen — those attributes differ per host) — every route 404'd on both hosts. Same bug class as
    Order's own Task 17 and MerchandiseReturn's live smoke test finding in this initiative. Fixed and
    independently re-verified live before continuing. New regression tests added
    (VendorReviewControllerRoutingTests/VendorReviewControllerSurfaceTests) asserting the attribute sets
    on both hosts, since no unit test can otherwise catch this (constructing a controller directly bypasses
    MVC routing entirely).
  • Important, found by the final whole-branch review: ApproveVendorReviews/DisapproveVendorReviews
    used the client-supplied vendor half of the composite selection id (rather than the loaded, access-
    checked entity's own VendorId) to refresh vendor rating totals — a tampered composite id could recompute
    the wrong vendor's aggregate totals, or throw an unhandled exception for a malformed/legacy id. Fixed to
    use vendorReview.VendorId (the entity's own field) in both methods, with a regression test for the
    mismatched-id case.

Breaking changes

None for end users. For anyone with custom localization overrides of Vendor.VendorReviews.Fields.* keys:
Vendor's kept edit-form field labels (Customer/Title/Rating/etc., in the still-host-specific
Partials/CreateOrUpdate.cshtml) now resolve off the shared model's Admin.VendorReviews.Fields.* keys
instead — the page title and buttons (which use Scope.ResourceKeyPrefix) are unaffected and still
resolve Vendor.*. This matches the same choice already shipped for Order/Shipment's Vendor screens; any
store that customized the Vendor.VendorReviews.Fields.* resource strings specifically will see them
revert to the Admin.* wording on this one screen.

Testing

  1. dotnet build GrandNode.sln — 0 errors, 0 warnings introduced by this change (4 pre-existing warnings
    unrelated to this phase, same as develop).
  2. Per-project tests (not full-solution, per this repo's known parallel-test-flake pattern):
    dotnet test src/Tests/Grand.Web.Admin.Tests (657/657), dotnet test src/Tests/Grand.Web.Vendor.Tests
    (15/15), dotnet test src/Tests/Grand.Mapping.Tests (234/234).
  3. Live cross-vendor smoke test performed against a real Kestrel instance + a real MongoDB dev database,
    with 2 synthetic VendorReview documents (one owned by a real logged-in vendor, one owned by a
    different vendor) and real login credentials:
    • Vendor: List shows only the vendor's own review; Edit(GET)/Delete(POST) on the other vendor's
      review both deny (redirect to List) with zero DB mutation confirmed via a direct read; a tampered
      ApproveSelected naming the other vendor's review is silently skipped (confirmed via DB read, not
      just the response shape); the same action on the vendor's own review succeeds (positive control,
      proves the deny path isn't just a broken pipeline).
    • Admin: List shows both vendors' reviews with the VendorName column populated;
      VendorSearchAutoComplete returns matching vendors (Admin-only, confirmed absent/404 on Vendor's
      host); Edit(GET) on any vendor's review succeeds; Edit(POST, save-and-continue) redirects to
      .../Edit/<id>?VendorId=<vendorId> correctly.
    • All synthetic test data removed after the test; confirmed the database returned to its pre-test
      state.
  4. Final whole-branch review (opus-tier) — "Ready to merge: With fixes"; both Important findings fixed in
    one round, scoped re-review confirmed both addressed with no new breakage. Minor findings (localization
    key change disclosed above; a few defensive/cosmetic notes already shipped identically in prior ARCH-001
    phases) parked as non-blocking.

KrzysztofPajak and others added 8 commits August 29, 2026 05:59
…lService, delete Vendor's duplicate

- IVendorViewModelService/VendorViewModelService: ApproveVendorReviews/
  DisapproveVendorReviews now take an IAdminDataScope<VendorReview> scope
  parameter, consumed via scope.HasAccess before mutating (deliberately
  reorders the GetVendorById call to happen after the scope/null check).
- Delete Grand.Web.Vendor's duplicate IVendorReviewViewModelService/
  VendorReviewViewModelService (now fully superseded by AdminShared's
  VendorViewModelService + Task 1's VendorVendorReviewDataScope).
- Add VendorViewModelServiceTests covering the entity-populate path, the
  list-scope path, and both approve/disapprove scope-filtering paths
  (global vs vendor-owned).
- Necessary transitional fix beyond the task's named file list: Admin's
  own VendorReviewController.cs (not part of the duplication being
  removed) already called the old 1-arg signature and would otherwise
  fail to build; updated it to constructor-inject the already
  DI-registered IAdminDataScope<VendorReview> (Task 1) and pass it
  through unchanged behavior-wise (Admin's routed scope resolves to
  GlobalAdminDataScope for this host). Task 4/5 are expected to further
  refactor this controller into a shared base class.

Remaining build errors after this change are confined to
Grand.Web.Vendor/Controllers/VendorReviewController.cs, which still
references the deleted IVendorReviewViewModelService/Vendor model types
- resolved by Task 5.
…eview actions

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0141KkNM5HDRq3ZqAXunF9Ug
…ubclasses

Also removes the leftover Grand.Web.Vendor DI registration for the deleted
IVendorReviewViewModelService/VendorReviewViewModelService (dangling since
Task 2/3 deleted those types), which was blocking the solution build.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0141KkNM5HDRq3ZqAXunF9Ug
…hared, extract widget-zone satellites

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0141KkNM5HDRq3ZqAXunF9Ug
…VendorReview thin subclasses

Found by Task 7's live cross-vendor smoke test: neither concrete
VendorReviewController restated [Area]/[AuthorizeAdmin|Vendor]/[AuthorizeMenu]
after BaseVendorReviewController stopped inheriting from
BaseAdminController/BaseVendorController - every route 404'd on both hosts.
Same bug class as Order's Task 17 and MerchandiseReturn's own live smoke
test finding; a green build + passing unit tests cannot catch it since
unit tests construct the controller directly, bypassing MVC routing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0141KkNM5HDRq3ZqAXunF9Ug
…tion

- ApproveVendorReviews/DisapproveVendorReviews: derive the vendor id used
  for totals refresh from the loaded, access-checked VendorReview entity
  instead of the client-supplied composite id half. A vendor could submit
  'myReviewId:otherVendorId' and mutate another vendor's aggregate rating
  totals, or crash the request with a garbage/non-composite id.
- PrepareVendorReviewModel: null-guard VendorName when the vendor behind
  a review has since been deleted.
- Add service test coverage for the mismatched-composite-vendor-id case
  on both Approve/DisapproveVendorReviews.
- Add controller routing/surface regression tests for both concrete
  VendorReviewControllers (Admin, Vendor) covering Area/Authorize
  attributes and the Admin-only VendorSearchAutoComplete action, mirroring
  PaymentTransactionControllerRoutingTests and OrderControllerSurfaceTests.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0141KkNM5HDRq3ZqAXunF9Ug
Copilot AI lite review requested due to automatic review settings August 29, 2026 07:11

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.

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

{
var result = _controller.Index() as RedirectToActionResult;
Assert.IsNotNull(result);
Assert.AreEqual("List", result.ActionName);
{
var result = _controller.List() as ViewResult;
Assert.IsNotNull(result);
Assert.IsInstanceOfType(result.Model, typeof(VendorReviewListModel));
var result = await _controller.Edit("missing") as RedirectToActionResult;

Assert.IsNotNull(result);
Assert.AreEqual("List", result.ActionName);
var result = await _controller.Edit("r1") as RedirectToActionResult;

Assert.IsNotNull(result);
Assert.AreEqual("List", result.ActionName);
var result = await _controller.Delete("r1") as RedirectToActionResult;

Assert.IsNotNull(result);
Assert.AreEqual("List", result.ActionName);
var result = await _controller.Delete("r1") as RedirectToActionResult;

Assert.IsNotNull(result);
Assert.AreEqual("List", result.ActionName);
…ndorreview-consolidation

# Conflicts:
#	src/Web/Grand.Web.AdminShared/Startup/StartupApplication.cs
#	src/Web/Grand.Web.Vendor/Areas/Vendor/Views/_ViewImports.cshtml
#	src/Web/Grand.Web.Vendor/Extensions/HasAccess.cs
#	src/Web/Grand.Web.Vendor/Startup/StartupApplication.cs
@KrzysztofPajak
KrzysztofPajak merged commit 9ae0971 into develop Aug 29, 2026
6 checks passed
@KrzysztofPajak
KrzysztofPajak deleted the arch001/phase10-vendorreview-consolidation branch August 29, 2026 08:01
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