Cover generation issue on first scan flow (#517)

* Cover generation issue on first scan flow

- Fixed logic around whether a chapter cover image should be generated. New logic adds grouping priority, changes an AND to an OR and adds an additional check to see if the cover image has been lock (custom image uploaded)

* Sonar update

* Refactored out the cover image updating logic to a new call (ShouldUpdateCoverImage) and updated ONLY chapters. Added a blank slate unit test to build out conditions.

* Fixed up unit case

* Fixed some logic on when to update a cover image

* Fixed an issue where 1) we were refreshing metadata anytime we adjusted cover image on a series and 2) Cover generation wasn't properly being handled on first run.

* Cleaned up the code for when a cover image change needs to trigger a refresh metadata task

Co-authored-by: Joseph Milazzo <joseph.v.milazzo@gmail.com>

API.Tests/Extensions/Test Data/modified on run.txt

@@ -1,2 +1,3 @@
 This file should be modified by the unit test08/20/2021 10:26:03
 08/20/2021 10:26:29
+08/22/2021 12:39:58

API.Tests/Services/MetadataServiceTests.cs

@@ -0,0 +1,105 @@
+using System;
+using System.IO;
+using API.Entities;
+using API.Interfaces;
+using API.Interfaces.Services;
+using API.Services;
+using Microsoft.Extensions.Logging;
+using NSubstitute;
+using Xunit;
+
+namespace API.Tests.Services
+{
+    public class MetadataServiceTests
+    {
+        private readonly string _testDirectory = Path.Join(Directory.GetCurrentDirectory(), "../../../Services/Test Data/ArchiveService/Archives");
+        private readonly MetadataService _metadataService;
+        private readonly IUnitOfWork _unitOfWork = Substitute.For<IUnitOfWork>();
+        private readonly IImageService _imageService = Substitute.For<IImageService>();
+        private readonly IBookService _bookService = Substitute.For<IBookService>();
+        private readonly IArchiveService _archiveService = Substitute.For<IArchiveService>();
+        private readonly ILogger<MetadataService> _logger = Substitute.For<ILogger<MetadataService>>();
+
+        public MetadataServiceTests()
+        {
+            _metadataService = new MetadataService(_unitOfWork, _logger, _archiveService, _bookService, _imageService);
+        }
+
+        [Fact]
+        public void ShouldUpdateCoverImage_OnFirstRun()
+        {
+            // Represents first run
+            Assert.True(MetadataService.ShouldUpdateCoverImage(null, new MangaFile()
+            {
+                FilePath = Path.Join(_testDirectory, "file in folder.zip"),
+                LastModified = DateTime.Now
+            }, false, false));
+        }
+
+        [Fact]
+        public void ShouldUpdateCoverImage_OnSecondRun_FileModified()
+        {
+            // Represents first run
+            Assert.True(MetadataService.ShouldUpdateCoverImage(null, new MangaFile()
+            {
+                FilePath = Path.Join(_testDirectory, "file in folder.zip"),
+                LastModified = new FileInfo(Path.Join(_testDirectory, "file in folder.zip")).LastWriteTime.Subtract(TimeSpan.FromDays(1))
+            }, false, false));
+        }
+
+        [Fact]
+        public void ShouldUpdateCoverImage_OnSecondRun_CoverImageLocked()
+        {
+            // Represents first run
+            Assert.False(MetadataService.ShouldUpdateCoverImage(null, new MangaFile()
+            {
+                FilePath = Path.Join(_testDirectory, "file in folder.zip"),
+                LastModified = new FileInfo(Path.Join(_testDirectory, "file in folder.zip")).LastWriteTime
+            }, false, true));
+        }
+
+        [Fact]
+        public void ShouldUpdateCoverImage_OnSecondRun_ForceUpdate()
+        {
+            // Represents first run
+            Assert.True(MetadataService.ShouldUpdateCoverImage(null, new MangaFile()
+            {
+                FilePath = Path.Join(_testDirectory, "file in folder.zip"),
+                LastModified = new FileInfo(Path.Join(_testDirectory, "file in folder.zip")).LastWriteTime
+            }, true, false));
+        }
+
+        [Fact]
+        public void ShouldUpdateCoverImage_OnSecondRun_NoFileChangeButNoCoverImage()
+        {
+            // Represents first run
+            Assert.True(MetadataService.ShouldUpdateCoverImage(null, new MangaFile()
+            {
+                FilePath = Path.Join(_testDirectory, "file in folder.zip"),
+                LastModified = new FileInfo(Path.Join(_testDirectory, "file in folder.zip")).LastWriteTime
+            }, false, false));
+        }
+
+        [Fact]
+        public void ShouldUpdateCoverImage_OnSecondRun_FileChangeButNoCoverImage()
+        {
+            // Represents first run
+            Assert.True(MetadataService.ShouldUpdateCoverImage(null, new MangaFile()
+            {
+                FilePath = Path.Join(_testDirectory, "file in folder.zip"),
+                LastModified = new FileInfo(Path.Join(_testDirectory, "file in folder.zip")).LastWriteTime + TimeSpan.FromDays(1)
+            }, false, false));
+        }
+
+        [Fact]
+        public void ShouldUpdateCoverImage_OnSecondRun_CoverImageSet()
+        {
+            // Represents first run
+            Assert.False(MetadataService.ShouldUpdateCoverImage(new byte[] {1}, new MangaFile()
+            {
+                FilePath = Path.Join(_testDirectory, "file in folder.zip"),
+                LastModified = new FileInfo(Path.Join(_testDirectory, "file in folder.zip")).LastWriteTime
+            }, false, false));
+        }
+    }
+}

