review comment fixes: Updating to use assetid, adding finished date a…

…nd tightening up how we get assets from the generated manifest body + general fixes

src/IIIFPresentation/API.Tests/Converters/ManifestConverterTests.cs

@@ -3,7 +3,7 @@
 using API.Tests.Helpers;
 using Models.API.Manifest;
 using Models.Database.General;
-using Newtonsoft.Json.Linq;
+using Models.DLCS;
 using CanvasPainting = Models.Database.CanvasPainting;
 using DBManifest = Models.Database.Collections.Manifest;
 
@@ -169,7 +169,7 @@ public void SetGeneratedFields_SetsCanvasPainting_IfPresent()
                     Id = "the-canvas",
                     ChoiceOrder = 10,
                     CanvasOrder = 100,
-                    AssetId = "1/2/assetId"
+                    AssetId = new AssetId(1, 2, "assetId")
                 }
             ]
         };
@@ -184,7 +184,7 @@ public void SetGeneratedFields_SetsCanvasPainting_IfPresent()
         paintedResource.CanvasPainting.CanvasOrder.Should().Be(100);
         paintedResource.CanvasPainting.CanvasOriginalId.Should().Be("http://example.test/canvas1");
 
-        paintedResource.Asset.GetValue("assetId").ToString().Should().Be("https://dlcs.test/customers/1/spaces/2/images/assetId");
+        paintedResource.Asset.GetValue("@id").ToString().Should().Be("https://dlcs.test/customers/1/spaces/2/images/assetId");
     }
 
     [Fact]

src/IIIFPresentation/API.Tests/Helpers/PathGeneratorTests.cs

@@ -3,6 +3,7 @@
 using Models.Database;
 using Models.Database.Collections;
 using Models.Database.General;
+using Models.DLCS;
 
 namespace API.Tests.Helpers;
 
@@ -385,31 +386,18 @@ public void GenerateAssetUri_Correct_IfCanvasPaintingHasAssetWithThreeSlashes()
         {
             Id = "hello",
             CustomerId = 123,
-            AssetId = "5/4/12"
+            AssetId = new AssetId(5, 4, "12")
         };
 
         var expected = "https://dlcs.test/customers/5/spaces/4/images/12";
 
         pathGenerator.GenerateAssetUri(manifest).Should().Be(expected);
     }
 
-    [Fact]
-    public void GenerateAssetUri_Null_IfCanvasPaintingHasAssetWithNotThreeSlashes()
-    {
-        var manifest = new CanvasPainting()
-        {
-            Id = "hello",
-            CustomerId = 123,
-            AssetId = "5/4"
-        };
-
-        pathGenerator.GenerateAssetUri(manifest).Should().BeNull();
-    }
-
     [Fact]
     public void GenerateAssetUri_Null_IfCanvasPaintingDoesNotHaveAsset()
     {
-        var manifest = new CanvasPainting()
+        var manifest = new CanvasPainting
         {
             Id = "hello",
             CustomerId = 123,

src/IIIFPresentation/API.Tests/Integration/ModifyManifestAssetCreationTests.cs

@@ -187,7 +187,7 @@ public async Task CreateManifest_CorrectlyCreatesAssetRequests_WithSpace()
         dbManifest.CanvasPaintings.Should().HaveCount(1);
         dbManifest.CanvasPaintings!.First().Label!.First().Value[0].Should().Be("canvas testing");
         // space comes from the asset
-        dbManifest.CanvasPaintings!.First().AssetId.Should().Be($"{Customer}/{space}/{assetId}");
+        dbManifest.CanvasPaintings!.First().AssetId.ToString().Should().Be($"{Customer}/{space}/{assetId}");
         dbManifest.Batches.Should().HaveCount(1);
         dbManifest.Batches!.First().Status.Should().Be(BatchStatus.Ingesting);
         dbManifest.Batches!.First().Id.Should().Be(batchId);
@@ -288,7 +288,7 @@ public async Task CreateManifest_CorrectlyCreatesAssetRequests_WithoutSpace()
         dbManifest.CanvasPaintings.Should().HaveCount(1);
         dbManifest.CanvasPaintings!.First().Label!.First().Value[0].Should().Be("canvas testing");
         // space added using the manifest space
-        dbManifest.CanvasPaintings!.First().AssetId.Should()
+        dbManifest.CanvasPaintings!.First().AssetId.ToString().Should()
             .Be($"{Customer}/{NewlyCreatedSpace}/{assetId}");
         dbManifest.Batches.Should().HaveCount(1);
         dbManifest.Batches!.First().Status.Should().Be(BatchStatus.Ingesting);
@@ -383,7 +383,7 @@ await amazonS3.GetObjectAsync(LocalStackFixture.StorageBucketName,
         dbManifest.CanvasPaintings.Should().HaveCount(1);
         dbManifest.CanvasPaintings!.First().Label?.First().Should().BeNull();
         // space added using the manifest space
-        dbManifest.CanvasPaintings!.First().AssetId.Should()
+        dbManifest.CanvasPaintings!.First().AssetId.ToString().Should()
             .Be($"{Customer}/{NewlyCreatedSpace}/{assetId}");
         dbManifest.Batches.Should().HaveCount(1);
         dbManifest.Batches!.First().Status.Should().Be(BatchStatus.Ingesting);
@@ -595,7 +595,7 @@ public async Task CreateManifest_CorrectlyCreatesAssetRequests_WhenMultipleAsset
          foreach (var canvasPainting in dbManifest.CanvasPaintings!)
          {
              canvasPainting.CanvasOrder.Should().Be(currentCanvasOrder);
-             canvasPainting.AssetId.Should()
+             canvasPainting.AssetId.ToString().Should()
                  .Be($"{Customer}/{NewlyCreatedSpace}/testAssetByPresentation-multipleAssets-{currentCanvasOrder}");
              currentCanvasOrder++;
          }

src/IIIFPresentation/API/Converters/ManifestConverter.cs

@@ -67,7 +67,7 @@ public static PresentationManifest SetGeneratedFields(this PresentationManifest
             },
             Asset = cp.AssetId == null ? null : new JObject
             {
-                ["assetId"] = pathGenerator.GenerateAssetUri(cp)?.ToString()
+                ["@id"] = pathGenerator.GenerateAssetUri(cp)?.ToString()
             }
         }).ToList();
     }

src/IIIFPresentation/API/Features/Manifest/CanvasPaintingResolver.cs

@@ -10,6 +10,7 @@
 using Models.API.General;
 using Models.API.Manifest;
 using Models.Database;
+using Models.DLCS;
 using Newtonsoft.Json.Linq;
 using Repository.Manifests;
 using CanvasPainting = Models.Database.CanvasPainting;
@@ -217,7 +218,7 @@ public class CanvasPaintingResolver(
                 Created = DateTime.UtcNow,
                 CustomerId = customerId,
                 CanvasOrder = count,
-                AssetId = $"{customerId}/{space}/{id}",
+                AssetId = AssetId.FromString($"{customerId}/{space}/{id}"),
                 ChoiceOrder = -1,
                 Ingesting = true
             };

src/IIIFPresentation/API/Helpers/PathGenerator.cs

@@ -4,6 +4,7 @@
 using Microsoft.Extensions.Options;
 using Models.Database.Collections;
 using Models.Database.General;
+using Models.DLCS;
 using CanvasPainting = Models.Database.CanvasPainting;
 
 namespace API.Helpers;
@@ -92,22 +93,11 @@ public string GenerateCanvasId(CanvasPainting canvasPainting)
 
     public Uri? GenerateAssetUri(CanvasPainting canvasPainting)
     {
-        if (string.IsNullOrEmpty(canvasPainting.AssetId)) return null;
-
-        AssetId assetId;
-
-        try
-        {
-            assetId = AssetId.FromString(canvasPainting.AssetId);
-        }
-        catch // swallow error as it's not needed
-        {
-            return null;
-        }
-
+        if (canvasPainting.AssetId == null) return null;
+
         var uriBuilder = new UriBuilder(dlcsSettings.ApiUri)
         {
-            Path = $"/customers/{assetId.Customer}/spaces/{assetId.Space}/images/{assetId.Asset}",
+            Path = $"/customers/{canvasPainting.AssetId.Customer}/spaces/{canvasPainting.AssetId.Space}/images/{canvasPainting.AssetId.Asset}",
         };
         return uriBuilder.Uri;
     }

src/IIIFPresentation/BackgroundHandler.Tests/BatchCompletion/BatchCompletionMessageHandlerTests.cs

@@ -4,7 +4,6 @@
 using BackgroundHandler.Tests.infrastructure;
 using DLCS;
 using DLCS.API;
-using DLCS.Models;
 using FakeItEasy;
 using FluentAssertions;
 using IIIF;
@@ -17,6 +16,7 @@
 using Microsoft.Extensions.Options;
 using Models.Database.Collections;
 using Models.Database.General;
+using Models.DLCS;
 using Repository;
 using Test.Helpers.Helpers;
 using Test.Helpers.Integration;
