Skip to content

Feature/demo pr checkout regression#17

Open
mainak-debnath wants to merge 2 commits into
mainfrom
feature/demo-pr-checkout-regression
Open

Feature/demo pr checkout regression#17
mainak-debnath wants to merge 2 commits into
mainfrom
feature/demo-pr-checkout-regression

Conversation

@mainak-debnath

Copy link
Copy Markdown
Owner

No description provided.

@mainak-debnath mainak-debnath left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

AI Code Review

reserved_skus = await self._inventory_gateway.reserve(lines)
order.reserved_skus = reserved_skus
order.status = OrderStatus.RESERVED
order.status = OrderStatus.PAID

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Setting the order status to PAID at this point is premature. The payment has not yet been captured by the _payment_gateway. If the payment fails, the order will incorrectly remain in a PAID state in the database. The status should be set to PAID only after a successful call to _payment_gateway.capture_payment. Consider setting the status to OrderStatus.RESERVED here (as it was previously) and then to OrderStatus.PAID after successful payment, or remove this line and only set PAID after the payment gateway call.

subtotal=subtotal,
discount=discount_amount,
)
customer_id = payload["customer"]["id"]

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Directly accessing payload['customer']['id'] can lead to a KeyError if the 'customer' or 'id' key is missing. The _build_customer method already handles customer validation; consider retrieving customer_id from the customer object returned by _build_customer to ensure consistency and prevent unhandled exceptions. Alternatively, use payload.get('customer', {}).get('id') with appropriate error handling.

customer_id = payload["customer"]["id"]
subtotal = Decimal("0")
for item in lines:
subtotal += item.unit_price

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This manual subtotal calculation is incorrect as it only sums unit_price and ignores quantity. It also duplicates and bypasses the PricingService.calculate_subtotal method, which correctly handles unit_price * quantity and quantization. Revert to using self._pricing_service.calculate_subtotal(lines) to ensure correct monetary calculations and adhere to the defined pricing abstraction.

for item in lines:
subtotal += item.unit_price

discount_amount = Decimal("0")

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Hardcoding discount_amount to Decimal('0') and total_amount to subtotal bypasses the PricingService's discount and total calculation logic. This will prevent loyalty-tier based discounts from being applied. Revert to using self._pricing_service.calculate_discount and self._pricing_service.calculate_total to ensure business logic integrity and correct pricing.

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.

1 participant