[ADD] phone validations #844
Conversation
github-actions
bot
requested review from
gonzalesedwin1123,
jeremi and
reichie020212
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @emjay0921, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the spp_registry_base module by implementing a flexible and configurable phone number validation system. It allows the system to enforce specific formats for phone numbers, improving data quality and consistency. Users can define custom validation rules, which are then applied automatically when phone numbers are entered or modified.
Highlights
- Configurable Phone Number Validation: Introduces a new configuration to define and manage multiple accepted phone number formats, including options for prefixes and specific digit counts.
- Automated Validation on Creation/Update: Phone numbers are now automatically validated against the active configured formats when a phone number record is created or updated, preventing invalid entries.
- Dedicated UI for Validation Rules: A new user interface has been added under 'Registry > Configuration > Phone Validation' to allow administrators to easily add, activate, and deactivate phone number validation rules.
- Comprehensive Unit Tests: New unit tests have been added to cover various scenarios for phone number validation, ensuring the robustness of the new feature.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable phone number validation system, which is a great addition. The implementation is mostly solid, with a new model for validation rules and logic integrated into the phone number model. However, I've found a critical issue where the validation logic doesn't handle operations on multiple records, which will lead to errors. I've also identified some areas for improvement regarding Odoo conventions, test coverage, and UI clarity. My detailed comments and suggestions are below.
| 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 |
There was a problem hiding this comment.
This method is called from create and write, which can operate on multiple records at once. However, the current implementation does not handle multi-recordsets (self). For example, self.phone_no will raise an error if self contains more than one record. The entire logic should be wrapped in a loop to iterate over each record in self.
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| @api.model_create_multi | ||
| def create(self, vals): | ||
| record = super().create(vals) | ||
| record._onchange_phone_validation() | ||
| return record |
There was a problem hiding this comment.
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 signature def create(cls, vals_list): and conventional variable names.
| @api.model_create_multi | |
| def create(self, vals): | |
| record = super().create(vals) | |
| record._onchange_phone_validation() | |
| return record | |
| @api.model_create_multi | |
| def create(cls, vals_list): | |
| records = super().create(vals_list) | |
| records._onchange_phone_validation() | |
| return records |
| from odoo.tests import TransactionCase | ||
|
|
||
|
|
||
| class TestPhoneValidation(TransactionCase): |
There was a problem hiding this comment.
The current test suite is a good start, but it could be improved to cover more cases:
- Directly test
createandwritefailures: Instead of manually calling_onchange_phone_validation, you should test thatcreate()andwrite()raise aValidationErrorwhen provided with invalid phone numbers. This will properly test the ORM method overrides. - Test multi-record operations: The identified issue with multi-recordset handling in
_onchange_phone_validationwas not caught because the tests only operate on single records. Please add tests for creating and writing multipleg2p.phone.numberrecords at once.
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"})| <field name="number_of_digits" /> | ||
| <field name="with_prefix" /> | ||
| <field name="prefix" /> | ||
| <field name="name" /> |
There was a problem hiding this comment.
The name field is computed by the _compute_name method and is not stored in the database. In an editable tree view (editable="top"), making a non-stored computed field editable can be confusing for the user, as any changes they make will be lost. It's better to make it readonly.
| <field name="name" /> | |
| <field name="name" readonly="1" /> |
Why is this change needed?
Add a way to have multiple accepted format on phone numbers
How was the change implemented?
Added a new configuration to be able to add different formats for phone, and use that as validation
New unit tests
Unit tests executed by the author
How to test manually
spp_registry_baseRelated links