ARCH-001 Phase 16: News controller/service consolidation (Admin/Store) + Comments tab fix - #810
Merged
Merged
Conversation
…+ 3 ruled bug fixes Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tore's broken Comments tab) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…regression tests Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…st.cshtml, extract widget-zone satellites Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ents(POST), closing a cross-tenant comment-read leak on Store Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A newly exposed Comments(GET) endpoint returns a non-existent view (runtime 500) and should be made to fail gracefully before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Consolidates duplicated Admin and Store News CRUD/comment-moderation logic behind a shared BaseNewsController in Grand.Web.AdminShared, introducing a routed per-request IAdminDataScope<NewsItem> to preserve tenant isolation, and extracting host-specific widget zones into satellite partials so the shared views can remain host-agnostic. This also enables Store’s previously-shipped-but-broken “Comments” tab by adding the missing Store-reachable Comments / CommentDelete endpoints (with proper access checks).
Changes:
- Introduced
BaseNewsControllerandRoutedNewsItemDataScopeto centralize News actions while enforcing correct per-area access scope. - Moved/unified News views into
Grand.Web.AdminSharedand extracted widget-zone hooks into per-host partials (Admin vs Store). - Added characterization/unit tests covering scope routing, host attributes, and the merged access-check behavior.
File summaries
| File | Description |
|---|---|
| src/Web/Grand.Web.Store/Controllers/NewsController.cs | Converted Store News controller to a thin BaseNewsController subclass; keeps Store-only warning hook + Preview. |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Partials/WidgetZone.ListButtons.cshtml | Store widget-zone hook for list buttons. |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Partials/WidgetZone.DetailsTabs.cshtml | Store widget-zone hook for details tab extensions. |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Partials/WidgetZone.DetailsSeoTop.cshtml | Store widget-zone hook for SEO top content. |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Partials/WidgetZone.DetailsSeoBottom.cshtml | Store widget-zone hook for SEO bottom content. |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Partials/WidgetZone.DetailsInfoTop.cshtml | Store widget-zone hook for info top content. |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Partials/WidgetZone.DetailsInfoBottom.cshtml | Store widget-zone hook for info bottom content. |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Partials/WidgetZone.DetailsCommentsTop.cshtml | Store widget-zone hook for comments top content. |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Partials/WidgetZone.DetailsCommentsList.cshtml | Store widget-zone hook for comments list content. |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Partials/WidgetZone.DetailsCommentsBottom.cshtml | Store widget-zone hook for comments bottom content. |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Partials/WidgetZone.DetailsButtons.Edit.cshtml | Store widget-zone hook for edit-page buttons. |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Partials/WidgetZone.DetailsButtons.Create.cshtml | Store widget-zone hook for create-page buttons. |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Partials/CreateOrUpdate.TabInfo.cshtml | Removed Store-specific News “Info” tab partial (now provided by shared views). |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Partials/CreateOrUpdate.cshtml | Removed Store-specific News tabstrip partial (now provided by shared views). |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/List.cshtml | Removed Store-specific News list view (now unified/shared). |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Edit.cshtml | Removed Store-specific edit view (now shared). |
| src/Web/Grand.Web.Store/Areas/Store/Views/News/Create.cshtml | Removed Store-specific create view (now shared). |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/News/Partials/CreateOrUpdate.TabSeo.cshtml | Updated shared SEO tab to delegate widget zones via host-specific partials. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/News/Partials/CreateOrUpdate.TabInfo.cshtml | Updated shared Info tab; conditionally renders Admin-only fields and delegates widget zones via partials. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/News/Partials/CreateOrUpdate.TabComments.cshtml | Updated shared Comments tab; routes AJAX endpoints by current area and delegates widget zones via partials. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/News/Partials/CreateOrUpdate.cshtml | Updated shared tabstrip to delegate widget zones via host-specific partials. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/News/List.cshtml | Unified List view for Admin/Store; uses route area and conditionally shows Admin-only store selector UI. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/News/Edit.cshtml | Shared Edit view; binds asp-area to current route and delegates button widget zone via partial. |
| src/Web/Grand.Web.AdminShared/Views/AdminShared/News/Create.cshtml | Shared Create view; binds asp-area to current route and delegates button widget zone via partial. |
| src/Web/Grand.Web.AdminShared/Startup/StartupApplication.cs | Registers routed IAdminDataScope<NewsItem> for correct per-area resolution in combined host. |
| src/Web/Grand.Web.AdminShared/Services/RoutedNewsItemDataScope.cs | New: routes NewsItem data scope by request area, failing closed on missing/unrecognized areas. |
| src/Web/Grand.Web.AdminShared/Controllers/BaseNewsController.cs | New: shared controller implementing CRUD + comment moderation with scope gates and shared view usage. |
| src/Web/Grand.Web.Admin/Controllers/NewsController.cs | Converted Admin News controller to a thin BaseNewsController subclass with required Admin host attributes. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/News/Partials/WidgetZone.ListButtons.cshtml | Admin widget-zone hook for list buttons. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/News/Partials/WidgetZone.DetailsTabs.cshtml | Admin widget-zone hook for details tab extensions. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/News/Partials/WidgetZone.DetailsSeoTop.cshtml | Admin widget-zone hook for SEO top content. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/News/Partials/WidgetZone.DetailsSeoBottom.cshtml | Admin widget-zone hook for SEO bottom content. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/News/Partials/WidgetZone.DetailsInfoTop.cshtml | Admin widget-zone hook for info top content. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/News/Partials/WidgetZone.DetailsInfoBottom.cshtml | Admin widget-zone hook for info bottom content. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/News/Partials/WidgetZone.DetailsCommentsTop.cshtml | Admin widget-zone hook for comments top content. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/News/Partials/WidgetZone.DetailsCommentsList.cshtml | Admin widget-zone hook for comments list content. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/News/Partials/WidgetZone.DetailsCommentsBottom.cshtml | Admin widget-zone hook for comments bottom content. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/News/Partials/WidgetZone.DetailsButtons.Edit.cshtml | Admin widget-zone hook for edit-page buttons. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/News/Partials/WidgetZone.DetailsButtons.Create.cshtml | Admin widget-zone hook for create-page buttons. |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/News/Partials/CreateOrUpdate.TabSeo.cshtml | Removed Admin-specific SEO tab partial (now shared). |
| src/Web/Grand.Web.Admin/Areas/Admin/Views/News/Partials/CreateOrUpdate.TabComments.cshtml | Removed Admin-specific Comments tab partial (now shared). |
| src/Tests/Grand.Web.Store.Tests/Controllers/NewsControllerTests.cs | New: Store controller attribute + warning-hook + preview behavior tests. |
| src/Tests/Grand.Web.Admin.Tests/Controllers/RoutedNewsItemDataScopeTests.cs | New: validates routed scope resolves correctly and fails closed. |
| src/Tests/Grand.Web.Admin.Tests/Controllers/NewsControllerTests.cs | New: validates Admin controller is a thin subclass and carries required attributes. |
| src/Tests/Grand.Web.Admin.Tests/Controllers/BaseNewsControllerTests.cs | New: characterization tests for shared controller behavior (list forcing, language loading, scope gates, comment moderation). |
Review details
- Files reviewed: 45/45 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.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| var view = result as ViewResult; | ||
| Assert.IsNotNull(view); | ||
| var model = (NewsItemListModel)view.Model; |
|
|
||
| var view = result as ViewResult; | ||
| Assert.IsNotNull(view); | ||
| var model = (NewsItemListModel)view.Model; |
|
|
||
| var redirect = result as RedirectToActionResult; | ||
| Assert.IsNotNull(redirect); | ||
| Assert.AreEqual("List", redirect.ActionName); |
|
|
||
| var redirect = result as RedirectToActionResult; | ||
| Assert.IsNotNull(redirect); | ||
| Assert.AreEqual("List", redirect.ActionName); |
|
|
||
| var redirect = result as RedirectToActionResult; | ||
| Assert.IsNotNull(redirect); | ||
| Assert.AreEqual("Edit", redirect.ActionName); |
|
|
||
| var redirect = result as RedirectToActionResult; | ||
| Assert.IsNotNull(redirect); | ||
| Assert.AreEqual("List", redirect.ActionName); |
|
|
||
| var redirect = result as RedirectToActionResult; | ||
| Assert.IsNotNull(redirect); | ||
| Assert.AreEqual("List", redirect.ActionName); |
|
|
||
| var json = result as JsonResult; | ||
| Assert.IsNotNull(json); | ||
| var gridModel = (DataSourceResult)json.Value; |
|
|
||
| var json = result as JsonResult; | ||
| Assert.IsNotNull(json); | ||
| var gridModel = (DataSourceResult)json.Value; |
…er.EditWarningCheck Same CodeQL finding/fix as Page (arch001/phase15): the inner 'newsItem.LimitedToStores &&' re-check in the OR's second branch is always true and was dead code inherited from the Category/Blog idiom. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRztBQVfXimBPMFAioLabo
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRztBQVfXimBPMFAioLabo
…ing nonexistent view Copilot review on PR #810: BaseNewsController.Comments(string) returned View() but no Comments.cshtml exists for News (unlike Blog, which has one). Preexisting bug carried over from Admin's original NewsController, verified against the merge-base. The GET action is unreachable via the UI — the Comments tab's grid calls the POST overload only — so redirect to List instead of leaving a dead View() call that 500s if ever hit directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRztBQVfXimBPMFAioLabo
Comments_ReturnsView asserted the old (buggy) View() behavior. Replaced with Comments_RedirectsToList to match the redirect fix in the prior commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FRztBQVfXimBPMFAioLabo
|
|
||
| var redirect = result as RedirectToActionResult; | ||
| Assert.IsNotNull(redirect); | ||
| Assert.AreEqual("List", redirect.ActionName); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves ARCH-001 Phase 16
Type: bugfix, refactor
Issue
Grand.Web.AdminandGrand.Web.Storeeach carried their ownNewsController(228 / 234 lines, noVendor equivalent) duplicating CRUD, comment-moderation, and access-control logic for
NewsIteminsteadof sharing it through
Grand.Web.AdminShared. Same class of drag and security-gap risk as every priorARCH-001 phase — but investigating this entity's shape surfaced something new: a live, pre-existing
bug independent of the duplication itself. Store's
Edit.cshtmlalready ships a fully-built "Comments"tab (identical Kendo grid markup to Admin's) whose AJAX calls target
/Store/News/Commentsand/Store/News/CommentDelete— but Store'sNewsControllerhas never had either action. The tab hasalways been broken for every Store user.
Solution
Consolidated both hosts behind a shared
BaseNewsControllerinGrand.Web.AdminShared, driven by a newRoutedNewsItemDataScope(IAdminDataScope<NewsItem>). No Vendor work needed.BaseNewsControllerholds List/Create/Edit/Delete and — the headline change —Comments/CommentDelete, now genuinely shared rather than left Admin-only. This makes Store's already-built tabwork for the first time.
NewsControllers are thin subclasses. Store also keepsEditWarningCheck(the sameproven re-derived condition already used by Category/Blog/Page) and its own
Previewaction.Grand.Web.AdminShared/Views/AdminShared/News/...— unlike Page's phase,List.cshtmlunifies too (News's list is a single shared grid, not a two-tab split). 10 widget zonesextracted into real per-host satellite partials (Store's
<vc:admin-widget>calls were dead literalHTML before this change, the same bug class found and fixed in every prior phase).
Full design rationale:
docs/superpowers/specs/2026-09-04-arch001-news-consolidation-design.md(spec)and
docs/superpowers/plans/2026-09-04-arch001-news-consolidation.md(plan) — both kept on disk only(gitignored), summarized here.
Disclosed behavior changes (intentional, not regressions)
IAdminDataScopeaccess checks that Admin's originals neverhad (
CommentDeleteusesHasAccess,CommentsusesCanView) — required now that these actionsare reachable from Store; Admin is unaffected since its scope is a no-op.
List(POST)now uses the sharedINewsViewModelService.PrepareNewsItemModelfor both hosts,dropping Store's original manual re-implementation, which also loaded
NewsSettingsviaISettingService.LoadSetting<NewsSettings>without ever using the result (confirmed dead code). As aside effect, Store's news list now shows unpublished, scheduled, expired, and customer-group-
restricted items it couldn't see before (matching Admin's existing behavior) — Store managers
couldn't previously see their own drafts on this screen. No tenant-isolation impact: store filtering
and the
SearchStoreIdforcing are unchanged.Create(POST)no longer double-writes. Store's original called_newsService.UpdateNews(newsItem)immediately after
InsertNewsItemModel(which already inserts internally) — a genuine, purposelessredundant write, removed.
awaitbug inCreate(GET)and its invalid-redisplay path is fixed —ViewBag.AllLanguageswas being assigned an unawaitedTaskobject instead of the resolved languagelist (Admin's own
Editactions already did this correctly).SearchNewsTitlesearch-by-title JS plumbing was dropped, not fixed into a workingfeature. Both hosts' original code was already non-functional here (Store hardcoded an empty string;
Admin never sent the field at all) — implementing a real search-by-title feature would have been scope
creep beyond fixing what's broken.
#search-newsitemclick handler was correctly kept, deviating from the plan's owninstruction to drop it — the unified
List.cshtmlstill renders that button for Admin, so removing thehandler would have broken Admin's live search. A real implementer catch of a plan defect.
Final whole-branch review
Dispatched on the most capable model per this initiative's process. Verdict: "Ready to merge: With
fixes" — 1 Critical, found by re-deriving tenant isolation from the two original controllers rather
than trusting that per-task reviews already covered it:
Critical, fixed:
Comments(POST)had no access check at all —CommentDeletegot the "add theprotection Store now needs, even though Admin's original never needed one" treatment, but its sibling
read action, fifteen lines above it in the same task's diff, did not (because that code was a verbatim,
untouched copy of Admin's already-correct original, which never needed a check since Admin is global).
Two real leaks existed before the fix: a store manager could pass a different store's news item id and
read its full comment list — including commenter email addresses — and an empty id fell through to
GetAllComments(""), returning every comment across every store in the installation. Fixed with aCanViewgate (deliberately not the stricterHasAccess, so the tab still opens for global/multi-storeitems a Store user is already permitted to view via
Edit); the empty-id leak closes as a side effectsince
GetNewsById("")returnsnull. Scoped re-review confirmed the fix, no new breakage.Breaking changes
None for storefront/API consumers. See "Disclosed behavior changes" above — the news-list visibility
widening for Store managers is the most user-visible change, and it's a widening, not a leak.
Testing
Automated (all green):
dotnet build GrandNode.sln— 0 errors.dotnet test src/Tests/Grand.Web.Admin.Tests— full suite green (1025+ tests, incl. tests proving eachof the 3 ruled fixes and the
Comments/CommentDeleteaccess gates).dotnet test src/Tests/Grand.Web.Store.Tests— full suite green (67 tests).dotnet test src/Tests/Grand.Mapping.Tests— 234/234.Live smoke test (run directly against a real dev DB, not sandboxed — specifically targeting this phase's
headline fix):
Listrenders correctly with the store-filter dropdown; created a news item limitedexclusively to a second store ("Store2"); confirmed its
Editscreen's Comments tab loads a real,working Kendo grid (not a 404/error) — positive control on the fix's foundation.
Edit(GET) denied (redirect toList), and the Store2-onlyitem correctly excluded from store1's own news list. Crafted antiforgery-tokened requests to
/Store/News/Commentswith the Store2-only item's id, and again with an empty id, both deniedwith
"No access to this news item's comments"— directly proving both halves of the Critical fix.Deletedenied, redirected toList.Editsucceeded with correct fieldvisibility (no CustomerGroups/Stores fields); clicked the Comments tab in the actual browser UI and
confirmed the Kendo grid genuinely initialized and loaded (
total: 0, not an error) — this is theconcrete, end-to-end proof that Store's Comments tab, broken since before this initiative existed, now
works.
[Area]/[Authorize*]omission that has hit roughly half ofthis initiative's phases did not recur here.
Disclosed, out-of-scope finding: navigating directly to the bare
GET /Admin/News/Commentsaction(not the tab — the standalone route) throws a 500, because no
Comments.cshtmlview has ever existed forthis action on either host. Confirmed via the pre-consolidation commit that
List.cshtml's "Viewcomments" link has always pointed at
Edit/{id}, never at this action — it's dead, pre-existing codefrom before this initiative, untouched by this PR.
🤖 Generated with Claude Code