[19.0][IMP] subscription_oca: support invoice and delivery addresses#1453
Conversation
8c1919d to
ab4f036
Compare
|
This improvement makes sense, please rebase the branch and update the PR |
|
@alvaro-domatix I think rebasing would help resolve the conflicts with the base branch. |
rrebollo
left a comment
rrebollo
left a comment
There was a problem hiding this comment.
Code Review: Great work! The code looks good to me (LGTM). Thank you for your contribution! I've provided a few suggestions for your consideration—feel free to address them as you see fit.
| "parent_id": cls.customer.id, | ||
| } | ||
| ) | ||
| cls.pricelist = cls.env["product.pricelist"].create( |
There was a problem hiding this comment.
You are overwriting the pricelist variable from the core ProductCommon class (defined here). Instead, you could either modify it in place (which is not recommended) or, better yet, create your own pricelist using a different variable name, such as test_pricelist, to avoid unintended side effects on the parent class's behavior.
| cls.pricelist = cls.env["product.pricelist"].create( | ||
| {"name": "Addr PL", "currency_id": cls.company.currency_id.id} | ||
| ) | ||
| cls.template = cls.env["sale.subscription.template"].create( |
There was a problem hiding this comment.
I would prefix all the names of these variables to avoid collisions with variables defined higher up in the testing class hierarchy (Accounting < Product < UOM < BaseCommon).
| "name": "Subscription management", | ||
| "summary": "Generate recurring invoices.", | ||
| "version": "19.0.1.0.0", | ||
| "version": "19.0.1.1.0", |
There was a problem hiding this comment.
Generally, this is handled automatically by the OCA merge bot.
ab4f036 to
36268c5
Compare
|
Rebased on top of the current 19.0 ( @rrebrollo good catch — I've prefixed the test setUp variables ( |
|
With all of these changes, you are converting |
|
I don't think it will reach the same level of complexity as contract. To me, these improvements are still part of the core subscription domain and make subscription_oca more complete rather than turning it into a contract management module. |
|
I said "complexity" meaning the same feature set, and yes, you are doing just exactly that: putting the features that |
|
I see your point. My understanding, however, is that the goal is to strengthen subscription_oca as the subscription engine rather than to keep it intentionally limited. I don't think the current improvements bring it to the same scope as contract, but I'd be interested in aligning on the long-term vision for both modules. |
Add invoice and delivery addresses on subscriptions, defaulting to the customer's corresponding addresses and overridable per subscription. They are propagated to the recurring invoices and to the sale orders generated by the subscription. As on a sale order invoice, the move is addressed to the invoice address (account.move.partner_id) and records the delivery address; commercial_partner_id still rolls up to the contracting company, so the receivable stays on the parent. The delivery address is passed explicitly so it is not re-derived from the invoice partner. The fields are shown under the "Display Delivery / Invoice addresses" group, mirroring sale orders.
36268c5 to
9d27b23
Compare
|
Rebased on top of the current 19.0 ( |
|
/ocabot merge patch |
|
On my way to merge this fine PR! |
|
Congratulations, your PR was merged at faf7e2f. Thanks a lot for contributing to OCA. ❤️ |
7 participants
Adds invoice and delivery addresses on subscriptions, defaulting to the customer's corresponding addresses and overridable per subscription, mirroring sale orders.
Both addresses are propagated to the recurring invoices (the move is addressed to the invoice address and records the delivery address;
commercial_partner_idstill rolls up to the contracting company, so the receivable stays on the parent) and to the sale orders generated by the subscription.The fields show under the standard "Display Delivery / Invoice addresses" group.