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

[Shipment] Applying discounts and taxes for each shipment #12282

Merged
merged 1 commit into from Jan 29, 2021

Conversation

GSadee
Copy link
Member

@GSadee GSadee commented Jan 27, 2021

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? I guess there is no
Deprecations? no
Related tickets
License MIT

@GSadee GSadee added the Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). label Jan 27, 2021
@GSadee GSadee requested a review from a team as a code owner January 27, 2021 10:56
@probot-autolabeler probot-autolabeler bot added the Admin AdminBundle related issues and PRs. label Jan 27, 2021
And the order's tax total should be "$24.15"
And the order's total should be "$129.15"

@ui @domain
Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if this scenario should have @ui tag, because multiple shipments in Sylius could be used in background and displayed in admin panel, but there is no possibility to create more than one shipment during our checkout for now.

Copy link
Member

Choose a reason for hiding this comment

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

Domain test is really nice, but I would leave it with UI test as well. One cannot create multiple shipments in pure Sylius, but it does not take much to change it.

@GSadee GSadee force-pushed the promotions-with-multiple-shipments branch from f8489f1 to 6e1db83 Compare January 28, 2021 06:18
@probot-autolabeler probot-autolabeler bot added the Documentation Documentation related issues and PRs - requests, fixes, proposals. label Jan 28, 2021
$taxRate = $this->taxRateResolver->resolve($shippingMethod, ['zone' => $zone]);
if (null === $taxRate) {
return;
if (!$order->hasShipments()) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it is 100%. I'm wondering if we can have shippingTotal > 0 with no shipments? Maybe we can make an early return for lack of shipment and then check the total value of it?

Copy link
Member

Choose a reason for hiding this comment

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

Or at least we should change the error message, as the order may not have any shipment at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest, I tried to keep the previous behaviour as much as possible, this check and error throw was moved here from the private method that was below: https://github.com/Sylius/Sylius/blob/master/src/Sylius/Component/Core/Taxation/Applicator/OrderShipmentTaxesApplicator.php#L96:L105

@GSadee GSadee force-pushed the promotions-with-multiple-shipments branch 2 times, most recently from a370552 to 31053ce Compare January 28, 2021 14:03
@probot-autolabeler probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Jan 28, 2021
@GSadee GSadee force-pushed the promotions-with-multiple-shipments branch 2 times, most recently from 201a3ae to 881054e Compare January 29, 2021 06:06
@GSadee GSadee force-pushed the promotions-with-multiple-shipments branch from 881054e to 1e64629 Compare January 29, 2021 06:16
@lchrusciel lchrusciel merged commit 2117a44 into Sylius:master Jan 29, 2021
@lchrusciel
Copy link
Member

Thank you, Grzegorz! 🥇

@GSadee GSadee deleted the promotions-with-multiple-shipments branch January 29, 2021 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin AdminBundle related issues and PRs. Documentation Documentation related issues and PRs - requests, fixes, proposals. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants