Comic Rework Bugfixes Round 1 (#2774)

API.Tests/Services/ReadingListServiceTests.cs

@@ -1205,6 +1205,65 @@ public async Task CreateReadingListFromCBL_ShouldUpdateAnExistingList()
         Assert.Equal(2, createdList.Items.First(item => item.Order == 2).ChapterId);
         Assert.Equal(4, createdList.Items.First(item => item.Order == 3).ChapterId);
     }
+
+    /// <summary>
+    /// This test is about ensuring Annuals that are a separate series can be linked up properly (ComicVine)
+    /// </summary>
+    //[Fact]
+    public async Task CreateReadingListFromCBL_ShouldCreateList_WithAnnuals()
+    {
+        // TODO: Implement this correctly
+        await ResetDb();
+        var cblReadingList = LoadCblFromPath("Annual.cbl");
+
+        // Mock up our series
+        var fablesSeries = new SeriesBuilder("Fables")
+            .WithVolume(new VolumeBuilder("2002")
+                .WithMinNumber(1)
+                .WithChapter(new ChapterBuilder("1").Build())
+                .WithChapter(new ChapterBuilder("2").Build())
+                .WithChapter(new ChapterBuilder("3").Build())
+                .Build())
+            .Build();
+
+        var fables2Series = new SeriesBuilder("Fables Annual")
+            .WithVolume(new VolumeBuilder("2003")
+                .WithMinNumber(1)
+                .WithChapter(new ChapterBuilder("1").Build())
+                .Build())
+            .Build();
+
+        _context.AppUser.Add(new AppUser()
+        {
+            UserName = "majora2007",
+            ReadingLists = new List<ReadingList>(),
+            Libraries = new List<Library>()
+            {
+                new LibraryBuilder("Test LIb 2", LibraryType.Book)
+                    .WithSeries(fablesSeries)
+                    .WithSeries(fables2Series)
+                    .Build()
+            },
+        });
+        await _unitOfWork.CommitAsync();
+
+        var importSummary = await _readingListService.CreateReadingListFromCbl(1, cblReadingList);
+
+        Assert.Equal(CblImportResult.Success, importSummary.Success);
+        Assert.NotEmpty(importSummary.Results);
+
+        var createdList = await _unitOfWork.ReadingListRepository.GetReadingListByIdAsync(1);
+
+        Assert.NotNull(createdList);
+        Assert.Equal("Annual", createdList.Title);
+
+        Assert.Equal(4, createdList.Items.Count);
+        Assert.Equal(1, createdList.Items.First(item => item.Order == 0).ChapterId);
+        Assert.Equal(2, createdList.Items.First(item => item.Order == 1).ChapterId);
+        Assert.Equal(4, createdList.Items.First(item => item.Order == 2).ChapterId);
+        Assert.Equal(3, createdList.Items.First(item => item.Order == 3).ChapterId);
+    }
+
     #endregion
 
     #region CreateReadingListsFromSeries

API.Tests/Services/Test Data/ReadingListService/Annual.cbl

@@ -0,0 +1,19 @@
+<?xml version="1.0"?>
+<ReadingList xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+  <Name>Fables</Name>
+  <Books>
+    <Book Series="Fables" Number="1" Volume="2002" Year="2002">
+      <Id>5bd3dd55-2a85-4325-aefa-21e9f19b12c9</Id>
+    </Book>
+    <Book Series="Fables" Number="2" Volume="2002" Year="2002">
+      <Id>3831761c-604a-4420-bed2-9f5ac4e94bd4</Id>
+    </Book>
+    <Book Series="Fables Annual" Number="1" Volume="2003" Year="2003" Format="Annual">
+      <Id>23acefd4-1bc7-4c3c-99df-133045d1f266</Id>
+    </Book>
+    <Book Series="Fables" Number="3" Volume="2002" Year="2002">
+      <Id>27a5d7db-9f7e-4be1-aca6-998a1cc1488f</Id>
+    </Book>
+  </Books>
+  <Matchers />
+</ReadingList>

API/Extensions/SeriesExtensions.cs

@@ -1,6 +1,4 @@
-using System.Collections.Generic;
-using System.Globalization;
-using System.Linq;
+using System.Linq;
 using API.Comparators;
 using API.Entities;
 using API.Services.Tasks.Scanner.Parser;
@@ -25,7 +23,7 @@ public static class SeriesExtensions
         if (firstVolume == null) return null;
 
         var chapters = firstVolume.Chapters
