[IMP] sale_invoice_policy: Several improvements#3376
Conversation
rousseldenis
force-pushed
the
16.0-imp-sale-invoice-policy-dro
branch
2 times, most recently
from
fb39a75 to
7b26e33
Compare
|
Don't hesitate to test it on runboat |
rousseldenis
force-pushed
the
16.0-imp-sale-invoice-policy-dro
branch
from
655eb48 to
f4ea1a1
Compare
rousseldenis
force-pushed
the
16.0-imp-sale-invoice-policy-dro
branch
from
f4ea1a1 to
dd6f488
Compare
ferran-S73
left a comment
ferran-S73
left a comment
There was a problem hiding this comment.
Code review OK. As per your question I see no problem in allowing product configuration for sale orders' invoice policy. It adds more flexibility to the module
marielejeune
left a comment
marielejeune
left a comment
There was a problem hiding this comment.
LGTM (code + functional review)
Thanks for the work
Hello @rousseldenis Thank you!!! |
jbaudoux
left a comment
jbaudoux
left a comment
There was a problem hiding this comment.
Can you add in the readme that the SO invoice policy affects only stock products. Service products continue to be invoiced based on the product invoice policy.
There was a problem hiding this comment.
And generate requirement.txt as it is not part of it in this repo
2025-01-25 16:00:20,521 417 INFO odoo odoo.modules.loading: Loading module sale_invoice_policy (66/67)
2025-01-25 16:00:20,522 417 CRITICAL odoo odoo.modules.module: Couldn't load module sale_invoice_policy
2025-01-25 16:00:20,522 417 CRITICAL odoo odoo.modules.module: No module named 'openupgradelib'
|
@rousseldenis Could you have a look at the last reviews please? |
rousseldenis
force-pushed
the
16.0-imp-sale-invoice-policy-dro
branch
from
dd6f488 to
f734e36
Compare
Don't use the 'required' parameter to compute the quantities to invoice or not. In fact, this should only depends on the invoice policy itself.
As ORM appends new depends() parameters to existing ones, simplify the writing
…he amount As for the 'qty_to_invoice' field, the 'untaxed_amount_to_invoice' field should not depends on the required option to compute its value
As we don't want to rewrite Odoo core method inside this module, we should not rewrite the method to compute the untaxed amount to invoice. Instead, save the product invoice policy, change it to sale order one without recomputing context, call the super() method and then, restore its former value.
rousseldenis
force-pushed
the
16.0-imp-sale-invoice-policy-dro
branch
2 times, most recently
from
86ca255 to
a24b407
Compare
|
@jbaudoux Could you update your review ? |
…olicy With default behavior in Odoo, the invoicing policy depends on products configuration. So, include this as possible value for sale order invoicing policy. Possible values are so: - Products Invoice Policy: each product applies its invoicing policy (by sale line) - Ordered quantities: the whole sale order will apply the invoicing policy on ordered quantities - Delivered Quantities: the whole sale order will apply the invoicing policy on delivered quantities That said, the 'required' configuration is no more useful as the invoicing policy is now required on sale order level.
…ecompute() To avoid marking 'invoice_policy' product field as dirty for products temporary value, use the working context manager 'protecting()' instead of deprecated 'norecompute() one. Put the whole process in its own contextmanager.
rousseldenis
force-pushed
the
16.0-imp-sale-invoice-policy-dro
branch
from
a24b407 to
f707990
Compare
jbaudoux
left a comment
jbaudoux
left a comment
There was a problem hiding this comment.
Can you drop auto-generated files from the commits?
alexey-pelykh
left a comment
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thanks for the refactor Denis, the move to a proper three-valued selection with "product" as a default is a much cleaner design than the old invoice_policy_required boolean approach. The delegation to Odoo core in _compute_untaxed_amount_to_invoice instead of reimplementing the whole calculation is a welcome simplification too.
A few things I noticed:
There is a copy-paste typo in readme/CONTEXT.rst (and consequently in the generated README.rst and index.html) -- the two bullet points both say "Invoicing on ordered quantities". The second one should be "Invoicing on delivered quantities".
In _sale_invoice_policy, a bare Exception is raised when called with mixed policies. Using ValueError (or UserError if the intent is to surface this to the UI) would be more conventional.
As @jbaudoux mentioned in his last review, the auto-generated files (README.rst, static/description/index.html) should be dropped from the commits. OCA CI regenerates them.
One note on the context manager pattern: the env.protecting + temporary product field patching approach is clever, but it does mean that if an exception occurs between setting the new values and the yield, the old values would not be restored. In practice this seems unlikely to cause issues since any such exception would typically roll back the transaction, but worth being aware of.
Otherwise the implementation looks solid -- pre_init_hook with openupgradelib for the migration, the company-level default, tests covering both policy paths and the context manager guard. Nice work.
|
/ocabot merge major |
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
Congratulations, your PR was merged at 05c66dc. Thanks a lot for contributing to OCA. ❤️ |
10 participants
depends()Refactor:
I think the default policy that was applied on sale orders from the default product configuration (defined in sale module) was the wrong approach as it is not contextually correct (products are not sale orders).
That said, I introduced an invoice policy on sale order level more explicitly than before (void) which is 'Products Invoicing Policy'. That let the user free to choose between standard behavior or sale order level configuration.
The question is: