umbraco / Umbraco.Commerce.Issues

18 stars 2 forks source link

Events and pipeline tasks registered on `IUmbracoCommerceBuilder` doesn't execute #475

Closed bjarnef closed 7 months ago

bjarnef commented 7 months ago

Describe the bug I have used the example for demo store to register events and pipeline tasks. https://github.com/umbraco/Umbraco.Commerce.DemoStore/blob/e0c09fbf1307d9e08b40205886553b7a065806d8/src/Umbraco.Commerce.DemoStore/DemoStoreBuilderExtensions.cs#L14

However it doesn't seem these executes, it hit the IUmbracoBuilder extension method, but doesn't hit the events / pipeline task registered here.

public static class StoreBuilderExtensions
{
    public static IUmbracoBuilder AddMyStore(this IUmbracoBuilder umbracoBuilder)
    {
        umbracoBuilder.AddUmbracoCommerce(v =>
        {
            // Enable SQLite support
            //v.AddSQLite();

            // Replace the umbraco product name extractor with one that supports child variants
            //v.Services.AddUnique<IUmbracoProductNameExtractor, CompositeProductNameExtractor>();

            // Register event handlers
            v.WithNotificationEvent<OrderProductAddingNotification>()
                .RegisterHandler<OrderProductAddingHandler>();

            v.WithNotificationEvent<OrderLineChangingNotification>()
                .RegisterHandler<OrderLineChangingHandler>();

            v.WithNotificationEvent<OrderLineRemovingNotification>()
                .RegisterHandler<OrderLineRemovingHandler>();

            v.WithNotificationEvent<OrderPaymentCountryRegionChangingNotification>()
                .RegisterHandler<OrderPaymentCountryRegionChangingHandler>();

            v.WithNotificationEvent<OrderShippingCountryRegionChangingNotification>()
                .RegisterHandler<OrderShippingCountryRegionChangingHandler>();

            v.WithNotificationEvent<OrderShippingMethodChangingNotification>()
                .RegisterHandler<OrderShippingMethodChangingHandler>();

            v.WithCalculateOrderPipeline()
                .Add<ShoppingBagOrderCalculationPipelineTask>();
                //.InsertAfter<CalculateOrderSubtotalPriceWithoutAdjustmentsTask, ShoppingBagOrderCalculationPipelineTask>();
        });

        return umbracoBuilder;
    }
}
public class SiteComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        // Add services, notifications, etc.
        builder.Services.AddTransient<ISiteSettings, SiteSettings>();
        builder.Services.AddTransient<INewsletterSignup, CampaignMonitorSignup>();
        builder.Services.AddTransient<IWeatherService, WeatherService>();

        builder.AddMyStore();
    }
}

I tried directly in composer, but same behaviour:

public class SiteComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.AddUmbracoCommerce(v =>
                {
                    v.WithCalculateOrderPipeline()
                        .Add<ShoppingBagOrderCalculationPipelineTask>();
                });
    }
}

However if I use this instead it works:

public class SiteComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.WithCalculateOrderPipeline()
            .Add<ShoppingBagOrderCalculationPipelineTask>();
    }
}

These are the obsolete extension methods though.

image

Expected behavior The events / pipeline tasks should execute.

Screenshots If applicable, add screenshots to help explain your problem.

Additional context Are you using any Umbraco Commerce add-on packages such as Umbraco.Commerce.Checkout? or is there any other contextual information that might help.

Umbraco version: 13.2.0

Umbraco Commerce version: 13.1.1

Umbraco Commerce Deploy version: 13.1.1

bjarnef commented 7 months ago

The following almost work, but it may be too late to add the product as the orderline isn't persisted and with _commerceApi.SaveOrder(writableOrder) it fails with concurrent save.

public class SiteComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        //builder.AddUmbracoCommerce(v =>
        //{
        //  v.WithCalculateOrderPipeline()
        //      .Add<ShoppingBagOrderCalculationPipelineTask>();
        //});

        builder.WithCalculateOrderPipeline()
            .Add<ShoppingBagOrderCalculationPipelineTask>();
    }
}
public class ShoppingBagOrderCalculationPipelineTask : PipelineTaskWithTypedArgsBase<OrderCalculationPipelineArgs, OrderCalculation>
{
    private readonly IUmbracoCommerceApi _commerceApi;
    private readonly ILogger<ShoppingBagOrderCalculationPipelineTask> _logger;

