diff --git a/docs/architecture/foundation/README.md b/docs/architecture/foundation/README.md index d5afbf7..fc7ec73 100644 --- a/docs/architecture/foundation/README.md +++ b/docs/architecture/foundation/README.md @@ -57,6 +57,7 @@ It is a platform runtime that must support unlimited tenants from one Angular ap - [Seller-Management-Backoffice-Readiness-Audit.md](Seller-Management-Backoffice-Readiness-Audit.md) — per-module scoping/permissions readiness audit - [Seller-Management-Storefront-Audit.md](Seller-Management-Storefront-Audit.md) — `market.com`/`seller.market.com` storefront readiness audit - [Seller-Management-Backend-Migration-Plan.md](Seller-Management-Backend-Migration-Plan.md) — full backend migration plan, module-by-module, phased +- [Seller-Management-Final-Design-Review.md](Seller-Management-Final-Design-Review.md) — principal-architect review, findings, verdict ### Engineering Rule Documents diff --git a/docs/architecture/foundation/Seller-Management-Final-Design-Review.md b/docs/architecture/foundation/Seller-Management-Final-Design-Review.md new file mode 100644 index 0000000..6b83583 --- /dev/null +++ b/docs/architecture/foundation/Seller-Management-Final-Design-Review.md @@ -0,0 +1,190 @@ +# Seller Management — Final Design Review + +Principal-architect-level review of everything built and documented for +Seller Management so far. One real bug found and fixed (below, in scope per +this mission's "unless absolutely required" carve-out); everything else is +findings only, no further code changed. Reviewed against source directly +(fresh `tsc --noEmit` and `arch:check` run for this review, not recalled +from memory) plus every doc in the series: +[Seller-Management.md](Seller-Management.md), +[ADR-011](adr/ADR-011-optional-seller-management-module.md), +[Seller-Management-Diagrams.md](Seller-Management-Diagrams.md), +[Seller-Management-Domain-Models.md](Seller-Management-Domain-Models.md), +[Seller-Management-UX-Review.md](Seller-Management-UX-Review.md), +[Seller-Management-Backoffice-Readiness-Audit.md](Seller-Management-Backoffice-Readiness-Audit.md), +[Seller-Management-Storefront-Audit.md](Seller-Management-Storefront-Audit.md), +[Seller-Management-Backend-Migration-Plan.md](Seller-Management-Backend-Migration-Plan.md), +and `BACKEND.md` §11. + +## Checklist verification (evidence-based, not asserted) + +| Item | Status | Evidence | +|---|---|---| +| No existing marketplace breaks | ✓ Verified | Fresh `tsc --noEmit` clean, fresh `arch:check` clean (this review); live browser tests at 3 separate checkpoints across storefront home + backoffice dashboard/route | +| Feature is optional | ✓ Verified | `DEFAULT_PLATFORM_MODULES_CONFIG.sellerManagement.enabled = false`; no backend anywhere sets it true | +| Bootstrap remains backward compatible | ✓ Verified | `modules?`/`seller?` both optional on `BootstrapConfig`; `tsc` stayed clean the moment they were added, no consumer touched | +| No API breaking changes | ✓ Verified (trivially) | No API exists to break — documentation-only for the backend side | +| Existing frontend continues working | ✓ Verified | Live-tested at every UI-touching commit, zero regressions found | +| Existing backend continues working | N/A | No backend exists; not applicable until implementation begins | +| Dependency direction (ADR-002) | ✓ Verified | `arch:check:boundaries` clean, run fresh for this review | +| Import boundaries (ADR-003) | ✓ Verified | Same tool, same clean result | +| No tenant-specific conditions | ✓ Verified | Reviewed every new file directly — zero `if (tenant...)`/`if (seller===...)` conditionals exist anywhere | +| Seller scope is additive | ✓ Verified | Every new field on every touched type is optional; nothing required changed | +| UI consistency | ✓ Verified, with 2 fixes already applied | Dedicated UX-review pass found and fixed a label/a11y gap and a native-bullet inconsistency | +| Translation readiness | ✓ Verified | en/ru/hy all carry every new key, confirmed by exact-count grep | +| Accessibility readiness | ✓ Verified structurally, **one caveat** | Confirmed via accessibility-tree inspection (`role=dialog`, `aria-modal`, focus trap, `aria-label`) — **no real screen-reader software (NVDA/VoiceOver) pass was ever done**, only automated tree inspection. Flagged below (Low). | +| Performance considerations | ✓ Verified | New route is its own lazy chunk (confirmed in build output), doesn't touch the initial bundle | +| Future scalability | ✓ Addressed at design level | `Seller-Management-Backend-Migration-Plan.md` covers indexes, caching, phased rollout | + +## Findings + +### 1. `SellerConfig` vs. `Seller`/`SellerBranding` — two unreconciled type hierarchies — **Medium** + +`shared/models/config/seller.model.ts` (`SellerConfig`, the bootstrap wire +shape: `id, marketplaceId, slug, name, defaultLocale, supportedLocales`) and +`core/sellers/models/seller.model.ts` (`Seller`, the domain entity: +`id, marketplace: MarketplaceRef, name, slug, status, branding?, +createdAt, updatedAt`) describe overlapping concepts with different shapes +and no conversion function between them. This was **self-identified during +this same body of work** (`BACKEND.md` §11.6 already flags it as an open +question) — restating it here as an independently-confirmed architectural +finding, not a new discovery, because a final design review should not +let a self-flagged gap quietly become "someone else's problem later." +**Recommendation:** resolve before real backend work starts — either +`SellerConfig` becomes a strict projection of `Seller` (documented mapping), +or they're merged into one type with bootstrap-specific fields marked +optional. Either is fine; leaving it unreconciled through implementation +risks two competing "seller" shapes drifting further apart. + +### 2. No reusable capability-guard abstraction exists — **Medium** + +ADR-011 (and ADR-009 before it) both prescribe checking a capability flag +"in one place, not scattered conditionals." In practice, **no such +reusable guard exists anywhere in this codebase** — not for +`sellerManagement.enabled`, and not for any existing feature flag either. +ADR-009 itself describes a `FeatureFlagService` that was never actually +built (confirmed: no file of that name exists in `src/app/core`). The one +current consumer (`AdminSellerManagementPageComponent`) hand-rolls the +optional-chain read directly. With one consumer this is harmless; the +moment a second consumer needs the same check, it will either duplicate +the same expression (drift risk: `?? false` vs `=== true` vs missing a +null-check) or someone will need to build the guard ADR-009 already +promised. **Recommendation:** build one small `SellerManagementGuardService` +(or equivalent) the first time a second consumer needs the flag — don't +let a third or fourth hand-rolled copy accumulate first. + +### 3. Reactive-signal bug in the Phase 1 page — **Fixed during this review** + +`AdminSellerManagementPageComponent.sellerManagementEnabled` read +`configService.getBootstrapSnapshot()` once via a plain `signal()` at +construction time — not reactively tied to `configService.bootstrapRevision()` +the way `UiRuntimeFacade` and `SeoService` both correctly do. If bootstrap +ever reloaded after initial page load (tenant context switch, revalidation) +with the flag now `true`, this signal would never update — a real +staleness bug, currently invisible because the flag is always `false` and +the signal was never even read in the template. **Fixed in this review** +(changed to `computed()` keyed on `bootstrapRevision()`, matching the +established codebase pattern exactly) — a one-line correctness fix to +already-committed code, not new feature work, so it fell inside this +mission's "unless absolutely required" carve-out. Verified `tsc --noEmit` +clean after the change. + +### 4. `sellerId` typed as bare `string`, not `UUID` — **Low / Nice to have** + +`Item.sellerId?`, `AdminProduct.sellerId?`, `AdminOrder.sellerId?` are all +typed `string`, while every ID in the new `core/sellers/models/` uses the +`UUID` type alias (`type UUID = string` — functionally identical, purely a +signaling convention used consistently elsewhere in this codebase, e.g. +`TenantConfig.id: UUID`). Zero functional impact since `UUID` is a bare +alias, but a future reader will reasonably wonder why the new sellerId +fields didn't follow the convention the sellers domain itself established +one file away. **Recommendation:** trivial fix, do it opportunistically +next time any of these three files is touched — not worth a dedicated pass. + +### 5. `MarketplaceRef` vs. `TenantConfig` — acceptable but worth flagging — **Low** + +`MarketplaceRef {id, slug, name}` and the existing `TenantConfig` (id, slug, +code, host, name, locales, currencies, timezone, base URLs) both represent +"a marketplace," from two different vantage points (seller-record reference +vs. full runtime tenant contract). This is a deliberate, documented +distinction (`Seller-Management-Domain-Models.md` explains it), not an +accidental duplication — but it's the kind of decision that reads clearly +today and could easily read as "why are there two Marketplace types" to +someone joining later without the context. **Recommendation:** no action +needed now; if a third marketplace-shaped type is ever proposed, that's the +signal to consolidate, not before. + +### 6. The flag's "true" branch has never been exercised, even manually — **Medium** + +Every verification claim in this document's checklist table (and every +prior audit) was tested with `modules.sellerManagement.enabled` at its +real-world value: `false` (or absent). **Nobody has ever manually set it to +`true`** — not in a browser dev-tools override, not in a mock fixture — to +confirm the flag-reading code path actually behaves as intended when the +condition it exists to detect is met. Today that's low-stakes (there's no +enabled-state UI to differ), but the review checklist item "Seller scope is +additive" was verified by reading the code, not by observing the `true` +branch execute. **Recommendation:** the first time any enabled-state UI is +built, that's also the moment to add one manual (or fixture-based) test +confirming the `true` path — don't let a second feature get built on top of +an assumption that was never actually observed. + +### 7. Documentation-to-code ratio is unusually high — **Low / Nice to have** + +Eight documents (this one included) exist for a capability that has zero +backend bytes and one placeholder frontend page. That's not inherently +wrong — the mission explicitly asked for staged documentation-first work — +but it carries two real risks worth naming: (a) maintenance burden keeping +eight cross-linked documents consistent if any single decision changes +(e.g., if Unified-vs-Split-Orders resolves one way, at least three of these +docs reference it and would need a coordinated update), and (b) the more +times an undecided item ("Future," "not designed") is repeated across +documents, the more it can start to feel settled by sheer repetition even +though nothing has actually been decided. **Recommendation:** before +backend implementation begins, do one consolidation pass collapsing +overlapping content (the Unified/Split-Orders question in particular +appears in `Seller-Management.md`, the Storefront audit, `BACKEND.md` §11, +and the Migration Plan) into a single canonical statement the others link +to, rather than four independent restatements. + +### 8. No automated test coverage — **Low / Nice to have, not new** + +Zero unit or integration tests cover any file introduced in this work — +consistent with the rest of the codebase (`PROJECT_STATUS.md` already +documents "no automated test suite exists," a pre-existing, repo-wide gap, +not something this work introduced or made worse). Noting it here for +completeness, not as a Seller-Management-specific defect. + +## What I did NOT find + +No architectural weaknesses beyond the above. Specifically checked for and +did **not** find: circular dependencies (verified fresh, clean), scattered +tenant/seller conditionals (none exist anywhere), over-engineering relative +to the "typed models only" mandate (the six new model files map 1:1 to the +six concepts explicitly requested, nothing extra), hidden coupling between +`core/sellers/models` and any `features/` folder (the new domain models +import only from `shared/types`, nothing reaches into a feature module), +or any security-sensitive code path touched (no auth, no payment code was +modified anywhere in this entire body of work). + +## Verdict + +**Not an unqualified "ready for implementation."** Two Medium findings +(#1, the unreconciled `SellerConfig`/`Seller` type split, and #2, the +missing capability-guard abstraction) are genuine architectural loose ends +that should be resolved by decision or by a small build, respectively, +before real backend/CRUD work begins — not because either blocks anything +today, but because both compound in cost the longer they're left +unresolved (more consumers = more places to reconcile later; the type +duality especially, since a real `SellerRepository`/`SellerGateway` would +otherwise have to pick one shape or invent a mapping ad hoc under time +pressure). Finding #6 (the flag's true-branch never observed) is a +process gap to close at the next milestone, not before it. + +**Everything that has actually been built — the typed foundation, the +disabled-by-default feature flag, the Phase 1 UI, and the one real bug +this review found and fixed — is solid and ready to stay exactly as it +is.** The design as a *whole plan* is sound and internally consistent; the +two Medium findings are refinements to make before the next phase starts, +not defects in what exists today. No Critical or High-severity issue was +found anywhere in this review. diff --git a/src/app/features/admin/seller-management/pages/admin-seller-management-page.component.ts b/src/app/features/admin/seller-management/pages/admin-seller-management-page.component.ts index f8eea46..63a502c 100644 --- a/src/app/features/admin/seller-management/pages/admin-seller-management-page.component.ts +++ b/src/app/features/admin/seller-management/pages/admin-seller-management-page.component.ts @@ -1,4 +1,4 @@ -import { ChangeDetectionStrategy, Component, inject, signal } from '@angular/core'; +import { ChangeDetectionStrategy, Component, computed, inject, signal } from '@angular/core'; import { FormsModule } from '@angular/forms'; import { TranslatePipe } from '../../../../i18n/translate.pipe'; import { TranslateService } from '../../../../i18n/translate.service'; @@ -48,9 +48,18 @@ export class AdminSellerManagementPageComponent { private readonly configService = inject(ConfigService); private readonly i18n = inject(TranslateService); - protected readonly sellerManagementEnabled = signal( - this.configService.getBootstrapSnapshot()?.modules?.sellerManagement?.enabled ?? false - ); + /** + * Reactive, not a one-time snapshot - re-reads on every bootstrap reload + * (same pattern as UiRuntimeFacade/SeoService's bootstrapRevision()-keyed + * effects), so this won't go stale if bootstrap ever reloads with the + * flag now true. Not yet consumed in the template - there is no + * enabled-state UI to gate yet (see class doc comment) - but it must be + * correct now rather than silently wrong later. + */ + protected readonly sellerManagementEnabled = computed(() => { + this.configService.bootstrapRevision(); + return this.configService.getBootstrapSnapshot()?.modules?.sellerManagement?.enabled ?? false; + }); protected readonly requestDialogOpen = signal(false); protected readonly successDialogOpen = signal(false);