diff --git a/src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/MediaPickerWithCropsValueConverter.cs b/src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/MediaPickerWithCropsValueConverter.cs index a33bb9870d..19f9b5a908 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/MediaPickerWithCropsValueConverter.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/MediaPickerWithCropsValueConverter.cs @@ -131,7 +131,16 @@ public class MediaPickerWithCropsValueConverter : PropertyValueConverterBase, ID ApiMediaWithCrops ToApiMedia(MediaWithCrops media) { IApiMedia inner = _apiMediaBuilder.Build(media.Content); - return new ApiMediaWithCrops(inner, media.LocalCrops.FocalPoint, media.LocalCrops.Crops); + + // make sure we merge crops and focal point defined at media level with the locally defined ones (local ones take precedence in case of a conflict) + ImageCropperValue? mediaCrops = media.Content.Value(_publishedValueFallback, Constants.Conventions.Media.File); + ImageCropperValue localCrops = media.LocalCrops; + if (mediaCrops != null) + { + localCrops = localCrops.Merge(mediaCrops); + } + + return new ApiMediaWithCrops(inner, localCrops.FocalPoint, localCrops.Crops); } // NOTE: eventually we might implement this explicitly instead of piggybacking on the default object conversion. however, this only happens once per cache rebuild, diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/DeliveryApi/MediaPickerWithCropsValueConverterTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/DeliveryApi/MediaPickerWithCropsValueConverterTests.cs index 23dc5bb8b3..c02526054e 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/DeliveryApi/MediaPickerWithCropsValueConverterTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/DeliveryApi/MediaPickerWithCropsValueConverterTests.cs @@ -2,7 +2,6 @@ using Moq; using NUnit.Framework; using Umbraco.Cms.Core; using Umbraco.Cms.Core.DeliveryApi; -using Umbraco.Cms.Core.DeliveryApi.Accessors; using Umbraco.Cms.Core.Models.DeliveryApi; using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.PropertyEditors; @@ -63,32 +62,15 @@ public class MediaPickerWithCropsValueConverterTests : PropertyValueConverterTes var result = valueConverter.ConvertIntermediateToDeliveryApiObject(Mock.Of(), publishedPropertyType, PropertyCacheLevel.Element, inter, false) as IEnumerable; Assert.NotNull(result); Assert.AreEqual(1, result.Count()); - Assert.AreEqual("My media", result.First().Name); - Assert.AreEqual("my-media", result.First().Url); - Assert.AreEqual(".jpg", result.First().Extension); - Assert.AreEqual(200, result.First().Width); - Assert.AreEqual(400, result.First().Height); - Assert.AreEqual(800, result.First().Bytes); - Assert.NotNull(result.First().FocalPoint); - Assert.AreEqual(".jpg", result.First().Extension); - Assert.AreEqual(200, result.First().Width); - Assert.AreEqual(400, result.First().Height); - Assert.AreEqual(800, result.First().Bytes); - Assert.AreEqual(.2m, result.First().FocalPoint.Left); - Assert.AreEqual(.4m, result.First().FocalPoint.Top); - Assert.NotNull(result.First().Crops); - Assert.AreEqual(1, result.First().Crops.Count()); - Assert.AreEqual("one", result.First().Crops.First().Alias); - Assert.AreEqual(100, result.First().Crops.First().Height); - Assert.AreEqual(200, result.First().Crops.First().Width); - Assert.NotNull(result.First().Crops.First().Coordinates); - Assert.AreEqual(1m, result.First().Crops.First().Coordinates.X1); - Assert.AreEqual(2m, result.First().Crops.First().Coordinates.X2); - Assert.AreEqual(10m, result.First().Crops.First().Coordinates.Y1); - Assert.AreEqual(20m, result.First().Crops.First().Coordinates.Y2); - Assert.NotNull(result.First().Properties); - Assert.AreEqual(1, result.First().Properties.Count); - Assert.AreEqual("My alt text", result.First().Properties["altText"]); + var first = result.Single(); + ValidateMedia(first, "My media", "my-media", ".jpg", 200, 400, 800); + ValidateFocalPoint(first.FocalPoint, .2m, .4m); + Assert.NotNull(first.Crops); + Assert.AreEqual(1, first.Crops.Count()); + ValidateCrop(first.Crops.First(), "one", 200, 100, 1m, 2m, 10m, 20m); + Assert.NotNull(first.Properties); + Assert.AreEqual(1, first.Properties.Count); + Assert.AreEqual("My alt text", first.Properties["altText"]); } [Test] @@ -138,52 +120,147 @@ public class MediaPickerWithCropsValueConverterTests : PropertyValueConverterTes var result = valueConverter.ConvertIntermediateToDeliveryApiObject(Mock.Of(), publishedPropertyType, PropertyCacheLevel.Element, inter, false) as IEnumerable; Assert.NotNull(result); Assert.AreEqual(2, result.Count()); + var first = result.First(); + var last = result.Last(); - Assert.AreEqual("My media", result.First().Name); - Assert.AreEqual("my-media", result.First().Url); - Assert.AreEqual(".jpg", result.First().Extension); - Assert.AreEqual(200, result.First().Width); - Assert.AreEqual(400, result.First().Height); - Assert.AreEqual(800, result.First().Bytes); - Assert.NotNull(result.First().FocalPoint); - Assert.AreEqual(.2m, result.First().FocalPoint.Left); - Assert.AreEqual(.4m, result.First().FocalPoint.Top); - Assert.NotNull(result.First().Crops); - Assert.AreEqual(1, result.First().Crops.Count()); - Assert.AreEqual("one", result.First().Crops.First().Alias); - Assert.AreEqual(100, result.First().Crops.First().Height); - Assert.AreEqual(200, result.First().Crops.First().Width); - Assert.NotNull(result.First().Crops.First().Coordinates); - Assert.AreEqual(1m, result.First().Crops.First().Coordinates.X1); - Assert.AreEqual(2m, result.First().Crops.First().Coordinates.X2); - Assert.AreEqual(10m, result.First().Crops.First().Coordinates.Y1); - Assert.AreEqual(20m, result.First().Crops.First().Coordinates.Y2); - Assert.NotNull(result.First().Properties); - Assert.AreEqual(1, result.First().Properties.Count); - Assert.AreEqual("My alt text", result.First().Properties["altText"]); + ValidateMedia(first, "My media", "my-media", ".jpg", 200, 400, 800); + ValidateFocalPoint(first.FocalPoint, .2m, .4m); + Assert.NotNull(first.Crops); + Assert.AreEqual(1, first.Crops.Count()); + ValidateCrop(first.Crops.First(), "one", 200, 100, 1m, 2m, 10m, 20m); + Assert.NotNull(first.Properties); + Assert.AreEqual(1, first.Properties.Count); + Assert.AreEqual("My alt text", first.Properties["altText"]); - Assert.AreEqual("My other media", result.Last().Name); - Assert.AreEqual("my-other-media", result.Last().Url); - Assert.AreEqual(".png", result.Last().Extension); - Assert.AreEqual(800, result.Last().Width); - Assert.AreEqual(600, result.Last().Height); - Assert.AreEqual(200, result.Last().Bytes); - Assert.NotNull(result.Last().FocalPoint); - Assert.AreEqual(.8m, result.Last().FocalPoint.Left); - Assert.AreEqual(.6m, result.Last().FocalPoint.Top); - Assert.NotNull(result.Last().Crops); - Assert.AreEqual(1, result.Last().Crops.Count()); - Assert.AreEqual("one", result.Last().Crops.Last().Alias); - Assert.AreEqual(100, result.Last().Crops.Last().Height); - Assert.AreEqual(200, result.Last().Crops.Last().Width); - Assert.NotNull(result.Last().Crops.Last().Coordinates); - Assert.AreEqual(40m, result.Last().Crops.Last().Coordinates.X1); - Assert.AreEqual(20m, result.Last().Crops.Last().Coordinates.X2); - Assert.AreEqual(2m, result.Last().Crops.Last().Coordinates.Y1); - Assert.AreEqual(1m, result.Last().Crops.Last().Coordinates.Y2); - Assert.NotNull(result.Last().Properties); - Assert.AreEqual(1, result.Last().Properties.Count); - Assert.AreEqual("My other alt text", result.Last().Properties["altText"]); + ValidateMedia(last, "My other media", "my-other-media", ".png", 800, 600, 200); + ValidateFocalPoint(last.FocalPoint, .8m, .6m); + Assert.NotNull(last.Crops); + Assert.AreEqual(1, last.Crops.Count()); + ValidateCrop(last.Crops.First(), "one", 200, 100, 40m, 20m, 2m, 1m); + Assert.NotNull(last.Properties); + Assert.AreEqual(1, last.Properties.Count); + Assert.AreEqual("My other alt text", last.Properties["altText"]); + } + + [Test] + public void MediaPickerWithCropsValueConverter_MergesMediaCropsWithLocalCrops() + { + var publishedPropertyType = SetupMediaPropertyType(false); + var mediaCrops = new ImageCropperValue + { + Crops = new[] + { + new ImageCropperValue.ImageCropperCrop + { + Alias = "mediaOne", + Width = 111, + Height = 222, + Coordinates = new ImageCropperValue.ImageCropperCropCoordinates { X1 = 2m, X2 = 4m, Y1 = 20m, Y2 = 40m } + } + }, + FocalPoint = new ImageCropperValue.ImageCropperFocalPoint { Left = .9m, Top = .1m } + }; + var mediaKey = SetupMedia("Some media", ".123", 123, 456, "My alt text", 789, mediaCrops); + + var serializer = new JsonNetSerializer(); + + var valueConverter = MediaPickerWithCropsValueConverter(); + Assert.AreEqual(typeof(IEnumerable), valueConverter.GetDeliveryApiPropertyValueType(publishedPropertyType)); + + var inter = serializer.Serialize(new[] + { + new MediaPicker3PropertyEditor.MediaPicker3PropertyValueEditor.MediaWithCropsDto + { + Key = Guid.NewGuid(), + MediaKey = mediaKey, + Crops = new [] + { + new ImageCropperValue.ImageCropperCrop + { + Alias = "one", + Coordinates = new ImageCropperValue.ImageCropperCropCoordinates { X1 = 1m, X2 = 2m, Y1 = 10m, Y2 = 20m } + } + } + } + }); + + var result = valueConverter.ConvertIntermediateToDeliveryApiObject(Mock.Of(), publishedPropertyType, PropertyCacheLevel.Element, inter, false) as IEnumerable; + Assert.NotNull(result); + Assert.AreEqual(1, result.Count()); + var mediaWithCrops = result.Single(); + ValidateMedia(mediaWithCrops, "Some media", "some-media", ".123", 123, 456, 789); + + // no local focal point, should revert to media focal point + ValidateFocalPoint(mediaWithCrops.FocalPoint, .9m, .1m); + + // media crops should be merged with local crops + Assert.NotNull(mediaWithCrops.Crops); + Assert.AreEqual(2, mediaWithCrops.Crops.Count()); + + // local crops should be first, media crops should be last + ValidateCrop(mediaWithCrops.Crops.First(), "one", 200, 100, 1m, 2m, 10m, 20m); + ValidateCrop(mediaWithCrops.Crops.Last(), "mediaOne", 111, 222, 2m, 4m, 20m, 40m); + } + + + [Test] + public void MediaPickerWithCropsValueConverter_LocalCropsAndFocalPointTakesPrecedenceOverMediaCropsAndFocalPoint() + { + var publishedPropertyType = SetupMediaPropertyType(false); + var mediaCrops = new ImageCropperValue + { + Crops = new[] + { + new ImageCropperValue.ImageCropperCrop + { + Alias = "one", + Width = 111, + Height = 222, + Coordinates = new ImageCropperValue.ImageCropperCropCoordinates { X1 = 2m, X2 = 4m, Y1 = 20m, Y2 = 40m } + } + }, + FocalPoint = new ImageCropperValue.ImageCropperFocalPoint { Left = .9m, Top = .1m } + }; + var mediaKey = SetupMedia("Some media", ".123", 123, 456, "My alt text", 789, mediaCrops); + + var serializer = new JsonNetSerializer(); + + var valueConverter = MediaPickerWithCropsValueConverter(); + Assert.AreEqual(typeof(IEnumerable), valueConverter.GetDeliveryApiPropertyValueType(publishedPropertyType)); + + var inter = serializer.Serialize(new[] + { + new MediaPicker3PropertyEditor.MediaPicker3PropertyValueEditor.MediaWithCropsDto + { + Key = Guid.NewGuid(), + MediaKey = mediaKey, + Crops = new [] + { + new ImageCropperValue.ImageCropperCrop + { + Alias = "one", + Coordinates = new ImageCropperValue.ImageCropperCropCoordinates { X1 = 1m, X2 = 2m, Y1 = 10m, Y2 = 20m } + } + }, + FocalPoint = new ImageCropperValue.ImageCropperFocalPoint { Left = .2m, Top = .3m } + } + }); + + var result = valueConverter.ConvertIntermediateToDeliveryApiObject(Mock.Of(), publishedPropertyType, PropertyCacheLevel.Element, inter, false) as IEnumerable; + Assert.NotNull(result); + Assert.AreEqual(1, result.Count()); + var mediaWithCrops = result.Single(); + ValidateMedia(mediaWithCrops, "Some media", "some-media", ".123", 123, 456, 789); + + // local focal point should take precedence over media focal point + ValidateFocalPoint(mediaWithCrops.FocalPoint, .2m, .3m); + + // media crops should be discarded when merging with local crops (matching aliases, local ones take precedence) + Assert.NotNull(mediaWithCrops.Crops); + Assert.AreEqual(1, mediaWithCrops.Crops.Count()); + + // local crops should be first, media crops should be last + ValidateCrop(mediaWithCrops.Crops.First(), "one", 200, 100, 1m, 2m, 10m, 20m); } [TestCase("")] @@ -194,8 +271,6 @@ public class MediaPickerWithCropsValueConverterTests : PropertyValueConverterTes { var publishedPropertyType = SetupMediaPropertyType(false); - var serializer = new JsonNetSerializer(); - var valueConverter = MediaPickerWithCropsValueConverter(); var result = valueConverter.ConvertIntermediateToDeliveryApiObject(Mock.Of(), publishedPropertyType, PropertyCacheLevel.Element, inter, false) as IEnumerable; @@ -211,8 +286,6 @@ public class MediaPickerWithCropsValueConverterTests : PropertyValueConverterTes { var publishedPropertyType = SetupMediaPropertyType(true); - var serializer = new JsonNetSerializer(); - var valueConverter = MediaPickerWithCropsValueConverter(); var result = valueConverter.ConvertIntermediateToDeliveryApiObject(Mock.Of(), publishedPropertyType, PropertyCacheLevel.Element, inter, false) as IEnumerable; @@ -241,7 +314,7 @@ public class MediaPickerWithCropsValueConverterTests : PropertyValueConverterTes return publishedPropertyType.Object; } - private Guid SetupMedia(string name, string extension, int width, int height, string altText, int bytes) + private Guid SetupMedia(string name, string extension, int width, int height, string altText, int bytes, ImageCropperValue? imageCropperValue = null) { var publishedMediaType = new Mock(); publishedMediaType.SetupGet(c => c.ItemType).Returns(PublishedItemType.Media); @@ -266,6 +339,7 @@ public class MediaPickerWithCropsValueConverterTests : PropertyValueConverterTes AddProperty(Constants.Conventions.Media.Width, width); AddProperty(Constants.Conventions.Media.Height, height); AddProperty(Constants.Conventions.Media.Bytes, bytes); + AddProperty(Constants.Conventions.Media.File, imageCropperValue); AddProperty("altText", altText); PublishedMediaCacheMock @@ -281,4 +355,49 @@ public class MediaPickerWithCropsValueConverterTests : PropertyValueConverterTes return mediaKey; } + + private void ValidateMedia( + ApiMediaWithCrops actual, + string expectedName, + string expectedUrl, + string expectedExtension, + int expectedWidth, + int expectedHeight, + int expectedBytes) + { + Assert.AreEqual(expectedName, actual.Name); + Assert.AreEqual(expectedUrl, actual.Url); + Assert.AreEqual(expectedExtension, actual.Extension); + Assert.AreEqual(expectedWidth, actual.Width); + Assert.AreEqual(expectedHeight, actual.Height); + Assert.AreEqual(expectedBytes, actual.Bytes); + + } + + private void ValidateFocalPoint(ImageCropperValue.ImageCropperFocalPoint? actual, decimal expectedLeft, decimal expectedTop) + { + Assert.NotNull(actual); + Assert.AreEqual(expectedLeft, actual.Left); + Assert.AreEqual(expectedTop, actual.Top); + } + + private void ValidateCrop( + ImageCropperValue.ImageCropperCrop actual, + string expectedAlias, + int expectedWidth, + int expectedHeight, + decimal expectedX1, + decimal expectedX2, + decimal expectedY1, + decimal expectedY2) + { + Assert.AreEqual(expectedAlias, actual.Alias); + Assert.AreEqual(expectedWidth, actual.Width); + Assert.AreEqual(expectedHeight, actual.Height); + Assert.NotNull(actual.Coordinates); + Assert.AreEqual(expectedX1, actual.Coordinates.X1); + Assert.AreEqual(expectedX2, actual.Coordinates.X2); + Assert.AreEqual(expectedY1, actual.Coordinates.Y1); + Assert.AreEqual(expectedY2, actual.Coordinates.Y2); + } }