    public ShoppingBagOrderCalculationPipelineTask(IUmbracoCommerceApi commerceApi, ILogger<ShoppingBagOrderCalculationPipelineTask> logger)
    {
        _commerceApi = commerceApi;
        _logger = logger;
    }

    public override PipelineResult<OrderCalculation> Execute(OrderCalculationPipelineArgs args)
    {
        try
        {
            _commerceApi.Uow.Execute(uow =>
            {
                var writableOrder = args.Order.AsWritable(uow);

                if (writableOrder.OrderLines.Count > 0)
                {
                    if (!writableOrder.OrderLines.Any(x => x.Sku == Configuration.ShopBagSku))
                    {
                        writableOrder?.AddProduct(Configuration.ShopBagReference.ToString(), 1);
                    }
                }
                else
                {
                    var line = writableOrder.OrderLines.FirstOrDefault(x => x.Sku == Configuration.ShopBagSku);

                    if (line != null)
                    {
                        writableOrder.RemoveOrderLine(line);
                    }
                }

                //_commerceApi.SaveOrder(writableOrder);

                uow.Complete();
            });
        }
        catch (Exception ex)
        {
            _logger.LogError(ex, "Failed to add shopping bag");
        }

        return Ok(args.Model);
    }
}

Alternative it can hook in e.g. OrderLineChangingNotification / OrderLineChangedNotification notification to detect with an orderline it added / removed.

bjarnef commented 7 months ago

@mattbrailsford do you know why this happens?

What's the actually difference between registered an notification event / pipeline task on IUmbracoBuilder and IUmbracoCommerceBuilder since the former works.

Has it forgotten to register the builder perhaps or not included in latest build? 🤔

mattbrailsford commented 7 months ago

If you are using composers, you'll need to make sure your composer runs before the UmbracoCommerceComposer, otherwise a default config will get applied.

bjarnef commented 7 months ago

@mattbrailsford okay, that makes sense.

Yes, I think this was how it worked in until v12: https://github.com/umbraco/Umbraco.Commerce.DemoStore/blob/e0c09fbf1307d9e08b40205886553b7a065806d8/src/Umbraco.Commerce.DemoStore.Web/Startup.cs#L48

But with Umbraco 13+ using minimal hosting model it is recommended using a composer.

I think it wasn't clear in the docs here: https://docs.umbraco.com/umbraco-commerce/key-concepts/umbraco-commerce-builder

[ComposeBefore(typeof(UmbracoCommerceComposer))]
public class StoreComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.AddMyStore();
    }
}

Updated docs: https://github.com/umbraco/UmbracoDocs/pull/5961

bjarnef commented 7 months ago

With the change, I got this error

v.WithCalculateOrderPipeline()
    .InsertAfter<CalculateOrderSubtotalPriceWithoutAdjustmentsTask, ShoppingBagOrderCalculationPipelineTask>();

image

while this work though:

v.WithCalculateOrderPipeline()
    .Add<ShoppingBagOrderCalculationPipelineTask>();

but it makes sense if the UmbracoCommerceComposer register CalculateOrderSubtotalPriceWithoutAdjustmentsTask and this isn't registered yet and the composer run before UmbracoCommerceComposer.

bjarnef commented 7 months ago

How would we handle InsertBefore<TTask>() or InsertAfter<TTask>() in this scenario? https://docs.umbraco.com/umbraco-commerce/key-concepts/pipelines

bjarnef commented 7 months ago

@mattbrailsford do you have a similar example to this in Vendr? https://our.umbraco.com/packages/website-utilities/vendr/vendr-support/102159-add-custom-fee-to-order

I tried something similar in Umbraco Commerce, but couldn't make it persisting the order line changes - or just setting an order property.

mattbrailsford commented 7 months ago

You'd be better off using Price Adjustments https://docs.umbraco.com/umbraco-commerce/key-concepts/price-amount-adjustments

bjarnef commented 7 months ago

That may work as well. Actually we don't really need to check on pricing for the "Shop bag" feature - just if there's an orderline or not. Much like in a physical store, where one can choose to get a (plastic/paper) bag - here is is just not an option :)

I guess we could hook into OrderLineChangingNotification or OrderLineChangedNotification if it need to happens after orderline has been updated.

If we want to ensure only 1 bag is added, we could implement something like this: https://docs.umbraco.com/umbraco-commerce/how-to-guides/limit-orderline-quantity

bjarnef commented 7 months ago

Seems like an notification handler in OrderLineChangingNotification works for this specific case, where the shopping bag exists as product in the store and added as orderline.

public class ShoppingBagOrderLineHandler : NotificationEventHandlerBase<OrderLineChangingNotification>
{
    private readonly IUmbracoCommerceApi _commerceApi;
    private readonly ILogger<ShoppingBagOrderLineHandler> _logger;

    public ShoppingBagOrderLineHandler(IUmbracoCommerceApi commerceApi, ILogger<ShoppingBagOrderLineHandler> logger)
    {
        _commerceApi = commerceApi;
        _logger = logger;
    }

    public override void Handle(OrderLineChangingNotification evt)
    {
        if (!evt.Order.IsFinalized)
        {
            try
            {
                _commerceApi.Uow.Execute(uow =>
                {
                    var writableOrder = evt.Order.AsWritable(uow);

                    if (writableOrder.OrderLines.Count > 0)
                    {
                        if (!writableOrder.OrderLines.Any(x => x.Sku == Configuration.ShopBagSku))
                        {
                            writableOrder?.AddProduct(Configuration.ShopBagReference.ToString(), 1);
                        }
                    }
                    else
                    {
                        var line = writableOrder.OrderLines.FirstOrDefault(x => x.Sku == Configuration.ShopBagSku);

                        if (line != null)
                        {
                            writableOrder.RemoveOrderLine(line);
                        }
                    }

                    //_commerceApi.SaveOrder(writableOrder);

                    uow.Complete();
                });
            }
            catch (Exception ex)
            {
                _logger.LogError(ex, "Failed to add shopping bag");
            }
        }
    }
}

I guess there could be benefits using the price adjustments, but I wonder if these can be listed as orderlines - the order model returned to view or as JSON from API could possible simulate this in basket although not as orderline in cart/order.

It depends on if the bag is considered as a product (which maybe can be added on it's own) or it is considered as a fee. For this specific project probably the former as this is how it works in the current store.

bjarnef commented 7 months ago

I ended up with the following which work when a product is added to cart and orderline is removed, so shopping bag is removed as well in empty basket.

public class ShoppingBagAddingHandler : NotificationEventHandlerBase<OrderProductAddingNotification>
{
    private readonly IUmbracoCommerceApi _commerceApi;
    private readonly ILogger<ShoppingBagAddingHandler> _logger;

    public ShoppingBagAddingHandler(IUmbracoCommerceApi commerceApi, ILogger<ShoppingBagAddingHandler> logger)
    {
        _commerceApi = commerceApi;
        _logger = logger;
    }

    public override void Handle(OrderProductAddingNotification evt)
    {
        if (evt.Order.IsFinalized)
            return;

        try
        {
            _commerceApi.Uow.Execute(uow =>
            {
                var writableOrder = evt.Order.AsWritable(uow);

                if (!writableOrder.OrderLines.Any(x => x.Sku == Configuration.ShopBagSku))
                {
                    writableOrder?.AddProduct(Configuration.ShopBagReference.ToString(), 1);
                }

                uow.Complete();
            });
        }
        catch (Exception ex)
        {
            _logger.LogError(ex, "Failed to add shopping bag");
        }
    }
}

public class ShoppingBagRemovingHandler : NotificationEventHandlerBase<OrderLineRemovingNotification>
{
    private readonly IUmbracoCommerceApi _commerceApi;
    private readonly ILogger<ShoppingBagRemovingHandler> _logger;

    public ShoppingBagRemovingHandler(IUmbracoCommerceApi commerceApi, ILogger<ShoppingBagRemovingHandler> logger)
    {
        _commerceApi = commerceApi;
        _logger = logger;
    }

    public override void Handle(OrderLineRemovingNotification evt)
    {
        if (evt.Order.IsFinalized)
            return;

        try
        {
            _commerceApi.Uow.Execute(uow =>
            {
                var writableOrder = evt.Order.AsWritable(uow);

                if (writableOrder.OrderLines.Count(x => x.Sku != Configuration.ShopBagSku) == 0)
                {
                    var line = writableOrder.OrderLines.FirstOrDefault(x => x.Sku == Configuration.ShopBagSku);

                    if (line != null)
                    {
                        writableOrder.RemoveOrderLine(line);
                    }
                }

                uow.Complete();
            });
        }
        catch (Exception ex)
        {
            _logger.LogError(ex, "Failed to remove shopping bag");
        }
    }
}