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

Recalculate order adjustments total after adjustment is added or remo… #12502

Merged
merged 3 commits into from
Aug 3, 2023

Conversation

kayue
Copy link
Contributor

@kayue kayue commented Apr 1, 2021

Q A
Branch? 1.9
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

In the current code (1.8 at least) adjustment total is not being updated after shipping fee is added to order. For example in the code below:

$shippingCharge = $this->shippingChargesCalculator->calculate($shipment);
/** @var AdjustmentInterface $adjustment */
$adjustment = $this->adjustmentFactory->createNew();
$adjustment->setType(AdjustmentInterface::SHIPPING_ADJUSTMENT);
$adjustment->setAmount($shippingCharge);
$adjustment->setLabel($shipment->getMethod()->getName());
$order->addAdjustment($adjustment);

Recalculation is designed to happen when $adjustment->setAmount() is called, however the Order object is only added after. (in the last line)

Proposed fixed is to trigger recalculateAdjustmentsTotal() whenever adjustment is added (or removed) to order.

@kayue kayue requested a review from a team as a code owner April 1, 2021 09:44
@Zales0123 Zales0123 added the Bug Confirmed bugs or bugfixes. label Apr 2, 2021
Copy link
Member

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

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

Generally, I think it's a good idea 👍 2 notes from me:

  • we should probably add such a recalculation in every adjustable object (Order, OrderItem and OrderItemUnit)
  • I would start with changing the spec for the proper objects (e.g. it_adds_adjustments_properly for the Order) and call $this->getAdjustmentsTotal()->shouldReturn(XXX); at the end

🖖

src/Sylius/Component/Order/Model/Order.php Outdated Show resolved Hide resolved
@SirDomin SirDomin changed the base branch from 1.8 to 1.9 July 29, 2021 09:20
@SirDomin
Copy link
Contributor

The base of this pull-request was changed, you need fetch and reset your local branch
if you want to add new commits to this pull request. Reset before you pull, else commits
may become messed-up.

Unless you added new commits (to this branch) locally that you did not push yet,
execute git fetch origin && git reset "patch-6" to update your local branch.

Feel free to ask for assistance when you get stuck 👍

Zales0123
Zales0123 previously approved these changes Jul 30, 2021
@Zales0123
Copy link
Member

OR 😄 in fact I'm not 100% sure should we merge it without adding the same feature on OrderItem, OrderItemUnit and Shipment level - currently no adjustment is added to Order directly, so we cannot be sure it does not break anything, as these change would not be covered by any Behat scenario 💃

@probot-autolabeler probot-autolabeler bot added Docker Docker-related issues and PRs. Maintenance CI configurations, READMEs, releases, etc. labels Jul 31, 2023
@TheMilek TheMilek changed the base branch from 1.9 to 1.12 July 31, 2023 12:37
@TheMilek TheMilek dismissed Zales0123’s stale review July 31, 2023 12:37

The base branch was changed.

@github-actions
Copy link

github-actions bot commented Jul 31, 2023

Bunnyshell Preview Environment deleted

Available commands:

  • /bns:deploy to redeploy the environment

@GSadee GSadee merged commit 69798e4 into Sylius:1.12 Aug 3, 2023
25 checks passed
@GSadee
Copy link
Member

GSadee commented Aug 3, 2023

Thanks, @kayue! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bugs or bugfixes. Docker Docker-related issues and PRs. Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants