# Settings (5-tab) — Staging vs Mockup Parity Fix

**Status:** DONE — Payments critical bug fixed; Bank/IBAN inline editing, min-withdrawal
default, and the Notifications in-app/3h-reminder fixes all implemented per your answers
in §6 and browser-verified. Uncommitted.
**Branch:** tailwind-poc · **Created:** 2026-07-21, decisions resolved same day.
**Trigger:** design-review bug report across all 5 Settings tabs (General, Scheduling,
Payments, Commercials, Notifications).

## Method

Every finding below was checked against the **live rendered page**
(`shop.navagoo.localhost/settings/index?tab=...`, logged in as the standing dev
shop-owner test account), not just source reading — source reading alone was
actively misleading on the Payments tab (see §1). `get_page_text` / DOM dumps are
quoted where they drove a conclusion.

## 1. Payments — CRITICAL bug, FIXED

**Report:** all three methods mislabeled "Walk-in"; wrong control model (one toggle +
checkbox instead of two toggles per method); missing Deposit % field; missing helper text.

**Live repro (before fix)** — `get_page_text` on `?tab=payments`:
```
Payment Methods
Which payment timings the app offers at checkout.
Walk-in
Walk-in
Walk-in
Save Payment Methods
```
Titles/descriptions ("Pay 100% Online", "Pay Deposit", "Pay on Visit" + their sentences)
were **completely absent** from the DOM — not a CSS/visibility issue, the text was never
rendered. The Deposit % field and helper text further down in the source *do* exist in
the template but are unreachable in that state (see below).

**Root cause** — confirmed against `vendor/yiisoft/yii2-bootstrap4/src/ActiveField.php:250-270`:
`ActiveField::checkbox($options, $enclosedByLabel)` reads `'template'` from **its own**
`$options` argument, not from the field-config array passed to `$form->field($model,
$attr, [...])`. `frontend/views/settings/_payments.php` (and the orphaned legacy
`frontend/views/payment-settings/index.php`, see §7) passed the custom pill template
(icon + title + description markup around `{input}`/`{error}`) via the **field-config**
array, then called `->checkbox([...])` with a *different* options array that had no
`'template'` key. `checkbox()`'s own logic:
```php
if (!isset($options['template'])) {
    $this->template = ($enclosedByLabel) ? $this->checkEnclosedTemplate : $this->checkTemplate;
} else {
    $this->template = $options['template'];
}
```
unconditionally **overwrites** `$this->template` back to Bootstrap4's default checkbox
markup the instant `->checkbox()` runs, discarding the field-config template entirely.
That's why only "Walk-in" (written as plain sibling markup in the PHP, never routed
through a field template) ever showed text — every row's own title/desc lived *only*
inside the template that got clobbered.

**Fix applied** (`frontend/views/settings/_payments.php`, 3 fields: `pay_online_enabled`,
`pay_deposit_enabled`, `pay_on_visit_enabled`) — moved `'template' => $pillRow(...)` from
the field-config array into `->checkbox([...])`'s own options array, which is what
`ActiveField::checkbox()` actually reads.

**Verified live (after fix):**
```
Pay 100% Online
Customer pays the full amount online at checkout.
Walk-in
Pay Deposit
Customer pays a deposit online; the rest is paid at the salon.
Walk-in
Pay on Visit
Customer books without paying; pays at the salon during the visit.
Walk-in
```
Screenshot confirms correct icon + bold title + description + working pill toggle for
all three rows. The Deposit % field (existing, capped at the platform max via
`getEffectiveMaxDeposit()`) and its warning banner were already correctly implemented —
they were just never described as "missing" incorrectly once the row above them
renders normally; they show/hide correctly keyed off the Pay Deposit toggle.
`php -l` clean.

### Remaining Payments gap (not yet implemented — needs a short follow-up, no schema change)

The report's **second** complaint — "two toggles per method (APP / WALK-INS)" vs our
"one toggle + plain checkbox" — is still true and is a separate, purely cosmetic fix:
`ShopPaymentSettings` **already has** `walkin_online_enabled` / `walkin_deposit_enabled`
/ `walkin_on_visit_enabled` columns (no migration needed) — they're just rendered as a
small plain checkbox in an indented sub-row instead of a second full pill toggle
side-by-side with the first, per the demo's `PayRow` component
(`Navagoo_MI/navagoo-app/src/portals/shop/Settings.tsx:710-764` — two `<Toggle>`s in a
`flex items-center gap-5` block, each with a tiny `APP`/`WALK-INS` caption underneath).
Scoped out of this pass only because it's a markup/JS restructure (capturing two
`ActiveField::checkbox()` renders and recombining them into one manually-built row)
that deserves its own focused iteration rather than a blind rewrite — not because it's
ambiguous. Ready to implement on request; no open questions block it.

## 2. Commercials — mostly data, not code

**Report:** Subscription plan / Status / Next billing / Settlement hold all show "—";
Min withdrawal ⃁5,000 vs design's ⃁50.

