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

[API][UI][Behat][Refactor] OriginalUnitPrice property on the OrderItem entity #13563

Merged
merged 3 commits into from Feb 3, 2022
Merged

[API][UI][Behat][Refactor] OriginalUnitPrice property on the OrderItem entity #13563

merged 3 commits into from Feb 3, 2022

Conversation

Rafikooo
Copy link
Contributor

@Rafikooo Rafikooo commented Jan 28, 2022

Q A
Branch? 1.11
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
License MIT

To increase immutability, at the moment of adding an item to the cart, we should save the original price to the originalUnitPrice of this order item.
Using saved originalUnitPrice instead of using the prices from ChannelPricing while processing cart each time.

@Rafikooo Rafikooo added the Shop ShopBundle related issues and PRs. label Jan 28, 2022
@Rafikooo Rafikooo requested a review from a team as a code owner January 28, 2022 18:22
@probot-autolabeler probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Jan 31, 2022
UPGRADE-1.11.md Outdated Show resolved Hide resolved
UPGRADE-1.8.md Outdated Show resolved Hide resolved
@vvasiloi
Copy link
Contributor

vvasiloi commented Jan 31, 2022

WDYT about adding a method on the Order model to get the items total based on original unit price?

Edit: it would also require additional methods on the order item to calculate it's total first, plus a property to store the result and methods to recalculate the total. Just like with the current units and items total.

@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Feb 1, 2022
@Rafikooo Rafikooo changed the title [OrderItem] Save originalUnitPrice to the database each time an item added to the cart [API][UI][Behat][Refactor] OriginalUnitPrice property on OrderItem entity Feb 1, 2022
@Rafikooo Rafikooo changed the title [API][UI][Behat][Refactor] OriginalUnitPrice property on OrderItem entity [API][UI][Behat][Refactor] OriginalUnitPrice property on the OrderItem entity Feb 1, 2022
@@ -37,8 +37,7 @@
"variant": "\/api\/v2\/shop\/product-variants\/MUG_BLUE",
"productName": "Mug",
"id": @integer@,
"quantity": 3,
"originalPrice": null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH I'm not sure about that field. I checked the master demo API endpoint
api/v2/shop/account/orders/{tokenValue}/payments/{paymentId}
and it seems that that field should not be included, but I had one and it broke the tests

@@ -57,6 +57,7 @@
"id": @integer@,
"quantity": 3,
"unitPrice": 2000,
"originalUnitPrice": 2000,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After consultation with @GSadee originalUnitPrice can be included in that response

Comment on lines 31 to 49
if ($this->originalUnitPriceIsGreaterThanUnitPriceAndDiscountedUnitPrice($item)) {
return $item->getOriginalUnitPrice();
} elseif($this->originalUnitPriceIsNullAndUnitPriceIsGreaterThanDiscountedUnitPrice($item)) {
return $item->getUnitPrice();
}

return null;
}

private function originalUnitPriceIsGreaterThanUnitPriceAndDiscountedUnitPrice(OrderItem $item): bool
{
return ($item->getOriginalUnitPrice() !== null && $item->getOriginalUnitPrice() > $item->getUnitPrice()) ||
($item->getOriginalUnitPrice() !== null && $item->getOriginalUnitPrice() > $item->getDiscountedUnitPrice());
}

private function originalUnitPriceIsNullAndUnitPriceIsGreaterThanDiscountedUnitPrice(OrderItem $item): bool
{
return $item->getOriginalUnitPrice() === null && $item->getUnitPrice() > $item->getDiscountedUnitPrice();
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ($this->originalUnitPriceIsGreaterThanUnitPriceAndDiscountedUnitPrice($item)) {
return $item->getOriginalUnitPrice();
} elseif($this->originalUnitPriceIsNullAndUnitPriceIsGreaterThanDiscountedUnitPrice($item)) {
return $item->getUnitPrice();
}
return null;
}
private function originalUnitPriceIsGreaterThanUnitPriceAndDiscountedUnitPrice(OrderItem $item): bool
{
return ($item->getOriginalUnitPrice() !== null && $item->getOriginalUnitPrice() > $item->getUnitPrice()) ||
($item->getOriginalUnitPrice() !== null && $item->getOriginalUnitPrice() > $item->getDiscountedUnitPrice());
}
private function originalUnitPriceIsNullAndUnitPriceIsGreaterThanDiscountedUnitPrice(OrderItem $item): bool
{
return $item->getOriginalUnitPrice() === null && $item->getUnitPrice() > $item->getDiscountedUnitPrice();
}
if (
$item->getOriginalUnitPrice() !== null &&
($item->getOriginalUnitPrice() > $item->getUnitPrice()) || $item->getOriginalUnitPrice() > $item->getDiscountedUnitPrice())
) {
return $item->getOriginalUnitPrice();
}
if ($item->getOriginalUnitPrice() === null && $item->getUnitPrice() > $item->getDiscountedUnitPrice()) {
return $item->getUnitPrice();
}
return null;
}

@GSadee GSadee merged commit d07a74b into Sylius:1.11 Feb 3, 2022
@GSadee
Copy link
Member

GSadee commented Feb 3, 2022

Thanks, Rafał! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs. Maintenance CI configurations, READMEs, releases, etc. Shop ShopBundle related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants