-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
Changes from 6 commits
dae21b8
6716e31
dbe9d7b
774df4c
26ece8b
d0653c3
6f11e56
78089d8
b32b305
4d63763
0393cae
ec0b451
cfd17fd
d585d9a
10afd2e
69057a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) |
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 | ||
|
||
|
||
|
@@ -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( | ||
|
@@ -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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call. I will patch this up, because simply switching to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was surprisingly difficult to fix. Done now. Ready for another review. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
|
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. |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, maybe let's write 'one or both of these toggles'? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, LGTM |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
../../../../l10n_be_cooperator_national_number |
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, | ||
) |
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 think that delimiting strings with double quotes is more common in user-facing text.