**Live (`get_page_text`, `?tab=commercials`):**
```
Subscription plan   —
Status               —
Next billing         —
Marketing fee rate           5%
Payment processing fee rate  3.5% + ⃁ 1.00
VAT registered       No
Min. withdrawal      ⃁ 5,000.00
Settlement hold (days)       —
```
**Marketing fee rate and Payment processing fee rate render real values** — proving the
wiring pattern (`FinanceLedgerService` → `CommercialConfig`/per-shop override) works
correctly. Subscription plan/Status/Next billing are correctly `—` **because this
specific test shop has no active subscription** (`ShopSubscription::findCurrentForShop()`
returns null) — confirmed independently on the Home tab screenshot for the same shop:
*"No active plan · Subscribe to unlock the full platform"*. This is the fallback the
code is explicitly designed to show, not a wiring bug — **re-test on a shop with an
active subscription** before concluding otherwise.

Settlement hold (days) is `—` independently of the subscription (sourced straight from
`CommercialConfig::settlement_hold_days`, unrelated to `$sub`/`$plan`) while its sibling
config values (marketing/processing rates) resolve fine — this looks like the
`commercial_config` row simply has a `NULL` `settlement_hold_days` in this environment
(a data gap), not a code bug, but wasn't independently confirmed against the DB — flagged
in §6 rather than guessed at.

**Min withdrawal ⃁5,000 vs demo's ⃁50** — a business-config value, not a bug; flagged
rather than guessed at. **Resolved in §6: changed to ⃁50.**

## 3. Notifications — mix of data gaps and real product questions

**Live (`get_page_text`, `?tab=notifications`):** usage tiles correct (SMS 0/50 free,
WhatsApp 0/20 free). Customer table shows 4 triggers — **Booking Confirmed, Booking
Reminder (1 day before · 22:00), Booking Cancelled, Appointment Completed** — every one
of them **In-app: Off**, none showing preview text. Shop table shows 3 triggers — **New
Booking Received, Booking Cancelled by Customer, Settlement Received**.

- **"Reminder — 3 hours before" missing**: only a 1-day-before reminder trigger exists.
  Adding it means inserting a new `NotificationTrigger` row (not a code change — the
  render loop already handles any number of triggers generically) — confirm this is a
  platform-wide addition (affects all shops) before seeding it.
- **No preview text anywhere**: the code's `$previewText()` correctly falls back to `''`
  when a trigger has no `sms_template_*`/`wa_template_*`/`inapp_template_*` content — **all
  4 customer triggers currently have empty templates in this environment**. Not a
  rendering bug; a content/seed gap. Needs real template copy authored per trigger
  (bilingual, per the mandatory i18n rule) before preview text can appear anywhere.
- **In-app defaults to Off for every trigger**: `$inAppOn` is driven by
  `NotificationDispatchService::channelUsable($trigger, CHANNEL_IN_APP)` — a
  **trigger-level** (platform-wide, admin-configured) flag, not a per-shop setting.
  Every trigger in this environment currently has in-app disabled/unapproved. Flipping
  this is a platform-wide data change (or an admin console default), not a per-shop
  code fix — confirm before changing, since it affects every shop, not just this one.
- **Extra triggers vs mockup** (Booking Cancelled, Appointment Completed, Booking
  Cancelled by Customer): these are real, already-built features already firing through
  the same dispatch engine as the mockup's set — removing them would be a functionality
  regression, not a parity fix. Likely the mockup is simply an earlier/thinner snapshot.
  Flagged for confirmation, not touched.

## 4. Scheduling — not a bug (same rule the demo itself models)

**Report:** Calendar slot size is locked to admins; design shows it editable.

**Live:** confirmed via `document.getElementById('settings-slot-step').disabled === true`
for this shop. Server-side: `$shop->isSlotTimeStepLocked() && !user->can('administrator')
&& !user->can('manager')` — a subscription-tier gate. Critically, **the demo's own
Settings.tsx models the identical concept** (`Settings.tsx:196`:
`<Select ... disabled={shop.slotStepLocked} ...>`), just with a mock shop where that flag
happens to be `false`. This is not portal drift from the design — it's data-dependent,
consistent behavior on both sides. Re-test against a shop whose plan doesn't lock this
field before treating it as a bug.

## 5. General — real gaps, two needed a product/compliance decision (see §6)

**Report confirmed on the live page:**
- **Commercial Name is a single field** (`Shop::commercial_name`, plain `string` column,
  no `_ar` variant) — while a *separate* "Shop Title" field two sections down (Basic
  Information) **is already bilingual** via `MyMultiLanguageActiveField`. The demo has
  only ONE bilingual name field (`shop.commercialName` / `commercialNameAr`) — our portal
  appears to have split what the demo treats as one field into two different fields with
  different i18n models. **Resolved in §6: keep both, intentionally distinct** — no
  code change.
