[Adjustment] Add more details to adjustment and make shipment adjustable #12116

GSadee · 2020-12-02T12:18:56Z

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

This PR is the first step to solve the problem with too little information while generating documents on InvoicingPlugin and RefundPlugin.

I've added also information about shipment tax on order show page in admin panel:

example with tax excluded:

example with tax included:

TODO:

Add array of details on Adjustment entity
Make Shipment entity adjustable
Prepare a proper migration to fill details on existing adjustments
Prepare a proper migration to add related adjustments to existing shipments
Consider adding a note about above migrations in UPGRADE file

src/Sylius/Bundle/CoreBundle/Migrations/Version20201130071338.php

src/Sylius/Bundle/OrderBundle/Resources/config/doctrine/model/Adjustment.orm.xml

lchrusciel · 2020-12-03T12:08:39Z

src/Sylius/Component/Core/OrderProcessing/ShippingChargesProcessor.php

+                $adjustment->setDetails([
+                    'shippingMethodCode' => $shippingMethod->getCode(),
+                    'shippingMethodName' => $shippingMethod->getName(),
+                ]);


Just a thought for improvement. What do you think about some factory service, that would be responsible for this array creation? Seems to provide a more easier way to customize its structure.

I agree with the need to freeze data format for details, but external service to handle internal state of model is a bit of anti-pattern. (Something along the lines of anemic domain model)

Model should be able to handle format on it's own, for example like so

$adjustment->setShippingMethodDetails($shippingMethod->getCode(), $shippingMethod->getName());

// Adjustment public function setShippingMethodDetails(string $shippingMethodCode, string $shippingMethodName): void { $this->details['shippingMethodCode'] = $shippingMethodCode; $this->details['shippingMethodName'] = $shippingMethodName; }

or alternatively as set of value objects.

lchrusciel · 2020-12-03T12:11:37Z

src/Sylius/Component/Core/Taxation/Applicator/OrderShipmentTaxesApplicator.php

+            [
+                'shippingMethodCode' => $shippingMethod->getCode(),
+                'shippingMethodName' => $shippingMethod->getName(),
+                'taxRateCode' => $taxRate->getCode(),
+                'taxRateName' => $taxRate->getName(),
+                'taxRateAmount' => $taxRate->getAmount(),
+            ]


If we would introduce detailed factories, this structure seems like the result of merging two previous factories.

In such aa situation, some recommendation for prefixes could be also a good idea.

@lchrusciel

Alternative solution,

$adjustment->setDetailsFromValueObjects( new ShippingMethodVO($shippingMethod->getCode(), $shippingMethod->getName()), new TaxRateVO($taxRate->getCode(), $taxRate->getName(), $taxRate->getAmount()) );

Which will allow to specify new data more easily and offer extension point for developers.

For example, I've implemented payment method charges custom feature before and currently doing country of origin for Tax Rate, which will require new custom fields.

But regardless of this two examples, I'm sure there are whole CS patterns on how to store/retrieve complex data in array field.

for sure, what we've used is far from the ideal pattern, you are correct. We will think about alternatives. Hopefully, this details field will be good enough for now

lchrusciel · 2020-12-03T12:12:41Z

src/Sylius/Component/Order/Model/Adjustment.php

@@ -49,6 +49,9 @@ class Adjustment implements AdjustmentInterface
    /** @var string|null */
    protected $originCode;

+    /** @var array */


I'm wondering if this value cannot be empty due to migration chages

As I wrote in the description, there should be probably prepared a proper migration to fill this array for existing adjustments, so it should resolve this problem.

vvasiloi · 2020-12-04T11:20:19Z

Have you guys ever thought that maybe you ask too much from this adjustment and maybe it would be better to split it into separate entities?
Adding a free-form field to store random data isn't going to improve the DX and might even make it worse.
If you are interested I can elaborate more on the issues I see with this approach and suggest others.

GSadee · 2020-12-04T11:43:15Z

Have you guys ever thought that maybe you ask too much from this adjustment and maybe it would be better to split it into separate entities?
Adding a free-form field to store random data isn't going to improve the DX and might even make it worse.
If you are interested I can elaborate more on the issues I see with this approach and suggest others.

Hey @vvasiloi! Thank you for sharing your doubts with us. I wanted to propose some simple and non-bc breaks solution for a known problem and I'm of course aware, that it is probably not the best approach in the end. So If you would like to share your suggestions with us, it would be really appreciated 😃

vvasiloi · 2020-12-08T14:58:29Z

@GSadee Thanks for the answer! Good to know that you also thought about it. I will open a new issue for it when I have the time. Hopefully we can do something about it in 2.0.

src/Sylius/Bundle/CoreBundle/Migrations/Version20201208105207.php

UPGRADE-1.9.md

…nd taxes

SirDomin · 2020-12-09T11:04:00Z

src/Sylius/Bundle/CoreBundle/Migrations/Version20201208105207.php

+                FROM sylius_adjustment adjustment
+                LEFT JOIN sylius_shipment shipment ON shipment.order_id = adjustment.order_id
+                LEFT JOIN sylius_shipping_method shipping_method on shipment.method_id = shipping_method.id
+                LEFT JOIN sylius_tax_rate tax_rate on adjustment.label LIKE CONCAT(tax_rate.name, \'%\')


Thats the way to find tax_rate based on adjustment label

lchrusciel

Some additional review and manual QA with some data in database would be needed here as well

UPGRADE-1.9.md

Co-authored-by: Grzegorz Sadowski <sadowskigp@gmail.com>

lchrusciel · 2020-12-11T08:12:28Z

Thank you, Grzegorz! 🥇

…rti0090) This PR was merged into the 1.9-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | master | Bug fix? | no/yes | Related tickets | This PR is related to failing build from sylius/standard Sylius/Sylius-Standard#459 and also #12116 | License | MIT Commits ------- 85f5b34 update upgrade file after changes with tax adjustment 9b0973a minor fix

This PR was merged into the 1.9-dev branch. Discussion ---------- fix for changes related to Sylius/Sylius#12116 Commits ------- 2a9868b fix for new adjustment migration

This PR was merged into the 1.9-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | after #12116 and associated with Sylius/RefundPlugin#242 | License | MIT Commits ------- 066364b [Behat] Reproduce error with dirty entity c849cde [OrderProcessor] Fix clearing tax adjustments 2060b09 [Behat] Add different scenario to cover issue with clearing tax adjustments

…adee) This PR was merged into the 1.9-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | master | Bug fix? | yes? | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | after #12116 | License | MIT According to Sylius/RefundPlugin#242 (comment) Commits ------- 60cc71d [Migrations] Use adding SQL instead of query execution

This PR was merged into the 1.9-dev branch. Discussion ---------- fix for changes related to Sylius/Sylius#12116 Commits ------- 2a9868bf93754fc1c653305e3194d30e338a40a9 fix for new adjustment migration

GSadee added RFC Discussions about potential changes or new features. DX Issues and PRs aimed at improving Developer eXperience. labels Dec 2, 2020

GSadee requested a review from a team as a code owner December 2, 2020 12:18

probot-autolabeler bot added the Admin AdminBundle related issues and PRs. label Dec 2, 2020

lchrusciel reviewed Dec 3, 2020

View reviewed changes

GSadee force-pushed the adjustments-details branch 2 times, most recently from c6fd1d7 to 2e3835f Compare December 4, 2020 10:25

GSadee force-pushed the adjustments-details branch from 2e3835f to d79fb74 Compare December 4, 2020 12:24

probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Dec 8, 2020

SirDomin force-pushed the adjustments-details branch from 16aed39 to c040f27 Compare December 8, 2020 14:41

SirDomin changed the title ~~[WIP][PoC][Adjustment] Add more details to adjustment and make shipment adjustable~~ [PoC][Adjustment] Add more details to adjustment and make shipment adjustable Dec 8, 2020

SirDomin force-pushed the adjustments-details branch from c040f27 to dc884e9 Compare December 8, 2020 14:44

AdamKasp reviewed Dec 8, 2020

View reviewed changes

SirDomin force-pushed the adjustments-details branch 2 times, most recently from 3727236 to 42164bd Compare December 8, 2020 15:31

GSadee commented Dec 9, 2020

View reviewed changes

src/Sylius/Bundle/CoreBundle/Migrations/Version20201208105207.php Outdated Show resolved Hide resolved

src/Sylius/Bundle/CoreBundle/Migrations/Version20201208105207.php Outdated Show resolved Hide resolved

lchrusciel reviewed Dec 9, 2020

View reviewed changes

UPGRADE-1.9.md Outdated Show resolved Hide resolved

GSadee and others added 10 commits December 9, 2020 12:00

[Adjustment] Add details array to adjustment entity

517a229

[Adjustment] Fill details of adjustment during order processor

cf4fe3f

[Adjustment] Remove unnecesary constants

c563158

[Shipment] Make shipment adjustable entity

90a94da

[Shipment][Adjustment] Adjust adding adjustments to shipments

5f44b62

[Admin][Order] Refactor displaying adjustments for shipment charges a…

a62696c

…nd taxes

Fixes after Psalm analysis

c071c8a

Fixes for PHPUnit tests

cc1cf25

[Adjustment] Change details type from array to json

3877b38

[Migration] Create migration

cd7c687

SirDomin added 6 commits December 9, 2020 12:00

[Migration] Add SQL to fill adjustment data

35efcf3

[Migration] Default details value added

3c9f7d3

[UPGRADE] Add migration info to Upgrade file

75a86ea

[Static Check] fix static check problems

2538862

pr-fix

b27aad4

[Migrations] added support to tax adjustmentType

522c88f

SirDomin force-pushed the adjustments-details branch from 42164bd to 522c88f Compare December 9, 2020 11:01

SirDomin reviewed Dec 9, 2020

View reviewed changes

[UPGRADE] clarify upgrade file

ce0c452

lchrusciel approved these changes Dec 10, 2020

View reviewed changes

UPGRADE-1.9.md Outdated Show resolved Hide resolved

GSadee commented Dec 11, 2020

View reviewed changes

UPGRADE-1.9.md Outdated Show resolved Hide resolved

UPGRADE-1.9.md Outdated Show resolved Hide resolved

[UPGRADE] Minor structure and semantic changes

58d358c

Co-authored-by: Grzegorz Sadowski <sadowskigp@gmail.com>

lchrusciel changed the title ~~[PoC][Adjustment] Add more details to adjustment and make shipment adjustable~~ [Adjustment] Add more details to adjustment and make shipment adjustable Dec 11, 2020

lchrusciel merged commit 62e869b into Sylius:master Dec 11, 2020

This was referenced Dec 16, 2020

fix for new adjustment migration Sylius/Sylius-Standard#459

Merged

update upgrade file after changes with tax adjustment #12164

Merged

GSadee deleted the adjustments-details branch January 8, 2021 11:16

GSadee mentioned this pull request Jan 14, 2021

[OrderProcessor] Fix clearing tax adjustments #12246

Merged

This was referenced Jan 18, 2021

Refunding shipments with taxes Sylius/RefundPlugin#242

Merged

[Migrations] Use adding SQL instead of query execution #12262

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Adjustment] Add more details to adjustment and make shipment adjustable #12116

[Adjustment] Add more details to adjustment and make shipment adjustable #12116

GSadee commented Dec 2, 2020 •

edited by SirDomin

Loading

lchrusciel Dec 3, 2020

diimpp Feb 25, 2021

lchrusciel Dec 3, 2020

diimpp Feb 25, 2021 •

edited

Loading

lchrusciel Apr 19, 2021

lchrusciel Dec 3, 2020

GSadee Dec 4, 2020

vvasiloi commented Dec 4, 2020 •

edited

Loading

GSadee commented Dec 4, 2020

vvasiloi commented Dec 8, 2020

SirDomin Dec 9, 2020

lchrusciel left a comment

lchrusciel commented Dec 11, 2020

[Adjustment] Add more details to adjustment and make shipment adjustable #12116

[Adjustment] Add more details to adjustment and make shipment adjustable #12116

Conversation

GSadee commented Dec 2, 2020 • edited by SirDomin Loading

lchrusciel Dec 3, 2020

Choose a reason for hiding this comment

diimpp Feb 25, 2021

Choose a reason for hiding this comment

lchrusciel Dec 3, 2020

Choose a reason for hiding this comment

diimpp Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

lchrusciel Apr 19, 2021

Choose a reason for hiding this comment

lchrusciel Dec 3, 2020

Choose a reason for hiding this comment

GSadee Dec 4, 2020

Choose a reason for hiding this comment

vvasiloi commented Dec 4, 2020 • edited Loading

GSadee commented Dec 4, 2020

vvasiloi commented Dec 8, 2020

SirDomin Dec 9, 2020

Choose a reason for hiding this comment

lchrusciel left a comment

Choose a reason for hiding this comment

lchrusciel commented Dec 11, 2020

GSadee commented Dec 2, 2020 •

edited by SirDomin

Loading

diimpp Feb 25, 2021 •

edited

Loading

vvasiloi commented Dec 4, 2020 •

edited

Loading