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

[Bug] Provider\RemainingTotalProvider meant to work by ShipmentRefund, but in practice works by shipment Adjustment #286

Open
diimpp opened this issue Apr 23, 2021 · 1 comment
Labels
DX Issues and PRs aimed at improving Developer eXperience. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).

Comments

@diimpp
Copy link
Member

diimpp commented Apr 23, 2021

Provider\RemainingTotalProvider is used to get available amount for refund for both OrderItemUnitRefund and ShipmentRefund.

That's the theory, in practice Validator\RefundAmountValidator expects to be working with Model\ShipmentRefund, which is correct, but order refund page _shipping.html.twig works with shipping Adjustments which is incorrect and source codes of Provider\RemainingTotalProvider were hacked for UI benefit.

    $shippingAdjustment = $this->adjustmentRepository->findOneBy([
            'id' => $id,
            'type' => AdjustmentInterface::SHIPPING_ADJUSTMENT,
        ]);
        Assert::notNull($shippingAdjustment);

So problem is public function getTotalLeftToRefund(int $id, RefundType $type): int expects one kind of $id and gets totally another and underling code was hacked to support adjustments usage.

Maybe this was conscious decision due to shipment and adjustments not being related in prior 1.9, but it should have been handled explicitly in such case.

Solution is to refactor UI to work with shipments directly.

Personally I've encountered this at non-standard use-case, while loading historical data in DB, so there might be no active bugs with standard usage.

@diimpp diimpp changed the title [Bug] Provider\RemainingTotalProvider meant to be working by ShipmentRefund, but in practise works by shipment Adjustment [Bug] Provider\RemainingTotalProvider meant to work by ShipmentRefund, but in practise works by shipment Adjustment Apr 23, 2021
@diimpp diimpp changed the title [Bug] Provider\RemainingTotalProvider meant to work by ShipmentRefund, but in practise works by shipment Adjustment [Bug] Provider\RemainingTotalProvider meant to work by ShipmentRefund, but in practice works by shipment Adjustment Apr 23, 2021
@GSadee
Copy link
Member

GSadee commented Apr 29, 2021

Hello @diimpp, thank you for opening an issue and your thoughts.

As you wrote, I guess this was a conscious decision due to shipment and adjustments not being related then. But I agree, that the current solution with using shipping adjustments instead of shipments doesn't seem to be the best one and it is certainly not the expected behaviour.

However, I'm not sure if it is a bug, or just sth for future improvement that could be quite difficult to refactor due to keeping backward compatibility.

@GSadee GSadee added DX Issues and PRs aimed at improving Developer eXperience. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). labels Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Issues and PRs aimed at improving Developer eXperience. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

No branches or pull requests

2 participants