Skip to content

Conversation

@vicire1
Copy link

@vicire1 vicire1 commented Nov 13, 2025

in Chile it is necessary to print a copy of the invoice which sometimes is used to yield the invoice physically and so need a infomation box that will be filled by hand. the purpose of this improvement is to add an action in the cog menu which download the copy of the invoice which includes this information box

task-5231331

in Chile it is necessary to print a copy of the invoice which sometimes is used to yield the invoice physically and so need a infomation box that will be filled by hand.
the purpose of this improvement is to add an action in the cog menu which download the copy of the invoice which includes this information box

task-5231331
@robodoo
Copy link

robodoo commented Nov 13, 2025

This PR targets the un-managed branch odoo-dev/odoo:18.0-rd-accounting-onboarding-malb, it needs to be retargeted before it can be merged.

Copy link

@malb-odoo malb-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the pr 😄

</div>
</template>


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless diff, even if you are right let's not change stuff for the pleasure of it ahah, the real reason why is that it break the github history and so it's more difficult to find the original pr 😄

<field name="report_type">qweb-pdf</field>
<field name="report_name">l10n_cl.yielding_invoice</field>
<field name="report_file">l10n_cl.yielding_invoice</field>
<field name="print_report_name">(object._get_report_base_filename() + "COPY")</field>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on other maybe a bit more clean is to extend the function

<field name="name">PDF with Copy</field>
<field name="model">account.move</field>
<field name="report_type">qweb-pdf</field>
<field name="report_name">l10n_cl.yielding_invoice</field>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name of the template could be a bit better like report_invoice_document_copy or something like this

</t>
</t>
</t>
</template>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it would be better to move everything you did here in enterprise, more precisely in l10n_cl_edi 👀

Comment on lines +322 to +328
<t t-call="web.html_container">
<t t-foreach="docs" t-as="o">
<t t-call="l10n_cl.report_invoice_document">
<t t-set="show_info_box" t-value="True"/>
</t>
</t>
</t>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally you shouldn't need to use the redefinition of the o variable if you target the right template 🤔 check l10n_cl.report_invoice instead of report_invoice_document.

Suggested change
<t t-call="web.html_container">
<t t-foreach="docs" t-as="o">
<t t-call="l10n_cl.report_invoice_document">
<t t-set="show_info_box" t-value="True"/>
</t>
</t>
</t>
<t t-call="l10n_cl.report_invoice">
<t t-set="show_info_box" t-value="True"/>
</t>

</t>

<xpath expr="//div[hasclass('invoice_main')]" position="inside">
<t t-if="show_info_box">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t-if can be added to a div directly

</div>
<div class="row g-0 pt-2 border-top border-dark">
<div class="col-12">
<p class="small mb-0">El acuse de recibo que se declara en este acto, de acuerdo a lo dispuesto en la letra b) del Art. 4° y la letra c) del Art. 5° de la Ley 19.983, acredita que la entrega de mercadería(s) o servicio(s)</p>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We always add text in english and translate after with pot and po files

<t t-if="show_info_box">
<div class="row g-0" style="float: right;">
<div class="col-12">
<div class="d-inline-block ms-auto" style="width: 440px;">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style should be avoided at all cost for different reason:

  • Difficult to maintain, using class we can actually modify the class for everything in the code to have consistency, here you cannot
  • Readability: here you have just one styling so ok but you can have a lot and it will become a nightmare
  • Priority: let's say we have a min height in one of the class, the styling will take priority on everything, which you could lead to problematic behavior

So always use bootstrap class instead 😄


<xpath expr="//div[hasclass('invoice_main')]" position="inside">
<t t-if="show_info_box">
<div class="row g-0" style="float: right;">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in term of construction of code you have 2 choices either using different rows or one big row and put everything in it. That's the solution we chose even it's not the best in term of html it was the cleaner and compact code

</div>
</div>
</t>
</xpath>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use the technique of only one row all of this can be obtain with 25 lines of code maximum ;) And most importantly no style only bootstrap class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants