-
-
Notifications
You must be signed in to change notification settings - Fork 284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[14.0][IMP] account_invoice_inter_company: add setting to disable or enable… #684
[14.0][IMP] account_invoice_inter_company: add setting to disable or enable… #684
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code reviewed LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional ok!
@pedrobaeza good for merge? |
@@ -82,6 +82,12 @@ def create_counterpart_invoices(self): | |||
# nor the invoices that were already validated in the past | |||
dest_company = src_invoice._find_company_from_invoice_partner() | |||
if dest_company: | |||
# If one of the involved companies have the intercompany setting disabled, skip | |||
if ( | |||
not dest_company.intercompany_invoicing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition should be only for dest_company
. You may want to do invoices from A to B, but not from B to A.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedrobaeza was this overlooked when original PR #631 was merged in v16 or is there another reason for changing it in v14?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An overlook, sorry. It should be changed also in 16.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedrobaeza I think functionally it's a bit unclear with current documentation, is this the behavior you'd like to achieve?
With "Generate Intercompany Invoices" enabled for both Company 1 and Company 2:
Customer invoice from C1 to C2 generates vendor bill from C2 to C1
Customer invoice from C2 to C1 generates vendor bill from C1 to C2
Vendor bill from C1 to C2 generates customer invoice from C1 to C2
Vendor bill from C1 to C2 generates customer invoice from C1 to C2With "Generate Intercompany Invoices" enabled for C1 but not for Company 2:
Customer invoice from C1 to C2 generates vendor bill from C2 to C1
Vendor bill from C1 to C2 generates customer invoice from C1 to C2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, vendor bills don't generate customer invoices, no matter these checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to DESCRIPTION file it should:
Imagine you have company A and company B in the same Odoo database.
First scenario: company B create an invoice with company A as customer. The module will automate the generation of the supplier invoice in company A.
Second scenario: company A create an invoice with company B as supplier. The module will automate the generation of the customer invoice in company B.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's not correct IMO. Only one side should generate the document for the other side. If not, you may have duplicated things. It's the same as sale/purchase. The purchase order generates the sales order, not the reverse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the current documentation is valid, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know right now, as you have touched a lot 14.0 version, but the original intention when this module was forked in 9.0 wasn't the bi-directional creation IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know right now, as you have touched a lot 14.0 version, but the original intention when this module was forked in 9.0 wasn't the bi-directional creation IIRC.
As far as I can see the feature has been the same since v12 (2019) and from what I've tested it works as described (with several caveats)
That said, I don't agree with this behavior:
With "Generate Intercompany Invoices" enabled for both Company 1 and Company 2:
Customer invoice from C1 to C2 generates vendor bill from C2 to C1
Customer invoice from C2 to C1 generates vendor bill from C1 to C2
Vendor bill from C1 to C2 generates customer invoice from C1 to C2
Vendor bill from C1 to C2 generates customer invoice from C1 to C2With "Generate Intercompany Invoices" enabled for C1 but not for Company 2:
Customer invoice from C1 to C2 generates vendor bill from C2 to C1
Vendor bill from C1 to C2 generates customer invoice from C1 to C2
As the use case I need to address is "I want to prevent C1 from generating invoices for C2, but I want C1 to generate invoices to C3" as we're in a multi-company environment where C2 is not ready for receiving invoices.
So this PR's behavior (and that of the merged one in 16) looks correct to me, if both companies don't have flag enabled there's no invoice/bill creation.
For a more complete implementation, the boolean could be changed to a select with options:
- Disable intercompany invoice/bills creation
- Only create invoices from intercompany bills
- Only create bills from intercompany invoices
- Create both bills and invoices from other companies
So as to accommodate all use cases. But this can be done in a later PR with a migration.
I would anyway improve description specifying "if two companies don't have both flag "Generate Intercompany Invoices" enabled, there's no invoice or bill creation between them."
This PR has the |
47430b3
to
de48113
Compare
… intercompany invoicing This commit adds a new setting called intercompany_invoicing that allows the user to activate or deactivate the intercompany invoicing. When positing an intercompany invoice, if one of the companies have the setting disabled, the other invoice will not be automatically created.
de48113
to
6d9e25c
Compare
@pedrobaeza since this is the same behavior as it currently is in v16, can it be merged? If someone wants to change behavior to avoid bidirectional creation, this is major change and should be addressed in another PR thanks! |
Well, I think the module goal has been perverted over time, but if needed, we'll fix newer versions. v14 is so old. /ocabot merge minor |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at 17736c9. Thanks a lot for contributing to OCA. ❤️ |
… intercompany invoicing
This commit adds a new setting called intercompany_invoicing that allows the user to activate or deactivate the intercompany invoicing. When positing an intercompany invoice, if one of the companies have the setting disabled, the other invoice will not be automatically created.
Backport of #631