[12.0][FIX] account_invoice_triple_discount: subtotal computation - #876
[12.0][FIX] account_invoice_triple_discount: subtotal computation#876SimoRubi wants to merge 5 commits into
Conversation
| 'Discount 3 (%)', | ||
| digits=dp.get_precision('Discount'), | ||
| ) | ||
| discounting_type = fields.Selection( |
There was a problem hiding this comment.
I don't understand where this field can be changed ? Should not be a setting on the related res.partner ? I mean some suppliers are using additive method and other multiplicative method.
Don't you think ?
There was a problem hiding this comment.
This can be changed directly on the account invoice line's form, but it can be interesting to inherit that from the partner.
I'm currently investigating why Travis is still 🔴 (that's why this is still a Draft), after that I can try to add this new behavior.
There was a problem hiding this comment.
While implementing this feature I noticed that it is not as trivial as it seems because default value for invoice line cannot rely on the line's fields (default methods are api.model), so I'd need to save it in the context etc..
I think that deserves its own PR, and here let's focus on the described issue, is that ok for you?
BTW Now that Travis is 🟢, I'll mark this PR as ready for reviews
There was a problem hiding this comment.
well, I was more thinking on storing the discounting_type only on the res.partner or at least on the order/invoice method.
it will be a lighter implementation.
Do you think there are real cases when there are some line with additive method, and other with multiplicative ?
I mean, setting this value 50 times, if you have 50 lines will be a mess, in a UX point of view.
There was a problem hiding this comment.
That is why I was talking about a default value for new lines: each of the 50 lines would have the default value inherited from the partner; and switching partner in the invoice would trigger the edit of all the invoice's lines.
I don't know about real cases of lines having different kinds of discounts but this is how sale orders + triple_discount currently work and it would be nice to have it in invoices too.
There was a problem hiding this comment.
I added the logic explained above, can you update your review?
There was a problem hiding this comment.
I don't know about real cases of lines having different kinds of discounts but this is how sale orders + triple_discount currently work and it would be nice to have it in invoices too.
-
Well if it's not a use case, it should be changed I think. Don't you ?
-
the problem with the current implementation is that it will create inconsistencies. I mean, it's possible to have "multiplicative" and invoice level and "additive" on line level.
-
you are adding new field on invoice / invoice.line. don't you think you should add a pre-migration script to populate correctly the database. I fear that an update of the current module on a production database will be slow. Don't you think ?
|
Might it be related to OCA/purchase-workflow#834 and my comment in #733 ? |
It might be, I didn't know purchase's version of triple discount but looks like every module is re-implementing that logic in its own way. |
OK! |
daa1d21 to
baa149f
Compare
When `super` method is `api.one`, each line has to be processed separately in order to have cache consistency.
|
@OCA/accounting-maintainers can this be merged? Thanks! |
|
Last commit solves the same issue as OCA/sale-workflow#1618 |
… discount's precision
a847827 to
83c27b1
Compare
|
@OCA/accounting-maintainers can this be merged or do I have to fix anything? |
|
@SimoRubi could you please have a look at SimoRubi#1 ? |
|
@OCA/accounting-maintainers can this be merged or do I have to fix anything? |
I had a look and I think that cannot be merged as explained in SimoRubi#1 (review) |
|
@OCA/accounting-maintainers can this be merged or do I have to fix anything? |
|
So this is happening on v13 #733 and i guess it happens on v14 too?? |
|
@legalsylvain could you review this one again? thanks |
legalsylvain
left a comment
legalsylvain
left a comment
There was a problem hiding this comment.
Regarding Point 1, I think it can be changed later, but could you write a bullet in the ROADMAP.rst file to mention that topic ?
Also write a bullet to say that sale_triple_discount should be refactored to use new mixin introduced in that module.
https://github.com/OCA/sale-workflow/blob/14.0/sale_triple_discount/__manifest__.py
Regarding point 2 & 3, let me know your point of view.
|
@SirTakobi can you take a look at the recent review? |
Thanks for letting me know of #876 (review). I can't edit this PR because it is based on @SimoRubi's branch. |
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
github-actions
Bot
added
the
stale
8 participants
Steps (see also added test case)
Before this PR
The line's subtotal is 707
After this PR
The line's subtotal is 706.88
This modification borrows the triple discount computation from https://github.com/OCA/sale-workflow/blob/361d4b039f281fe9293e4dcde30f5a8ce9c805bb/sale_triple_discount/models/sale_order_line.py and puts everything in abstract model, in order to be reused from sale order line (PR coming soon) and purchase order line.
todo
PR for purchase orderDoes not make sense until the Odoo issue [ORM] Column dropped if moved to dependency's mixin odoo/odoo#67168 is solved (found during implementation of [12.0][REF] sale_triple_discount: subtotal computation sale-workflow#1495)