Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Jul 16, 2025

What do these changes do?

✨ Add phone number validation

Add phone number validation using pydantic_extra_types.phone_numbers to prevent SMS waste. Note that this validation only happens in input schemas, namely the phone field in:

  • the account request form
  • the phone registration (on creation)
  • the phone registration (on profile)

ReDoc

image

⬆️ adds phonenumber library

Adds in models_library and propagates downstream from models_library requirements

♻️ Normalized error in request_validation to comply with EnvelopedError

Found this issue and decided to refactor it properly. Now, 422 validation errors display correct enveloped error model. For instance

{
 "error": {
  "message": "Invalid parameter/s 'label' in request query",
  "status": 422,
  "errors": [
   {
    "code": "missing",
    "message": "Field required",
    "resource": "/projects/9a164106-cf6a-459e-b486-2b21fb97d435",
    "field": "label"
   }
  ]
 }
}

Related issue/s

How to test

cd packages/models_library
make install-dev
pytest -vv tests/test_users.py
image

Dev-ops

None

@codecov
Copy link

codecov bot commented Jul 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.20%. Comparing base (49fe1e2) to head (8bc1c1c).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8115      +/-   ##
==========================================
+ Coverage   87.67%   88.20%   +0.52%     
==========================================
  Files        1700     1890     +190     
  Lines       66037    72631    +6594     
  Branches      822     1277     +455     
==========================================
+ Hits        57900    64061    +6161     
- Misses       7920     8189     +269     
- Partials      217      381     +164     
Flag Coverage Δ
integrationtests 64.25% <90.32%> (+<0.01%) ⬆️
unittests 86.82% <100.00%> (+0.66%) ⬆️
Components Coverage Δ
pkg_aws_library 93.93% <ø> (ø)
pkg_celery_library 87.34% <ø> (ø)
pkg_dask_task_models_library 79.62% <ø> (ø)
pkg_models_library 93.15% <100.00%> (ø)
pkg_notifications_library 85.26% <ø> (ø)
pkg_postgres_database 88.22% <ø> (ø)
pkg_service_integration 70.17% <ø> (ø)
pkg_service_library 71.48% <100.00%> (∅)
pkg_settings_library 90.45% <ø> (ø)
pkg_simcore_sdk 85.10% <ø> (+0.05%) ⬆️
agent 93.81% <ø> (ø)
api_server 93.02% <ø> (ø)
autoscaling 95.88% <ø> (ø)
catalog 92.34% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.81% <ø> (-0.57%) ⬇️
datcore_adapter 97.94% <ø> (ø)
director 76.14% <ø> (-0.10%) ⬇️
director_v2 91.06% <ø> (+0.04%) ⬆️
dynamic_scheduler 96.27% <ø> (ø)
dynamic_sidecar 90.07% <ø> (ø)
efs_guardian 89.76% <ø> (ø)
invitations 91.44% <ø> (ø)
payments 92.60% <ø> (ø)
resource_usage_tracker 92.55% <ø> (+0.37%) ⬆️
storage 86.69% <ø> (+0.20%) ⬆️
webclient ∅ <ø> (∅)
webserver 88.53% <100.00%> (+6.78%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49fe1e2...8bc1c1c. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pcrespov pcrespov self-assigned this Jul 16, 2025
@pcrespov pcrespov changed the title WIP: Is280/validate phone numbers ✨ feat(phone): Add Pydantic phone number validation to reduce SMS waste in input schemas Jul 16, 2025
@pcrespov pcrespov force-pushed the is280/validate-phone-numbers branch from f6154b0 to 53c8407 Compare July 16, 2025 11:05
@pcrespov pcrespov added this to the Engage milestone Jul 16, 2025
@pcrespov pcrespov marked this pull request as ready for review July 16, 2025 16:38
@pcrespov pcrespov enabled auto-merge (squash) July 16, 2025 16:41
@pcrespov pcrespov disabled auto-merge July 16, 2025 16:41
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

Thanks!
Nevertheless I more and more find that the way we install our packages is wrong. Why would for example the clusers-keeper care about phone numbers... and now all of these services need to install an additional library even though only one uses it. This blows up the container image sizes, and decreases security. nothing to do now though. that is just a thought.

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

fancy!

Copy link
Contributor

@giancarloromeo giancarloromeo left a comment

Choose a reason for hiding this comment

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

👌

Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

Thanks!

@pcrespov pcrespov force-pushed the is280/validate-phone-numbers branch 2 times, most recently from 19bcf78 to f2a1c8f Compare July 17, 2025 11:59
@pcrespov pcrespov added the a:webserver webserver's codebase. Assigning the area is particularly useful for bugs label Jul 17, 2025
@pcrespov pcrespov enabled auto-merge (squash) July 17, 2025 13:37
@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label Jul 17, 2025
@pcrespov
Copy link
Member Author

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Jul 17, 2025

queue

🟠 Waiting for conditions to match

  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • any of: [🛡 GitHub branch protection]
        • check-neutral = deploy to dockerhub
        • check-skipped = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-neutral = system-tests
        • check-skipped = system-tests
        • check-success = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-neutral = unit-tests
        • check-skipped = unit-tests
        • check-success = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
        • check-success = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-neutral = integration-tests
        • check-skipped = integration-tests
        • check-success = integration-tests
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • label!=🤖-do-not-merge
      • label=🤖-automerge
      • any of: [🛡 GitHub branch protection]
        • check-success = build-test-images (frontend) / build-test-images
        • check-neutral = build-test-images (frontend) / build-test-images
        • check-skipped = build-test-images (frontend) / build-test-images
  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

@pcrespov pcrespov force-pushed the is280/validate-phone-numbers branch from fe78048 to da58d27 Compare July 17, 2025 14:39
@pcrespov
Copy link
Member Author

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Jul 18, 2025

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • label!=🤖-do-not-merge
      • label=🤖-automerge
      • any of: [🛡 GitHub branch protection]
        • check-skipped = deploy to dockerhub
        • check-neutral = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-success = system-tests
        • check-neutral = system-tests
        • check-skipped = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = unit-tests
        • check-neutral = unit-tests
        • check-skipped = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = check OAS' are up to date
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-success = integration-tests
        • check-neutral = integration-tests
        • check-skipped = integration-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = build-test-images (frontend) / build-test-images
        • check-neutral = build-test-images (frontend) / build-test-images
        • check-skipped = build-test-images (frontend) / build-test-images

@pcrespov pcrespov disabled auto-merge July 18, 2025 14:40
@sonarqubecloud
Copy link

@pcrespov pcrespov merged commit 28dd617 into ITISFoundation:master Jul 18, 2025
89 of 95 checks passed
@pcrespov pcrespov deleted the is280/validate-phone-numbers branch July 18, 2025 14:55
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Aug 5, 2025
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:models-library a:webserver webserver's codebase. Assigning the area is particularly useful for bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants