vendrhub / vendr-demo-store

Demo store for Vendr, the eCommerce solution for Umbraco v8+
MIT License
25 stars 15 forks source link

Use ToPublishedSearchResults in ProductSurfaceController #3

Closed bjarnef closed 4 years ago

bjarnef commented 4 years ago

In ProductSurfaceController it use the following to convert the Examine search results into IEnumerable<IPublishedContent> and cast these type of ProductPage. https://github.com/vendrhub/vendr-demo-store/blob/dev/src/Vendr.DemoStore/Web/Controllers/ProductSurfaceController.cs#L67

It can however used the method ToPublishedSearchResults() here.

In Articulate it does the following: https://github.com/Shazwazza/Articulate/blob/3ff42711f38130bdde577b13c3fba240d97c51ac/src/Articulate/DefaultArticulateSearcher.cs#L83

@Shazwazza mentioned it probably would be better to inject IUmbracoContextAccessor instead of UmbracoContext (and that you in .NET core cannot inject UmbracoContext anymore).

Seomthing like this should work:

public class ProductSurfaceController : SurfaceController
{
    private readonly IExamineManager _examineManager;
    private readonly IUmbracoContextAccessor _umbracoContextAccessor;

    public ProductSurfaceController(IExamineManager examineManager, IUmbracoContextAccessor umbracoContextAccessor)
    {
        _examineManager = examineManager;
        _umbracoContextAccessor = umbracoContextAccessor;
    }
    ...
}
var results = query.OrderBy(new SortableField("name", SortType.String)).Execute(pageSize * page);
var totalResults = results.TotalItemCount;
var items = results.Skip(pageSize * (page - 1)).ToPublishedSearchResults(_umbracoContextAccessor.UmbracoContext.PublishedSnapshot.Content).OfType<ProductPage>();

return new PagedResult<ProductPage>(totalResults, page, pageSize)
{
      Items = items
};

Not sure if there is a difference in using _umbracoContextAccessor.UmbracoContext.PublishedSnapshot.Content vs _umbracoContextAccessor.UmbracoContext.Content?

mattbrailsford commented 4 years ago

Cool. Well, if that fixes one thing on the road to .NET Core then I'm happy with the suggestion. I'll take a look when I can, or feel free to submit a PR if you are up for that. đź‘Ť

bjarnef commented 4 years ago

Okay, tried the following on a project:

var results = query.OrderBy(new SortableField("name", SortType.String)).Execute(pageSize * page);
var totalResults = results.TotalItemCount;
var pagedResults = results.Skip(pageSize * (page - 1));

var test1 = pagedResults.Select(x => UmbracoContext.Content.GetById(int.Parse(x.Id)))
                        .OfType<ProductPage>();

var test2 = pagedResults.ToPublishedSearchResults(_umbracoContextAccessor.UmbracoContext.Content)
                        .OfType<ProductPage>();

var test3 = pagedResults.ToPublishedSearchResults(_umbracoContextAccessor.UmbracoContext.PublishedSnapshot.Content)
                        .OfType<ProductPage>();

However both test2 and test3 returned zero results, while test1 returned 12 results.

mattbrailsford commented 4 years ago

OfType may not work as ToPublishedSearchResults returns a PublishedSearchResult collection, not IPublishedContent. You'll probably need to extract it's .Content property.

bjarnef commented 4 years ago

Ahh, yes of course. I forgot the selecting the content results from PublishedSearchResult.

Both of these works. Not sure if one of them is preferred?

var test1 = pagedResults.ToPublishedSearchResults(_umbracoContextAccessor.UmbracoContext.PublishedSnapshot.Content)
                        .Select(x => x.Content)
                        .OfType<ProductPage>();

var test2 = pagedResults.ToPublishedSearchResults(_umbracoContextAccessor.UmbracoContext.Content)
                        .Select(x => x.Content)
                        .OfType<ProductPage>();
mattbrailsford commented 4 years ago

Under the hood _umbracoContextAccessor.UmbracoContext.Content just proxies to PublishedSnapshot anyways, but I'd be tempted to use _umbracoContextAccessor.UmbracoContext.Content as it exposes less Umbraco domain knowledge. If it ever becomes obsolete we can always update it.

bjarnef commented 4 years ago

Yes, it seems a bit simpler. Maybe @Shazwazza has some feedback on which of these that should be used?

bjarnef commented 4 years ago

Fixed in https://github.com/vendrhub/vendr-demo-store/pull/6