OCA / account-fiscal-rule

Odoo Taxes & Fiscal Rules Management
GNU Affero General Public License v3.0
58 stars 182 forks source link

FIX acc_fisc_classif: find or create a classif if none #435

Closed bealdav closed 4 months ago

bealdav commented 5 months ago

This PR is needed to install all modules of the branch

cc @gurneyalex @yankinmax

OCA-git-bot commented 5 months ago

Hi @legalsylvain, some modules you are maintaining are being modified, check this out!

bealdav commented 5 months ago

Thanks a lot @legalsylvain for your review, less code, more test

legalsylvain commented 5 months ago

@mourad-ehm : what do you think about the bug described here ? https://github.com/OCA/account-fiscal-rule/pull/435#pullrequestreview-2114201055

mourad-ehm commented 5 months ago

I discussed with @bealdav about this bug. He will fix it.

bealdav commented 5 months ago

You're right !

# this code fails to find classification
>>> env["account.product.fiscal.classification"].search([('sale_tax_ids', 'in', []), ('purchase_tax_ids', 'in', [])])
account.product.fiscal.classification()
# this code achieve to find classifications
>>> env["account.product.fiscal.classification"].search([('sale_tax_ids', '=', False), ('purchase_tax_ids', '=', False)])
account.product.fiscal.classification(5, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44)
# syntax in incorrect
>>> env["account.product.fiscal.classification"].search([('sale_tax_ids', '=', []), ('purchase_tax_ids', '=', [])])
2024-06-14 10:12:07,924 337 WARNING db3 odoo.osv.expression: The domain term '('sale_tax_ids', '=', [])' should use the 'in' or 'not in' operator. 
2024-06-14 10:12:07,924 337 WARNING db3 odoo.osv.expression: The domain term '('purchase_tax_ids', '=', [])' should use the 'in' or 'not in' operator. 

Then the code to define domain should be

        domain = [("sale_tax_ids", "=", False)]
        if sale_tax_ids:
            domain = [("sale_tax_ids", "in", sale_tax_ids)]
        if purchase_tax_ids:
            domain.append(("purchase_tax_ids", "in", purchase_tax_ids))
        else:
            domain.append(("purchase_tax_ids", "=", False))

Are you ok ? @legalsylvain

I have done some additional tests in last commit to secure this kind of failures

legalsylvain commented 5 months ago

I don't think that line works :

domain = [("sale_tax_ids", "in", sale_tax_ids)]

In mean, if sale_tax_ids = [2] and sale_tax_ids = [2, 3] the domain is respected, but we should not select this classification. Don't you think ?

bealdav commented 5 months ago

Finalement je vois pas d'autres solution qui marche a 100%

        domain = [("sale_tax_ids", "=", False)]
        if sale_tax_ids:
            domain = [("sale_tax_ids", "=", sale_tax_ids)]
        if purchase_tax_ids:
            domain.append(("purchase_tax_ids", "=", purchase_tax_ids))
        else:
            domain.append(("purchase_tax_ids", "=", False))

On pourras rendre cela plus joli tard, mais si cela marche comme ça on pourrais merger ?

legalsylvain commented 5 months ago

supersed by https://github.com/OCA/account-fiscal-rule/pull/441

legalsylvain commented 4 months ago

Hi @bealdav and @mourad-ehm. I fixed this PR, in #441, but I need review. Could you take a time to review it ?

thanks a lot.