umbraco / Umbraco.Commerce.Issues

18 stars 2 forks source link

OrderReadOnly seems to be caching data but not refreshed when a Product is added #449

Open robertjf opened 10 months ago

robertjf commented 10 months ago

We have noticed in testing that adding an item to an Order does not refresh the OrderReadOnly, although the Order is updated succcesfully.

For example:

                commerceApi.Uow.Execute(uow =>
                {
                    var store = CurrentPage.GetStore();
                    var order = commerceApi.GetOrCreateCurrentOrder(store.Id);

                    var writableOrder = order.AsWritable(uow);

                    // order.OrderLines.Count() == 4
                    // writableOrder.OrderLines.Count() == 5

                    writableOrder.AddProduct(postModel.ProductReference, postModel.ProductVariantReference, 1,
                                    new Dictionary<string, string>{
                                        { "imageUrl", product?.RollupImageV2?.MediaUrl() }
                                    });

                    commerceApi.SaveOrder(writableOrder);

                    uow.Complete();
                });

The only way we've gotten the true order line-item count/quantity count, is to restart the website. This is a problem in both the back-office Orders list as well as the front end.

Umbraco.Commerce version: 12.1.2

robertjf commented 10 months ago

We've gotten around this issue in the front end using the following code:

OrderReadOnly? order = null;
        commerceApi.Uow.Execute(uow => {
            var writableOrder = commerceApi.GetCurrentOrder(store.Id)?.AsWritable(uow);
            if (writableOrder is not null) {
                order = writableOrder.AsReadOnly();
            }
        });

while this works, it's not ideal, obviously...

mattbrailsford commented 10 months ago

The read only order is a snapshot in time. It doesn’t auto update when you make it writable and make changes to it. It needs to be re fetched / the writable order converted back to read only.

robertjf commented 10 months ago

@mattbrailsford yes - I get that, but what we're observing is that when the order is written back, the read-only order does not reflect the update.

robertjf commented 10 months ago

i.e., we always use the following to retrieve the Read-only order:

var order = commerceApi.GetOrCreateCurrentOrder(store.Id)

However it's state does not change after subsequent writes and then reads using the above line.

mattbrailsford commented 10 months ago

Can you share a full code block where you are retrieving the order, making a change and the re fetching the order and when within this things are wrong?

robertjf commented 10 months ago

Certainly :)

In the CartController:

        [HttpPost]
        public IActionResult AddToCart(AddToCartDto postModel)
        {
                commerceApi.Uow.Execute(uow =>
                {
                    var store = CurrentPage.GetStore();
                    var order = commerceApi.GetOrCreateCurrentOrder(store.Id);

                    var writableOrder = order.AsWritable(uow);

                    // order.OrderLines.Count() == 4
                    // writableOrder.OrderLines.Count() == 5

                    writableOrder.AddProduct(postModel.ProductReference, postModel.ProductVariantReference, 1,
                                    new Dictionary<string, string>{
                                        { "imageUrl", product?.RollupImageV2?.MediaUrl() }
                                    });

                    commerceApi.SaveOrder(writableOrder);

                    uow.Complete();
                });

            TempData["addedProductReference"] = postModel.ProductReference;

            return RedirectToCurrentUmbracoPage();
        }

Then in the Cart ViewComponent:


    public IViewComponentResult Invoke(StoreReadOnly store, bool badgeOnly = false, bool isModal = true) 
    {
        string view = "default";
        // If asking for all completed tasks, render with the "PVC" view.
        if (badgeOnly)
        {
            view = "badge";
        }

        var vm = new CartViewModel(commerceApi.GetCurrentOrder(store.Id), isModal);
        return View(view, vm);
    }

This is then used in the View:

@inherits UmbracoViewPage<CartViewModel>
@{
    var checkout = Umbraco.GetHomePage().GetCheckoutPage();
}
<!-- Partial: _CartContent -->
<div class="mini-cart-items">
    @if (!Model.Order.OrderLines.Any())
    {
        <div class="mini-cart-item">
            <i>Your cart is empty.</i>
        </div>
    }
    @foreach (var item in Model.Order.OrderLines.Select((ol, i) => new { OrderLine = ol, Index = i }))
    {
...
    }
</div>
<div class="cart-total">
    <span class="subtotal-value">@Model.Order.SubtotalPrice.WithoutAdjustments.Formatted()</span>
</div>
<div class="cart-total">
    <span class="label">Tax &amp; Shipping</span>
    <span class="subtotal-value">TBD</span>
</div>

Note also, that even after calling the AddToCart method on the SurfaceController, the cart is not updated in the Umbraco Backoffice either.

mattbrailsford commented 10 months ago

But the changes are definitely persisted?

robertjf commented 10 months ago

Yes - I can also prove that by restarting the website, and the Read-only Cart has the latest changes. I also inspected the order when adding new items (AddToCart method) and could see the changes each time.

robertjf commented 10 months ago

I wondered if there's something in our code that somehow is caching the cart, but don't see how...

mattbrailsford commented 10 months ago

Yea, the usual reason for things not updating is that you have a uow somewhere that isn't completing, but if that was the case it shouldn't persist at all (hence me checking).

You aren't in a load balanced setup or anything are you? Or is there anything unique about your setup? What OS are you using?

robertjf commented 10 months ago

nope, this is happening on local dev as well as Umbraco Cloud. Local dev is MacOS.

robertjf commented 10 months ago

it was working initially when I used version 12.0.0 - it only seems to have stopped since upgrading to 12.1.1 (and today 12.1.2)

mattbrailsford commented 10 months ago

Ok, let me see what changed since then.

mattbrailsford commented 10 months ago

Hmmm, so I've been through the commit log since v12 and there is nothing I can see that would affect the cache. I've also updated the demo store to v12.1.2 and tested this and changes are persisted and cache updated https://github.com/umbraco/Umbraco.Commerce.DemoStore

If you pull/run the demo store, does this show the same issue?

robertjf commented 10 months ago

Hm. I'll take a look in the morning - about to go to bed... is there any caching at all going on with the readonly cart? I've borrowed some code from the demo commerce store, but other than that it should be pretty standard.

mattbrailsford commented 10 months ago

There is an in memory cache for orders yes, but the uow.Complete() should copy the latest changed back to it. I'd check to see if you have any event handlers at play that could be interfering. Always worth ruling things out.

robertjf commented 10 months ago

@mattbrailsford I'm a little stumped on this one. I've downloaded the Commerce DemoStore and it's working as expected.

I then did a direct copy from the DemoStore to our project for the CartSurfaceController and removed Umbraco.Commerce.Checkout as well, but nothing has changed in relation to adding items to or updating the cart.

So now there's no custom cart/order code at all in the project save the EventHandlers from the DemoStore project (which I also copied over).

I've also done a few debug tests, setting a watch on the expression commerceApi.GetCurrentOrder(CurrentPage.GetStore().Id) for the below code block and hitting a couple of breakpoints:

        public IActionResult AddToCart(AddToCartDto postModel)
        {
            try
            {
                if (!Guid.TryParse(postModel.ProductReference, out var productReference)) {
                    // Invalid Product code.
                    ModelState.AddModelError("productReference", "Failed to add product to cart");
                    return CurrentUmbracoPage();
                }

                var product = UmbracoContextAccessor.GetRequiredUmbracoContext().Content.GetById(productReference).OfType<IRollup>();
                commerceApi.Uow.Execute(uow =>
                {
                    var store = CurrentPage.GetStore();
                    var order = commerceApi.GetOrCreateCurrentOrder(store.Id)
                        .AsWritable(uow)
                        .AddProduct(postModel.ProductReference, postModel.ProductVariantReference, 1);

                    commerceApi.SaveOrder(order);

                    uow.Complete();
// --> At this point directly after the `SaveOrder` has completed but inside the UoW context, the watch expression above shows the _updated_ OrderLines count. 
                });
// --> At this point outside the `commerceApi.Uow.Execute` code block, the refreshed watch expression above shows the _original_ OrderLines count. 
            }
            catch (ValidationException ex)
            {
                ModelState.AddModelError("productReference", "Failed to add product to cart");
                logger.LogError(ex, "Failed to add product to cart {@PostModel}", postModel);
                return CurrentUmbracoPage();
            }

            TempData["addedProductReference"] = postModel.ProductReference;

            return RedirectToCurrentUmbracoPage();
        }

