From 5d068e4cb47e1b4b8b0229446f9d6cc395dc0056 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Mon, 29 Jun 2026 16:25:01 -0700 Subject: [PATCH 01/24] VPR-59 fix(cms): preserve file bytes when save fails after overwrite - Snapshot a file's original bytes before an overwrite or replacement and restore them if the database save fails, so a failed create or edit can't orphan the new bytes or destroy the original. - If the restore itself fails, keep the backup and log its path for recovery instead of deleting the only remaining copy. - Import preview reserves each planned name so sources sharing a base name preview the unique names the import will actually assign. --- test/CMS/CmsFileImportServiceTests.cs | 17 +- test/CMS/CmsFileServiceTests.cs | 147 ++++++++++ .../CMS/Services/CmsFileImportService.cs | 15 +- web/Areas/CMS/Services/CmsFileService.cs | 253 ++++++++++++++---- .../CMS/Services/CmsFileStorageService.cs | 34 ++- 5 files changed, 399 insertions(+), 67 deletions(-) diff --git a/test/CMS/CmsFileImportServiceTests.cs b/test/CMS/CmsFileImportServiceTests.cs index 30c6d4feb..c02065c8f 100644 --- a/test/CMS/CmsFileImportServiceTests.cs +++ b/test/CMS/CmsFileImportServiceTests.cs @@ -334,15 +334,19 @@ public async Task Preview_DiskConflict_ReportsAutoRename() } [Fact] - public async Task Preview_TwoLinesSameFinalName_NonBlockingMessageOnSecond() + public async Task Preview_TwoLinesSameBasename_SecondPreviewsImportRenamedName() { CreateWebrootFile(@"cats\docs\manual.pdf"); // A second file in a different sub-path that produces the same final name after rename. Directory.CreateDirectory(Path.Join(_webroot, "cats", "other")); CreateWebrootFile(@"cats\other\manual.pdf"); - // Stub storage to always return the same available name so both resolve to "manual.pdf". + // Disk/DB alone leaves the name free, so both lines resolve to "manual.pdf" on their own. _storage.GetAvailableFileName("cats", "manual.pdf").Returns("manual.pdf"); + // But once the first line reserves "manual.pdf", the import would rename the second; the + // preview must reserve names across the batch and show that same renamed name up front. + _storage.GetAvailableFileName("cats", "manual.pdf", + Arg.Is?>(s => s != null && s.Contains("manual.pdf"))).Returns("manual_0.pdf"); var request = new CmsFileImportRequest { @@ -357,11 +361,16 @@ public async Task Preview_TwoLinesSameFinalName_NonBlockingMessageOnSecond() var results = await _service.PreviewImportAsync(request, TestContext.Current.CancellationToken); Assert.Equal(2, results.Count); - // First entry: CanImport true, no message. + // First entry: keeps the name, no message. Assert.True(results[0].CanImport); + Assert.Equal("manual.pdf", results[0].FileName); Assert.Null(results[0].Message); - // Second entry: CanImport true (non-blocking), but has a message about renaming. + // Second entry: previews the exact renamed name the import will assign (not the colliding + // name), with a non-blocking message explaining the rename. Assert.True(results[1].CanImport); + Assert.Equal("manual_0.pdf", results[1].FileName); + Assert.Equal("manual.pdf", results[1].RenamedFrom); + Assert.Equal("cats-manual_0.pdf", results[1].FriendlyName); Assert.NotNull(results[1].Message); Assert.Contains("renamed", results[1].Message, StringComparison.OrdinalIgnoreCase); } diff --git a/test/CMS/CmsFileServiceTests.cs b/test/CMS/CmsFileServiceTests.cs index 55bda3cc0..4d9218e96 100644 --- a/test/CMS/CmsFileServiceTests.cs +++ b/test/CMS/CmsFileServiceTests.cs @@ -67,6 +67,18 @@ private static IFormFile MakeFormFile(string fileName = "report.pdf") return new FormFile(stream, 0, stream.Length, "file", fileName); } + /// + /// Writes a real temp file to stand in for a backup the service returns from BackupManagedFile. + /// A real path lets the cleanup branch (System.IO.File.Exists/Delete) actually execute rather + /// than short-circuiting on a fake path that never exists. Callers delete it in a finally. + /// + private static string MakeRealBackupFile() + { + string path = Path.Join(Path.GetTempPath(), "Viper-CMS-Test-" + Guid.NewGuid().ToString("N")); + System.IO.File.WriteAllBytes(path, new byte[] { 9, 9, 9 }); + return path; + } + private async Task SeedFileAsync(Action? customize = null) { var file = new File @@ -284,6 +296,66 @@ public async Task CreateFile_OverwriteWithoutConflict_StoresNormally() _storage.DidNotReceive().ReplaceInPlace(Arg.Any(), Arg.Any()); } + [Fact] + public async Task CreateFile_OverwriteOrphan_SaveFails_RestoresOriginalFile() + { + const string targetPath = @"C:\FakeRoot\cats\orphan.pdf"; + const string backupPath = @"C:\FakeTemp\backup-orphan"; + _storage.FileNameInUse("cats", "orphan.pdf").Returns(true); + _storage.BuildManagedPath("cats", "orphan.pdf").Returns(targetPath); + _storage.ManagedFileExists(targetPath).Returns(true); + _storage.BackupManagedFile(targetPath).Returns(backupPath); + + var (service, _, _) = await BuildServiceOverFailingSaveAsync(); + var request = new CmsFileCreateRequest { Folder = "cats", Overwrite = true }; + + await Assert.ThrowsAsync( + () => service.CreateFileAsync(request, MakeFormFile("orphan.pdf"), TestContext.Current.CancellationToken)); + + // The orphaned original (no DB row) was overwritten on disk; the failed save must move the + // pre-overwrite backup back into place rather than deleting it, so the file isn't lost. + _storage.Received(1).BackupManagedFile(targetPath); + _storage.Received(1).ReplaceInPlace(backupPath, targetPath); + _storage.DidNotReceive().DeleteManagedFile(targetPath); + } + + [Fact] + public async Task CreateFile_OverwriteOrphan_SaveFails_RestoreFails_PreservesBackup() + { + const string targetPath = @"C:\FakeRoot\cats\orphan.pdf"; + _storage.FileNameInUse("cats", "orphan.pdf").Returns(true); + _storage.BuildManagedPath("cats", "orphan.pdf").Returns(targetPath); + _storage.ManagedFileExists(targetPath).Returns(true); + + // A real temp file stands in for the backup so the cleanup branch (File.Exists/Delete) + // actually runs instead of short-circuiting on a non-existent fake path. + string backupPath = MakeRealBackupFile(); + try + { + _storage.BackupManagedFile(targetPath).Returns(backupPath); + // The restore attempt itself fails (e.g., the target is locked); the backup is then the + // only surviving copy of the original bytes and must not be deleted. + _storage.When(s => s.ReplaceInPlace(backupPath, targetPath)) + .Do(_ => throw new IOException("restore failed")); + + var (service, _, _) = await BuildServiceOverFailingSaveAsync(); + var request = new CmsFileCreateRequest { Folder = "cats", Overwrite = true }; + + await Assert.ThrowsAsync( + () => service.CreateFileAsync(request, MakeFormFile("orphan.pdf"), TestContext.Current.CancellationToken)); + + _storage.Received(1).ReplaceInPlace(backupPath, targetPath); + Assert.True(System.IO.File.Exists(backupPath)); + } + finally + { + if (System.IO.File.Exists(backupPath)) + { + System.IO.File.Delete(backupPath); + } + } + } + #endregion #region Update @@ -453,6 +525,81 @@ await Assert.ThrowsAsync( _encryption.Received(1).EncryptFileInPlace(filePath, "old-key"); } + [Fact] + public async Task UpdateFile_ReplacementUpload_SaveFails_RestoresOriginalFile() + { + var (service, fileGuid, filePath) = await BuildServiceOverFailingSaveAsync(); + _storage.ManagedFileExists(filePath).Returns(true); + const string backupPath = @"C:\FakeTemp\backup-original"; + _storage.BackupManagedFile(filePath).Returns(backupPath); + var request = new CmsFileUpdateRequest { Description = "seeded" }; + + await Assert.ThrowsAsync( + () => service.UpdateFileAsync(fileGuid, request, MakeFormFile("newversion.pdf"), TestContext.Current.CancellationToken)); + + // ReplaceInPlace swapped in the new upload, but the save failed, so the pre-replacement + // backup must be moved back over the managed path to restore the original bytes. + _storage.Received(1).BackupManagedFile(filePath); + _storage.Received(1).ReplaceInPlace(backupPath, filePath); + } + + [Fact] + public async Task UpdateFile_ReplacementUpload_SaveFails_RestoreFails_PreservesBackup() + { + var (service, fileGuid, filePath) = await BuildServiceOverFailingSaveAsync(); + _storage.ManagedFileExists(filePath).Returns(true); + + string backupPath = MakeRealBackupFile(); + try + { + _storage.BackupManagedFile(filePath).Returns(backupPath); + // The restore attempt fails, so the backup holds the only copy of the pre-update bytes + // (the database rolled back to that older record) and must be preserved, not deleted. + _storage.When(s => s.ReplaceInPlace(backupPath, filePath)) + .Do(_ => throw new IOException("restore failed")); + var request = new CmsFileUpdateRequest { Description = "seeded" }; + + await Assert.ThrowsAsync( + () => service.UpdateFileAsync(fileGuid, request, MakeFormFile("newversion.pdf"), TestContext.Current.CancellationToken)); + + _storage.Received(1).ReplaceInPlace(backupPath, filePath); + Assert.True(System.IO.File.Exists(backupPath)); + } + finally + { + if (System.IO.File.Exists(backupPath)) + { + System.IO.File.Delete(backupPath); + } + } + } + + [Fact] + public async Task UpdateFile_ReplacementUpload_SaveSucceeds_DeletesBackup() + { + var file = await SeedFileAsync(); + _storage.ManagedFileExists(file.FilePath).Returns(true); + + string backupPath = MakeRealBackupFile(); + try + { + _storage.BackupManagedFile(file.FilePath).Returns(backupPath); + var request = new CmsFileUpdateRequest { Description = "seeded" }; + + await _service.UpdateFileAsync(file.FileGuid, request, MakeFormFile("newversion.pdf"), TestContext.Current.CancellationToken); + + // A committed save makes the pre-replacement backup obsolete; it must be cleaned up. + Assert.False(System.IO.File.Exists(backupPath)); + } + finally + { + if (System.IO.File.Exists(backupPath)) + { + System.IO.File.Delete(backupPath); + } + } + } + #endregion #region Delete / Restore diff --git a/web/Areas/CMS/Services/CmsFileImportService.cs b/web/Areas/CMS/Services/CmsFileImportService.cs index 8361e1242..b3927e68f 100644 --- a/web/Areas/CMS/Services/CmsFileImportService.cs +++ b/web/Areas/CMS/Services/CmsFileImportService.cs @@ -109,13 +109,22 @@ public async Task> PreviewImportAsync(CmsFileIm continue; } - string finalName = _storage.GetAvailableFileName(request.Folder, source.FileName); + // GetAvailableFileName only sees disk + DB, so two source files sharing a basename + // would both preview the same name. Reserving each planned name (as the sequential + // import writes would) makes a later line resolve to the same unique name the import + // actually assigns it, instead of advertising one it will lose. + string availableOnStore = _storage.GetAvailableFileName(request.Folder, source.FileName); + bool sharesNameWithEarlierLine = plannedNames.Contains(availableOnStore); + string finalName = sharesNameWithEarlierLine + ? _storage.GetAvailableFileName(request.Folder, source.FileName, plannedNames) + : availableOnStore; string friendlyName = CmsFileNaming.BuildFriendlyName(request.Folder, finalName); if (await _context.Files.AnyAsync(f => f.FriendlyName == friendlyName, ct)) { result.Message = $"A file with the name {friendlyName} already exists."; continue; } + plannedNames.Add(finalName); result.CanImport = true; result.FileName = finalName; @@ -125,9 +134,9 @@ public async Task> PreviewImportAsync(CmsFileIm } result.FriendlyName = friendlyName; result.OldUrl = "/" + source.Relative.Replace('\\', '/'); - if (!plannedNames.Add(finalName)) + if (sharesNameWithEarlierLine) { - result.Message = "Another line uses the same file name; this one will be renamed during import."; + result.Message = "Shares a name with an earlier line in this import; renamed to keep both."; } } return results; diff --git a/web/Areas/CMS/Services/CmsFileService.cs b/web/Areas/CMS/Services/CmsFileService.cs index 443e61a93..0b12d7832 100644 --- a/web/Areas/CMS/Services/CmsFileService.cs +++ b/web/Areas/CMS/Services/CmsFileService.cs @@ -220,6 +220,9 @@ public async Task CreateFileAsync(CmsFileCreateRequest request, IFor string tempPath = await _storage.SaveToTempAsync(file, ct); string finalPath; + // Overwriting an orphaned on-disk file (no DB row) destroys its bytes; snapshot them first + // so a later failure can roll the original back instead of leaving nothing in its place. + string? overwriteBackup = null; try { if (dbKey != null) @@ -234,6 +237,10 @@ public async Task CreateFileAsync(CmsFileCreateRequest request, IFor throw new InvalidOperationException( $"{uploadName} belongs to an existing file record; replace its content by editing that file."); } + if (_storage.ManagedFileExists(targetPath)) + { + overwriteBackup = _storage.BackupManagedFile(targetPath); + } _storage.ReplaceInPlace(tempPath, targetPath); finalPath = targetPath; } @@ -253,7 +260,9 @@ public async Task CreateFileAsync(CmsFileCreateRequest request, IFor string friendlyName = CmsFileNaming.BuildFriendlyName(request.Folder, Path.GetFileName(finalPath)); if (await _context.Files.AnyAsync(f => f.FriendlyName == friendlyName, ct)) { - _storage.DeleteManagedFile(finalPath); + // The new bytes are already on disk; reconcile the store (restore an overwritten + // original, or delete a fresh copy) before failing so nothing is left behind. + ReconcileStoreAfterFailedCreate(finalPath, overwriteBackup); throw new InvalidOperationException($"A file with the name {friendlyName} already exists."); } @@ -284,18 +293,31 @@ public async Task CreateFileAsync(CmsFileCreateRequest request, IFor _context.Files.Add(entity); _audit.Audit(entity, CmsFileAuditActions.AddFile, BuildCreateDetail(entity)); _audit.Audit(entity, CmsFileAuditActions.UploadFile, "NewFile"); + bool saved = false; try { await _context.SaveChangesAsync(ct); + saved = true; } catch (Exception) { - // The upload is already in the managed store; any failed save would orphan it on - // disk, so drop the pending changes and remove the stored copy before rethrowing. + // A failed save never persisted the record. Drop the pending changes, then reconcile + // the managed store: restore an overwritten file's original bytes, or remove the + // freshly stored copy, so nothing is left orphaned or destroyed. _context.ChangeTracker.Clear(); - _storage.DeleteManagedFile(finalPath); + ReconcileStoreAfterFailedCreate(finalPath, overwriteBackup); throw; } + finally + { + // Only clean up on a committed save (the backup is then obsolete). On a failed save the + // reconcile above owns the backup: a successful restore already consumed it via Move, and + // a FAILED restore must keep it as the last copy of the original bytes. + if (saved && overwriteBackup != null && System.IO.File.Exists(overwriteBackup)) + { + System.IO.File.Delete(overwriteBackup); + } + } var names = await GetNamesByIamIdAsync(entity.FileToPeople.Select(p => p.IamId), ct); return CmsFileMapper.ToCmsFileDto(entity, names); @@ -309,9 +331,12 @@ public async Task CreateFileAsync(CmsFileCreateRequest request, IFor return null; } - var changes = new List(); - bool encrypt = request.Encrypt ?? false; - bool allowPublicAccess = request.AllowPublicAccess ?? false; + // Validate a replacement upload before anything touches the file on disk, so a bad type + // can't leave the file half-transformed or strand a backup copy. + if (file != null) + { + ValidateReplacementUpload(entity, file); + } // The crypto toggle and any replacement upload below transform the file on disk before // the metadata save. Capture the pre-change state so a failed save can reconcile the @@ -319,8 +344,59 @@ public async Task CreateFileAsync(CmsFileCreateRequest request, IFor bool originalEncrypted = entity.Encrypted; string? originalKey = entity.Key; - // Encryption toggle happens before any file replacement so the replacement upload - // is written with the file's final encryption state. + // A replacement overwrites the bytes on disk before the save commits. Snapshot the + // original file first (it carries the original encryption state too) so a failed save + // can roll it back; restoring it supersedes the crypto-only reconcile below. + string? originalFileBackup = file != null && _storage.ManagedFileExists(entity.FilePath) + ? _storage.BackupManagedFile(entity.FilePath) + : null; + + var changes = new List(); + ApplyEncryptionToggle(entity, request.Encrypt ?? false, changes); + ApplyScalarFieldChanges(entity, request, changes); + + ApplyPermissionDeltas(entity, CleanList(request.Permissions), changes); + ApplyPersonDeltas(entity, CleanList(request.IamIds), changes); + + if (file != null) + { + await WriteReplacementUploadAsync(entity, file, changes, ct); + } + + entity.ModifiedOn = DateTime.Now; + entity.ModifiedBy = CurrentLoginId(); + + _audit.Audit(entity, CmsFileAuditActions.EditFile, string.Join("; ", changes)); + await SaveUpdateAndReconcileAsync(entity, originalFileBackup, originalEncrypted, originalKey); + + var names = await GetNamesByIamIdAsync(entity.FileToPeople.Select(p => p.IamId), ct); + return CmsFileMapper.ToCmsFileDto(entity, names); + } + + /// + /// A replacement upload keeps the record's name and path, so its bytes must be an allowed + /// type and match the original extension; otherwise the stored file would hold mismatched + /// content and serve a corrupt download. Runs before anything touches disk. + /// + private static void ValidateReplacementUpload(File entity, IFormFile file) + { + if (!CmsFileTypes.IsAllowedFileName(file.FileName)) + { + throw new ArgumentException("File type is not allowed."); + } + string replacementExt = Path.GetExtension(entity.FilePath); + if (!string.Equals(replacementExt, Path.GetExtension(file.FileName), StringComparison.OrdinalIgnoreCase)) + { + throw new ArgumentException($"The replacement file must be the same type as the original ({replacementExt})."); + } + } + + /// + /// Applies the encryption toggle in place before any replacement upload, so the replacement + /// is written with the file's final encryption state. Records the change for the audit trail. + /// + private void ApplyEncryptionToggle(File entity, bool encrypt, List changes) + { if (encrypt && !entity.Encrypted) { string dbKey = _encryption.GenerateKeyForDb(); @@ -342,13 +418,21 @@ public async Task CreateFileAsync(CmsFileCreateRequest request, IFor entity.Encrypted = false; changes.Add("Decrypted file"); } + } + /// + /// Applies the description, public-access, and Old URL edits, recording each that actually + /// changes for the audit trail. + /// + private static void ApplyScalarFieldChanges(File entity, CmsFileUpdateRequest request, List changes) + { string newDescription = request.Description ?? string.Empty; if (entity.Description != newDescription) { entity.Description = newDescription; changes.Add("Description updated"); } + bool allowPublicAccess = request.AllowPublicAccess ?? false; if (entity.AllowPublicAccess != allowPublicAccess) { entity.AllowPublicAccess = allowPublicAccess; @@ -360,72 +444,86 @@ public async Task CreateFileAsync(CmsFileCreateRequest request, IFor entity.OldUrl = newOldUrl; changes.Add("Old URL updated"); } + } - ApplyPermissionDeltas(entity, CleanList(request.Permissions), changes); - ApplyPersonDeltas(entity, CleanList(request.IamIds), changes); - - if (file != null) + /// + /// Writes a replacement upload over the record's file: encrypts the temp copy to the file's + /// current state if needed, swaps it in, audits the replacement, and revives a soft-deleted + /// record (the overwrite-conflict flow). The temp copy is always cleaned up. + /// + private async Task WriteReplacementUploadAsync(File entity, IFormFile file, List changes, CancellationToken ct) + { + string tempPath = await _storage.SaveToTempAsync(file, ct); + try { - if (!CmsFileTypes.IsAllowedFileName(file.FileName)) + if (entity.Encrypted && !string.IsNullOrEmpty(entity.Key)) { - throw new ArgumentException("File type is not allowed."); + _encryption.EncryptFileInPlace(tempPath, entity.Key); } - // The replacement keeps the existing record's name and path, so its bytes must - // stay the same type; a mismatched extension would leave the stored .pdf holding - // .png bytes and serve a corrupt download. - string currentExt = Path.GetExtension(entity.FilePath); - if (!string.Equals(currentExt, Path.GetExtension(file.FileName), StringComparison.OrdinalIgnoreCase)) - { - throw new ArgumentException($"The replacement file must be the same type as the original ({currentExt})."); - } - string tempPath = await _storage.SaveToTempAsync(file, ct); - try - { - if (entity.Encrypted && !string.IsNullOrEmpty(entity.Key)) - { - _encryption.EncryptFileInPlace(tempPath, entity.Key); - } - _storage.ReplaceInPlace(tempPath, entity.FilePath); - } - finally - { - if (System.IO.File.Exists(tempPath)) - { - System.IO.File.Delete(tempPath); - } - } - _audit.Audit(entity, CmsFileAuditActions.UploadFile, "ReplacingFile"); - - // Uploading new content into a soft-deleted record (the overwrite-conflict - // flow) makes it live again; metadata-only edits leave deletion state alone. - if (entity.DeletedOn != null) + _storage.ReplaceInPlace(tempPath, entity.FilePath); + } + finally + { + if (System.IO.File.Exists(tempPath)) { - entity.DeletedOn = null; - changes.Add("Restored file"); + System.IO.File.Delete(tempPath); } } + _audit.Audit(entity, CmsFileAuditActions.UploadFile, "ReplacingFile"); - entity.ModifiedOn = DateTime.Now; - entity.ModifiedBy = CurrentLoginId(); + // Uploading new content into a soft-deleted record (the overwrite-conflict flow) makes + // it live again; metadata-only edits leave deletion state alone. + if (entity.DeletedOn != null) + { + entity.DeletedOn = null; + changes.Add("Restored file"); + } + } - _audit.Audit(entity, CmsFileAuditActions.EditFile, string.Join("; ", changes)); + /// + /// Persists the metadata edit, reconciling the on-disk file when the save fails: restore the + /// pre-replacement backup (which also restores the original encryption state) or, with no + /// replacement, revert just the crypto transform. The backup is deleted once the save commits; + /// if a restore fails it is left in place as the last copy of the original bytes. + /// + private async Task SaveUpdateAndReconcileAsync(File entity, string? originalFileBackup, + bool originalEncrypted, string? originalKey) + { + bool saved = false; try { // The file on disk may already be in its new encryption state; cancelling the save // would strand it without a matching key, so it runs to completion even if aborted. await _context.SaveChangesAsync(CancellationToken.None); + saved = true; } catch (Exception) { // Any failure type means the new state was not persisted; drop the pending changes - // and reconcile the on-disk file back to its original encryption state before rethrowing. + // and reconcile the on-disk file back to its original state before rethrowing. _context.ChangeTracker.Clear(); - RevertCryptoOnSaveFailure(entity, originalEncrypted, originalKey); + if (originalFileBackup != null) + { + // A replacement overwrote the file; restoring the original bytes also restores + // the original encryption state, so the crypto-only reconcile is skipped. + RestoreOriginalFileOnSaveFailure(entity, originalFileBackup); + } + else + { + RevertCryptoOnSaveFailure(entity, originalEncrypted, originalKey); + } throw; } - - var names = await GetNamesByIamIdAsync(entity.FileToPeople.Select(p => p.IamId), ct); - return CmsFileMapper.ToCmsFileDto(entity, names); + finally + { + // Only clean up on a committed save (the backup is then obsolete). On a failed save the + // reconcile above owns the backup: a successful restore already consumed it via Move, and + // a FAILED restore must keep it as the last copy of the original bytes. + if (saved && originalFileBackup != null && System.IO.File.Exists(originalFileBackup)) + { + System.IO.File.Delete(originalFileBackup); + } + } } /// @@ -460,6 +558,53 @@ private void RevertCryptoOnSaveFailure(File entity, bool originalEncrypted, stri } } + /// + /// A failed save after a replacement upload leaves the new bytes on disk while the database + /// rolled back to the original record. Move the pre-replacement backup back into place so a + /// download serves the original content (and its original encryption state). A failure to + /// restore is logged as critical (with the preserved backup path) rather than swallowed; the + /// caller leaves the backup in place so it remains the last copy of the original bytes. + /// + private void RestoreOriginalFileOnSaveFailure(File entity, string backupPath) + { + try + { + _storage.ReplaceInPlace(backupPath, entity.FilePath); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or ArgumentException) + { + _logger.LogCritical(ex, "CMS file {FileGuid} could not be restored after a failed save; " + + "the on-disk content may not match the database. The original bytes are preserved " + + "at {BackupPath} for manual recovery.", entity.FileGuid, backupPath); + } + } + + /// + /// A create that has already written its bytes but then fails (friendly-name clash or save + /// error) must not leave the store changed. When the write overwrote an orphaned file, move + /// its pre-overwrite backup back so the original content survives; otherwise just delete the + /// freshly stored copy. A failed restore is logged as critical (with the preserved backup + /// path) rather than swallowed; the caller leaves the backup in place for recovery. + /// + private void ReconcileStoreAfterFailedCreate(string finalPath, string? overwriteBackup) + { + if (overwriteBackup == null) + { + _storage.DeleteManagedFile(finalPath); + return; + } + try + { + _storage.ReplaceInPlace(overwriteBackup, finalPath); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or ArgumentException) + { + _logger.LogCritical(ex, "CMS overwrite at {FilePath} could not be restored after a failed " + + "create; the original bytes are preserved at {BackupPath} for manual recovery.", + Viper.Classes.Utilities.LogSanitizer.SanitizeString(finalPath), overwriteBackup); + } + } + public async Task SoftDeleteFileAsync(Guid fileGuid, CancellationToken ct = default) { var entity = await LoadFileAsync(fileGuid, tracking: true, ct); diff --git a/web/Areas/CMS/Services/CmsFileStorageService.cs b/web/Areas/CMS/Services/CmsFileStorageService.cs index 4e8714349..518c79526 100644 --- a/web/Areas/CMS/Services/CmsFileStorageService.cs +++ b/web/Areas/CMS/Services/CmsFileStorageService.cs @@ -30,9 +30,11 @@ public interface ICmsFileStorageService /// /// The name MoveIntoPlace would store folder\fileName under: the name itself when free, - /// otherwise the first free name_0..name_999 candidate. + /// otherwise the first free name_0..name_999 candidate. + /// (optional) are treated as already taken, so callers planning several writes in one + /// batch get the same unique names the sequential moves would actually assign. /// - string GetAvailableFileName(string folder, string fileName); + string GetAvailableFileName(string folder, string fileName, IReadOnlySet? reservedNames = null); /// Resolved managed path for folder\fileName (validated to stay under the root). string BuildManagedPath(string folder, string fileName); @@ -50,6 +52,13 @@ public interface ICmsFileStorageService /// Overwrite the managed file at with a temp file. void ReplaceInPlace(string tempPath, string existingFilePath); + /// + /// Copy a managed file to a temp backup outside the storage root and return its path. Pair + /// with ReplaceInPlace(backupPath, originalPath) to roll the original bytes back into place + /// after a failed save. + /// + string BackupManagedFile(string filePath); + /// Delete a file, verifying it lives under the storage root first. void DeleteManagedFile(string filePath); @@ -134,10 +143,12 @@ public bool FileNameInUse(string folder, string fileName) return _context.Files.Any(f => f.FilePath == targetPath); } - public string GetAvailableFileName(string folder, string fileName) + public string GetAvailableFileName(string folder, string fileName, IReadOnlySet? reservedNames = null) { string finalName = Path.GetFileName(fileName); - return FileNameInUse(folder, finalName) ? GetUniqueFileName(folder, finalName) : finalName; + bool taken = FileNameInUse(folder, finalName) + || (reservedNames != null && reservedNames.Contains(finalName)); + return taken ? GetUniqueFileName(folder, finalName, reservedNames) : finalName; } public string BuildManagedPath(string folder, string fileName) @@ -189,6 +200,16 @@ public void ReplaceInPlace(string tempPath, string existingFilePath) System.IO.File.Move(tempPath, existingFilePath, overwrite: true); } + public string BackupManagedFile(string filePath) + { + AssertUnderRoot(filePath); + string tempFolder = Path.Join(Path.GetTempPath(), "Viper-CMS-Uploads"); + System.IO.Directory.CreateDirectory(tempFolder); + string backupPath = Path.Join(tempFolder, Guid.NewGuid().ToString("N")); + System.IO.File.Copy(filePath, backupPath, overwrite: true); + return backupPath; + } + public void DeleteManagedFile(string filePath) { AssertUnderRoot(filePath); @@ -203,7 +224,7 @@ public bool ManagedFileExists(string filePath) return IsUnderRoot(filePath) && System.IO.File.Exists(filePath); } - private string GetUniqueFileName(string folder, string fileName) + private string GetUniqueFileName(string folder, string fileName, IReadOnlySet? reservedNames = null) { string baseName = Path.GetFileNameWithoutExtension(fileName); string extension = Path.GetExtension(fileName); @@ -211,7 +232,8 @@ private string GetUniqueFileName(string folder, string fileName) for (int i = 0; i < 1000; i++) { string candidate = $"{baseName}_{i}{extension}"; - if (!FileNameInUse(folder, candidate)) + if (!FileNameInUse(folder, candidate) + && (reservedNames == null || !reservedNames.Contains(candidate))) { return candidate; } From 2418dfb1ae90cb95e4f53d93783a21b6e401706c Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Mon, 29 Jun 2026 16:26:01 -0700 Subject: [PATCH 02/24] VPR-59 fix(cms): reject protocol-relative URLs in link validation - Treats "//host" as off-site and blocks it; backslash variants ("/\", "\\", "\/") normalize to the same bypass and are rejected too. Ordinary relative paths still validate. --- test/CMS/SafeUrlAttributeTests.cs | 14 ++++++++++++++ web/Areas/CMS/Validation/SafeUrlAttribute.cs | 18 +++++++++++++++--- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/test/CMS/SafeUrlAttributeTests.cs b/test/CMS/SafeUrlAttributeTests.cs index 0cdd5eb4a..3c857e19d 100644 --- a/test/CMS/SafeUrlAttributeTests.cs +++ b/test/CMS/SafeUrlAttributeTests.cs @@ -62,6 +62,20 @@ public void RelativeMode_RejectsColonInFirstSegment() Assert.NotEqual(ValidationResult.Success, Validate("foo:bar/baz", allowRelative: true)); } + [Theory] + [InlineData("//evil.example/path")] + [InlineData("//evil.example")] + [InlineData("/\\evil.example/path")] + [InlineData("\\\\evil.example\\path")] + [InlineData("\\/evil.example")] + public void BothModes_RejectProtocolRelativeUrls(string url) + { + // A scheme-less "//host" (or its backslash variants) navigates off-site, so it must be + // rejected even in relative mode, where bare relative paths are otherwise allowed. + Assert.NotEqual(ValidationResult.Success, Validate(url, allowRelative: true)); + Assert.NotEqual(ValidationResult.Success, Validate(url, allowRelative: false)); + } + [Fact] public void RelativeMode_AllowsColonAfterFirstSegment() { diff --git a/web/Areas/CMS/Validation/SafeUrlAttribute.cs b/web/Areas/CMS/Validation/SafeUrlAttribute.cs index 7055a784e..91c025f5d 100644 --- a/web/Areas/CMS/Validation/SafeUrlAttribute.cs +++ b/web/Areas/CMS/Validation/SafeUrlAttribute.cs @@ -33,9 +33,21 @@ public sealed class SafeUrlAttribute : ValidationAttribute // rooted paths like "/CMS/files" (parsed as file:// on Unix) if (!url.Split('/', '?', '#')[0].Contains(':')) { - return AllowRelative - ? ValidationResult.Success - : new ValidationResult("URL must be a full address starting with http, https, mailto, or tel."); + if (!AllowRelative) + { + return new ValidationResult("URL must be a full address starting with http, https, mailto, or tel."); + } + + // Reject protocol-relative ("network-path") references like "//evil.example/x": + // a browser navigates those off-site even though they carry no scheme. Backslashes + // are normalized to forward slashes when parsing URLs, so "/\", "\\", and "\/" are + // equivalent bypasses and must be rejected too. + if (url.Length >= 2 && (url[0] == '/' || url[0] == '\\') && (url[1] == '/' || url[1] == '\\')) + { + return new ValidationResult("URL must not start with // (that would point to another site)."); + } + + return ValidationResult.Success; } return Uri.TryCreate(url, UriKind.Absolute, out var uri) From 9f3fbdfd8c87cfeba89fc1cfb6a506c37f437133 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Mon, 29 Jun 2026 16:28:31 -0700 Subject: [PATCH 03/24] VPR-59 feat(cms): open the Home hub to granular-permission users - The /CMS/Home route and the /CMS/ root redirect both accept any granular CMS permission (Files, Content Blocks, Left Nav, ...), not just base SVMSecure.CMS, reading one shared set so the guard and the canonicalization can't drift. --- .../__tests__/router-canonicalization.test.ts | 50 +++++++++++++++++++ VueApp/src/CMS/router/index.ts | 8 +-- VueApp/src/CMS/router/routes.ts | 18 ++++++- 3 files changed, 71 insertions(+), 5 deletions(-) create mode 100644 VueApp/src/CMS/__tests__/router-canonicalization.test.ts diff --git a/VueApp/src/CMS/__tests__/router-canonicalization.test.ts b/VueApp/src/CMS/__tests__/router-canonicalization.test.ts new file mode 100644 index 000000000..7559c6400 --- /dev/null +++ b/VueApp/src/CMS/__tests__/router-canonicalization.test.ts @@ -0,0 +1,50 @@ +import { createPinia, setActivePinia } from "pinia" +import { useUserStore } from "@/store/UserStore" +import { cmsRouter } from "@/CMS/router" + +// The real beforeEach guard calls requireLogin, which hits the network and needs a Quasar/inject +// context. Stub it (Vitest hoists this above the imports) so the test exercises only the +// permission-driven canonicalization, not the auth plumbing. +vi.mock("@/composables/RequireLogin", () => ({ + useRequireLogin: () => ({ requireLogin: () => Promise.resolve(true) }), + getLoginUrl: () => ({ value: "" }), +})) + +// Park on a neutral route first so the push to the area root is never a redundant navigation +// (which would resolve to a NavigationFailure and leave currentRoute unchanged). +async function goToAreaRoot(): Promise { + await cmsRouter.push("/__reset__") + await cmsRouter.push("/CMS/") +} + +describe("CMS area-root canonicalization", () => { + beforeEach(() => { + setActivePinia(createPinia()) + }) + + it("redirects base SVMSecure.CMS users from /CMS/ to the Home hub", async () => { + useUserStore().setPermissions(["SVMSecure.CMS"]) + + await goToAreaRoot() + + expect(cmsRouter.currentRoute.value.name).toBe("CmsHome") + }) + + it("redirects granular-only users (no base SVMSecure.CMS) to the Home hub", async () => { + // The regression: AllFiles/ManageNavigation/etc. users can enter the area but used to be + // stranded on /CMS/ because canonicalization only checked the base permission. + useUserStore().setPermissions(["SVMSecure.CMS.AllFiles"]) + + await goToAreaRoot() + + expect(cmsRouter.currentRoute.value.name).toBe("CmsHome") + }) + + it("leaves visitors with no CMS permissions on the CmsAuth landing", async () => { + useUserStore().setPermissions([]) + + await goToAreaRoot() + + expect(cmsRouter.currentRoute.value.name).toBe("CmsAuth") + }) +}) diff --git a/VueApp/src/CMS/router/index.ts b/VueApp/src/CMS/router/index.ts index b93c5bca5..94a0fd030 100644 --- a/VueApp/src/CMS/router/index.ts +++ b/VueApp/src/CMS/router/index.ts @@ -1,5 +1,5 @@ import { createSpaRouter } from "@/shared/create-spa-router" -import { routes } from "./routes" +import { routes, cmsHomePermissions } from "./routes" import { useRequireLogin } from "@/composables/RequireLogin" import { checkHasOnePermission } from "@/composables/CheckPagePermission" @@ -19,8 +19,10 @@ router.beforeEach(async (to) => { } // Canonicalize the area root for CMS users so the left nav "Home" link // highlights; unauthenticated and permission-less visitors stay on the - // CmsAuth landing (redirecting them would force a login or loop). - if (to.name === "CmsAuth" && checkHasOnePermission(["SVMSecure.CMS"])) { + // CmsAuth landing (redirecting them would force a login or loop). Gate on + // the same set as the CmsHome route so granular-only users (no base + // SVMSecure.CMS) canonicalize too, instead of being stranded on /CMS/. + if (to.name === "CmsAuth" && checkHasOnePermission(cmsHomePermissions)) { return { name: "CmsHome" } } }) diff --git a/VueApp/src/CMS/router/routes.ts b/VueApp/src/CMS/router/routes.ts index e07154132..2c4febff3 100644 --- a/VueApp/src/CMS/router/routes.ts +++ b/VueApp/src/CMS/router/routes.ts @@ -2,6 +2,17 @@ import ViperLayout from "@/layouts/ViperLayout.vue" import type { RouteLocationNormalized } from "vue-router" import { checkHasOnePermission } from "@/composables/CheckPagePermission" +// Granular CMS permissions that grant the Home hub. The hub adapts to whichever of these the user +// holds, and CmsNavMenu links it for any of them, so the Home route guard and the area-root +// canonicalization (router/index.ts) gate on this one shared set rather than two lists that drift. +const cmsHomePermissions = [ + "SVMSecure.CMS", + "SVMSecure.CMS.ManageContentBlocks", + "SVMSecure.CMS.CreateContentBlock", + "SVMSecure.CMS.AllFiles", + "SVMSecure.CMS.ManageNavigation", +] + const routes = [ { path: "/CMS/", @@ -11,7 +22,10 @@ const routes = [ }, { path: "/CMS/Home", - meta: { layout: ViperLayout, permissions: ["SVMSecure.CMS"] }, + meta: { + layout: ViperLayout, + permissions: cmsHomePermissions, + }, component: () => import("@/CMS/pages/CmsHome.vue"), name: "CmsHome", }, @@ -92,4 +106,4 @@ const routes = [ }, ] -export { routes } +export { routes, cmsHomePermissions } From 8ddec7ca8a08f8c5f976aa77049c6392b4bd49dc Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Mon, 29 Jun 2026 16:29:42 -0700 Subject: [PATCH 04/24] VPR-59 refactor(cms): align badges, tokens, and a11y across CMS pages - Adopt the shared StatusBadge over raw q-badge and an ad-hoc palette, and replace hard-coded px/hex with rem and brand tokens. - Improve a11y (SR-hidden status icons, grouped/labelled left-nav rows) and keep button labels visible while loading. - ContentBlock watches only its name prop (drops a duplicate fetch), renders null-safe, and shares heading styles via the global sheet. - ContentBlockEdit skips the section-path/file APIs for create-only users so they don't fire requests that can only 403; the left-nav list now surfaces load errors. --- .../CMS/__tests__/link-collections.test.ts | 21 ++-- VueApp/src/CMS/__tests__/test-utils.ts | 27 +++++- VueApp/src/CMS/components/ContentBlock.vue | 33 ++----- VueApp/src/CMS/components/FileFormDialog.vue | 20 +++- VueApp/src/CMS/components/Link.vue | 36 +++---- VueApp/src/CMS/components/LinkCollections.vue | 96 +++++++++---------- VueApp/src/CMS/components/RecentActivity.vue | 1 - VueApp/src/CMS/components/StatusIcon.vue | 1 + VueApp/src/CMS/pages/CmsHome.vue | 2 +- VueApp/src/CMS/pages/ContentBlockEdit.vue | 13 ++- VueApp/src/CMS/pages/ContentBlockHistory.vue | 5 +- VueApp/src/CMS/pages/FileAuditLog.vue | 2 +- VueApp/src/CMS/pages/ImportFiles.vue | 12 ++- VueApp/src/CMS/pages/LeftNavEdit.vue | 18 ++-- VueApp/src/CMS/pages/LeftNavMenus.vue | 16 +++- .../src/CMS/pages/ManageLinkCollections.vue | 28 +++--- VueApp/src/styles/base.css | 11 +++ 17 files changed, 190 insertions(+), 152 deletions(-) diff --git a/VueApp/src/CMS/__tests__/link-collections.test.ts b/VueApp/src/CMS/__tests__/link-collections.test.ts index 681fed2c5..c622807ec 100644 --- a/VueApp/src/CMS/__tests__/link-collections.test.ts +++ b/VueApp/src/CMS/__tests__/link-collections.test.ts @@ -176,26 +176,21 @@ describe("LinkCollections.vue - tag category filter", () => { describe("LinkCollections.vue - group by tag category", () => { beforeEach(() => primeFetch()) - it("getInGroup returns only links whose group tag matches the group value", async () => { + it("buckets links by their value in the group-by tag category", async () => { const wrapper = await mountLoaded({ linkCollectionName: "Resources", groupByTagCategory: "Subject" }) - const vm = wrapper.vm as unknown as { - getInGroup: (links: Link[], group: string) => Link[] - filteredLinks: Link[] - } - const anatomy = vm.getInGroup(vm.filteredLinks, "Anatomy").map((l) => l.title) + const vm = wrapper.vm as unknown as { groupedLinks: Map } + const anatomy = (vm.groupedLinks.get("Anatomy") ?? []).map((l) => l.title) expect(anatomy.toSorted()).toEqual(["Anatomy Atlas", "Anatomy Guide"]) - const pharm = vm.getInGroup(vm.filteredLinks, "Pharmacology").map((l) => l.title) + const pharm = (vm.groupedLinks.get("Pharmacology") ?? []).map((l) => l.title) expect(pharm).toEqual(["Pharmacology Notes"]) }) - it("getInGroup returns all links unchanged when no group category is set", async () => { + it("renders all links and builds no buckets when no group category is set", async () => { primeFetch() const wrapper = await mountLoaded({ linkCollectionName: "Resources" }) - const vm = wrapper.vm as unknown as { - getInGroup: (links: Link[], group: string) => Link[] - filteredLinks: Link[] - } - expect(vm.getInGroup(vm.filteredLinks, "anything")).toHaveLength(3) + expect(linkCardTitles(wrapper)).toHaveLength(3) + const vm = wrapper.vm as unknown as { groupedLinks: Map } + expect(vm.groupedLinks.size).toBe(0) }) it("computes the sorted set of group header values", async () => { diff --git a/VueApp/src/CMS/__tests__/test-utils.ts b/VueApp/src/CMS/__tests__/test-utils.ts index 697f62ba2..0cb4bc343 100644 --- a/VueApp/src/CMS/__tests__/test-utils.ts +++ b/VueApp/src/CMS/__tests__/test-utils.ts @@ -3,20 +3,33 @@ import type { ComponentMountingOptions } from "@vue/test-utils" import { createRouter, createWebHistory } from "vue-router" import type { Router, RouteRecordRaw } from "vue-router" import { Quasar, Notify, Dialog } from "quasar" +import { createPinia, setActivePinia } from "pinia" import { nextTick } from "vue" +import { useUserStore } from "@/store/UserStore" /** * Shared test utilities for CMS component/page tests. * - * CMS has no Pinia store (state is local to each component), so these helpers - * only need a Quasar plugin set, a test router, and an "apiURL" provide value - * that mirrors what cms.ts injects at runtime. + * CMS keeps page state local, but some pages read the signed-in user's permissions from the + * shared user store (e.g. ContentBlockEdit gates its section-path and file pickers). So the + * helpers install Pinia and seed a full CMS admin so mounted pages behave as they would for a + * manager (the role these tests assume). */ // Components build request URLs as inject("apiURL") + "cms/...". Tests assert against // this prefix, so keep it stable and obvious. const TEST_API_URL = "/api/" +// The CMS area's full permission set; mounted pages default to this so existing tests see the +// same UI a manager would. Mirrors the route meta in src/CMS/router/routes.ts. +const CMS_ADMIN_PERMISSIONS = [ + "SVMSecure.CMS", + "SVMSecure.CMS.ManageContentBlocks", + "SVMSecure.CMS.CreateContentBlock", + "SVMSecure.CMS.AllFiles", + "SVMSecure.CMS.ManageNavigation", +] + // Route table mirroring the real CMS route names/paths (src/CMS/router/routes.ts) so // router-link :to and programmatic navigation resolve exactly as they do in production. const stub = { template: "
" } @@ -41,9 +54,13 @@ function createTestRouter(): Router { } // Generic mount wrapper: registers Quasar (with Notify + Dialog so components that call -// $q.notify / $q.dialog don't blow up), a router, and the apiURL provide. +// $q.notify / $q.dialog don't blow up), Pinia (seeded as a CMS admin), a router, and the +// apiURL provide. function mountCms(component: T, options: ComponentMountingOptions = {}, router?: Router) { const testRouter = router ?? createTestRouter() + const pinia = createPinia() + setActivePinia(pinia) + useUserStore().setPermissions(CMS_ADMIN_PERMISSIONS) const { global: globalOptions = {}, ...rest } = options as Record & { global?: Record } @@ -52,7 +69,7 @@ function mountCms(component: T, options: ComponentMountingOptions = {}, ro { ...rest, global: { - plugins: [[Quasar, { plugins: { Notify, Dialog } }], testRouter], + plugins: [[Quasar, { plugins: { Notify, Dialog } }], testRouter, pinia], provide: { apiURL: TEST_API_URL }, ...globalOptions, }, diff --git a/VueApp/src/CMS/components/ContentBlock.vue b/VueApp/src/CMS/components/ContentBlock.vue index e37953b89..35464cf1e 100644 --- a/VueApp/src/CMS/components/ContentBlock.vue +++ b/VueApp/src/CMS/components/ContentBlock.vue @@ -23,33 +23,12 @@ const contentBlock: Ref = ref(null) async function loadContentBlock() { const { get } = useFetch() const r = await get(import.meta.env.VITE_API_URL + "cms/content/fn/" + props.contentBlockName) - contentBlock.value = r.result + contentBlock.value = r.success ? r.result : null } -watch( - () => props, - () => { - loadContentBlock() - }, - { immediate: true, deep: true }, -) - -loadContentBlock() +// Reload only when the block name changes. props is reactive, so watching the +// specific source (instead of a deep watch on props) avoids the duplicate +// immediate fetch and unrelated re-runs. Headings for the sanitized HTML are +// styled globally in styles/base.css (shared with the diff view). +watch(() => props.contentBlockName, loadContentBlock, { immediate: true }) - - diff --git a/VueApp/src/CMS/components/FileFormDialog.vue b/VueApp/src/CMS/components/FileFormDialog.vue index 5a9c42e23..53db92575 100644 --- a/VueApp/src/CMS/components/FileFormDialog.vue +++ b/VueApp/src/CMS/components/FileFormDialog.vue @@ -149,7 +149,15 @@ dense no-caps :loading="saving" - /> + > + + @@ -204,7 +212,15 @@ :label="conflictChoice === 'rename' ? 'Upload with new name' : 'Overwrite'" :loading="saving" @click="resolveConflict" - /> + > + + diff --git a/VueApp/src/CMS/components/Link.vue b/VueApp/src/CMS/components/Link.vue index b03cae42e..6cc2e2b42 100644 --- a/VueApp/src/CMS/components/Link.vue +++ b/VueApp/src/CMS/components/Link.vue @@ -31,14 +31,13 @@ v-for="tag in props.link.linkTags" :key="tag.linkTagId" > - {{ tag.value }} - + @@ -49,6 +48,7 @@ import { computed } from "vue" import type { Link, LinkCollection } from "@/CMS/types" import { safeHref } from "@/CMS/utils/url" +import StatusBadge from "@/components/StatusBadge.vue" const props = defineProps<{ link: Link linkCollection: LinkCollection @@ -57,26 +57,20 @@ const props = defineProps<{ const hrefForLink = computed(() => safeHref(props.link.url)) const isSafe = computed(() => hrefForLink.value !== "#") -// Each tag category's sortOrder (1-based) indexes this palette. Gold and Tahoe -// are light enough to require dark text for WCAG AA contrast (≥4.5:1). -const TAG_STYLES: ReadonlyArray<{ color: string; textColor: string }> = [ - { color: "warning", textColor: "dark" }, - { color: "secondary", textColor: "white" }, - { color: "negative", textColor: "white" }, - { color: "positive", textColor: "white" }, - { color: "info", textColor: "dark" }, - { color: "primary", textColor: "white" }, -] +// Each tag category's sortOrder (1-based) indexes this palette of Quasar brand +// roles. StatusBadge derives the WCAG-AA text color for each (dark on the light +// warning/info tints, white on the rest). +const TAG_COLORS = ["warning", "secondary", "negative", "positive", "info", "primary"] as const -function getTagStyle(order: number) { - const idx = order >= 1 ? (order - 1) % TAG_STYLES.length : 0 - return TAG_STYLES[idx]! +function getTagColor(order: number) { + const idx = order >= 1 ? (order - 1) % TAG_COLORS.length : 0 + return TAG_COLORS[idx] } diff --git a/VueApp/src/CMS/components/LinkCollections.vue b/VueApp/src/CMS/components/LinkCollections.vue index 22c79326f..5dc9a4a99 100644 --- a/VueApp/src/CMS/components/LinkCollections.vue +++ b/VueApp/src/CMS/components/LinkCollections.vue @@ -17,8 +17,7 @@ v-model="filtersExpandedComputed" icon="filter_list" label="Filters" - header-class="bg-grey-2 lt-md" - :header-style="$q.screen.gt.xs ? 'display: none' : ''" + header-class="bg-grey-2 lt-sm" class="q-mb-md" > @@ -81,27 +80,25 @@
@@ -149,7 +146,7 @@ async function getLinkCollection() { const { get } = useFetch() const baseUrl = import.meta.env.VITE_API_URL + "cms/linkcollections/" const r = await get(baseUrl + "?linkCollectionName=" + encodeURIComponent(props.linkCollectionName)) - linkCollection.value = r.result[0] as LinkCollection + linkCollection.value = r.success && Array.isArray(r.result) ? ((r.result[0] as LinkCollection) ?? null) : null if (linkCollection.value) { await loadLinks(linkCollection.value.linkCollectionId) await loadFilters() @@ -171,7 +168,7 @@ async function loadLinks(collectionId: number) { function loadFilters() { if (linkCollection.value !== null) { - for (var tagCat of linkCollection.value.linkCollectionTagCategories) { + for (const tagCat of linkCollection.value.linkCollectionTagCategories) { tagFilters.value.push({ linkCollectionTagCategoryId: tagCat.linkCollectionTagCategoryId, linkCollectionTagCategory: tagCat.linkCollectionTagCategory, @@ -179,19 +176,21 @@ function loadFilters() { selected: null, }) } - for (var l of links.value) { - for (var lt of l.linkTags) { - var t = tagFilters.value.find((t) => t.linkCollectionTagCategoryId === lt.linkCollectionTagCategoryId) - if (t) { - t.options.push(lt.value) + for (const l of links.value) { + for (const lt of l.linkTags) { + const filter = tagFilters.value.find( + (f) => f.linkCollectionTagCategoryId === lt.linkCollectionTagCategoryId, + ) + if (filter) { + filter.options.push(lt.value) } } } - for (var tf of tagFilters.value) { + for (const tf of tagFilters.value) { tf.options = tf.options.sort() } - for (var tagOptions of tagFilters.value) { + for (const tagOptions of tagFilters.value) { tagOptions.options = [...new Set(tagOptions.options)] if ( props.groupByTagCategory !== null && @@ -204,21 +203,32 @@ function loadFilters() { } } -function getInGroup(linksIn: Link[], groupByValue: string) { - if (props.groupByTagCategory === null || props.groupByTagCategory.length === 0 || groupById.value === null) { - return linksIn +// Bucket the filtered links by their value in the group-by category once per render, +// so the template does an O(1) Map.get per group instead of re-filtering every link +// (twice) for every group. +const groupedLinks = computed(() => { + const buckets = new Map() + if (groupById.value === null) { + return buckets } - return linksIn.filter((l) => { - const findTag = l.linkTags.find((lt) => { - return lt.linkCollectionTagCategoryId === groupById.value && lt.value === groupByValue - }) - return findTag !== undefined - }) -} + for (const value of groupByValues.value) { + buckets.set(value, []) + } + for (const link of filteredLinks.value) { + const seen = new Set() + for (const tag of link.linkTags) { + if (tag.linkCollectionTagCategoryId === groupById.value && !seen.has(tag.value)) { + seen.add(tag.value) + buckets.get(tag.value)?.push(link) + } + } + } + return buckets +}) function applyFilters() { filteredLinks.value = links.value - for (var tf of tagFilters.value) { + for (const tf of tagFilters.value) { if (tf.selected !== null && tf.selected !== "") { filteredLinks.value = filteredLinks.value.filter((fl: Link) => { const findTag = fl.linkTags.find((lt: LinkTag) => { @@ -253,19 +263,3 @@ watch(search, () => { getLinkCollection() - - diff --git a/VueApp/src/CMS/components/RecentActivity.vue b/VueApp/src/CMS/components/RecentActivity.vue index c25ec97df..6c2446d2a 100644 --- a/VueApp/src/CMS/components/RecentActivity.vue +++ b/VueApp/src/CMS/components/RecentActivity.vue @@ -34,7 +34,6 @@ :key="item.key" clickable :to="item.to" - role="link" class="q-px-none" > diff --git a/VueApp/src/CMS/components/StatusIcon.vue b/VueApp/src/CMS/components/StatusIcon.vue index 6237c192a..9dc5e659f 100644 --- a/VueApp/src/CMS/components/StatusIcon.vue +++ b/VueApp/src/CMS/components/StatusIcon.vue @@ -2,6 +2,7 @@ diff --git a/VueApp/src/CMS/pages/CmsHome.vue b/VueApp/src/CMS/pages/CmsHome.vue index f0d2a4d70..30be70ace 100644 --- a/VueApp/src/CMS/pages/CmsHome.vue +++ b/VueApp/src/CMS/pages/CmsHome.vue @@ -155,7 +155,7 @@ const showRecentActivity = computed(() => canManageBlocks.value || canManageFile display: grid; grid-template-columns: repeat(auto-fit, minmax(17rem, 1fr)); grid-auto-rows: 1fr; - gap: 16px; + gap: 1rem; } .cms-home-cards .q-card { diff --git a/VueApp/src/CMS/pages/ContentBlockEdit.vue b/VueApp/src/CMS/pages/ContentBlockEdit.vue index a3c523503..e9f60b504 100644 --- a/VueApp/src/CMS/pages/ContentBlockEdit.vue +++ b/VueApp/src/CMS/pages/ContentBlockEdit.vue @@ -105,9 +105,9 @@ class="col-auto" > Viewing an old version, save to restore it @@ -204,6 +204,7 @@ @@ -236,6 +237,7 @@ (route.params.id ? Number(route.params.id) : null)) const isNew = computed(() => blockId.value === null) @@ -590,7 +599,7 @@ onMounted(() => { // QEditor renders the focusable contenteditable as an inner element, so its accessible // name has to be set there rather than on the wrapper the "Content" label sits beside. contentEditorRef.value?.getContentEl()?.setAttribute("aria-labelledby", "content-editor-label") - loadSectionPaths() + if (canManageContent) loadSectionPaths() loadBlock() // loadBlock sets the baseline for an existing block after it loads; a brand-new form's // baseline is just the empty block, so capture it synchronously here. diff --git a/VueApp/src/CMS/pages/ContentBlockHistory.vue b/VueApp/src/CMS/pages/ContentBlockHistory.vue index 7edf1ba78..013c8d373 100644 --- a/VueApp/src/CMS/pages/ContentBlockHistory.vue +++ b/VueApp/src/CMS/pages/ContentBlockHistory.vue @@ -87,7 +87,7 @@ {{ blockLabel(cellProps.row) }} - {{ blockLabel(row) }} - .file-path { - max-width: 320px; + max-width: 20rem; display: inline-block; vertical-align: middle; } diff --git a/VueApp/src/CMS/pages/ImportFiles.vue b/VueApp/src/CMS/pages/ImportFiles.vue index 890ee04aa..031bad4f2 100644 --- a/VueApp/src/CMS/pages/ImportFiles.vue +++ b/VueApp/src/CMS/pages/ImportFiles.vue @@ -146,7 +146,15 @@ :disable="importableCount === 0" :loading="importing" @click="runImport" - /> + > + + @@ -240,7 +248,7 @@ const importableCount = computed(() => preview.value?.filter((p) => p.canImport) function previewStatus(row: ImportPreview) { if (!row.canImport) return { icon: "error", color: "negative", label: "Will be skipped" } - if (row.message) return { icon: "warning", color: "warning", label: "Ready" } + if (row.message) return { icon: "warning", color: "warning", label: "Ready (with note)" } return { icon: "check_circle", color: "positive", label: "Ready" } } diff --git a/VueApp/src/CMS/pages/LeftNavEdit.vue b/VueApp/src/CMS/pages/LeftNavEdit.vue index 7e7eb2a44..68e724443 100644 --- a/VueApp/src/CMS/pages/LeftNavEdit.vue +++ b/VueApp/src/CMS/pages/LeftNavEdit.vue @@ -107,7 +107,11 @@ @reorder="onItemsReorder" > + + ((resolve) => { $q.dialog({ title: "Delete Menu", - message: `Permanently delete "${menu.menuHeaderText}" and its ${menu.items.length} items? This cannot be undone.`, + message: `Permanently delete "${menu.menuHeaderText || "(untitled)"}" and its ${menu.items.length} ${inflect("item", menu.items.length)}? This cannot be undone.`, cancel: { label: "Cancel", flat: true }, persistent: true, ok: { label: "Delete Menu", color: "negative", unelevated: true }, diff --git a/VueApp/src/CMS/pages/ManageLinkCollections.vue b/VueApp/src/CMS/pages/ManageLinkCollections.vue index 9d2bdbf6c..fda28f2a2 100644 --- a/VueApp/src/CMS/pages/ManageLinkCollections.vue +++ b/VueApp/src/CMS/pages/ManageLinkCollections.vue @@ -91,6 +91,7 @@ = {} - for (var lt of r.linkTags || []) { + for (const lt of r.linkTags || []) { if (tags[lt.linkCollectionTagCategoryId] === undefined) { tags[lt.linkCollectionTagCategoryId] = lt.value } else { @@ -806,14 +807,15 @@ loadCollections() diff --git a/VueApp/src/styles/base.css b/VueApp/src/styles/base.css index 7ede7daaf..68294d48d 100644 --- a/VueApp/src/styles/base.css +++ b/VueApp/src/styles/base.css @@ -484,6 +484,17 @@ header .q-avatar { font-size: 1.4rem; } +/* Headings inside sanitized CMS content (v-html). Shared by the live content + block (CMS/components/ContentBlock.vue) and the history diff view + (CMS/components/ContentDiffDialog.vue) so both render block headings the same. + Declared globally because scoped styles don't reach v-html children. */ +.content-block h1, +.content-block h2, +.content-block h3 { + font-size: 2rem; + line-height: 2rem; +} + /* Screen reader only — visually hidden but accessible to assistive technology */ .sr-only { position: absolute; From 326a36c38cfcebf3f9940c728e985797f7ef420a Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Wed, 1 Jul 2026 20:12:37 -0700 Subject: [PATCH 05/24] VPR-59 feat(cms): inline block file uploads and trash auto-purge - Add an inline uploader to the content-block editor: files stage client-side and only upload on Save, using the block's folder and permissions, with per-file conflict resolution (rename/overwrite/reuse) and rollback of newly created files if the save then fails - Make the block's VIPER section path required on create and read-only after, sourced from a new content-scoped /content/folders endpoint so create-only users can pick a folder without AllFiles - Restore legacy 30-day trash retention: a daily CmsTrashPurgeScheduledJob permanently deletes files (record + disk) past the cutoff, detaching content-block links first and tolerating per-file disk failures. Gated by a Cms:TrashPurgeEnabled flag (default off) so it ships disabled until the legacy VIPER 1 purge is retired and survives app restarts - Scope the trash to its owner for non-admins (restore and list match on ModifiedBy, failing closed) and open a Trash entry on the hub; surface the purge date on deleted files - Drop the System and Order columns from the content-blocks list; polish responsive list headers, stacked permission chips, and link a11y --- .../src/CMS/__tests__/content-blocks.test.ts | 3 +- .../CMS/__tests__/file-form-dialog.test.ts | 1 + VueApp/src/CMS/__tests__/files.test.ts | 1 + VueApp/src/CMS/components/FileFormDialog.vue | 4 +- .../src/CMS/components/InlineFileUpload.vue | 329 ++++++++++++++++++ VueApp/src/CMS/components/PermissionChips.vue | 85 +++-- VueApp/src/CMS/file-types.ts | 5 + VueApp/src/CMS/pages/CmsHome.vue | 16 +- VueApp/src/CMS/pages/ContentBlockEdit.vue | 149 ++++++-- VueApp/src/CMS/pages/ContentBlocks.vue | 42 +-- VueApp/src/CMS/pages/Files.vue | 15 +- VueApp/src/CMS/pages/LeftNavMenus.vue | 2 +- VueApp/src/CMS/types/index.ts | 1 + VueApp/src/styles/base.css | 24 +- test/CMS/CMSContentControllerTests.cs | 16 +- test/CMS/CMSFilesControllerTests.cs | 82 ++++- test/CMS/CmsFileServiceTests.cs | 92 ++++- test/CMS/CmsTrashPurgeScheduledJobTests.cs | 64 ++++ web/Areas/CMS/Constants/CmsTrash.cs | 20 ++ .../CMS/Controllers/CMSContentController.cs | 16 +- .../CMS/Controllers/CMSFilesController.cs | 34 +- .../CMS/Jobs/CmsTrashPurgeScheduledJob.cs | 51 +++ web/Areas/CMS/Models/CmsFileMapper.cs | 4 + web/Areas/CMS/Models/DTOs/CmsFileDto.cs | 4 + web/Areas/CMS/Services/CmsFileService.cs | 82 ++++- web/appsettings.json | 6 + 26 files changed, 1009 insertions(+), 139 deletions(-) create mode 100644 VueApp/src/CMS/components/InlineFileUpload.vue create mode 100644 VueApp/src/CMS/file-types.ts create mode 100644 test/CMS/CmsTrashPurgeScheduledJobTests.cs create mode 100644 web/Areas/CMS/Constants/CmsTrash.cs create mode 100644 web/Areas/CMS/Jobs/CmsTrashPurgeScheduledJob.cs diff --git a/VueApp/src/CMS/__tests__/content-blocks.test.ts b/VueApp/src/CMS/__tests__/content-blocks.test.ts index 65ef04ca1..129988a7c 100644 --- a/VueApp/src/CMS/__tests__/content-blocks.test.ts +++ b/VueApp/src/CMS/__tests__/content-blocks.test.ts @@ -121,10 +121,9 @@ describe("ContentBlocks.vue - URL filter sync", () => { beforeEach(() => routeGet()) it("initializes filters from the URL query (deep-link) and reflects them in the request", async () => { - await mountPage({ status: "deleted", system: "CTS", section: "courses", search: "welcome", public: "1" }) + await mountPage({ status: "deleted", section: "courses", search: "welcome", public: "1" }) const url = lastListUrl() expect(url).toContain("status=deleted") - expect(url).toContain("system=CTS") expect(url).toContain("viperSectionPath=courses") expect(url).toContain("search=welcome") expect(url).toContain("isPublic=true") diff --git a/VueApp/src/CMS/__tests__/file-form-dialog.test.ts b/VueApp/src/CMS/__tests__/file-form-dialog.test.ts index 0f197d0a4..04f3c98f0 100644 --- a/VueApp/src/CMS/__tests__/file-form-dialog.test.ts +++ b/VueApp/src/CMS/__tests__/file-form-dialog.test.ts @@ -49,6 +49,7 @@ function existingFile(overrides: Partial = {}): CmsFile { modifiedOn: "2024-01-01T00:00:00", modifiedBy: "u", deletedOn: null, + purgeOn: null, permissions: ["SVMSecure.CMS"], people: [{ iamId: "iam1", name: "Person One" }], url: "/files/guid-123", diff --git a/VueApp/src/CMS/__tests__/files.test.ts b/VueApp/src/CMS/__tests__/files.test.ts index 0f3818013..78bc3358e 100644 --- a/VueApp/src/CMS/__tests__/files.test.ts +++ b/VueApp/src/CMS/__tests__/files.test.ts @@ -37,6 +37,7 @@ const FILE_ROW = { modifiedOn: "2024-01-01T00:00:00", modifiedBy: "u", deletedOn: null, + purgeOn: null, permissions: [], people: [], url: "/files/g1", diff --git a/VueApp/src/CMS/components/FileFormDialog.vue b/VueApp/src/CMS/components/FileFormDialog.vue index 53db92575..f6c5799b1 100644 --- a/VueApp/src/CMS/components/FileFormDialog.vue +++ b/VueApp/src/CMS/components/FileFormDialog.vue @@ -236,6 +236,7 @@ import { useUnsavedChanges } from "@/composables/use-unsaved-changes" import PermissionSelector from "@/CMS/components/PermissionSelector.vue" import PersonSelector from "@/CMS/components/PersonSelector.vue" import StatusBanner from "@/components/StatusBanner.vue" +import { CMS_ACCEPTED_EXTENSIONS } from "@/CMS/file-types" import type { CmsFile, CmsFilePerson } from "@/CMS/types/" const props = defineProps<{ @@ -253,8 +254,7 @@ const apiURL = inject("apiURL") + "cms/files/" const $q = useQuasar() const { get, postForm, putForm, createUrlSearchParams } = useFetch() -const acceptedExtensions = - ".pdf,.docx,.doc,.xls,.xlsx,.csv,.ppt,.pptx,.pptm,.txt,.html,.gif,.png,.jpg,.jpeg,.tiff,.mp3,.wav,.mp4,.webm,.oft,.eps,.zip,.7z,.dmg,.exe" +const acceptedExtensions = CMS_ACCEPTED_EXTENSIONS const isEdit = computed(() => props.file !== null) const formRef = ref() diff --git a/VueApp/src/CMS/components/InlineFileUpload.vue b/VueApp/src/CMS/components/InlineFileUpload.vue new file mode 100644 index 000000000..03529bd5c --- /dev/null +++ b/VueApp/src/CMS/components/InlineFileUpload.vue @@ -0,0 +1,329 @@ + + + + + diff --git a/VueApp/src/CMS/components/PermissionChips.vue b/VueApp/src/CMS/components/PermissionChips.vue index 37f64921b..cc5ed87fb 100644 --- a/VueApp/src/CMS/components/PermissionChips.vue +++ b/VueApp/src/CMS/components/PermissionChips.vue @@ -1,40 +1,42 @@ + + diff --git a/VueApp/src/CMS/file-types.ts b/VueApp/src/CMS/file-types.ts new file mode 100644 index 000000000..bf57998af --- /dev/null +++ b/VueApp/src/CMS/file-types.ts @@ -0,0 +1,5 @@ +// File extensions the CMS accepts for upload. Shared by the file manager's upload dialog and the +// content block's inline uploader so the allow-list stays in one place. Mirrors the backend's +// CmsFileTypes allow-list; keep the two in sync. +export const CMS_ACCEPTED_EXTENSIONS = + ".pdf,.docx,.doc,.xls,.xlsx,.csv,.ppt,.pptx,.pptm,.txt,.html,.gif,.png,.jpg,.jpeg,.tiff,.mp3,.wav,.mp4,.webm,.oft,.eps,.zip,.7z,.dmg,.exe" diff --git a/VueApp/src/CMS/pages/CmsHome.vue b/VueApp/src/CMS/pages/CmsHome.vue index 30be70ace..0a2adcf3e 100644 --- a/VueApp/src/CMS/pages/CmsHome.vue +++ b/VueApp/src/CMS/pages/CmsHome.vue @@ -91,6 +91,9 @@ const sections: Section[] = [ permissions: ["SVMSecure.CMS.AllFiles"], actions: [ { label: "Manage Files", to: { name: "CmsFiles" } }, + // File managers see their own deleted files here; admins see the whole trash (scoped by + // the API). Inherits the section's AllFiles gate. + { label: "Trash", to: { name: "CmsFiles", query: { status: "deleted" } } }, { label: "Audit Trail", to: { name: "CmsFileAudit" } }, ], }, @@ -149,15 +152,22 @@ const showRecentActivity = computed(() => canManageBlocks.value || canManageFile max-width: 80rem; } -/* CSS grid with equal rows so all four cards render as same-size tiles - regardless of description length; collapses to one column when narrow. */ +/* Fixed two-column grid so cards always tile evenly (4 -> 2x2) instead of + reflowing to 3x1 at wider widths; equal rows keep tiles the same size + regardless of description length. Collapses to one column when narrow. */ .cms-home-cards { display: grid; - grid-template-columns: repeat(auto-fit, minmax(17rem, 1fr)); + grid-template-columns: repeat(2, minmax(0, 1fr)); grid-auto-rows: 1fr; gap: 1rem; } +@media (max-width: 599.98px) { + .cms-home-cards { + grid-template-columns: 1fr; + } +} + .cms-home-cards .q-card { display: flex; flex-direction: column; diff --git a/VueApp/src/CMS/pages/ContentBlockEdit.vue b/VueApp/src/CMS/pages/ContentBlockEdit.vue index e9f60b504..511336430 100644 --- a/VueApp/src/CMS/pages/ContentBlockEdit.vue +++ b/VueApp/src/CMS/pages/ContentBlockEdit.vue @@ -31,7 +31,7 @@ @validation-error="onValidationError" >
-
+
-
+
+ + + (opens in new window) +
@@ -313,6 +347,7 @@ import { useUnsavedChanges } from "@/composables/use-unsaved-changes" import { checkHasOnePermission } from "@/composables/CheckPagePermission" import BreadcrumbHeading from "@/components/BreadcrumbHeading.vue" import PermissionSelector from "@/CMS/components/PermissionSelector.vue" +import InlineFileUpload from "@/CMS/components/InlineFileUpload.vue" import StatusBanner from "@/components/StatusBanner.vue" import ContentDiffDialog from "@/CMS/components/ContentDiffDialog.vue" import type { @@ -328,12 +363,11 @@ const filesApiURL = inject("apiURL") + "cms/files/" const route = useRoute() const router = useRouter() const $q = useQuasar() -const { get, post, put, createUrlSearchParams } = useFetch() +const { get, post, put, del, createUrlSearchParams } = useFetch() -// This route also admits CreateContentBlock-only users, but the section-path list and the file -// catalog APIs require ManageContentBlocks / AllFiles. Gate those calls so a create-only user -// doesn't fire requests that can only 403 (the section field still accepts a typed-in path). -const canManageContent = checkHasOnePermission(["SVMSecure.CMS.ManageContentBlocks"]) +// This route also admits CreateContentBlock-only users. The folder list (for the section path) +// is served by a content-scoped endpoint they can reach; the file catalog (attach/upload) still +// requires AllFiles, so gate only those. const canAccessFiles = checkHasOnePermission(["SVMSecure.CMS.AllFiles"]) const blockId = computed(() => (route.params.id ? Number(route.params.id) : null)) @@ -352,7 +386,7 @@ const emptyBlock = (): CmsContentBlock => ({ application: null, page: null, viperSectionPath: null, - blockOrder: 1, + blockOrder: 0, friendlyName: null, allowPublicAccess: false, modifiedOn: "", @@ -364,12 +398,20 @@ const emptyBlock = (): CmsContentBlock => ({ const block = ref(emptyBlock()) -const { setInitialState, resetDirtyState, confirmClose } = useUnsavedChanges(block) +// Files chosen in the inline uploader are staged client-side and only uploaded on Save. Fold their +// count into the dirty state so staging alone trips the unsaved-changes guard. +const inlineUploadRef = ref<{ + commit: () => Promise<{ attached: CmsFile[]; createdGuids: string[] }> +} | null>(null) +const stagedCount = ref(0) +const dirtyState = computed(() => ({ block: block.value, staged: stagedCount.value })) + +const { setInitialState, resetDirtyState, confirmClose } = useUnsavedChanges(dirtyState) // Prompt before leaving with unsaved edits, matching the Effort forms' guard. onBeforeRouteLeave(async () => await confirmClose()) -const sectionPaths = ref([]) +const folders = ref([]) const history = ref([]) const selectedHistory = ref(null) const viewingVersion = ref(false) @@ -408,9 +450,12 @@ async function loadHistory() { history.value = res.success ? res.result : [] } -async function loadSectionPaths() { - const res = await get(apiURL + "/section-paths") - sectionPaths.value = res.success ? res.result : [] +// The section path IS a file folder (legacy parity): a block can only point at a real upload +// folder, so its files always land somewhere valid. Sourced from a content-scoped endpoint so +// create-only users (who lack AllFiles) can still populate it. +async function loadFolders() { + const res = await get(apiURL + "/folders") + folders.value = res.success ? res.result : [] } function historyLabel(h: CmsContentHistoryItem): string { @@ -498,6 +543,17 @@ function attachFile() { fileToAttach.value = null } +// Add a file that was just committed (uploaded/overwritten/reused) by the inline uploader to the +// block's attachment list, de-duping by GUID. +function attachUploadedFile(file: CmsFile) { + if (block.value.files.some((f) => f.fileGuid === file.fileGuid)) return + block.value.files.push({ + fileGuid: file.fileGuid, + friendlyName: file.friendlyName, + url: file.friendlyUrl, + }) +} + function detachFile(file: CmsContentBlockFile) { block.value.files = block.value.files.filter((f) => f.fileGuid !== file.fileGuid) } @@ -523,10 +579,40 @@ function onValidationError() { : "Please complete the required fields before saving your changes." } +// Roll back files freshly created during a Save that then failed, so a failed save leaves nothing +// attached. The files go to the trash (soft-delete); the trash-purge job clears them later, and +// they're recoverable meanwhile. Overwritten or reused existing files are never in this list - see +// InlineFileUpload.commit. +async function rollbackFiles(createdGuids: string[]) { + if (createdGuids.length === 0) return + for (const guid of createdGuids) { + await del(filesApiURL + guid) + } + const removed = new Set(createdGuids) + block.value.files = block.value.files.filter((f) => !removed.has(f.fileGuid)) +} + async function saveBlock() { formError.value = "" saving.value = true + + // Commit any staged inline uploads first (this is when they're actually created on the server), + // then attach them so they're included in fileGuids below. Abort the save if an upload fails. + // Track freshly-created files so we can roll them back if the block save itself then fails. + let rollbackGuids: string[] = [] + if (inlineUploadRef.value) { + try { + const { attached, createdGuids } = await inlineUploadRef.value.commit() + attached.forEach(attachUploadedFile) + rollbackGuids = createdGuids + } catch (e) { + saving.value = false + formError.value = e instanceof Error ? e.message : "Failed to upload one or more files." + return + } + } + const payload = { contentBlockId: block.value.contentBlockId, content: block.value.content, @@ -549,6 +635,7 @@ async function saveBlock() { saving.value = false if (res.status === 409) { + await rollbackFiles(rollbackGuids) $q.dialog({ title: "Edit Conflict", message: @@ -566,6 +653,7 @@ async function saveBlock() { } if (!res.success) { + await rollbackFiles(rollbackGuids) formError.value = res.errors?.[0] ?? "Failed to save content block" return } @@ -599,7 +687,8 @@ onMounted(() => { // QEditor renders the focusable contenteditable as an inner element, so its accessible // name has to be set there rather than on the wrapper the "Content" label sits beside. contentEditorRef.value?.getContentEl()?.setAttribute("aria-labelledby", "content-editor-label") - if (canManageContent) loadSectionPaths() + // Only the create form offers the (editable) section-path select; on edit it's read-only. + if (isNew.value) loadFolders() loadBlock() // loadBlock sets the baseline for an existing block after it loads; a brand-new form's // baseline is just the empty block, so capture it synchronously here. @@ -614,4 +703,24 @@ onMounted(() => { content: " *"; color: var(--q-negative); } + +/* Let the editor toolbar wrap onto multiple rows on narrow screens instead of + scrolling horizontally; `dense` keeps each button group intact so groups wrap + as whole units rather than splitting mid-group. */ +.content-editor :deep(.q-editor__toolbar) { + flex-wrap: wrap; +} + +/* On phones, trim the inter-button and inter-group gaps so the toolbar packs + into two rows instead of three. Only the gaps shrink - the buttons keep their + size, so touch targets are unchanged. */ +@media (width <= 599.98px) { + .content-editor :deep(.q-editor__toolbar-group) { + margin: 0 2px; + } + + .content-editor :deep(.q-editor__toolbar .q-btn) { + margin: 2px; + } +} diff --git a/VueApp/src/CMS/pages/ContentBlocks.vue b/VueApp/src/CMS/pages/ContentBlocks.vue index e9bce085e..c7a1b90ad 100644 --- a/VueApp/src/CMS/pages/ContentBlocks.vue +++ b/VueApp/src/CMS/pages/ContentBlocks.vue @@ -1,6 +1,6 @@ @@ -166,10 +157,6 @@
- - @@ -237,7 +220,6 @@ const loading = ref(false) // the Files list and audit trail. const filters = ref({ status: typeof route.query.status === "string" ? route.query.status : "active", - system: typeof route.query.system === "string" ? route.query.system : null, viperSectionPath: typeof route.query.section === "string" ? route.query.section : null, search: typeof route.query.search === "string" ? route.query.search : "", publicOnly: route.query.public === "1", @@ -249,7 +231,6 @@ function syncFiltersToUrl() { query: { ...route.query, status: filters.value.status !== "active" ? filters.value.status : undefined, - system: filters.value.system || undefined, section: filters.value.viperSectionPath || undefined, search: filters.value.search || undefined, public: filters.value.publicOnly ? "1" : undefined, @@ -263,18 +244,10 @@ const statusOptions = [ { label: "All", value: "all" }, ] -const systemOptions = [ - { label: "All", value: null }, - { label: "Viper", value: "Viper" }, - { label: "Public", value: "Public" }, -] - const columns: QTableProps["columns"] = [ { name: "title", label: "Title", field: "title", align: "left", sortable: true }, - { name: "system", label: "System", field: "system", align: "left", sortable: true }, { name: "viperSectionPath", label: "VIPER section", field: "viperSectionPath", align: "left", sortable: true }, { name: "page", label: "Page", field: "page", align: "left", sortable: true }, - { name: "blockOrder", label: "Order", field: "blockOrder", align: "center", sortable: true }, { name: "permissions", label: "Access", field: "permissions", align: "left" }, { name: "modifiedOn", label: "Modified", field: "modifiedOn", align: "left", sortable: true }, { name: "actions", label: "Actions", field: "contentBlockId", align: "center" }, @@ -284,7 +257,6 @@ async function loadBlocks() { loading.value = true const params = createUrlSearchParams({ status: filters.value.status, - system: filters.value.system, viperSectionPath: filters.value.viperSectionPath, search: filters.value.search || null, isPublic: filters.value.publicOnly ? "true" : null, @@ -347,7 +319,6 @@ watch( (query) => { const next = { status: typeof query.status === "string" ? query.status : "active", - system: typeof query.system === "string" ? query.system : null, viperSectionPath: typeof query.section === "string" ? query.section : null, search: typeof query.search === "string" ? query.search : "", publicOnly: query.public === "1", @@ -355,7 +326,6 @@ watch( const f = filters.value if ( next.status === f.status && - next.system === f.system && next.viperSectionPath === f.viperSectionPath && next.search === f.search && next.publicOnly === f.publicOnly diff --git a/VueApp/src/CMS/pages/Files.vue b/VueApp/src/CMS/pages/Files.vue index 576186e35..100c63e83 100644 --- a/VueApp/src/CMS/pages/Files.vue +++ b/VueApp/src/CMS/pages/Files.vue @@ -1,6 +1,6 @@ @@ -303,7 +303,8 @@ const showDialog = ref(false) const editingFile = ref(null) // Filters initialize from the URL so views can be shared/deep-linked -// (the hub's recent-activity rail uses ?search=). +// (the hub's recent-activity rail uses ?search=). The deleted/all views are open to all +// file managers; the API scopes them to files the user deleted (admins see the whole trash). const filters = ref({ folder: typeof route.query.folder === "string" ? route.query.folder : ALL_FOLDERS, status: typeof route.query.status === "string" ? route.query.status : "active", @@ -491,6 +492,12 @@ function formatDate(value: string | null): string { return new Date(value).toLocaleDateString("en-US", { year: "2-digit", month: "2-digit", day: "2-digit" }) } +// In the trash, show when the file will be auto-purged so it's clear it isn't kept forever. +function deletedLabel(row: CmsFile): string { + const base = `Deleted ${formatDate(row.deletedOn)}` + return row.purgeOn ? `${base} · purges ${formatDate(row.purgeOn)}` : base +} + // When in-app navigation reuses this view with a different query (e.g. re-clicking the // left-nav "Manage Files" link while a filter is active, or a hub deep-link), the component // instance is kept, so re-sync the filters from the URL. The equality guard skips our own diff --git a/VueApp/src/CMS/pages/LeftNavMenus.vue b/VueApp/src/CMS/pages/LeftNavMenus.vue index 1ce27936c..9c9d9e38b 100644 --- a/VueApp/src/CMS/pages/LeftNavMenus.vue +++ b/VueApp/src/CMS/pages/LeftNavMenus.vue @@ -1,6 +1,6 @@ diff --git a/VueApp/src/CMS/composables/use-server-table.ts b/VueApp/src/CMS/composables/use-server-table.ts new file mode 100644 index 000000000..7d73d580c --- /dev/null +++ b/VueApp/src/CMS/composables/use-server-table.ts @@ -0,0 +1,78 @@ +import type { Ref } from "vue" +import { ref } from "vue" +import { useQuasar } from "quasar" +import { useFetch } from "@/composables/ViperFetch" + +type ServerTablePagination = { + sortBy: string + descending: boolean + page: number + rowsPerPage: number + rowsNumber: number +} + +// Quasar's QTable @request emits pagination without a guaranteed rowsNumber. +type TableRequestPagination = { + sortBy: string + descending: boolean + page: number + rowsPerPage: number + rowsNumber?: number +} + +type TableRequest = { pagination: TableRequestPagination } + +type TableQueryParams = Record + +type ServerTableOptions = { + url: string + // Build the query params for one page request from the table's current pagination. + buildParams: (pagination: TableRequestPagination) => TableQueryParams + errorMessage?: string + pagination?: Partial +} + +// Server-side paged QTable core shared by the CMS list pages: owns rows/loading/pagination, issues +// the GET on @request, and binds rowsNumber from the response envelope (falling back to the row +// count when the API returns none). Callers supply the URL and a params builder, keeping each +// page's filter mapping local while the fetch/pagination plumbing lives here once. +export function useServerTable(options: ServerTableOptions) { + const $q = useQuasar() + const { get, createUrlSearchParams } = useFetch() + const failureMessage = options.errorMessage ?? "Failed to load" + + const rows = ref([]) as Ref + const loading = ref(false) + const pagination = ref({ + sortBy: "", + descending: false, + page: 1, + rowsPerPage: 50, + rowsNumber: 0, + ...options.pagination, + }) + + async function onRequest(request: TableRequest) { + const { page, rowsPerPage, sortBy, descending } = request.pagination + loading.value = true + const params = createUrlSearchParams(options.buildParams(request.pagination)) + const res = await get(`${options.url}?${params}`) + if (res.success) { + rows.value = res.result + pagination.value.rowsNumber = res.pagination?.totalRecords ?? res.result.length + pagination.value.page = page + pagination.value.rowsPerPage = rowsPerPage + pagination.value.sortBy = sortBy + pagination.value.descending = descending + } else { + $q.notify({ type: "negative", message: res.errors?.[0] ?? failureMessage }) + } + loading.value = false + } + + function reloadFirstPage() { + return onRequest({ pagination: { ...pagination.value, page: 1 } }) + } + + return { rows, loading, pagination, onRequest, reloadFirstPage } +} diff --git a/VueApp/src/CMS/composables/use-url-filtered-table.ts b/VueApp/src/CMS/composables/use-url-filtered-table.ts new file mode 100644 index 000000000..98c976e9a --- /dev/null +++ b/VueApp/src/CMS/composables/use-url-filtered-table.ts @@ -0,0 +1,111 @@ +import type { Ref } from "vue" +import type { LocationQuery, LocationQueryRaw } from "vue-router" +import { ref, watch } from "vue" +import { useRoute, useRouter } from "vue-router" +import { useServerTable } from "./use-server-table" + +type UrlFilters = Record + +type UrlFilteredTableOptions = { + url: string + errorMessage: string + // The single deep-link id filter shown as a removable chip (e.g. fileGuid, contentBlockId). + primaryKey: string + // Default (empty) value for each synced filter; also defines the filter key set and order. + defaultFilters: () => F + pagination: { sortBy: string; descending: boolean } +} + +// "" and null both mean "unset": collapse to null so query params and the URL omit them. +function blankToNull(value: string | null): string | null { + return value || null +} + +function readString(query: LocationQuery, key: string, fallback: string | null): string | null { + return typeof query[key] === "string" ? (query[key] as string) : fallback +} + +// A URL-synced, server-paged filter table shared by the CMS audit/history views. Filters and the +// primary id deep-link from route.query and reflect back on change (empties omitted). When in-app +// navigation reuses the view with a different query, the watcher re-syncs and re-fetches; its +// equality guard skips our own syncFiltersToUrl writes so they don't trigger a redundant fetch. +export function useUrlFilteredTable(options: UrlFilteredTableOptions) { + const route = useRoute() + const router = useRouter() + + function readFilters(query: LocationQuery): F { + const defaults = options.defaultFilters() + const next = { ...defaults } + for (const key of Object.keys(defaults) as (keyof F)[]) { + next[key] = readString(query, key as string, defaults[key]) as F[keyof F] + } + return next + } + + const primary = ref(readString(route.query, options.primaryKey, null)) as Ref + const filters = ref(readFilters(route.query)) as Ref + + const table = useServerTable({ + url: options.url, + errorMessage: options.errorMessage, + pagination: options.pagination, + buildParams: (p) => { + const params: Record = { + [options.primaryKey]: primary.value, + } + for (const [key, value] of Object.entries(filters.value)) { + params[key] = blankToNull(value) + } + params.page = p.page + params.perPage = p.rowsPerPage + return params + }, + }) + + function syncFiltersToUrl() { + const query: LocationQueryRaw = { [options.primaryKey]: primary.value || undefined } + for (const [key, value] of Object.entries(filters.value)) { + query[key] = value || undefined + } + router.replace({ query }) + } + + function reload() { + syncFiltersToUrl() + table.reloadFirstPage() + } + + function clearPrimaryFilter() { + primary.value = null + reload() + } + + function filtersMatch(next: F): boolean { + return Object.keys(next).every((key) => next[key as keyof F] === filters.value[key as keyof F]) + } + + watch( + () => route.query, + (query) => { + const nextPrimary = readString(query, options.primaryKey, null) + const next = readFilters(query) + if (nextPrimary === primary.value && filtersMatch(next)) { + return + } + primary.value = nextPrimary + filters.value = next + table.reloadFirstPage() + }, + ) + + return { + rows: table.rows, + loading: table.loading, + pagination: table.pagination, + onRequest: table.onRequest, + filters, + primary, + reload, + clearPrimaryFilter, + } +} diff --git a/VueApp/src/CMS/pages/BulkEncrypt.vue b/VueApp/src/CMS/pages/BulkEncrypt.vue index 4c04030bf..8d16adc5f 100644 --- a/VueApp/src/CMS/pages/BulkEncrypt.vue +++ b/VueApp/src/CMS/pages/BulkEncrypt.vue @@ -137,10 +137,13 @@ diff --git a/VueApp/src/CMS/pages/ContentBlocks.vue b/VueApp/src/CMS/pages/ContentBlocks.vue index c7a1b90ad..5758af162 100644 --- a/VueApp/src/CMS/pages/ContentBlocks.vue +++ b/VueApp/src/CMS/pages/ContentBlocks.vue @@ -193,6 +193,8 @@ diff --git a/VueApp/src/CMS/pages/Files.vue b/VueApp/src/CMS/pages/Files.vue index 100c63e83..584e4f7a6 100644 --- a/VueApp/src/CMS/pages/Files.vue +++ b/VueApp/src/CMS/pages/Files.vue @@ -271,10 +271,13 @@ diff --git a/test/CMS/CMSContentControllerTests.cs b/test/CMS/CMSContentControllerTests.cs index 9fb69e3ae..f9ce442dc 100644 --- a/test/CMS/CMSContentControllerTests.cs +++ b/test/CMS/CMSContentControllerTests.cs @@ -104,14 +104,15 @@ public override ValidationProblemDetails CreateValidationProblemDetails(HttpCont public async Task GetContentBlocks_PassesFiltersThrough() { var blocks = new List { Block() }; - _blockService.GetContentBlocksAsync("deleted", "Viper", "cats", "search", true, Arg.Any()) - .Returns(blocks); + _blockService.GetContentBlocksAsync("deleted", "Viper", "cats", "search", true, 1, 50, "title", false, + Arg.Any()).Returns((blocks, 1)); - var result = await _controller.GetContentBlocks("deleted", "Viper", "cats", "search", true, TestContext.Current.CancellationToken); + var result = await _controller.GetContentBlocks(null, "deleted", "Viper", "cats", "search", true, + ct: TestContext.Current.CancellationToken); Assert.Same(blocks, result.Value); - await _blockService.Received(1).GetContentBlocksAsync("deleted", "Viper", "cats", "search", true, - Arg.Any()); + await _blockService.Received(1).GetContentBlocksAsync("deleted", "Viper", "cats", "search", true, 1, 50, + "title", false, Arg.Any()); } [Fact] diff --git a/test/CMS/CmsContentBlockServiceTests.cs b/test/CMS/CmsContentBlockServiceTests.cs index 3ab3e07bf..a23b57acd 100644 --- a/test/CMS/CmsContentBlockServiceTests.cs +++ b/test/CMS/CmsContentBlockServiceTests.cs @@ -83,13 +83,37 @@ public async Task GetContentBlocks_FiltersByStatus() await SeedBlockAsync(); await SeedBlockAsync(b => b.DeletedOn = DateTime.Now); - var active = await _service.GetContentBlocksAsync("active", null, null, null, ct: TestContext.Current.CancellationToken); - var deleted = await _service.GetContentBlocksAsync("deleted", null, null, null, ct: TestContext.Current.CancellationToken); - var all = await _service.GetContentBlocksAsync("all", null, null, null, ct: TestContext.Current.CancellationToken); + var active = await _service.GetContentBlocksAsync("active", null, null, null, null, 1, 50, "title", false, TestContext.Current.CancellationToken); + var deleted = await _service.GetContentBlocksAsync("deleted", null, null, null, null, 1, 50, "title", false, TestContext.Current.CancellationToken); + var all = await _service.GetContentBlocksAsync("all", null, null, null, null, 1, 50, "title", false, TestContext.Current.CancellationToken); + + Assert.Single(active.Blocks); + Assert.Single(deleted.Blocks); + Assert.Equal(2, all.Blocks.Count); + Assert.Equal(2, all.Total); + } - Assert.Single(active); - Assert.Single(deleted); - Assert.Equal(2, all.Count); + [Fact] + public async Task GetContentBlocks_PagesSortsAndReturnsTotal() + { + await SeedBlockAsync(b => b.Title = "Charlie"); + await SeedBlockAsync(b => b.Title = "Alpha"); + await SeedBlockAsync(b => b.Title = "Bravo"); + + var page1 = await _service.GetContentBlocksAsync("active", null, null, null, null, 1, 2, "title", false, + TestContext.Current.CancellationToken); + var page2 = await _service.GetContentBlocksAsync("active", null, null, null, null, 2, 2, "title", false, + TestContext.Current.CancellationToken); + + // Total counts all matches; the page returns only its slice, sorted by title across pages. + Assert.Equal(3, page1.Total); + Assert.Equal(new[] { "Alpha", "Bravo" }, page1.Blocks.Select(b => b.Title)); + Assert.Single(page2.Blocks); + Assert.Equal("Charlie", page2.Blocks[0].Title); + + var desc = await _service.GetContentBlocksAsync("active", null, null, null, null, 1, 1, "title", true, + TestContext.Current.CancellationToken); + Assert.Equal("Charlie", desc.Blocks[0].Title); } [Fact] @@ -97,9 +121,9 @@ public async Task GetContentBlocks_ListOmitsContent() { await SeedBlockAsync(); - var list = await _service.GetContentBlocksAsync("active", null, null, null, ct: TestContext.Current.CancellationToken); + var list = await _service.GetContentBlocksAsync("active", null, null, null, null, 1, 50, "title", false, TestContext.Current.CancellationToken); - Assert.Equal(string.Empty, list[0].Content); + Assert.Equal(string.Empty, list.Blocks[0].Content); } [Fact] diff --git a/web/Areas/CMS/Controllers/CMSContentController.cs b/web/Areas/CMS/Controllers/CMSContentController.cs index 5d94f5a1a..76696185c 100644 --- a/web/Areas/CMS/Controllers/CMSContentController.cs +++ b/web/Areas/CMS/Controllers/CMSContentController.cs @@ -37,15 +37,27 @@ public CMSContentController(VIPERContext context, RAPSContext rapsContext, //GET: content [HttpGet] [Permission(Allow = CmsPermissions.ManageContentBlocks)] + [ApiPagination(DefaultPerPage = 50, MaxPerPage = 500)] public async Task>> GetContentBlocks( + ApiPagination? pagination, string status = "active", string? system = null, string? viperSectionPath = null, string? search = null, bool? isPublic = null, + string? sortBy = "title", + bool descending = false, CancellationToken ct = default) { - return await _blockService.GetContentBlocksAsync(status, system, viperSectionPath, search, isPublic, ct); + var page = pagination?.Page ?? 1; + var perPage = pagination?.PerPage ?? 50; + var (blocks, total) = await _blockService.GetContentBlocksAsync(status, system, viperSectionPath, search, + isPublic, page, perPage, sortBy, descending, ct); + if (pagination != null) + { + pagination.TotalRecords = total; + } + return blocks; } //GET: content/section-paths diff --git a/web/Areas/CMS/Services/CmsContentBlockService.cs b/web/Areas/CMS/Services/CmsContentBlockService.cs index f67e42c32..72f7f3ce6 100644 --- a/web/Areas/CMS/Services/CmsContentBlockService.cs +++ b/web/Areas/CMS/Services/CmsContentBlockService.cs @@ -20,8 +20,9 @@ public CmsConcurrencyException(string message) : base(message) { } public interface ICmsContentBlockService { - Task> GetContentBlocksAsync(string status, string? system, string? viperSectionPath, - string? search, bool? isPublic = null, CancellationToken ct = default); + Task<(List Blocks, int Total)> GetContentBlocksAsync(string status, string? system, + string? viperSectionPath, string? search, bool? isPublic, int page, int perPage, string? sortBy, + bool descending, CancellationToken ct = default); Task GetContentBlockAsync(int contentBlockId, CancellationToken ct = default); @@ -84,8 +85,9 @@ public CmsContentBlockService(VIPERContext context, IHtmlSanitizerService saniti _userHelper = userHelper; } - public async Task> GetContentBlocksAsync(string status, string? system, - string? viperSectionPath, string? search, bool? isPublic = null, CancellationToken ct = default) + public async Task<(List Blocks, int Total)> GetContentBlocksAsync(string status, string? system, + string? viperSectionPath, string? search, bool? isPublic, int page, int perPage, string? sortBy, + bool descending, CancellationToken ct = default) { var query = _context.ContentBlocks .AsNoTracking() @@ -122,10 +124,25 @@ public async Task> GetContentBlocksAsync(string status, st || b.Content.Contains(search)); } + int total = await query.CountAsync(ct); + + query = (sortBy?.ToLowerInvariant(), descending) switch + { + ("vipersectionpath", false) => query.OrderBy(b => b.ViperSectionPath).ThenBy(b => b.Title), + ("vipersectionpath", true) => query.OrderByDescending(b => b.ViperSectionPath).ThenBy(b => b.Title), + ("page", false) => query.OrderBy(b => b.Page).ThenBy(b => b.Title), + ("page", true) => query.OrderByDescending(b => b.Page).ThenBy(b => b.Title), + ("modifiedon", false) => query.OrderBy(b => b.ModifiedOn), + ("modifiedon", true) => query.OrderByDescending(b => b.ModifiedOn), + (_, true) => query.OrderByDescending(b => b.Title), + _ => query.OrderBy(b => b.Title) + }; + // List view: project without Content (it can be large) — the editor loads the full // block. Two stages because GetFriendlyURL reads HttpContext and can't translate to SQL. var blocks = await query - .OrderBy(b => b.Title) + .Skip((page - 1) * perPage) + .Take(perPage) .Select(b => new { b.ContentBlockId, @@ -156,7 +173,7 @@ public async Task> GetContentBlocksAsync(string status, st }) .ToListAsync(ct); - return blocks.Select(b => new ContentBlockDto + var dtos = blocks.Select(b => new ContentBlockDto { ContentBlockId = b.ContentBlockId, Title = b.Title, @@ -178,6 +195,8 @@ public async Task> GetContentBlocksAsync(string status, st Url = Data.CMS.GetFriendlyURL(f.FriendlyName, f.AllowPublicAccess) }).ToList() }).ToList(); + + return (dtos, total); } public async Task GetContentBlockAsync(int contentBlockId, CancellationToken ct = default) From df63a61d5a290c6637fa8b516ccfa7be04aa0218 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Thu, 2 Jul 2026 00:37:01 -0700 Subject: [PATCH 17/24] VPR-59 refactor(cms): dialog guard, filter consistency, token cleanup - Link-collection edit dialog now guards unsaved tag edits on close, matching the link dialog and the DESIGN.md close-guard pattern - Filter bars unified to dense outlined across the five list pages - Tag colors moved off semantic status roles to a categorical palette; diff tints and focus ring tokenized; content-block headings get a descending scale; px to rem; deep-watcher and stale ?upload=1 query-sync race fixed --- VueApp/src/CMS/__tests__/files.test.ts | 7 +- .../CMS/__tests__/link-collections.test.ts | 92 +++++++++++++++++++ VueApp/src/CMS/__tests__/link.test.ts | 27 +++--- .../src/CMS/components/ContentDiffDialog.vue | 4 +- .../src/CMS/components/InlineFileUpload.vue | 8 +- VueApp/src/CMS/components/Link.vue | 11 ++- VueApp/src/CMS/components/LinkCollections.vue | 14 +-- VueApp/src/CMS/pages/BulkEncrypt.vue | 2 + VueApp/src/CMS/pages/Files.vue | 7 ++ VueApp/src/CMS/pages/LeftNavMenus.vue | 7 +- .../src/CMS/pages/ManageLinkCollections.vue | 28 +++++- VueApp/src/components/SortableList.vue | 8 +- VueApp/src/config/colors.ts | 3 +- VueApp/src/styles/base.css | 16 +++- VueApp/src/styles/colors.css | 20 ++++ 15 files changed, 209 insertions(+), 45 deletions(-) diff --git a/VueApp/src/CMS/__tests__/files.test.ts b/VueApp/src/CMS/__tests__/files.test.ts index 6bf75848b..c6530de7e 100644 --- a/VueApp/src/CMS/__tests__/files.test.ts +++ b/VueApp/src/CMS/__tests__/files.test.ts @@ -268,11 +268,14 @@ describe("Files.vue - row presentation", () => { describe("Files.vue - URL and query watcher", () => { beforeEach(() => routeGet()) - it("opens the upload dialog when mounted with the ?upload=1 deep-link", async () => { - const { wrapper } = await mountPageWithRouter({ upload: "1" }) + it("opens the upload dialog when mounted with the ?upload=1 deep-link and strips the flag", async () => { + const { wrapper, router } = await mountPageWithRouter({ upload: "1" }) await flushRouter() expect(wrapper.findComponent({ name: "FileFormDialog" }).props("modelValue")).toBeTruthy() + // The on-mount filter sync races the upload-watcher's strip; the flag must not + // survive either write, so re-clicking the nav link can re-open the dialog later. + expect(router.currentRoute.value.query.upload).toBeUndefined() }) it("re-opens the upload dialog when navigation adds ?upload=1, consuming the flag", async () => { diff --git a/VueApp/src/CMS/__tests__/link-collections.test.ts b/VueApp/src/CMS/__tests__/link-collections.test.ts index c622807ec..1c6660619 100644 --- a/VueApp/src/CMS/__tests__/link-collections.test.ts +++ b/VueApp/src/CMS/__tests__/link-collections.test.ts @@ -1,4 +1,5 @@ import LinkCollections from "@/CMS/components/LinkCollections.vue" +import ManageLinkCollections from "@/CMS/pages/ManageLinkCollections.vue" import type { Link, LinkCollection } from "@/CMS/types" import { mountCms, flushPromises } from "./test-utils" @@ -7,12 +8,20 @@ import { mountCms, flushPromises } from "./test-utils" * its links, derives tag-category filters, then filters (search across title/description/tags, * case-insensitive; plus per-category tag select) and optionally groups links by a category. * These tests mock ViperFetch to seed deterministic data and assert the filter/group output. + * The ManageLinkCollections admin page (same domain) is also exercised here for its + * unsaved-changes guard on the Edit Collection dialog. */ const mockGet = vi.fn<(...args: unknown[]) => unknown>() +const mockPut = vi.fn<(...args: unknown[]) => unknown>() +const mockPost = vi.fn<(...args: unknown[]) => unknown>() +const mockDel = vi.fn<(...args: unknown[]) => unknown>() vi.mock("@/composables/ViperFetch", () => ({ useFetch: () => ({ get: (...args: unknown[]) => mockGet(...args), + put: (...args: unknown[]) => mockPut(...args), + post: (...args: unknown[]) => mockPost(...args), + del: (...args: unknown[]) => mockDel(...args), createUrlSearchParams: (obj: Record) => { const params = new URLSearchParams() for (const [k, v] of Object.entries(obj)) { @@ -199,3 +208,86 @@ describe("LinkCollections.vue - group by tag category", () => { expect(vm.groupByValues).toEqual(["Anatomy", "Pharmacology"]) }) }) + +// Quasar plugin dialogs teleport to document.body; click the LAST matching button since a +// dismissed dialog's portal can briefly linger mid-transition. +function clickBodyButton(label: string) { + const btn = [...document.body.querySelectorAll("button")].filter((b) => b.textContent?.includes(label)).at(-1) + expect(btn, `expected a "${label}" button`).toBeTruthy() + btn!.click() +} + +const MANAGE_COLLECTION = { linkCollectionId: 7, linkCollection: "Resources" } +const MANAGE_TAGS = [{ linkCollectionTagCategoryId: 1, linkCollectionTagCategory: "Type", sortOrder: 1 }] + +// The manage page fans out to the collection list, then the selected collection's links + tags. +function primeManageFetch() { + mockGet.mockReset() + mockPut.mockReset() + mockPost.mockReset() + mockDel.mockReset() + mockGet.mockImplementation((...args: unknown[]) => { + const url = args[0] as string + if (url.includes("/links")) return Promise.resolve({ success: true, result: [] }) + if (url.includes("/tags")) return Promise.resolve({ success: true, result: MANAGE_TAGS }) + return Promise.resolve({ success: true, result: [MANAGE_COLLECTION] }) + }) +} + +type ManageVm = { + showCollectionDialog: boolean + draftTags: { linkCollectionTagCategoryId: number; linkCollectionTagCategory: string; sortOrder: number }[] +} + +describe("ManageLinkCollections.vue - Edit Collection dialog unsaved-changes guard", () => { + beforeEach(() => primeManageFetch()) + + async function mountManage() { + const wrapper = mountCms(ManageLinkCollections) + await flushPromises() + await flushPromises() + return wrapper + } + + function closeCollectionDialog() { + const closeBtn = document.body.querySelector('button[aria-label="Close dialog"]') + expect(closeBtn, "expected the collection dialog close (X) button").toBeTruthy() + closeBtn!.click() + } + + it("prompts before discarding staged tag edits, then discards on confirm", async () => { + const wrapper = await mountManage() + const vm = wrapper.vm as unknown as ManageVm + + vm.showCollectionDialog = true + await flushPromises() + // Stage a new tag category so the dialog is dirty. + vm.draftTags.push({ linkCollectionTagCategoryId: -1, linkCollectionTagCategory: "New", sortOrder: 2 }) + await flushPromises() + + closeCollectionDialog() + await flushPromises() + // The guard intercepts the close instead of silently discarding the staged edits. + expect(document.body.textContent).toContain("Unsaved Changes") + expect(vm.showCollectionDialog).toBe(true) + + clickBodyButton("Discard Changes") + await flushPromises() + await flushPromises() + expect(vm.showCollectionDialog).toBe(false) + }) + + it("closes without prompting when nothing was edited", async () => { + const wrapper = await mountManage() + const vm = wrapper.vm as unknown as ManageVm + + vm.showCollectionDialog = true + await flushPromises() + + closeCollectionDialog() + await flushPromises() + await flushPromises() + // Clean dialog: confirmClose resolves immediately, so the dialog just closes. + expect(vm.showCollectionDialog).toBe(false) + }) +}) diff --git a/VueApp/src/CMS/__tests__/link.test.ts b/VueApp/src/CMS/__tests__/link.test.ts index cc8fd6c26..aca3d3b47 100644 --- a/VueApp/src/CMS/__tests__/link.test.ts +++ b/VueApp/src/CMS/__tests__/link.test.ts @@ -65,8 +65,9 @@ describe("Link.vue - URL safety", () => { }) describe("Link.vue - tag badge color cycling", () => { - // The palette has 6 entries; the component maps sortOrder n -> index (n-1) % 6. - // sortOrder 1 -> warning/dark, 2 -> secondary/white, 7 -> wraps back to warning/dark. + // The categorical palette has 5 non-semantic entries; the component maps + // sortOrder n -> index (n-1) % 5. sortOrder 1 -> primary/white, + // 2 -> arboretum/dark, 6 -> wraps back to primary/white. function mountWithTag(categorySortOrder: number) { const categoryId = 1 const collection: LinkCollection = { @@ -88,30 +89,30 @@ describe("Link.vue - tag badge color cycling", () => { return mountCms(LinkComponent, { props: { link, linkCollection: collection } }) } - it("uses the first palette entry (warning) for sortOrder 1", () => { + it("uses the first palette entry (primary) for sortOrder 1", () => { const wrapper = mountWithTag(1) const badge = wrapper.findComponent({ name: "QBadge" }) expect(badge.exists()).toBeTruthy() - expect(badge.props("color")).toBe("warning") - expect(badge.props("textColor")).toBe("dark") + expect(badge.props("color")).toBe("primary") + expect(badge.props("textColor")).toBe("white") expect(badge.text()).toContain("TagValue") }) - it("uses the second palette entry (secondary) for sortOrder 2", () => { + it("uses the second palette entry (arboretum) for sortOrder 2, paired with dark text", () => { const badge = mountWithTag(2).findComponent({ name: "QBadge" }) - expect(badge.props("color")).toBe("secondary") - expect(badge.props("textColor")).toBe("white") + expect(badge.props("color")).toBe("arboretum") + expect(badge.props("textColor")).toBe("dark") }) - it("wraps modulo the palette length so sortOrder 7 matches sortOrder 1", () => { - const badge = mountWithTag(7).findComponent({ name: "QBadge" }) - expect(badge.props("color")).toBe("warning") - expect(badge.props("textColor")).toBe("dark") + it("wraps modulo the palette length so sortOrder 6 matches sortOrder 1", () => { + const badge = mountWithTag(6).findComponent({ name: "QBadge" }) + expect(badge.props("color")).toBe("primary") + expect(badge.props("textColor")).toBe("white") }) it("falls back to the first entry for a non-positive sortOrder", () => { const badge = mountWithTag(0).findComponent({ name: "QBadge" }) - expect(badge.props("color")).toBe("warning") + expect(badge.props("color")).toBe("primary") }) it("only renders badges for tags whose category matches the collection category", () => { diff --git a/VueApp/src/CMS/components/ContentDiffDialog.vue b/VueApp/src/CMS/components/ContentDiffDialog.vue index b803d3cb2..5f7cbf0c9 100644 --- a/VueApp/src/CMS/components/ContentDiffDialog.vue +++ b/VueApp/src/CMS/components/ContentDiffDialog.vue @@ -124,7 +124,7 @@ const emit = defineEmits<{ "update:modelValue": [value: boolean] }>() .cms-diff :deep(ins.diffins), .cms-diff :deep(ins.diffmod), .cms-diff-legend ins { - background-color: #d7efdb; + background-color: var(--diff-add); color: var(--q-dark); text-decoration: underline; } @@ -132,7 +132,7 @@ const emit = defineEmits<{ "update:modelValue": [value: boolean] }>() .cms-diff :deep(del.diffdel), .cms-diff :deep(del.diffmod), .cms-diff-legend del { - background-color: #f6d9dc; + background-color: var(--diff-remove); color: var(--q-dark); text-decoration: line-through; } diff --git a/VueApp/src/CMS/components/InlineFileUpload.vue b/VueApp/src/CMS/components/InlineFileUpload.vue index 985c77d56..76ba55b31 100644 --- a/VueApp/src/CMS/components/InlineFileUpload.vue +++ b/VueApp/src/CMS/components/InlineFileUpload.vue @@ -282,11 +282,11 @@ defineExpose({ commit }) flex-direction: column; align-items: center; justify-content: center; - gap: 4px; - padding: 16px; + gap: 0.25rem; + padding: 1rem; text-align: center; border: 2px dashed var(--q-primary); - border-radius: 4px; + border-radius: 0.25rem; background-color: transparent; font: inherit; color: var(--ucdavis-black-60); @@ -308,7 +308,7 @@ defineExpose({ commit }) outline: none; box-shadow: 0 0 0 0.1rem white, - 0 0 0 0.25rem #258cfb; + 0 0 0 0.25rem var(--focus-ring-color); } .inline-upload:disabled { diff --git a/VueApp/src/CMS/components/Link.vue b/VueApp/src/CMS/components/Link.vue index 6cc2e2b42..757958d32 100644 --- a/VueApp/src/CMS/components/Link.vue +++ b/VueApp/src/CMS/components/Link.vue @@ -57,10 +57,13 @@ const props = defineProps<{ const hrefForLink = computed(() => safeHref(props.link.url)) const isSafe = computed(() => hrefForLink.value !== "#") -// Each tag category's sortOrder (1-based) indexes this palette of Quasar brand -// roles. StatusBadge derives the WCAG-AA text color for each (dark on the light -// warning/info tints, white on the rest). -const TAG_COLORS = ["warning", "secondary", "negative", "positive", "info", "primary"] as const +// Each tag category's sortOrder (1-based) indexes this categorical palette. It is +// built from non-semantic brand roles only (Aggie Blue, the secondary-palette +// arboretum/cabernet accents, Blue 70, and Ink) so status colors — positive, +// negative, warning, info — keep their meaning and never read as a tag category. +// StatusBadge derives the WCAG-AA text color for each (dark on arboretum, white +// on the rest). +const TAG_COLORS = ["primary", "arboretum", "cabernet", "secondary", "dark"] as const function getTagColor(order: number) { const idx = order >= 1 ? (order - 1) % TAG_COLORS.length : 0 diff --git a/VueApp/src/CMS/components/LinkCollections.vue b/VueApp/src/CMS/components/LinkCollections.vue index 5dc9a4a99..ea94e6429 100644 --- a/VueApp/src/CMS/components/LinkCollections.vue +++ b/VueApp/src/CMS/components/LinkCollections.vue @@ -250,13 +250,13 @@ function applyFilters() { } } -watch( - tagFilters, - () => { - applyFilters() - }, - { deep: true }, -) +// Re-filter only when a selection or the search term changes. Watching the +// mapped selected values (not the whole tagFilters tree) avoids re-running +// applyFilters for every option pushed onto a filter during loadFilters. +const selectedTagValues = computed(() => tagFilters.value.map((tf) => tf.selected)) +watch(selectedTagValues, () => { + applyFilters() +}) watch(search, () => { applyFilters() }) diff --git a/VueApp/src/CMS/pages/BulkEncrypt.vue b/VueApp/src/CMS/pages/BulkEncrypt.vue index 8d16adc5f..019d6020c 100644 --- a/VueApp/src/CMS/pages/BulkEncrypt.vue +++ b/VueApp/src/CMS/pages/BulkEncrypt.vue @@ -17,6 +17,7 @@ v-model="filters.folder" dense options-dense + outlined emit-value map-options label="VIPER app" @@ -29,6 +30,7 @@ v-model="filters.search" dense clearable + outlined debounce="400" label="Search" @update:model-value="reload" diff --git a/VueApp/src/CMS/pages/Files.vue b/VueApp/src/CMS/pages/Files.vue index 584e4f7a6..9886493c6 100644 --- a/VueApp/src/CMS/pages/Files.vue +++ b/VueApp/src/CMS/pages/Files.vue @@ -45,6 +45,7 @@ v-model="filters.folder" dense options-dense + outlined emit-value map-options label="VIPER app" @@ -57,6 +58,7 @@ v-model="filters.status" dense options-dense + outlined emit-value map-options label="Status" @@ -69,6 +71,7 @@ v-model="filters.search" dense clearable + outlined debounce="400" label="Search name, description, or URL" @update:model-value="reload" @@ -315,6 +318,9 @@ const filters = ref({ }) // Reflect the active filters back into the URL (defaults are omitted). +// `upload` is a one-shot action flag, never a persisted filter: strip it here so a +// filter-sync write that races the upload-watcher's own strip (both fire on mount) +// can't re-add the stale flag via the ...route.query spread. function syncFiltersToUrl() { void router.replace({ query: { @@ -324,6 +330,7 @@ function syncFiltersToUrl() { search: filters.value.search || undefined, encrypted: filters.value.encryptedOnly ? "1" : undefined, public: filters.value.publicOnly ? "1" : undefined, + upload: undefined, }, }) } diff --git a/VueApp/src/CMS/pages/LeftNavMenus.vue b/VueApp/src/CMS/pages/LeftNavMenus.vue index 9c9d9e38b..3d017a6c9 100644 --- a/VueApp/src/CMS/pages/LeftNavMenus.vue +++ b/VueApp/src/CMS/pages/LeftNavMenus.vue @@ -19,6 +19,7 @@ v-model="filters.system" dense options-dense + outlined emit-value map-options label="System" @@ -31,6 +32,7 @@ v-model="filters.search" dense clearable + outlined debounce="400" label="Search header, name, or page" @update:model-value="loadMenus" @@ -42,13 +44,16 @@
+ @@ -68,7 +69,7 @@ round dense aria-label="Close dialog" - v-close-popup + @click="handleCollectionClose" /> @@ -395,7 +396,7 @@ + + diff --git a/VueApp/src/CMS/composables/use-content-diff-viewer.ts b/VueApp/src/CMS/composables/use-content-diff-viewer.ts new file mode 100644 index 000000000..0badafbda --- /dev/null +++ b/VueApp/src/CMS/composables/use-content-diff-viewer.ts @@ -0,0 +1,68 @@ +import { ref } from "vue" +import { useQuasar } from "quasar" +import { useDateFunctions } from "@/composables/DateFunctions" +import type { CmsContentHistoryDiff } from "@/CMS/types/" + +// Shared state and helpers for every ContentDiffDialog consumer (history page, block editor, +// recent-activity rail): one open/reset shape, one way to apply a diff response, one error +// path, and the shared "date by author" stamp wording for subtitles. +export function useContentDiffViewer() { + const $q = useQuasar() + const { formatDateTime } = useDateFunctions() + + const viewer = ref({ + open: false, + loading: false, + title: "", + subtitle: "", + content: "", + hasComparison: true, + hasChanges: true, + }) + + function openViewer(title: string) { + viewer.value = { + open: true, + loading: true, + title, + subtitle: "", + content: "", + hasComparison: true, + hasChanges: true, + } + } + + function applyDiff(diff: CmsContentHistoryDiff, subtitle: string) { + viewer.value.content = diff.content + viewer.value.hasComparison = diff.hasComparison + viewer.value.hasChanges = diff.hasChanges + viewer.value.subtitle = subtitle + viewer.value.loading = false + } + + function closeViewer() { + viewer.value.open = false + viewer.value.loading = false + } + + function failViewer(message: string) { + $q.notify({ type: "negative", message }) + closeViewer() + } + + function diffStamp(on: string | null, by: string | null): string { + return ( + (on ? formatDateTime(on, { dateStyle: "short", timeStyle: "short" }) : "unknown date") + + (by ? ` by ${by}` : "") + ) + } + + // Subtitle for a saved-version diff (GET .../diff): direction reads old version -> new version. + function savedDiffSubtitle(diff: CmsContentHistoryDiff): string { + return diff.hasComparison + ? `Changes from ${diffStamp(diff.oldModifiedOn, diff.oldModifiedBy)} to ${diffStamp(diff.newModifiedOn, diff.newModifiedBy)}` + : `Original version, ${diffStamp(diff.newModifiedOn, diff.newModifiedBy)}` + } + + return { viewer, openViewer, applyDiff, closeViewer, failViewer, diffStamp, savedDiffSubtitle } +} diff --git a/VueApp/src/CMS/pages/CmsHome.vue b/VueApp/src/CMS/pages/CmsHome.vue index 0a2adcf3e..6a3575181 100644 --- a/VueApp/src/CMS/pages/CmsHome.vue +++ b/VueApp/src/CMS/pages/CmsHome.vue @@ -9,6 +9,16 @@ Your account does not have access to any CMS tools. Contact the VIPER team if you need access. + + {{ purgeWarningText }} + Review the Trash + to restore anything still needed. + +
@@ -59,12 +69,17 @@ diff --git a/VueApp/src/CMS/pages/ContentBlockEdit.vue b/VueApp/src/CMS/pages/ContentBlockEdit.vue index 526037c08..51887a04c 100644 --- a/VueApp/src/CMS/pages/ContentBlockEdit.vue +++ b/VueApp/src/CMS/pages/ContentBlockEdit.vue @@ -346,6 +346,7 @@ import { useRoute, useRouter, onBeforeRouteLeave } from "vue-router" import { useQuasar } from "quasar" import { useFetch } from "@/composables/ViperFetch" import { useUnsavedChanges } from "@/composables/use-unsaved-changes" +import { useContentDiffViewer } from "@/CMS/composables/use-content-diff-viewer" import { checkHasOnePermission } from "@/composables/CheckPagePermission" import BreadcrumbHeading from "@/components/BreadcrumbHeading.vue" import PermissionSelector from "@/CMS/components/PermissionSelector.vue" @@ -481,41 +482,25 @@ async function loadHistoryVersion() { } } -const diffViewer = ref({ - open: false, - loading: false, - title: "", - subtitle: "", - content: "", - hasChanges: true, -}) +const { viewer: diffViewer, openViewer: openDiffViewer, applyDiff, failViewer } = useContentDiffViewer() // Compare the editor's current content (the "new" side) against the selected historical version // (the "old" side). The current content is posted because it may include unsaved edits. async function diffAgainstCurrent() { if (!selectedHistory.value) return - diffViewer.value = { - open: true, - loading: true, - title: "Current editor content vs previous version", - subtitle: "", - content: "", - hasChanges: true, - } + openDiffViewer("Current editor content vs previous version") const res = await post( apiURL + "/" + blockId.value + "/history/" + selectedHistory.value.contentHistoryId + "/diff", { content: block.value.content }, ) if (res.success) { - const diff = res.result as CmsContentHistoryDiff - diffViewer.value.content = diff.content - diffViewer.value.hasChanges = diff.hasChanges - diffViewer.value.subtitle = `Changes from ${historyLabel(selectedHistory.value)} to your current editor content` + applyDiff( + res.result as CmsContentHistoryDiff, + `Changes from ${historyLabel(selectedHistory.value)} to your current editor content`, + ) } else { - $q.notify({ type: "negative", message: res.errors?.[0] ?? "Failed to build the diff" }) - diffViewer.value.open = false + failViewer(res.errors?.[0] ?? "Failed to build the diff") } - diffViewer.value.loading = false } async function searchFiles(val: string, update: (fn: () => void) => void) { diff --git a/VueApp/src/CMS/pages/ContentBlockHistory.vue b/VueApp/src/CMS/pages/ContentBlockHistory.vue index 8aff5920b..c08ea754f 100644 --- a/VueApp/src/CMS/pages/ContentBlockHistory.vue +++ b/VueApp/src/CMS/pages/ContentBlockHistory.vue @@ -186,10 +186,11 @@ + + diff --git a/VueApp/src/CMS/components/RecentActivity.vue b/VueApp/src/CMS/components/RecentActivity.vue index 2b9d6571f..49e2e6178 100644 --- a/VueApp/src/CMS/components/RecentActivity.vue +++ b/VueApp/src/CMS/components/RecentActivity.vue @@ -31,56 +31,11 @@ v-else dense > - - - - - - {{ item.label }} - - {{ item.typeLabel }} · {{ item.verb ? item.verb + " " : "" - }}{{ formatTimeAgo(new Date(item.modifiedOn)) }} by - {{ item.modifiedBy }} - - - -
- - {{ action.label }} - -
- - + :item="item" + /> import { inject, onMounted, ref } from "vue" import { useQuasar } from "quasar" -import { formatTimeAgo } from "@vueuse/core" import { useFetch } from "@/composables/ViperFetch" import { useContentDiffViewer } from "@/CMS/composables/use-content-diff-viewer" +import ActivityRow from "@/CMS/components/ActivityRow.vue" import ContentDiffDialog from "@/CMS/components/ContentDiffDialog.vue" import type { + ActivityItem, CmsContentBlock, CmsContentHistoryAudit, CmsContentHistoryDiff, CmsFile, CmsLeftNavMenu, + RailAction, } from "@/CMS/types/" const MAX_ITEMS = 8 @@ -123,26 +80,6 @@ const { showLeftNavs?: boolean }>() -type RailAction = { - icon: string - label: string - to?: { name: string; query?: Record } - run?: () => void -} - -type ActivityItem = { - key: string - icon: string - typeLabel: string - // Rendered before the time-ago stamp (e.g. "deleted 2 days ago"); empty means plain recency. - verb?: string - label: string - to: { name: string; params?: Record; query?: Record } - modifiedOn: string - modifiedBy: string - actions?: RailAction[] -} - const $q = useQuasar() const apiURL = inject("apiURL") const { get, post, createUrlSearchParams } = useFetch() @@ -152,19 +89,6 @@ const loading = ref(true) const failed = ref(false) const items = ref([]) -function formatFullDate(value: string): string { - return new Date(value).toLocaleString() -} - -// Row-level navigation must not fire for action clicks (`.stop` in the template). Actions with -// their own `to` navigate via QBtn; only run-actions preventDefault, both to keep the row's -// anchor inert and because a prevented default makes QBtn cancel its router navigation. -function runAction(action: RailAction, event: Event) { - if (!action.run) return - event.preventDefault() - action.run() -} - // History rows hold only superseded versions, and the GET diff endpoint compares two of them — // so the block's latest change (newest history row -> live content) needs the POST diff with // the current content, exactly like the editor's compare-against-draft flow. @@ -331,38 +255,3 @@ onMounted(() => { void loadActivity() }) - - diff --git a/VueApp/src/CMS/types/index.ts b/VueApp/src/CMS/types/index.ts index ae26f5446..d5c4d1ac9 100644 --- a/VueApp/src/CMS/types/index.ts +++ b/VueApp/src/CMS/types/index.ts @@ -75,6 +75,28 @@ type CmsContentHistoryDiff = { newModifiedBy: string | null } +// One row in the hub's recent-activity rail, normalized across sources (blocks, files, +// trashed files, left-nav menus); actions are the per-row shortcuts (history, audit, diff). +type RailAction = { + icon: string + label: string + to?: { name: string; query?: Record } + run?: () => void +} + +type ActivityItem = { + key: string + icon: string + typeLabel: string + // Rendered before the time-ago stamp (e.g. "deleted 2 days ago"); empty means plain recency. + verb?: string + label: string + to: { name: string; params?: Record; query?: Record } + modifiedOn: string + modifiedBy: string + actions?: RailAction[] +} + type CmsLeftNavMenu = { leftNavMenuId: number menuHeaderText: string | null @@ -177,6 +199,8 @@ export type { CmsFileAudit, CmsContentHistoryAudit, CmsContentHistoryDiff, + RailAction, + ActivityItem, LinkCollection, LinkCollectionTagCategory, Link, From 236a7ed03816ae17365cba1c008f6cfe9fac7efe Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Thu, 2 Jul 2026 15:48:46 -0700 Subject: [PATCH 21/24] VPR-59 fix(cms): apply full-branch review fixes across API and UI - Serve the anonymous content fn endpoint from a minimal public DTO: the full DTO disclosed editor login ids, permission names, and System/section placement to unauthenticated callers - Restore legacy download auditing: anonymous public hits are audited again (null login/iam), ZIP permission denies are audited at all, and internal 192.168.* requests are excluded like the legacy app - Resolve user permissions once per content-block render instead of per block-permission, mirroring CheckFilePermission - Guard the shared server-table against out-of-order responses so a slow earlier filter request cannot clobber a newer one, and stop query-only navigations (URL-synced filters) from yanking focus to the main landmark mid-keystroke - Guard link-collection loads against failed fetches that crashed the render; drop the dead always-empty GetAllFiles; AsNoTracking on the download-path file lookups - Make the activity row a single stretched title link so its action buttons are no longer interactive controls nested inside an anchor --- .../src/CMS/__tests__/recent-activity.test.ts | 4 +- .../CMS/__tests__/use-server-table.test.ts | 27 ++++- VueApp/src/CMS/components/ActivityRow.vue | 57 ++++++--- VueApp/src/CMS/components/LinkCollections.vue | 2 +- .../src/CMS/composables/use-server-table.ts | 10 ++ VueApp/src/CMS/pages/ContentBlockEdit.vue | 2 + .../src/CMS/pages/ManageLinkCollections.vue | 5 +- VueApp/src/composables/use-route-focus.ts | 8 +- .../__tests__/create-spa-router.test.ts | 21 +++- test/CMS/CMSContentControllerTests.cs | 6 + .../CMS/Controllers/CMSContentController.cs | 13 ++- web/Areas/CMS/Data/CMS.cs | 109 ++++++++---------- web/Areas/CMS/Models/CmsContentBlockMapper.cs | 2 + web/Areas/CMS/Models/DTOs/ContentBlockDto.cs | 13 +++ 14 files changed, 188 insertions(+), 91 deletions(-) diff --git a/VueApp/src/CMS/__tests__/recent-activity.test.ts b/VueApp/src/CMS/__tests__/recent-activity.test.ts index fae4bd793..151d05e56 100644 --- a/VueApp/src/CMS/__tests__/recent-activity.test.ts +++ b/VueApp/src/CMS/__tests__/recent-activity.test.ts @@ -191,8 +191,10 @@ describe("recentActivity.vue - deleted files and item actions", () => { expect(wrapper.text()).toContain("Gone file") expect(wrapper.text()).toContain("deleted") + // The row's single anchor is the stretched title link (controls must not nest in an anchor). const item = wrapper.findAllComponents({ name: "QItem" }).find((i) => i.text().includes("Gone file"))! - expect(item.props("to")).toStrictEqual({ + const titleLink = item.findComponent({ name: "RouterLink" }) + expect(titleLink.props("to")).toStrictEqual({ name: "CmsFiles", query: { status: "deleted", search: "Gone file" }, }) diff --git a/VueApp/src/CMS/__tests__/use-server-table.test.ts b/VueApp/src/CMS/__tests__/use-server-table.test.ts index b8c6e03c2..a90804d48 100644 --- a/VueApp/src/CMS/__tests__/use-server-table.test.ts +++ b/VueApp/src/CMS/__tests__/use-server-table.test.ts @@ -3,7 +3,7 @@ import { useServerTable } from "@/CMS/composables/use-server-table" import { mountCms, flushPromises } from "./test-utils" /** - * useServerTable is the server-paged QTable core shared by the CMS list pages: onRequest issues + * UseServerTable is the server-paged QTable core shared by the CMS list pages: onRequest issues * the GET built from buildParams, binds rows and rowsNumber (falling back to the row count when * the API returns no pagination envelope), copies the request pagination into state, and notifies * on failure. Direct spec so regressions surface here rather than in every list page. @@ -68,7 +68,7 @@ describe("useServerTable", () => { await table.onRequest(request(2, { sortBy: "modifiedOn", descending: true })) expect(mockGet.mock.calls[0]![0]).toBe("/api/things?page=2&perPage=25&sortBy=modifiedOn&descending=true") - expect(table.rows.value).toEqual([{ id: 1 }]) + expect(table.rows.value).toStrictEqual([{ id: 1 }]) expect(table.pagination.value).toMatchObject({ rowsNumber: 91, page: 2, @@ -98,7 +98,7 @@ describe("useServerTable", () => { // The notification teleports to document.body. expect(document.body.textContent).toContain("backend exploded") - expect(table.rows.value).toEqual([{ id: 1 }]) + expect(table.rows.value).toStrictEqual([{ id: 1 }]) expect(table.loading.value).toBeFalsy() }) @@ -112,6 +112,27 @@ describe("useServerTable", () => { expect(document.body.textContent).toContain("Failed to load things") }) + it("drops a stale response that resolves after a newer request", async () => { + // First request resolves LAST (slow); second request resolves first. The slow + // first response must not clobber the rows/pagination of the newer request. + let resolveFirst!: (value: unknown) => void + mockGet.mockImplementationOnce(() => new Promise((resolve) => (resolveFirst = resolve))) + mockGet.mockResolvedValueOnce({ success: true, result: [{ id: 2 }], pagination: { totalRecords: 1 } }) + const table = mountTable() + + const first = table.onRequest(request(1)) + const second = table.onRequest(request(2)) + await second + + resolveFirst({ success: true, result: [{ id: 99 }], pagination: { totalRecords: 999 } }) + await first + await flushPromises() + + expect(table.rows.value).toStrictEqual([{ id: 2 }]) + expect(table.pagination.value.rowsNumber).toBe(1) + expect(table.pagination.value.page).toBe(2) + }) + it("reloadFirstPage re-requests page 1 while keeping the current sort and page size", async () => { mockGet.mockResolvedValue({ success: true, result: [], pagination: { totalRecords: 0 } }) const table = mountTable() diff --git a/VueApp/src/CMS/components/ActivityRow.vue b/VueApp/src/CMS/components/ActivityRow.vue index 4b9044a25..b720b0ad7 100644 --- a/VueApp/src/CMS/components/ActivityRow.vue +++ b/VueApp/src/CMS/components/ActivityRow.vue @@ -1,9 +1,5 @@