adobe / aem-core-cif-components

A set of configurations and components to get you started with AEM Commerce development
Apache License 2.0
102 stars 80 forks source link

CIF-2982 - Support product filter hooks in ProductList component #966

Closed herzog31 closed 1 year ago

herzog31 commented 1 year ago

Description

How Has This Been Tested?

Types of changes

Checklist:

codecov[bot] commented 1 year ago

Codecov Report

Merging #966 (6f31774) into master (00af1c5) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master     #966   +/-   ##
=========================================
  Coverage     89.14%   89.15%           
- Complexity     2208     2211    +3     
=========================================
  Files           354      354           
  Lines          9980     9985    +5     
  Branches       1437     1437           
=========================================
+ Hits           8897     8902    +5     
  Misses          787      787           
  Partials        296      296           
Flag Coverage Δ
integration 51.80% <79.16%> (-0.05%) :arrow_down:
jest 86.68% <ø> (ø)
karma 95.53% <ø> (ø)
unittests 87.42% <100.00%> (+0.01%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ore/components/models/productlist/ProductList.java 50.00% <ø> (ø)
...ch/internal/services/SearchResultsServiceImpl.java 94.49% <ø> (ø)
...nternal/models/v1/productlist/ProductListImpl.java 89.01% <100.00%> (ø)
...mponents/models/productlist/CategoryRetriever.java 100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

buuhuu commented 1 year ago

With this approach we now have two ways to extend the query of the product list component: by using the productQueryHook of the ProductList#getCategoryRetriever() and by using the newly introduced filter hook.

Wouldn't it make more sense to add the product filter hook to the AbstractCategoryRetriever? That way the extension pattern remains the same. Also that the SearchResults and the ProductList use the same queries is an implementation detail, it does not mean that any ProductCollection would allow to do that.

herzog31 commented 1 year ago

@Buuhuu I think we have a few consistency issues here:

I don't have a good answer here, since frankly the SearchResultsService is a mess.