VPR-59 [2/4] CMS migration: content blocks, left nav, link collections (backend)#252
Open
rlorenzo wants to merge 4 commits into
Open
VPR-59 [2/4] CMS migration: content blocks, left nav, link collections (backend)#252rlorenzo wants to merge 4 commits into
rlorenzo wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds the backend portion of the CMS migration for content blocks, left-nav menu management, and link collections, aligning behavior with the legacy CMS while introducing new APIs and unit tests.
Changes:
- Introduces
CmsContentBlockService+ rebuiltCMSContentControllerwith paging/filtering, edit history (including sanitized diffs), restore/delete, and concurrency guards. - Adds left-nav menu management via
CmsLeftNavService+CMSLeftNavController, including batch item replace semantics and legacy-parity display filtering. - Fixes/adjusts link-collection API behaviors (Location headers, delete cascades, tag-category create/reorder robustness) and adds comprehensive unit tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/Areas/CMS/Services/CmsNavMenu.cs | Updates CMS admin nav composition and permission gating for new CMS features. |
| web/Areas/CMS/Services/CmsLeftNavService.cs | New service for left-nav menu CRUD + batch item save semantics. |
| web/Areas/CMS/Services/CmsContentBlockService.cs | New service implementing content-block CRUD, history, diffs, concurrency, and paging/filtering. |
| web/Areas/CMS/Models/DTOs/LeftNavDtos.cs | New DTOs + validation for left-nav menus/items. |
| web/Areas/CMS/Models/DTOs/ContentBlockDto.cs | New DTOs for content blocks, history, diffs, and anonymous public rendering. |
| web/Areas/CMS/Models/CmsContentBlockMapper.cs | Mapperly mapper for content-block DTO shapes (full + public). |
| web/Areas/CMS/Models/CMSBlockAddEdit.cs | Extends content-block add/edit contract with file GUIDs + LastModifiedOn concurrency token. |
| web/Areas/CMS/Data/LeftNavMenu.cs | Adjusts display-time filtering to match legacy behavior (permission-less items visible to signed-in users). |
| web/Areas/CMS/Controllers/CMSLinkCollectionLinks.cs | Adjusts POST Location to a routable action for link creation. |
| web/Areas/CMS/Controllers/CMSLinkCollectionController.cs | Permission constant usage + delete cascade fix + tag-category create/reorder robustness. |
| web/Areas/CMS/Controllers/CMSLeftNavController.cs | New API controller for left-nav menu management. |
| web/Areas/CMS/Controllers/CMSContentController.cs | Rebuilt content API controller delegating to services; adds history/diff endpoints and admin-only permanent delete. |
| test/CMS/CMSLinkCollectionLinksTests.cs | New tests for link CRUD, ordering, grouping, and tag save behavior. |
| test/CMS/CMSLinkCollectionControllerTests.cs | New tests for collection CRUD and tag-category behaviors. |
| test/CMS/CmsLeftNavServiceTests.cs | New tests for left-nav service CRUD + batch item replace behavior. |
| test/CMS/CmsLeftNavDisplayTests.cs | New tests for left-nav display permission filtering parity. |
| test/CMS/CMSLeftNavControllerTests.cs | New controller wiring tests for left-nav endpoints and status mapping. |
| test/CMS/CMSContentControllerTests.cs | New controller wiring tests for content endpoints (including history/diff) and admin-gated permanent delete. |
| test/CMS/CmsContentBlockServiceTests.cs | New tests for content-block service filtering, history semantics, diffs, concurrency, and delete/restore behavior. |
This was referenced Jul 3, 2026
Contributor
📝 WalkthroughWalkthroughCMS content-block handling now uses DTO/service endpoints, left-nav management adds a dedicated service/controller path, and link-collection endpoints change permission constants, deletion order, and validation. Tests cover the new controller and service flows across all three areas. ChangesContent Block Service and Controller
Estimated code review effort: 4 (Complex) | ~75 minutes Left-Nav Menu Service, Controller, and Permission Filtering
Estimated code review effort: 4 (Complex) | ~60 minutes Link Collection Controller Updates
Estimated code review effort: 3 (Moderate) | ~30 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/Areas/CMS/Controllers/CMSLinkCollectionLinks.cs (2)
99-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPermission strings still hardcoded — inconsistent with sibling controller.
CMSLinkCollectionController.cswas updated to useCmsPermissions.ManageContentBlocks, but this file still hardcodes"SVMSecure.CMS.ManageContentBlocks"in five places. Same string, but future changes to the constant won't propagate here.♻️ Proposed fix
+using Viper.Areas.CMS.Constants; ... - [Permission(Allow = "SVMSecure.CMS.ManageContentBlocks")] + [Permission(Allow = CmsPermissions.ManageContentBlocks)](repeat for all five occurrences)
Also applies to: 132-132, 157-157, 179-179, 210-210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Areas/CMS/Controllers/CMSLinkCollectionLinks.cs` at line 99, Replace the hardcoded permission string in CMSLinkCollectionLinks so it uses the shared CmsPermissions.ManageContentBlocks constant instead of "SVMSecure.CMS.ManageContentBlocks". Update all five Permission attributes in this controller (including the one on the visible action and the other occurrences in the class) to match the sibling CMSLinkCollectionController pattern and keep permission changes centralized.
198-202: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard each
LinkIdlookup.updateDtocan still carry an unknown or duplicateLinkIdwith the same item count, solinks.First(...)can throw or update the wrong row. UseFirstOrDefault(or a lookup) and returnBadRequestwhen an id is missing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Areas/CMS/Controllers/CMSLinkCollectionLinks.cs` around lines 198 - 202, The link update loop in CMSLinkCollectionLinks is doing an unsafe `links.First(...)` lookup for each `LinkId`, which can throw on unknown ids or silently target the wrong row when duplicates are present. Update the `foreach` logic to use a safe lookup in the `updateDto` processing path (for example, `FirstOrDefault` or a keyed lookup), and validate that every `li.LinkId` exists in `links` before applying `SortOrder`. If any id is missing, return `BadRequest` instead of continuing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/CMS/CmsLeftNavDisplayTests.cs`:
- Around line 66-96: Add test coverage for the anonymous-user path in
LeftNavMenu.GetLeftNavMenus: both existing tests use a signed-in AaudUser, so
they miss the currentUser == null branch. Add a new test alongside
GetLeftNavMenus_HidesItemsTheUserLacksPermissionFor and
GetLeftNavMenus_ReturnsAllItems_WhenFilteringDisabled that sets
_userHelper.GetCurrentUser() to return null/ReturnsNull and asserts
permissionless items are hidden for an anonymous caller, using the existing
SeedMixedMenuAsync and DataLeftNav setup.
In `@test/CMS/CMSLinkCollectionLinksTests.cs`:
- Around line 265-306: Add a test for UpdateLinkOrder that uses the correct
number of updates but includes an unknown LinkId, mirroring
UpdateLinkCollectionTagCategoryOrder_RejectsUnknownCategoryId. In
CMSLinkCollectionLinksTests, extend the UpdateLinkOrder coverage with a
count-matching but invalid LinkId case and assert the controller does not crash;
this should expose the .First() failure path in UpdateLinkOrder on the
CMSLinkCollectionLinksController flow.
In `@web/Areas/CMS/Controllers/CMSLinkCollectionController.cs`:
- Around line 223-230: Replace the hand-written ToTagCategoryDto helper with a
Mapperly static partial mapper to match the repo convention. Move the
LinkCollectionTagCategory to LinkCollectionTagCategoryDto mapping into a
dedicated mapper class for this area, using [Mapper(RequiredMappingStrategy =
RequiredMappingStrategy.None)] like CmsContentBlockMapper. Then update
CMSLinkCollectionController to use the generated mapper instead of manually
constructing the DTO.
- Around line 102-113: The delete flow in CMSLinkCollectionController currently
queries Links twice using the same collection id, once for collectionLinkIds and
again for RemoveRange, causing a redundant SELECT. Update the removal logic
around collectionLinkIds so the Links set is materialized once and reused for
both LinkTags cleanup and link deletion, while keeping the existing atomic
delete order in the same controller action.
In `@web/Areas/CMS/Models/CmsContentBlockMapper.cs`:
- Around line 10-24: The ContentBlock file projection is duplicated between
CmsContentBlockMapper.ToDto and CmsContentBlockService.GetContentBlocksAsync.
Extract the shared FileGuid/FriendlyName/Url mapping into a reusable helper on
CmsContentBlockMapper, such as a public ToFileDto method, and update both ToDto
and GetContentBlocksAsync to call it so the file-mapping logic lives in one
place.
In `@web/Areas/CMS/Services/CmsContentBlockService.cs`:
- Around line 160-193: The file projection in
CmsContentBlockService.GetContentBlocksAsync is duplicating
CmsContentBlockMapper.ToDto by manually building ContentBlockFileDto with
FileGuid, FriendlyName, and Url via Data.CMS.GetFriendlyURL. Refactor this block
to reuse the existing mapper/DTO projection instead of re-implementing the file
mapping, so the file shape and URL-generation logic live in one place and stay
consistent.
- Around line 212-261: Add a unique constraint for ContentBlock.FriendlyName so
duplicate blocks cannot be created even if AssertFriendlyNameUniqueAsync passes
concurrently. Update the create/update flow in CreateContentBlockAsync and
UpdateContentBlockAsync to rely on the database constraint as the final source
of truth, and keep the pre-check only for user-friendly validation. If the save
fails due to a duplicate key, translate that into a clear validation error for
FriendlyName.
In `@web/Areas/CMS/Services/CmsLeftNavService.cs`:
- Around line 44-73: GetMenusAsync is loading full LeftNavMenu entity graphs
with Include/ThenInclude/AsSplitQuery and then mapping them to DTOs afterward,
which is unnecessary for this read-only query. Update
CmsLeftNavService.GetMenusAsync to project directly to LeftNavMenuDto inside the
IQueryable with Select, keeping AsNoTracking and the existing filters/order, and
remove the Include/ThenInclude/AsSplitQuery chain since EF can resolve needed
navigations in the projection.
- Around line 95-111: UpdateMenuAsync currently overwrites the record without
checking whether the caller is working from a stale copy. Make
LeftNavMenuAddEdit round-trip ModifiedOn (or another concurrency token) and,
inside CmsLeftNavService.UpdateMenuAsync, compare the incoming value against the
loaded menu before ApplyMenuFields/save; if it does not match, reject the update
with a 409-style concurrency response consistent with the content-block flow.
---
Outside diff comments:
In `@web/Areas/CMS/Controllers/CMSLinkCollectionLinks.cs`:
- Line 99: Replace the hardcoded permission string in CMSLinkCollectionLinks so
it uses the shared CmsPermissions.ManageContentBlocks constant instead of
"SVMSecure.CMS.ManageContentBlocks". Update all five Permission attributes in
this controller (including the one on the visible action and the other
occurrences in the class) to match the sibling CMSLinkCollectionController
pattern and keep permission changes centralized.
- Around line 198-202: The link update loop in CMSLinkCollectionLinks is doing
an unsafe `links.First(...)` lookup for each `LinkId`, which can throw on
unknown ids or silently target the wrong row when duplicates are present. Update
the `foreach` logic to use a safe lookup in the `updateDto` processing path (for
example, `FirstOrDefault` or a keyed lookup), and validate that every
`li.LinkId` exists in `links` before applying `SortOrder`. If any id is missing,
return `BadRequest` instead of continuing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6056fa5e-e25f-4cbf-a133-e7a09e767771
📒 Files selected for processing (19)
test/CMS/CMSContentControllerTests.cstest/CMS/CMSLeftNavControllerTests.cstest/CMS/CMSLinkCollectionControllerTests.cstest/CMS/CMSLinkCollectionLinksTests.cstest/CMS/CmsContentBlockServiceTests.cstest/CMS/CmsLeftNavDisplayTests.cstest/CMS/CmsLeftNavServiceTests.csweb/Areas/CMS/Controllers/CMSContentController.csweb/Areas/CMS/Controllers/CMSLeftNavController.csweb/Areas/CMS/Controllers/CMSLinkCollectionController.csweb/Areas/CMS/Controllers/CMSLinkCollectionLinks.csweb/Areas/CMS/Data/LeftNavMenu.csweb/Areas/CMS/Models/CMSBlockAddEdit.csweb/Areas/CMS/Models/CmsContentBlockMapper.csweb/Areas/CMS/Models/DTOs/ContentBlockDto.csweb/Areas/CMS/Models/DTOs/LeftNavDtos.csweb/Areas/CMS/Services/CmsContentBlockService.csweb/Areas/CMS/Services/CmsLeftNavService.csweb/Areas/CMS/Services/CmsNavMenu.cs
Contributor
Author
|
@coderabbitai review |
rlorenzo
changed the title
rlorenzo
force-pushed
the
VPR-59-cms-r2-content-nav-backend
branch
from
July 10, 2026 07:57
1b1df26 to
1ab4737
Compare
rlorenzo
force-pushed
the
VPR-59-cms-r1-files-backend
branch
from
July 10, 2026 08:28
5b15989 to
8b2cee3
Compare
rlorenzo
force-pushed
the
VPR-59-cms-r2-content-nav-backend
branch
2 times, most recently
from
July 10, 2026 15:11
0ff3092 to
1e7597e
Compare
rlorenzo
force-pushed
the
VPR-59-cms-r2-content-nav-backend
branch
from
July 17, 2026 05:11
1e7597e to
d364f87
Compare
Comment on lines
+185
to
+204
| foreach (var requested in items) | ||
| { | ||
| if (requested.LeftNavItemId > 0 && existingById.TryGetValue(requested.LeftNavItemId, out var existing)) | ||
| { | ||
| existing.MenuItemText = requested.MenuItemText; | ||
| existing.IsHeader = requested.IsHeader; | ||
| existing.Url = requested.IsHeader ? null : requested.Url; | ||
| existing.DisplayOrder = order; | ||
| ApplyItemPermissionDeltas(existing, CleanList(requested.Permissions)); | ||
| } | ||
| else | ||
| { | ||
| var newItem = new LeftNavItem | ||
| { | ||
| LeftNavMenuId = leftNavMenuId, | ||
| MenuItemText = requested.MenuItemText, | ||
| IsHeader = requested.IsHeader, | ||
| Url = requested.IsHeader ? null : requested.Url, | ||
| DisplayOrder = order | ||
| }; |
rlorenzo
force-pushed
the
VPR-59-cms-r2-content-nav-backend
branch
from
July 17, 2026 07:50
d364f87 to
c783cfc
Compare
Comment on lines
+28
to
+31
| if (canManageBlocks || canCreateBlocks || canManageFiles || canManageNav) | ||
| { | ||
| nav.Add(new NavMenuItem { MenuItemText = "Home", MenuItemURL = "Home", IndentLevel = 1 }); | ||
| } |
Comment on lines
+38
to
+48
| nav.Add(new NavMenuItem { MenuItemText = "Manage Content Blocks", MenuItemURL = "ManageContentBlocks", IndentLevel = 1 }); | ||
| } | ||
| if (canCreateBlocks) | ||
| { | ||
| nav.Add(new NavMenuItem { MenuItemText = "Add Content Block", MenuItemURL = "ManageContentBlocks/Edit", IndentLevel = 1 }); | ||
| } | ||
| if (canManageBlocks) | ||
| { | ||
| nav.Add(new NavMenuItem { MenuItemText = "Edit History", MenuItemURL = "ManageContentBlocks/History", IndentLevel = 1 }); | ||
| nav.Add(new NavMenuItem { MenuItemText = "Manage Link Collections", MenuItemURL = "ManageLinkCollections", IndentLevel = 1 }); | ||
| } |
Comment on lines
+53
to
+57
| nav.Add(new NavMenuItem { MenuItemText = "Files", IsHeader = true }); | ||
| nav.Add(new NavMenuItem { MenuItemText = "Manage Files", MenuItemURL = "ManageFiles", IndentLevel = 1 }); | ||
| nav.Add(new NavMenuItem { MenuItemText = "Add File", MenuItemURL = "ManageFiles?upload=1", IndentLevel = 1 }); | ||
| nav.Add(new NavMenuItem { MenuItemText = "Audit Trail", MenuItemURL = "ManageFiles/Audit", IndentLevel = 1 }); | ||
| } |
Comment on lines
+61
to
64
| nav.Add(new NavMenuItem { MenuItemText = "Left Navigation Menus", IsHeader = true }); | ||
| nav.Add(new NavMenuItem { MenuItemText = "Manage Left-Nav Menus", MenuItemURL = "ManageLeftNav", IndentLevel = 1 }); | ||
| nav.Add(new NavMenuItem { MenuItemText = "Add Left-Nav Menu", MenuItemURL = "ManageLeftNav?add=1", IndentLevel = 1 }); | ||
| } |
rlorenzo
force-pushed
the
VPR-59-cms-r2-content-nav-backend
branch
from
July 17, 2026 08:13
c783cfc to
d73c63f
Compare
Comment on lines
+101
to
+105
| // status: 1 = active only. A public display endpoint must never serve soft-deleted | ||
| // blocks (passing null would include DeletedOn != null rows). | ||
| var blocks = new Data.CMS(_context, _rapsContext, _sanitizerService) | ||
| .GetContentBlocksAllowed(null, friendlyName, null, null, null, null, null, 1)?.ToList(); | ||
| if (blocks == null || blocks.Count == 0) |
Comment on lines
+268
to
+273
| var existing = item.LeftNavItemToPermissions.ToList(); | ||
| foreach (var p in existing.Where(p => !requested.Contains(p.Permission, StringComparer.OrdinalIgnoreCase))) | ||
| { | ||
| item.LeftNavItemToPermissions.Remove(p); | ||
| _context.Remove(p); | ||
| } |
Comment on lines
+603
to
+608
| var existing = block.ContentBlockToPermissions.ToList(); | ||
| foreach (var cbp in existing.Where(cbp => !requested.Contains(cbp.Permission, StringComparer.OrdinalIgnoreCase))) | ||
| { | ||
| block.ContentBlockToPermissions.Remove(cbp); | ||
| _context.Remove(cbp); | ||
| } |
Comment on lines
+622
to
+627
| var existing = block.ContentBlockToFiles.ToList(); | ||
| foreach (var cbf in existing.Where(cbf => !requested.Contains(cbf.FileGuid))) | ||
| { | ||
| block.ContentBlockToFiles.Remove(cbf); | ||
| _context.Remove(cbf); | ||
| } |
…ns API Slice 2/3 of the restacked CMS migration (tip file states). Content blocks with version history, diffs, and the 409 stale-edit guard; left-nav menu and item management with display-parity filtering; link-collection fixes; and their unit tests. Stacks on the files backend; the management SPA follows in slice 3.
…th a null URL - Header items are persisted with a null Url, but NavMenuItem's LeftNavItem constructor passed that null straight through to the non-nullable MenuItemURL; the left-nav view's header/spacer branch checks MenuItemURL against "" specifically, so a null took the link branch instead and failed resolving a null URL - Reject saving a non-header item with a blank Url too, since it would otherwise save successfully but render as an inert, non-clickable spacer row
- GetContentBlocksAllowed already returns a materialized list; the extra ToList() copied it again. FirstOrDefault replaces the null/Count/indexer dance with a single null check.
- The removal pass checked List.Contains inside a loop over existing permissions; the add pass right below it already builds a HashSet for the same kind of check. Do the same for removal.
rlorenzo
force-pushed
the
VPR-59-cms-r2-content-nav-backend
branch
from
July 17, 2026 08:45
c839741 to
62c4d96
Compare
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.
Slice 2 of 4 (stacks on #251; the diff shows only this slice). Built from the CI-verified branch tip — see #251 for why the earlier 6-PR stack (#245-#250) was replaced.
Scope — content/nav backend
CmsContentBlockService+ rebuiltCMSContentController— server-paged filterable list, version history with htmldiff-based sanitized diffs, restore, attached-file GUID deltas, ModifiedOn 409 concurrency guard, content-only PATCH, admin-gated permanent delete, and a minimal public DTO on the anonymouscontent/fn/{name}display endpoint (no editor login ids, permission names, placement metadata, or file keys).CmsLeftNavService+CMSLeftNavController— menu CRUD, batch item save with ordering and per-item permissions, unknown-id rejection, empty-header spacer rows preserved (legacy parity), display filtering that hides permission-less items from anonymous requests but shows them to any signed-in user (legacy parity).SafeUrlvalidation.Stack: #251 (files backend) → this PR → management SPA.