Fix GoogleMerchantCenterFeedService to make use of multiple value property extractors (#29)

* Add comments,  rename variables and params

* change some wordings

* Rework GoogleMerchantCenterFeedService to actually make use of IMultipleValuePropertyExtractor

* refactor how we're adding a new xml node

* obsolete extension method and give a heads up about its namespace changing.

Author: umbracotrd

src/Umbraco.Commerce.ProductFeeds.Core/Extensions/XmlElementExtensions.cs

@@ -0,0 +1,28 @@
+using System.Xml;
+
+namespace Umbraco.Commerce.ProductFeeds.Core.Extensions
+{
+    internal static class XmlElementExtensions
+    {
+        /// <summary>
+        /// Add a new child node at the bottom of the list of child nodes, of this node, only when the child node inner text has value.
+        /// </summary>
+        /// <param name="element"></param>
+        /// <param name="childNodeName"></param>
+        /// <param name="childNodeInnerText"></param>
+        /// <param name="xmlNameSpace"></param>
+        /// <returns>The added child node or null.</returns>
+        public static XmlElement? AddChild(this XmlElement element, string childNodeName, string childNodeInnerText, string? xmlNameSpace = null)
+        {
+            if (!string.IsNullOrWhiteSpace(childNodeInnerText))
+            {
+                XmlElement childNode = element.OwnerDocument.CreateElement(childNodeName, xmlNameSpace);
+                childNode.InnerText = childNodeInnerText;
+                element.AppendChild(childNode);
+                return childNode;
+            }
+
+            return null;
+        }
+    }
+}

src/Umbraco.Commerce.ProductFeeds.Core/Features/FeedGenerators/Implementations/GoogleMerchantCenterFeedService.cs

@@ -8,16 +8,19 @@
 using Umbraco.Commerce.Core.Models;
 using Umbraco.Commerce.Core.Services;
 using Umbraco.Commerce.Extensions;
+using Umbraco.Commerce.ProductFeeds.Core.Extensions;
 using Umbraco.Commerce.ProductFeeds.Core.Features.FeedGenerators.Implementations;
 using Umbraco.Commerce.ProductFeeds.Core.Features.FeedSettings.Application;
-using Umbraco.Commerce.ProductFeeds.Core.Features.PropertyValueExtractors.Implementations;
 using Umbraco.Commerce.ProductFeeds.Core.FeedGenerators.Application;
 using Umbraco.Commerce.ProductFeeds.Core.ProductQueries.Application;
 using Umbraco.Commerce.ProductFeeds.Core.PropertyValueExtractors.Application;
 using Umbraco.Commerce.ProductFeeds.Extensions;
 
 namespace Umbraco.Commerce.ProductFeeds.Core.FeedGenerators.Implementations
 {
+    /// <summary>
+    /// This is the feed generator that follows Google Merchant Center's standard.
+    /// </summary>
     public class GoogleMerchantCenterFeedService : IProductFeedGeneratorService
     {
         private const string GoogleXmlNamespaceUri = "http://base.google.com/ns/1.0";
@@ -50,6 +53,12 @@ public GoogleMerchantCenterFeedService(
             _multipleValuePropertyExtractorFactory = multipleValuePropertyExtractorFactory;
         }
 
+        /// <summary>
+        /// Generate the product feed following the inputted settings.
+        /// </summary>
+        /// <param name="feedSetting"></param>
+        /// <returns></returns>
+        /// <exception cref="IdPropertyNodeMappingNotFoundException"></exception>
         public XmlDocument GenerateFeed(ProductFeedSettingReadModel feedSetting)
         {
             ArgumentNullException.ThrowIfNull(feedSetting, nameof(feedSetting));
@@ -65,14 +74,10 @@ public XmlDocument GenerateFeed(ProductFeedSettingReadModel feedSetting)
             XmlElement channel = doc.CreateElement("channel");
 
             // doc/channel/title
-            XmlElement titleNode = channel.OwnerDocument.CreateElement("title");
-            titleNode.InnerText = feedSetting.FeedName;
-            channel.AppendChild(titleNode);
+            channel.AddChild("title", feedSetting.FeedName);
 
             // doc/channel/description
-            XmlElement descriptionNode = channel.OwnerDocument.CreateElement("description");
-            descriptionNode.InnerText = feedSetting.FeedDescription;
-            channel.AppendChild(descriptionNode);
+            channel.AddChild("description", feedSetting.FeedDescription);
             root.AppendChild(channel);
 
             ICollection<IPublishedContent> products = _productQueryService.GetPublishedProducts(new GetPublishedProductsParams
@@ -142,9 +147,9 @@ public XmlDocument GenerateFeed(ProductFeedSettingReadModel feedSetting)
         /// Create a new node for the one, in this case, each product/variant is one &lt;item&gt; node.
         /// </summary>
         /// <param name="feedSetting"></param>
-        /// <param name="channel"></param>
-        /// <param name="variant"></param>
-        /// <param name="mainProduct"></param>
+        /// <param name="channel">The XML &lt;channel&gt; node that holds the &lt;item&gt; node.</param>
+        /// <param name="variant">Product variant.</param>
+        /// <param name="mainProduct">Main product which may have multiple variants. Some common properties can only be found in the main product, not in a variant.</param>
         /// <returns></returns>
         private XmlElement NewItemNode(ProductFeedSettingReadModel feedSetting, XmlElement channel, IPublishedElement variant, IPublishedElement? mainProduct)
         {
@@ -153,21 +158,32 @@ private XmlElement NewItemNode(ProductFeedSettingReadModel feedSetting, XmlEleme
             // add custom properties
             foreach (PropertyAndNodeMapItem map in feedSetting.PropertyNameMappings)
             {
-                if (map.ValueExtractorName == nameof(DefaultMultipleMediaPickerPropertyValueExtractor))
+                if (_singleValuePropertyExtractorFactory.TryGetExtractor(map.ValueExtractorId, out ISingleValuePropertyExtractor? singleValueExtractor)
+                    && singleValueExtractor != null)
                 {
-                    AddImageNodes(itemNode, map.ValueExtractorName, map.PropertyAlias, variant, mainProduct);
+                    string propValue = singleValueExtractor.Extract(variant, map.PropertyAlias, mainProduct);
+                    itemNode.AddChild(map.NodeName, propValue, GoogleXmlNamespaceUri);
                 }
-                else
+                else if (_multipleValuePropertyExtractorFactory.TryGetExtractor(map.ValueExtractorId!, out IMultipleValuePropertyExtractor? multipleValueExtractor)
+                    && multipleValueExtractor != null)
                 {
-                    ISingleValuePropertyExtractor valueExtractor = _singleValuePropertyExtractorFactory.GetExtractor(map.ValueExtractorName);
-                    string propValue = valueExtractor.Extract(variant, map.PropertyAlias, mainProduct);
-                    if (!string.IsNullOrWhiteSpace(propValue))
+                    var values = multipleValueExtractor.Extract(variant, map.PropertyAlias, mainProduct).ToList();
+                    if (map.NodeName.Equals("g:image_link", StringComparison.OrdinalIgnoreCase))
+                    {
+                        AddImageNodes(itemNode, values);
+                    }
+                    else
                     {
-                        XmlElement propertyNode = itemNode.OwnerDocument.CreateElement(map.NodeName, GoogleXmlNamespaceUri);
-                        propertyNode.InnerText = propValue;
-                        itemNode.AppendChild(propertyNode);
+                        foreach (string value in values)
+                        {
+                            itemNode.AddChild(map.NodeName, value, GoogleXmlNamespaceUri);
+                        }
                     }
                 }
+                else
+                {
+                    throw new InvalidOperationException($"Unable to locate property value extractor with id = '{map.ValueExtractorId}'");
+                }
             }
 
             if (mainProduct == null) // when the variant is the main product itself
@@ -181,27 +197,23 @@ private XmlElement NewItemNode(ProductFeedSettingReadModel feedSetting, XmlEleme
 
         private static void AddItemGroupNode(XmlElement itemNode, string groupId)
         {
-            XmlElement availabilityNode = itemNode.OwnerDocument.CreateElement("g:item_group_id", GoogleXmlNamespaceUri);
-            availabilityNode.InnerText = groupId;
-            itemNode.AppendChild(availabilityNode);
+            itemNode.AddChild("g:item_group_id", groupId, GoogleXmlNamespaceUri);
         }
 
         /// <summary>
         /// Add a &lt;url&gt; node under the provided &lt;item&gt; node.
         /// </summary>
-        /// <param name="itemNode"></param>
+        /// <param name="itemNode">The XML item node that represents the current product.</param>
         /// <param name="product"></param>
         private static void AddUrlNode(XmlElement itemNode, IPublishedContent product)
         {
-            XmlElement linkNode = itemNode.OwnerDocument.CreateElement("g:link", GoogleXmlNamespaceUri);
-            linkNode.InnerText = product.Url(mode: UrlMode.Absolute);
-            itemNode.AppendChild(linkNode);
+            itemNode.AddChild("g:link", product.Url(mode: UrlMode.Absolute), GoogleXmlNamespaceUri);
         }
 
         /// <summary>
         /// Add a &lt;price&gt; node under the provided &lt;item&gt; node.
         /// </summary>
-        /// <param name="itemNode"></param>
+        /// <param name="itemNode">The XML item node that represents the current product.</param>
         /// <param name="feedSetting"></param>
         /// <param name="product"></param>
         /// <param name="complexVariant"></param>
@@ -223,38 +235,28 @@ private void AddPriceNode(XmlElement itemNode, ProductFeedSettingReadModel feedS
             Attempt<Price> calculatePriceAttempt = productSnapshot.TryCalculatePrice();
             Price calculatedPrice = calculatePriceAttempt.Success ? calculatePriceAttempt.Result! : throw new NotImplementedException("Failed to calculate the price");
             decimal priceForShow = feedSetting.IncludeTaxInPrice ? calculatedPrice.WithTax : calculatedPrice.WithoutTax;
-            priceNode.InnerText = $"{priceForShow.ToString("0.00", CultureInfo.InvariantCulture)} {_currencyService.GetCurrency(calculatedPrice.CurrencyId).Code}";
-            itemNode.AppendChild(priceNode);
+            string formattedPrice = $"{priceForShow.ToString("0.00", CultureInfo.InvariantCulture)} {_currencyService.GetCurrency(calculatedPrice.CurrencyId).Code}";
+            itemNode.AddChild("g:price", formattedPrice, GoogleXmlNamespaceUri);
         }
 
         /// <summary>
-        /// Add image nodes under the provided &lt;item&gt; node.
+        /// This method adds appropriate image nodes under the provided &lt;item&gt; node. It exists because Google Merchant Center treats multiple product images abnormally.
         /// </summary>
-        /// <param name="itemNode"></param>
-        /// <param name="valueExtractorName"></param>
-        /// <param name="propertyAlias"></param>
-        /// <param name="product"></param>
-        /// <param name="mainProduct"></param>
-        private void AddImageNodes(XmlElement itemNode, string valueExtractorName, string propertyAlias, IPublishedElement product, IPublishedElement? mainProduct)
+        /// <param name="itemNode">The XML item node that represents the current product.</param>
+        /// <param name="imageUrls"></param>
+        private static void AddImageNodes(XmlElement itemNode, List<string> imageUrls)
         {
-            IMultipleValuePropertyExtractor multipleValuePropertyExtractor = _multipleValuePropertyExtractorFactory.GetExtractor(valueExtractorName);
-            var imageUrls = multipleValuePropertyExtractor.Extract(product, propertyAlias, mainProduct).ToList();
-            if (imageUrls.Count <= 0)
+            if (imageUrls.Count == 0)
             {
                 return;
             }
 
-            XmlElement imageNode = itemNode.OwnerDocument.CreateElement("g:image_link", GoogleXmlNamespaceUri);
-            imageNode.InnerText = imageUrls.First();
-            itemNode.AppendChild(imageNode);
-
+            itemNode.AddChild("g:image_link", imageUrls.First(), GoogleXmlNamespaceUri);
             if (imageUrls.Count > 1)
             {
                 for (int i = 1; i < imageUrls.Count; i++)
                 {
-                    XmlElement additionalImageNode = itemNode.OwnerDocument.CreateElement("g:additional_image_link", GoogleXmlNamespaceUri);
-                    additionalImageNode.InnerText = imageUrls[i];
-                    itemNode.AppendChild(additionalImageNode);
+                    itemNode.AddChild("g:additional_image_link", imageUrls[i], GoogleXmlNamespaceUri);
                 }
             }
         }

src/Umbraco.Commerce.ProductFeeds.Core/Features/FeedSettings/Application/PropertyAndNodeMapItem.cs

@@ -15,6 +15,12 @@ public class PropertyAndNodeMapItem
         /// <summary>
         /// Property value extractor id.
         /// </summary>
+        [Obsolete("Will be removed in v15. Use ValueExtractorId instead.")]
         public string? ValueExtractorName { get; set; }
+
+        /// <summary>
+        /// Property value extractor id.
+        /// </summary>
+        public string? ValueExtractorId => ValueExtractorName;
     }
 }

src/Umbraco.Commerce.ProductFeeds.Core/Features/PropertyValueExtractors/Application/IMultipleValuePropertyExtractor.cs

@@ -7,16 +7,22 @@ namespace Umbraco.Commerce.ProductFeeds.Core.PropertyValueExtractors.Application
     /// </summary>
     public interface IMultipleValuePropertyExtractor
     {
+        /// <summary>
+        /// The unique id of this extractor.
+        /// </summary>
         string Id { get; }
 
+        /// <summary>
+        /// The user friendly name of this extractor.
+        /// </summary>
         string DisplayName { get; }
 
         /// <summary>
-        /// Get a list of absolute url of the media.
+        /// Extracts the value from the property and returns them as a collection.
         /// </summary>
-        /// <param name="content"></param>
+        /// <param name="content">The umbraco content that holds the property.</param>
         /// <param name="propertyAlias"></param>
-        /// <param name="fallbackElement"></param>
+        /// <param name="fallbackElement">If the property alias can't be found on the main content, this method should try looking for it on the fallback content.</param>
         /// <returns></returns>
         ICollection<string> Extract(IPublishedElement content, string propertyAlias, IPublishedElement? fallbackElement);
     }

src/Umbraco.Commerce.ProductFeeds.Core/Features/PropertyValueExtractors/Application/IMultipleValuePropertyExtractorFactory.cs

@@ -5,8 +5,17 @@ public interface IMultipleValuePropertyExtractorFactory
         /// <summary>
         /// Get the property extractor.
         /// </summary>
-        /// <param name="uniqueExtractorName"></param>
+        /// <param name="valueExtractorId"></param>
         /// <returns></returns>
-        IMultipleValuePropertyExtractor GetExtractor(string uniqueExtractorName);
+        [Obsolete("Will be removed in v15. Use TryGetExtractor instead.")]
+        IMultipleValuePropertyExtractor GetExtractor(string valueExtractorId);
+
+        /// <summary>
+        /// Try to get the property extractor. Returns the default extractor implementation if no extractor id is provided.
+        /// </summary>
+        /// <param name="valueExtractorId"></param>
+        /// <param name="valueExtractor">The located value extractor. It can be null if no extractor was found.</param>
+        /// <returns>True if an extractor was found, otherwise False.</returns>
+        bool TryGetExtractor(string valueExtractorId, out IMultipleValuePropertyExtractor? valueExtractor) => throw new NotImplementedException();
     }
 }

src/Umbraco.Commerce.ProductFeeds.Core/Features/PropertyValueExtractors/Application/ISingleValuePropertyExtractor.cs

@@ -7,16 +7,22 @@ namespace Umbraco.Commerce.ProductFeeds.Core.PropertyValueExtractors.Application
     /// </summary>
     public interface ISingleValuePropertyExtractor
     {
+        /// <summary>
+        /// Returns the value extractor id. Must be unique.
+        /// </summary>
         public string Id { get; }
 
+        /// <summary>
+        /// Returns a user friendly name of the value extractor.
+        /// </summary>
         public string DisplayName { get; }
 
         /// <summary>
-        /// Get property value using property alias.
+        /// Gets property value using property alias.
         /// </summary>
         /// <param name="content"></param>
         /// <param name="propertyAlias"></param>
-        /// <param name="fallbackElement">Store fallback value of the property.</param>
+        /// <param name="fallbackElement">Stores fallback value of the property.</param>
         /// <returns></returns>
         string Extract(IPublishedElement content, string propertyAlias, IPublishedElement? fallbackElement);
     }

src/Umbraco.Commerce.ProductFeeds.Core/Features/PropertyValueExtractors/Application/ISingleValuePropertyExtractorFactory.cs

@@ -3,10 +3,19 @@ namespace Umbraco.Commerce.ProductFeeds.Core.PropertyValueExtractors.Application
     public interface ISingleValuePropertyExtractorFactory
     {
         /// <summary>
-        /// Get the property extractor. Returns the default extractor implementation if no extractor name is provided.
+        /// Get the property extractor. Returns the default extractor implementation if no extractor id is provided.
         /// </summary>
         /// <param name="extractorId"></param>
         /// <returns></returns>
+        [Obsolete("Will be removed in v15. Use TryGetExtractor instead.")]
         ISingleValuePropertyExtractor GetExtractor(string? extractorId = null);
+
+        /// <summary>
+        /// Try to get the property extractor. Returns the default extractor implementation if no extractor id is provided.
+        /// </summary>
+        /// <param name="extractorId"></param>
+        /// <param name="extractor">The located value extractor. It can be null if no extractor was found.</param>
+        /// <returns>True if an extractor was found, otherwise False.</returns>
+        bool TryGetExtractor(string? extractorId, out ISingleValuePropertyExtractor? extractor) => throw new NotImplementedException();
     }
 }

src/Umbraco.Commerce.ProductFeeds.Core/Features/PropertyValueExtractors/Implementations/MultipleValuePropertyExtractorFactory.cs

@@ -12,18 +12,30 @@ public MultipleValuePropertyExtractorFactory(MultipleValuePropertyExtractorColle
         }
 
         /// <inheritdoc/>
-        public IMultipleValuePropertyExtractor GetExtractor(string uniqueExtractorName)
+        [Obsolete("Will be removed in v15. Use TryGetExtractor instead.")]
+        public IMultipleValuePropertyExtractor GetExtractor(string valueExtractorId)
         {
-            if (string.IsNullOrWhiteSpace(uniqueExtractorName))
+            if (string.IsNullOrWhiteSpace(valueExtractorId))
             {
-                throw new ArgumentNullException(nameof(uniqueExtractorName));
+                throw new ArgumentNullException(nameof(valueExtractorId));
             }
 
-
-            IMultipleValuePropertyExtractor? valueExtractor = _valueExtractors.FirstOrDefault(x => x.Id == uniqueExtractorName)
-                ?? throw new InvalidOperationException($"Can't find property extractor with name '{uniqueExtractorName}'");
+            IMultipleValuePropertyExtractor? valueExtractor = _valueExtractors.FirstOrDefault(x => x.Id == valueExtractorId)
+                ?? throw new InvalidOperationException($"Can't find property extractor with id '{valueExtractorId}'");
 
             return valueExtractor;
         }
+
+        /// <inheritdoc/>
+        public bool TryGetExtractor(string valueExtractorId, out IMultipleValuePropertyExtractor? valueExtractor)
+        {
+            if (string.IsNullOrWhiteSpace(valueExtractorId))
+            {
+                throw new ArgumentNullException(nameof(valueExtractorId));
+            }
+
+            valueExtractor = _valueExtractors.FirstOrDefault(x => x.Id == valueExtractorId);
+            return valueExtractor != null ? true : false;
+        }
     }
 }