Skip to content

fix(booking): validate phone numbers on the guest and custom-field paths - #335

Merged
harshtandiya merged 4 commits into
developfrom
claude/issue-325-plan-m1wyar
Aug 11, 2026
Merged

fix(booking): validate phone numbers on the guest and custom-field paths#335
harshtandiya merged 4 commits into
developfrom
claude/issue-325-plan-m1wyar

Conversation

@harshtandiya

@harshtandiyaharshtandiya commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Fixes#325.

Problem

On an event with guest_verification_method = "Phone OTP", a guest could type abcd into the guest Phone Number field and submit. The server cached an OTP against abcd and handed it to the SMS gateway — Frappe's validate_receiver_nos only strips " -()" and only throws when the list is empty, so the string reached the provider.

Three gaps, all confirmed in the code:

  1. send_booking_otp validated the email branch with validate_email_address(..., throw=True) and validated nothing in the phone branch.
  2. Booking-level Phone custom fields were written to additional_fields raw, while attendee-level ones already went through validate_custom_fields.
  3. The guest field was a plain FormControl type="tel", which filters nothing — unlike PhoneInput, used everywhere else, which already strips non-digits.

Changes

buzz/api/booking/guests.py — validate the phone branch of send_booking_otp with validate_phone_number_with_country_code. Validate only, never rewrite: the OTP cache key is built from identifier here and rebuilt from the submitted phone in BookingService.verify_guest, so normalising one side and not the other would silently break verification.

buzz/api/booking/services.py — one validate_phone_fields, called from process() before build_booking(), covers booking-level and attendee-level Phone custom fields. Ordering is the point: verify_guest_otp deletes the code from the cache and no rollback puts it back, so validating later (as append_custom_fields and attendee_row did) meant a mistyped phone cost the guest a valid OTP and a fresh round of SMS. It also left Attendee Ticket Add-on rows inserted for earlier attendees before a later one failed. verify_guest validates the guest phone at submit too, so the direct-API path is covered, not just the OTP send.

dashboard/src/components/BookingForm.vue — swap the guest phone FormControl for the existing PhoneInput. It blocks alphabets at the keystroke and emits the canonical +91-9000090000 the validator needs to pick the right per-country length. validate_receiver_nos strips the hyphen before the gateway, so SMS delivery is unaffected. The placeholder is passed through explicitly because the existing e2e spec selects on it. sendOtpForVerification clears the error before submitting, so a delivery failure does not leave a stale message under the field on the next attempt.

dashboard/src/components/PhoneInput.vue — optional error prop rendering frappe-ui's ErrorMessage under the field. Existing callers (CustomFieldInput.vue, ProposalEditDialog.vue) pass nothing and are unchanged. No client-side phone library was added: length rules live on the server, which already owns them.

Length is deliberately left to phonenumbers rather than a hardcoded ten digits — the dial code picker offers every country, so a fixed length would break US, UK and others.

E2E fixtures no longer infer a team

Second commit, needed to run the suite at all. Team-direct doctypes make team mandatory, and set_team_from_sole_membership fills it only when the acting user has exactly one enabled membership. The fixtures relied on that, which holds on a fresh CI site and nowhere else — a developer whose Administrator sits on two teams cannot create so much as an Event Host, and the guest, check-in, tickets and offline-payment projects all die in setup with Value missing for Buzz Event: Team.

ensureTestTeam finds or creates a team named "E2E Team" and returns its name for the fixtures to stamp on the twelve Buzz Event / Event Host creations. The list call is already scoped to the acting user's own teams, so a same-named team belonging to somebody else stays invisible, and BuzzTeam.after_insert supplies the membership.

No production code changes here. Refusing to pick a team on the user's behalf is the #312 behaviour and stays; the Desk form has always posted the field.

Tests

  • TestSendGuestBookingOtpabcd and +91-12345 are refused and leave no cache key; a valid number still returns the code.
  • TestBookingPhoneCustomFields — an invalid booking-level Phone custom field is refused, a valid one persists to additional_fields, the attendee-level path stays gated, and a rejected phone leaves the guest's OTP in the cache. That last one fails if the validation moves back after resolve_user; verified by moving it and watching it fail.
  • e2e/tests/guest-booking.spec.ts — a too-short number surfaces the message under the field rather than opening the OTP dialog.

Verification

  • bench run-tests --module buzz.api.booking.test_booking: 26 pass.
  • ./typecheck.sh and pre-commit run --files on every changed file: clean. The e2e tsc errors are the same 8 before and after.
  • Playwright guest-chromium against a live site: setup passes, 5 of 7 pass. The two guest booking with … OTP cases fail on expect(otp).toBeTruthy() because send_booking_otp returns the code only under frappe.in_test, which a live bench start never sets. Confirmed pre-existing by stashing every change and re-running — same two failures. They pass in CI, where the whole run is a test site.

Generated by Claude Code

claudeand others added 4 commits August 11, 2026 13:24
Guest bookings on Phone OTP events accepted any string as a phone number.
`send_booking_otp` validated the email branch but not the phone branch, so
`abcd` was cached as an OTP key and handed to the SMS gateway — Frappe's
`validate_receiver_nos` only strips " -()" and only throws on an empty list.
Booking-level Phone custom fields had a matching gap: attendee-level fields
went through `validate_custom_fields`, booking-level ones were written raw.
- guests.py: validate the phone branch of `send_booking_otp` with
`validate_phone_number_with_country_code`. Validate only, never rewrite —
the OTP cache key is built from the identifier here and rebuilt from the
submitted phone in `verify_guest`, so normalising one side would silently
break verification.
- services.py: gate `append_custom_fields` on `validate_custom_fields`, and
validate the guest phone at submit as well as at OTP send. Make
`phone_field_labels` a cached_property so both append paths share one query.
- BookingForm.vue: replace the unvalidated `FormControl type="tel"` with the
existing PhoneInput, which blocks alphabets and emits the canonical
"+91-9000090000" the validator needs to pick the right per-country length.
- PhoneInput.vue: optional `error` prop rendering ErrorMessage under the
field, so the server's message lands next to the input instead of a toast.
Length is deliberately left to `phonenumbers` rather than a hardcoded ten
digits, which would break every country but India.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0169SVTCxRBMMz6kQETDUAJz
verify_guest_otp deletes the code from the cache, and no rollback puts it back.
Validation sat in append_custom_fields and attendee_row, both of which run after
resolve_user, so a mistyped phone cost the guest a valid OTP and a fresh round of
SMS. It also left behind Attendee Ticket Add-on rows inserted for earlier
attendees before a later one failed.
Both call sites now fold into one validate_phone_fields, called from process()
ahead of build_booking. phone_field_labels is back to a plain method with the one
caller that remains, so cached_property and its import go with it.
sendOtpForVerification clears guestPhoneError before submitting, so a delivery
failure does not leave a stale message under the field on the next attempt.
The e2e case dropped its alphabet assertion: PhoneInput has always stripped
non-digits, so that half tested untouched behaviour rather than this change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Team-direct doctypes make `team` mandatory and set_team_from_sole_membership
fills it only when the acting user has exactly one enabled membership. The
fixtures relied on that, which holds on a fresh CI site and nowhere else: a
developer whose Administrator sits on two teams cannot create so much as an
Event Host, and every guest, check-in, tickets and offline-payment project dies
in setup with "Value missing for Buzz Event: Team".
ensureTestTeam finds or creates a team named "E2E Team" and hands back its name
for the fixtures to stamp. The list call is already scoped to the acting user's
own teams, so a same-named team belonging to somebody else stays invisible, and
BuzzTeam.after_insert supplies the membership.
Nothing changes in the resolver: refusing to pick a team on the user's behalf is
the intended behaviour, and the Desk form has always posted the field.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@harshtandiya
harshtandiya marked this pull request as ready for review August 11, 2026 17:16
@harshtandiyaharshtandiya added the backport main backport to main branch label Aug 11, 2026
@greptile-apps

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adds server-side phone validation to guest OTP issuance, guest booking submission, and booking/attendee custom fields, while updating the guest form to use the shared formatted phone input. It also makes E2E fixtures explicitly create and assign a stable test team.

  • Validates phone identifiers before creating or consuming guest OTPs.
  • Validates booking- and attendee-level Phone custom fields before booking side effects.
  • Displays guest-phone delivery and validation errors beneath the phone input.
  • Explicitly assigns an E2E team to fixture events and hosts.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete changed-code failure identified.