@@ -34,6 +34,7 @@ public class BatchCompletionMessageHandlerTests
     private readonly BatchCompletionMessageHandler sut;
     private readonly IDlcsOrchestratorClient dlcsClient;
     private readonly IIIIFS3Service iiifS3;
+    private readonly DlcsSettings dlcsSettings;
     private readonly int customerId = 1;
 
     public BatchCompletionMessageHandlerTests(PresentationContextFixture dbFixture)
@@ -42,7 +43,13 @@ public BatchCompletionMessageHandlerTests(PresentationContextFixture dbFixture)
         dbContext.ChangeTracker.QueryTrackingBehavior = QueryTrackingBehavior.TrackAll;
         dlcsClient = A.Fake<IDlcsOrchestratorClient>();
         iiifS3 = A.Fake<IIIIFS3Service>();
-        sut = new BatchCompletionMessageHandler(dbFixture.DbContext, dlcsClient, iiifS3,
+        dlcsSettings = new DlcsSettings()
+        {
+            ApiUri = new Uri("https://localhost:5000"),
+            OrchestratorUri = new Uri("https://localhost:5000")
+        };
+
+        sut = new BatchCompletionMessageHandler(dbFixture.DbContext, dlcsClient, Options.Create(dlcsSettings), iiifS3,
             new NullLogger<BatchCompletionMessageHandler>());
     }
 
@@ -64,7 +71,7 @@ public async Task HandleMessage_DoesNotUpdateAnything_WhenBatchNotTracked()
 
         // Act and Assert
         (await sut.HandleMessage(message, CancellationToken.None)).Should().BeTrue();
-        A.CallTo(() => dlcsClient.RetrieveImagesForManifest(A<int>._, A<List<int>>._, A<CancellationToken>._))
+        A.CallTo(() => dlcsClient.RetrieveAssetsForManifest(A<int>._, A<List<int>>._, A<CancellationToken>._))
             .MustNotHaveHappened();
     }
 
@@ -106,14 +113,14 @@ public async Task HandleMessage_UpdatesBatchedImages_WhenBatchTracked()
 
         var message = GetMessage(batchMessage);
 