When I stop debugging and restart, the new items are visible in the cart so this is clearly a caching issue, but I can't see what's causing it.

There are no error logs being generated at all.

robertjf commented 10 months ago

@mattbrailsford we were discussing this today, and I realised that our first version that was working was actually Umbraco 10.6.1 with Umbraco.Commerce 10 - it also seems that was the version that we had this working on, and since upgrading to v12 we've not had the CartReadOnly object stay up to date.

mattbrailsford commented 10 months ago

Hey @robertjf ok, I'll have to review what changed between 10 and 12.

Out of interest, have you tried running the solution on another developers machine?

alirobe commented 10 months ago

I get the same issues on my machine and we get the same issues in cloud @mattbrailsford

robertjf commented 10 months ago

@mattbrailsford any update on this?

alirobe commented 10 months ago

@mattbrailsford not super happy with the level of support being recieved here. You should know the business is currently discussing dropping umbraco cloud + umbraco commerce... We want this to work, but we may have to if communication doesn't improve. If this was in production it would be a disaster.

mattbrailsford commented 10 months ago

Hey all, so I'm going to take a look at this personally today.

As a potential workaround one thing we could try is if you inject the IOrderService interface into your Controller instead of using the IUmbracoCommerceApi facade you should be able to cast the service instance to ICachedEntityService<OrderReadonly> this should then expose a method InvalidateEntityCache(Guid orderId) which should remove the given order from the current cache and so the next time it is accessed should be re fetched from the database.

I'll see if I can replicate this, but hopefully that might give you something to try in the meantime.

robertjf commented 10 months ago

Thanks @mattbrailsford - I'll give that a try - this issue also affects the back office Order list as well though

mattbrailsford commented 10 months ago

@robertjf hmm, that is a wonder then as the back office should always fetch from the database, but it's worth a try

mattbrailsford commented 10 months ago

Ok, so having investigated your codebase it looks like the issue is in your custom CountryDiscountRuleProvider and specifically it's CountryDiscountRuleProviderSettings model.

The Country property on this model is defined as IDictionary<Guid, Country?> however in your property editor code that is set as the editor, the model it returns is more like an array of objects like {"countryId":"322ed674-2a66-42cd-9ced-018ab4f0dfa7","code":"AU","name":"Australia"} and so when that persisted value is attempted to be Json deserialized to your settings model type it fails. This in turn causes the surrounding UoW to fail and so the changes never get updated.

I can confirm that creating a new model like:

public class CountryDto
{
    public Guid CountryId { get; set; }
    public string Code { get; set; }
    public string Name { get; set; }
}

And then updating your settings model to be IEnumerable<CountryDto> and finally updating your CountryDiscountRuleProvider with a check more like settings.Countries.Any(x => x.CountryId == ctx.Order.ShippingInfo.CountryId.Value) resolves the issue.

I do need to look at why changes actually got persisted though as if the transaction failed, then nothing should have gotten saved so I'm not sure why that happened, but either way, this update should fix your immediate issue.

robertjf commented 10 months ago

Wow, okay - thanks @mattbrailsford - it might be worth adding some failure logging to the UoW as well perhaps?

mattbrailsford commented 10 months ago

Yea, I’m going to review that and see what can be done better. I just wanted to get a fix to you asap.

alirobe commented 9 months ago

Thanks Matt! Appreciate the professional and quick response. Good to know it's not a product fault, and really appreciate the support. Excited to see project back on track again. Great stuff :)