-            .OrderBy(c => c.SortOrder, ChapterSortComparerDefaultLast.Default)
+            .OrderBy(c => c.SortOrder)
             .ToList();
 
         if (chapters.Count > 1 && chapters.Exists(c => c.IsSpecial))
@@ -34,7 +32,7 @@ public static class SeriesExtensions
         }
 
         // just volumes
-        if (volumes.TrueForAll(v => $"{v.MinNumber}" != Parser.LooseLeafVolume))
+        if (volumes.TrueForAll(v => v.MinNumber.IsNot(Parser.LooseLeafVolumeNumber)))
         {
             return firstVolume.CoverImage;
         }
@@ -45,11 +43,13 @@ public static class SeriesExtensions
         {
             var looseLeafChapters = volumes.Where(v => v.MinNumber.Is(Parser.LooseLeafVolumeNumber))
                 .SelectMany(c => c.Chapters.Where(c2 => !c2.IsSpecial))
-                .OrderBy(c => c.MinNumber, ChapterSortComparerDefaultFirst.Default)
+                .OrderBy(c => c.SortOrder)
                 .ToList();
 
             if (looseLeafChapters.Count > 0 && volumes[0].MinNumber > looseLeafChapters[0].MinNumber)
             {
+                var first = looseLeafChapters.Find(c => c.SortOrder.Is(1f));
+                if (first != null) return first.CoverImage;
                 return looseLeafChapters[0].CoverImage;
             }
             return firstVolume.CoverImage;
@@ -58,14 +58,11 @@ public static class SeriesExtensions
         var chpts = volumes
             .First(v => v.MinNumber.Is(Parser.LooseLeafVolumeNumber))
             .Chapters
-            //.Where(v => v.MinNumber.Is(Parser.LooseLeafVolumeNumber))
-            //.SelectMany(v => v.Chapters)
-
             .Where(c => !c.IsSpecial)
             .OrderBy(c => c.MinNumber, ChapterSortComparerDefaultLast.Default)
             .ToList();
 
-        var exactlyChapter1 = chpts.FirstOrDefault(c => c.MinNumber.Is(1f));
+        var exactlyChapter1 = chpts.Find(c => c.MinNumber.Is(1f));
         if (exactlyChapter1 != null)
         {
             return exactlyChapter1.CoverImage;

API/Services/ArchiveService.cs

@@ -218,7 +218,7 @@ public int GetNumberOfPagesFromArchive(string archivePath)
     /// <returns></returns>
     public string GetCoverImage(string archivePath, string fileName, string outputDirectory, EncodeFormat format, CoverImageSize size = CoverImageSize.Default)
     {
-        if (archivePath == null || !IsValidArchive(archivePath)) return string.Empty;
+        if (string.IsNullOrEmpty(archivePath) || !IsValidArchive(archivePath)) return string.Empty;
         try
         {
             var libraryHandler = CanOpen(archivePath);

API/Services/Plus/LicenseService.cs

@@ -181,6 +181,11 @@ public async Task<bool> HasActiveLicense(bool forceCheck = false)
         return false;
     }
 
+    /// <summary>
+    /// Checks if the sub is active and caches the result. This should not be used too much over cache as it will skip backend caching.
+    /// </summary>
+    /// <param name="license"></param>
+    /// <returns></returns>
     public async Task<bool> HasActiveSubscription(string? license)
     {
         if (string.IsNullOrWhiteSpace(license)) return false;

API/Services/ReaderService.cs

@@ -797,6 +797,7 @@ public static string FormatChapterName(LibraryType libraryType, bool includeHash
             case LibraryType.Manga:
                 return "Chapter" + (includeSpace ? " " : string.Empty);
             case LibraryType.Comic:
+            case LibraryType.ComicVine:
                 if (includeHash) {
                     return "Issue #";
                 }

API/Services/ReadingListService.cs

@@ -619,6 +619,26 @@ public async Task<CblImportSummaryDto> CreateReadingListFromCbl(int userId, CblR
         readingList.Items ??= new List<ReadingListItem>();
         foreach (var (book, i) in cblReading.Books.Book.Select((value, i) => ( value, i )))
         {
+
+            // I want to refactor this so that we move the matching logic into a method.
+            // But when I looked, we are returning statuses on different conditions, hard to keep it single responsibility
+            // Either refactor to return an enum for the state, make it return the BookResult, or refactor the reasoning so it's more straightforward
+
+            // var match = FindMatchingCblBookSeries(book);
+            // if (match == null)
+            // {
+            //     importSummary.Results.Add(new CblBookResult(book)
+            //     {
+            //         Reason = CblImportReason.SeriesMissing,
+            //         Order = i
+            //     });
+            //     continue;
+            // }
+
+
+            // TODO: I need a dedicated db query to get Series name's processed if they are ComicVine.
+            // In comicvine, series names are Series(Volume), but the spec just has Series="Series" Volume="Volume"
+            // So we need to combine them for comics that are in ComicVine libraries.
             var normalizedSeries = Parser.Normalize(book.Series);
             if (!allSeries.TryGetValue(normalizedSeries, out var bookSeries) && !allSeriesLocalized.TryGetValue(normalizedSeries, out bookSeries))
             {
@@ -645,7 +665,7 @@ public async Task<CblImportSummaryDto> CreateReadingListFromCbl(int userId, CblR
                 continue;
             }
 
-            // We need to handle chapter 0 or empty string when it's just a volume
+            // We need to handle default chapter or empty string when it's just a volume
             var bookNumber = string.IsNullOrEmpty(book.Number)
                 ? Parser.DefaultChapterNumber
                 : float.Parse(book.Number);

API/Services/Tasks/Scanner/ProcessSeries.cs

@@ -113,13 +113,15 @@ public async Task ProcessSeriesAsync(IList<ParserInfo> parsedInfos, Library libr
         var seriesName = parsedInfos[0].Series;
         await _eventHub.SendMessageAsync(MessageFactory.NotificationProgress,
             MessageFactory.LibraryScanProgressEvent(library.Name, ProgressEventType.Updated, seriesName));
-        _logger.LogInformation("[ScannerService] Beginning series update on {SeriesName}", seriesName);
+        _logger.LogInformation("[ScannerService] Beginning series update on {SeriesName}, Forced: {ForceUpdate}", seriesName, forceUpdate);
 
         // Check if there is a Series
         var firstInfo = parsedInfos[0];
         Series? series;
         try
         {
+            // There is an opportunity to allow duplicate series here. Like if One is in root/marvel/batman and another is root/dc/batman
+            // by changing to a ToList() and if multiple, doing a firstInfo.FirstFolder/RootFolder type check
             series =
                 await _unitOfWork.SeriesRepository.GetFullSeriesByAnyName(firstInfo.Series, firstInfo.LocalizedSeries,
                     library.Id, firstInfo.Format);

API/Services/Tasks/ScannerService.cs

@@ -258,41 +258,41 @@ public async Task ScanSeries(int seriesId, bool bypassFolderOptimizationChecks =
         // Remove any parsedSeries keys that don't belong to our series. This can occur when users store 2 series in the same folder
         RemoveParsedInfosNotForSeries(parsedSeries, series);
 
-         // If nothing was found, first validate any of the files still exist. If they don't then we have a deletion and can skip the rest of the logic flow
-         if (parsedSeries.Count == 0)
+        // If nothing was found, first validate any of the files still exist. If they don't then we have a deletion and can skip the rest of the logic flow
+        if (parsedSeries.Count == 0)
+        {
+         var seriesFiles = (await _unitOfWork.SeriesRepository.GetFilesForSeries(series.Id));
+         if (!string.IsNullOrEmpty(series.FolderPath) && !seriesFiles.Where(f => f.FilePath.Contains(series.FolderPath)).Any(m => File.Exists(m.FilePath)))
          {
-             var seriesFiles = (await _unitOfWork.SeriesRepository.GetFilesForSeries(series.Id));
-             if (!string.IsNullOrEmpty(series.FolderPath) && !seriesFiles.Where(f => f.FilePath.Contains(series.FolderPath)).Any(m => File.Exists(m.FilePath)))
+             try
              {
-                 try
-                 {
-                     _unitOfWork.SeriesRepository.Remove(series);
-                     await CommitAndSend(1, sw, scanElapsedTime, series);
-                     await _eventHub.SendMessageAsync(MessageFactory.SeriesRemoved,
-                         MessageFactory.SeriesRemovedEvent(seriesId, string.Empty, series.LibraryId), false);
-                 }
-                 catch (Exception ex)
-                 {
-                     _logger.LogCritical(ex, "There was an error during ScanSeries to delete the series as no files could be found. Aborting scan");
-                     await _unitOfWork.RollbackAsync();
-                     return;
-                 }
+                 _unitOfWork.SeriesRepository.Remove(series);
+                 await CommitAndSend(1, sw, scanElapsedTime, series);
+                 await _eventHub.SendMessageAsync(MessageFactory.SeriesRemoved,
+                     MessageFactory.SeriesRemovedEvent(seriesId, string.Empty, series.LibraryId), false);
              }
-             else
+             catch (Exception ex)
              {
-                 // I think we should just fail and tell user to fix their setup. This is extremely expensive for an edge case
-                 _logger.LogCritical("We weren't able to find any files in the series scan, but there should be. Please correct your naming convention or put Series in a dedicated folder. Aborting scan");
-                 await _eventHub.SendMessageAsync(MessageFactory.Error,
-                     MessageFactory.ErrorEvent($"Error scanning {series.Name}", "We weren't able to find any files in the series scan, but there should be. Please correct your naming convention or put Series in a dedicated folder. Aborting scan"));
+                 _logger.LogCritical(ex, "There was an error during ScanSeries to delete the series as no files could be found. Aborting scan");
                  await _unitOfWork.RollbackAsync();
                  return;
              }
-             // At this point, parsedSeries will have at least one key and we can perform the update. If it still doesn't, just return and don't do anything
-             if (parsedSeries.Count == 0) return;
          }
+         else
+         {
+             // I think we should just fail and tell user to fix their setup. This is extremely expensive for an edge case
+             _logger.LogCritical("We weren't able to find any files in the series scan, but there should be. Please correct your naming convention or put Series in a dedicated folder. Aborting scan");
+             await _eventHub.SendMessageAsync(MessageFactory.Error,
+                 MessageFactory.ErrorEvent($"Error scanning {series.Name}", "We weren't able to find any files in the series scan, but there should be. Please correct your naming convention or put Series in a dedicated folder. Aborting scan"));
+             await _unitOfWork.RollbackAsync();
+             return;
+         }
+         // At this point, parsedSeries will have at least one key and we can perform the update. If it still doesn't, just return and don't do anything
+         if (parsedSeries.Count == 0) return;
+        }
 
 
-         await _eventHub.SendMessageAsync(MessageFactory.NotificationProgress, MessageFactory.LibraryScanProgressEvent(library.Name, ProgressEventType.Ended, series.Name));
+        await _eventHub.SendMessageAsync(MessageFactory.NotificationProgress, MessageFactory.LibraryScanProgressEvent(library.Name, ProgressEventType.Ended, series.Name));
         // Tell UI that this series is done
         await _eventHub.SendMessageAsync(MessageFactory.ScanSeries,
             MessageFactory.ScanSeriesEvent(library.Id, seriesId, series.Name));
@@ -598,6 +598,8 @@ async Task TrackFiles(Tuple<bool, IList<ParserInfo>> parsedInfo)
 
 
             seenSeries.Add(foundParsedSeries);
+            // TODO: This is extremely expensive to lock the thread on this. We should instead move this onto Hangfire
+            // or in a queue to be processed.
             await _seriesProcessingSemaphore.WaitAsync();
             try
             {

UI/Web/src/app/_pipes/library-type.pipe.ts

@@ -18,6 +18,10 @@ export class LibraryTypePipe implements PipeTransform {
         return this.translocoService.translate('library-type-pipe.book');
       case LibraryType.Comic:
         return this.translocoService.translate('library-type-pipe.comic');
+      case LibraryType.ComicVine:
+        return this.translocoService.translate('library-type-pipe.comicVine');
+      case LibraryType.Images:
+        return this.translocoService.translate('library-type-pipe.image');
       case LibraryType.Manga:
         return this.translocoService.translate('library-type-pipe.manga');
       default:

UI/Web/src/assets/langs/en.json

@@ -497,7 +497,9 @@
     "library-type-pipe": {
         "book": "Book",
         "comic": "Comic",
-        "manga": "Manga"
+        "manga": "Manga",
+        "comicVine": "ComicVine",
+        "image": "Image"
     },
 
     "age-rating-pipe": {

openapi.json

@@ -7,7 +7,7 @@
       "name": "GPL-3.0",
       "url": "https://github.com/Kareadita/Kavita/blob/develop/LICENSE"
     },
-    "version": "0.7.14.6"
+    "version": "0.7.14.7"
   },
   "servers": [
     {