Here are the responses to your questions about the PR:
In response to all "add tests" notes, I will add tests once the bug has been resolved. I have a separate issue for that, so the tests can be reviewed when done and it should be a fairly quick review.
ProcessItem implementation is irrelevant as the method is called from ProcessItems method, which is overriden
ProcessItem needs to be implemented because it's abstract, but doing it this way we can process tasks in bulk. If we didn't override ProcessItems, the default implementation would process each task individually which would cause more Algolia API calls.
IAlgoliaFacetFilter- What is the purpose or expected flow of this method?
Do you mean what is the purpose of the whole interface? It is for setting up faceted search which is described for KX13 here. Using IAlgoliaFacetFilter in an action allows the submitted form data to be passed back into the controller and we can see which facets were checked, and generate an Algolia query based on the selection. The checked state of all facets is also persisted between requests to help with the faceting UI.
When creating AlgoliaFacetFilterViewModel instance, attributes are provided and they should have assigned display name, or not?
The Algolia facets and their values are only retrieved after the search. So when you first arrive on the page the FacetedAttributes list will be empty, but it gets populated from the Algolia search response with the proper names.
Make FacetedAttributes property setter private
I tested this, but it broke faceted search functionality UI on the front-end (the FacetedAttributes always ended up zero-length). I believe it needs to be public so the controller can populate the data from the submitted form.
Does it make sense to install Algolia integration and be forced to add resource keys to resx file in order to get display names localized. Especially is the IAlgoliaFacetFilter.Localize has to be called. Besides, just one culture is supported by admin UI at the moment.
I agree it's kind of strange, though I wasn't sure how else to make the relationship between attribute name+facet value (like "CoffeeIsDecaf:true") and some string like "Yes." I made some large changes to AlgoliaFacetFilter in the PR so now localization is done by default and the mapping is done in code. In my testing I added the mapping to the search class itself:
public class CoffeeSearchModel : AlgoliaSearchModel
{
public static Dictionary<string, string> Translations
{
get
{
return new Dictionary<string, string>
{
{ nameof(CoffeeSearchModel.CoffeeIsDecaf), "Decaffeinated" },
{ $"{nameof(CoffeeSearchModel.CoffeeIsDecaf)}.true", "Yes" },
{ $"{nameof(CoffeeSearchModel.CoffeeIsDecaf)}.false", "No" },
{ nameof(CoffeeSearchModel.CoffeeProcessing), "Processing" },
{ $"{nameof(CoffeeSearchModel.CoffeeProcessing)}.washed", "Washed" },
{ $"{nameof(CoffeeSearchModel.CoffeeProcessing)}.natural", "Natural" }
};
}
}
and now to get the facets and localize, it's one easy step: filter.UpdateFacets(new FacetConfiguration(searchResponse.Facets, CoffeeSearchModel.Translations));
Do we expect that customers will call LogTask or LogTasks methods directly?
I don't expect so, but there could be some edge-cases. I was mostly just trying to keep customization points open for customers, but if you think it makes more sense I can review all the interfaces and make them internal only if customers might not use it.
DefaultAlgoliaInsightsService - How is this implementation meant to be used? What is the purpose of IAlgoliaInsightsService interface? I believe that conversion logging is not part of opportunity specification.
It can be used to personalize search results for visitors. Since it was already in the KX13 integration, it made sense to transfer it as there was no cost.
Methods DeleteRecords and UpsertRecords do not check index existence, Rebuild does. Is it intended?
Those methods handle the logged tasks, and there are no logged tasks for invalid indexes. The Rebuild method is manually executed from the UI so there are no tasks and it needs to make sure the index is valid.
GetAssetUrlsForColumn calls deserialization directly, obtain data type from DataTypeManager and convert data using DataType.ConversionFunc.
I started using DataType, but I noticed in generated page type code we use JsonDataTypeConverter.ConvertToModels<AssetRelatedItem>(strValue) which is much easier. Is that okay to use?
I do not see a re-try policy. It means that if something fails during Algolia API call the index content becomes outdated, right?
Algolia has a retry policy, is that sufficient? Or, should we have an application level policy? What would it look like, maybe logging the task to the queue again so it gets processed the next time?
Task-based API should allow to pass CancellationToken object
I must admit I'm not familiar with using CancellationTokens. I added it to async methods here, please let me know if it makes any sense at all
Here are the responses to your questions about the PR:
In response to all "add tests" notes, I will add tests once the bug has been resolved. I have a separate issue for that, so the tests can be reviewed when done and it should be a fairly quick review.
ProcessItem needs to be implemented because it's abstract, but doing it this way we can process tasks in bulk. If we didn't override ProcessItems, the default implementation would process each task individually which would cause more Algolia API calls.
Do you mean what is the purpose of the whole interface? It is for setting up faceted search which is described for KX13 here. Using IAlgoliaFacetFilter in an action allows the submitted form data to be passed back into the controller and we can see which facets were checked, and generate an Algolia query based on the selection. The checked state of all facets is also persisted between requests to help with the faceting UI.
The Algolia facets and their values are only retrieved after the search. So when you first arrive on the page the FacetedAttributes list will be empty, but it gets populated from the Algolia search response with the proper names.
I tested this, but it broke faceted search functionality UI on the front-end (the FacetedAttributes always ended up zero-length). I believe it needs to be public so the controller can populate the data from the submitted form.
I agree it's kind of strange, though I wasn't sure how else to make the relationship between attribute name+facet value (like "CoffeeIsDecaf:true") and some string like "Yes." I made some large changes to AlgoliaFacetFilter in the PR so now localization is done by default and the mapping is done in code. In my testing I added the mapping to the search class itself:
and now to get the facets and localize, it's one easy step: filter.UpdateFacets(new FacetConfiguration(searchResponse.Facets, CoffeeSearchModel.Translations));
I don't expect so, but there could be some edge-cases. I was mostly just trying to keep customization points open for customers, but if you think it makes more sense I can review all the interfaces and make them internal only if customers might not use it.
It can be used to personalize search results for visitors. Since it was already in the KX13 integration, it made sense to transfer it as there was no cost.
Those methods handle the logged tasks, and there are no logged tasks for invalid indexes. The Rebuild method is manually executed from the UI so there are no tasks and it needs to make sure the index is valid.
I started using DataType, but I noticed in generated page type code we use
JsonDataTypeConverter.ConvertToModels<AssetRelatedItem>(strValue)which is much easier. Is that okay to use?Algolia has a retry policy, is that sufficient? Or, should we have an application level policy? What would it look like, maybe logging the task to the queue again so it gets processed the next time?
I must admit I'm not familiar with using CancellationTokens. I added it to async methods here, please let me know if it makes any sense at all