Skip to content

fix(settings): stop Save All when the business phone is invalid - #1504

Merged
khaira777 merged 1 commit into
mainfrom
fix/settings-save-all-phone-validation
Nov 7, 2026
Merged

khaira777 merged 1 commit into
mainfrom
fix/settings-save-all-phone-validation

Conversation

@khaira777

Copy link
Copy Markdown
Contributor

Related work

Issue / Discussion: None (audit finding F-14)

Summary

saveBusinessInfo returned undefined on an invalid business phone instead of throwing. In the Save All effect, Promise.all therefore resolved: loyalty, discount, cloud, numbering, printing and bill template were written, and "Settings saved" appeared right after the "Invalid phone number format" toast.

Save All now checks the phone with the same normalizeOptionalPhone call before any write. If it is invalid, nothing is saved, only the phone error shows, and the save bar stays dirty. The existing finally resets the in-flight state.

Scope

The order-numbering validation in saveOrderNumbering cannot fail from the UI (both prefix inputs strip to [A-Z0-9] with a 12-character limit), so it gets no preflight.

Verification

Commands run:

  • npm run lint, npm run build:frontend
  • Playwright on isolated ports: npx playwright test e2e/settings-request-budget.spec.ts --project=chromium
  • oxfmt --check, git diff --check

Results: all passed. The new test "Save All with an invalid business phone writes nothing and does not report success" fails without the fix (checked against a frontend build with the fix removed).

Compatibility & data safety

  • Database/data impact: None. This prevents a partial save.
  • Migration: None
  • User-visible behavior: Save All with an invalid business phone saves nothing and shows only the phone error
  • Offline/network impact: None

Cross-cutting surfaces

Surface Status
UI affected
API not affected
Persistence affected (no writes on validation failure)
Permissions not affected

Developer documentation

  • No developer documentation change needed
  • Existing developer documentation updated
  • New page justified
  • Not applicable (docs-only or code-only change)

Contributor checklist

  • This is a small isolated fix/doc update OR follows an approved issue/direction.
  • Unrelated cleanups, refactors, and dependency changes were kept out of this PR.
  • Tests were added or updated for any behavior changes.
  • Verification commands were run and documented above.
  • Customer data and upgrade safety were preserved (if touching database/storage). (N/A)
  • Documentation or translations were updated where relevant.
  • I understand and can explain all submitted changes.

saveBusinessInfo returned without throwing on an invalid phone, so Save All still wrote the other sections and reported Settings saved. Validate the phone before any write so Save All saves nothing and shows only the phone error.
@coderabbitai

coderabbitai Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Prevent partial Save All when the business phone is invalid

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Validate the business phone before Save All writes, preventing partial saves and false success
 messages.
• Add a browser regression test confirming no settings writes occur and edits remain unsaved.
Diagram

graph TD
  A["Save Changes"] --> B["Hydrate tabs"] --> C{"Phone valid?"} -->|Yes| D["Save settings"] --> E["Success toast"] --> F["Reset in-flight"]
  C -->|No| G["Phone error"] --> F
Loading
High-Level Assessment

Preflighting with the existing phone validator is the right approach. Making the business save throw instead would not prevent other saves already started by Promise.all.

Files changed (2) +28 / -0

Bug fix (1) +4 / -0
page.tsxValidate the phone before Save All writes +4/-0

Validate the phone before Save All writes

• Checks the hydrated business phone with the existing optional-phone normalizer before starting the settings saves. Invalid input shows the phone error and returns, while the existing finally block clears the in-flight state.

frontend/src/app/(dashboard)/settings/page.tsx

Tests (1) +24 / -0
settings-request-budget.spec.tsCover invalid-phone Save All behavior +24/-0

Cover invalid-phone Save All behavior

• Adds a Playwright test that enters an invalid business phone and clicks Save Changes. It verifies the phone error, absence of a success message and settings PUT requests, and an enabled save button for the unsaved edits.

frontend/e2e/settings-request-budget.spec.ts

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@khaira777
khaira777 merged commit 5c6b780 into main Nov 7, 2026
23 checks passed
@khaira777
khaira777 deleted the fix/settings-save-all-phone-validation branch November 7, 2026 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant