Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEXT-00000 - Move ProductReviewLoader logic to core #3671

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

akf-bw
Copy link
Contributor

@akf-bw akf-bw commented Apr 17, 2024

1. Why is this change necessary?

  • Currently the ProductReviewLoader is in the Storefront package, but we also need the functionality in the core (for example for the ProductDescriptionReviewsCmsElementResolver).
  • The logic in both classes is nearly (will be explained below) the same, so we should bring the ProductReviewLoader to the Core and remove the duplicate code with this step
  • I also added the missing core.listing.reviewsPerPage config option, which allows the administration user to change the size of the review section

Logic-Fixes:

  • Currently there are two small things different from the Core ProductDescriptionReviewsCmsElementResolver logic and the Storefront ProductReviewLoader
  • Because of this there are differences the logic of the initial review load and the refetch on a filter change!
  • The ProductReviewsLoadedEvent is currently only in the Storefront package, because of this on the initial review load by the cms there is no ProductReviewsLoadedEvent fired
  • The logic of createReviewCriteria is slightly different. In the Core ProductDescriptionReviewsCmsElementResolver the TOTAL_COUNT_MODE_EXACT is missing, because of this missing Criteria setting the initial review total count is wrong
  • With this PR both logics use the same ProductReviewLoader
Initial page load review total count problem Before:

After:
  • The ProductReviewsLoadedEvent & ReviewLoaderResult in the Core already had deprecated code, but with this PR we can fully depreciate this classes

2. What does this change do, exactly?

  • Changed ProductDescriptionReviewsCmsElementResolver to use the AbstractProductReviewLoader and removed the duplicate functions
  • Changed ProductDescriptionReviewsCmsElementResolver to now execute the Core ProductReviewsWidgetLoadedHook hook
  • Added AbstractProductReviewLoader to allow overwriting product review load logic
  • Added ProductReviewLoader based on the Storefront ProductReviewLoader
  • Changed ProductReviewResult to include the totalNativeReviews field
  • Added ProductReviewsWidgetLoadedHook based on the Storefront ProductReviewsWidgetLoadedHook
  • Added ProductReviewsLoadedEvent based on the Storefront ProductReviewsLoadedEvent
  • Added Migration1711461585AddDefaultSettingConfigValueForReviewListingPerPage to include the new config option
  • Added core.listing.reviewsPerPage to config listing with default value 10
  • Changed ProductDescriptionReviewsTypeDataResolverTest to match Core changes
  • Added ProductReviewLoaderTest to match core changes
  • Changed CmsController to use AbstractProductReviewLoader
  • Changed ProductController to use AbstractProductReviewLoader
  • Changed ProductReviewLoader to @deprecated and copy logic from Core ProductReviewLoader
  • Changed ProductReviewsLoadedEvent to @deprecated
  • Changed ProductReviewsWidgetLoadedHook to @deprecated
  • Changed ReviewLoaderResult to @deprecated
  • Changed review.html.twig template to include the new core.listing.reviewsPerPage to config
  • Changed review.html.twig template to include missing nativeReviewsCount and foreignReviewsCount variables
  • Changed review.html.twig by including additional component_review_list_action_filters and component_review_list_counter blocks
  • Changed CmsControllerTest to match Storefront changes
  • Changed ProductControllerTest to match Storefront changes
  • Changed ProductReviewLoaderTest to match Storefront changes

3. Describe each step to reproduce the issue or behaviour.

  • (For the wrong total, just visit a product with more than 10 reviews)

4. Please link to the relevant issues (if any).

5. Checklist

  • I have rebased my changes to remove merge conflicts
  • I have written tests and verified that they fail without my change
  • I have created a changelog file with all necessary information about my changes
  • I have written or adjusted the documentation according to my changes
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I have read the contribution requirements and fulfil them.

Copy link

github-actions bot commented Apr 17, 2024

Fails
🚫 Please add tests for your new Migration file
🚫 Please add tests for your new Migration file
🚫 Please add tests for your new Migration file
Warnings
⚠️ You probably moved or deleted a twig block. This is likely a hard break. Please check your template changes and make sure that deleted blocks are already deprecated.
If you are sure everything is fine with your changes, you can resolve this warning.
Moved or deleted block:
Array ( [0] => Array ( [0] => component_review_list_action_language [1] => component_review_list_action_sortby [2] => component_review_list_action_sortby_form [3] => component_review_list_action_sortby_label [4] => component_review_list_action_sortby_select )
)

@akf-bw akf-bw force-pushed the product-review-loader-core branch 3 times, most recently from 02d02a6 to 42ed756 Compare April 17, 2024 07:16
@keulinho
Copy link
Contributor

Hey @akf-bw thanks for your contribution!

There was already a similiar PR here: #2389

However deprecation and break strategy can be a hassle when we want to move code from the storefront to the core, please follow up on the discussion in the previous PR and adjust the breaking changes here accordingly.

@akf-bw akf-bw force-pushed the product-review-loader-core branch 2 times, most recently from 08ad3f2 to c5cfed6 Compare April 17, 2024 07:57
@akf-bw akf-bw force-pushed the product-review-loader-core branch from c5cfed6 to 27d2d39 Compare April 17, 2024 08:15
@akf-bw akf-bw force-pushed the product-review-loader-core branch 2 times, most recently from f9442c8 to dd31dcf Compare April 17, 2024 09:08
@aragon999
Copy link
Contributor

What I am not sure, if one wants to have public methods like: AbstractProductReviewLoader::getReviewRatingMatrix and AbstractProductReviewLoader::createReviewCriteria, which are only public due to compatibility reasons.

@akf-bw akf-bw force-pushed the product-review-loader-core branch 5 times, most recently from 09618cf to 3a88731 Compare April 19, 2024 06:48
@akf-bw
Copy link
Contributor Author

akf-bw commented Apr 19, 2024

I now included all features of the long time missing NEXT-16994 changes in review.html.twig releated to the nativeReviewsCount & foreignReviewsCount variables.
I also added 2 new non-breaking additional template blocks to review.html.twig, which can also be used by SwagCommercial to better override the template.
@aragon999 I changed it to non-public.

@akf-bw akf-bw force-pushed the product-review-loader-core branch 4 times, most recently from fac45b7 to 25cb403 Compare April 19, 2024 09:12
@aragon999
Copy link
Contributor

@akf-bw thank you, I did not find time to respond, but I think in particular when you can decorate such services, this makes working in (store)-plugins quite difficult, as there can be so many moving parts.

@akf-bw akf-bw force-pushed the product-review-loader-core branch from 25cb403 to 244b0bd Compare April 25, 2024 05:44
@akf-bw akf-bw force-pushed the product-review-loader-core branch from 244b0bd to 4545703 Compare April 25, 2024 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants