[CreditMemo] Rework credit memo #179

GSadee · 2020-01-23T07:55:55Z

features/seeing_details_of_credit_memo.feature

features/seeing_details_of_credit_memo_with_promotion_applied.feature

Resolved

features/having_credit_memo_generated.feature

lchrusciel · 2020-01-28T10:04:15Z

src/Converter/LineItemsConverter.php

+
+        $label = $taxAdjustments->first()->getLabel();
+
+        if (preg_match('#\((.*?)\)#', $label, $matches)) {


o.O What has happened here?

I don't have an idea what we try to achieve here.

This is the simplest way to get tax rate from tax adjustment, as it is set in tax rate label, I was wondering if it is ok for now 😃

lchrusciel · 2020-01-28T10:11:18Z

src/Generator/CreditMemoGenerator.php

-        CreditMemoUnitGeneratorInterface $orderItemUnitCreditMemoUnitGenerator,
-        CreditMemoUnitGeneratorInterface $shipmentCreditMemoUnitGenerator,
+        LineItemsConverterInterface $lineItemsConverter,
+        LineItemsConverterInterface $shipmentLineItemsConverter,


As the order of these services is important, I would recommend creating two interfaces instead of one. Otherwise, errors may be really misleading (when units would be passed to shipmentLineItemsConvertera and shipments to lineItemsConverter)

I'm not convinced to that, the input and output are the same and having the same interface is logical to me

Maybe the best would be the queue with priorities accepting only array of specific units 💃 But I would leave it as it is right now

src/Resources/config/doctrine/CreditMemo.orm.xml

lchrusciel · 2020-01-28T10:16:57Z

src/Resources/views/Download/creditMemo.html.twig

+                <td>{{ loop.index }}</td>
+                <td>{{ item.name }}</td>
+                <td>{{ item.quantity }}</td>
+                <td>{{ (item.unitNetPrice/100)|number_format(2) }}</td>


Why didn't you used sylius_format_money?

Because I want to have prices without currency 😢

So the other question - why we want here prices without currency? 😄

Because credit memo should look like this 😃 /cc @CoderMaggie

Confirmed by PO 👩‍⚖

tests/Behat/Context/Transform/PriceContext.php

UPGRADE.md

pamil · 2020-01-28T10:02:52Z

migrations/Version20200125182414.php

+        $this->addSql('CREATE TABLE sylius_refund_line_item (id INT AUTO_INCREMENT NOT NULL, name VARCHAR(255) NOT NULL, quantity INT NOT NULL, unit_net_price INT NOT NULL, unit_gross_price INT NOT NULL, net_value INT NOT NULL, gross_value INT NOT NULL, tax_amount INT NOT NULL, tax_rate VARCHAR(255) DEFAULT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET UTF8 COLLATE `UTF8_unicode_ci` ENGINE = InnoDB');
+        $this->addSql('ALTER TABLE sylius_refund_credit_memo_line_items ADD CONSTRAINT FK_1453B90E8E574316 FOREIGN KEY (credit_memo_id) REFERENCES sylius_refund_credit_memo (id)');
+        $this->addSql('ALTER TABLE sylius_refund_credit_memo_line_items ADD CONSTRAINT FK_1453B90EA7CBD339 FOREIGN KEY (line_item_id) REFERENCES sylius_refund_line_item (id)');
+        $this->addSql('ALTER TABLE sylius_refund_credit_memo DROP units');


Could we migrate those units to line items?

IMO no, because I do not have enough information and also the old credit memos shouldn't be changed

They'll be changed after we drop this column though

src/Converter/LineItemsConverterInterface.php

src/Converter/LineItemsConverter.php

src/Entity/LineItem.php

src/Generator/TaxItemsGeneratorInterface.php

src/Resources/views/Download/creditMemo.html.twig

tests/Behat/Context/Transform/PriceContext.php

features/refunding_part_of_order_unit.feature

spec/Entity/CreditMemoSpec.php

Zales0123 · 2020-01-29T09:42:14Z

src/Converter/LineItemsConverter.php

+
+            $grossValue = $unitRefund->total();
+            $taxAmount = (int) ($grossValue * $orderItemUnit->getTaxTotal() / $orderItemUnit->getTotal());
+            $netValue = $grossValue - $taxAmount;


I feel that there is a lot of things we could extract there - even if they're done only there, it still does not seem nice 😢 Maybe we could have some VO like CreditMemoValues which would calculate these variables? Or some LineItemFactory? It's not a must-have, for now, for me, but would definitely improve a DX 🖖

For now, I've extracted creating line item to the separate private method, but I agree that in future some refactor could improve a DX

src/Converter/LineItemsConverterInterface.php

src/Entity/LineItem.php

Zales0123 · 2020-01-29T09:49:14Z

src/Generator/CreditMemoGenerator.php

-        CreditMemoUnitGeneratorInterface $orderItemUnitCreditMemoUnitGenerator,
-        CreditMemoUnitGeneratorInterface $shipmentCreditMemoUnitGenerator,
+        LineItemsConverterInterface $lineItemsConverter,
+        LineItemsConverterInterface $shipmentLineItemsConverter,


Maybe the best would be the queue with priorities accepting only array of specific units 💃 But I would leave it as it is right now

src/Converter/LineItemsConverter.php

Zales0123 · 2020-01-30T07:55:37Z

src/Converter/LineItemsConverter.php

+
+        $label = $taxAdjustments->first()->getLabel();
+
+        if (preg_match('/\((.*?)\)/', $label, $matches)) {


We should definitely think how to handle it better (either here on on Sylius/Sylius) but let's just open an issue for that :)

pamil · 2020-01-30T12:54:44Z

Thanks, Grzegorz! 🎉

…adee) This PR was merged into the 1.0-dev branch. Discussion ---------- After #179 Commits ------- 146c371 Extract tax rate provider from line items converter 6af2b26 Fixes for tax rate provider after PR review

[Behat][CreditMemo] Update scenarios for generating credit memos

76445f2

GSadee added the Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). label Jan 23, 2020

GSadee requested a review from a team as a code owner January 23, 2020 07:55

CoderMaggie previously requested changes Jan 23, 2020

View reviewed changes

lchrusciel approved these changes Jan 23, 2020

View reviewed changes

[Behat][CreditMemo] Scenarios fixes after PR review

a34b8ad

GSadee force-pushed the rework-credit-memo-template branch from f92af2c to 3eccda5 Compare January 27, 2020 08:27

GSadee changed the title ~~[WIP][CreditMemo] Rework credit memo~~ [CreditMemo] Rework credit memo Jan 27, 2020

GSadee force-pushed the rework-credit-memo-template branch 2 times, most recently from 05d57f7 to 45c0c62 Compare January 27, 2020 08:57

GSadee added 2 commits January 27, 2020 10:06

[CreditMemo] Change credit memo units to be resource line items

ad5c265

[CreditMemo] Add notes to UPGRADE file

e6c7706

GSadee force-pushed the rework-credit-memo-template branch from 45c0c62 to e6c7706 Compare January 27, 2020 09:06

[Behat][CreditMemo] Fixes

981220d

GSadee force-pushed the rework-credit-memo-template branch from b9a6f57 to 981220d Compare January 27, 2020 12:21

CoderMaggie self-requested a review January 27, 2020 14:36

Tomanhez approved these changes Jan 28, 2020

View reviewed changes

lchrusciel reviewed Jan 28, 2020

View reviewed changes

pamil suggested changes Jan 28, 2020

View reviewed changes

GSadee force-pushed the rework-credit-memo-template branch from 598c940 to ee91d19 Compare January 29, 2020 09:10

Zales0123 reviewed Jan 29, 2020

View reviewed changes

GSadee force-pushed the rework-credit-memo-template branch 4 times, most recently from f9869ef to b7a1869 Compare January 29, 2020 14:45

[CreditMemo] Fixes for reworking credit memos after PR review

f62c77d

GSadee force-pushed the rework-credit-memo-template branch from b7a1869 to f62c77d Compare January 29, 2020 14:52

Zales0123 approved these changes Jan 30, 2020

View reviewed changes

pamil approved these changes Jan 30, 2020

View reviewed changes

pamil merged commit 757c53f into Sylius:master Jan 30, 2020

GSadee deleted the rework-credit-memo-template branch January 30, 2020 12:57

GSadee mentioned this pull request Jan 31, 2020

Extract tax rate provider from line items converter #181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CreditMemo] Rework credit memo #179

[CreditMemo] Rework credit memo #179

GSadee commented Jan 23, 2020 •

edited

Loading

lchrusciel Jan 28, 2020

GSadee Jan 28, 2020 •

edited

Loading

lchrusciel Jan 28, 2020

GSadee Jan 28, 2020

Zales0123 Jan 29, 2020

lchrusciel Jan 28, 2020

GSadee Jan 28, 2020

Zales0123 Jan 29, 2020

GSadee Jan 29, 2020

CoderMaggie Jan 30, 2020

pamil Jan 28, 2020

GSadee Jan 28, 2020

pamil Jan 30, 2020

Zales0123 Jan 29, 2020

GSadee Jan 29, 2020

Zales0123 Jan 29, 2020

Zales0123 Jan 30, 2020

pamil commented Jan 30, 2020


		$label = $taxAdjustments->first()->getLabel();

		if (preg_match('#\((.*?)\)#', $label, $matches)) {


		$label = $taxAdjustments->first()->getLabel();

		if (preg_match('/\((.*?)\)/', $label, $matches)) {

[CreditMemo] Rework credit memo #179

[CreditMemo] Rework credit memo #179

Conversation

GSadee commented Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

GSadee Jan 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pamil commented Jan 30, 2020

GSadee commented Jan 23, 2020 •

edited

Loading

GSadee Jan 28, 2020 •

edited

Loading