[17.0][MIG] account_ecotax: Migration to 17.0

[mourad-ehm]

No description provided.

[mourad-ehm]

@florian-dacosta

[vvrossem]

LGTM.
Suggestions are "comestic"

[vvrossem]

Use BaseCommon as base test class to reduce overhead from tracking and boost the test suite.

[SilvioC2C]

Template account.document_tax_totals can be called from different other templates, which all define a different object variable name (most of them use o or doc, using something different is very rare).

If o._name breaks the rendering of the report, I would:

1. check which variable is used by the caller template, and force it to use o instead of doc[s] if possible
2. else, "normalize" the variable name before this t-if check by adding this line before the check itself:

<t t-set="o" t-value="o or doc"/>

[florian-dacosta]

What is wrong with using docs, which, AFAIK will work every time ?
Because, how o or doc are defined, is always with something like <t t-foreach="docs" t-as="doc">

Your suggestion 1 could imply to make multiple override, 1 for each caller template, and sometimes from module this one does not even depend (purchase). It does not seem reasonable to me.
For the 2 <t t-set="o" t-value="o or doc"/>, I do not think it will work (but I may be wrong) because o could not exist at the time it evaluate the t-value (unless the variable defined by t-set is initialized before, I don't know). Anyway ,it could fail if the caller does not define nor o nor doc. It should not happen, but it could, seems less robust that using the always existing docs, no?

[SilvioC2C]

If o is not set, then o.amount_ecotax will crash the render of the template too with AttributeError: 'NoneType' object has no attribute 'amount_ecotax' 😄

99% of the templates use the formula you mentioned:

<t t-foreach="docs" t-as="variable_name">

but different modules use different variable names.
For example:

• module account uses o as variable name here and here
• module purchase uses o as variable name here and here
• module sale uses doc as variable name here and here
• other modules use different variable names as well

Unfortunately, Odoo doesn't have a clear guideline for naming report variables, so we cannot be certain about which variable name we'll receive from the caller.
However, the main modules that call account.document_tax_totals[_template] all use o or doc. That's why I propose to use the formula

<t t-set="o" t-value="o or doc"/>

to make sure o is correctly defined before checking its value.

We could also rearrange the values in a safer, more generic way:

        <xpath expr="//t[@t-set='subtotal_to_show']" position="before">
            <t t-set="move" t-value="o or doc"/>
            <tr t-if="move and move._name == 'account.move' and move.amount_ecotax" name="ecotax_line">
                <td name="ecotax_label">Including Eco Part</td>
                <td class="text-end">
                    <span t-esc="move._get_formatted_ecotax_amount()"/>
                </td>
            </tr>
        </xpath>

With this solution, move can either be equal to o or doc; if none of those variables are defined, then move will be None and the t-if will be evaluated to False before trying to access the _name attribute or the amount_ecotax field.

[florian-dacosta]

I am sorry, I still don't get what is the issue with docs.

If o is not set, then o.amount_ecotax will crash the render of the template too with AttributeError: 'NoneType' object has no attribute 'amount_ecotax' 😄

No it won't because the first condition using docs._name will be False and the second one (o.amount_ecotax) won't be evaluated.
That is why I replaced o by docs.

For example:

module account uses o as variable name [here](https://github.com/odoo/odoo/blob/44a1b163481b0b781028ce337d72fbb0c8730475/addons/account/views/report_invoice.xml#L437-L446) and [here](https://github.com/odoo/odoo/blob/44a1b163481b0b781028ce337d72fbb0c8730475/addons/account/views/report_invoice.xml#L457-L463)
module purchase uses o as variable name [here](https://github.com/odoo/odoo/blob/44a1b163481b0b781028ce337d72fbb0c8730475/addons/purchase/report/purchase_quotation_templates.xml#L66-L72) and [here](https://github.com/odoo/odoo/blob/44a1b163481b0b781028ce337d72fbb0c8730475/addons/purchase/report/purchase_order_templates.xml#L136-L142)
module sale uses doc as variable name [here](https://github.com/odoo/odoo/blob/44a1b163481b0b781028ce337d72fbb0c8730475/addons/sale/report/ir_actions_report_templates.xml#L191-L197) and [here](https://github.com/odoo/odoo/blob/44a1b163481b0b781028ce337d72fbb0c8730475/addons/sale/report/ir_actions_report_templates.xml#L203-L211)
other modules use different variable names as well

But they all have docs defined !
In this override, I only want to add something for the account.move report.
We know that docs will always be defined, then it can be used to detect if we are in the case of the account.move report. Then I use o which seems quite safe since we already know that we are in the case of the invoice report which define o.
With the present, I don't think any report is failing.

The only case that could fail, would be a new report on account.move, that would use this document_tax_totals template...
It is quite unlikely.

But anyway, to check the model (_name) attribute docs is more reliable than both o and doc together.
I you believe it is necessary to manage the o and doc variable, in case the first condition docs._name == 'account.move' is filled and o does not exists, then we can do the following :

        <xpath expr="//t[@t-set='subtotal_to_show']" position="before">
            <t t-if="docs._name == 'account.move'">
                <t t-set="move" t-value="o or doc"/>
                <tr t-if="move.amount_ecotax" name="ecotax_line">
                    <td name="ecotax_label">Including Eco Part</td>
                    <td class="text-end">
                        <span t-esc="move._get_formatted_ecotax_amount()"/>
                    </td>
                </tr>
            </t>
        </xpath>

Would that be fine for you ?

[SilvioC2C]

But then what would happen if o or doc is not an item contained in docs, in case some other reports redefine such variables?
We should also check move in docs, but it would be equivalent to checking whether move._name == "account.move", so we're back to square one.

However, maybe there's a better way: if we only need this field for the invoices, why isn't its info added to field account.move.tax_totals and then fetched from the tax_totals that is always available in the account.document_tax_totals report?

[florian-dacosta]

I really feel we are trying to solve a problem that does and probably won't exists.
If o or doc is not defined, then the ecotax amount won't be displayed.

I mean, the goal here, is to display the ecotax amount in the invoice report.
We know that the invoice report define o. So, this work.
We know that docs will always exists, so the report won't crash for report of different models than account.move (and won't display anything), because of <t t-if="docs._name == 'account.move'">

If a new report is created on account.move, that does not define o nor doc. Then, it won't display anything because of condition <tr t-if="move and move.amount_ecotax" name="ecotax_line"> so it is fine.
And if we are afraid that this new report on account.move define move and we do not to override it, we change
<t t-set="move" t-value="o or doc"/> by <t t-set="move" t-value="move or o or doc"/> and we should be fine.
What other case could be wrong ?

Overriding tax_totals does not seem that easy.

[SilvioC2C]

If we want to use this override for invoicing reports only (which uses o), then the fix for avoiding the issue with sales report (which doesn't use o) would be to check if:

• o is defined
• o is an account.move record
• o.amount_ecotax is valued

without the need of checking 2 different variables.
The easiest solution would be:

<t t-if="o and o._name == 'account.move' and o.amount_ecotax"/>

[florian-dacosta]

Ok, let's go for this one if you prefer, I've done the change.

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[mourad-ehm]

Thanks @vvrossem I accepted your commit suggestions