From 3daf2fcdc526d67ff3251953f94640581dfa1fbd Mon Sep 17 00:00:00 2001 From: jmcqueen Date: Thu, 9 Jul 2026 13:53:38 -0400 Subject: [PATCH] Verify (don't order) phone number; fix DECT location lookup endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs surfaced by the same failed finalize run for store 7311. 1. addPhoneNumbersToLocation was POSTing to /locations/{id}/numbers, which Webex treats as "provision a NEW number" — that triggered duplicate PSTN orders against numbers AE had already manually ordered through Control Hub, and the provider rejected them with ORDER_CREATION_FAILED. AE's workflow is manual ordering, so this step now VERIFIES the number's current state in the org via GET /telephony/config/numbers?phoneNumber=..., classifies the outcome (already here / pending / wrong location / not in org), and either succeeds silently or throws with a specific Control-Hub fix-it instruction. The bot never triggers a PSTN order now. 2. findDectNetworkInLocation was hitting GET /telephony/config/locations/{id}/dectNetworks, which Webex does not implement (returns HTTP 404 "No static resource ..." unconditionally). That silently broke both the finalize idempotency pre-check and the /provisionDect fallback, so /provisionDect kept showing the "create network" card for stores that already had one, then 409'd on the create attempt. Switched to the real endpoint GET /telephony/config/dectNetworks with a locationId filter. Also added translateCreateDectError to rewrite the deeply-nested 27453 "access code in use" 409 blob into an actionable "already exists, re-run /provisionDect" message. Extracted both error/state translators as pure exported functions (classifyNumberAssignment, translateCreateDectError) with unit-test coverage locking down the exact Webex payload shapes. README: updated /finalizeStore to reflect the manual-order expectation and specific fix-it instructions on mismatch. Co-authored-by: Cursor --- README.md | 14 +++-- src/flows/finalizeStore.js | 21 ++++--- src/webex/dect.js | 89 ++++++++++++++++++++--------- src/webex/locations.js | 112 ++++++++++++++++++++++++++++++++++--- test/dect.test.js | 47 +++++++++++++++- test/locations.test.js | 89 ++++++++++++++++++++++------- 6 files changed, 301 insertions(+), 71 deletions(-) diff --git a/README.md b/README.md index b742709..5fa9cff 100644 --- a/README.md +++ b/README.md @@ -132,11 +132,15 @@ Registered in [src/commands](src/commands): and attaches the Webex Calling license to the store user. Everything except the phone number, which has to be purchased separately in Control Hub. - `/finalizeStore ` — cut-over for a previously staged store: - attaches the purchased phone number, sets caller ID, creates the - `Store XXXX` DECT network (DBS-210) with the per-store default access - code, creates the auto-attendant, and finalizes user licensing. - Preflights that the store was actually staged first, and is idempotent - on the DECT network (skips if one already exists). + verifies the store's phone number is already assigned to the location + (it must be manually ordered in Control Hub first — this step never + triggers a new PSTN order), sets caller ID, creates the `Store XXXX` + DECT network (DBS-210) with the per-store default access code, creates + the auto-attendant, and finalizes user licensing. Preflights that the + store was actually staged first, and is idempotent on the DECT network + (skips if one already exists). On phone-number mismatches (pending + order, wrong location, or number missing from the org) finalize aborts + early with a specific Control-Hub fix-it instruction. - `/provisionDect ` — manage DECT phones for a store: card for adding basestations by MAC and adding/removing handsets. Every removal goes through an explicit confirmation card. New stores have diff --git a/src/flows/finalizeStore.js b/src/flows/finalizeStore.js index d0408ed..62880cd 100644 --- a/src/flows/finalizeStore.js +++ b/src/flows/finalizeStore.js @@ -44,12 +44,15 @@ export async function finalizeStoreLocation(bot, locationInfo, userInfo) { // Critical: every downstream step (calling identity, per-user // caller ID = LOCATION_NUMBER, auto attendant on this DID) needs - // the number attached to the location. If this fails there is no - // point cascading the rest and burying the root cause in noise — - // bail early so the actionable Webex message reaches the operator. + // the number attached to the location. The AE workflow expects + // this to already be true — the number is manually ordered in + // Control Hub before finalize runs — so this step just VERIFIES + // the assignment (never re-orders). On mismatch it errors with a + // specific Control-Hub instruction rather than cascading the + // failure into three unrelated 400s downstream. await runStep( bot, - 'Added phone numbers to location', + 'Verified phone number is assigned to location', () => addPhoneNumbersToLocation(location, locationInfo.phoneNumber), { critical: true }, ); @@ -113,10 +116,12 @@ async function ensureDectNetwork(bot, location, storeNumber) { return existing; } } catch (error) { - // 404s are handled inside findDectNetworkInLocation (return - // null), so anything reaching us here is a real problem worth - // flagging in the log. We still fall through to try creating, - // since a transient read failure shouldn't block finalize. + // findDectNetworkInLocation talks to the org-wide DECT list + // endpoint, so any error here is a real problem (auth, 5xx, + // etc.) worth flagging in the log. We still fall through to + // try creating: createDectNetwork translates the 409 "access + // code already in use" case into a clear "already exists" + // outcome, so a transient read failure never blocks finalize. logger.warn( `DECT network pre-check failed for ${netName} (status=${error?.status ?? '?'}): ${error?.message}`, ); diff --git a/src/webex/dect.js b/src/webex/dect.js index fd22c2f..3aad10d 100644 --- a/src/webex/dect.js +++ b/src/webex/dect.js @@ -107,25 +107,25 @@ export async function findDectNetworkForStore(storeNumber) { /** * Location-scoped DECT network lookup. Unlike findDectNetworkForStore - * this doesn't rely on the store user having lines on the network, so it - * safely finds freshly-created empty networks (e.g. right after finalize - * creates one and before any handsets are added). Returns the raw match - * or null. + * this doesn't rely on the store user having lines on the network, so + * it safely finds freshly-created empty networks (e.g. right after + * finalize creates one and before any handsets are added). Returns + * the raw match or null. + * + * We use the org-wide GET /telephony/config/dectNetworks endpoint + * with a locationId filter — NOT the per-location + * GET /telephony/config/locations/{id}/dectNetworks path. The latter + * looks natural but Webex has no such route: it returns HTTP 404 + * "No static resource ..." unconditionally, whether the location has + * DECT networks or not. That silently broke the finalize + * idempotency pre-check AND the /provisionDect fallback, causing + * /provisionDect to show the "create network" card for stores that + * already had one and then 409 on the create attempt. */ export async function findDectNetworkInLocation(locationId, storeNumber) { if (!locationId) return null; - let data; - try { - data = await webexJson('GET', `/telephony/config/locations/${locationId}/dectNetworks`); - } catch (err) { - // Webex returns 404 ("No static resource ... /dectNetworks") - // when a location has never had a DECT network — as opposed - // to returning an empty list. Treat that as "no networks - // yet" so the caller can proceed to create one without a - // spurious warning on the finalize output. - if (err?.status === 404) return null; - throw err; - } + const encoded = encodeURIComponent(locationId); + const data = await webexJson('GET', `/telephony/config/dectNetworks?locationId=${encoded}`); const items = data?.dectNetworks ?? data?.items ?? []; const target = dectNetworkName(storeNumber).toLowerCase(); return items.find((n) => (n.name ?? '').trim().toLowerCase() === target) ?? null; @@ -233,13 +233,43 @@ export async function getDectProvisioningStatus(storeNumber) { // Mutating helpers /** - * Create a multi-cell DECT network for this store's location. Uses the - * "Store <4-digit>" name convention. Access code defaults to the - * generateDectAccessCode(storeNumber) rule but can be overridden by the - * caller (the /provisionDect create card lets the operator edit it). + * Rewrite the raw Webex error from POST /locations/.../dectNetworks + * into something an operator can act on for the known "already + * exists" failure. Pure + exported so it can be unit-tested. * - * Idempotency is the caller's responsibility — Webex will 409 if a network - * with the same name already exists in this location. + * Webex code 27453 ("Default access code is in use by another DECT + * network: ") is the concrete symptom: our access code is + * derived from the store number, so a duplicate almost always means + * the store's own DECT network already exists. The right operator + * action is to re-run /provisionDect (which will now find it via + * the org-wide lookup) — not to pick a different access code. + */ +export function translateCreateDectError(err, storeNumber) { + if (typeof err?.body === 'string' && err.body.includes("'27453'")) { + const clearer = new Error( + `DECT network for Store ${String(storeNumber).padStart(4, '0')} ` + + 'already exists (default access code is in use). Re-run ' + + '`/provisionDect ' + + storeNumber + + '` to open the management card.', + ); + clearer.cause = err; + clearer.status = err.status; + return clearer; + } + return err; +} + +/** + * Create a multi-cell DECT network for this store's location. Uses + * the "Store <4-digit>" name convention. Access code defaults to the + * generateDectAccessCode(storeNumber) rule but can be overridden by + * the caller (the /provisionDect create card lets the operator edit + * it). + * + * Idempotency is the caller's responsibility, but we do translate the + * 409 "access code in use" error into a clearer message pointing at + * the fix (re-run /provisionDect to see the management card). */ export async function createDectNetwork(locationId, storeNumber, { defaultAccessCode } = {}) { if (!locationId) throw new Error('locationId required'); @@ -253,11 +283,16 @@ export async function createDectNetwork(locationId, storeNumber, { defaultAccess defaultAccessCodeEnabled: true, defaultAccessCode: code, }; - const res = await webexJson( - 'POST', - `/telephony/config/locations/${locationId}/dectNetworks`, - body, - ); + let res; + try { + res = await webexJson( + 'POST', + `/telephony/config/locations/${locationId}/dectNetworks`, + body, + ); + } catch (err) { + throw translateCreateDectError(err, storeNumber); + } logger.info(`Created DECT network ${name} (id=${res?.dectNetworkId})`); return { id: res?.dectNetworkId, name, locationId }; } diff --git a/src/webex/locations.js b/src/webex/locations.js index 33d1165..b9d59fa 100644 --- a/src/webex/locations.js +++ b/src/webex/locations.js @@ -1,4 +1,5 @@ import { webexJson } from './client.js'; +import { logger } from '../logger.js'; import { CALLER_ID, MUSIC_ON_HOLD, @@ -84,16 +85,109 @@ export function translateAddNumberError(err, phoneNumber) { return err; } +/** + * Look up a single phone number in the org's inventory. Returns the + * first matching record from Webex's /telephony/config/numbers list + * (which includes assignment / state / location info), or null when + * the number is not in the org at all. + * + * We compare against the org-wide listing rather than a + * per-location endpoint because we need to distinguish between + * (a) already in this location → skip, already done + * (b) in a different location → operator needs to move it + * (c) not in the org yet → operator needs to order it + * and only (a) is safe to proceed from silently. + */ +export async function findPhoneNumber(phoneNumber) { + const encoded = encodeURIComponent(phoneNumber); + const data = await webexJson('GET', `/telephony/config/numbers?phoneNumber=${encoded}`); + const items = data?.phoneNumbers ?? data?.items ?? []; + return items[0] ?? null; +} + +/** + * Classify what we found in the org against the location we're + * finalizing, and return a { kind, message?, ... } discriminator the + * caller can act on. Exported as a pure function so the branching + * logic is unit-testable without any HTTP. + * + * kinds: + * 'already-here' - number is ACTIVE in this location; nothing to do. + * 'pending' - number is in the org but not yet ACTIVE. Wait. + * 'wrong-location'- number is ACTIVE in a different location; move it. + * 'not-in-org' - number isn't in the org at all; order it first. + */ +export function classifyNumberAssignment(existing, phoneNumber, location) { + if (!existing) { + return { + kind: 'not-in-org', + message: + `Phone number ${phoneNumber} is not in the Webex org's number inventory. ` + + 'Order it in Control Hub (Calling → Numbers → Add), wait for the order ' + + 'to complete against location "' + + location.name + + '", then re-run /finalizeStore.', + }; + } + const state = String(existing.state ?? '').toUpperCase(); + const existingLocationId = existing.location?.id; + if (existingLocationId === location.id && state === 'ACTIVE') { + return { kind: 'already-here', existing }; + } + if (state && state !== 'ACTIVE') { + return { + kind: 'pending', + message: + `Phone number ${phoneNumber} is in the org but not yet ACTIVE ` + + `(state=${state}). The carrier order is still processing — wait a ` + + 'few minutes for it to complete, then re-run /finalizeStore.', + }; + } + // ACTIVE but in a different (or missing) location. + const currentLoc = existing.location?.name ?? '(unassigned)'; + return { + kind: 'wrong-location', + message: + `Phone number ${phoneNumber} is ACTIVE in the Webex org but assigned to ` + + `"${currentLoc}", not "${location.name}". Move it in Control Hub ` + + '(Calling → Numbers → the number → Actions → Move) so it lives on the ' + + 'store location, then re-run /finalizeStore.', + }; +} + +/** + * Ensure the given phone number is ACTIVE on the given location. This + * used to POST to /locations/{id}/numbers, which the Webex API treats + * as "provision a NEW number" — that triggered duplicate PSTN orders + * against numbers AE had already manually ordered through Control + * Hub, and the provider rejected them with ORDER_CREATION_FAILED. + * + * The AE workflow is: manually order the number in Control Hub + * (assigning it to the store's location during the wizard), then run + * /finalizeStore. All this helper needs to do here is verify that the + * number is already assigned to the location — if so, the very next + * step (updateLocationCallingIdentity) will select it as the main + * number. If not, we give the operator a concrete instruction for + * what to fix in Control Hub instead of triggering a bad order. + */ export async function addPhoneNumbersToLocation(location, phoneNumber) { - const body = { phoneNumbers: [phoneNumber], state: 'ACTIVE' }; - try { - return await webexJson( - 'POST', - `/telephony/config/locations/${encodeURIComponent(location.id)}/numbers`, - body, - ); - } catch (err) { - throw translateAddNumberError(err, phoneNumber); + const existing = await findPhoneNumber(phoneNumber); + const outcome = classifyNumberAssignment(existing, phoneNumber, location); + switch (outcome.kind) { + case 'already-here': + logger.info( + `${phoneNumber} already ACTIVE on location ${location.name} — skipping add`, + ); + return { alreadyAssigned: true, existing: outcome.existing }; + case 'pending': + case 'wrong-location': + case 'not-in-org': + throw new Error(outcome.message); + default: + // Defensive: classifyNumberAssignment should be total, but + // if a new kind is added and forgotten here, don't silently + // pretend everything's fine. + throw new Error(`Unhandled number-assignment outcome: ${outcome.kind}`); } } diff --git a/test/dect.test.js b/test/dect.test.js index c35a516..01d9e51 100644 --- a/test/dect.test.js +++ b/test/dect.test.js @@ -1,7 +1,11 @@ import { describe, it } from 'node:test'; import assert from 'node:assert/strict'; -import { dectNetworkName, generateDectAccessCode } from '../src/webex/dect.js'; +import { + dectNetworkName, + generateDectAccessCode, + translateCreateDectError, +} from '../src/webex/dect.js'; // AE's access code convention: 4-digit stores use their own digits; anything // shorter prefixes '8' and pads the rest to 3. Ports collabFinder's rule @@ -36,3 +40,44 @@ describe('dectNetworkName', () => { assert.equal(dectNetworkName('67'), 'Store 0067'); }); }); + +// Regression: when /provisionDect's location lookup was broken (see +// findDectNetworkInLocation notes) the create card would appear even +// though a DECT network already existed, and the resulting POST +// 409'd with a deeply-nested Webex error blob. The translator +// surfaces the actionable "re-run /provisionDect" instruction. +describe('translateCreateDectError', () => { + function fakeErr(status, body) { + const err = new Error(`HTTP ${status} for POST /whatever\n${body}`); + err.status = status; + err.body = body; + return err; + } + + const CONFLICT_BODY = + '{"error":[{"key":"409","message":[{"description":"POST failed: HTTP/1.1 409 Conflict (url = https://cpapi-a.wbx2.com/api/v1/customers/DUMMY/locations/DUMMY/dects, request/response TrackingId = ROUTERGW_xxx, errorCode = \'27453\', error = \'[Error 27453] Default access code is in use by another DECT network: 7311\')","code":"27453"}]}],"trackingId":"ROUTERGW_xxx"}'; + + it('rewrites Webex error 27453 into an actionable message', () => { + const raw = fakeErr(409, CONFLICT_BODY); + const translated = translateCreateDectError(raw, 7311); + assert.notEqual(translated, raw, 'should be a new Error instance'); + assert.match(translated.message, /already exists/i); + assert.match(translated.message, /\/provisionDect 7311/); + assert.match(translated.message, /Store 7311/); + assert.equal(translated.status, 409); + assert.equal(translated.cause, raw); + }); + + it('pads short store numbers in the error text (matches dectNetworkName convention)', () => { + const raw = fakeErr(409, CONFLICT_BODY); + const translated = translateCreateDectError(raw, 499); + assert.match(translated.message, /Store 0499/); + }); + + it('returns unrelated errors unchanged', () => { + const raw = fakeErr(400, '{"message":"something else","errorCode":9999}'); + const result = translateCreateDectError(raw, 7311); + assert.equal(result, raw); + assert.doesNotMatch(result.message, /already exists/i); + }); +}); diff --git a/test/locations.test.js b/test/locations.test.js index 20df685..ef83a01 100644 --- a/test/locations.test.js +++ b/test/locations.test.js @@ -1,7 +1,9 @@ import { describe, it } from 'node:test'; import assert from 'node:assert/strict'; -import { translateAddNumberError } from '../src/webex/locations.js'; +import { classifyNumberAssignment, translateAddNumberError } from '../src/webex/locations.js'; + +const LOC = { id: 'LOC-STORE-7311', name: 'Store 7311' }; // The exact JSON Webex returns when a freshly-purchased number is // still being provisioned by the carrier. Kept verbatim from a real @@ -18,11 +20,6 @@ function fakeHttpError(status, body) { } describe('translateAddNumberError', () => { - // Regression: without the translation, the raw Webex error was - // propagated through runStep, cascaded into a Caller-ID failure - // (LOCATION_NUMBER unavailable) and an auto-attendant failure - // (number "used in another location"), and the actionable root - // cause was buried under three stack traces. it('rewrites NUMBER_HAS_PENDING_ORDERS into an actionable operator message', () => { const raw = fakeHttpError(400, PENDING_ORDERS_BODY); const translated = translateAddNumberError(raw, '+14045551212'); @@ -34,26 +31,76 @@ describe('translateAddNumberError', () => { assert.equal(translated.cause, raw, 'raw error should be preserved as .cause'); }); - it('returns unrelated errors unchanged (same instance, no message rewrite)', () => { + it('returns unrelated errors unchanged (same instance)', () => { const raw = fakeHttpError(400, '{"message":"Nope","errorCode":9999}'); const result = translateAddNumberError(raw, '+14045551212'); - assert.equal(result, raw, 'must be same instance so callers can rethrow'); + assert.equal(result, raw); assert.doesNotMatch(result.message, /pending order/i); }); - it('tolerates errors with no body property (e.g. transport failures)', () => { + it('tolerates errors with no body property', () => { const raw = new Error('ENOTFOUND webexapis.com'); - const result = translateAddNumberError(raw, '+14045551212'); - assert.equal(result, raw); - }); - - it('does not misfire on other 400 payloads that happen to be arrays', () => { - // Same array-of-errors shape but a different error code. - const raw = fakeHttpError( - 400, - '[{"errors":[{"errorCode":"ERR.OTHER","errorMessage":"unrelated"}]}]', - ); - const result = translateAddNumberError(raw, '+14045551212'); - assert.equal(result, raw); + assert.equal(translateAddNumberError(raw, '+14045551212'), raw); + }); +}); + +// classifyNumberAssignment decides what to do about the number's +// current state in the org. The whole point of extracting it is so +// this branching is unit-testable without any HTTP; the four kinds +// here map 1:1 to the four operator-visible outcomes. +describe('classifyNumberAssignment', () => { + // Regression: the endpoint that used to run in this step + // (POST /telephony/config/locations/{id}/numbers) triggers a new + // PSTN order — which is exactly wrong for AE's manual-order + // workflow. The "already-here" path is the common case now and + // must be a clean no-op. + it("returns 'already-here' when the number is ACTIVE in this location", () => { + const existing = { + phoneNumber: '+14045551212', + state: 'ACTIVE', + location: { id: LOC.id, name: LOC.name }, + }; + const out = classifyNumberAssignment(existing, '+14045551212', LOC); + assert.equal(out.kind, 'already-here'); + assert.equal(out.existing, existing); + }); + + it("returns 'pending' when the number is in the org but not ACTIVE yet", () => { + const existing = { + phoneNumber: '+14045551212', + state: 'PENDING', + location: { id: LOC.id, name: LOC.name }, + }; + const out = classifyNumberAssignment(existing, '+14045551212', LOC); + assert.equal(out.kind, 'pending'); + assert.match(out.message, /still processing/i); + assert.match(out.message, /re-run \/finalizeStore/); + }); + + it("returns 'wrong-location' when the number is ACTIVE in a different location", () => { + const existing = { + phoneNumber: '+14045551212', + state: 'ACTIVE', + location: { id: 'LOC-OTHER', name: 'Store 0499' }, + }; + const out = classifyNumberAssignment(existing, '+14045551212', LOC); + assert.equal(out.kind, 'wrong-location'); + assert.match(out.message, /Store 0499/); // current location shown + assert.match(out.message, /Store 7311/); // target location shown + assert.match(out.message, /Move it in Control Hub/); + }); + + it('treats a missing location on the record as wrong-location (unassigned)', () => { + const existing = { phoneNumber: '+14045551212', state: 'ACTIVE' }; + const out = classifyNumberAssignment(existing, '+14045551212', LOC); + assert.equal(out.kind, 'wrong-location'); + assert.match(out.message, /\(unassigned\)/); + }); + + it("returns 'not-in-org' when the number isn't in the inventory at all", () => { + const out = classifyNumberAssignment(null, '+14045551212', LOC); + assert.equal(out.kind, 'not-in-org'); + assert.match(out.message, /Order it in Control Hub/); + assert.match(out.message, /Store 7311/); }); });