-        A.CallTo(() => dlcsClient.RetrieveImagesForManifest(A<int>._, A<List<int>>._, A<CancellationToken>._))
+        A.CallTo(() => dlcsClient.RetrieveAssetsForManifest(A<int>._, A<List<int>>._, A<CancellationToken>._))
             .Returns(new IIIF.Presentation.V3.Manifest
             {
                 Items = new List<Canvas>
                 {
                     new ()
                     {
-                        Id = fullAssetId.ToString(),
+                        Id = $"{dlcsSettings.OrchestratorUri}/iiif-img/{fullAssetId}/canvas/c/1",
                         Width = 100,
                         Height = 100,
                         Annotations = new List<AnnotationPage>
@@ -149,16 +156,17 @@ public async Task HandleMessage_UpdatesBatchedImages_WhenBatchTracked()
 
         // Assert
         handleMessage.Should().BeTrue();
-        A.CallTo(() => dlcsClient.RetrieveImagesForManifest(A<int>._, A<List<int>>._, A<CancellationToken>._))
+        A.CallTo(() => dlcsClient.RetrieveAssetsForManifest(A<int>._, A<List<int>>._, A<CancellationToken>._))
             .MustHaveHappened();
         var batch = dbContext.Batches.Single(b => b.Id == batchId);
         batch.Status.Should().Be(BatchStatus.Completed);
-        batch.Processed.Should().BeCloseTo(finished, TimeSpan.FromSeconds(10));
-        var canvasPainting = dbContext.CanvasPaintings.Single(c => c.AssetId == fullAssetId.ToString());
+        batch.Finished.Should().BeCloseTo(finished, TimeSpan.FromSeconds(10));
+        batch.Processed.Should().BeCloseTo(DateTime.UtcNow, TimeSpan.FromSeconds(10));
+        var canvasPainting = dbContext.CanvasPaintings.Single(c => c.AssetId == fullAssetId);
         canvasPainting.Ingesting.Should().BeFalse();
         canvasPainting.StaticWidth.Should().Be(100);
         canvasPainting.StaticHeight.Should().Be(100);
-        canvasPainting.AssetId.Should()
+        canvasPainting.AssetId.ToString().Should()
             .Be(fullAssetId.ToString());
     }
 
@@ -201,8 +209,10 @@ public async Task HandleMessage_DoesNotUpdateBatchedImages_WhenAnotherBatchWaiti
 
         // Act and Assert
         (await sut.HandleMessage(message, CancellationToken.None)).Should().BeTrue();
-        A.CallTo(() => dlcsClient.RetrieveImagesForManifest(A<int>._, A<List<int>>._, A<CancellationToken>._))
+        A.CallTo(() => dlcsClient.RetrieveAssetsForManifest(A<int>._, A<List<int>>._, A<CancellationToken>._))
             .MustNotHaveHappened();
+        var batch = await dbContext.Batches.SingleOrDefaultAsync(b => b.Id == batchId);
+        batch.Status.Should().Be(BatchStatus.Completed);
     }
 
     private Manifest CreateManifest(string manifestId, string slug, string assetId, int space, int batchId)
@@ -227,7 +237,7 @@ private Manifest CreateManifest(string manifestId, string slug, string assetId,
             CanvasPaintings = [
                 new CanvasPainting
                 {
-                    AssetId = $"{customerId}/{space}/{assetId}",
+                    AssetId = new AssetId(customerId, space, assetId),
                     CanvasOrder = 0,
                     ChoiceOrder = 1,
                     CustomerId = customerId,
@@ -238,6 +248,7 @@ private Manifest CreateManifest(string manifestId, string slug, string assetId,
                 new Batch
                 {
                     Id = batchId,
+                    ManifestId = manifestId,
                     CustomerId = customerId,
                     Submitted = DateTime.UtcNow,
                     Status = BatchStatus.Ingesting

src/IIIFPresentation/BackgroundHandler.Tests/Helpers/ManifestMergerTests.cs

@@ -6,6 +6,7 @@
 using IIIF.Presentation.V3.Content;
 using IIIF.Presentation.V3.Strings;
 using Models.Database;
+using Models.DLCS;
 using Canvas = IIIF.Presentation.V3.Canvas;
 using Manifest = IIIF.Presentation.V3.Manifest;
 
@@ -18,11 +19,15 @@ public void Merge_MergesBlankManifestWithGeneratedManifest()
     {
         // Arrange
         var blankManifest = new Manifest();
-        var generatedManifest = GeneratedManifest(nameof(Merge_MergesBlankManifestWithGeneratedManifest));
+        var assetId = $"1/2/{nameof(Merge_MergesBlankManifestWithGeneratedManifest)}";
+
+        var generatedManifest = GeneratedManifest(assetId);
+        var itemDictionary = generatedManifest.Items.ToDictionary(i => AssetId.FromString(i.Id), i => i);
 
         // Act
-        var mergedManifest = ManifestMerger.Merge(blankManifest, generatedManifest,
-            GenerateCanvasPaintings([nameof(Merge_MergesBlankManifestWithGeneratedManifest)]));
+        var mergedManifest = ManifestMerger.Merge(blankManifest,
+            GenerateCanvasPaintings([assetId]), itemDictionary,
+            generatedManifest.Thumbnail);
 
         // Assert
         mergedManifest.Items.Count.Should().Be(1);
@@ -38,14 +43,17 @@ public void Merge_MergesBlankManifestWithGeneratedManifest()
     public void Merge_MergesFullManifestWithGeneratedManifest()
     {
         // Arrange
-        var blankManifest = GeneratedManifest(nameof(Merge_MergesFullManifestWithGeneratedManifest));
+        var assetId = $"1/2/{nameof(Merge_MergesFullManifestWithGeneratedManifest)}";
+        var blankManifest = GeneratedManifest(assetId);
         blankManifest.Items[0].Width = 200;
         blankManifest.Items[0].Height = 200;
-        var generatedManifest = GeneratedManifest(nameof(Merge_MergesFullManifestWithGeneratedManifest));
+        var generatedManifest = GeneratedManifest(assetId);
+        var itemDictionary = generatedManifest.Items.ToDictionary(i => AssetId.FromString(i.Id), i => i);
 
         // Act
-        var mergedManifest = ManifestMerger.Merge(blankManifest, generatedManifest,
-            GenerateCanvasPaintings([nameof(Merge_MergesFullManifestWithGeneratedManifest)]));
+        var mergedManifest = ManifestMerger.Merge(blankManifest,
+            GenerateCanvasPaintings([assetId]), itemDictionary,
+            generatedManifest.Thumbnail);
 
         // Assert
         mergedManifest.Items.Count.Should().Be(1);
@@ -58,16 +66,19 @@ public void Merge_MergesFullManifestWithGeneratedManifest()
     public void Merge_ShouldNotUpdateAttachedManifestThumbnail()
     {
         // Arrange
-        var blankManifest = GeneratedManifest(nameof(Merge_ShouldNotUpdateAttachedManifestThumbnail));
+        var assetId = $"1/2/{nameof(Merge_ShouldNotUpdateAttachedManifestThumbnail)}";
+        var blankManifest = GeneratedManifest(assetId);
         blankManifest.Items[0].Width = 200;
         blankManifest.Items[0].Height = 200;
         blankManifest.Thumbnail.Add(GenerateImage());
         blankManifest.Thumbnail[0].Service[0].Id = "generatedId";
-        var generatedManifest = GeneratedManifest(nameof(Merge_ShouldNotUpdateAttachedManifestThumbnail));
+        var generatedManifest = GeneratedManifest(assetId);
+        var itemDictionary = generatedManifest.Items.ToDictionary(i => AssetId.FromString(i.Id), i => i);
 
         // Act
-        var mergedManifest = ManifestMerger.Merge(blankManifest, generatedManifest,
-            GenerateCanvasPaintings([nameof(Merge_ShouldNotUpdateAttachedManifestThumbnail)]));
+        var mergedManifest = ManifestMerger.Merge(blankManifest,
+            GenerateCanvasPaintings([assetId]), itemDictionary,
+            generatedManifest.Thumbnail);
 
         // Assert
         mergedManifest.Items.Count.Should().Be(1);
@@ -82,34 +93,35 @@ public void Merge_CorrectlyOrdersMultipleItems()
     {
         // Arrange
         var blankManifest = new Manifest();
-        var generatedManifest = GeneratedManifest($"{nameof(Merge_ShouldNotUpdateAttachedManifestThumbnail)}_1");
-        generatedManifest.Items.Add(GenerateCanvas($"{nameof(Merge_ShouldNotUpdateAttachedManifestThumbnail)}_2"));
+        var generatedManifest = GeneratedManifest($"1/2/{nameof(Merge_CorrectlyOrdersMultipleItems)}_1");
+        generatedManifest.Items.Add(GenerateCanvas($"1/2/{nameof(Merge_CorrectlyOrdersMultipleItems)}_2"));
+        var itemDictionary = generatedManifest.Items.ToDictionary(i => AssetId.FromString(i.Id), i => i);
 
         var canvasPaintings = GenerateCanvasPaintings([
-            $"{nameof(Merge_ShouldNotUpdateAttachedManifestThumbnail)}_1",
-            $"{nameof(Merge_ShouldNotUpdateAttachedManifestThumbnail)}_2"
+            $"1/2/{nameof(Merge_CorrectlyOrdersMultipleItems)}_1",
+            $"1/2/{nameof(Merge_CorrectlyOrdersMultipleItems)}_2"
         ]);
 
         canvasPaintings[0].CanvasOrder = 1;
         canvasPaintings[1].CanvasOrder = 0;
 
         // Act
-        var mergedManifest = ManifestMerger.Merge(blankManifest, generatedManifest,
-            canvasPaintings);
+        var mergedManifest =
+            ManifestMerger.Merge(blankManifest, canvasPaintings, itemDictionary, generatedManifest.Thumbnail);
 
         // Assert
         mergedManifest.Items.Count.Should().Be(2);
         mergedManifest.Thumbnail[0].Service[0].Id.Should().Be("imageId");
         mergedManifest.Thumbnail.Count.Should().Be(1);
         // order flipped due to canvas order
-        mergedManifest.Items[0].Id.Should().Be($"{nameof(Merge_ShouldNotUpdateAttachedManifestThumbnail)}_2");
-        mergedManifest.Items[1].Id.Should().Be($"{nameof(Merge_ShouldNotUpdateAttachedManifestThumbnail)}_1");
+        mergedManifest.Items[0].Id.Should().Be($"1/2/{nameof(Merge_CorrectlyOrdersMultipleItems)}_2");
+        mergedManifest.Items[1].Id.Should().Be($"1/2/{nameof(Merge_CorrectlyOrdersMultipleItems)}_1");
     }
 
     private List<CanvasPainting> GenerateCanvasPaintings(List<string> idList)
     {
         var canvasOrder = 0;
-        return idList.Select(id => new CanvasPainting{Id = id, AssetId = id, CanvasOrder = canvasOrder++}).ToList();
+        return idList.Select(id => new CanvasPainting{Id = id, AssetId = AssetId.FromString(id), CanvasOrder = canvasOrder++}).ToList();
     }
 
     private Manifest GeneratedManifest(string id)