Phone validation now occurs before OTP and booking side effects, both OTP paths retain identical identifiers, and the fixture changes explicitly supply required team values without establishing a reachable regression.

Important Files Changed

FilenameOverview
buzz/api/booking/guests.pyAdds phone-number validation before OTP generation and cache insertion while preserving the submitted identifier as the cache key.
buzz/api/booking/services.pyMoves Phone custom-field validation ahead of booking side effects and validates guest phone numbers before OTP consumption.
buzz/api/booking/test_booking.pyAdds regression coverage for invalid guest and custom-field phone numbers, valid persistence, and preservation of OTP cache state on early rejection.
dashboard/src/components/BookingForm.vueReplaces the raw guest phone control with PhoneInput and routes phone OTP errors to the field.
dashboard/src/components/PhoneInput.vueAdds optional inline ErrorMessage rendering without changing existing callers that omit the new prop.
e2e/helpers/frappe.tsAdds an idempotent helper that finds or creates the named E2E team for fixture assignment.
e2e/tests/auth.setup.tsEnsures the shared E2E team exists during authentication setup.
e2e/tests/check-in.setup.tsAssigns the explicit E2E team to check-in host and event fixtures.
e2e/tests/custom-forms.setup.tsAssigns the explicit E2E team to custom-form host and event fixtures.
e2e/tests/event.setup.tsAssigns the explicit E2E team to the primary booking host and event fixtures.
e2e/tests/guest-booking.spec.tsCovers server rejection and inline display of a too-short guest phone number.
e2e/tests/guest-event.setup.tsAssigns the explicit E2E team to guest-booking host and event fixtures.
e2e/tests/offline-payment.setup.tsAssigns the explicit E2E team to offline-payment host and event fixtures.
e2e/tests/tickets.setup.tsAssigns the explicit E2E team to ticket-test host and event fixtures.

Reviews (1): Last reviewed commit: "test(e2e): stamp an explicit team on the..." | Re-trigger Greptile

@harshtandiya
harshtandiya merged commit 23644b7 into developAug 11, 2026
10 checks passed
@harshtandiya
harshtandiya deleted the claude/issue-325-plan-m1wyar branch August 11, 2026 18:14
@github-actions

Copy link
Copy Markdown
Contributor

Backport failed for main, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin main
git worktree add -d .worktree/backport-335-to-main origin/main
cd .worktree/backport-335-to-main
git switch --create backport-335-to-main
git cherry-pick -x 23644b70b4db074f37a50a758d51f72949af36cd

harshtandiya added a commit that referenced this pull request Aug 12, 2026
…ths (backport #335) (#336)
(cherry picked from commit 23644b7)
The e2e team fixtures from the original PR are dropped: main has no Buzz
Team doctype (team-based multi-tenancy, #312, is develop-only), so
`ensureTestTeam` and the `team` stamps have nothing to bind to here.
Claude-Session: https://claude.ai/code/session_01EePTfzyKVxj7bcpbF9roWx
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport mainbackport to main branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Phone number fields accept alphabets and invalid lengths

2 participants

@harshtandiya@claude