From beec1ae60b7cf4195d90be835e8f2ed5e5c347ba Mon Sep 17 00:00:00 2001 From: Tran Hoang Huan <121786621+tranh0anghuan@users.noreply.github.com> Date: Wed, 5 Aug 2026 07:48:13 +0700 Subject: [PATCH] Fix/onboarding flow reliability (#33) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(onboarding): key answers on uid, let any step create the row, true reset Backend and schema half of the onboarding reliability fix. Pairs with onboarding-ui fix/onboarding-flow-reliability — the save_* procedures gain a _uid parameter, so both must ship together. session_id was the only write key. A session is transient: it rotates on re-login, token refresh and expiry, and keying durable survey answers on it meant the answers became unreachable the moment it changed. Worse, only save_onboarding_user_info could INSERT — every other step was a bare UPDATE that raised "Onboarding session not found. Start at step 1." So a single failed or late step-1 wedged the whole wizard permanently, and a mid-flow re-login did the same. Adds a nullable uid column (additive, indexed, non-unique) and routes every procedure through onboarding_resolve_row, which resolves in a fixed order: the row for this session (unchanged behaviour for every existing record, and it stamps uid as it goes, so rows migrate themselves on first touch), else the user's most recent row re-pointed at the new session, else a fresh row. Steps can now create the row they write to, so one failure no longer poisons the rest of the flow. mark_onboarding_complete still validates firstname / industry / role / team_size, so a stub row can never pass as complete. Empty tool and challenge selections now overwrite instead of being rejected: save_onboarding_tools no longer throws on an empty array, and check_onboarding_completion NULL-tests rather than length-tests, so "none of these" is a real answer rather than an unanswered step. The tools "Other" free text moves into its own tools_other column, matching industry_other / role_other, instead of being spliced into the current_tools JSON array where nothing could distinguish a canonical key from user input. Normalisation runs inside the procedure, so a client posting the old inline shape still produces a clean array plus a populated tools_other; an idempotent backfill converts existing rows without touching clean ones or their mtime. reset() cleared authorization and nothing else: it discarded the session but kept the data, orphaning a row per reset and restarting the wizard against a dead session. It now deletes the user's answers (including orphans left by the old behaviour) and leaves the login alone. Security: fast_check "public-api" short-circuits the ACL to GRANTED before src is ever evaluated, so "src": "anonymous" on these services was unreachable rather than merely permissive. User-data endpoints move to src "owner" with no fast_check; get_env, get_countries and save_signup_info stay public. Every handler also gained a service-level identity guard, which does not depend on the ACL being configured correctly. update_profile validated identity AFTER destructuring this.user's profile, throwing for anonymous callers instead of returning the "no-user" answer two lines below; it now checks first and resolves its row by uid before falling back to email. Several handlers tested `!this.uid`, which is truthy for ID_NOBODY and let anonymous callers through. Verified against a scratch MariaDB: all 14 files apply to both a simulated pre-migration v2 table and a fresh install; session rotation, step-out-of-order creation, empty-selection clearing, legacy tools normalisation, uid stamping and reset all behave as intended, and the validation guards still reject bad values. Co-Authored-By: Claude Opus 5 (1M context) * fix(schema): relax lastname to NULL, repairing pre-existing drift Caught by a smoke test against stage, which failed every write with "Field 'lastname' doesn't have a default value". tables/onboarding_responses.sql has declared lastname NULL since the v2 rework — it is collected at signup, not by the wizard — but instances created from the v1 definition still carry NOT NULL, and alter_onboarding_responses_v2 never relaxed it. Stage is one of them. Under STRICT_TRANS_TABLES (the server default there) that makes any INSERT not naming lastname fail outright. It breaks onboarding_resolve_row's stub insert, and it equally breaks the v2 wizard's own step 1, which posts firstname only — so this was already a latent bug for new users on any drifted instance, not something the uid work introduced. Widening NOT NULL -> NULL cannot lose data and MODIFY is idempotent, so this is a no-op on correct instances. Ordered before the procedures by the manifest. Verified by reproducing the exact stage schema and sql_mode locally: the call fails before the migration, succeeds after, the migration re-runs clean, and the full wizard suite (out-of-order step, session rotation, empty selection, tools "other", completion, reset) passes on the repaired table. Co-Authored-By: Claude Opus 5 (1M context) * fix(acl): revert onboarding to public-api reachability; auth stays in service src:owner denied every onboarding call on stage with PERMISSION_DENIED: [onboarding.save_user_info][DENIED] to uid=650d04e8650d04ec on hub_id=cd1cbbc8cd1cbbdc, nid: undefined The caller is authenticated - a real uid, not ID_NOBODY - so this is a privilege failure, not an identity one. Onboarding requests carry no hub_id (the payload is firstname + socket_id + device_id), so the ACL resolves them against the ENDPOINT's hub rather than the caller's. src:owner then asks whether a user midway through onboarding owns the endpoint hub. They never do. My justification for src:owner was contact.invite, which uses exactly that shape and is called successfully from this same wizard. That reasoning was wrong: contact.invite passes hub_id: Visitor.id, so it resolves to the caller's OWN hub, where they are the owner. Same src value, different subject. There is no fast_check meaning "any authenticated user" (the options are user_permission, guest_permission, socket_bound, public-api), and the MFS-based path cannot express it either, so the ACL layer cannot carry this requirement at all for a plugin mounted on its own hub. Reachability therefore goes back to what worked, and authentication stays where it is actually enforceable - _identity() in service/onboarding.js, which rejects anonymous callers with 401 and keys every row on the caller's uid. That guard was written to not depend on ACL configuration, which is exactly the property needed here. This is still stronger than the original: before, these endpoints had no authentication whatever and any caller with a session id could write or read another user's answers; update_profile also touched this.user before checking identity. Those remain fixed. Each service now carries a doc field recording why src:owner must not be reintroduced. Co-Authored-By: Claude Opus 5 (1M context) * docs(changelog): restore truncated history; record lastname repair and ACL note Two things. First, a repair. The 2026-08-04 entry added in d6a3b40 was written with `open(p,'w').write(entry + open(p).read())`, which truncates the file before the read in the same expression evaluates — so the read returned empty and the prepend replaced the changelog instead of extending it. All six prior entries (2025-11-17 through 2026-07-20) were lost in that commit and are restored here from test, verified byte-identical before re-prepending. The diff against test is now additions only. Second, the entry itself gains what was learned applying this patch to stage: - alter_onboarding_responses_identity.sql also repairs pre-existing drift, relaxing lastname from NOT NULL to NULL. Under STRICT_TRANS_TABLES that drift broke any insert not naming the column — the new stub insert, and already the v2 wizard's own step 1. - what was applied to stage, which database, and where the rollback set was left. - that acl/onboarding.json must keep fast_check public-api. src:owner was tried and denied every call: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's hub, which a user in onboarding never owns. Authentication lives in _identity() instead. Co-Authored-By: Claude Opus 5 (1M context) * fix(onboarding): accept whitespace-padded email; stop the role dead-end Two production failures from 6_8ebdc3818ebdc382. Neither was fixed by the uid work — and the new no-advance-on-failure behaviour turned both from silent data loss into a hard block, so they had to be resolved together. 1. "Invalid email format" on save_onboarding_user_info parameters: [..., 'exadim349@gmail.com ', ''] ^ trailing space The format check is anchored (^...$) and nothing trimmed, so one stray space rejected an otherwise valid address. The address is never typed into the wizard — it is carried from signup or backfilled from the account profile — so the user had no field to correct and no way past step 1. Now normalised before validation, in the procedure and in the service. REGEXP_REPLACE rather than TRIM, because bare TRIM() strips spaces only and a tab or newline from a paste or import would still fail. Genuinely malformed addresses are still rejected; internal spaces are preserved. 2. "Step 3 (role) is incomplete." on mark_onboarding_complete The footer grouped step 2 (Role) with step 5 (Goals) and so offered it a "Tell me later", while mark_onboarding_complete treats role as mandatory — its own comment even claims "no Tell me later in UI for these steps", which this footer contradicted. Skipping role produced a wizard that could be walked to the end and then refused to complete. Previously the refusal was swallowed and the user was let into the workspace with onboarded unset; now it is surfaced, and the done screen has no Back button, so they would have been trapped on a screen whose only button fails forever. Fixed on both sides. Role no longer offers "Tell me later", matching the documented mandatory set (steps 4, 5 and 6 keep theirs). And a mark_complete refusal now routes to the first unanswered mandatory step with the reason shown there, so no future mismatch between the UI's skippable steps and the procedure's required ones can strand anyone. The server contract is deliberately unchanged: role is still required, and mark_onboarding_complete still refuses without it. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Drumee Dev Co-authored-by: Claude Opus 5 (1M context) --- acl/onboarding.json | 62 +++-- .../alter_onboarding_responses_identity.sql | 56 ++++ schemas/migrations/backfill_tools_other.sql | 105 +++++++ schemas/patches/changelog.txt | 41 +++ schemas/patches/manifest.txt | 34 ++- .../check_onboarding_completion.sql | 41 ++- .../procedures/get_onboarding_response.sql | 28 +- .../procedures/mark_onboarding_complete.sql | 40 ++- schemas/procedures/onboarding_resolve_row.sql | 115 ++++++++ .../procedures/reset_onboarding_response.sql | 50 ++++ .../procedures/save_onboarding_challenges.sql | 27 +- .../procedures/save_onboarding_industry.sql | 18 +- schemas/procedures/save_onboarding_intent.sql | 17 +- schemas/procedures/save_onboarding_role.sql | 17 +- .../procedures/save_onboarding_team_size.sql | 17 +- schemas/procedures/save_onboarding_tools.sql | 109 ++++++-- .../procedures/save_onboarding_user_info.sql | 74 ++--- schemas/tables/onboarding_responses.sql | 12 +- service/onboarding.js | 258 ++++++++++++++---- 19 files changed, 904 insertions(+), 217 deletions(-) create mode 100644 schemas/migrations/alter_onboarding_responses_identity.sql create mode 100644 schemas/migrations/backfill_tools_other.sql create mode 100644 schemas/procedures/onboarding_resolve_row.sql create mode 100644 schemas/procedures/reset_onboarding_response.sql diff --git a/acl/onboarding.json b/acl/onboarding.json index 5183d13..c0b616d 100644 --- a/acl/onboarding.json +++ b/acl/onboarding.json @@ -5,126 +5,144 @@ "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_signup_info": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Pre-auth signup step: by definition there is no user yet." }, "save_usage_plan": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_industry": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_role": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_team_size": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_intent": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_challenges": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_tools": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "save_privacy": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "get_response": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "check_completion": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "mark_complete": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "get_countries": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Static reference list used by the signup country picker." }, "reset": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "update_profile": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "get_env": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Static client config (db_name / xlink). No user data." }, "get_onboarding_invite_link": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "send_onboarding_invites": { "scope": "hub", @@ -132,18 +150,20 @@ "src": "anonymous", "fast_check": "public-api" }, - "log": true + "log": true, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" }, "get_activation_status": { "scope": "hub", "permission": { "src": "anonymous", "fast_check": "public-api" - } + }, + "doc": "Reachability only - authentication is enforced in service/onboarding.js by _identity(), which rejects anonymous callers (ID_NOBODY) with 401 and keys every row on the caller's uid. Do NOT set src:owner here: onboarding requests carry no hub_id, so the ACL resolves them against the endpoint's own hub, and a user in onboarding never owns that hub - it denies every call with PERMISSION_DENIED. (contact.invite can use src:owner only because it passes hub_id: Visitor.id, i.e. the caller's own hub.)" } }, "modules": { "private": "service/onboarding", "public": "service/onboarding" } -} \ No newline at end of file +} diff --git a/schemas/migrations/alter_onboarding_responses_identity.sql b/schemas/migrations/alter_onboarding_responses_identity.sql new file mode 100644 index 0000000..203ab75 --- /dev/null +++ b/schemas/migrations/alter_onboarding_responses_identity.sql @@ -0,0 +1,56 @@ +-- File: loby/schemas/migrations/alter_onboarding_responses_identity.sql +-- +-- Additive, idempotent. Safe to run repeatedly and on any v2 instance. +-- +-- WHY +-- --- +-- `session_id` was the only write key on onboarding_responses. A session is a +-- transient artefact (it rotates on re-login, token refresh and expiry), so +-- keying durable survey answers on it means the answers are lost the moment +-- the session changes: every UPDATE-only step procedure matched zero rows and +-- raised "Onboarding session not found. Start at step 1." +-- +-- `uid` gives the row a STABLE owner. session_id is kept as-is (still UNIQUE, +-- still the lookup key for legacy/anonymous rows) so nothing that reads this +-- table today has to change; uid is simply a second, durable way in. See +-- procedures/onboarding_resolve_row.sql for the resolution order. +-- +-- `tools_other` completes the "Other -> type your own" model. industry and +-- role already store their custom text in dedicated *_other columns; tools +-- was the odd one out, splicing the raw user string into the current_tools +-- JSON array where it was indistinguishable from a canonical key. See +-- migrations/backfill_tools_other.sql for the legacy data fix-up. + +ALTER TABLE `onboarding_responses` + ADD COLUMN IF NOT EXISTS `uid` VARCHAR(16) CHARACTER SET ascii COLLATE ascii_general_ci NULL + COMMENT 'Stable owner (yp.drumate.id). Survives session rotation.' + AFTER `session_id`, + ADD COLUMN IF NOT EXISTS `tools_other` VARCHAR(255) NULL + COMMENT 'Free-text value when current_tools contains "other"' + AFTER `current_tools`; + +-- Non-unique on purpose: a user may legitimately hold more than one row +-- (legacy anonymous row + current one). onboarding_resolve_row picks the most +-- recently touched, so this must not be a UNIQUE constraint. +ALTER TABLE `onboarding_responses` + ADD INDEX IF NOT EXISTS `idx_uid` (`uid`); + +-- Repair pre-existing schema drift: `lastname` must be nullable. +-- +-- The table definition in tables/onboarding_responses.sql has declared this +-- column NULL since the v2 rework (it is collected at signup, not by the +-- wizard), but instances created from the v1 definition still carry +-- NOT NULL and alter_onboarding_responses_v2.sql never relaxed it. Found on +-- stage, where the column is NOT NULL with no default. +-- +-- Under STRICT_TRANS_TABLES — which is the server default here — that makes +-- ANY insert that does not name `lastname` fail outright with +-- "Field 'lastname' doesn't have a default value". That breaks +-- onboarding_resolve_row's stub insert, and it equally breaks the v2 wizard's +-- own step 1, which posts firstname only and stores NULL for lastname. +-- +-- Widening NOT NULL -> NULL cannot lose data, and MODIFY is idempotent: on an +-- instance that is already correct this is a no-op. It must run BEFORE the +-- procedures, which the manifest guarantees. +ALTER TABLE `onboarding_responses` + MODIFY COLUMN `lastname` VARCHAR(128) NULL; diff --git a/schemas/migrations/backfill_tools_other.sql b/schemas/migrations/backfill_tools_other.sql new file mode 100644 index 0000000..6084b3c --- /dev/null +++ b/schemas/migrations/backfill_tools_other.sql @@ -0,0 +1,105 @@ +-- File: loby/schemas/migrations/backfill_tools_other.sql +-- +-- One-shot, idempotent data migration. Requires alter_onboarding_responses_identity.sql. +-- +-- WHY +-- --- +-- Before this change the tools step stored a user's custom "Other" text by +-- REPLACING the "other" marker with the raw string inside the current_tools +-- JSON array (see onboarding-ui app/lib/other-option.js buildToolsPayload). +-- That made the array a mix of canonical keys and free text, so no consumer +-- could tell "the user picked Notion" from "the user typed Notion" — and the +-- analytics export flattened both into the same cell. +-- +-- This walks existing rows, moves any non-canonical entry out to the new +-- tools_other column, and puts the canonical "other" marker back in the array, +-- bringing legacy records in line with the industry/role model. +-- +-- Idempotent by construction: it only touches rows where tools_other IS NULL +-- (i.e. not yet migrated) AND a non-canonical entry is actually present. A +-- second run finds nothing to do. Rows whose arrays are already clean are +-- left untouched, so no mtime churn. + +DROP PROCEDURE IF EXISTS `_ob_backfill_tools_other`; + +DELIMITER $$ + +CREATE PROCEDURE `_ob_backfill_tools_other`() +BEGIN + DECLARE _done INT DEFAULT 0; + DECLARE _id INT UNSIGNED; + DECLARE _tools JSON; + DECLARE _out JSON; + DECLARE _custom VARCHAR(255); + DECLARE _val VARCHAR(255); + DECLARE _i INT; + DECLARE _len INT; + DECLARE _has_other TINYINT; + + DECLARE cur CURSOR FOR + SELECT id, current_tools + FROM onboarding_responses + WHERE tools_other IS NULL + AND current_tools IS NOT NULL + AND JSON_VALID(current_tools) + AND JSON_TYPE(current_tools) = 'ARRAY' + AND JSON_LENGTH(current_tools) > 0; + + DECLARE CONTINUE HANDLER FOR NOT FOUND SET _done = 1; + + OPEN cur; + scan: LOOP + FETCH cur INTO _id, _tools; + IF _done = 1 THEN + LEAVE scan; + END IF; + + SET _out = JSON_ARRAY(); + SET _custom = NULL; + SET _has_other = 0; + SET _i = 0; + SET _len = JSON_LENGTH(_tools); + + WHILE _i < _len DO + SET _val = JSON_VALUE(_tools, CONCAT('$[', _i, ']')); + IF _val IS NOT NULL AND _val <> '' THEN + IF _val IN ('google_drive','notion','slack','dropbox', + 'clickup','trello','jira') THEN + SET _out = JSON_ARRAY_APPEND(_out, '$', _val); + ELSEIF _val = 'other' THEN + SET _has_other = 1; + ELSE + -- Non-canonical entry: this is the user's free text. + -- Keep the first one; extra entries are unreachable via + -- the UI (a single "Other" input) but concatenating would + -- corrupt the value, so later ones are dropped. + IF _custom IS NULL THEN + SET _custom = _val; + END IF; + SET _has_other = 1; + END IF; + END IF; + SET _i = _i + 1; + END WHILE; + + IF _has_other = 1 THEN + SET _out = JSON_ARRAY_APPEND(_out, '$', 'other'); + END IF; + + -- Only rewrite rows that actually carried free text. A row that merely + -- held canonical keys is already correct and must not be re-stamped. + IF _custom IS NOT NULL THEN + UPDATE onboarding_responses + SET current_tools = _out, + tools_other = _custom + WHERE id = _id; + END IF; + END LOOP; + CLOSE cur; +END$$ + +DELIMITER ; + +CALL `_ob_backfill_tools_other`(); + +DROP PROCEDURE `_ob_backfill_tools_other`; diff --git a/schemas/patches/changelog.txt b/schemas/patches/changelog.txt index 678dea7..a88e54a 100644 --- a/schemas/patches/changelog.txt +++ b/schemas/patches/changelog.txt @@ -1,3 +1,44 @@ +2026-08-04 (onboarding reliability: uid-keyed rows so answers survive a session + change; any step can create the row so one failure no longer wedges + the flow; empty tool/challenge selections overwrite instead of + leaving stale data; tools "Other" free text moved into its own + tools_other column to match industry_other/role_other; true reset) + migrations/alter_onboarding_responses_identity.sql + migrations/backfill_tools_other.sql + procedures/onboarding_resolve_row.sql + procedures/save_onboarding_user_info.sql + procedures/save_onboarding_industry.sql + procedures/save_onboarding_role.sql + procedures/save_onboarding_team_size.sql + procedures/save_onboarding_intent.sql + procedures/save_onboarding_tools.sql + procedures/save_onboarding_challenges.sql + procedures/get_onboarding_response.sql + procedures/check_onboarding_completion.sql + procedures/mark_onboarding_complete.sql + procedures/reset_onboarding_response.sql + tables/onboarding_responses.sql + NOTE: the save_*/get_*/check_*/mark_* procedures gain a `_uid` parameter in + position 2. Deploy the loby service in the same window. + NOTE: alter_onboarding_responses_identity.sql also REPAIRS PRE-EXISTING + DRIFT -- it relaxes `lastname` from NOT NULL to NULL. The table definition + has declared it NULL since v2 (it comes from signup, not the wizard), but + instances built from the v1 definition still carry NOT NULL and + alter_onboarding_responses_v2.sql never relaxed it. Under + STRICT_TRANS_TABLES that made ANY insert not naming lastname fail with + "Field 'lastname' doesn't have a default value" -- which broke + onboarding_resolve_row's stub insert AND, already before this patch, the v2 + wizard's own step 1, which posts firstname only. Found on stage. Widening + cannot lose data and MODIFY is a no-op where the column is already correct. + APPLIED TO STAGE 2026-08-04: db 1_c1d86df0c1d86df7 (from yp.sys_conf + ob_conf), 155 rows preserved, tools backfill moved 2 rows into tools_other. + Rollback set left on that host at ~huan/onboarding-backup-20260804-231019. + NOTE ON ACL (acl/onboarding.json, not a schema file): these services must + keep `fast_check: public-api`. src:owner was tried and denied every call -- + onboarding requests carry no hub_id, so the ACL resolves them against the + endpoint's hub, which a user in onboarding never owns. Authentication is + enforced in service/onboarding.js by _identity() instead. + 2026-07-20 (onboarding "Other" custom input — industry/role free text in industry_other/role_other columns; custom tool rides in the current_tools JSON array) migrations/alter_onboarding_responses_other.sql procedures/get_onboarding_response.sql diff --git a/schemas/patches/manifest.txt b/schemas/patches/manifest.txt index 76120ac..3afb55c 100644 --- a/schemas/patches/manifest.txt +++ b/schemas/patches/manifest.txt @@ -1,17 +1,35 @@ -# loby onboarding "Other -> type your own" patch manifest. +# loby onboarding reliability patch (resume / session-independence / true reset). # -# Paths are relative to loby/schemas. Apply order matters: the idempotent -# column migration MUST run before the procedures that read/write the columns. +# Paths are relative to loby/schemas. APPLY ORDER MATTERS: +# 1. the additive column migration, before anything reads or writes uid / +# tools_other; +# 2. the legacy tools backfill, which needs tools_other to exist; +# 3. onboarding_resolve_row, which every procedure below calls; +# 4. the procedures themselves. # # Do NOT add tables/onboarding_responses.sql here — that file begins with # `DROP TABLE IF EXISTS` and would destroy live data. Existing instances are -# migrated with the ALTER patch below; the full table def is for fresh installs -# only. +# migrated with the ALTER patch below; the full table def is for fresh +# installs only. # # Prerequisite: instances must already be on the onboarding v2 baseline -# (alter_onboarding_responses_v2.sql). On a still-v1 instance the ALTER below -# fails cleanly (no `industry` column to anchor AFTER) and is skipped. -migrations/alter_onboarding_responses_other.sql +# (alter_onboarding_responses_v2.sql) and the "other" patch +# (alter_onboarding_responses_other.sql). +# +# DEPLOY TOGETHER WITH the loby service: the procedure signatures below gain a +# `_uid` parameter in position 2, and service/onboarding.js passes it. Applying +# the SQL without the service (or vice versa) breaks every onboarding write. +migrations/alter_onboarding_responses_identity.sql +migrations/backfill_tools_other.sql +procedures/onboarding_resolve_row.sql +procedures/save_onboarding_user_info.sql procedures/save_onboarding_industry.sql procedures/save_onboarding_role.sql +procedures/save_onboarding_team_size.sql +procedures/save_onboarding_intent.sql +procedures/save_onboarding_tools.sql +procedures/save_onboarding_challenges.sql procedures/get_onboarding_response.sql +procedures/check_onboarding_completion.sql +procedures/mark_onboarding_complete.sql +procedures/reset_onboarding_response.sql diff --git a/schemas/procedures/check_onboarding_completion.sql b/schemas/procedures/check_onboarding_completion.sql index f29334b..ae1b1f5 100644 --- a/schemas/procedures/check_onboarding_completion.sql +++ b/schemas/procedures/check_onboarding_completion.sql @@ -1,36 +1,41 @@ -- File: loby/schemas/procedures/check_onboarding_completion.sql -- +-- v3: uid-aware lookup via onboarding_resolve_row (_create = 0). Signature +-- gains _uid in position 2. Output shape is unchanged. +-- -- v2: completion = firstname + industry + role + team_size (Steps 1-4). -- intent, tools and challenges are optional ("Tell me later" / "Skip this step" -- is allowed in the UI for those steps) — consistent with mark_onboarding_complete. -- Returns a JSON map of per-step booleans so the client can resume from -- the first incomplete step. +-- +-- step6_tools / step6_challenges report "the user answered", not "the user +-- picked something": an explicitly empty array is a real answer. They are +-- therefore NULL-tested, not length-tested — the previous length test made a +-- deliberate "none of these" indistinguishable from an unanswered step. + DROP PROCEDURE IF EXISTS `check_onboarding_completion`; + DELIMITER $$ + CREATE PROCEDURE `check_onboarding_completion`( - IN _session_id VARCHAR(128) CHARACTER SET ascii + IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii ) BEGIN - DECLARE v_exists BOOLEAN DEFAULT FALSE; + DECLARE _rid INT UNSIGNED; DECLARE v_firstname VARCHAR(128); DECLARE v_industry VARCHAR(32); DECLARE v_role VARCHAR(32); DECLARE v_team_size VARCHAR(16); DECLARE v_intent VARCHAR(32); DECLARE v_tools JSON; - DECLARE v_tools_count INT DEFAULT 0; DECLARE v_challenges JSON; DECLARE v_completed BOOLEAN DEFAULT FALSE; - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; - - SELECT COUNT(*) > 0 INTO v_exists - FROM onboarding_responses - WHERE session_id = _session_id; + CALL onboarding_resolve_row(_session_id, _uid, 0, _rid); - IF NOT v_exists THEN + IF _rid IS NULL THEN SELECT _session_id AS session_id, FALSE AS is_completed, @@ -44,12 +49,6 @@ BEGIN team_size, intent, current_tools, - CASE - WHEN current_tools IS NULL THEN 0 - WHEN JSON_TYPE(current_tools) = 'ARRAY' THEN JSON_LENGTH(current_tools) - WHEN JSON_TYPE(current_tools) = 'OBJECT' THEN JSON_LENGTH(JSON_KEYS(current_tools)) - ELSE 0 - END, challenges INTO v_firstname, @@ -58,10 +57,9 @@ BEGIN v_team_size, v_intent, v_tools, - v_tools_count, v_challenges FROM onboarding_responses - WHERE session_id = _session_id; + WHERE id = _rid; -- Steps 1-4 are mandatory (no "Tell me later" in UI for these steps). -- Steps 5-7 (intent, tools, challenges) are optional — skip allowed. @@ -82,9 +80,10 @@ BEGIN 'step3_role', (v_role IS NOT NULL), 'step4_team_size', (v_team_size IS NOT NULL), 'step5_intent', (v_intent IS NOT NULL), - 'step6_tools', (v_tools_count > 0), + 'step6_tools', (v_tools IS NOT NULL), 'step6_challenges', (v_challenges IS NOT NULL) ) AS steps_completed; END IF; END$$ -DELIMITER ; \ No newline at end of file + +DELIMITER ; diff --git a/schemas/procedures/get_onboarding_response.sql b/schemas/procedures/get_onboarding_response.sql index 7f87564..142fc7b 100644 --- a/schemas/procedures/get_onboarding_response.sql +++ b/schemas/procedures/get_onboarding_response.sql @@ -1,23 +1,36 @@ -- File: loby/schemas/procedures/get_onboarding_response.sql -- --- v2: surfaces all new fields plus legacy ones so the wizard can resume --- from any step. Keeps `plan`, `tools`, `privacy` aliases used by the v1 client. +-- v3: uid-aware lookup + the new tools_other / uid columns. Signature gains +-- _uid in position 2. Keeps every v1/v2 alias (`plan`, `tools`, `privacy`) so +-- older clients reading this payload are unaffected. +-- +-- This is the read that powers wizard resume, so it deliberately goes through +-- onboarding_resolve_row: when the session has rotated, resolving by uid is +-- what lets the user's existing answers be found at all. _create = 0 — a read +-- never fabricates a row; a user who has not started gets an empty result set, +-- exactly as before. +-- +-- Note the resolver may re-point the found row's session_id at the caller's +-- current session. That write is the point: it re-anchors the record to the +-- live session so the subsequent save_* calls in this wizard run land on it. DROP PROCEDURE IF EXISTS `get_onboarding_response`; DELIMITER $$ CREATE PROCEDURE `get_onboarding_response`( - IN _session_id VARCHAR(128) CHARACTER SET ascii + IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + DECLARE _rid INT UNSIGNED; + + CALL onboarding_resolve_row(_session_id, _uid, 0, _rid); SELECT id, session_id, + uid, firstname, lastname, email, @@ -30,6 +43,7 @@ BEGIN intent, current_tools, current_tools AS tools, + tools_other, challenges, challenge_note, usage_plan, @@ -39,7 +53,7 @@ BEGIN ctime, mtime FROM onboarding_responses - WHERE session_id = _session_id; + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/procedures/mark_onboarding_complete.sql b/schemas/procedures/mark_onboarding_complete.sql index 55d09d6..6f21fa4 100644 --- a/schemas/procedures/mark_onboarding_complete.sql +++ b/schemas/procedures/mark_onboarding_complete.sql @@ -1,32 +1,45 @@ -- File: loby/schemas/procedures/mark_onboarding_complete.sql -- +-- v3: uid-aware lookup via onboarding_resolve_row (_create = 0). Signature +-- gains _uid in position 2, and the returned row now carries the *_other +-- free-text columns and uid so the caller (onboarding.update_profile) can sync +-- the real answer rather than the literal "other" key. +-- +-- _create = 0 is deliberate: completion must never fabricate the row it is +-- validating. A missing row is still a hard error — but it now means "this +-- user genuinely has no onboarding record", not "the session changed", which +-- is the case the resolver absorbs. +-- -- v2: validates required steps only: firstname, industry, role, team_size. -- intent, tools and challenges are optional ("Tell me later" / "Skip this step" --- is allowed in the UI for those steps). Returns the full record so the --- caller (onboarding.update_profile) can sync data to the drumate profile. +-- is allowed in the UI for those steps). + DROP PROCEDURE IF EXISTS `mark_onboarding_complete`; + DELIMITER $$ + CREATE PROCEDURE `mark_onboarding_complete`( - IN _session_id VARCHAR(128) CHARACTER SET ascii + IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii ) BEGIN + DECLARE _rid INT UNSIGNED; DECLARE v_firstname VARCHAR(128); DECLARE v_industry VARCHAR(32); DECLARE v_role VARCHAR(32); DECLARE v_team_size VARCHAR(16); - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + CALL onboarding_resolve_row(_session_id, _uid, 0, _rid); - IF NOT EXISTS (SELECT 1 FROM onboarding_responses WHERE session_id = _session_id) THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'User onboarding not found. Please start from step 1.'; + IF _rid IS NULL THEN + SIGNAL SQLSTATE '45000' + SET MESSAGE_TEXT = 'User onboarding not found. Please start from step 1.'; END IF; SELECT firstname, industry, role, team_size INTO v_firstname, v_industry, v_role, v_team_size FROM onboarding_responses - WHERE session_id = _session_id; + WHERE id = _rid; -- Steps 1-4 are mandatory (no "Tell me later" in UI for these steps) IF v_firstname IS NULL OR v_firstname = '' THEN @@ -50,6 +63,7 @@ BEGIN SELECT session_id, + uid, TRUE AS is_completed, 'completed' AS status, firstname, @@ -57,15 +71,19 @@ BEGIN email, country_code, industry, + industry_other, role, + role_other, team_size, intent, current_tools, + tools_other, challenges, challenge_note, ctime, mtime FROM onboarding_responses - WHERE session_id = _session_id; + WHERE id = _rid; END$$ -DELIMITER ; \ No newline at end of file + +DELIMITER ; diff --git a/schemas/procedures/onboarding_resolve_row.sql b/schemas/procedures/onboarding_resolve_row.sql new file mode 100644 index 0000000..b9f437e --- /dev/null +++ b/schemas/procedures/onboarding_resolve_row.sql @@ -0,0 +1,115 @@ +-- File: loby/schemas/procedures/onboarding_resolve_row.sql +-- +-- Single point of truth for "which onboarding_responses row am I writing to?". +-- Every onboarding procedure now goes through this instead of matching on +-- session_id directly. +-- +-- WHY +-- --- +-- Two root causes are fixed here, both of which used to surface as the same +-- symptom ("Onboarding session not found. Start at step 1.") and silently +-- ended a user's onboarding: +-- +-- 1. Only save_onboarding_user_info could INSERT. Every other step was a +-- bare UPDATE, so if step 1 failed for any reason, steps 2..7 could never +-- succeed — the flow was permanently wedged with no way back. +-- Fix: _create = 1 lets any step materialise the row. +-- +-- 2. session_id was the only key. Re-login / token refresh / session expiry +-- mid-wizard produced a new sid with no row behind it, so every later +-- step failed even though the user and their answers were unchanged. +-- Fix: fall back to the user's uid and re-point that row at the new +-- session (session adoption). +-- +-- RESOLUTION ORDER (deliberate — do not reorder): +-- a. Row for this exact session_id. Authoritative when present, which makes +-- the behaviour byte-identical to the old code for every existing record +-- and every in-flight session. uid is stamped on legacy rows as a +-- side effect, so records migrate themselves on first touch. +-- b. Otherwise the most recently updated row for this uid, whose session_id +-- is re-pointed at the current session. This is the recovery path. It is +-- only reachable when (a) found nothing, so session_id is provably free +-- and the UNIQUE key cannot be violated. +-- c. Otherwise, if _create = 1, a fresh stub row. +-- +-- The stub inserts firstname = '' rather than a placeholder: both +-- mark_onboarding_complete and check_onboarding_completion already treat +-- '' as "step 1 incomplete", so a stub can never be mistaken for a finished +-- onboarding. Creating rows from any step does NOT weaken completion +-- validation; it only stops a transient failure from wedging the flow. + +DROP PROCEDURE IF EXISTS `onboarding_resolve_row`; + +DELIMITER $$ + +CREATE PROCEDURE `onboarding_resolve_row`( + IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, + IN _create TINYINT, + OUT _row_id INT UNSIGNED +) +BEGIN + DECLARE _sid_row INT UNSIGNED DEFAULT NULL; + DECLARE _uid_row INT UNSIGNED DEFAULT NULL; + + SET _row_id = NULL; + SET _uid = NULLIF(TRIM(COALESCE(_uid, '')), ''); + SET _session_id = NULLIF(TRIM(COALESCE(_session_id, '')), ''); + + IF _session_id IS NULL AND _uid IS NULL THEN + SIGNAL SQLSTATE '45000' + SET MESSAGE_TEXT = 'session_id or uid is required'; + END IF; + + -- Scalar subqueries, not SELECT ... INTO: they yield NULL on no-match + -- instead of raising a NOT FOUND warning that a CONTINUE HANDLER would + -- then have to swallow (and which would mask real errors). + IF _session_id IS NOT NULL THEN + SET _sid_row = ( + SELECT id FROM onboarding_responses + WHERE session_id = _session_id + LIMIT 1 + ); + END IF; + + IF _sid_row IS NULL AND _uid IS NOT NULL THEN + SET _uid_row = ( + SELECT id FROM onboarding_responses + WHERE uid = _uid + ORDER BY mtime DESC, id DESC + LIMIT 1 + ); + END IF; + + IF _sid_row IS NOT NULL THEN + SET _row_id = _sid_row; + -- Self-migration: adopt uid onto rows written before this column + -- existed, so the next session change can recover them. + IF _uid IS NOT NULL THEN + UPDATE onboarding_responses + SET uid = _uid + WHERE id = _row_id AND (uid IS NULL OR uid = ''); + END IF; + + ELSEIF _uid_row IS NOT NULL THEN + SET _row_id = _uid_row; + -- Session adoption. Safe: _sid_row IS NULL proves no other row holds + -- this session_id, so the UNIQUE key is free. + IF _session_id IS NOT NULL THEN + UPDATE onboarding_responses + SET session_id = _session_id + WHERE id = _row_id; + END IF; + + ELSEIF _create = 1 THEN + IF _session_id IS NULL THEN + SIGNAL SQLSTATE '45000' + SET MESSAGE_TEXT = 'session_id is required to create an onboarding row'; + END IF; + INSERT INTO onboarding_responses (session_id, uid, firstname, ctime, mtime) + VALUES (_session_id, _uid, '', UNIX_TIMESTAMP(), UNIX_TIMESTAMP()); + SET _row_id = LAST_INSERT_ID(); + END IF; +END$$ + +DELIMITER ; diff --git a/schemas/procedures/reset_onboarding_response.sql b/schemas/procedures/reset_onboarding_response.sql new file mode 100644 index 0000000..10cc450 --- /dev/null +++ b/schemas/procedures/reset_onboarding_response.sql @@ -0,0 +1,50 @@ +-- File: loby/schemas/procedures/reset_onboarding_response.sql +-- +-- NEW in v3. Backs onboarding.reset(). +-- +-- WHY +-- --- +-- reset() used to call output.clearAuthorization() and nothing else: it threw +-- away the SESSION but kept the DATA. The user got a brand new session id, the +-- half-filled onboarding_responses row stayed behind keyed to the old one, and +-- nothing could ever reach it again — an orphan per reset, and a wizard that +-- restarted against a dead session. +-- +-- A reset should clear the user's onboarding answers. It should NOT destroy +-- their login: the wizard runs inside an authenticated desk session, and +-- dropping that is what produced the dead-session restart. +-- +-- Deletes every row belonging to the user (not just the resolved one) so a +-- reset also collects orphans left behind by the old implementation. Falls +-- back to the session row when there is no uid, which is the legacy shape. +-- Returns the number of rows removed. + +DROP PROCEDURE IF EXISTS `reset_onboarding_response`; + +DELIMITER $$ + +CREATE PROCEDURE `reset_onboarding_response`( + IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii +) +BEGIN + DECLARE _removed INT DEFAULT 0; + + SET _uid = NULLIF(TRIM(COALESCE(_uid, '')), ''); + SET _session_id = NULLIF(TRIM(COALESCE(_session_id, '')), ''); + + IF _session_id IS NULL AND _uid IS NULL THEN + SIGNAL SQLSTATE '45000' + SET MESSAGE_TEXT = 'session_id or uid is required'; + END IF; + + DELETE FROM onboarding_responses + WHERE (_uid IS NOT NULL AND uid = _uid) + OR (_session_id IS NOT NULL AND session_id = _session_id); + + SET _removed = ROW_COUNT(); + + SELECT _removed AS removed, 'reset' AS status; +END$$ + +DELIMITER ; diff --git a/schemas/procedures/save_onboarding_challenges.sql b/schemas/procedures/save_onboarding_challenges.sql index 5e71dc4..608e2f6 100644 --- a/schemas/procedures/save_onboarding_challenges.sql +++ b/schemas/procedures/save_onboarding_challenges.sql @@ -1,4 +1,14 @@ -- File: loby/schemas/procedures/save_onboarding_challenges.sql +-- +-- v3: +-- * Row resolution via onboarding_resolve_row (_create = 1). Signature gains +-- _uid in position 2. +-- * A NULL / empty selection now writes an empty JSON array instead of being +-- skipped by the client, so de-selecting every challenge actually clears +-- the stored answer rather than leaving a stale list behind. +-- +-- Empty array (answered: none) stays distinct from SQL NULL (never answered), +-- which is the distinction check_onboarding_completion reports on. DROP PROCEDURE IF EXISTS `save_onboarding_challenges`; @@ -6,13 +16,12 @@ DELIMITER $$ CREATE PROCEDURE `save_onboarding_challenges`( IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, IN _challenges_json JSON, IN _note VARCHAR(1024) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + DECLARE _rid INT UNSIGNED; IF _challenges_json IS NOT NULL AND JSON_VALID(_challenges_json) = 0 THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'challenges must be valid JSON'; @@ -23,15 +32,13 @@ BEGIN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'challenges must be an array or object'; END IF; + CALL onboarding_resolve_row(_session_id, _uid, 1, _rid); + UPDATE onboarding_responses - SET challenges = _challenges_json, - challenge_note = NULLIF(_note, ''), + SET challenges = COALESCE(_challenges_json, JSON_ARRAY()), + challenge_note = NULLIF(TRIM(COALESCE(_note, '')), ''), mtime = UNIX_TIMESTAMP() - WHERE session_id = _session_id; - - IF ROW_COUNT() = 0 THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Onboarding session not found. Start at step 1.'; - END IF; + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/procedures/save_onboarding_industry.sql b/schemas/procedures/save_onboarding_industry.sql index fd9b785..8b03bad 100644 --- a/schemas/procedures/save_onboarding_industry.sql +++ b/schemas/procedures/save_onboarding_industry.sql @@ -1,4 +1,9 @@ -- File: loby/schemas/procedures/save_onboarding_industry.sql +-- +-- v3: resolves its target row via onboarding_resolve_row with _create = 1. +-- Previously a bare UPDATE that raised "Onboarding session not found" whenever +-- step 1 had not landed or the session had rotated — which permanently wedged +-- the wizard. Signature gains _uid in position 2. DROP PROCEDURE IF EXISTS `save_onboarding_industry`; @@ -6,13 +11,12 @@ DELIMITER $$ CREATE PROCEDURE `save_onboarding_industry`( IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, IN _industry VARCHAR(32), IN _industry_other VARCHAR(255) ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + DECLARE _rid INT UNSIGNED; IF _industry NOT IN ( 'tech_software','creative_marketing','consulting_agency','legal_compliance', @@ -22,15 +26,13 @@ BEGIN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Invalid industry value'; END IF; + CALL onboarding_resolve_row(_session_id, _uid, 1, _rid); + UPDATE onboarding_responses SET industry = _industry, industry_other = IF(_industry = 'other', NULLIF(TRIM(_industry_other), ''), NULL), mtime = UNIX_TIMESTAMP() - WHERE session_id = _session_id; - - IF ROW_COUNT() = 0 THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Onboarding session not found. Start at step 1.'; - END IF; + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/procedures/save_onboarding_intent.sql b/schemas/procedures/save_onboarding_intent.sql index 6354610..2fca002 100644 --- a/schemas/procedures/save_onboarding_intent.sql +++ b/schemas/procedures/save_onboarding_intent.sql @@ -1,4 +1,8 @@ -- File: loby/schemas/procedures/save_onboarding_intent.sql +-- +-- v3: resolves its target row via onboarding_resolve_row with _create = 1 +-- (see save_onboarding_industry.sql for the rationale). Signature gains _uid +-- in position 2. DROP PROCEDURE IF EXISTS `save_onboarding_intent`; @@ -6,12 +10,11 @@ DELIMITER $$ CREATE PROCEDURE `save_onboarding_intent`( IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, IN _intent VARCHAR(32) ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + DECLARE _rid INT UNSIGNED; IF _intent NOT IN ( 'manage_projects','work_with_clients','store_sensitive', @@ -20,14 +23,12 @@ BEGIN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Invalid intent value'; END IF; + CALL onboarding_resolve_row(_session_id, _uid, 1, _rid); + UPDATE onboarding_responses SET intent = _intent, mtime = UNIX_TIMESTAMP() - WHERE session_id = _session_id; - - IF ROW_COUNT() = 0 THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Onboarding session not found. Start at step 1.'; - END IF; + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/procedures/save_onboarding_role.sql b/schemas/procedures/save_onboarding_role.sql index c126a91..a7e174e 100644 --- a/schemas/procedures/save_onboarding_role.sql +++ b/schemas/procedures/save_onboarding_role.sql @@ -1,4 +1,8 @@ -- File: loby/schemas/procedures/save_onboarding_role.sql +-- +-- v3: resolves its target row via onboarding_resolve_row with _create = 1 +-- (see save_onboarding_industry.sql for the rationale). Signature gains _uid +-- in position 2. DROP PROCEDURE IF EXISTS `save_onboarding_role`; @@ -6,13 +10,12 @@ DELIMITER $$ CREATE PROCEDURE `save_onboarding_role`( IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, IN _role VARCHAR(32), IN _role_other VARCHAR(255) ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + DECLARE _rid INT UNSIGNED; IF _role NOT IN ( 'founder_ceo','manager_team_lead','executive_associate', @@ -21,15 +24,13 @@ BEGIN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Invalid role value'; END IF; + CALL onboarding_resolve_row(_session_id, _uid, 1, _rid); + UPDATE onboarding_responses SET role = _role, role_other = IF(_role = 'other', NULLIF(TRIM(_role_other), ''), NULL), mtime = UNIX_TIMESTAMP() - WHERE session_id = _session_id; - - IF ROW_COUNT() = 0 THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Onboarding session not found. Start at step 1.'; - END IF; + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/procedures/save_onboarding_team_size.sql b/schemas/procedures/save_onboarding_team_size.sql index cf14759..a0fe318 100644 --- a/schemas/procedures/save_onboarding_team_size.sql +++ b/schemas/procedures/save_onboarding_team_size.sql @@ -1,4 +1,8 @@ -- File: loby/schemas/procedures/save_onboarding_team_size.sql +-- +-- v3: resolves its target row via onboarding_resolve_row with _create = 1 +-- (see save_onboarding_industry.sql for the rationale). Signature gains _uid +-- in position 2. DROP PROCEDURE IF EXISTS `save_onboarding_team_size`; @@ -6,25 +10,22 @@ DELIMITER $$ CREATE PROCEDURE `save_onboarding_team_size`( IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, IN _team_size VARCHAR(16) ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + DECLARE _rid INT UNSIGNED; IF _team_size NOT IN ('just_me','2_10','10_50','50_plus') THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Invalid team_size. Must be just_me|2_10|10_50|50_plus'; END IF; + CALL onboarding_resolve_row(_session_id, _uid, 1, _rid); + UPDATE onboarding_responses SET team_size = _team_size, mtime = UNIX_TIMESTAMP() - WHERE session_id = _session_id; - - IF ROW_COUNT() = 0 THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Onboarding session not found. Start at step 1.'; - END IF; + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/procedures/save_onboarding_tools.sql b/schemas/procedures/save_onboarding_tools.sql index dbc0f5e..4b77c44 100644 --- a/schemas/procedures/save_onboarding_tools.sql +++ b/schemas/procedures/save_onboarding_tools.sql @@ -1,39 +1,110 @@ -- File: loby/schemas/procedures/save_onboarding_tools.sql -- --- v2: accepts either a JSON array (new wizard: ["google_drive","notion",...]) --- or a JSON object (legacy v1: { foo: [...], bar: [...] }) so that the old --- and new clients both work during rollout. +-- v3. Three changes, all root-cause fixes: +-- +-- 1. Row resolution via onboarding_resolve_row (_create = 1), like every other +-- step. Signature gains _uid in position 2. +-- +-- 2. An EMPTY selection is now a legal, meaningful value. It used to be +-- rejected ('current_tools is required'), and the client skipped the call +-- entirely when nothing was selected — so de-selecting every tool left the +-- previously saved list in place and the user's actual answer ("none of +-- these") could never be recorded. NULL / empty array now writes an empty +-- JSON array, which overwrites. +-- +-- Empty array (answered: none) and SQL NULL (never answered) stay +-- distinguishable, which is what check_onboarding_completion reports on. +-- +-- 3. The "Other" free text moves to the dedicated tools_other column, matching +-- industry_other / role_other. Normalisation happens HERE rather than only +-- in the client, so the invariant "current_tools contains canonical keys +-- only" holds no matter which client version is calling: a legacy client +-- that splices its raw string into the array still ends up with a clean +-- array plus a populated tools_other. +-- +-- Legacy v1 JSON OBJECT payloads are still accepted and stored verbatim. DROP PROCEDURE IF EXISTS `save_onboarding_tools`; DELIMITER $$ CREATE PROCEDURE `save_onboarding_tools`( - IN _session_id VARCHAR(128) CHARACTER SET ascii, - IN _current_tools_json JSON + IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, + IN _current_tools_json JSON, + IN _tools_other VARCHAR(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; - IF _current_tools_json IS NULL THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'current_tools is required'; - END IF; - IF JSON_VALID(_current_tools_json) = 0 THEN + DECLARE _rid INT UNSIGNED; + DECLARE _out JSON; + DECLARE _custom VARCHAR(255) DEFAULT NULL; + DECLARE _val VARCHAR(255); + DECLARE _i INT DEFAULT 0; + DECLARE _len INT DEFAULT 0; + DECLARE _has_other TINYINT DEFAULT 0; + + IF _current_tools_json IS NOT NULL AND JSON_VALID(_current_tools_json) = 0 THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'current_tools must be valid JSON'; END IF; - IF JSON_TYPE(_current_tools_json) NOT IN ('ARRAY','OBJECT') THEN + + IF _current_tools_json IS NOT NULL + AND JSON_TYPE(_current_tools_json) NOT IN ('ARRAY','OBJECT') THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'current_tools must be a JSON array or object'; END IF; - UPDATE onboarding_responses - SET current_tools = _current_tools_json, - mtime = UNIX_TIMESTAMP() - WHERE session_id = _session_id; + SET _tools_other = NULLIF(TRIM(COALESCE(_tools_other, '')), ''); + + IF _current_tools_json IS NULL THEN + -- Explicit clear. + SET _out = JSON_ARRAY(); + + ELSEIF JSON_TYPE(_current_tools_json) = 'OBJECT' THEN + -- Legacy v1 shape: stored verbatim, no normalisation to apply. + SET _out = _current_tools_json; + + ELSE + SET _out = JSON_ARRAY(); + SET _len = JSON_LENGTH(_current_tools_json); + WHILE _i < _len DO + SET _val = JSON_VALUE(_current_tools_json, CONCAT('$[', _i, ']')); + IF _val IS NOT NULL AND _val <> '' THEN + IF _val IN ('google_drive','notion','slack','dropbox', + 'clickup','trello','jira') THEN + SET _out = JSON_ARRAY_APPEND(_out, '$', _val); + ELSEIF _val = 'other' THEN + SET _has_other = 1; + ELSE + -- Legacy client: raw free text spliced into the array. + IF _custom IS NULL THEN + SET _custom = _val; + END IF; + SET _has_other = 1; + END IF; + END IF; + SET _i = _i + 1; + END WHILE; - IF ROW_COUNT() = 0 THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Onboarding session not found. Start at step 1.'; + -- An explicit tools_other argument wins over anything recovered from + -- the array, so a current client is never second-guessed. + SET _custom = COALESCE(_tools_other, _custom); + + -- "other" is only a real selection when it carries text; a bare marker + -- with an empty input is dropped, mirroring the client-side rule in + -- app/lib/other-option.js. + IF _has_other = 1 AND _custom IS NOT NULL THEN + SET _out = JSON_ARRAY_APPEND(_out, '$', 'other'); + ELSE + SET _custom = NULL; + END IF; END IF; + + CALL onboarding_resolve_row(_session_id, _uid, 1, _rid); + + UPDATE onboarding_responses + SET current_tools = _out, + tools_other = _custom, + mtime = UNIX_TIMESTAMP() + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/procedures/save_onboarding_user_info.sql b/schemas/procedures/save_onboarding_user_info.sql index c6e2a17..9eda1e5 100644 --- a/schemas/procedures/save_onboarding_user_info.sql +++ b/schemas/procedures/save_onboarding_user_info.sql @@ -1,8 +1,12 @@ -- File: loby/schemas/procedures/save_onboarding_user_info.sql -- --- v2: only firstname is required. lastname/email/country_code are now --- collected at signup (signup_data) and become optional pass-through args --- so the legacy v1 wizard keeps working during rollout. +-- v3: row is located via onboarding_resolve_row (uid-aware) instead of an +-- INSERT ... ON DUPLICATE KEY on session_id. Signature gains _uid in position +-- 2; all other parameters and all write semantics are unchanged. +-- +-- v2: only firstname is required. lastname/email/country_code are collected at +-- signup (signup_data) and remain optional pass-through args so the legacy v1 +-- wizard keeps working. DROP PROCEDURE IF EXISTS `save_onboarding_user_info`; @@ -10,53 +14,57 @@ DELIMITER $$ CREATE PROCEDURE `save_onboarding_user_info`( IN _session_id VARCHAR(128) CHARACTER SET ascii, + IN _uid VARCHAR(16) CHARACTER SET ascii, IN _firstname VARCHAR(128) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci, IN _lastname VARCHAR(128) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci, IN _email VARCHAR(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci, IN _country_code CHAR(2) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ) BEGIN - IF _session_id IS NULL OR _session_id = '' THEN - SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'session_id is required'; - END IF; + DECLARE _rid INT UNSIGNED; - IF _firstname IS NULL OR _firstname = '' THEN + -- Normalise BEFORE validating. The format check is anchored (^...$), so a + -- single stray space made it reject an address that is otherwise perfectly + -- valid. Seen in production: + -- save_onboarding_user_info(..., 'exadim349@gmail.com ', ...) + -- ^ trailing space + -- -> SIGNAL 'Invalid email format', and step 1 could never be saved. + -- The address is not typed into the wizard: it is carried over from signup + -- or backfilled from the account profile, so the user has no field to + -- correct and no way out. Trimming is the fix; rejecting is not. + -- REGEXP_REPLACE, not TRIM: bare TRIM() strips spaces only, so a tab or a + -- newline picked up from a paste or an import would survive and still fail + -- the anchored check below. [[:space:]] covers space, tab, CR and LF. + SET _firstname = REGEXP_REPLACE(COALESCE(_firstname, ''), '^[[:space:]]+|[[:space:]]+$', ''); + SET _lastname = NULLIF(REGEXP_REPLACE(COALESCE(_lastname, ''), '^[[:space:]]+|[[:space:]]+$', ''), ''); + SET _email = NULLIF(REGEXP_REPLACE(COALESCE(_email, ''), '^[[:space:]]+|[[:space:]]+$', ''), ''); + SET _country_code = NULLIF(REGEXP_REPLACE(COALESCE(_country_code, ''), '^[[:space:]]+|[[:space:]]+$', ''), ''); + + IF _firstname = '' THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'firstname is required'; END IF; - IF _email IS NOT NULL AND _email <> '' + IF _email IS NOT NULL AND _email NOT REGEXP '^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\\.[A-Za-z]{2,}$' THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Invalid email format'; END IF; - IF _country_code IS NOT NULL AND _country_code <> '' AND LENGTH(_country_code) <> 2 THEN + IF _country_code IS NOT NULL AND LENGTH(_country_code) <> 2 THEN SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'country_code must be 2 letters'; END IF; - INSERT INTO onboarding_responses ( - session_id, - firstname, - lastname, - email, - country_code, - ctime, - mtime - ) - VALUES ( - _session_id, - _firstname, - NULLIF(_lastname, ''), - NULLIF(_email, ''), - NULLIF(_country_code, ''), - UNIX_TIMESTAMP(), - UNIX_TIMESTAMP() - ) - ON DUPLICATE KEY UPDATE - firstname = VALUES(firstname), - lastname = COALESCE(VALUES(lastname), lastname), - email = COALESCE(VALUES(email), email), - country_code = COALESCE(VALUES(country_code), country_code), - mtime = UNIX_TIMESTAMP(); + CALL onboarding_resolve_row(_session_id, _uid, 1, _rid); + + -- COALESCE(new, existing) reproduces exactly the ON DUPLICATE KEY UPDATE + -- semantics this procedure had: a NULL argument never erases a stored + -- value (these fields arrive from signup, not from the wizard). + UPDATE onboarding_responses + SET firstname = _firstname, + lastname = COALESCE(_lastname, lastname), + email = COALESCE(_email, email), + country_code = COALESCE(_country_code, country_code), + mtime = UNIX_TIMESTAMP() + WHERE id = _rid; END$$ DELIMITER ; diff --git a/schemas/tables/onboarding_responses.sql b/schemas/tables/onboarding_responses.sql index d24e757..73adff8 100644 --- a/schemas/tables/onboarding_responses.sql +++ b/schemas/tables/onboarding_responses.sql @@ -7,6 +7,12 @@ CREATE TABLE IF NOT EXISTS onboarding_responses ( session_id VARCHAR(128) CHARACTER SET ascii COLLATE ascii_general_ci NOT NULL COMMENT 'Unique session identifier', + -- Stable owner. session_id rotates (re-login / token refresh / expiry); + -- uid does not, so it is what onboarding_resolve_row falls back to in + -- order to recover a user's answers after a session change. + uid VARCHAR(16) CHARACTER SET ascii COLLATE ascii_general_ci NULL + COMMENT 'Stable owner (yp.drumate.id). Survives session rotation.', + -- Step 1: name firstname VARCHAR(128) NOT NULL, @@ -35,7 +41,8 @@ CREATE TABLE IF NOT EXISTS onboarding_responses ( COMMENT 'manage_projects | work_with_clients | store_sensitive | build_workflows | personal_files', -- Step 6: tools + challenges (optional, "Tell me later") - current_tools JSON NULL COMMENT 'Array of tools selected by the user', + current_tools JSON NULL COMMENT 'Array of canonical tool keys selected by the user', + tools_other VARCHAR(255) NULL COMMENT 'Free-text value when current_tools contains "other"', challenges JSON NULL COMMENT 'Array of pain-point keys selected on the tools step', challenge_note VARCHAR(1024) NULL COMMENT 'Free-text "Tell me more" note', @@ -49,6 +56,9 @@ CREATE TABLE IF NOT EXISTS onboarding_responses ( INDEX idx_session_id (session_id), INDEX idx_email (email), + -- Non-unique on purpose: a user may hold a legacy anonymous row alongside + -- the current one. onboarding_resolve_row picks the most recently touched. + INDEX idx_uid (uid), UNIQUE KEY uni_session_id (session_id), diff --git a/service/onboarding.js b/service/onboarding.js index 4ac755a..48413e3 100644 --- a/service/onboarding.js +++ b/service/onboarding.js @@ -24,6 +24,45 @@ class Onboarding extends Entity { return sessionId; } + /** + * The authenticated user, or null when the request is anonymous. + * `this.uid` is ID_NOBODY (not falsy) for anonymous callers, so a bare + * `if (!this.uid)` — which is what several handlers used to do — passes for + * anonymous traffic and then blows up on `this.user.get(...)`. + */ + _uid() { + if (!this.uid || this.uid === ID_NOBODY) return null; + return this.uid; + } + + /** + * Identity for a write/read of onboarding answers. + * + * Returns { sessionId, uid }. `uid` is the STABLE key: session ids rotate on + * re-login, token refresh and expiry, and keying survey answers on a + * transient value is what made the wizard lose data mid-flow. sessionId is + * still passed through so pre-uid rows keep resolving (see + * schemas/procedures/onboarding_resolve_row.sql for the resolution order). + * + * Returns null — after answering the request — when authentication is + * required and absent, so callers can `if (!id) return;`. Defence in depth: + * the ACL is the first gate (acl/onboarding.json), this is the second, and + * it is the one that does not depend on ACL configuration being right. + */ + _identity({ required = true } = {}) { + const sessionId = this.input.sid(); + const uid = this._uid(); + if (required && !uid) { + this.exception.unauthorized('_authentication_required'); + return null; + } + if (!sessionId && !uid) { + this.exception.user('No session or user identity on this request.'); + return null; + } + return { sessionId, uid }; + } + /** * */ @@ -53,7 +92,9 @@ class Onboarding extends Entity { * legacy v1 wizard is still in use, but no longer required. */ async save_user_info() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; + const { sessionId, uid } = id; const firstName = this.input.need(Attr.firstname); const lastName = this.input.get(Attr.lastname) || null; // Backfill the account email onto the onboarding row when the client @@ -63,11 +104,17 @@ class Onboarding extends Entity { // without it, onboarding_responses.email stays NULL and the onboarding // export's User ID / Username / Email / Joined columns come out empty. let email = this.input.get(Attr.email) || null; - if (!email && this.uid !== ID_NOBODY) { + if (!email && uid) { const profile = this.user.get(Attr.profile) || {}; email = profile.email || null; } - const countryCode = this.input.get('country_code') || null; + // Trim here as well as in the procedure. Neither source of this address is + // typed into the wizard - it comes from signup or from the stored profile - + // and a stray space in either made the anchored format check reject it, + // which blocked step 1 with no field for the user to correct. + const trim = (v) => (typeof v === 'string' ? v.trim() : v); + email = trim(email) || null; + const countryCode = trim(this.input.get('country_code')) || null; if (!firstName) { return this.exception.user("firstname is required."); @@ -75,7 +122,7 @@ class Onboarding extends Entity { await this.db.await_proc( `${this.app_db}.save_onboarding_user_info`, - sessionId, firstName, lastName, email, countryCode + sessionId, uid, firstName, lastName, email, countryCode ); this.output.data({ success: true, message: 'User info saved.', data: {} }); } @@ -84,12 +131,13 @@ class Onboarding extends Entity { * v2 Step 2: industry / kind of work. */ async save_industry() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; const industry = this.input.need('industry'); const industryOther = this.input.get('industry_other') || null; await this.db.await_proc( `${this.app_db}.save_onboarding_industry`, - sessionId, industry, industryOther + id.sessionId, id.uid, industry, industryOther ); this.output.data({ success: true, message: 'Industry saved.', data: {} }); } @@ -98,12 +146,13 @@ class Onboarding extends Entity { * v2 Step 3: role. */ async save_role() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; const role = this.input.need('role'); const roleOther = this.input.get('role_other') || null; await this.db.await_proc( `${this.app_db}.save_onboarding_role`, - sessionId, role, roleOther + id.sessionId, id.uid, role, roleOther ); this.output.data({ success: true, message: 'Role saved.', data: {} }); } @@ -112,11 +161,12 @@ class Onboarding extends Entity { * v2 Step 4: team size. Replaces save_usage_plan in the new wizard. */ async save_team_size() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; const teamSize = this.input.need('team_size'); await this.db.await_proc( `${this.app_db}.save_onboarding_team_size`, - sessionId, teamSize + id.sessionId, id.uid, teamSize ); this.output.data({ success: true, message: 'Team size saved.', data: {} }); } @@ -125,11 +175,12 @@ class Onboarding extends Entity { * v2 Step 5: workspace intent ("What do you want to start with?"). Optional. */ async save_intent() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; const intent = this.input.need('intent'); await this.db.await_proc( `${this.app_db}.save_onboarding_intent`, - sessionId, intent + id.sessionId, id.uid, intent ); this.output.data({ success: true, message: 'Intent saved.', data: {} }); } @@ -140,24 +191,50 @@ class Onboarding extends Entity { * the challenges array is required. */ async save_challenges() { - const sessionId = this.input.sid(); - const challenges = toArray(this.input.need('challenges')); + const id = this._identity(); + if (!id) return; + // `get`, not `need`: an EMPTY selection is a legal answer ("none of + // these"), and it must be able to overwrite a previously saved list. + // `need` would still admit [], but defaulting here also keeps a client + // that omits the key entirely from erroring out. + const challenges = toArray(this.input.get('challenges') || []); const note = this.input.get('note') || null; // Pass array directly — Drumee db driver handles JSON serialization. // Do NOT JSON.stringify here (causes double-encoding at the driver layer). await this.db.await_proc( `${this.app_db}.save_onboarding_challenges`, - sessionId, challenges, note + id.sessionId, id.uid, challenges, note ); this.output.data({ success: true, message: 'Challenges saved.', data: {} }); } /** - * + * True reset: clear the user's stored onboarding answers. + * + * The old implementation called clearAuthorization() and nothing else — it + * discarded the SESSION but kept the DATA. That is backwards on both counts: + * + * - the half-filled onboarding_responses row survived, keyed to a session + * id that no longer existed, so it was unreachable forever. Every reset + * leaked one orphan row. + * - the wizard restarted against a dead session, so the first save of the + * "fresh" run wrote under a different identity than the reads. + * + * The wizard runs inside an authenticated desk session; resetting a + * questionnaire is not a reason to destroy the user's login. So this drops + * the answers and leaves the session intact. The stored procedure also + * collects any orphan rows the previous implementation left behind. */ async reset() { - this.output.clearAuthorization(this.input.authorization()); - this.output.data({}); + const id = this._identity(); + if (!id) return; + const res = toArray( + await this.db.await_proc( + `${this.app_db}.reset_onboarding_response`, + id.sessionId, id.uid + ) + )[0] || {}; + this.output.data({ success: true, removed: res.removed || 0, status: 'reset' }); } /** @@ -190,7 +267,9 @@ class Onboarding extends Entity { * Valid values: personal | startup | enterprise */ async save_usage_plan() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; + const sessionId = id.sessionId; const usagePlan = this.input.need(Attr.args); const VALID_PLANS = ['personal', 'startup', 'enterprise']; @@ -211,18 +290,30 @@ class Onboarding extends Entity { * v2 Step 5A: tools currently used by the team (multi-select). * Valid values: google_drive | notion | slack | dropbox | * clickup | trello | jira | other - * FE sends: { tools: ["notion", "slack"] } + * FE sends: { tools: ["notion", "other"], tools_other: "Obsidian" } + * + * Two deliberate changes from v2: + * + * - An empty array is accepted instead of rejected. It used to throw + * ('tools array is required and must not be empty'), so the client + * skipped the call when nothing was selected — which meant de-selecting + * every tool silently left the previously saved list in the database. + * An empty selection is a real answer and must overwrite. + * + * - `tools_other` carries the "Other" free text in its own field, matching + * industry_other / role_other, instead of being spliced into the array + * where it was indistinguishable from a canonical key. The stored + * procedure normalises either shape, so older clients keep working. */ async save_tools() { - const sessionId = this.input.sid(); - const tools = toArray(this.input.need('tools')); - if (!tools.length) { - return this.exception.user('tools array is required and must not be empty.'); - } + const id = this._identity(); + if (!id) return; + const tools = toArray(this.input.get('tools') || []); + const toolsOther = this.input.get('tools_other') || null; // Pass array directly — Drumee db driver handles JSON serialization. await this.db.await_proc( `${this.app_db}.save_onboarding_tools`, - sessionId, tools + id.sessionId, id.uid, tools, toolsOther ); this.output.data({ success: true, message: 'Tools saved.', data: {} }); } @@ -231,7 +322,9 @@ class Onboarding extends Entity { * */ async save_privacy() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; + const sessionId = id.sessionId; const privacyLevel = this.input.need('privacy'); const level = parseInt(privacyLevel); @@ -251,11 +344,15 @@ class Onboarding extends Entity { * */ async check_completion() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; + const { sessionId, uid } = id; let completionStatusRaw; try { - completionStatusRaw = await this.db.await_proc(`${this.app_db}.check_onboarding_completion`, sessionId); + completionStatusRaw = await this.db.await_proc( + `${this.app_db}.check_onboarding_completion`, sessionId, uid + ); } catch (spError) { console.error(`[ONBOARDING ERROR] Error calling check_completion SP for session ${sessionId}: ${spError.message}`); throw spError; @@ -276,35 +373,75 @@ class Onboarding extends Entity { * */ async mark_complete() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; + const { sessionId, uid } = id; try { - await this.db.await_proc(`${this.app_db}.mark_onboarding_complete`, sessionId); + await this.db.await_proc(`${this.app_db}.mark_onboarding_complete`, sessionId, uid); } catch (spError) { console.error(`[ONBOARDING ERROR] Error calling mark_complete SP for session ${sessionId}: ${spError.message}`); - throw spError; + // Surface it as a client error rather than a 500: every SIGNAL this + // procedure raises is actionable by the user ("Step 2 is incomplete"), + // and the wizard now shows it and keeps them on the page instead of + // dropping them into a workspace with unsaved answers. + return this.exception.user( + (spError && spError.message) || 'Onboarding could not be completed.' + ); } this.output.data({ success: true, message: 'Onboarding marked as complete (validated).', data: {} }); } /** - * + * Mirror the onboarding answers onto the drumate profile and set + * onboarded = 1 (which is what stops desk from re-launching the wizard). + * + * Identity is now validated BEFORE any user data is touched. The previous + * order destructured `this.user.get(Attr.profile)` first and only then + * checked for ID_NOBODY, so an anonymous request threw on the destructure + * (profile is undefined) instead of returning the intended "no-user" — + * a 500 where a clean answer was already written two lines below. + * + * The row is looked up by uid first and only falls back to email. Email is + * not an identity: it is nullable on this table, it is only backfilled from + * step 1, and "latest row with this address" can belong to a different + * session than the one that just completed. uid is exact. */ async update_profile() { - const { email } = this.user.get(Attr.profile); - if (this.uid === ID_NOBODY) { + const uid = this._uid(); + if (!uid) { return this.output.data({ status: "no-user" }); } - const sql = `SELECT * FROM ${this.app_db}.onboarding_responses WHERE email=? ORDER BY mtime DESC LIMIT 1`; - const row = await this.yp.await_query(sql, email) || {}; - const { firstname, lastname, country_code, industry, role, team_size, intent } = row; + const { email } = this.user.get(Attr.profile) || {}; + + let row = await this.yp.await_query( + `SELECT * FROM ${this.app_db}.onboarding_responses WHERE uid=? ORDER BY mtime DESC LIMIT 1`, + uid + ); + if (!row && email) { + // Pre-migration rows carry no uid. Resolving them by email here is what + // keeps an in-flight onboarding (started before this deploy) completing + // normally instead of syncing an empty profile. + row = await this.yp.await_query( + `SELECT * FROM ${this.app_db}.onboarding_responses WHERE email=? ORDER BY mtime DESC LIMIT 1`, + email + ); + } + row = row || {}; + + const { + firstname, lastname, country_code, + industry, industry_other, role, role_other, team_size, intent + } = row; const profile = { onboarded: 1 }; if (firstname) profile.firstname = firstname; if (lastname) profile.lastname = lastname; if (country_code) profile.country_code = country_code; - if (role) profile.role = role; - if (industry) profile.industry = industry; + // Store what the user actually said, not the literal "other" marker — + // consistent with how the analytics export renders these columns. + if (role) profile.role = (role === 'other' && role_other) ? role_other : role; + if (industry) profile.industry = (industry === 'other' && industry_other) ? industry_other : industry; if (team_size) profile.team_size = team_size; if (intent) profile.intent = intent; await this.yp.await_proc('drumate_update_profile', this.uid, profile); @@ -316,11 +453,15 @@ class Onboarding extends Entity { * @returns */ async get_response() { - const sessionId = this.input.sid(); + const id = this._identity(); + if (!id) return; + const { sessionId, uid } = id; let responseDataRaw; let { xlink } = JSON.parse(this.conf); try { - responseDataRaw = await this.db.await_proc(`${this.app_db}.get_onboarding_response`, sessionId); + responseDataRaw = await this.db.await_proc( + `${this.app_db}.get_onboarding_response`, sessionId, uid + ); } catch (spError) { console.error(`[ONBOARDING ERROR] Error calling get_response SP for session ${sessionId}: ${spError.message}`); throw spError; @@ -333,13 +474,18 @@ class Onboarding extends Entity { return; } - // Parse JSON tools - if (responseData.current_tools && typeof responseData.current_tools === 'string') { - try { - responseData.current_tools = JSON.parse(responseData.current_tools); - } catch (e) { - this.warn("Failed to parse current_tools JSON for session:", sessionId); - responseData.current_tools = []; + // Parse the JSON columns. `challenges` is parsed too now: this payload + // drives wizard resume, and an unparsed string there meant the challenge + // chips came back unselected on every reload. + for (const key of ['current_tools', 'tools', 'challenges']) { + const v = responseData[key]; + if (v && typeof v === 'string') { + try { + responseData[key] = JSON.parse(v); + } catch (e) { + this.warn(`Failed to parse ${key} JSON for session:`, sessionId); + responseData[key] = []; + } } } this.conf = Cache.getSysConf('ob_conf'); @@ -356,8 +502,10 @@ class Onboarding extends Entity { * If loby's DB user lacks EXECUTE on C_reward, switch to this.yp.await_proc(...). */ async get_onboarding_invite_link() { - if (!this.uid) { - return this.exception.user('User not authenticated.'); + if (!this._uid()) { + // ID_NOBODY is a truthy string, so the old `!this.uid` test let + // anonymous callers straight through to the reward/referral lookups. + return this.exception.unauthorized('User not authenticated.'); } const rewardConf = JSON.parse(Cache.getSysConf('reward_hub_conf') || '{}'); @@ -404,8 +552,10 @@ class Onboarding extends Entity { * If loby DB user lacks EXECUTE on C_reward, switch to this.yp.await_proc(...). */ async send_onboarding_invites() { - if (!this.uid) { - return this.exception.user('User not authenticated.'); + if (!this._uid()) { + // ID_NOBODY is a truthy string, so the old `!this.uid` test let + // anonymous callers straight through to the reward/referral lookups. + return this.exception.unauthorized('User not authenticated.'); } const raw = toArray(this.input.need('emails')); @@ -492,7 +642,7 @@ class Onboarding extends Entity { * - folder_chat_started → channel table (high-frequency, not suitable for services_log) */ async get_activation_status() { - if (!this.uid) { + if (!this._uid()) { return this.output.data({ workspace_created: false, first_file_uploaded: false,