-
Notifications
You must be signed in to change notification settings - Fork 51
30397 fix phone validation #1885
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
30397 fix phone validation #1885
Conversation
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.
Pull Request Overview
This PR fixes phone number validation by replacing the string punctuation removal approach with a more robust regex-based solution that removes all non-digit characters.
Key changes:
- Replaces
string.maketranswithre.subfor phone number cleaning - Changes from removing only punctuation to removing all non-digit characters
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
severinbeauvais
left a comment
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.
Namex API has version number somewhere. Can you please update it?
Eg: https://namex-dev.apps.silver.devops.gov.bc.ca/api/v1/meta/info
severinbeauvais
left a comment
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.
LGTM but wait for more reviews and consider CP's suggestion.
…fix_phone_validation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 9442085 | Triggered | Generic Password | 05ee813 | api/.env.sample | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Bobby, this is referring to a file you didn't change, but that was picked up in your rebase. Anyway, I think it's nothing that needs to be changed in your PR. cc: @bolyachevets , looks like this was from your commit yesterday (#1883 and #1884). |
it is a generic placeholder username postgres/postgres for unit tests using local db - nothing to worry about |
loneil
left a comment
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.
Not really anything to add other than the copilot notes (though the string import removal looks to be fine). First time looking at this repo myself as well.
Maybe could additionally be a good idea to extract the duplicate regex call to some normalize_phone_number helper function, then could be good to have a targetted unit test that tries out all the phone number options like
@pytest.mark.parametrize("input_phone,expected", [
("(555) 123-4567", "5551234567"),
("+1-555-123-4567", "15551234567"),
("555.123.4567", "5551234567"),
("555 123 4567", "5551234567"),
("5551234567", "5551234567"),
("", ""),
(None, ""),
])
def test_normalize_phone_number(input_phone, expected):
result = normalize_phone_number(input_phone or "")
assert result == expected
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 code changes look good to me.
|
@bobdev-94 , what do you think about create a unit test like Lucas suggested? |
|
severinbeauvais
left a comment
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.
LGTM!
But we will need a review from someone on the Names Team.
|
@stevenc987 , @eve-git , @mengdong19 Could you guys review this when you have a moment, please? |



Issue: bcgov/entity#30397
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the namex license (Apache 2.0).