Rework invoice template on pdf and admin show page #234

Tomanhez · 2021-07-14T13:17:09Z

PDF:
Before:

After:

Show Page:
Before:

After:

GSadee

The invoice doesn't have the same columns as you have described in the task, I would be for changing them to be exactly the same.

What is more, there are missing tests for cases like:

items with a promotion applied
shipments with taxes
shipments with a promotion applied
shipments with taxes and a promotion applied

I also wonder if a view of invoice show should be the same as PDF file, it wasn't in the scope of task, so I'm not convinced to changing that in this PR 🤔 /cc @CoderMaggie

features/managing_invoices/seeing_invoices_when_items_has_promitions_applied.feature

features/managing_invoices/seeing_invoices_when_shipment_has_promotions_applied.feature

src/Resources/views/Invoice/invoice.html.twig

src/Resources/views/Invoice/show.html.twig

tests/Behat/Page/Admin/Invoice/ShowPage.php

features/managing_invoices/seeing_invoices_when_items_has_promitions_applied.feature

features/managing_invoices/seeing_invoices_when_shipment_has_promotions_applied.feature

features/managing_invoices/seeing_invoices_when_shipment_has_taxes.feature

...res/managing_invoices/seeing_invoices_when_shipment_has_taxes_and_promotions_applied.feature

features/managing_invoices/seeing_an_invoice_with_items_having_promotion_applied.feature

src/Entity/LineItem.php

spec/Provider/TaxRateProviderSpec.php

features/managing_invoices/seeing_an_invoice_with_shipment_having_promotion_applied.feature

features/managing_invoices/seeing_an_invoice_with_shipment_having_taxes_applied.feature

...anaging_invoices/seeing_an_invoice_with_items_and_shipments_having_promotion_applied.feature

...managing_invoices/seeing_an_invoice_with_shipment_having_taxes_and_promotion_applied.feature

features/managing_invoices/seeing_invoice.feature

I've already fixed that

CoderMaggie

The "Name" column should be aligned to left, not right :)

GSadee · 2021-07-30T04:55:26Z

The "Name" column should be aligned to left, not right :)

Done 👌🏻

src/Resources/views/Invoice/Download/pdf.html.twig

arti0090 · 2021-07-30T10:18:57Z

tests/Behat/Context/Ui/Admin/ManagingInvoicesContext.php

+    /**
+     * @Then it should have :quantity :name item with unit price :unitPrice, net value :netValue, tax total :taxTotal and total :total in :currencyCode currency
+     * @Then it should have :quantity :name items with unit price :unitPrice, net value :netValue, tax total :taxTotal and total :total in :currencyCode currency
+     */


Wouldn't here this funny structure with ?:s ( I don't remember it exactly :D ) work?

You are right, I would use the structure with optional s, but then I would have to change the whole definition to a regexp, which in my opinion is less readable with so many arguments

👌 I will stay with Team readable then 😄

tests/Behat/Page/Admin/Invoice/ShowPage.php

spec/Converter/OrderItemUnitsToLineItemsConverterSpec.php

spec/Provider/TaxRatePercentageProviderSpec.php

src/Generator/InvoiceGenerator.php

Zales0123 · 2021-07-30T13:25:15Z

src/Provider/TaxRatePercentageProvider.php

