Allow to hide zero tax rows via plugin #3484

LiaraAlis · 2022-08-23T08:26:34Z

Description

This PR adds some twig blocks in the invoice templates. Additionally it hides subtotal and tax rows in the table when no tax is used.
For more information see original PR: #3480

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

I verified that my code applies to the guidelines (composer code-check)
I updated the documentation (see here)
I agree that this code is used in Kimai and will be published under the MIT license

… no tax is charged

kevinpapst

Thank you @LiaraAlis 👍

I found many small things that I would like to smooth out, mostly block names which should be consistent between export and invoice templates.

My review is only for the first template, can you adapt the other ones accordingly with the same changes (search and replace should work if you used the same names).

templates/invoice/renderer/default-pdf.pdf.twig

templates/invoice/renderer/default.html.twig

LiaraAlis · 2022-08-23T10:10:37Z

Should we also add a empty pdf_header block in the export template to match the invoice templates?

LiaraAlis · 2022-08-26T16:07:03Z

@kevinpapst Can you please check my comments above? After that I can complete your change requests. 😁

kevinpapst · 2022-08-27T13:14:44Z

You know what, I am a little afraid of the merge problems that will show pushen merging this PR.
Not sure if I mentioned it before, I am working on a massive rewrite of Kimai: see #2902

I see three options how to move forward:

you try to merge your changes with this linked branch to see if merging is easy possible
you only add the necessary changes for your feature in here and
you move all optional changes to the other branch

I know this is annoying, but I try to keep the changes as small as possible currently to avoid further merge issues.
Do you have any opinion on this?

LiaraAlis · 2022-08-29T17:48:20Z

I totally understand this.
I tried to merge it into the v2 branch. It is not possible without conflicts. I can do it manually without problems, because the conflicts are very easy to solve, the changes are not big.
If you want I can also create a PR for v2 branch?

kevinpapst

Okay, I really love your input in the PRs and hate being a pain in the ass... but I have to reject this for now. Can you re-target the changes to #2902 instead and ONLY apply the minimum necessary changes here.

I am fine with the blocks for the tax = 0 change, but everything else doesn't feel right.

The problem are the pretty generic block names which might to layout issues in existing templates. I searched through the library of my customers templates and found some duplicates already eg block items.

I simply cannot guarantee that these new blocks will not lead to some weird BC breaks, which should be avoided at all cost.

So my proposal:

target the v2 branch (the three comments are for it)
add only the tax=0 change only in this PR

kevinpapst · 2022-08-30T15:22:51Z

templates/export/pdf-layout.html.twig

 <!--mpdf
+{% block pdf_footer %}


This needs to be reverted.
If someone used this block (for some weird reason) to render HTML instead of mpdf tags, then this change will break their templates. Also (just a personal preference) I think it is better to create self-containing blocks.

And I am not sure what happens if someone has overwritten these blocks and then they end up with two opening mpdf tags.

What exactly do you mean with "self-containing blocks"?

kevinpapst · 2022-08-30T15:23:05Z

templates/export/pdf-layout.html.twig

 {% endblock %}
+mpdf-->


kevinpapst · 2022-08-30T15:24:18Z

templates/invoice/renderer/default-pdf.pdf.twig

 </head>
 <body>
 <!--mpdf
+{% block pdf_header %}


can we do the same here and create self-contained blocks with the start/end mpdf tag starting inside the block.

LiaraAlis · 2022-08-30T16:16:36Z

Okay, I really love your input in the PRs and hate being a pain in the ass... but I have to reject this for now. Can you re-target the changes to #2902 instead and ONLY apply the minimum necessary changes here.

I go with you. But I suggest that we go back to the start and think carefully about the topic how the templates in v2 could be structured to make it consistent across all invoice and export templates.
I'm also not happy with these namings and I think it is more elegant to refactor the export and invoice templates in v2.
In my eyes the current templates are too static and inflexible. For example, not even a logo is possible without copying the template to a new one.

For this, to make my plugin better, I need a change for the subtotal and VAT row and also a block before the payment terms. Can we do this?

And a off topic question:
Is there a collection of different invoice templates anywhere? I would like to change my templates but I don't like the default ones.

kevinpapst · 2022-08-30T17:06:45Z

👍 Yes, sure. I am fine with the tax related changes.

And agree with everything else. More eyes and brains that work together usually end in a better result.
I know it could be better... building such a software from scratch will never lead to the best solution initially. And once it is out it is hard to change.

This reverts commit 3eb6f9d.

This reverts commit 1b4cb39.

This reverts commit a1d8017.

This reverts commit 10a912f.

…tax when no tax is charged" This reverts commit 9a5dd76.

LiaraAlis · 2022-09-02T01:56:27Z

@kevinpapst I've updated the PR and reverted all the changes that added blocks. So only the change about the tax is left.
So we should take about the v2 templates later. Should we create an issue for that?

kevinpapst · 2022-09-02T15:16:48Z

So we should take about the v2 templates later. Should we create an issue for that?

Yes please 👍

I tried to get proper information online but I could only find out that you can add a 0,00€ Tax or 0% VAT row without problems (as registered EU VAT company). And that you have to stick to all regular invoice rules, even with reverse charge invoices (which likely means having that row is mandatory).

Honestly: no company ever complained, so I kinda think that this row is no trouble for VAT registered companies.
So I am not sure if that change is legally a risk for some companies / scenarios.

This PR is a never ending story, I am sorry about that! But it is a sensitive topic...

We should probably add a method public function isHideZeroTax(): bool somewhere, which can be triggered by your plugin. That value should be passed down to the templates, so templates can hide the tax row or leave it, depending on that setting.

I will look at it later and send an update.

LiaraAlis · 2022-09-02T22:25:27Z

Yes please 👍

#3510 😁

This PR is a never ending story, I am sorry about that! It is a sensitive topic...

You have not to sorry about that. You are right, I also thought about this and searched. But I cannot find anything about this.
In my eyes, for small businesses printing 0% VAT is misleading. In reality there is not 0% tax, but none at all (or in programming language: 0 != null 😁). But I don't know if this is true. I have to ask a tax consultant, but I do not have someone. 😅

We should probably add a method public function isHideZeroTax(): bool somewhere, which can be triggered by your plugin. That values should be passed down to the templates, so they can hide the tax row or leave it, depending on the setting.

I like the idea. I think this way is the best to lower the risk.

…or not

LiaraAlis · 2022-09-04T01:49:38Z

I've added the property to the InvoiceModel.php. I think it is a good place and I didn't want to add the property to a doctrine model, because it is only a internal property and not stored in database.
I also changed the check in the templates.

(already tested locally with my plugin)

codecov · 2022-09-04T01:55:56Z

Codecov Report

Merging #3484 (143999b) into master (a04b72e) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #3484      +/-   ##
============================================
- Coverage     91.77%   91.76%   -0.02%     
- Complexity     7878     7880       +2     
============================================
  Files           742      742              
  Lines         23760    23764       +4     
============================================
  Hits          21806    21806              
- Misses         1954     1958       +4

Impacted Files	Coverage Δ
src/Invoice/InvoiceModel.php	`94.73% <0.00%> (-4.17%)`	⬇️

kevinpapst

That was a long journey, but I like where it brought us 😄

Thank you @LiaraAlis 👍

Added twig blocks to html and pdf invoice templates and hide tax when…

9a5dd76

… no tax is charged

kevinpapst requested changes Aug 23, 2022

View reviewed changes

kevinpapst added feature request invoice labels Aug 23, 2022

LiaraAlis added 4 commits August 23, 2022 12:12

Fix pdf header + footer blocks

10a912f

Rename recipient and sender blocks

a1d8017

Added empty body_start and body_end blocks

1b4cb39

Add additional empty blocks in items table

3eb6f9d

kevinpapst requested changes Aug 30, 2022

View reviewed changes

LiaraAlis mentioned this pull request Aug 30, 2022

Stylesheet in export pdf layout #3485

Closed

LiaraAlis mentioned this pull request Aug 31, 2022

remove default templates LiaraAlis/kimai2-SmallBusinessRuleBundle#2

Merged

kevinpapst added this to the 1.24 milestone Aug 31, 2022

LiaraAlis and others added 7 commits September 2, 2022 03:41

Merge branch 'kevinpapst:master' into feature/invoice-template-blocks

3aef44a

Revert "Add additional empty blocks in items table"

39cca18

This reverts commit 3eb6f9d.

Revert "Added empty body_start and body_end blocks"

e94d701

This reverts commit 1b4cb39.

Revert "Rename recipient and sender blocks"

97fcedd

This reverts commit a1d8017.

Revert "Fix pdf header + footer blocks"

2169477

This reverts commit 10a912f.

Revert "Added twig blocks to html and pdf invoice templates and hide …

ddb4a73

…tax when no tax is charged" This reverts commit 9a5dd76.

Hide tax rows when no tax is charged

6764c08

LiaraAlis mentioned this pull request Sep 2, 2022

Refactoring invoice/export templates for v2 #3510

Closed

Added property to InvoiceModel to control when zero tax is displayed …

c2b9f35

…or not

Merge branch 'kevinpapst:master' into feature/invoice-template-blocks

143999b

kevinpapst approved these changes Sep 4, 2022

View reviewed changes

kevinpapst merged commit 1d1f583 into kimai:master Sep 4, 2022

kevinpapst changed the title ~~Optimize invoice templates for better customizability (new twig blocks)~~ Allow to hide zero tax rows via plugin Sep 4, 2022

github-actions bot locked and limited conversation to collaborators Mar 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to hide zero tax rows via plugin #3484

Allow to hide zero tax rows via plugin #3484

LiaraAlis commented Aug 23, 2022

kevinpapst left a comment

LiaraAlis commented Aug 23, 2022

LiaraAlis commented Aug 26, 2022

kevinpapst commented Aug 27, 2022

LiaraAlis commented Aug 29, 2022

kevinpapst left a comment •

edited

Loading

kevinpapst Aug 30, 2022

LiaraAlis Aug 30, 2022

kevinpapst Aug 30, 2022

kevinpapst Aug 30, 2022

LiaraAlis commented Aug 30, 2022 •

edited

Loading

kevinpapst commented Aug 30, 2022 •

edited

Loading

LiaraAlis commented Sep 2, 2022

kevinpapst commented Sep 2, 2022 •

edited

Loading

LiaraAlis commented Sep 2, 2022 •

edited

Loading

LiaraAlis commented Sep 4, 2022 •

edited

Loading

codecov bot commented Sep 4, 2022 •

edited

Loading

kevinpapst left a comment

		{% endblock %}
		mpdf-->

Allow to hide zero tax rows via plugin #3484

Allow to hide zero tax rows via plugin #3484

Conversation

LiaraAlis commented Aug 23, 2022

Description

Types of changes

Checklist

kevinpapst left a comment

Choose a reason for hiding this comment

LiaraAlis commented Aug 23, 2022

LiaraAlis commented Aug 26, 2022

kevinpapst commented Aug 27, 2022

LiaraAlis commented Aug 29, 2022

kevinpapst left a comment • edited Loading

Choose a reason for hiding this comment

kevinpapst Aug 30, 2022

Choose a reason for hiding this comment

LiaraAlis Aug 30, 2022

Choose a reason for hiding this comment

kevinpapst Aug 30, 2022

Choose a reason for hiding this comment

kevinpapst Aug 30, 2022

Choose a reason for hiding this comment

LiaraAlis commented Aug 30, 2022 • edited Loading

kevinpapst commented Aug 30, 2022 • edited Loading

LiaraAlis commented Sep 2, 2022

kevinpapst commented Sep 2, 2022 • edited Loading

LiaraAlis commented Sep 2, 2022 • edited Loading

LiaraAlis commented Sep 4, 2022 • edited Loading

codecov bot commented Sep 4, 2022 • edited Loading

Codecov Report

kevinpapst left a comment

Choose a reason for hiding this comment

kevinpapst left a comment •

edited

Loading

LiaraAlis commented Aug 30, 2022 •

edited

Loading

kevinpapst commented Aug 30, 2022 •

edited

Loading

kevinpapst commented Sep 2, 2022 •

edited

Loading

LiaraAlis commented Sep 2, 2022 •

edited

Loading

LiaraAlis commented Sep 4, 2022 •

edited

Loading

codecov bot commented Sep 4, 2022 •

edited

Loading