Skip to content

fix: [SFCC-526] Miscellaneous fixes#279

Merged
ede-somogyi-algolia merged 5 commits into
developfrom
fix/sfcc-472/sfcc-526-misc-code-smells
Jun 18, 2026
Merged

fix: [SFCC-526] Miscellaneous fixes#279
ede-somogyi-algolia merged 5 commits into
developfrom
fix/sfcc-472/sfcc-526-misc-code-smells

Conversation

@ede-somogyi-algolia

@ede-somogyi-algolia ede-somogyi-algolia commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Various fixes for some of the issues flagged by the automated reviewer on PR #278.
I've added the details for each fix as PR comments.

@ede-somogyi-algolia ede-somogyi-algolia self-assigned this Jun 17, 2026
@codacy-production

codacy-production Bot commented Jun 17, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results
Complexity 0
Duplication 0

View in Codacy

TIP This summary will be updated as you push new changes.

if (promoPrices.length > 0) {
lowestPromoPrice = promoPrices.reduce(function (a, b) {
return Math.min(a.value, b.value);
return a.getValue() <= b.getValue() ? a : b;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The original code typecast the Money object into a number, which can lead to issues downstream (e.g. when trying to call .getValue() or .getCurrencyCode() on it).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

good catch 👍


if (promotionPrice.getValue() && promotionPrice.getValue() < minPrice ) {
minPrice = promotionPrice.getValue();
if (promotionPrice.isAvailable() && promotionValue < minPrice) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

A promotion can legitimately drive the price to 0 (100%-off / free gift). The original if (promotionPrice.getValue() && ...) treats that 0 as falsy and skips the promotion, and it calls getValue() three times. The fix caches the value once and gates on promotionPrice.isAvailable(), which correctly rejects NOT_AVAILABLE prices (where getValue() returns NaN) while letting a real 0 through.

this.brand = null;
this.bundle = false;
this.bundled = false;
this.images = images || PRODUCT_IMAGES;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The constructor sets this.bundle but isBundled() read this.bundled, which is a different attribute (but still valid). Added a distinct this.bundled field. Both this.bundle and this.bundled should be false for a master.

Comment thread .github/workflows/ci.yml

e2e-tests:
runs-on: ubuntu-latest
timeout-minutes: 15

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added the 15-minute timeout from #277. If the job doesn't finish in 15 minutes, chances are high that it will not be successful. With the timeout the job can be retried at least.

@ede-somogyi-algolia
ede-somogyi-algolia marked this pull request as ready for review June 17, 2026 15:08
if (promoPrices.length > 0) {
lowestPromoPrice = promoPrices.reduce(function (a, b) {
return Math.min(a.value, b.value);
return a.getValue() <= b.getValue() ? a : b;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

good catch 👍

Base automatically changed from fix/sfcc-472/sfcc-523-getters-and-setters to develop June 18, 2026 10:10
@ede-somogyi-algolia
ede-somogyi-algolia merged commit b0a2769 into develop Jun 18, 2026
5 checks passed
@ede-somogyi-algolia
ede-somogyi-algolia deleted the fix/sfcc-472/sfcc-526-misc-code-smells branch June 18, 2026 10:52
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