+        }
+
+        if ($taxAdjustments->isEmpty()) {
+            return null;


Is there any reason why we throw an exception in one undesired situation (more tax adjustments than one), but return a null in another (no tax adjustments at all)?

no tax adjustment seems like totally ok situation. I'm considering if two tax adjustments are such an edge case, but I'm fine with this decision

No tax adjustment is the normal case, but more than 2 tax adjustments is the case not supported by default by Sylius, so this is the reason why this exception is thrown here. What is more, there is a similar behaviour in RefundPlugin.

Doubled taxation is not a particularly not supported feature of the main Sylius. We already support both included and excluded taxes on order items. Therefore it may happen. Nonetheless, I'm still fine with it for the first iteration. Especially, as RefundPlugin works in similar fashion

src/Resources/views/Invoice/show.html.twig

src/Twig/PercentFromLabelExtension.php

Zales0123 · 2021-07-30T13:35:28Z

features/managing_invoices/seeing_invoice_with_items_having_promotion_applied.feature

@@ -0,0 +1,30 @@
+@managing_invoices
+Feature: Seeing an invoice with items and shipment having promotion applied
+    In order to have items with proper amounts


I don't want to be picky 😄 But I feel we should work over these features descriptions a little bit 🚀 Having proper amounts is not a goal in the essence, it's more a way to... what? Not having legal problems? 🤯 I can leave with it, but I know we can do better 😄

Yup, you're 😄 For me, the intention of Administrator to have proper amounts on the invoice is enough, but feel free to refactor these features 😃

lchrusciel · 2021-08-02T11:09:33Z

spec/Converter/ShippingAdjustmentsToLineItemsConverterSpec.php

+        $this->shouldImplement(LineItemsConverterInterface::class);
+    }
+
+    function it_extracts_line_items_from_orders_shipping_adjusments(


We could cover a few more cases here, like lack of tax adjustment or having two of them.

Zales0123 · 2021-08-02T11:24:40Z

Thank you, Tomasz and Grzegorz! 🎉

Tomanhez requested a review from a team as a code owner July 14, 2021 13:17

GSadee requested changes Jul 15, 2021

View reviewed changes

Zales0123 reviewed Jul 20, 2021

View reviewed changes

GSadee reviewed Jul 20, 2021

View reviewed changes

Tomanhez force-pushed the rework-invoice-template branch 2 times, most recently from 3d5841d to c7f12b2 Compare July 21, 2021 07:18

GSadee reviewed Jul 21, 2021

View reviewed changes

Tomanhez force-pushed the rework-invoice-template branch 16 times, most recently from 45ef97b to 6539dc5 Compare July 22, 2021 09:07

GSadee reviewed Jul 22, 2021

View reviewed changes

Tomanhez force-pushed the rework-invoice-template branch 6 times, most recently from 23205ec to 65cb889 Compare July 22, 2021 11:36

GSadee force-pushed the rework-invoice-template branch 3 times, most recently from 3c14b49 to 1c2fdb8 Compare July 29, 2021 11:42

GSadee added 2 commits July 29, 2021 13:53

[LineItem] Fixes to line items converter refactor after PR review

bae8fa4

[LineItem] Add taxRate to LineItem mapping

bcdd5d5

GSadee force-pushed the rework-invoice-template branch from 1c2fdb8 to 00d7b2c Compare July 29, 2021 11:53

GSadee added 4 commits July 29, 2021 14:01

[Invoice] Rework PDF file and show page templates

1cc6f7e

[TaxRate] Change name of tax rate percentage provider

d31c1cd

[LineItem] Split converting to line items into 2 separate services

9905b68

Static analysis fixes

c3494fa

GSadee force-pushed the rework-invoice-template branch from 00d7b2c to 7a19695 Compare July 29, 2021 12:13

GSadee force-pushed the rework-invoice-template branch from 7a19695 to 2a8ceb0 Compare July 29, 2021 12:28

[Behat] Adjust scenarios and show page

ee8ea4e

GSadee force-pushed the rework-invoice-template branch from 2a8ceb0 to ee8ea4e Compare July 29, 2021 12:36

CoderMaggie reviewed Jul 29, 2021

View reviewed changes

lchrusciel approved these changes Jul 30, 2021

View reviewed changes

arti0090 reviewed Jul 30, 2021

View reviewed changes

[Invoice] Minor PDF improvements

10d34a0

GSadee force-pushed the rework-invoice-template branch from 739088d to 10d34a0 Compare July 30, 2021 10:32

arti0090 approved these changes Jul 30, 2021

View reviewed changes

Zales0123 reviewed Jul 30, 2021

View reviewed changes

SirDomin approved these changes Aug 2, 2021

View reviewed changes

Improvements after review a PR with invoice refactor

ced4305

GSadee force-pushed the rework-invoice-template branch from e0b0893 to ced4305 Compare August 2, 2021 10:30

lchrusciel approved these changes Aug 2, 2021

View reviewed changes

Zales0123 approved these changes Aug 2, 2021

View reviewed changes

Zales0123 merged commit 2ef31f2 into Sylius:master Aug 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework invoice template on pdf and admin show page #234

Rework invoice template on pdf and admin show page #234

Tomanhez commented Jul 14, 2021 •

edited by GSadee

Loading

GSadee left a comment

CoderMaggie left a comment

GSadee commented Jul 30, 2021

arti0090 Jul 30, 2021

GSadee Jul 30, 2021

arti0090 Jul 30, 2021

Zales0123 Jul 30, 2021

lchrusciel Jul 30, 2021

GSadee Aug 2, 2021

lchrusciel Aug 2, 2021

Zales0123 Jul 30, 2021

GSadee Aug 2, 2021

lchrusciel Aug 2, 2021

Zales0123 commented Aug 2, 2021 •

edited

Loading

Rework invoice template on pdf and admin show page #234

Rework invoice template on pdf and admin show page #234

Conversation

Tomanhez commented Jul 14, 2021 • edited by GSadee Loading

GSadee left a comment

Choose a reason for hiding this comment

CoderMaggie left a comment

Choose a reason for hiding this comment

GSadee commented Jul 30, 2021

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

Zales0123 commented Aug 2, 2021 • edited Loading

Tomanhez commented Jul 14, 2021 •

edited by GSadee

Loading

Zales0123 commented Aug 2, 2021 •

edited

Loading