API/Controllers/SeriesController.cs

@@ -155,13 +155,14 @@ namespace API.Controllers
             series.Name = updateSeries.Name.Trim();
             series.LocalizedName = updateSeries.LocalizedName.Trim();
             series.SortName = updateSeries.SortName?.Trim();
-            series.Summary = updateSeries.Summary?.Trim(); // BUG: There was an exceptionSystem.NullReferenceException: Object reference not set to an instance of an object.
+            series.Summary = updateSeries.Summary?.Trim();
 
             var needsRefreshMetadata = false;
-            if (!updateSeries.CoverImageLocked)
+            if (series.CoverImageLocked && !updateSeries.CoverImageLocked)
             {
-                series.CoverImageLocked = false;
+                // Trigger a refresh when we are moving from a locked image to a non-locked
                 needsRefreshMetadata = true;
+                series.CoverImageLocked = updateSeries.CoverImageLocked;
             }
 
             _unitOfWork.SeriesRepository.Update(series);

API/Entities/MangaFile.cs

@@ -5,6 +5,9 @@ using API.Entities.Enums;
 
 namespace API.Entities
 {
+    /// <summary>
+    /// Represents a wrapper to the underlying file. This provides information around file, like number of pages, format, etc.
+    /// </summary>
     public class MangaFile
     {
         public int Id { get; set; }

API/Extensions/FileInfoExtensions.cs

@@ -13,7 +13,8 @@ namespace API.Extensions
         /// <returns></returns>
         public static bool HasFileBeenModifiedSince(this FileInfo fileInfo, DateTime comparison)
         {
-            return fileInfo?.LastWriteTime > comparison;
+            return DateTime.Compare(fileInfo.LastWriteTime, comparison) > 0;
+            //return fileInfo?.LastWriteTime > comparison;
         }
     }
 }

API/Services/MetadataService.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.IO;
@@ -37,13 +37,35 @@ namespace API.Services
             _imageService = imageService;
         }
 
-        private static bool ShouldFindCoverImage(byte[] coverImage, bool forceUpdate = false)
+        private static bool IsCoverImageSet(byte[] coverImage, bool forceUpdate = false)
         {
-            return forceUpdate || coverImage == null || !coverImage.Any();
+            return forceUpdate || HasCoverImage(coverImage);
+        }
+
+        /// <summary>
+        /// Determines whether an entity should regenerate cover image
+        /// </summary>
+        /// <param name="coverImage"></param>
+        /// <param name="firstFile"></param>
+        /// <param name="forceUpdate"></param>
+        /// <param name="isCoverLocked"></param>
+        /// <returns></returns>
+        public static bool ShouldUpdateCoverImage(byte[] coverImage, MangaFile firstFile, bool forceUpdate = false,
+            bool isCoverLocked = false)
+        {
+            if (isCoverLocked) return false;
+            if (forceUpdate) return true;
+            return (firstFile != null && firstFile.HasFileBeenModified()) || !HasCoverImage(coverImage);
+        }
+
+        private static bool HasCoverImage(byte[] coverImage)
+        {
+            return coverImage != null && coverImage.Any();
         }
 
         private byte[] GetCoverImage(MangaFile file, bool createThumbnail = true)
         {
+            file.LastModified = DateTime.Now;
             switch (file.Format)
             {
                 case MangaFormat.Pdf:
@@ -57,6 +79,7 @@ namespace API.Services
                 default:
                     return Array.Empty<byte>();
             }
+
         }
 
         /// <summary>
@@ -67,12 +90,9 @@ namespace API.Services
         public void UpdateMetadata(Chapter chapter, bool forceUpdate)
         {
             var firstFile = chapter.Files.OrderBy(x => x.Chapter).FirstOrDefault();
-            if (!chapter.CoverImageLocked
+
-                && ShouldFindCoverImage(chapter.CoverImage, forceUpdate)
+            if (ShouldUpdateCoverImage(chapter.CoverImage, firstFile, forceUpdate, chapter.CoverImageLocked))
-                && firstFile != null
-                && (forceUpdate || new FileInfo(firstFile.FilePath).HasFileBeenModifiedSince(firstFile.LastModified)))
             {
-                chapter.Files ??= new List<MangaFile>();
                 chapter.CoverImage = GetCoverImage(firstFile);
             }
         }
@@ -84,7 +104,7 @@ namespace API.Services
         /// <param name="forceUpdate">Force updating cover image even if underlying file has not been modified or chapter already has a cover image</param>
         public void UpdateMetadata(Volume volume, bool forceUpdate)
         {
-            if (volume == null || !ShouldFindCoverImage(volume.CoverImage, forceUpdate)) return;
+            if (volume == null || !IsCoverImageSet(volume.CoverImage, forceUpdate)) return;
 
             volume.Chapters ??= new List<Chapter>();
             var firstChapter = volume.Chapters.OrderBy(x => double.Parse(x.Number), _chapterSortComparer).FirstOrDefault();
@@ -102,7 +122,7 @@ namespace API.Services
         public void UpdateMetadata(Series series, bool forceUpdate)
         {
             if (series == null) return;
-            if (!series.CoverImageLocked && ShouldFindCoverImage(series.CoverImage, forceUpdate))
+            if (ShouldUpdateCoverImage(series.CoverImage, null, forceUpdate, series.CoverImageLocked))
             {
                 series.Volumes ??= new List<Volume>();
                 var firstCover = series.Volumes.GetCoverImage(series.Format);
@@ -116,7 +136,7 @@ namespace API.Services
                             .FirstOrDefault(c => !c.IsSpecial)?.CoverImage;
                     }
 
-                    if (coverImage == null)
+                    if (!HasCoverImage(coverImage))
                     {
                         coverImage = series.Volumes[0].Chapters.OrderBy(c => double.Parse(c.Number), _chapterSortComparer)
                             .FirstOrDefault()?.CoverImage;
@@ -151,7 +171,7 @@ namespace API.Services
 
 
         /// <summary>
-        /// Refreshes Metatdata for a whole library
+        /// Refreshes Metadata for a whole library
         /// </summary>
         /// <remarks>This can be heavy on memory first run</remarks>
         /// <param name="libraryId"></param>

API/Services/Tasks/ScannerService.cs

@@ -522,14 +522,8 @@ namespace API.Services.Tasks
              if (file != null)
              {
                 chapter.Files.Add(file);
-                existingFile = chapter.Files.Last();
              }
           }
-
-          if (existingFile != null)
-          {
-             existingFile.LastModified = new FileInfo(existingFile.FilePath).LastWriteTime;
-          }
        }
     }
 }