-
Notifications
You must be signed in to change notification settings - Fork 20
[ADD] phone validations #844
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
Changes from 1 commit
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 |
---|---|---|
@@ -1 +1,3 @@ | ||
from . import res_partner | ||
from . import phone_number | ||
from . import phone_number_validation |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
import logging | ||
import re | ||
|
||
from odoo import _, api, models | ||
from odoo.exceptions import ValidationError | ||
|
||
_logger = logging.getLogger(__name__) | ||
|
||
|
||
class G2PPhoneNumber(models.Model): | ||
_inherit = "g2p.phone.number" | ||
|
||
def write(self, vals): | ||
res = super().write(vals) | ||
if "phone_no" in vals or "country_id" in vals: | ||
self._onchange_phone_validation() | ||
return res | ||
|
||
@api.model_create_multi | ||
def create(self, vals): | ||
record = super().create(vals) | ||
record._onchange_phone_validation() | ||
return record | ||
|
||
@api.onchange("phone_no", "country_id") | ||
def _onchange_phone_validation(self): | ||
phone_validation = self.env["spp.phone.validation"].search([("state", "=", "active")]) | ||
if not self.phone_no: | ||
return | ||
|
||
phone_no = self.phone_no | ||
if phone_validation: | ||
validated_success_count = 0 | ||
error_msg = [] | ||
for validation in phone_validation: | ||
if validation.with_prefix: | ||
pattern = r"^\+?" + re.escape(validation.prefix) + r"\d{" + str(validation.number_of_digits) + r"}$" | ||
else: | ||
pattern = r"^\d{" + str(validation.number_of_digits) + r"}$" | ||
if re.match(pattern, phone_no): | ||
validated_success_count += 1 | ||
else: | ||
error_msg.append(validation.name) | ||
|
||
if validated_success_count == 0: | ||
message = "Phone number must match one of the following formats: " + ", ".join(error_msg) | ||
raise ValidationError(_(message)) | ||
return | ||
Comment on lines
+26
to
+48
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 method is called from Additionally, the validation logic can be made more efficient. Instead of iterating through all validations and counting successes, you can stop at the first successful match. If no match is found after checking all validations, then you can raise the error. def _onchange_phone_validation(self):
phone_validation = self.env["spp.phone.validation"].search([("state", "=", "active")])
if not phone_validation:
return
for record in self:
if not record.phone_no:
continue
phone_no = record.phone_no
is_valid = False
for validation in phone_validation:
if validation.with_prefix:
pattern = r"^\+?" + re.escape(validation.prefix) + r"\d{" + str(validation.number_of_digits) + r"}$"
else:
pattern = r"^\d{" + str(validation.number_of_digits) + r"}$"
if re.match(pattern, phone_no):
is_valid = True
break
if not is_valid:
error_msg = [v.name for v in phone_validation]
message = "Phone number must match one of the following formats: " + ", ".join(error_msg)
raise ValidationError(_(message))
return |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
from odoo import api, fields, models | ||
|
||
|
||
class SPPPhoneValidation(models.Model): | ||
_name = "spp.phone.validation" | ||
_description = "SPP Phone Validation" | ||
|
||
name = fields.Char(string="Sample Format", compute="_compute_name") | ||
number_of_digits = fields.Integer(string="Number of Digits", required=True) | ||
with_prefix = fields.Boolean(string="With Prefix") | ||
prefix = fields.Char(string="Prefix") | ||
state = fields.Selection( | ||
[("active", "Active"), ("inactive", "Inactive")], | ||
string="State", | ||
default="active", | ||
) | ||
|
||
@api.depends("number_of_digits", "with_prefix", "prefix") | ||
def _compute_name(self): | ||
for record in self: | ||
if record.with_prefix and record.prefix: | ||
record.name = f"{record.prefix}{'X'*record.number_of_digits}" | ||
else: | ||
record.name = "X" * record.number_of_digits | ||
|
||
def activate_phone_validation(self): | ||
for record in self: | ||
record.state = "active" | ||
|
||
def deactivate_phone_validation(self): | ||
for record in self: | ||
record.state = "inactive" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
from . import test_res_partner | ||
from . import test_phone_number_validation |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
from odoo.exceptions import ValidationError | ||
from odoo.tests import TransactionCase | ||
|
||
|
||
class TestPhoneValidation(TransactionCase): | ||
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. The current test suite is a good start, but it could be improved to cover more cases:
Example for point 1: # Test create
with self.assertRaises(ValidationError):
self.phone_model.create({
"partner_id": self.registrant.id,
"phone_no": "12345",
"country_id": self.env.ref("base.ph").id,
})
# Test write
phone = self.phone_model.create(...) # with valid data
with self.assertRaises(ValidationError):
phone.write({"phone_no": "12345"}) |
||
@classmethod | ||
def setUpClass(cls): | ||
super().setUpClass() | ||
cls.partner_model = cls.env["res.partner"] | ||
cls.phone_validation_model = cls.env["spp.phone.validation"] | ||
cls.phone_model = cls.env["g2p.phone.number"] | ||
cls.registrant = cls.partner_model.create( | ||
{ | ||
"name": "Test Registrant", | ||
"is_registrant": True, | ||
} | ||
) | ||
cls.phone_validation_1 = cls.phone_validation_model.create( | ||
{ | ||
"number_of_digits": 10, | ||
"with_prefix": True, | ||
"prefix": "+63", | ||
"state": "active", | ||
} | ||
) | ||
cls.phone_validation_2 = cls.phone_validation_model.create( | ||
{ | ||
"number_of_digits": 11, | ||
"with_prefix": False, | ||
"state": "active", | ||
} | ||
) | ||
|
||
def test_01_create_phone_with_invalid_number(self): | ||
phone = self.phone_model.create( | ||
{ | ||
"partner_id": self.registrant.id, | ||
"phone_no": "+639123456789", | ||
"country_id": self.env.ref("base.ph").id, | ||
} | ||
) | ||
|
||
with self.assertRaises(ValidationError) as cm: | ||
phone.phone_no = "12345" | ||
phone._onchange_phone_validation() | ||
|
||
self.assertIn("Phone number must match one of the following formats", str(cm.exception)) | ||
|
||
def test_02_create_phone_with_valid_number_with_prefix(self): | ||
phone = self.phone_model.create( | ||
{ | ||
"partner_id": self.registrant.id, | ||
"phone_no": "+639123456789", | ||
"country_id": self.env.ref("base.ph").id, | ||
} | ||
) | ||
phone._onchange_phone_validation() | ||
self.assertEqual(phone.phone_no, "+639123456789") | ||
|
||
def test_03_create_phone_with_valid_number_without_prefix(self): | ||
phone = self.phone_model.create( | ||
{ | ||
"partner_id": self.registrant.id, | ||
"phone_no": "09123456789", | ||
"country_id": self.env.ref("base.ph").id, | ||
} | ||
) | ||
phone._onchange_phone_validation() | ||
self.assertEqual(phone.phone_no, "09123456789") |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,74 @@ | ||||||
<?xml version="1.0" encoding="UTF-8" ?> | ||||||
<!-- | ||||||
Part of OpenSPP Registry. See LICENSE file for full copyright and licensing details. | ||||||
--> | ||||||
<odoo> | ||||||
<record id="view_phone_validation_tree" model="ir.ui.view"> | ||||||
<field name="name">view_phone_validation_tree</field> | ||||||
<field name="model">spp.phone.validation</field> | ||||||
<field name="priority">1</field> | ||||||
<field name="arch" type="xml"> | ||||||
<tree create="true" edit="true" delete="false" editable="top"> | ||||||
<field name="number_of_digits" /> | ||||||
<field name="with_prefix" /> | ||||||
<field name="prefix" /> | ||||||
<field name="name" /> | ||||||
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. The
Suggested change
|
||||||
<field | ||||||
name="state" | ||||||
decoration-danger="state == 'inactive'" | ||||||
decoration-success="state == 'active'" | ||||||
readonly="1" | ||||||
/> | ||||||
<button | ||||||
name="activate_phone_validation" | ||||||
title="Activate" | ||||||
type="object" | ||||||
icon="fa-check" | ||||||
class="btn-success" | ||||||
invisible="state == 'active'" | ||||||
/> | ||||||
<button | ||||||
name="deactivate_phone_validation" | ||||||
title="Deactivate" | ||||||
type="object" | ||||||
icon="fa-times" | ||||||
class="btn-danger" | ||||||
invisible="state == 'inactive'" | ||||||
/> | ||||||
</tree> | ||||||
</field> | ||||||
</record> | ||||||
|
||||||
<record id="action_phone_validation" model="ir.actions.act_window"> | ||||||
<field name="name">Phone Validation</field> | ||||||
<field name="type">ir.actions.act_window</field> | ||||||
<field name="res_model">spp.phone.validation</field> | ||||||
<field name="view_mode">tree</field> | ||||||
<field name="context">{}</field> | ||||||
<field name="domain">[]</field> | ||||||
<field name="help" type="html"> | ||||||
<p class="o_view_nocontent_smiling_face"> | ||||||
Add a Phone Validation! | ||||||
</p><p> | ||||||
Click the create button to enter a phone validation. | ||||||
</p> | ||||||
</field> | ||||||
</record> | ||||||
|
||||||
<record id="action_phone_validation_tree_view" model="ir.actions.act_window.view"> | ||||||
<field name="sequence" eval="1" /> | ||||||
<field name="view_mode">tree</field> | ||||||
<field name="view_id" ref="view_phone_validation_tree" /> | ||||||
<field name="act_window_id" ref="action_phone_validation" /> | ||||||
</record> | ||||||
|
||||||
<menuitem | ||||||
id="menu_phone_validation" | ||||||
name="Phone Validation" | ||||||
action="action_phone_validation" | ||||||
parent="g2p_registry_base.g2p_configuration_menu_root" | ||||||
sequence="10" | ||||||
groups="g2p_registry_base.group_g2p_admin,g2p_registry_base.group_g2p_registrar" | ||||||
/> | ||||||
|
||||||
</odoo> |
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
create
method is decorated with@api.model_create_multi
but is defined as an instance method (def create(self, vals):
). According to Odoo conventions, it should be a class method. This is confusing and can lead to unexpected behavior. Please refactor it to use the standard signaturedef create(cls, vals_list):
and conventional variable names.