[16.0][ADD] account_invoice_partner_company_only: Restrict partner…#2180
[16.0][ADD] account_invoice_partner_company_only: Restrict partner…#2180
Conversation
The domain seems oververvrited by another module in the repo. That means this is probably not the right way to do it. Maybe with a Python constraint? Let me take a look. |
|
After some tests, using a constraint that throws an exception is too intrusive, so I just left the contact filter for "company" types in the form. I'm moving this pull request to draft status because this module will be an optional dependency (using preferences) of OCA/sale-workflow#4039, and I'll be making some changes. This time, I'll wait until everything is ready before requesting a review. :) |
maisim
force-pushed
the
16.0-add-account_invoice_partner_id_company_only
branch
from
fce3525 to
11fdd50
Compare
|
@maisim the name of the addon should be
Look at my review comments on the sale-workflow addon. |
maisim
force-pushed
the
16.0-add-account_invoice_partner_id_company_only
branch
from
11fdd50 to
387be53
Compare
maisim
changed the title
|
Hi @rrebollo,
Let me know if that's okay with you |
…unctional tests from runboat TODO: remove once the following PRs are merged: - OCA#4042 (sale_order_partner_company_only) - OCA/project#1611 (project_partner_company_only) - OCA/account-invoicing#2180 (account_invoice_partner_company_only)
…unctional tests from runboat TODO: remove once the following PRs are merged: - OCA#4042 (sale_order_partner_company_only) - OCA/project#1611 (project_partner_company_only) - OCA/account-invoicing#2180 (account_invoice_partner_company_only)
rrebollo
left a comment
rrebollo
left a comment
There was a problem hiding this comment.
The design and implementation still feel quite naive at this point. The base requirement may be valid, but you need to work on it a bit longer.
Take a look at already merged addons to grasp the philosophy behind OCA contributions.
Thanks for your effort.
| def test_01_partner_domain_in_view(self): | ||
| """Test that partner_id field has company-only domain in the view.""" | ||
| account_move = self.env["account.move"] | ||
| view = self.env.ref("account.view_move_form") | ||
|
|
||
| # Get the view arch with our module's modifications | ||
| view_info = account_move.get_view(view_id=view.id) | ||
| doc = etree.fromstring(view_info["arch"]) | ||
|
|
||
| # Find partner_id field in the view | ||
| partner_fields = doc.xpath("//field[@name='partner_id']") | ||
|
|
||
| # Check that the first partner_id field has the compayny type domain | ||
| domain_found = False | ||
| field = partner_fields[0] | ||
| domain = field.get("domain") | ||
| if "('is_company', '=', True)" in domain: | ||
| domain_found = True | ||
|
|
||
| self.assertTrue( | ||
| domain_found, | ||
| "partner_id field should have a domain restricting to companies", | ||
| ) |
There was a problem hiding this comment.
Here, you're testing whether Odoo is functioning as expected, but what you should be testing is the feature of this addon—not Odoo's built-in functionality.
There was a problem hiding this comment.
Hi !
As mentioned here OCA/sale-workflow#4039 (comment) , I'm going to close this PR because I agree it's strange to have this module on its own, without a strong link to the "main" module.
However, and with the performance goal we all share here, I don't quite understand your comment: this test verifies that the filter is indeed present, which is precisely the role of this module.
| def test_02_invoice_with_company(self): | ||
| """Test creating an invoice with a company partner""" | ||
| invoice = self.env["account.move"].create( | ||
| { | ||
| "partner_id": self.partner_company.id, | ||
| "move_type": "out_invoice", | ||
| } | ||
| ) | ||
| self.assertEqual( | ||
| invoice.partner_id, | ||
| self.partner_company, | ||
| "Invoice should be created with company partner", | ||
| ) |
| def test_03_individual_contact_excluded_by_domain(self): | ||
| """Test that individual contacts are excluded by the view domain. | ||
|
|
||
| The domain restricts partner_id to companies only. A contact with | ||
| is_company=False must not appear in a search using that domain. | ||
| """ | ||
| domain = [("is_company", "=", True)] | ||
| results = self.env["res.partner"].search(domain) | ||
| self.assertIn( | ||
| self.partner_company, | ||
| results, | ||
| "Company partner should be included in restricted domain search", | ||
| ) | ||
| self.assertNotIn( | ||
| self.individual_contact, | ||
| results, | ||
| "Individual contact should be excluded by the is_company domain", | ||
| ) |
There was a problem hiding this comment.
Similar to before. You're testing whether domains are correctly handled by Odoo—that's not a feature of your addon.
| <xpath expr="//field[@name='partner_id']" position="attributes"> | ||
| <attribute | ||
| name="domain" | ||
| operation="domain_add" | ||
| >[('is_company', '=', True)]</attribute> | ||
| </xpath> |
There was a problem hiding this comment.
Are you sure this view is used only for invoices? I believe it's generally used for journal entries. Is that also your goal?
During the tests, you seemed interested only in the out_invoice type, so is this the correct implementation? Also, you're only limiting at this level. What about account.move records created via the API?
You need to think more carefully about your design.
|
Hi @rrebollo, thanx for your review.
As mentioned in the first comment of this PR and in the documentation for this module:
But I admit that:
|
3 participants
… to companies