VPR-59 [2/3] CMS migration: content blocks, left nav, link collections (backend)#252
Open
rlorenzo wants to merge 22 commits into
Open
VPR-59 [2/3] CMS migration: content blocks, left nav, link collections (backend)#252rlorenzo wants to merge 22 commits into
rlorenzo wants to merge 22 commits into
Conversation
…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.
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
- Invariant lowercasing for friendly-name normalization; the content-block uniqueness check is now case-insensitive like its left-nav sibling (matches SQL Server default collation)
Same 500-row upper bound as the files domain.
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
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: 3
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/Services/CmsContentBlockService.cs (1)
160-188: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the unused
AllowPublicAccessprojection.ContentBlockFileDtoonly carriesFileGuid,FriendlyName, andUrl, so this field is fetched and then dropped.🤖 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/Services/CmsContentBlockService.cs` around lines 160 - 188, The query in CmsContentBlockService is projecting AllowPublicAccess from ContentBlockToFiles but the value is never used when building ContentBlockDto. Remove that unused field from the Files projection in the CmsContentBlockService mapping, keeping only the properties needed by CmsContentBlockMapper.ToFileDto and ContentBlockFileDto.web/Areas/CMS/Controllers/CMSLinkCollectionLinks.cs (1)
99-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMigrate hardcoded permission strings to
CmsPermissions.ManageContentBlocks.
CMSLinkCollectionController.cswas updated in this PR to use theCmsPermissions.ManageContentBlocksconstant, but this file still hardcodes"SVMSecure.CMS.ManageContentBlocks"in five places. Using the constant here too avoids future permission-string drift between the two controllers guarding the same resource tree.♻️ Proposed fix
+using Viper.Areas.CMS.Constants; ... - [Permission(Allow = "SVMSecure.CMS.ManageContentBlocks")] + [Permission(Allow = CmsPermissions.ManageContentBlocks)]Apply to all five occurrences (lines 99, 132, 157, 179, 214).
Also applies to: 132-132, 157-157, 179-179, 214-214
🤖 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, The permission attribute in CMSLinkCollectionLinks still hardcodes the manage-content-blocks string in multiple actions, while CMSLinkCollectionController already uses the shared CmsPermissions.ManageContentBlocks constant. Update all five [Permission] usages in CMSLinkCollectionLinks to reference CmsPermissions.ManageContentBlocks so both controllers stay aligned and avoid permission-string drift.
🤖 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/CMSLinkCollectionLinksTests.cs`:
- Around line 307-324: Add a test in CMSLinkCollectionLinksTests for
UpdateLinkOrder that mirrors the existing tag-category duplicate-id coverage:
create two updates that both use the same LinkId while still matching the total
count, call CMSLinkCollectionController.UpdateLinkOrder, and assert
BadRequestObjectResult. Use SeedCollectionAsync, SeedLinkAsync, and the
UpdateLinkOrderDto list to target the duplicate-id guard in the UpdateLinkOrder
path.
In `@web/Areas/CMS/Services/CmsLeftNavService.cs`:
- Around line 233-249: The AssertNotStale logic in CmsLeftNavService is
duplicated from CmsContentBlockService, so extract the shared concurrency checks
into a common helper and have both services call it instead of maintaining two
copies. Move the identical AssertNotStale behavior (including the missing-stamp
ArgumentException and stale-data CmsConcurrencyException checks) into a shared
static utility such as CmsConcurrencyHelpers, and do the same for the duplicated
CleanList implementation so both services reuse the same helper methods. Keep
the service-specific call sites in CmsLeftNavService and CmsContentBlockService
wired to the shared helpers to avoid future drift.
- Line 103: SaveItemsAsync currently mutates left-nav items without a stale-copy
check, so concurrent batch saves can overwrite changes. Update the
SaveItemsAsync flow in CmsLeftNavService to accept and pass through the menu’s
LastModifiedOn stamp alongside the existing List<LeftNavItemEdit> payload, then
call AssertNotStale before any item mutations. Use the existing request/handler
path that already maps InvalidOperationException to 409 so the new guard is
enforced consistently.
---
Outside diff comments:
In `@web/Areas/CMS/Controllers/CMSLinkCollectionLinks.cs`:
- Line 99: The permission attribute in CMSLinkCollectionLinks still hardcodes
the manage-content-blocks string in multiple actions, while
CMSLinkCollectionController already uses the shared
CmsPermissions.ManageContentBlocks constant. Update all five [Permission] usages
in CMSLinkCollectionLinks to reference CmsPermissions.ManageContentBlocks so
both controllers stay aligned and avoid permission-string drift.
In `@web/Areas/CMS/Services/CmsContentBlockService.cs`:
- Around line 160-188: The query in CmsContentBlockService is projecting
AllowPublicAccess from ContentBlockToFiles but the value is never used when
building ContentBlockDto. Remove that unused field from the Files projection in
the CmsContentBlockService mapping, keeping only the properties needed by
CmsContentBlockMapper.ToFileDto and ContentBlockFileDto.
🪄 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: cf817478-6923-4f38-a1c9-9bab24725fff
📒 Files selected for processing (11)
test/CMS/CMSLinkCollectionControllerTests.cstest/CMS/CMSLinkCollectionLinksTests.cstest/CMS/CmsLeftNavDisplayTests.cstest/CMS/CmsLeftNavServiceTests.csweb/Areas/CMS/Controllers/CMSLeftNavController.csweb/Areas/CMS/Controllers/CMSLinkCollectionController.csweb/Areas/CMS/Controllers/CMSLinkCollectionLinks.csweb/Areas/CMS/Models/CmsContentBlockMapper.csweb/Areas/CMS/Models/DTOs/LeftNavDtos.csweb/Areas/CMS/Services/CmsContentBlockService.csweb/Areas/CMS/Services/CmsLeftNavService.cs
…dits - the items endpoint took only the item list, so two editors could silently overwrite each other's order and permissions; it now takes the menu's LastModifiedOn stamp and 409s when stale, matching the settings save - content-block and left-nav services now use the shared stale-edit and permission-list helpers from r1 - cover duplicate link ids in the reorder guard
The list-view file projection fetched AllowPublicAccess the DTO mapping never used, and the GetContentBlockByFn missing-case test was async without an await (CS1998).
- LeftNavItemsSave.Items is JsonRequired and null-guarded: an omitted property would have silently deleted every menu item, and an explicit null would have been a 500 - content PATCH and history-diff DTOs mark Content [Required] (AllowEmptyStrings) so a JSON null is a 400 instead of a downstream NullReferenceException
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/Areas/CMS/Services/CmsContentBlockService.cs (1)
125-141: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a unique tiebreaker to the paged sort. Paging this split query with a non-deterministic
ORDER BYcan return unstable pages and mismatch relatedPermissions/Files. AppendThenBy(b => b.ContentBlockId)to every branch beforeSkip/Take.🤖 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/Services/CmsContentBlockService.cs` around lines 125 - 141, The paged sort in CmsContentBlockService is not fully deterministic, which can cause unstable pagination and mismatched related data when the query is split. Update the sort expression used before Skip/Take so every branch in the sort switch appends a unique tiebreaker with ThenBy(b => b.ContentBlockId), including the descending cases, to keep page ordering stable.web/Areas/CMS/Services/CmsLeftNavService.cs (2)
217-219: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant reload after
SaveItemsAsyncsave.
return await GetMenuAsync(leftNavMenuId, ct)re-queries the menu with items/permissions from scratch. The already-loaded, mutatedmenu(adds/removes/updates applied in-memory, and EF populates new item IDs onSaveChangesAsync) can be mapped directly, same asUpdateMenuAsyncdoes at Line 112 (return ToDto(menu)).⚡ Proposed fix
await _context.SaveChangesAsync(ct); - return await GetMenuAsync(leftNavMenuId, ct); + return ToDto(menu);🤖 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/Services/CmsLeftNavService.cs` around lines 217 - 219, The SaveItemsAsync flow in CmsLeftNavService is doing a redundant reload by calling GetMenuAsync after SaveChangesAsync, even though the in-memory menu entity has already been mutated and EF has populated new IDs; update the method to return the mapped menu directly, matching the existing UpdateMenuAsync pattern by using ToDto(menu) instead of re-querying with GetMenuAsync.
224-241: 🗄️ Data Integrity & Integration | 🔵 TrivialFriendly-name uniqueness check is check-then-act, no DB backstop.
AssertFriendlyNameUniqueAsyncguards Create/Update in application code only; without a unique index/constraint onFriendlyName, two concurrent creates/renames could both pass the check and persist duplicate names, whichLayoutController'sFirstOrDefaultresolution (per the file's own comment) depends on being unique.Given the bounded, admin-only nature of this table this is low-probability, but a unique index would close the gap cheaply.
🤖 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/Services/CmsLeftNavService.cs` around lines 224 - 241, The friendly-name validation in AssertFriendlyNameUniqueAsync is only an application-level check, so concurrent Create/Update requests can still persist duplicates. Add a database-level unique constraint/index on the FriendlyName column (while preserving the null-handling behavior used by CmsLeftNavService), and keep the existing pre-check as a user-friendly early failure. Ensure the fix is applied alongside the LeftNavMenus model/migration so LayoutController’s FirstOrDefault lookup can safely rely on uniqueness.
🤖 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.
Outside diff comments:
In `@web/Areas/CMS/Services/CmsContentBlockService.cs`:
- Around line 125-141: The paged sort in CmsContentBlockService is not fully
deterministic, which can cause unstable pagination and mismatched related data
when the query is split. Update the sort expression used before Skip/Take so
every branch in the sort switch appends a unique tiebreaker with ThenBy(b =>
b.ContentBlockId), including the descending cases, to keep page ordering stable.
In `@web/Areas/CMS/Services/CmsLeftNavService.cs`:
- Around line 217-219: The SaveItemsAsync flow in CmsLeftNavService is doing a
redundant reload by calling GetMenuAsync after SaveChangesAsync, even though the
in-memory menu entity has already been mutated and EF has populated new IDs;
update the method to return the mapped menu directly, matching the existing
UpdateMenuAsync pattern by using ToDto(menu) instead of re-querying with
GetMenuAsync.
- Around line 224-241: The friendly-name validation in
AssertFriendlyNameUniqueAsync is only an application-level check, so concurrent
Create/Update requests can still persist duplicates. Add a database-level unique
constraint/index on the FriendlyName column (while preserving the null-handling
behavior used by CmsLeftNavService), and keep the existing pre-check as a
user-friendly early failure. Ensure the fix is applied alongside the
LeftNavMenus model/migration so LayoutController’s FirstOrDefault lookup can
safely rely on uniqueness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3d731ff-c099-4a96-b046-f0af41c2af08
📒 Files selected for processing (9)
test/CMS/CMSContentControllerTests.cstest/CMS/CMSLeftNavControllerTests.cstest/CMS/CMSLinkCollectionLinksTests.cstest/CMS/CmsLeftNavServiceTests.csweb/Areas/CMS/Controllers/CMSContentController.csweb/Areas/CMS/Controllers/CMSLeftNavController.csweb/Areas/CMS/Models/DTOs/LeftNavDtos.csweb/Areas/CMS/Services/CmsContentBlockService.csweb/Areas/CMS/Services/CmsLeftNavService.cs
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 3 (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.