Skip to content

Conversation

@jmariconda
Copy link
Collaborator

@jmariconda jmariconda commented Nov 26, 2024

Ticket: COM-38695

Prerequisite PRs

Description

Port product-price template, formatter, unimplemented commerce utils, and test cases from template-compiler

NOTE: There are a lot of files changed, but most are just test templates that were ported from template-compiler. The main changes are in src/plugins.

Reasoning

The Commerce Merchandising team is currently working on a project to port the product list and detail pages over to the SDK, and as a result of that, those sections are now going through the client-side rendering pipeline on edit. Because of this, we need this formatter implemented in template engine so that the section re-renders properly on edit

NOTE: There will be a follow-up PR for the product-price formatter as well, so it might be good to wait on a release until then @phensley

Tests

  • Port over all of the tests from template-compiler and ensure that they are all passing

});

loader.paths('f-product-quick-view-%N.html').forEach((path) => {
test(`product quick view - ${path}`, () => loader.execute(path));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All of the new tests in this file and all of the related test files were ported over from template-compiler (tests, test templates)

};

export class ProductPriceFormatter extends Formatter {
private static BILLING_PERIOD_MONTHLY = 'MONTH';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was ported from the ProductPriceFormatter in template-compiler

const first = variants.get(0);
let moneyNode = isTruthy(first.path(['onSale'])) ? first.path(['salePriceMoney']) : first.path(['priceMoney']);
let price = getAmountFromMoneyNode(moneyNode);
if (price === undefined) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This condition is never true because getAmountFromMoneyNode always returns a Node

return DEFAULT_MONEY_NODE;
}
let moneyNode = variants.get(0);
let moneyNode = variants.get(0).path(['priceMoney']);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was an existing bug that I found from product-price tests failing. The corresponding line in template-compiler is here

@jmariconda jmariconda requested a review from sizhang12 November 26, 2024 22:05
@jmariconda jmariconda marked this pull request as ready for review November 26, 2024 22:05
@jmariconda jmariconda requested a review from phensley as a code owner November 26, 2024 22:05
@phensley
Copy link
Contributor

@jmariconda I created a PR #32 to remove the lodash dependency, otherwise these changes look good. I'm out for the rest of today day but should be able to merge and release this evening.

@jmariconda
Copy link
Collaborator Author

@phensley Sounds good. I misread that at first and merged the first PR in the stack, but I'll leave the rest for you

@jmariconda jmariconda changed the base branch from jmariconda/COM-38695-port-subscription-price-formatter to main November 27, 2024 16:17
@jmariconda jmariconda changed the base branch from main to master November 27, 2024 16:17
@jmariconda jmariconda changed the base branch from master to jmariconda/COM-38695-port-subscription-price-formatter November 27, 2024 16:17
@jmariconda
Copy link
Collaborator Author

jmariconda commented Nov 27, 2024

  • Change base branch before merging (I tried changing to main but it was including changes from the subscription-price PR even though those should already be there)

@phensley phensley force-pushed the jmariconda/COM-38695-port-product-price-formatter branch from 4aea92e to bf39a96 Compare November 28, 2024 15:34
@phensley phensley changed the base branch from jmariconda/COM-38695-port-subscription-price-formatter to main November 28, 2024 15:36
@phensley phensley merged commit dba7fa1 into main Nov 28, 2024
4 checks passed
@phensley
Copy link
Contributor

@sizhang12 @jmariconda All of your changes are released in version 2.10.7, let me know if you need any followups!

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.

4 participants