Skip to content

[IMP] sale_invoice_policy: Services qty to invoice calculation#3956

Open
dalonsofl wants to merge 5 commits intoOCA:16.0from
factorlibre:16.0-imp-sale_invoice_policy
Open

[IMP] sale_invoice_policy: Services qty to invoice calculation#3956
dalonsofl wants to merge 5 commits intoOCA:16.0from
factorlibre:16.0-imp-sale_invoice_policy

Conversation

@dalonsofl
Copy link
Copy Markdown
Contributor

Module

sale_invoice_policy

Describe the bug

The calculation of the quantity to invoice for service is different compared to other types of products, with an invoicing policy based on delivered quantities in the order and ordered quantities in the product.

This causes inconsistencies between the order and its lines and the generation of unwanted invoices.

To Reproduce

  1. Create an order with different products and invoice policy based on "Delivered quantities".

(image from runboat OCA)

imagen Selección_867 Selección_868 Selección_869 Selección_870 Selección_871 Selección_872

With this change, the calculation of quantity to invoice is always based on the invoice policy of the order in case it is filled and the configuration of "Sale Invoice Policy" is checked.

Affected versions:

16.0

Copy link
Copy Markdown
Contributor

@amarcosg amarcosg left a comment

Choose a reason for hiding this comment

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

Code review LGTM!

other_lines = self.filtered(
lambda l: l.product_id.type == "service"
or not l.order_id.invoice_policy
lambda l: not l.order_id.invoice_policy
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From reading previous PRs adding this condition, it seems like ignoring services so that they're handled with their product config is intended behaviour. Mostly likely because depending on your setup, you might be using service_policy from sale_project for services and it's easier to just let the product handle it.
I think it'll be a hard sell to get this merged, if you need it I suggest extracting the condition to a function and modifying it in your personal repo.

In any case you would also need to adapt _compute_untaxed_amount_to_invoice as well

Copy link
Copy Markdown

@alexey-pelykh alexey-pelykh left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. The bug report is well-documented with screenshots showing the inconsistency between service and non-service lines. However, the change as implemented has gaps that should be addressed before this can be merged.

Key findings:

  1. Incomplete fix — _compute_untaxed_amount_to_invoice not updated (blocking)
    As @hildickethan already flagged, the same product_id.type == "service" exclusion remains in _compute_untaxed_amount_to_invoice (line 43). Removing it from _compute_qty_to_invoice alone creates a new inconsistency: qty_to_invoice will reflect the order-level policy for services, but untaxed_amount_to_invoice will not. Both computed fields need to be aligned.

  2. No test coverage for the fixed scenario (blocking)
    The test class creates product3 as a service product in setUpClass but never uses it in any test. This PR should add a test that places a service product on an order with an explicit invoice_policy, confirms the order, and asserts that qty_to_invoice and untaxed_amount_to_invoice on the service line respect the order-level policy rather than the product-level one.

  3. Behavioral risk — interaction with sale_project / service_policy
    The original service exclusion was intentional: it allows standard Odoo service invoicing logic (and sale_project's service_policy field) to govern services independently. Removing it means sale_invoice_policy will now override the product-level behavior for all product types when the order policy is set. Consider whether this should be opt-in (e.g. a config flag) or whether the PR description should explicitly acknowledge this trade-off so maintainers can make an informed decision.

CI is green (all 14 checks pass). The direction of the fix makes sense for the reported use case, but please address the consistency gap and add tests before this is ready for merge.

other_lines = self.filtered(
lambda l: l.product_id.type == "service"
or not l.order_id.invoice_policy
lambda l: not l.order_id.invoice_policy
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Removing the service exclusion here is consistent with the stated goal, but the same condition (line.product_id.type == "service") still exists in _compute_untaxed_amount_to_invoice (line 43 of the same file). These two computed fields must stay aligned — otherwise qty_to_invoice will use the order policy for services but untaxed_amount_to_invoice will not, creating a different inconsistency than the one being fixed.

Copy link
Copy Markdown

@alexey-pelykh alexey-pelykh left a comment

Choose a reason for hiding this comment

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

Thank you for the significant rework (removing base_partition, openupgradelib, and the pre_init_hook). The simplification is welcome.

However, the two blocking issues from my previous review remain:

  1. _compute_untaxed_amount_to_invoice still excludes services. The product_id.type == "service" condition is still in the other_lines filter for the amount computation, while it has been removed from _compute_qty_to_invoice. This creates an inconsistency — qty_to_invoice respects the order-level policy for services, but untaxed_amount_to_invoice does not.

  2. No test for the service product scenario. product3 (service type) is created in setUpClass but never used in any test. Please add a test that places a service product on an order with an explicit invoice_policy and asserts both qty_to_invoice and untaxed_amount_to_invoice.

Co-Reviewed-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants