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
fix: addToCart takes actual product code into account #18731
Conversation
Not referring to unit under test
feature-libs/cart/base/components/added-to-cart-dialog/added-to-cart-dialog-event.listener.ts
Show resolved
Hide resolved
…rtacus into fix/CXSPA-6791-addToCart
feature-libs/cart/base/components/added-to-cart-dialog/added-to-cart-dialog-event.listener.ts
Outdated
Show resolved
Hide resolved
* Was the product added to the cart merged into an existing cart entry (with increased quantity), | ||
* or did the system create a new cart entry? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: The question mark in the end of the sentence is a bit confusing. What about:
* Was the product added to the cart merged into an existing cart entry (with increased quantity), | |
* or did the system create a new cart entry? | |
* Tells whether the product added to the cart was merged into an existing cart entry (with increased quantity), | |
* or the system created a new cart entry. |
@@ -72,6 +72,7 @@ export class CartAddEntrySuccessEvent extends CartEvent { | |||
entry?: OrderEntry; | |||
quantityAdded?: number; | |||
deliveryModeChanged?: boolean; | |||
pickupStore?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change really related to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is related (because we need to display the pickup store name in the confirmation dialogue).
But still this is a good hint, on a second look I saw that this information can also be derived from the cart modification via entry.deliveryPointOfService.name, then the event does not need to be touched!
@@ -43,6 +43,12 @@ export interface FeatureTogglesInterface { | |||
*/ | |||
productConfiguratorAttributeTypesV2?: boolean; | |||
|
|||
/** | |||
* The addedToCart dialog is driven by 'CartAddEntrySuccessEvent'. Previously it was driven | |||
* by 'CartUiEventAddToCart event. Code changes affect 'AddedToCartDialogEventListener' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* by 'CartUiEventAddToCart event. Code changes affect 'AddedToCartDialogEventListener' | |
* by 'CartUiEventAddToCart' event. Code changes affect 'AddedToCartDialogEventListener' |
protected calculateEntryWasMerged( | ||
successEvent: CartAddEntrySuccessEvent | ||
): boolean { | ||
// In case quantityAdded zero, the system could have run into stock issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From unit tests and implementation I understood that in case of stock issues the property quantityAdded
is undefined
(not 0
).
So what about updating the comment:
// In case quantityAdded zero, the system could have run into stock issues. | |
// In case `quantityAdded` is `undefined`, the system could have run into stock issues. |
Btw. IMHO we could turn this inline comment in to a fully-fledged JSDoc of this method.
4 flaky tests on run #43499 ↗︎
Details:
regression/checkout/checkout-flow.core-e2e.cy.ts • 1 flaky test • B2C
ssr/pages.core-e2e.cy.ts • 3 flaky tests • SSR
Review all test suite changes for PR #18731 ↗︎ |
No description provided.