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

Lazy Loading for Prices #201

Open
wants to merge 42 commits into
base: develop
Choose a base branch
from
Open

Lazy Loading for Prices #201

wants to merge 42 commits into from

Conversation

htuzel
Copy link
Collaborator

@htuzel htuzel commented Oct 31, 2024

Lazy Loading for Prices

Introduces lazy loading of promotional prices from SFCC instead of relying on indexed Algolia prices. This ensures real-time price accuracy and proper handling of promotional campaigns.

Features

  • New EnablePricingLazyLoad preference in Business Manager
  • Real-time price fetching from SFCC via new /Algolia-Price endpoint
  • Client-side price caching to minimize API calls
  • Dynamic update of promotional prices and messages

Testing

  1. Enable "Pricing Lazy Load" in Business Manager
  2. Create a test promotion in SFCC
  3. Verify promotional prices and messages display correctly

The feature is optional and can be toggled in Business Manager settings.

htuzel and others added 30 commits August 12, 2024 13:41
This commit adds the `promotionalPrices` attribute to the Algolia product configuration in order to support fetching and indexing promotional prices for products.
The code changes in `algoliaLocalizedProduct.js` remove the `startDate` and `endDate` fields in the `promotionalPrices` attribute. These fields are no longer needed for indexing promotional prices for products in Algolia.
…a/instantsearch-config.js

Co-authored-by: Sylvain Bellone <[email protected]>
This reverts commit b0f9b8a.
@htuzel htuzel changed the title Lay Lazy Loading for Prices Oct 31, 2024
@htuzel htuzel self-assigned this Oct 31, 2024
@htuzel htuzel requested a review from sbellone October 31, 2024 13:16
Copy link
Collaborator

@sbellone sbellone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many things can be simplified, but the most important thing that I'm surprised you don't even mention, is that you are deviating from the current behaviour of the cartridge:

We are currently indexing and displaying the sales price by default (the one returned by product.priceModel.price):
image

With lazy loading, you are displaying the list price, so we have the following:

image

What's the reason to do that? I think we need to discuss about it.

@@ -0,0 +1,7 @@
jest.mock('dw/object/CustomObjectMgr', () => ({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this file for? You don't require it anywhere, plus the CustomObjectMgr is already declared in jest.setup.js

if ((product && product.activePromotion && product.activePromotion.price) ||
(product && product.price && product.price.list && product.price.list.value)) {
return `<span class="strike-through list">
<span class="value"> ${algoliaData.currencySymbol} ${(priceObj && priceObj.list && priceObj.list.value) || (priceObj && priceObj.sales && priceObj.sales.value)} </span>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those super long conditions are hard to read and error prone.
Can you break them down into variables at the beginning of the function?

@@ -242,16 +243,18 @@ function enableInstantSearch(config) {
showMoreText: algoliaData.strings.moreResults,
empty: '',
item(hit, { html, components }) {
const hiddenCalloutMsg = !hit.calloutMsg && 'd-none';
const displayCalloutMsg = (isPricingLazyLoad && 'd-none') || (!isPricingLazyLoad && (!hit.calloutMsg && 'd-none'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution can be simplified: (isPricingLazyLoad || !hit.calloutMsg) && 'd-none'

But when there is a calloutMessage, displayCalloutMsg becomes set to false, so the callout message ends up with the following css classes:
class="callout-msg false callout-msg-placeholder-640188017195M"

It works, but it's not clean

Comment on lines +31 to +33
for (var j = 0; j < promotions.length; j++) {
var promotion = promotions[j]
var apiPromotion = PromotionMgr.getPromotion(promotion.id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure all this is needed? You are using the ProductFactory, which already sets the best active promotion in the sales price: https://github.com/SalesforceCommerceCloud/storefront-reference-architecture/blob/3a99a7990e70756eb21b775c73e7680a984be0d3/cartridges/app_storefront_base/cartridge/scripts/factories/price.js#L84

I've debugged and I confirm that I see the promotion price.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in this case, we don't know which promotion is applied for this price. There may be multiple promotions, and we need to know which promotion is causing this price and display call out message.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SFRA systematically uses the first promotion: https://github.com/SalesforceCommerceCloud/storefront-reference-architecture/blob/3a99a7990e70756eb21b775c73e7680a984be0d3/cartridges/app_storefront_base/cartridge/scripts/helpers/pricing.js#L46

So we actually know. But this is an good reason to compute it ourselves, as technically there could be a better price than the one present in the sales price.

@htuzel
Copy link
Collaborator Author

htuzel commented Nov 4, 2024

Many things can be simplified, but the most important thing that I'm surprised you don't even mention, is that you are deviating from the current behaviour of the cartridge:

We are currently indexing and displaying the sales price by default (the one returned by product.priceModel.price): image

With lazy loading, you are displaying the list price, so we have the following:

image

What's the reason to do that? I think we need to discuss about it.

As default SFRA behaviour, it is rendering like that. That is the main reason and I keep default logic of SFRA

@htuzel htuzel requested a review from sbellone November 4, 2024 12:52
Copy link
Collaborator

@sbellone sbellone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, we can't display the list price as the strike-out price, because the promotion is calculated from the best pricebook price (i.e. in my screenshot, the 20% promotion is calculated from $299.99 which is the sales price, not from $500).

We suppose that to manage promotions, people either uses pricebooks only or promotions only. Maybe we'll need to offer the choice.

Comment on lines +31 to +33
for (var j = 0; j < promotions.length; j++) {
var promotion = promotions[j]
var apiPromotion = PromotionMgr.getPromotion(promotion.id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SFRA systematically uses the first promotion: https://github.com/SalesforceCommerceCloud/storefront-reference-architecture/blob/3a99a7990e70756eb21b775c73e7680a984be0d3/cartridges/app_storefront_base/cartridge/scripts/helpers/pricing.js#L46

So we actually know. But this is an good reason to compute it ourselves, as technically there could be a better price than the one present in the sales price.

@htuzel htuzel requested a review from sbellone November 5, 2024 12:12
Copy link
Collaborator

@sbellone sbellone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok, just updating a bit the wording

@@ -200,6 +200,14 @@ Setting this preference replaces the first two segments, the final index name be
<default-value>false</default-value>
</attribute-definition>

<attribute-definition attribute-id="Algolia_EnablePricingLazyLoad">
<display-name xml:lang="x-default">Enables Pricing Lazy Load</display-name>
<description xml:lang="x-default">It fetches prices from SFCC not Algolia</description>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<description xml:lang="x-default">It fetches prices from SFCC not Algolia</description>
<description xml:lang="x-default">Fetch prices from SFCC after the search results are returned from Algolia</description>

@@ -200,6 +200,14 @@ Setting this preference replaces the first two segments, the final index name be
<default-value>false</default-value>
</attribute-definition>

<attribute-definition attribute-id="Algolia_EnablePricingLazyLoad">
<display-name xml:lang="x-default">Enables Pricing Lazy Load</display-name>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<display-name xml:lang="x-default">Enables Pricing Lazy Load</display-name>
<display-name xml:lang="x-default">Enable prices lazy loading</display-name>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants