Skip to content
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][MIG] l10n_be_cooperator_national_number #69

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion l10n_be_cooperator_national_number/__manifest__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
{
"name": "Belgium: Cooperator National Number",
"summary": "Ask for Belgian National Number in Cooperative Subscription Request.",
"version": "12.0.1.0.0",
"version": "14.0.1.1.0",
"depends": ["cooperator", "l10n_be_national_number"],
"author": "Coop IT Easy SC, Odoo Community Association (OCA)",
"category": "Cooperative management",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# SPDX-FileCopyrightText: 2023 Coop IT Easy SC
#
# SPDX-License-Identifier: AGPL-3.0-or-later

from openupgradelib import openupgrade


@openupgrade.migrate()
def migrate(env, version):
sql = """
UPDATE res_company
SET display_national_number = true
WHERE require_national_number = true
"""
openupgrade.logged_query(env.cr, sql)
20 changes: 19 additions & 1 deletion l10n_be_cooperator_national_number/models/company.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,28 @@
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html).


from odoo import fields, models
from odoo import _, api, fields, models
from odoo.exceptions import ValidationError


class ResCompany(models.Model):
_inherit = "res.company"

display_national_number = fields.Boolean(string="Display National Number")
require_national_number = fields.Boolean(string="Require National Number")

@api.constrains("display_national_number", "require_national_number")
def _check_national_number(self):
for company in self:
if company.require_national_number and not company.display_national_number:
raise ValidationError(
_(
"If the 'Require National Number' toggle is enabled,"
" then so must the 'Display National Number' toggle."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"If the 'Require National Number' toggle is enabled,"
" then so must the 'Display National Number' toggle."
'If the "Require National Number" toggle is enabled,'
' then so must the "Display National Number" toggle.'

i think that delimiting strings with double quotes is more common in user-facing text.

)
)

@api.onchange("display_national_number")
def _onchange_display_national_number(self):
if not self.display_national_number:
self.require_national_number = False
48 changes: 25 additions & 23 deletions l10n_be_cooperator_national_number/models/subscription_request.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from odoo import fields, models, api, _
from odoo import _, api, fields, models
from odoo.exceptions import UserError


Expand All @@ -8,16 +8,22 @@ class SubscriptionRequest(models.Model):
national_number = fields.Char(string="National Number")
display_national_number = fields.Boolean(
compute="_compute_display_national_number",
default=lambda self: self._check_national_number_required(),
)
require_national_number = fields.Boolean(
compute="_compute_require_national_number",
)

@api.depends("company_id", "company_id.require_national_number")
@api.depends("is_company", "company_id", "company_id.display_national_number")
def _compute_display_national_number(self):
self.display_national_number = self._check_national_number_required()
self.display_national_number = (
self.company_id.display_national_number and not self.is_company
)

def _check_national_number_required(self):
company = self.env["res.company"]._company_default_get()
return company.require_national_number
@api.depends("is_company", "company_id", "company_id.require_national_number")
def _compute_require_national_number(self):
self.require_national_number = (
self.company_id.require_national_number and not self.is_company
)

def get_national_number_from_partner(self, partner):
national_number_id_category = self.env.ref(
Expand All @@ -30,30 +36,26 @@ def get_national_number_from_partner(self, partner):

def validate_subscription_request(self):
self.ensure_one()
if (
self._check_national_number_required()
and not self.national_number
and not self.is_company
):
if self.require_national_number and not self.national_number:
raise UserError(_("National Number is required."))
super().validate_subscription_request()

def create_national_number(self, partner):
if self._check_national_number_required():
if not self.is_company:
values = {
"name": self.national_number,
"category_id": self.env.ref(
"l10n_be_national_number.l10n_be_national_number_category" # noqa
).id,
"partner_id": partner.id,
}
self.env["res.partner.id_number"].create(values)
if not self.is_company:
values = {
"name": self.national_number,
"category_id": self.env.ref(
"l10n_be_national_number.l10n_be_national_number_category" # noqa
).id,
"partner_id": partner.id,
}
self.env["res.partner.id_number"].create(values)
return partner

def create_coop_partner(self):
partner = super().create_coop_partner()
self.create_national_number(partner)
if self.require_national_number:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why putting a condition on "require" and not "display" here ? If the require config is unchecked but display is checked it will not write the national number on the partner ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I will patch this up, because simply switching to display won't suffice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was surprisingly difficult to fix. Done now. Ready for another review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code and functional review OK

self.create_national_number(partner)
return partner

def get_representative_vals(self):
Expand Down
2 changes: 1 addition & 1 deletion l10n_be_cooperator_national_number/readme/DESCRIPTION.rst
Original file line number Diff line number Diff line change
@@ -1 +1 @@
Ask for Belgian National Number in Subscription Request.
Ask for Belgian National Number in Subscription Request.
4 changes: 4 additions & 0 deletions l10n_be_cooperator_national_number/readme/USAGE.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
On the company, two new toggles 'Display National Number' and 'Require National
Number' are added. In order to expose the functionality of this module, you must
enable these toggles for every company that wishes to use it. 'Display' shows
the field and allows it to be filled in. 'Require' makes the field mandatory.
Comment on lines +1 to +4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we be able to display the field and use it without making it mandatory ? If not what's the point in having two toggles ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what the 'Display' toggle is for. When the 'Display' toggle is disabled, it's as if this module isn't installed. Useful for multi-company.

Copy link
Member

@victor-champonnois victor-champonnois Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it's just that the sentence "In order to expose the functionality of this module, you must enable these toggles for every company that wishes to use it.", seems to mean that you have to enable both toggles for the functionality to work, while actually you can only toggle "display" and the functionality will work (without requirements on the NRN).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, maybe let's write 'one or both of these toggles'?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, LGTM

Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# Robin Keunen <robin@coopiteasy.be>
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html).

from odoo.exceptions import UserError
from odoo.exceptions import UserError, ValidationError
from odoo.tests.common import SavepointCase

from odoo.addons.cooperator.tests.cooperator_test_mixin import CooperatorTestMixin
Expand All @@ -19,9 +19,34 @@ def create_subscription_request(self):
return self.env["subscription.request"].create(vals)

def set_national_number_required(self):
company = self.env["res.company"]._company_default_get()
company = self.env.company
company.display_national_number = True
company.require_national_number = True

def test_require_without_display(self):
"""You cannot require a national number without also displaying it."""
company = self.env.company
company.write(
{
"display_national_number": False,
"require_national_number": False,
}
)
with self.assertRaises(ValidationError):
company.require_national_number = True
self.set_national_number_required()
with self.assertRaises(ValidationError):
company.display_national_number = False

def test_company_not_required(self):
"""Subscription requests for companies do not require a national number."""
self.set_national_number_required()
vals = self.get_dummy_company_subscription_requests_vals()
subscription_request = self.env["subscription.request"].create(vals)
subscription_request.validate_subscription_request()
self.assertFalse(subscription_request.display_national_number)
self.assertFalse(subscription_request.require_national_number)

def test_national_number_applied_to_partner(self):
self.set_national_number_required()
subscription_request = self.create_subscription_request()
Expand Down
8 changes: 6 additions & 2 deletions l10n_be_cooperator_national_number/views/res_company_view.xml
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,13 @@
<field name="name">res.company.form.easymy.coop</field>
<field name="inherit_id" ref="base.view_company_form" />
<field name="model">res.company</field>
<field name="arch" type="xml">
<field name="arch" type="xml">
<field name="allow_id_card_upload" position="after">
<field name="require_national_number"/>
<field name="display_national_number" />
<field
name="require_national_number"
attrs="{'invisible': [('display_national_number', '=', False)]}"
/>
</field>
</field>
</record>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@
<field name="inherit_id" ref="cooperator.subscription_request_view_form" />
<field name="arch" type="xml">
<field name="country_id" position="after">
<field name="national_number" attrs="{'invisible': ['|', ('display_national_number', '=', False), ('is_company', '=', True)],
'required': [('display_national_number', '=', True),
('is_company', '=', False)]}"/>
<field name="display_national_number" invisible="True"/>
<field
name="national_number"
attrs="{'invisible': [('display_national_number', '=', False)],
'required': [('require_national_number', '=', True)]}"
/>
<field name="display_national_number" invisible="True" />
<field name="require_national_number" invisible="True" />
</field>
</field>
</record>
Expand Down
6 changes: 6 additions & 0 deletions setup/l10n_be_cooperator_national_number/setup.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import setuptools

setuptools.setup(
setup_requires=['setuptools-odoo'],
odoo_addon=True,
)