- **Bank/IBAN were read-only** ("Add via withdrawal request") — confirmed, by design,
  previously managed exclusively through the withdrawal-request flow. **Resolved in
  §6: made inline-editable** on this tab.
- **Scope check** (Media, Basic Information, Location, Additional Information sections):
  confirmed present and fully functional (image/gallery upload, Google-Maps-backed
  location picker, bilingual about/cancellation-terms, website link) — per the file's own
  docblock, this is the **legacy `/shop-settings` page merged verbatim** into this tab.
  These are real, working features, not build drift — recommend telling whoever owns the
  mockup that it under-scopes the General tab, rather than removing functionality to
  match it.

## 6. Decisions — resolved, and what was implemented for each

1. **Commercial Name vs Shop Title** → **keep both, distinct on purpose.** No code
   change. Commercial Name stays single-language; Shop Title stays the separate
   bilingual public display field.
2. **Bank/IBAN** → **make inline-editable.** Implemented:
   - `common/models/BankAccount.php` — added Saudi-IBAN format validation (`match`
     against `/^SA\d{2}[A-Z0-9]{20}$/`) + an uppercase/strip-spaces normalizing filter,
     centralising the check `AgentsWalletController` previously ran manually inline
     (that controller's own check still runs too — harmless, now redundant-but-safe).
   - `frontend/controllers/SettingsController.php` — `$bankAccount` is now loaded once
     before the POST branch (existing-or-new instance, never null); new
     `saveBankAccount()` method loads+saves it after a successful Shop save, and is a
     deliberate no-op when both fields are left blank (so shops with no bank details
     yet can still save the rest of General cleanly — BankAccount's own rules require
     both fields together once either is filled in).
   - `frontend/views/settings/_general.php` — the read-only `<dl>` became two
     `$form->field($bankAccount, ...)` text inputs, moved inside the same
     `ActiveForm`/submit button as the rest of General (was previously rendered
     *after* `ActiveForm::end()`).
   - Verified live: filled Account Name + IBAN, submitted, got "Settings saved.",
     reloaded and both fields round-tripped correctly (IBAN normalized to uppercase).
     Test row cleaned up from the dev DB afterward.
3. **Min withdrawal default** → **change to ⃁50.** Implemented via
   `common/migrations/db/m260721_213000_lower_shop_min_withdrawal_default.php` —
   alters the column default 5000→50 and backfills only shops still sitting on the
   untouched old default (preserves any admin-customised per-shop override). Also
   updated the in-memory model default in `common/models/base/Shop.php`. Migration
   applied; Commercials tab verified showing `⃁ 50.00`.
4. **Notifications** → **add the 3h reminder + flip in-app on** (did not request SMS/WA
   template authorship or removing the 3 extra triggers). Implemented via two
   migrations:
   - `m260721_214500_notif_trigger_inapp_templates_and_3h_reminder.php` — authored
     bilingual in-app template copy for all 7 existing triggers (all had NULL
     templates on every channel — confirmed by direct query — which is why preview
     text was blank everywhere) and inserted a new `booking_reminder_3h` trigger
     (offset mode, -180 min, customer audience, optional).
   - `m260721_215500_approve_notif_triggers_for_inapp.php` — a required follow-up:
     authoring the in-app template alone didn't flip the column to On, because the
     Settings view actually reads `NotificationDispatchService::channelUsable()`, not
     `NotificationTrigger::channelUsable()` — the former requires
     `approval_status = 1` for **every** channel including in-app (inconsistent with
     the latter's own docblock claiming in-app never needs approval; not reconciled
     here, just discovered and worked around). Approved all 8 triggers; since
     sms/wa template columns remain NULL, SMS/WhatsApp stay correctly locked — only
     in-app flipped on, matching what was actually asked for.
   - Verified live: all 4 customer triggers now show "On · free" with real preview
     text quoted, and "Reminder — 3 hours before · 180 min before" appears as a new row.

## 7. Not done — genuinely deferred, no decision was asked for these

- **Payments two-toggle (APP/WALK-INS) redesign** (§1) — scoped, ready, not yet built.
- **Settlement hold (days) showing "—"** — likely a `commercial_config.settlement_hold_days`
  NULL data gap (sibling config values resolve fine), not independently confirmed
  against the DB.
- **Subscription plan/Status/Next billing showing "—"** — this test shop genuinely has
  no active subscription; re-test on a subscribed shop before treating as a bug.
- **Scheduling slot-size lock** — intentional plan-tier gating, present in the demo's
  own model too; not a bug.

## 8. Found in passing, not fixed (out of scope)

`frontend/views/payment-settings/index.php` (`PaymentSettingsController`, route
`/payment-settings`) is an **orphaned duplicate** of the same pill-switch payments UI —
not linked from `ShopNav`, predates the unified 5-tab Settings page, has the **identical**
`checkbox()`/`template` bug described in §1, and additionally lacks the walk-in toggle
columns entirely. Still reachable by direct URL. Candidate for deletion once confirmed
unused, rather than patching a second copy of logic the unified page already owns.