From 26534965303f2e0a90ba557256ff1de314bf7234 Mon Sep 17 00:00:00 2001 From: yv01p Date: Tue, 23 Dec 2025 03:06:17 +0000 Subject: [PATCH] docs: update Phase 3 implementation plan to v1.3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applied all fixes from critical review v3: - Issue 3.1: Added input validation to GetVersionIds - Issue 3.2: Removed redundant WriteLock in Rollback - Issue 3.3: Added audit entry for prior versions in DeleteVersion - Issue 3.4: Fixed Rollback return type (compilation fix) - Issue 3.6: Added clarifying comment for cancellation behavior πŸ€– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- ...tservice-refactor-phase3-implementation.md | 1557 +++++++++++++++++ 1 file changed, 1557 insertions(+) create mode 100644 docs/plans/2025-12-23-contentservice-refactor-phase3-implementation.md diff --git a/docs/plans/2025-12-23-contentservice-refactor-phase3-implementation.md b/docs/plans/2025-12-23-contentservice-refactor-phase3-implementation.md new file mode 100644 index 0000000000..2b5828f728 --- /dev/null +++ b/docs/plans/2025-12-23-contentservice-refactor-phase3-implementation.md @@ -0,0 +1,1557 @@ +# ContentService Version Operations Extraction - Phase 3 Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Extract version operations (GetVersion, GetVersions, GetVersionsSlim, GetVersionIds, Rollback, DeleteVersions, DeleteVersion) from ContentService into a dedicated `IContentVersionOperationService` interface and `ContentVersionOperationService` implementation. + +**Architecture:** `ContentVersionOperationService` implements `IContentVersionOperationService` and inherits from `ContentServiceBase`. The existing `ContentService` facade will delegate to `ContentVersionOperationService` for version operations. The new service handles synchronous version operations extracted from `IContentService`, separate from the existing async `IContentVersionService` which provides API-layer functionality. + +**Tech Stack:** .NET 10.0, NUnit, Microsoft.Extensions.DependencyInjection + +--- + +## Plan Version History + +| Version | Date | Author | Changes | +|---------|------|--------|---------| +| 1.0 | 2025-12-23 | Claude | Initial plan creation | +| 1.1 | 2025-12-23 | Claude | Applied critical review fixes (Issues 2.1-2.5, 3.2-3.4) | +| 1.2 | 2025-12-23 | Claude | Applied critical review v2 fixes (Issues 2.1-2.2, 3.1-3.4) | +| 1.3 | 2025-12-23 | Claude | Applied critical review v3 fixes (Issues 3.1-3.4, 3.6) | + +--- + +## Changes in Version 1.1 + +**Summary:** Applied fixes from critical implementation review (2025-12-23-contentservice-refactor-phase3-implementation-critical-review-1.md) + +### Critical Fixes Applied: + +1. **Issue 2.1 - Rollback TOCTOU Race Condition**: Consolidated Rollback into a single scope to eliminate race condition between read and write operations. Removed `PerformRollback` helper method; all operations now occur within one scope. + +2. **Issue 2.2 - Rollback Error Handling**: Added try-catch around save operation with proper error logging ("was unable to rollback") and conditional notification (only fires `ContentRolledBackNotification` on success). + +3. **Issue 2.3 - GetVersionIds Missing ReadLock**: Added `scope.ReadLock(Constants.Locks.ContentTree)` for consistency with other read operations. Added code comment explaining this is a bug fix. + +4. **Issue 2.4 - DeleteVersion Nested Scope**: Changed `deletePriorVersions` branch to use `DocumentRepository.GetVersion()` and `DocumentRepository.DeleteVersions()` directly instead of calling the service methods that create nested scopes. + +5. **Issue 2.5 - Flaky Test Pattern**: Replaced `Thread.Sleep(100)` with deterministic date comparison using actual version update dates. + +### Minor Improvements Applied: + +6. **Issue 3.2 - Cancellation Test**: Added `Rollback_WhenNotificationCancelled_ReturnsCancelledResult` test. + +7. **Issue 3.3 - Published Version Protection Test**: Added `DeleteVersion_PublishedVersion_DoesNotDelete` test. + +8. **Issue 3.4 - Interface Documentation**: Enhanced `GetVersionIds` XML documentation to specify behavior for non-existent content and maxRows edge cases. + +--- + +## Changes in Version 1.2 + +**Summary:** Applied fixes from critical implementation review v2 (2025-12-23-contentservice-refactor-phase3-implementation-critical-review-2.md) + +### Critical Fixes Applied: + +1. **Issue 2.1 - Rollback Notification Preservation**: Changed Rollback to use `CrudService.Save` instead of `DocumentRepository.Save` to preserve `ContentSavingNotification` and `ContentSavedNotification` firing. This maintains backward compatibility with existing notification handlers and ensures proper validation and audit trail. + +2. **Issue 2.2 - DeleteVersion Double-Notification Preservation**: When `deletePriorVersions=true`, the method now fires notifications for both the prior versions deletion AND the specific version deletion, matching original behavior. + +### Minor Improvements Applied: + +3. **Issue 3.1 - Simplified Lock Acquisition**: Restructured DeleteVersion to acquire WriteLock once at the start instead of multiple times, avoiding lock upgrade pattern. + +4. **Issue 3.2 - Test Notification Registration**: Fixed the cancellation test to use the correct integration test pattern with `CustomTestSetup` and notification handler registration. + +5. **Issue 3.3 - CrudService Dependency**: Added `IContentCrudService` as a constructor dependency to support proper save operation in Rollback. + +6. **Issue 3.4 - Publish Method Signature**: Updated test to use correct `SaveAndPublish` method signature. + +--- + +## Changes in Version 1.3 + +**Summary:** Applied fixes from critical implementation review v3 (2025-12-23-contentservice-refactor-phase3-implementation-critical-review-3.md) + +### Must Fix Applied: + +1. **Issue 3.4 - Rollback Return Type**: Fixed `OperationResult` to `OperationResult` to match `IContentCrudService.Save` signature and avoid compilation error. + +### Should Fix Applied: + +2. **Issue 3.1 - GetVersionIds Input Validation**: Added `ArgumentOutOfRangeException` for `maxRows <= 0` to match interface documentation contract. + +3. **Issue 3.3 - Audit Gap in DeleteVersion**: Added audit entry for prior versions deletion when `deletePriorVersions=true` to preserve original two-audit-entry behavior. + +### Consider Applied: + +4. **Issue 3.2 - Redundant WriteLock in Rollback**: Removed explicit `WriteLock` before `CrudService.Save` since CrudService handles its own locking internally. + +5. **Issue 3.6 - Prior Versions Cancellation Comment**: Added clarifying comment documenting that specific version deletion proceeds even if prior versions notification is cancelled (matching original behavior). + +--- + +## Naming Decision + +**Conflict Resolution:** An `IContentVersionService` already exists in the codebase (`src/Umbraco.Core/Services/ContentVersionService.cs`). This is an async API-layer service that provides version cleanup, paged version listing, and rollback via `IContentService`. + +**Decision:** Name the new interface `IContentVersionOperationService` to avoid collision, following the pattern established for `IContentPublishOperationService`. The existing `IContentVersionService` remains unchanged as the higher-level async API orchestrator. + +--- + +## Phase 3 Overview + +Phase 3 extracts version-related operations following patterns established in Phases 1-2: + +1. **IContentVersionOperationService** - Interface for version operations (7 methods) +2. **ContentVersionOperationService** - Implementation inheriting from `ContentServiceBase` +3. **DI Registration** - Register new service alongside existing services +4. **ContentService Delegation** - Update facade to delegate to new service + +The extraction reduces `ContentService` complexity by ~150 lines. + +--- + +## Methods to Extract + +| Method | Signature | Notifications | +|--------|-----------|---------------| +| `GetVersion` | `IContent? GetVersion(int versionId)` | None (read-only) | +| `GetVersions` | `IEnumerable GetVersions(int id)` | None (read-only) | +| `GetVersionsSlim` | `IEnumerable GetVersionsSlim(int id, int skip, int take)` | None (read-only) | +| `GetVersionIds` | `IEnumerable GetVersionIds(int id, int maxRows)` | None (read-only) | +| `Rollback` | `OperationResult Rollback(int id, int versionId, string culture, int userId)` | `ContentRollingBackNotification`, `ContentRolledBackNotification` | +| `DeleteVersions` | `void DeleteVersions(int id, DateTime versionDate, int userId)` | `ContentDeletingVersionsNotification`, `ContentDeletedVersionsNotification` | +| `DeleteVersion` | `void DeleteVersion(int id, int versionId, bool deletePriorVersions, int userId)` | `ContentDeletingVersionsNotification`, `ContentDeletedVersionsNotification` | + +--- + +## Task 1: Create IContentVersionOperationService Interface + +**Files:** +- Create: `src/Umbraco.Core/Services/IContentVersionOperationService.cs` + +**Step 1: Create the interface file** + +```csharp +// src/Umbraco.Core/Services/IContentVersionOperationService.cs +using Umbraco.Cms.Core.Models; + +namespace Umbraco.Cms.Core.Services; + +/// +/// Service for content version operations (retrieving versions, rollback, deleting versions). +/// +/// +/// +/// Implementation Note: Do not implement this interface directly. +/// Instead, inherit from which provides required +/// infrastructure (scoping, repository access, auditing). Direct implementation +/// without this base class will result in missing functionality. +/// +/// +/// This interface is part of the ContentService refactoring initiative (Phase 3). +/// It extracts version operations into a focused, testable service. +/// +/// +/// Note: This interface provides synchronous version operations +/// extracted from . For async API-layer version operations, +/// see which orchestrates via this service. +/// +/// +/// Versioning Policy: This interface follows additive-only changes. +/// New methods may be added with default implementations. Existing methods will not +/// be removed or have signatures changed without a 2 major version deprecation period. +/// +/// +/// Version History: +/// +/// v1.0 (Phase 3): Initial interface with GetVersion, GetVersions, Rollback, DeleteVersions operations +/// +/// +/// +/// 1.0 +public interface IContentVersionOperationService : IService +{ + #region Version Retrieval + + /// + /// Gets a specific version of content by version id. + /// + /// The version id to retrieve. + /// The content version, or null if not found. + IContent? GetVersion(int versionId); + + /// + /// Gets all versions of a content item. + /// + /// The content id. + /// All versions of the content, ordered by version date descending. + IEnumerable GetVersions(int id); + + /// + /// Gets a paged subset of versions for a content item. + /// + /// The content id. + /// Number of versions to skip. + /// Number of versions to take. + /// Paged versions of the content, ordered by version date descending. + IEnumerable GetVersionsSlim(int id, int skip, int take); + + /// + /// Gets version ids for a content item, ordered with latest first. + /// + /// The content id. + /// Maximum number of version ids to return. Must be positive. + /// Version ids ordered with latest first. Empty if content not found. + /// Thrown if maxRows is less than or equal to zero. + /// + /// This method acquires a read lock on the content tree for consistency with other + /// version retrieval methods. If content with the specified id does not exist, + /// an empty enumerable is returned rather than throwing an exception. + /// + IEnumerable GetVersionIds(int id, int maxRows); + + #endregion + + #region Rollback + + /// + /// Rolls back content to a previous version. + /// + /// The content id to rollback. + /// The version id to rollback to. + /// The culture to rollback, or "*" for all cultures. + /// The user performing the rollback. + /// The operation result indicating success or failure. + /// + /// Fires (cancellable) before rollback + /// and after successful rollback. + /// The rollback copies property values from the target version to the current content + /// and saves it, creating a new version. + /// + OperationResult Rollback(int id, int versionId, string culture = "*", int userId = Constants.Security.SuperUserId); + + #endregion + + #region Version Deletion + + /// + /// Permanently deletes versions of content prior to a specific date. + /// + /// The content id. + /// Delete versions older than this date. + /// The user performing the deletion. + /// + /// This method will never delete the latest version of a content item. + /// Fires (cancellable) before deletion + /// and after deletion. + /// + void DeleteVersions(int id, DateTime versionDate, int userId = Constants.Security.SuperUserId); + + /// + /// Permanently deletes a specific version of content. + /// + /// The content id. + /// The version id to delete. + /// If true, also deletes all versions prior to the specified version. + /// The user performing the deletion. + /// + /// This method will never delete the current version or published version of a content item. + /// Fires (cancellable) before deletion + /// and after deletion. + /// If deletePriorVersions is true, it first deletes all versions prior to the specified version's date, + /// then deletes the specified version. + /// + void DeleteVersion(int id, int versionId, bool deletePriorVersions, int userId = Constants.Security.SuperUserId); + + #endregion +} +``` + +**Step 2: Run build to verify interface compiles** + +Run: `dotnet build src/Umbraco.Core --no-restore` +Expected: Build succeeds + +**Step 3: Commit** + +```bash +git add src/Umbraco.Core/Services/IContentVersionOperationService.cs +git commit -m "$(cat <<'EOF' +refactor(core): add IContentVersionOperationService interface + +Part of ContentService refactoring Phase 3. +Defines version operations to be extracted from ContentService. + +πŸ€– Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 2: Create ContentVersionOperationService Implementation + +**Files:** +- Create: `src/Umbraco.Core/Services/ContentVersionOperationService.cs` + +**Step 1: Create the implementation file** + +> **v1.1 Changes Applied:** +> - Issue 2.1: Consolidated Rollback into single scope (eliminated TOCTOU race condition) +> - Issue 2.2: Added try-catch error handling with proper logging +> - Issue 2.3: Added ReadLock to GetVersionIds for consistency +> - Issue 2.4: Fixed DeleteVersion to use repository directly instead of nested service calls +> +> **v1.2 Changes Applied:** +> - Issue 2.1: Use CrudService.Save instead of DocumentRepository.Save to preserve ContentSaving/ContentSaved notifications +> - Issue 2.2: Preserve double-notification behavior for DeleteVersion with deletePriorVersions=true +> - Issue 3.1: Simplified lock acquisition in DeleteVersion (single WriteLock at start) +> - Issue 3.3: Added IContentCrudService constructor dependency +> +> **v1.3 Changes Applied:** +> - Issue 3.1: Added input validation to GetVersionIds (ArgumentOutOfRangeException for maxRows <= 0) +> - Issue 3.2: Removed redundant WriteLock in Rollback (CrudService.Save handles locking) +> - Issue 3.3: Added audit entry for prior versions in DeleteVersion with deletePriorVersions=true +> - Issue 3.4: Fixed Rollback return type from OperationResult to OperationResult +> - Issue 3.6: Added clarifying comment for prior versions cancellation behavior + +```csharp +// src/Umbraco.Core/Services/ContentVersionOperationService.cs +using Microsoft.Extensions.Logging; +using Umbraco.Cms.Core.Events; +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Notifications; +using Umbraco.Cms.Core.Persistence.Repositories; +using Umbraco.Cms.Core.Scoping; + +namespace Umbraco.Cms.Core.Services; + +/// +/// Implements content version operations (retrieving versions, rollback, deleting versions). +/// +public class ContentVersionOperationService : ContentServiceBase, IContentVersionOperationService +{ + private readonly ILogger _logger; + // v1.2 Fix (Issue 3.3): Added IContentCrudService for proper save with notifications + private readonly IContentCrudService _crudService; + + public ContentVersionOperationService( + ICoreScopeProvider provider, + ILoggerFactory loggerFactory, + IEventMessagesFactory eventMessagesFactory, + IDocumentRepository documentRepository, + IAuditService auditService, + IUserIdKeyResolver userIdKeyResolver, + IContentCrudService crudService) // v1.2: Added for Rollback save operation + : base(provider, loggerFactory, eventMessagesFactory, documentRepository, auditService, userIdKeyResolver) + { + _logger = loggerFactory.CreateLogger(); + _crudService = crudService; + } + + #region Version Retrieval + + /// + public IContent? GetVersion(int versionId) + { + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + scope.ReadLock(Constants.Locks.ContentTree); + return DocumentRepository.GetVersion(versionId); + } + + /// + public IEnumerable GetVersions(int id) + { + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + scope.ReadLock(Constants.Locks.ContentTree); + return DocumentRepository.GetAllVersions(id); + } + + /// + public IEnumerable GetVersionsSlim(int id, int skip, int take) + { + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + scope.ReadLock(Constants.Locks.ContentTree); + return DocumentRepository.GetAllVersionsSlim(id, skip, take); + } + + /// + public IEnumerable GetVersionIds(int id, int maxRows) + { + // v1.3 Fix (Issue 3.1): Added input validation to match interface documentation. + // The interface documents ArgumentOutOfRangeException for maxRows <= 0. + if (maxRows <= 0) + { + throw new ArgumentOutOfRangeException(nameof(maxRows), maxRows, "Value must be greater than zero."); + } + + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + // v1.1 Fix (Issue 2.3): Added ReadLock for consistency with other read operations. + // The original ContentService.GetVersionIds did not acquire a ReadLock, which was + // inconsistent with GetVersion, GetVersions, and GetVersionsSlim. + scope.ReadLock(Constants.Locks.ContentTree); + return DocumentRepository.GetVersionIds(id, maxRows); + } + + #endregion + + #region Rollback + + /// + public OperationResult Rollback(int id, int versionId, string culture = "*", int userId = Constants.Security.SuperUserId) + { + EventMessages evtMsgs = EventMessagesFactory.Get(); + + // v1.1 Fix (Issue 2.1): Use a single scope for the entire operation to eliminate + // TOCTOU race condition. Previously used separate read and write scopes which + // could allow concurrent modification between reading content and writing changes. + using ICoreScope scope = ScopeProvider.CreateCoreScope(); + + // Read operations - acquire read lock first + scope.ReadLock(Constants.Locks.ContentTree); + IContent? content = DocumentRepository.Get(id); + // v1.1 Fix: Use DocumentRepository.GetVersion directly instead of calling + // this.GetVersion() which would create a nested scope + IContent? version = DocumentRepository.GetVersion(versionId); + + // Null checks - cannot rollback if content or version is missing, or if trashed + if (content == null || version == null || content.Trashed) + { + scope.Complete(); + return new OperationResult(OperationResultType.FailedCannot, evtMsgs); + } + + var rollingBackNotification = new ContentRollingBackNotification(content, evtMsgs); + if (scope.Notifications.PublishCancelable(rollingBackNotification)) + { + scope.Complete(); + return OperationResult.Cancel(evtMsgs); + } + + // Copy the changes from the version + content.CopyFrom(version, culture); + + // v1.2 Fix (Issue 2.1): Use CrudService.Save to preserve ContentSaving/ContentSaved notifications. + // The original ContentService.Rollback called Save(content, userId) which fires these notifications. + // Using DocumentRepository.Save directly would bypass validation, audit trail, and cache invalidation. + // v1.3 Fix (Issue 3.2): Removed explicit WriteLock - CrudService.Save handles its own locking internally. + // v1.3 Fix (Issue 3.4): Fixed return type from OperationResult to OperationResult. + OperationResult saveResult = _crudService.Save(content, userId); + if (!saveResult.Success) + { + _logger.LogError("User '{UserId}' was unable to rollback content '{ContentId}' to version '{VersionId}'", userId, id, versionId); + scope.Complete(); + return new OperationResult(OperationResultType.Failed, evtMsgs); + } + + // Only publish success notification if save succeeded + scope.Notifications.Publish( + new ContentRolledBackNotification(content, evtMsgs).WithStateFrom(rollingBackNotification)); + + // Logging & Audit + _logger.LogInformation("User '{UserId}' rolled back content '{ContentId}' to version '{VersionId}'", userId, content.Id, version.VersionId); + Audit(AuditType.RollBack, userId, content.Id, $"Content '{content.Name}' was rolled back to version '{version.VersionId}'"); + + scope.Complete(); + + return OperationResult.Succeed(evtMsgs); + } + + #endregion + + #region Version Deletion + + /// + public void DeleteVersions(int id, DateTime versionDate, int userId = Constants.Security.SuperUserId) + { + EventMessages evtMsgs = EventMessagesFactory.Get(); + + using ICoreScope scope = ScopeProvider.CreateCoreScope(); + + var deletingVersionsNotification = new ContentDeletingVersionsNotification(id, evtMsgs, dateToRetain: versionDate); + if (scope.Notifications.PublishCancelable(deletingVersionsNotification)) + { + scope.Complete(); + return; + } + + scope.WriteLock(Constants.Locks.ContentTree); + DocumentRepository.DeleteVersions(id, versionDate); + + scope.Notifications.Publish( + new ContentDeletedVersionsNotification(id, evtMsgs, dateToRetain: versionDate).WithStateFrom(deletingVersionsNotification)); + Audit(AuditType.Delete, userId, Constants.System.Root, "Delete (by version date)"); + + scope.Complete(); + } + + /// + public void DeleteVersion(int id, int versionId, bool deletePriorVersions, int userId = Constants.Security.SuperUserId) + { + EventMessages evtMsgs = EventMessagesFactory.Get(); + + using ICoreScope scope = ScopeProvider.CreateCoreScope(); + + // v1.2 Fix (Issue 3.1): Acquire WriteLock once at the start instead of multiple times. + // This simplifies the code and avoids the readβ†’write lock upgrade pattern. + scope.WriteLock(Constants.Locks.ContentTree); + + var deletingVersionsNotification = new ContentDeletingVersionsNotification(id, evtMsgs, versionId); + if (scope.Notifications.PublishCancelable(deletingVersionsNotification)) + { + scope.Complete(); + return; + } + + // v1.2 Fix (Issue 2.2): Preserve original double-notification behavior for deletePriorVersions. + // The original implementation called DeleteVersions() which fired its own notifications. + // We inline the notification firing to maintain backward compatibility. + // v1.3 Fix (Issue 3.6): Clarification - if prior versions deletion is cancelled, we still + // proceed with deleting the specific version. This matches original ContentService behavior. + if (deletePriorVersions) + { + IContent? versionContent = DocumentRepository.GetVersion(versionId); + DateTime cutoffDate = versionContent?.UpdateDate ?? DateTime.UtcNow; + + // Publish notifications for prior versions (matching original behavior) + var priorVersionsNotification = new ContentDeletingVersionsNotification(id, evtMsgs, dateToRetain: cutoffDate); + if (!scope.Notifications.PublishCancelable(priorVersionsNotification)) + { + DocumentRepository.DeleteVersions(id, cutoffDate); + scope.Notifications.Publish( + new ContentDeletedVersionsNotification(id, evtMsgs, dateToRetain: cutoffDate) + .WithStateFrom(priorVersionsNotification)); + + // v1.3 Fix (Issue 3.3): Add audit entry for prior versions deletion. + // The original DeleteVersions() method created its own audit entry. + Audit(AuditType.Delete, userId, Constants.System.Root, "Delete (by version date)"); + } + } + + IContent? c = DocumentRepository.Get(id); + + // Don't delete the current or published version + if (c?.VersionId != versionId && c?.PublishedVersionId != versionId) + { + DocumentRepository.DeleteVersion(versionId); + } + + scope.Notifications.Publish( + new ContentDeletedVersionsNotification(id, evtMsgs, versionId).WithStateFrom(deletingVersionsNotification)); + Audit(AuditType.Delete, userId, Constants.System.Root, "Delete (by version)"); + + scope.Complete(); + } + + #endregion +} +``` + +**Step 2: Run build to verify implementation compiles** + +Run: `dotnet build src/Umbraco.Core --no-restore` +Expected: Build succeeds + +**Step 3: Commit** + +```bash +git add src/Umbraco.Core/Services/ContentVersionOperationService.cs +git commit -m "$(cat <<'EOF' +refactor(core): add ContentVersionOperationService implementation + +Part of ContentService refactoring Phase 3. +Implements version retrieval, rollback, and version deletion operations. + +v1.1 fixes applied: +- Consolidated Rollback into single scope (Issue 2.1) +- Added error handling to Rollback (Issue 2.2) +- Added ReadLock to GetVersionIds (Issue 2.3) +- Fixed DeleteVersion nested scope (Issue 2.4) + +πŸ€– Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 3: Register Service in DI Container + +**Files:** +- Modify: `src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs` + +**Step 1: Add service registration** + +Locate the line: +```csharp +Services.AddUnique(); +``` + +Add after it: +```csharp +Services.AddUnique(); +``` + +**Step 2: Update ContentService factory registration** + +Find the `IContentService` factory registration (starts with `Services.AddUnique(sp =>`). + +Add the new parameter to the constructor call: +```csharp +sp.GetRequiredService() +``` + +The updated registration should look like: +```csharp +Services.AddUnique(sp => + new ContentService( + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService>(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService>(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService())); // NEW +``` + +**Step 3: Run build to verify registration** + +Run: `dotnet build src/Umbraco.Core --no-restore` +Expected: Build fails because ContentService doesn't have the new constructor parameter yet (expected) + +**Step 4: Commit** + +```bash +git add src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs +git commit -m "$(cat <<'EOF' +refactor(core): register IContentVersionOperationService in DI + +Part of ContentService refactoring Phase 3. +Adds service registration and updates ContentService factory. + +πŸ€– Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 4: Add VersionOperationService Property to ContentService + +**Files:** +- Modify: `src/Umbraco.Core/Services/ContentService.cs` + +**Step 1: Add private fields for the service** + +After the existing `_queryOperationServiceLazy` field declaration, add: + +```csharp +// Version operation service fields (for Phase 3 extracted version operations) +private readonly IContentVersionOperationService? _versionOperationService; +private readonly Lazy? _versionOperationServiceLazy; +``` + +**Step 2: Add property accessor** + +After the `QueryOperationService` property, add: + +```csharp +/// +/// Gets the version operation service. +/// +/// Thrown if the service was not properly initialized. +private IContentVersionOperationService VersionOperationService => + _versionOperationService ?? _versionOperationServiceLazy?.Value + ?? throw new InvalidOperationException("VersionOperationService not initialized. Ensure the service is properly injected via constructor."); +``` + +**Step 3: Update the primary constructor** + +Find the constructor marked with `[ActivatorUtilitiesConstructor]`. + +Add the new parameter: +```csharp +IContentVersionOperationService versionOperationService) // NEW PARAMETER - Phase 3 version operations +``` + +In the constructor body, add: +```csharp +// Phase 3: Version operation service (direct injection) +ArgumentNullException.ThrowIfNull(versionOperationService); +_versionOperationService = versionOperationService; +_versionOperationServiceLazy = null; // Not needed when directly injected +``` + +**Step 4: Update the obsolete constructors** + +Find the obsolete constructors (marked with `[Obsolete]`). Add lazy resolution for the new service: + +```csharp +// Phase 3: Lazy resolution of IContentVersionOperationService +_versionOperationServiceLazy = new Lazy(() => + StaticServiceProvider.Instance.GetRequiredService(), + LazyThreadSafetyMode.ExecutionAndPublication); +``` + +**Step 5: Run build to verify** + +Run: `dotnet build src/Umbraco.Core --no-restore` +Expected: Build succeeds + +**Step 6: Commit** + +```bash +git add src/Umbraco.Core/Services/ContentService.cs +git commit -m "$(cat <<'EOF' +refactor(core): add VersionOperationService property to ContentService + +Part of ContentService refactoring Phase 3. +Adds constructor parameter and property for version operations delegation. + +πŸ€– Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 5: Delegate Version Retrieval Methods + +**Files:** +- Modify: `src/Umbraco.Core/Services/ContentService.cs` + +**Step 1: Delegate GetVersion** + +Find the `GetVersion` method (around line 572). Replace the implementation with: + +```csharp +public IContent? GetVersion(int versionId) + => VersionOperationService.GetVersion(versionId); +``` + +**Step 2: Delegate GetVersions** + +Find the `GetVersions` method (around line 586). Replace the implementation with: + +```csharp +public IEnumerable GetVersions(int id) + => VersionOperationService.GetVersions(id); +``` + +**Step 3: Delegate GetVersionsSlim** + +Find the `GetVersionsSlim` method (around line 599). Replace the implementation with: + +```csharp +public IEnumerable GetVersionsSlim(int id, int skip, int take) + => VersionOperationService.GetVersionsSlim(id, skip, take); +``` + +**Step 4: Delegate GetVersionIds** + +Find the `GetVersionIds` method (around line 614). Replace the implementation with: + +```csharp +public IEnumerable GetVersionIds(int id, int maxRows) + => VersionOperationService.GetVersionIds(id, maxRows); +``` + +**Step 5: Run build to verify** + +Run: `dotnet build src/Umbraco.Core --no-restore` +Expected: Build succeeds + +**Step 6: Run tests to verify behavior unchanged** + +Run: `dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentService" --no-restore` +Expected: All tests pass + +**Step 7: Commit** + +```bash +git add src/Umbraco.Core/Services/ContentService.cs +git commit -m "$(cat <<'EOF' +refactor(core): delegate version retrieval methods to VersionOperationService + +Part of ContentService refactoring Phase 3. +Delegates GetVersion, GetVersions, GetVersionsSlim, GetVersionIds. + +πŸ€– Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 6: Delegate Rollback Method + +**Files:** +- Modify: `src/Umbraco.Core/Services/ContentService.cs` + +**Step 1: Update Rollback to delegate to VersionOperationService** + +Find the `Rollback` method in the `#region Rollback` section (around line 243). + +**Note:** The current Rollback implementation calls `GetById` and `Save` from the current ContentService. The VersionOperationService implementation must work independently. Review the implementation in Task 2 which directly uses DocumentRepository. + +Replace the entire Rollback method with: + +```csharp +public OperationResult Rollback(int id, int versionId, string culture = "*", int userId = Constants.Security.SuperUserId) + => VersionOperationService.Rollback(id, versionId, culture, userId); +``` + +**Step 2: Run build to verify** + +Run: `dotnet build src/Umbraco.Core --no-restore` +Expected: Build succeeds + +**Step 3: Run tests to verify rollback behavior** + +Run: `dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentService" --no-restore` +Expected: All tests pass + +**Step 4: Commit** + +```bash +git add src/Umbraco.Core/Services/ContentService.cs +git commit -m "$(cat <<'EOF' +refactor(core): delegate Rollback to VersionOperationService + +Part of ContentService refactoring Phase 3. +Notification ordering preserved: RollingBack -> RolledBack. + +πŸ€– Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 7: Delegate Version Deletion Methods + +**Files:** +- Modify: `src/Umbraco.Core/Services/ContentService.cs` + +**Step 1: Delegate DeleteVersions** + +Find the `DeleteVersions` method (around line 1978). Replace the implementation with: + +```csharp +public void DeleteVersions(int id, DateTime versionDate, int userId = Constants.Security.SuperUserId) + => VersionOperationService.DeleteVersions(id, versionDate, userId); +``` + +**Step 2: Delegate DeleteVersion** + +Find the `DeleteVersion` method (around line 2012). Replace the implementation with: + +```csharp +public void DeleteVersion(int id, int versionId, bool deletePriorVersions, int userId = Constants.Security.SuperUserId) + => VersionOperationService.DeleteVersion(id, versionId, deletePriorVersions, userId); +``` + +**Step 3: Run build to verify** + +Run: `dotnet build src/Umbraco.Core --no-restore` +Expected: Build succeeds + +**Step 4: Run full ContentService tests** + +Run: `dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentService" --no-restore` +Expected: All tests pass + +**Step 5: Commit** + +```bash +git add src/Umbraco.Core/Services/ContentService.cs +git commit -m "$(cat <<'EOF' +refactor(core): delegate DeleteVersions and DeleteVersion to VersionOperationService + +Part of ContentService refactoring Phase 3. +Completes version operations extraction. + +πŸ€– Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 8: Create Integration Tests for ContentVersionOperationService + +**Files:** +- Create: `tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentVersionOperationServiceTests.cs` + +**Step 1: Create test file with comprehensive tests** + +> **v1.1 Changes Applied:** +> - Issue 2.5: Replaced Thread.Sleep with deterministic date comparison +> - Issue 3.2: Added Rollback_WhenNotificationCancelled_ReturnsCancelledResult test +> - Issue 3.3: Added DeleteVersion_PublishedVersion_DoesNotDelete test + +```csharp +// tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentVersionOperationServiceTests.cs +using NUnit.Framework; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.DependencyInjection; +using Umbraco.Cms.Core.Events; +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Models.ContentPublishing; +using Umbraco.Cms.Core.Notifications; +using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Tests.Common.Builders; +using Umbraco.Cms.Tests.Common.Testing; +using Umbraco.Cms.Tests.Integration.Testing; + +namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services; + +[TestFixture] +[UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)] +public class ContentVersionOperationServiceTests : UmbracoIntegrationTest +{ + private IContentVersionOperationService VersionOperationService => GetRequiredService(); + private IContentService ContentService => GetRequiredService(); + private IContentTypeService ContentTypeService => GetRequiredService(); + private IContentPublishingService ContentPublishingService => GetRequiredService(); + + // v1.2 Fix (Issue 3.2): Use CustomTestSetup to register notification handlers + protected override void CustomTestSetup(IUmbracoBuilder builder) + => builder.AddNotificationHandler(); + + private IContentType CreateContentType() + { + var contentType = new ContentTypeBuilder() + .WithAlias("testContentType") + .WithName("Test Content Type") + .AddPropertyType() + .WithAlias("title") + .WithName("Title") + .WithDataTypeId(Constants.DataTypes.Textbox) + .Done() + .Build(); + ContentTypeService.Save(contentType); + return contentType; + } + + private IContent CreateAndSaveContent(IContentType contentType, string name = "Test Content") + { + var content = new ContentBuilder() + .WithContentType(contentType) + .WithName(name) + .Build(); + ContentService.Save(content); + return content; + } + + #region GetVersion Tests + + [Test] + public void GetVersion_ExistingVersion_ReturnsContent() + { + // Arrange + var contentType = CreateContentType(); + var content = CreateAndSaveContent(contentType); + var versionId = content.VersionId; + + // Act + var result = VersionOperationService.GetVersion(versionId); + + // Assert + Assert.That(result, Is.Not.Null); + Assert.That(result!.Id, Is.EqualTo(content.Id)); + } + + [Test] + public void GetVersion_NonExistentVersion_ReturnsNull() + { + // Act + var result = VersionOperationService.GetVersion(999999); + + // Assert + Assert.That(result, Is.Null); + } + + #endregion + + #region GetVersions Tests + + [Test] + public void GetVersions_ContentWithMultipleVersions_ReturnsAllVersions() + { + // Arrange + var contentType = CreateContentType(); + var content = CreateAndSaveContent(contentType); + + // Create additional versions + content.SetValue("title", "Version 2"); + ContentService.Save(content); + content.SetValue("title", "Version 3"); + ContentService.Save(content); + + // Act + var versions = VersionOperationService.GetVersions(content.Id).ToList(); + + // Assert + Assert.That(versions.Count, Is.GreaterThanOrEqualTo(3)); + } + + [Test] + public void GetVersions_NonExistentContent_ReturnsEmpty() + { + // Act + var versions = VersionOperationService.GetVersions(999999).ToList(); + + // Assert + Assert.That(versions, Is.Empty); + } + + #endregion + + #region GetVersionsSlim Tests + + [Test] + public void GetVersionsSlim_ReturnsPagedVersions() + { + // Arrange + var contentType = CreateContentType(); + var content = CreateAndSaveContent(contentType); + + // Create additional versions + for (int i = 2; i <= 5; i++) + { + content.SetValue("title", $"Version {i}"); + ContentService.Save(content); + } + + // Act + var versions = VersionOperationService.GetVersionsSlim(content.Id, skip: 1, take: 2).ToList(); + + // Assert + Assert.That(versions.Count, Is.EqualTo(2)); + } + + #endregion + + #region GetVersionIds Tests + + [Test] + public void GetVersionIds_ReturnsVersionIdsOrderedByLatestFirst() + { + // Arrange + var contentType = CreateContentType(); + var content = CreateAndSaveContent(contentType); + var firstVersionId = content.VersionId; + + content.SetValue("title", "Version 2"); + ContentService.Save(content); + var secondVersionId = content.VersionId; + + // Act + var versionIds = VersionOperationService.GetVersionIds(content.Id, maxRows: 10).ToList(); + + // Assert + Assert.That(versionIds.Count, Is.GreaterThanOrEqualTo(2)); + Assert.That(versionIds[0], Is.EqualTo(secondVersionId)); // Latest first + } + + #endregion + + #region Rollback Tests + + [Test] + public void Rollback_ToEarlierVersion_RestoresPropertyValues() + { + // Arrange + var contentType = CreateContentType(); + var content = CreateAndSaveContent(contentType); + content.SetValue("title", "Original Value"); + ContentService.Save(content); + var originalVersionId = content.VersionId; + + content.SetValue("title", "Changed Value"); + ContentService.Save(content); + + // Act + var result = VersionOperationService.Rollback(content.Id, originalVersionId); + + // Assert + Assert.That(result.Success, Is.True); + var rolledBackContent = ContentService.GetById(content.Id); + Assert.That(rolledBackContent!.GetValue("title"), Is.EqualTo("Original Value")); + } + + [Test] + public void Rollback_NonExistentContent_Fails() + { + // Act + var result = VersionOperationService.Rollback(999999, 1); + + // Assert + Assert.That(result.Success, Is.False); + Assert.That(result.Result, Is.EqualTo(OperationResultType.FailedCannot)); + } + + [Test] + public void Rollback_TrashedContent_Fails() + { + // Arrange + var contentType = CreateContentType(); + var content = CreateAndSaveContent(contentType); + var versionId = content.VersionId; + ContentService.MoveToRecycleBin(content); + + // Act + var result = VersionOperationService.Rollback(content.Id, versionId); + + // Assert + Assert.That(result.Success, Is.False); + Assert.That(result.Result, Is.EqualTo(OperationResultType.FailedCannot)); + } + + /// + /// v1.2 Fix (Issue 3.2): Test that cancellation notification works correctly. + /// Uses the correct integration test pattern with CustomTestSetup and static action. + /// + [Test] + public void Rollback_WhenNotificationCancelled_ReturnsCancelledResult() + { + // Arrange + var contentType = CreateContentType(); + var content = CreateAndSaveContent(contentType); + content.SetValue("title", "Original Value"); + ContentService.Save(content); + var originalVersionId = content.VersionId; + + content.SetValue("title", "Changed Value"); + ContentService.Save(content); + + // Set up the notification handler to cancel the rollback + VersionNotificationHandler.RollingBackContent = notification => notification.Cancel = true; + + try + { + // Act + var result = VersionOperationService.Rollback(content.Id, originalVersionId); + + // Assert + Assert.That(result.Success, Is.False); + Assert.That(result.Result, Is.EqualTo(OperationResultType.Cancelled)); + + // Verify content was not modified + var unchangedContent = ContentService.GetById(content.Id); + Assert.That(unchangedContent!.GetValue("title"), Is.EqualTo("Changed Value")); + } + finally + { + // Clean up the static action + VersionNotificationHandler.RollingBackContent = null; + } + } + + #endregion + + #region DeleteVersions Tests + + /// + /// v1.1 Fix (Issue 2.5): Use deterministic date comparison instead of Thread.Sleep. + /// + [Test] + public void DeleteVersions_ByDate_DeletesOlderVersions() + { + // Arrange + var contentType = CreateContentType(); + var content = CreateAndSaveContent(contentType); + var firstVersionId = content.VersionId; + + content.SetValue("title", "Version 2"); + ContentService.Save(content); + + // Get the actual update date of version 2 for deterministic comparison + var version2 = VersionOperationService.GetVersion(content.VersionId); + var cutoffDate = version2!.UpdateDate.AddMilliseconds(1); + + content.SetValue("title", "Version 3"); + ContentService.Save(content); + var version3Id = content.VersionId; + + var versionCountBefore = VersionOperationService.GetVersions(content.Id).Count(); + + // Act + VersionOperationService.DeleteVersions(content.Id, cutoffDate); + + // Assert + var remainingVersions = VersionOperationService.GetVersions(content.Id).ToList(); + Assert.That(remainingVersions.Any(v => v.VersionId == version3Id), Is.True, "Current version should remain"); + Assert.That(remainingVersions.Count, Is.LessThan(versionCountBefore)); + } + + #endregion + + #region DeleteVersion Tests + + [Test] + public void DeleteVersion_SpecificVersion_DeletesOnlyThatVersion() + { + // Arrange + var contentType = CreateContentType(); + var content = CreateAndSaveContent(contentType); + + content.SetValue("title", "Version 2"); + ContentService.Save(content); + var versionToDelete = content.VersionId; + + content.SetValue("title", "Version 3"); + ContentService.Save(content); + var currentVersionId = content.VersionId; + + // Act + VersionOperationService.DeleteVersion(content.Id, versionToDelete, deletePriorVersions: false); + + // Assert + var deletedVersion = VersionOperationService.GetVersion(versionToDelete); + Assert.That(deletedVersion, Is.Null); + var currentVersion = VersionOperationService.GetVersion(currentVersionId); + Assert.That(currentVersion, Is.Not.Null); + } + + [Test] + public void DeleteVersion_CurrentVersion_DoesNotDelete() + { + // Arrange + var contentType = CreateContentType(); + var content = CreateAndSaveContent(contentType); + var currentVersionId = content.VersionId; + + // Act + VersionOperationService.DeleteVersion(content.Id, currentVersionId, deletePriorVersions: false); + + // Assert + var version = VersionOperationService.GetVersion(currentVersionId); + Assert.That(version, Is.Not.Null); // Should not be deleted + } + + /// + /// v1.2 Fix (Issue 3.3, 3.4): Test that published version is protected from deletion. + /// Uses the correct async ContentPublishingService.PublishAsync method. + /// + [Test] + public async Task DeleteVersion_PublishedVersion_DoesNotDelete() + { + // Arrange + var contentType = CreateContentType(); + var content = CreateAndSaveContent(contentType); + + // v1.2 Fix (Issue 3.4): Use ContentPublishingService.PublishAsync with correct signature + var publishResult = await ContentPublishingService.PublishAsync( + content.Key, + new[] { new CulturePublishScheduleModel() }, + Constants.Security.SuperUserKey); + Assert.That(publishResult.Success, Is.True, "Publish should succeed"); + + // Refresh content to get the published version id + content = ContentService.GetById(content.Id)!; + var publishedVersionId = content.PublishedVersionId; + Assert.That(publishedVersionId, Is.Not.Null, "Content should have a published version"); + + // Create a newer draft version + content.SetValue("title", "Draft"); + ContentService.Save(content); + + // Act + VersionOperationService.DeleteVersion(content.Id, publishedVersionId!.Value, deletePriorVersions: false); + + // Assert + var version = VersionOperationService.GetVersion(publishedVersionId!.Value); + Assert.That(version, Is.Not.Null, "Published version should not be deleted"); + } + + #endregion + + #region Behavioral Equivalence Tests + + [Test] + public void GetVersion_ViaService_MatchesContentService() + { + // Arrange + var contentType = CreateContentType(); + var content = CreateAndSaveContent(contentType); + var versionId = content.VersionId; + + // Act + var viaService = VersionOperationService.GetVersion(versionId); + var viaContentService = ContentService.GetVersion(versionId); + + // Assert + Assert.That(viaService?.Id, Is.EqualTo(viaContentService?.Id)); + Assert.That(viaService?.VersionId, Is.EqualTo(viaContentService?.VersionId)); + } + + [Test] + public void GetVersions_ViaService_MatchesContentService() + { + // Arrange + var contentType = CreateContentType(); + var content = CreateAndSaveContent(contentType); + content.SetValue("title", "Version 2"); + ContentService.Save(content); + + // Act + var viaService = VersionOperationService.GetVersions(content.Id).ToList(); + var viaContentService = ContentService.GetVersions(content.Id).ToList(); + + // Assert + Assert.That(viaService.Count, Is.EqualTo(viaContentService.Count)); + } + + #endregion + + #region Notification Handler + + /// + /// v1.2 Fix (Issue 3.2): Notification handler for testing using the correct integration test pattern. + /// Uses static actions that can be set in individual tests. + /// + private class VersionNotificationHandler : INotificationHandler + { + public static Action? RollingBackContent { get; set; } + + public void Handle(ContentRollingBackNotification notification) + => RollingBackContent?.Invoke(notification); + } + + #endregion +} +``` + +**Step 2: Run the new tests** + +Run: `dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentVersionOperationServiceTests" --no-restore` +Expected: All tests pass + +**Step 3: Commit** + +```bash +git add tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentVersionOperationServiceTests.cs +git commit -m "$(cat <<'EOF' +test(integration): add ContentVersionOperationServiceTests + +Part of ContentService refactoring Phase 3. +Covers version retrieval, rollback, and version deletion. + +v1.1 fixes applied: +- Deterministic date comparison instead of Thread.Sleep (Issue 2.5) +- Added Rollback cancellation test (Issue 3.2) +- Added published version protection test (Issue 3.3) + +v1.2 fixes applied: +- Fixed notification handler registration pattern (Issue 3.2) +- Fixed Publish method signature using ContentPublishingService (Issue 3.4) + +πŸ€– Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 9: Run Phase Gate Tests + +**Step 1: Run ContentService refactoring tests** + +Run: `dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentServiceRefactoringTests" --no-restore` +Expected: All tests pass + +**Step 2: Run all ContentService tests** + +Run: `dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentService" --no-restore` +Expected: All tests pass (or same count as baseline) + +**Step 3: Run full integration test suite** + +Run: `dotnet test tests/Umbraco.Tests.Integration --no-restore` +Expected: All tests pass (or same count as baseline) + +**Step 4: Verify build with no errors** + +Run: `dotnet build src/Umbraco.Core --no-restore` +Expected: Build succeeds with no errors + +--- + +## Task 10: Update Design Document and Create Git Tag + +**Files:** +- Modify: `docs/plans/2025-12-19-contentservice-refactor-design.md` + +**Step 1: Update Phase 3 status in design document** + +Find the Implementation Order table and update Phase 3 row: + +Change: +``` +| 3 | Version Service | All ContentService*Tests | All pass | Pending | +``` + +To: +``` +| 3 | Version Service | All ContentService*Tests | All pass | βœ… Complete | +``` + +**Step 2: Add Phase 3 completion notes** + +Add after the Phase 2 notes in the Phase Details section: + +```markdown +4. **Phase 3: Version Service** βœ… - Complete! Created: + - `IContentVersionOperationService.cs` - Interface (7 methods) + - `ContentVersionOperationService.cs` - Implementation + - Updated `ContentService.cs` to delegate version operations + - Git tag: `phase-3-version-extraction` +``` + +**Step 3: Commit design document update** + +```bash +git add docs/plans/2025-12-19-contentservice-refactor-design.md +git commit -m "$(cat <<'EOF' +docs: mark Phase 3 complete in design document + +πŸ€– Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +**Step 4: Create git tag** + +```bash +git tag -a phase-3-version-extraction -m "Phase 3: Version operations extracted to ContentVersionOperationService" +``` + +--- + +## Task Summary + +| Task | Description | Files | +|------|-------------|-------| +| 1 | Create IContentVersionOperationService interface | `IContentVersionOperationService.cs` | +| 2 | Create ContentVersionOperationService implementation | `ContentVersionOperationService.cs` | +| 3 | Register service in DI container | `UmbracoBuilder.cs` | +| 4 | Add VersionOperationService property to ContentService | `ContentService.cs` | +| 5 | Delegate version retrieval methods | `ContentService.cs` | +| 6 | Delegate Rollback method | `ContentService.cs` | +| 7 | Delegate version deletion methods | `ContentService.cs` | +| 8 | Create integration tests | `ContentVersionOperationServiceTests.cs` | +| 9 | Run phase gate tests | N/A | +| 10 | Update design document and create tag | `design.md` | + +--- + +## v1.1 Changes Summary + +| Issue | Severity | Description | Fix Applied | +|-------|----------|-------------|-------------| +| 2.1 | Critical | Rollback TOCTOU race condition | Consolidated into single scope | +| 2.2 | Critical | Rollback missing error handling | Added try-catch with logging | +| 2.3 | Important | GetVersionIds missing ReadLock | Added ReadLock | +| 2.4 | Critical | DeleteVersion nested scope | Use repository directly | +| 2.5 | Important | Thread.Sleep in tests | Deterministic date comparison | +| 3.2 | Minor | Missing cancellation test | Added test | +| 3.3 | Minor | Missing published version test | Added test | +| 3.4 | Minor | Interface docs incomplete | Enhanced GetVersionIds docs | + +--- + +## v1.2 Changes Summary + +| Issue | Severity | Description | Fix Applied | +|-------|----------|-------------|-------------| +| 2.1 | Critical | Rollback bypasses ContentSaving/ContentSaved notifications | Use CrudService.Save to preserve notifications | +| 2.2 | Important | DeleteVersion with deletePriorVersions changes notification semantics | Inline notification firing to preserve double-notification | +| 3.1 | Minor | Redundant WriteLock acquisition in DeleteVersion | Single WriteLock at start | +| 3.2 | Minor | Test notification registration may not compile | Use CustomTestSetup with static action pattern | +| 3.3 | Minor | Missing IContentCrudService constructor dependency | Added to constructor | +| 3.4 | Minor | Publish method signature incorrect in test | Use ContentPublishingService.PublishAsync | + +--- + +## v1.3 Changes Summary + +| Issue | Severity | Description | Fix Applied | +|-------|----------|-------------|-------------| +| 3.1 | Should Fix | GetVersionIds missing input validation | Added ArgumentOutOfRangeException for maxRows <= 0 | +| 3.2 | Consider | Redundant WriteLock in Rollback | Removed explicit WriteLock (CrudService.Save handles locking) | +| 3.3 | Should Fix | Audit gap in DeleteVersion with deletePriorVersions | Added audit entry for prior versions deletion | +| 3.4 | Must Fix | Rollback return type mismatch | Fixed OperationResult to OperationResult | +| 3.6 | Consider | Prior versions cancellation behavior unclear | Added clarifying comment | + +--- + +## Rollback Procedure + +If tests fail during Phase 3: + +1. **Revert all Phase 3 commits:** + ```bash + git revert HEAD~N..HEAD # Where N is number of Phase 3 commits + ``` + +2. **Or reset to Phase 2 tag:** + ```bash + git reset --hard phase-2-query-extraction + ``` + +3. **Document the failure:** + - Which tests failed + - What behavior changed + - Root cause analysis + +4. **Create fix and retry:** + - Fix the identified issue + - Re-run all tasks from that point + +--- + +## Success Criteria + +- [ ] All 7 version methods delegated from ContentService +- [ ] IContentVersionOperationService interface created with documentation +- [ ] ContentVersionOperationService implementation with proper scoping +- [ ] DI registration complete +- [ ] All ContentService tests pass +- [ ] Integration tests for new service pass +- [ ] Design document updated with Phase 3 completion +- [ ] Git tag `phase-3-version-extraction` created