From f6d2c1700a309b8b3fbb0d632fbe903b15ac5539 Mon Sep 17 00:00:00 2001 From: Rainer Friederich Date: Fri, 19 Jun 2026 10:14:14 +0200 Subject: [PATCH 1/6] feat(serenity): mirror each Semrush market as a SpaceCat Site Every Semrush market (project) now gets a resolvable SpaceCat Site on our side, for backwards compatibility and integrations that look a site up by its base URL. Domain model: a brand is a shell with no domain of its own (like its Semrush sub-workspace). Each MARKET has its own primary URL/domain, and that domain maps to a single Site (global sites.base_url uniqueness => at most one Site per domain). A brand whose markets span distinct domains owns several Sites. - ensureMarketSite (site-linkage.js): idempotently ensures a Site per market domain (deliveryType 'other') and links it to the brand via a brand_sites row tagged type='serenity' (the marker names the owning feature, not the provider). Same-org guard; fully best-effort (never fails a live market). - Hooks (subworkspace mode only): brand creation (initial market), activation (activated markets' domain), market creation (that market's domain). Never auto-deleted; market deletion leaves the Site + link in place. - syncBrandSites preserves type='serenity' rows: a market's domain is generally not in brand.urls, so the delete-all-then-reinsert rebuild would otherwise wipe these links on every brand edit. Excludes them from the delete (incl. NULL-type rows) and keeps the marker from being downgraded on re-upsert. - mapDbBrandToV2 excludes type='serenity' rows from urls[] and siteIds: a market's domain is not a brand URL, so these rows stay a pure backend linkage (resolved via sites / brand_sites directly). Brand V2 response is unchanged. - SerenityMarket.domain exposed (per-market) on the markets API + OpenAPI; docs/serenity.md "Site mirroring" section added. Requires the mysticat-data-service migration allowing brand_sites.type= 'serenity' to be deployed first (else the link insert is silently rejected). Co-Authored-By: Claude Opus 4.8 --- docs/openapi/schemas.yaml | 9 + docs/serenity.md | 11 ++ src/controllers/brands.js | 21 +++ src/controllers/serenity.js | 28 +++ src/support/brands-storage.js | 48 ++++- src/support/serenity/site-linkage.js | 147 +++++++++++++++ src/support/serenity/subworkspace-projects.js | 4 + test/controllers/brands.test.js | 60 +++++- test/controllers/serenity.test.js | 63 +++++++ test/support/brands-storage.test.js | 113 +++++++++++- test/support/serenity/site-linkage.test.js | 173 ++++++++++++++++++ .../serenity/subworkspace-projects.test.js | 10 +- 12 files changed, 678 insertions(+), 9 deletions(-) create mode 100644 src/support/serenity/site-linkage.js create mode 100644 test/support/serenity/site-linkage.test.js diff --git a/docs/openapi/schemas.yaml b/docs/openapi/schemas.yaml index 232c0ec969..312ff95ba6 100644 --- a/docs/openapi/schemas.yaml +++ b/docs/openapi/schemas.yaml @@ -10642,6 +10642,15 @@ SerenityMarket: semrushProjectId: type: string description: Upstream Semrush AIO project id for this slice (sub-workspace mode only). + domain: + description: | + The market's own domain (its primary URL host, e.g. "example.com"). + Each market has its own primary URL/domain — the brand itself has none. + Additive, sub-workspace mode only. Surfaced so the market overview can + display it. Null when the upstream project carries no domain. + oneOf: + - type: string + - type: 'null' SerenityMarketListResponse: type: object diff --git a/docs/serenity.md b/docs/serenity.md index 5548990c0f..4f7ffd746c 100644 --- a/docs/serenity.md +++ b/docs/serenity.md @@ -137,6 +137,17 @@ Ordering (mirrors the create flow in reverse): The DELETE is **not soft**. The UI must confirm with the user before invoking — the linked upstream project (and all its prompts) is permanently destroyed. +## SpaceCat Site mirroring (`brand_sites`) + +For backwards compatibility and integrations, every Semrush market (project) is mirrored as a SpaceCat **Site** on our side. The domain model is the key thing to hold onto: + +- A **brand is a shell** with **no domain of its own** — like its Semrush sub-workspace. **Each market has its own primary URL/domain**, and that domain maps to a single Site (global `sites.base_url` uniqueness ⇒ at most one Site per domain). A brand whose markets span distinct domains therefore owns several market Sites. +- The Site is linked to the owning brand via a **`brand_sites` row tagged `type='serenity'`** (`src/support/serenity/site-linkage.js` → `ensureMarketSite`; the marker names the owning feature, not the provider). The marker is load-bearing: + - **`syncBrandSites` preserves it.** That function rebuilds `brand_sites` from `brand.urls` on every brand edit (delete-all-then-reinsert). A market's domain is generally **not** in `brand.urls`, so an unmarked row would be silently deleted on the next edit. The marker excludes these rows from the delete and keeps their type from being downgraded on re-upsert. + - **`mapDbBrandToV2` excludes it.** A market's domain is not a brand URL, so `type='serenity'` rows never surface in the brand V2 response (`urls[]` / `siteIds`). Integrations resolve them via the `sites` / `brand_sites` tables directly. +- **Lifecycle:** the Site (+ link) is ensured on **brand creation** (initial market), **activation** (the activated markets' domain), and **market creation** (that market's domain). It is **never auto-deleted** — market deletion leaves the Site and its link in place. +- **Best-effort:** the Semrush project is the primary outcome and has already succeeded when mirroring runs, so a Site/link failure is logged and swallowed (never fails a live market). `Site.create` uses `deliveryType: 'other'` (not an AEM target). + ## Activate / deactivate (sub-workspace dual-mode) A brand runs in one of two modes, decided entirely by `brands.semrush_workspace_id`: diff --git a/src/controllers/brands.js b/src/controllers/brands.js index e862d34107..e10e828c6f 100644 --- a/src/controllers/brands.js +++ b/src/controllers/brands.js @@ -54,6 +54,7 @@ import { getBrandCompetitors, } from '../support/brands-storage.js'; import { provisionBrandSubworkspace, releaseProvisionedWorkspace } from '../support/serenity/brand-provisioning.js'; +import { ensureMarketSite } from '../support/serenity/site-linkage.js'; import { createSerenityTransport, SerenityTransportError } from '../support/serenity/rest-transport.js'; import { syncBrandUrlsAcrossMarkets } from '../support/serenity/brand-urls.js'; import { @@ -1383,6 +1384,9 @@ function BrandsController(ctx, log, env) { // brand never exists without a valid Semrush side. The pre-generated id is // the sub-workspace title key and is forced onto the row. let provisionedBrandId = null; + // The initial market's domain, resolved once during provisioning and reused + // by the site-mirror hook below (avoids re-deriving from the payload). + let provisionedBrandDomain = null; // A pending (draft) brand defers ALL Semrush provisioning: no // sub-workspace, no project, and crucially no primary URL required. The // wizard's "Save as pending" path lands here so a user can stash a brand @@ -1419,6 +1423,7 @@ function BrandsController(ctx, log, env) { if (!hasText(brandDomain)) { return badRequest('A primary URL is required to provision a Semrush brand'); } + provisionedBrandDomain = brandDomain; // The project needs at least one AI model (LLM) to track. The wizard // collects them; reject a Semrush create that omits them. const modelIds = Array.isArray(brandData.semrushModelIds) @@ -1472,6 +1477,22 @@ function BrandsController(ctx, log, env) { semrushWorkspaceId: provisionedWorkspaceId, }); + // When a Semrush sub-workspace + initial market were provisioned, mirror that + // initial market as a SpaceCat Site (+ brand_sites link) keyed on the + // market's domain, so the Semrush project has a resolvable site entity. + // Best-effort: never throws (a throw here would trip the workspace-release + // compensation below for a live brand). + if (hasText(provisionedWorkspaceId)) { + await ensureMarketSite(context, { + organizationId: spaceCatId, + brandId: provisionedBrandId, + // The initial market's domain, resolved during provisioning above. + domain: provisionedBrandDomain, + updatedBy, + log, + }); + } + return createResponse(created, 201); } catch (error) { log.error(`Error creating brand for organization ${spaceCatId}:`, error); diff --git a/src/controllers/serenity.js b/src/controllers/serenity.js index bbc0241724..248dfa2d03 100644 --- a/src/controllers/serenity.js +++ b/src/controllers/serenity.js @@ -61,6 +61,7 @@ import { } from '../support/brands-storage.js'; import { ErrorWithStatusCode } from '../support/utils.js'; import { hostnameFromUrlString } from '../support/url-utils.js'; +import { ensureMarketSite } from '../support/serenity/site-linkage.js'; const MAX_ERR_MSG_LEN = 500; const BEARER_PREFIX = 'Bearer '; @@ -541,6 +542,18 @@ function SerenityController(context, log, env) { brandPointerReloader(ctx, auth.brandUuid), { brandAliases, brandUrlSources, competitors }, ); + // Mirror this market as a SpaceCat Site (+ brand_sites link) keyed on the + // market's own domain, once its Semrush project is created. Best-effort: + // never fails a live market. + if (result?.status === 201) { + await ensureMarketSite(ctx, { + organizationId: brand.getOrganizationId(), + brandId: auth.brandUuid, + domain: ctx.data?.brandDomain, + updatedBy: 'serenity-create-market', + log, + }); + } } else { result = await handleCreateMarket( transport, @@ -942,6 +955,21 @@ function SerenityController(context, log, env) { }); } } + // Mirror the activated markets as a SpaceCat Site (+ brand_sites link) once + // at least one is live. Every market provisioned by this activation was + // created against the single resolved `brandDomain` (the body/stash primary + // URL), so one idempotent ensure on that domain covers them. Markets added + // later via createMarket carry their own domain and are mirrored there. + // Best-effort: never fails the activation. + if (anyLive) { + await ensureMarketSite(ctx, { + organizationId: brand.getOrganizationId(), + brandId: auth.brandUuid, + domain: brandDomain, + updatedBy: 'serenity-activate', + log, + }); + } // Success-level summary so a completed activation can be correlated with // upstream state during incident investigation (counts + workspace). log.info('serenity activate: completed', { diff --git a/src/support/brands-storage.js b/src/support/brands-storage.js index 5c8b38c19a..26734b192e 100644 --- a/src/support/brands-storage.js +++ b/src/support/brands-storage.js @@ -12,6 +12,8 @@ import { composeBaseURL, hasText } from '@adobe/spacecat-shared-utils'; +import { SERENITY_BRAND_SITE_TYPE } from './serenity/site-linkage.js'; + /** * PostgREST select string — joins all normalized child tables. */ @@ -115,13 +117,20 @@ function parseUrlParts(urlString) { * `brand_sites` expansion, where every entry is by definition onboarded. */ function mapDbBrandToV2(row) { - const siteIds = (row.brand_sites || []).map((bs) => bs.site_id).filter(hasText); + // Exclude Semrush market-site rows from the brand response: a market's domain + // is NOT a brand URL (the brand is a shell with no domain of its own), so these + // rows must not surface in urls[] or siteIds. They are a pure backend linkage — + // integrations resolve them via the sites / brand_sites tables directly. + const ownBrandSites = (row.brand_sites || []) + .filter((bs) => bs.type !== SERENITY_BRAND_SITE_TYPE); + + const siteIds = ownBrandSites.map((bs) => bs.site_id).filter(hasText); // Index brand_sites by normalized base URL so brand_urls entries can be // tagged onboarded/siteId by matching their base. brand_sites.site_id is // NOT NULL in the schema, so no defensive filter on it here. const siteByBase = new Map(); - (row.brand_sites || []).forEach((bs) => { + ownBrandSites.forEach((bs) => { const base = bs.sites?.base_url; if (!hasText(base)) { return; @@ -135,7 +144,7 @@ function mapDbBrandToV2(row) { // Legacy fallback: expand brand_sites paths into URL entries (one per path, // or one for the base URL when no paths are set). Used when brand_urls is // empty — i.e. the brand predates the brand_urls child table. - const brandSitesUrls = (row.brand_sites || []).flatMap((bs) => { + const brandSitesUrls = ownBrandSites.flatMap((bs) => { const base = bs.sites?.base_url; if (!hasText(base)) { return []; @@ -251,10 +260,35 @@ async function replaceChildRows(table, brandId, rows, onConflict, postgrestClien * (via composeBaseURL) so that multiple paths under the same site share one brand_sites row. */ async function syncBrandSites(organizationId, brandId, urls, postgrestClient, updatedBy) { + // Serenity market-site rows (type='serenity') are owned by the serenity market + // lifecycle, NOT by the brand's URL list. A market's domain is generally not in + // brand.urls, so the delete-all-then-reinsert below would wipe these links on + // every brand edit. Preserve them: collect the protected site ids first, exclude + // them from the delete, and keep their type from being downgraded on re-upsert + // (when a brand URL happens to resolve to the same site as a market). + // + // The delete is type-based (IS DISTINCT FROM 'serenity'), so a serenity row + // inserted concurrently by ensureMarketSite is never deleted here. The only + // residual race is a downgrade: if a concurrent ensureMarketSite inserts a + // serenity row for a site that is ALSO a brand URL, between this SELECT and the + // upsert below, the upsert may re-tag it to the URL's type. That requires a + // simultaneous brand edit + market write whose domains collide — by design + // unusual — and self-heals on the next market write (ensureMarketSite re-upserts + // type='serenity'). Not worth a cross-request lock PostgREST can't cheaply give. + const { data: protectedRows } = await postgrestClient + .from('brand_sites') + .select('site_id') + .eq('brand_id', brandId) + .eq('type', SERENITY_BRAND_SITE_TYPE); + const protectedSiteIds = new Set((protectedRows || []).map((r) => r.site_id)); + const { error: deleteError } = await postgrestClient .from('brand_sites') .delete() - .eq('brand_id', brandId); + .eq('brand_id', brandId) + // Delete every non-semrush row (including NULL-type rows; a bare .neq would + // skip NULLs). type IS DISTINCT FROM 'serenity'. + .or(`type.is.null,type.neq.${SERENITY_BRAND_SITE_TYPE}`); if (deleteError) { throw new Error(`Failed to sync brand_sites: ${deleteError.message}`); } @@ -304,7 +338,11 @@ async function syncBrandSites(organizationId, brandId, urls, postgrestClient, up brand_id: brandId, site_id: s.id, paths: pathsByBase.get(s.base_url) || [], - type: typeByBase.get(s.base_url) || null, + // A brand URL may resolve to the same site as a preserved market row. Keep + // that row tagged 'serenity' rather than downgrading it to the URL's type. + type: protectedSiteIds.has(s.id) + ? SERENITY_BRAND_SITE_TYPE + : (typeByBase.get(s.base_url) || null), updated_by: updatedBy, })); diff --git a/src/support/serenity/site-linkage.js b/src/support/serenity/site-linkage.js new file mode 100644 index 0000000000..6601df776b --- /dev/null +++ b/src/support/serenity/site-linkage.js @@ -0,0 +1,147 @@ +/* + * Copyright 2026 Adobe. All rights reserved. + * This file is licensed to you under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. You may obtain a copy + * of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under + * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS + * OF ANY KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ + +import { composeBaseURL, hasText } from '@adobe/spacecat-shared-utils'; +import { Site as SiteModel } from '@adobe/spacecat-shared-data-access'; +import { hostnameFromUrlString } from '../url-utils.js'; + +/** + * The brand_sites.type marker for a Site that mirrors a Semrush market (project). + * It distinguishes these rows from the brand's own-URL / citation rows so that: + * - syncBrandSites preserves them (a brand edit must not delete a market's site + * link just because the market's domain isn't in the brand's URL list), and + * - mapDbBrandToV2 excludes them from the brand's urls[] (a market's domain is + * NOT a brand URL — the brand is a shell with no domain of its own). + */ +export const SERENITY_BRAND_SITE_TYPE = 'serenity'; + +/** + * Mirrors a Semrush market (project) as a SpaceCat Site entity, linked to the + * owning brand via a `brand_sites` row tagged `type='serenity'`. + * + * Domain model (per the Serenity setup): a brand is a shell with NO domain of its + * own — like its Semrush sub-workspace. Each MARKET (project) has its own primary + * URL / domain, and that domain maps to a single Site on our side (for backwards + * compatibility and integrations that resolve a site by its base URL). A brand + * with markets on distinct domains therefore owns several market Sites. + * + * Why the dedicated `type='serenity'` marker (not a plain own-site row): a market's + * domain is generally NOT in the brand's URL list, and `syncBrandSites` rebuilds + * `brand_sites` from `brand.urls` on every brand edit (delete-all-then-reinsert). + * An unmarked row would be silently deleted on the next edit. The marker lets + * syncBrandSites preserve the link and lets mapDbBrandToV2 keep it out of the + * brand's urls[]. + * + * Lifecycle: created on brand creation / activation / market creation; never + * auto-deleted (market deletion leaves the Site + link in place). + * + * Best-effort by contract: the Semrush project is the primary outcome and has + * already succeeded when this runs, so a site/link failure is logged and + * swallowed (never thrown) rather than failing a market the user can see is live. + * + * @param {object} ctx - request context (ctx.dataAccess.Site + postgrestClient). + * @param {object} opts + * @param {string} opts.organizationId - the brand's organization UUID. + * @param {string} opts.brandId - the brand UUID. + * @param {string} opts.domain - the market/project domain or primary URL. Tolerates + * a bare hostname ("example.com") or a full URL ("https://example.com/x"); it is + * normalized to the hostname via hostnameFromUrlString (the single source of truth + * for brand -> Semrush project domain derivation) so all call sites resolve to the + * same base URL as the brand-create path. + * @param {string} [opts.updatedBy] - audit actor for the brand_sites row. + * @param {object} [opts.log] - logger. + * @returns {Promise} the linked site id, or null when skipped/failed + * (including the cross-org case, where the site exists but is NOT linked). + */ +export async function ensureMarketSite(ctx, { + organizationId, + brandId, + domain, + updatedBy = 'serenity-market', + log, +} = {}) { + if (!hasText(domain) || !hasText(organizationId) || !hasText(brandId)) { + return null; + } + + const Site = ctx?.dataAccess?.Site; + const postgrestClient = ctx?.dataAccess?.services?.postgrestClient; + if (!Site || typeof Site.findByBaseURL !== 'function') { + log?.warn?.('ensureMarketSite: Site data-access unavailable; skipping', { brandId, domain }); + return null; + } + + // Normalize to a bare hostname first: callers pass either a bare domain or a + // full URL, and composeBaseURL does not strip a path/scheme — so a URL with a + // path would resolve to the wrong base_url, miss the existing Site, and hit the + // global base_url uniqueness constraint on every retry. hostnameFromUrlString + // keeps this in lockstep with brandDomainFromPayload (the brand-create path). + const hostname = hostnameFromUrlString(domain); + if (!hasText(hostname)) { + log?.warn?.('ensureMarketSite: domain did not resolve to a hostname; skipping', { brandId, domain }); + return null; + } + const baseURL = composeBaseURL(hostname); + + try { + // Global base_url uniqueness means at most one Site per domain; findByBaseURL + // makes this idempotent across markets that happen to share a domain. + let site = await Site.findByBaseURL(baseURL); + if (!site) { + // OTHER delivery type: a Semrush-managed market site is not an AEM target. + site = await Site.create({ + baseURL, + organizationId, + deliveryType: SiteModel.DELIVERY_TYPES.OTHER, + }); + } + const siteId = site.getId(); + + // Only link a same-org site. A pre-existing site for this domain in another + // org cannot be duplicated (base_url is globally unique) and must not be + // cross-linked — this mirrors syncBrandSites, which matches sites by org. + if (site.getOrganizationId() !== organizationId) { + log?.warn?.('ensureMarketSite: existing site for domain belongs to another org; not linked', { + brandId, domain, siteId, siteOrg: site.getOrganizationId(), brandOrg: organizationId, + }); + // Return null: the site exists but no brand_sites link was established, so a + // caller must not read this as a successful mirror. + return null; + } + + if (postgrestClient?.from) { + const { error } = await postgrestClient + .from('brand_sites') + .upsert({ + organization_id: organizationId, + brand_id: brandId, + site_id: siteId, + paths: ['/'], + type: SERENITY_BRAND_SITE_TYPE, + updated_by: updatedBy, + }, { onConflict: 'brand_id,site_id' }); + if (error) { + // Non-fatal: the Site exists; the link can be re-ensured on a later + // market write. The marker keeps it safe from syncBrandSites once set. + log?.warn?.('ensureMarketSite: brand_sites link upsert failed (non-fatal)', { + brandId, siteId, error: error.message, + }); + } + } + return siteId; + } catch (e) { + log?.error?.('ensureMarketSite: failed to ensure site for market domain (non-fatal)', { + brandId, domain, error: e.message, + }); + return null; + } +} diff --git a/src/support/serenity/subworkspace-projects.js b/src/support/serenity/subworkspace-projects.js index c403cb009d..84f3370a08 100644 --- a/src/support/serenity/subworkspace-projects.js +++ b/src/support/serenity/subworkspace-projects.js @@ -103,6 +103,10 @@ export function projectToSlice(project, brandId) { updatedAt: project?.updated_at ?? project?.published_at ?? null, status: mapPublishStatus(project?.publish_status), semrushProjectId: hasText(project?.id) ? String(project.id) : null, + // The project's domain (its primary URL host) — surfaced so the market + // overview can show it. Echoed at the project's top level on the v1 read + // view (the same field brand-urls re-sync reads back). Null when absent. + domain: hasText(project?.domain) ? String(project.domain) : null, }; } diff --git a/test/controllers/brands.test.js b/test/controllers/brands.test.js index 16e21f51cb..969ab9e36e 100644 --- a/test/controllers/brands.test.js +++ b/test/controllers/brands.test.js @@ -4363,9 +4363,16 @@ describe('Brands Controller', () => { semrushModelIds: ['model-a', 'model-b'], }; - async function buildController({ provisionBrandSubworkspace, upsertBrand }) { + async function buildController({ + provisionBrandSubworkspace, upsertBrand, ensureMarketSite, + }) { const Mocked = await esmock('../../src/controllers/brands.js', { '../../src/support/serenity/brand-provisioning.js': { provisionBrandSubworkspace }, + // Stub the site mirror by default so these tests stay isolated from the + // Site/brand_sites side effect; a test that cares passes its own stub. + '../../src/support/serenity/site-linkage.js': { + ensureMarketSite: ensureMarketSite || sinon.stub().resolves('site-x'), + }, ...(upsertBrand ? { '../../src/support/brands-storage.js': { upsertBrand } } : {}), }); return Mocked.default(context, loggerStub, mockEnv); @@ -4403,6 +4410,57 @@ describe('Brands Controller', () => { expect(upsertArgs.semrushWorkspaceId).to.equal('ws-1'); }); + it('mirrors the provisioned brand domain as a Site (+ brand_sites link) after the row is written', async () => { + const provisionStub = sinon.stub().resolves({ semrushWorkspaceId: 'ws-1' }); + const upsertStub = sinon.stub().resolves({ id: 'forced-id', name: 'New Brand' }); + const ensureSiteStub = sinon.stub().resolves('site-x'); + const controller = await buildController({ + provisionBrandSubworkspace: provisionStub, + upsertBrand: upsertStub, + ensureMarketSite: ensureSiteStub, + }); + + const response = await controller.createBrandForOrg({ + ...context, + params: { spaceCatId: ORGANIZATION_ID }, + data: { ...semrushData }, + dataAccess: mockDataAccess, + attributes: { authInfo: { profile: { email: 'user@test.com' } } }, + }); + + expect(response.status).to.equal(201); + expect(ensureSiteStub).to.have.been.calledOnce; + // runs after the brand row is persisted (compensation safety) + expect(ensureSiteStub.calledAfter(upsertStub)).to.equal(true); + const opts = ensureSiteStub.firstCall.args[1]; + expect(opts.organizationId).to.equal(ORGANIZATION_ID); + expect(opts.domain).to.equal('acme.com'); + expect(opts.brandId).to.equal(provisionStub.firstCall.args[1].brandId); + }); + + it('does NOT mirror a Site for a pending (draft) brand — nothing is provisioned yet', async () => { + const provisionStub = sinon.stub().resolves({ semrushWorkspaceId: 'ws-1' }); + const upsertStub = sinon.stub().resolves({ id: 'draft-id', name: 'New Brand', status: 'pending' }); + const ensureSiteStub = sinon.stub().resolves('site-x'); + const controller = await buildController({ + provisionBrandSubworkspace: provisionStub, + upsertBrand: upsertStub, + ensureMarketSite: ensureSiteStub, + }); + + const response = await controller.createBrandForOrg({ + ...context, + params: { spaceCatId: ORGANIZATION_ID }, + data: { ...semrushData, status: 'pending' }, + dataAccess: mockDataAccess, + attributes: { authInfo: { profile: { email: 'user@test.com' } } }, + }); + + expect(response.status).to.equal(201); + expect(provisionStub).to.not.have.been.called; + expect(ensureSiteStub).to.not.have.been.called; + }); + it('forwards the brand URL sources (urls + social + earned) to provisioning', async () => { const provisionStub = sinon.stub().resolves({ semrushWorkspaceId: 'ws-1' }); const upsertStub = sinon.stub().resolves({ id: 'forced-id', name: 'New Brand' }); diff --git a/test/controllers/serenity.test.js b/test/controllers/serenity.test.js index 7919c25ab2..6623eaeb96 100644 --- a/test/controllers/serenity.test.js +++ b/test/controllers/serenity.test.js @@ -39,6 +39,7 @@ function makeBrandModel(overrides = {}) { return { getId: () => BRAND, getName: () => 'Test Brand', + getOrganizationId: () => ORG, getSemrushWorkspaceId: () => 'subworkspace-ws-1', setSemrushWorkspaceId: sinon.stub(), setStatus: sinon.stub(), @@ -127,6 +128,7 @@ describe('SerenityController', () => { let getBrandUrlSourcesStub; let getBrandCompetitorsStub; let accessControlHasAccessStub; + let ensureMarketSiteStub; let MockTransportError; let SerenityController; @@ -148,6 +150,7 @@ describe('SerenityController', () => { .resolves({ urls: [], socialAccounts: [], earnedContent: [] }); getBrandCompetitorsStub = sinon.stub().resolves([]); accessControlHasAccessStub = sinon.stub().resolves(true); + ensureMarketSiteStub = sinon.stub().resolves('site-uuid-1'); MockTransportError = class extends Error { constructor(status, message, body) { super(message); @@ -218,6 +221,9 @@ describe('SerenityController', () => { getBrandUrlSources: getBrandUrlSourcesStub, getBrandCompetitors: getBrandCompetitorsStub, }, + '../../src/support/serenity/site-linkage.js': { + ensureMarketSite: ensureMarketSiteStub, + }, })).default; }); @@ -859,6 +865,32 @@ describe('SerenityController', () => { expect(args[2]).to.equal(WORKSPACE); // parentWorkspaceId }); + it('createMarket mirrors the new market as a Site (+ brand_sites link) on 201', async () => { + handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: { brandId: BRAND, geoTargetId: 2840, languageCode: 'en' } }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.createMarket(fakeContext({ + data: { + market: 'us', languageCode: 'en', brandDomain: 'x.com', brandNames: ['X'], + }, + })); + expect(response.status).to.equal(201); + expect(ensureMarketSiteStub).to.have.been.calledOnce; + const opts = ensureMarketSiteStub.firstCall.args[1]; + expect(opts).to.include({ organizationId: ORG, brandId: BRAND, domain: 'x.com' }); + }); + + it('createMarket does NOT mirror a Site when the upstream create did not return 201', async () => { + handlers.handleCreateMarketSubworkspace.resolves({ status: 409, body: { error: 'sliceExists' } }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.createMarket(fakeContext({ + data: { + market: 'us', languageCode: 'en', brandDomain: 'x.com', brandNames: ['X'], + }, + })); + expect(response.status).to.equal(409); + expect(ensureMarketSiteStub).to.not.have.been.called; + }); + it('createMarket forwards the brand aliases so the project carries them', async () => { handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: { brandId: BRAND, geoTargetId: 2840, languageCode: 'en' } }); getBrandAliasNamesStub.resolves(['Acme Inc', 'ACME']); @@ -1063,6 +1095,33 @@ describe('SerenityController', () => { expect(brand.save).to.have.been.called; }); + it('activate mirrors the brand domain as a Site once (not per market) when any market goes live', async () => { + handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: {} }); + const brand = makeBrandModel(); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, + data: { brandDomain: 'x.com', brandNames: ['X'], markets: [{ market: 'us', languageCode: 'en' }, { market: 'de', languageCode: 'de' }] }, + })); + expect(response.status).to.equal(200); + // All markets share the brand domain, so exactly one ensure for two markets. + expect(ensureMarketSiteStub).to.have.been.calledOnce; + const opts = ensureMarketSiteStub.firstCall.args[1]; + expect(opts).to.include({ organizationId: ORG, brandId: BRAND, domain: 'x.com' }); + }); + + it('activate does NOT mirror a Site when no market goes live', async () => { + handlers.handleCreateMarketSubworkspace.resolves({ status: 502, body: { error: 'serenityUpstreamError' } }); + const brand = makeBrandModel(); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, + data: { brandDomain: 'x.com', brandNames: ['X'], markets: [{ market: 'us', languageCode: 'en' }] }, + })); + expect(response.status).to.equal(207); + expect(ensureMarketSiteStub).to.not.have.been.called; + }); + it('activate falls back to the stashed pending_semrush_provisioning when the body omits markets + brandDomain', async () => { handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: {} }); const setPendingSemrushProvisioning = sinon.stub(); @@ -1087,6 +1146,10 @@ describe('SerenityController', () => { expect(createBody.languageCode).to.equal('en'); // brandDomain derived from the stashed primary URL (hostname only). expect(createBody.brandDomain).to.equal('acme.com'); + // The site mirror is fed the same resolved domain (not the raw stash URL), + // so the activate path and the create path agree on the base URL. + expect(ensureMarketSiteStub).to.have.been.calledOnce; + expect(ensureMarketSiteStub.firstCall.args[1]).to.include({ domain: 'acme.com' }); // The draft staging data is cleared on success, atomically with the flip. expect(setPendingSemrushProvisioning).to.have.been.calledWith(null); expect(brand.setStatus).to.have.been.calledWith('active'); diff --git a/test/support/brands-storage.test.js b/test/support/brands-storage.test.js index 0376d5f0de..3545cf7e49 100644 --- a/test/support/brands-storage.test.js +++ b/test/support/brands-storage.test.js @@ -284,6 +284,30 @@ describe('brands-storage', () => { expect(result.earnedContent).to.deep.equal([{ name: 'Blog', url: 'https://blog.example.com', regions: [] }]); }); + it('excludes Serenity market-site rows (type=serenity) from urls[] and siteIds', async () => { + // A brand is a shell with no domain of its own; each market has its own + // domain → its own site, linked via a type=serenity brand_sites row. Those + // rows are a pure backend linkage and must not surface in the brand response. + const dbRow = makeBrandRow({ + brand_sites: [ + { + site_id: 'own-site', paths: ['/'], type: 'base', sites: { base_url: 'https://acme.com' }, + }, + { + site_id: 'market-site', paths: ['/'], type: 'serenity', sites: { base_url: 'https://acme.fr' }, + }, + ], + }); + const query = createChainableQuery({ data: dbRow, error: null }); + const result = await getBrandById(ORG_ID, BRAND_ID, { from: sinon.stub().returns(query) }); + + // market-site excluded from siteIds; its domain absent from urls[]. + expect(result.siteIds).to.deep.equal(['own-site']); + const urlValues = result.urls.map((u) => u.value); + expect(urlValues).to.include('https://acme.com'); + expect(urlValues).to.not.include('https://acme.fr'); + }); + it('maps semrush_workspace_id to semrushWorkspaceId (sub-workspace), null when absent', async () => { const subWsRow = makeBrandRow({ semrush_workspace_id: 'ws-sub-123' }); const subWsQuery = createChainableQuery({ data: subWsRow, error: null }); @@ -677,7 +701,9 @@ describe('brands-storage', () => { // Like createTableMockClient, but records rows passed to write methods so // tests can assert exactly what was written. function createCapturingClient(tableMap) { - const calls = { upsert: [], update: [] }; + const calls = { + upsert: [], update: [], delete: [], or: [], + }; const callCounts = {}; const makeQuery = (table) => { const responses = tableMap[table] || [{ data: null, error: null }]; @@ -703,6 +729,18 @@ describe('brands-storage', () => { return new Proxy({}, handler); }; } + if (prop === 'delete') { + return () => { + calls.delete.push({ table }); + return new Proxy({}, handler); + }; + } + if (prop === 'or') { + return (filter) => { + calls.or.push({ table, filter }); + return new Proxy({}, handler); + }; + } return sinon.stub().returns(new Proxy({}, handler)); }, }; @@ -1393,6 +1431,41 @@ describe('brands-storage', () => { ]); }); + it('preserves a Serenity market-site link (type=serenity) through syncBrandSites instead of downgrading it', async () => { + const client = createCapturingClient({ + brands: [ + { data: { id: BRAND_ID, name: 'Test' }, error: null }, + { data: makeBrandRow({ name: 'Test' }), error: null }, + ], + sites: { data: [{ id: 'site-uuid-1', base_url: 'https://adobe.com' }], error: null }, + // The protected-rows SELECT returns this market site; reused for the + // delete/upsert calls too (single-object response). + brand_sites: { data: [{ site_id: 'site-uuid-1' }], error: null }, + }); + + await upsertBrand({ + organizationId: ORG_ID, + // The submitted brand URL resolves to the SAME site as the preserved + // market row, so the re-upsert would otherwise downgrade type to 'base'. + brand: { name: 'Test', urls: [{ value: 'https://adobe.com', type: 'base' }] }, + postgrestClient: client, + }); + + // The delete must spare serenity rows via the type filter — assert the actual + // filter string, otherwise this test would still pass if the carve-out were + // dropped and the delete wiped every row (including the serenity link). + const bsDelete = client.capturedCalls.or.find((c) => c.table === 'brand_sites'); + expect(bsDelete, 'a brand_sites delete filter was issued').to.exist; + expect(bsDelete.filter).to.include('type.neq.serenity'); + expect(bsDelete.filter).to.include('type.is.null'); + + const bsUpsert = client.capturedCalls.upsert.find((c) => c.table === 'brand_sites'); + expect(bsUpsert, 'a brand_sites upsert was issued').to.exist; + const row = (bsUpsert.row || []).find((r) => r.site_id === 'site-uuid-1'); + // type stays 'serenity' (preserved), NOT downgraded to the URL's 'base'. + expect(row.type).to.equal('serenity'); + }); + it('uses first type when multiple URLs share same base with different types', async () => { const fullBrandRow = makeBrandRow({ brand_sites: [{ @@ -1453,7 +1526,12 @@ describe('brands-storage', () => { it('throws when brand_sites delete fails during syncBrandSites', async () => { const postgrestClient = createTableMockClient({ brands: { data: { id: BRAND_ID, name: 'Test' }, error: null }, - brand_sites: { data: null, error: { message: 'delete error' } }, + brand_sites: [ + // call 0 = protected-rows SELECT (succeeds), call 1 = DELETE (fails). The + // SELECT is now prepended, so the error must be on the DELETE specifically. + { data: [], error: null }, + { data: null, error: { message: 'delete error' } }, + ], }); await expect(upsertBrand({ @@ -1463,6 +1541,37 @@ describe('brands-storage', () => { })).to.be.rejectedWith('Failed to sync brand_sites: delete error'); }); + it('proceeds (delete unprotected) when the protected-rows SELECT fails silently', async () => { + // The protected-rows SELECT error is deliberately swallowed: a failed lookup + // must not block the brand edit. protectedSiteIds ends up empty, so the + // re-upserted row carries the URL's type (no serenity preservation) rather + // than throwing. This pins that silent-swallow contract. + const client = createCapturingClient({ + brands: [ + { data: { id: BRAND_ID, name: 'Test' }, error: null }, + { data: makeBrandRow({ name: 'Test' }), error: null }, + ], + sites: { data: [{ id: 'site-uuid-1', base_url: 'https://adobe.com' }], error: null }, + brand_sites: [ + { data: null, error: { message: 'select error' } }, // SELECT fails + { data: null, error: null }, // DELETE succeeds + { data: null, error: null }, // UPSERT succeeds + ], + }); + + await upsertBrand({ + organizationId: ORG_ID, + brand: { name: 'Test', urls: [{ value: 'https://adobe.com', type: 'base' }] }, + postgrestClient: client, + }); + + const bsUpsert = client.capturedCalls.upsert.find((c) => c.table === 'brand_sites'); + expect(bsUpsert, 'a brand_sites upsert was issued').to.exist; + const row = (bsUpsert.row || []).find((r) => r.site_id === 'site-uuid-1'); + // Empty protectedSiteIds → type comes from the URL ('base'), not 'serenity'. + expect(row.type).to.equal('base'); + }); + it('falls back to base URL when URL string is invalid in syncBrandSites', async () => { const fullBrandRow = makeBrandRow({ brand_sites: [{ site_id: 'site-1', paths: [], sites: { base_url: 'not-a-valid-url' } }], diff --git a/test/support/serenity/site-linkage.test.js b/test/support/serenity/site-linkage.test.js new file mode 100644 index 0000000000..9e3e591f32 --- /dev/null +++ b/test/support/serenity/site-linkage.test.js @@ -0,0 +1,173 @@ +/* + * Copyright 2026 Adobe. All rights reserved. + * This file is licensed to you under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. You may obtain a copy + * of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under + * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS + * OF ANY KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ + +import { expect } from 'chai'; +import sinon from 'sinon'; + +import { ensureMarketSite, SERENITY_BRAND_SITE_TYPE } from '../../../src/support/serenity/site-linkage.js'; + +const ORG = 'org-1'; +const BRAND = 'brand-1'; + +function siteModel(id, org = ORG) { + return { getId: () => id, getOrganizationId: () => org }; +} + +describe('serenity site-linkage: ensureMarketSite', () => { + let upsertStub; + let fromStub; + let postgrestClient; + let Site; + let log; + let ctx; + + beforeEach(() => { + upsertStub = sinon.stub().resolves({ error: null }); + fromStub = sinon.stub().returns({ upsert: upsertStub }); + postgrestClient = { from: fromStub }; + Site = { findByBaseURL: sinon.stub(), create: sinon.stub() }; + log = { warn: sinon.spy(), error: sinon.spy() }; + ctx = { dataAccess: { Site, services: { postgrestClient } } }; + }); + + afterEach(() => sinon.restore()); + + it('no-ops (returns null) when domain, organizationId, or brandId is missing', async () => { + expect(await ensureMarketSite(ctx, { organizationId: ORG, brandId: BRAND, domain: '' })).to.equal(null); + expect(await ensureMarketSite(ctx, { organizationId: '', brandId: BRAND, domain: 'x.com' })).to.equal(null); + expect(await ensureMarketSite(ctx, { organizationId: ORG, brandId: '', domain: 'x.com' })).to.equal(null); + expect(await ensureMarketSite(ctx, {})).to.equal(null); + expect(Site.findByBaseURL).to.not.have.been.called; + }); + + it('warns and returns null when Site data-access is unavailable', async () => { + ctx.dataAccess.Site = {}; // no findByBaseURL + const result = await ensureMarketSite(ctx, { + organizationId: ORG, brandId: BRAND, domain: 'acme.com', log, + }); + expect(result).to.equal(null); + expect(log.warn).to.have.been.calledOnce; + }); + + it('links an existing same-org site without creating a new one', async () => { + Site.findByBaseURL.resolves(siteModel('site-9')); + const result = await ensureMarketSite(ctx, { + organizationId: ORG, brandId: BRAND, domain: 'acme.com', updatedBy: 'tester', log, + }); + expect(result).to.equal('site-9'); + expect(Site.findByBaseURL).to.have.been.calledOnceWith('https://acme.com'); + expect(Site.create).to.not.have.been.called; + expect(fromStub).to.have.been.calledOnceWith('brand_sites'); + const [row, opts] = upsertStub.firstCall.args; + expect(row).to.deep.equal({ + organization_id: ORG, + brand_id: BRAND, + site_id: 'site-9', + paths: ['/'], + type: SERENITY_BRAND_SITE_TYPE, + updated_by: 'tester', + }); + expect(SERENITY_BRAND_SITE_TYPE).to.equal('serenity'); + expect(opts).to.deep.equal({ onConflict: 'brand_id,site_id' }); + }); + + it('creates the site (deliveryType other) when none exists, then links it', async () => { + Site.findByBaseURL.resolves(null); + Site.create.resolves(siteModel('site-new')); + const result = await ensureMarketSite(ctx, { + organizationId: ORG, brandId: BRAND, domain: 'acme.com', log, + }); + expect(result).to.equal('site-new'); + expect(Site.create).to.have.been.calledOnceWith({ + baseURL: 'https://acme.com', organizationId: ORG, deliveryType: 'other', + }); + expect(upsertStub).to.have.been.calledOnce; + // default audit actor + expect(upsertStub.firstCall.args[0].updated_by).to.equal('serenity-market'); + }); + + it('returns null (no link) when an existing site belongs to another org', async () => { + Site.findByBaseURL.resolves(siteModel('site-other', 'org-2')); + const result = await ensureMarketSite(ctx, { + organizationId: ORG, brandId: BRAND, domain: 'acme.com', log, + }); + // Site exists but no brand_sites link was written → null, not the site id. + expect(result).to.equal(null); + expect(fromStub).to.not.have.been.called; + expect(log.warn).to.have.been.calledOnce; + }); + + it('treats a brand_sites upsert error as non-fatal and still returns the site id', async () => { + Site.findByBaseURL.resolves(siteModel('site-9')); + upsertStub.resolves({ error: { message: 'conflict' } }); + const result = await ensureMarketSite(ctx, { + organizationId: ORG, brandId: BRAND, domain: 'acme.com', log, + }); + expect(result).to.equal('site-9'); + expect(log.warn).to.have.been.calledOnce; + }); + + it('returns the site id without linking when no postgrest client is available', async () => { + ctx.dataAccess.services = {}; // no postgrestClient + Site.findByBaseURL.resolves(siteModel('site-9')); + const result = await ensureMarketSite(ctx, { + organizationId: ORG, brandId: BRAND, domain: 'acme.com', log, + }); + expect(result).to.equal('site-9'); + }); + + it('swallows a Site.create failure (returns null, logs error)', async () => { + Site.findByBaseURL.resolves(null); + Site.create.rejects(new Error('db down')); + const result = await ensureMarketSite(ctx, { + organizationId: ORG, brandId: BRAND, domain: 'acme.com', log, + }); + expect(result).to.equal(null); + expect(log.error).to.have.been.calledOnce; + }); + + it('swallows a Site.findByBaseURL rejection (returns null, logs error)', async () => { + Site.findByBaseURL.rejects(new Error('timeout')); + const result = await ensureMarketSite(ctx, { + organizationId: ORG, brandId: BRAND, domain: 'acme.com', log, + }); + expect(result).to.equal(null); + expect(log.error).to.have.been.calledOnce; + }); + + it('normalizes a full URL (scheme + path) to the hostname base URL', async () => { + Site.findByBaseURL.resolves(siteModel('site-9')); + const result = await ensureMarketSite(ctx, { + organizationId: ORG, brandId: BRAND, domain: 'https://www.acme.com/markets/fr?x=1', log, + }); + expect(result).to.equal('site-9'); + // Path/scheme/query/www stripped → same base URL as a bare-hostname caller. + expect(Site.findByBaseURL).to.have.been.calledOnceWith('https://acme.com'); + }); + + it('returns null when the domain does not resolve to a hostname', async () => { + const result = await ensureMarketSite(ctx, { + organizationId: ORG, brandId: BRAND, domain: 'http://', log, + }); + expect(result).to.equal(null); + expect(Site.findByBaseURL).to.not.have.been.called; + expect(log.warn).to.have.been.calledOnce; + }); + + it('returns null (does not throw) when ctx is null', async () => { + const result = await ensureMarketSite(null, { + organizationId: ORG, brandId: BRAND, domain: 'acme.com', log, + }); + expect(result).to.equal(null); + expect(log.warn).to.have.been.calledOnce; + }); +}); diff --git a/test/support/serenity/subworkspace-projects.test.js b/test/support/serenity/subworkspace-projects.test.js index bb8bd5e951..bd73a31cc7 100644 --- a/test/support/serenity/subworkspace-projects.test.js +++ b/test/support/serenity/subworkspace-projects.test.js @@ -33,17 +33,20 @@ const WS = 'subworkspace-ws-1'; function project({ id = 'p1', location = 2276, language = 'de', country = null, publishStatus = 'live', createdAt = '2026-06-01T00:00:00Z', updatedAt = '2026-06-02T00:00:00Z', + domain = 'example.com', } = {}) { // Mirrors the live v1 list item shape: nested location/language objects, // updated_at present, created_at usually absent (passed here for mapping // assertions; the no-created_at path is covered separately). `country` mirrors // the Semrush-UI shape where settings.ai.location.id is null but a country is - // set (settings.ai.country.code); omitted by default. + // set (settings.ai.country.code); omitted by default. `domain` is the project's + // top-level primary host (null to mirror a project that carries none). return { id, publish_status: publishStatus, created_at: createdAt, updated_at: updatedAt, + ...(domain === null ? {} : { domain }), settings: { ai: { location: location === null ? null : { id: location, name: 'X' }, @@ -82,8 +85,13 @@ describe('subworkspace-projects', () => { updatedAt: '2026-06-02T00:00:00Z', status: 'live', semrushProjectId: 'p1', + domain: 'example.com', }); }); + it('surfaces the project domain, and nulls it when the project carries none', () => { + expect(projectToSlice(project({ domain: 'acme.com' }), BRAND).domain).to.equal('acme.com'); + expect(projectToSlice(project({ domain: null }), BRAND).domain).to.equal(null); + }); it('lowercases the language and nulls an invalid geo', () => { const s = projectToSlice(project({ location: 'x', language: 'EN' }), BRAND); expect(s.geoTargetId).to.equal(null); From a97155ce78a8a5104dae3bb124a18e2289861be1 Mon Sep 17 00:00:00 2001 From: Rainer Friederich Date: Fri, 19 Jun 2026 11:24:53 +0200 Subject: [PATCH 2/6] fix(serenity): keep the market-site mirror from 500-ing a live activation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The activate/createMarket mirror hooks evaluated brand.getOrganizationId() as a call argument, OUTSIDE ensureMarketSite's best-effort try/catch. A missing or throwing accessor therefore 500'd the operation AFTER the Semrush markets were already live upstream — the opposite of the documented best-effort contract. Optional-chain the accessor so the mirror is skipped (not fatal) in that case. Also gives the openapi-contract fake Brand the getOrganizationId accessor the real model exposes (activateSerenityBrand was returning 500 instead of 200). Co-Authored-By: Claude Opus 4.8 --- src/controllers/serenity.js | 8 ++++++-- test/openapi-contract/serenity-api.test.js | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/controllers/serenity.js b/src/controllers/serenity.js index 248dfa2d03..ab9f925cb7 100644 --- a/src/controllers/serenity.js +++ b/src/controllers/serenity.js @@ -547,7 +547,9 @@ function SerenityController(context, log, env) { // never fails a live market. if (result?.status === 201) { await ensureMarketSite(ctx, { - organizationId: brand.getOrganizationId(), + // Optional-chained so a missing/throwing accessor can't 500 a market + // that is already live upstream — the mirror is best-effort. + organizationId: brand.getOrganizationId?.(), brandId: auth.brandUuid, domain: ctx.data?.brandDomain, updatedBy: 'serenity-create-market', @@ -963,7 +965,9 @@ function SerenityController(context, log, env) { // Best-effort: never fails the activation. if (anyLive) { await ensureMarketSite(ctx, { - organizationId: brand.getOrganizationId(), + // Optional-chained so a missing/throwing accessor can't 500 an + // activation whose markets are already live upstream — best-effort. + organizationId: brand.getOrganizationId?.(), brandId: auth.brandUuid, domain: brandDomain, updatedBy: 'serenity-activate', diff --git a/test/openapi-contract/serenity-api.test.js b/test/openapi-contract/serenity-api.test.js index 2a317888b1..a6f25cf1f2 100644 --- a/test/openapi-contract/serenity-api.test.js +++ b/test/openapi-contract/serenity-api.test.js @@ -46,6 +46,7 @@ function fakeContext({ params = {}, data = undefined, query = {} } = {}) { findById: sinon.stub().resolves({ getId: () => BRAND, getName: () => 'Test Brand', + getOrganizationId: () => ORG, getSemrushWorkspaceId: () => null, setSemrushWorkspaceId: sinon.stub(), setStatus: sinon.stub(), From fc2943dd387c7cf53eb27785efb06a7410bdc443 Mon Sep 17 00:00:00 2001 From: Rainer Friederich Date: Fri, 19 Jun 2026 11:39:23 +0200 Subject: [PATCH 3/6] fix(serenity): address MysticatBot blocking review on #2643 - mapDbBrandToV2: surface a serenity (market-mirror) brand_sites row when the brand ALSO lists that domain as a brand URL. Previously the serenity exclusion silently flipped such a brand URL to onboarded:false and dropped it from siteIds the moment a market was created for the same domain. Pure market domains (not in brand_urls) stay excluded. - syncBrandSites: throw on the protected-rows SELECT error instead of swallowing it. A swallowed error left protectedSiteIds empty and let the re-upsert downgrade a serenity row's type, silently unprotecting a market-mirror link. Fail closed, consistent with the existing delete/upsert error handling. Co-Authored-By: Claude Opus 4.8 --- src/support/brands-storage.js | 27 ++++++++++++- test/support/brands-storage.test.js | 62 +++++++++++++++++------------ 2 files changed, 61 insertions(+), 28 deletions(-) diff --git a/src/support/brands-storage.js b/src/support/brands-storage.js index 26734b192e..e5c653a653 100644 --- a/src/support/brands-storage.js +++ b/src/support/brands-storage.js @@ -117,12 +117,27 @@ function parseUrlParts(urlString) { * `brand_sites` expansion, where every entry is by definition onboarded. */ function mapDbBrandToV2(row) { + // The set of base URLs the brand explicitly lists as its own (brand_urls). + const brandUrlBases = new Set( + (row.brand_urls || []) + .map((bu) => composeBaseURL(parseUrlParts(bu.url).base)) + .filter(hasText), + ); + // Exclude Semrush market-site rows from the brand response: a market's domain // is NOT a brand URL (the brand is a shell with no domain of its own), so these // rows must not surface in urls[] or siteIds. They are a pure backend linkage — // integrations resolve them via the sites / brand_sites tables directly. + // + // Exception: when the brand ALSO lists that exact domain as a brand URL, it IS a + // brand URL (not just a hidden market mirror) and must keep its onboarded/siteId + // status in the response. syncBrandSites collapses such an overlap into a single + // serenity-typed row (one row per (brand, site)); surfacing it here is what keeps + // a brand URL from silently flipping to onboarded:false the moment a market is + // created for the same domain. const ownBrandSites = (row.brand_sites || []) - .filter((bs) => bs.type !== SERENITY_BRAND_SITE_TYPE); + .filter((bs) => bs.type !== SERENITY_BRAND_SITE_TYPE + || (hasText(bs.sites?.base_url) && brandUrlBases.has(composeBaseURL(bs.sites.base_url)))); const siteIds = ownBrandSites.map((bs) => bs.site_id).filter(hasText); @@ -275,11 +290,19 @@ async function syncBrandSites(organizationId, brandId, urls, postgrestClient, up // simultaneous brand edit + market write whose domains collide — by design // unusual — and self-heals on the next market write (ensureMarketSite re-upserts // type='serenity'). Not worth a cross-request lock PostgREST can't cheaply give. - const { data: protectedRows } = await postgrestClient + const { data: protectedRows, error: protectedError } = await postgrestClient .from('brand_sites') .select('site_id') .eq('brand_id', brandId) .eq('type', SERENITY_BRAND_SITE_TYPE); + // Fail closed (consistent with the delete/upsert error handling below): a + // swallowed SELECT error would leave protectedSiteIds empty, and the re-upsert + // would then downgrade a serenity row to the brand URL's type — silently + // unprotecting a market-mirror link. A failed brand edit is recoverable; a + // corrupted serenity marker surfaces later as a vanished market site. + if (protectedError) { + throw new Error(`Failed to sync brand_sites: cannot read protected rows: ${protectedError.message}`); + } const protectedSiteIds = new Set((protectedRows || []).map((r) => r.site_id)); const { error: deleteError } = await postgrestClient diff --git a/test/support/brands-storage.test.js b/test/support/brands-storage.test.js index 3545cf7e49..8046e7084f 100644 --- a/test/support/brands-storage.test.js +++ b/test/support/brands-storage.test.js @@ -308,6 +308,31 @@ describe('brands-storage', () => { expect(urlValues).to.not.include('https://acme.fr'); }); + it('surfaces a Serenity row when its domain is ALSO an explicit brand URL', async () => { + // A market domain that the brand also lists as a brand URL IS a brand URL — + // it must keep onboarded/siteId status rather than vanishing behind the + // serenity exclusion. syncBrandSites collapses the overlap into one + // serenity-typed row (one per (brand, site)); mapDbBrandToV2 surfaces it + // because brand_urls references the same base. + const dbRow = makeBrandRow({ + brand_urls: [{ url: 'https://acme.com' }], + brand_sites: [ + { + site_id: 'shared-site', paths: ['/'], type: 'serenity', sites: { base_url: 'https://acme.com' }, + }, + ], + }); + const query = createChainableQuery({ data: dbRow, error: null }); + const result = await getBrandById(ORG_ID, BRAND_ID, { from: sinon.stub().returns(query) }); + + // The brand URL stays onboarded and the site appears in siteIds. + expect(result.siteIds).to.deep.equal(['shared-site']); + const acme = result.urls.find((u) => u.value === 'https://acme.com'); + expect(acme).to.exist; + expect(acme.onboarded).to.equal(true); + expect(acme.siteId).to.equal('shared-site'); + }); + it('maps semrush_workspace_id to semrushWorkspaceId (sub-workspace), null when absent', async () => { const subWsRow = makeBrandRow({ semrush_workspace_id: 'ws-sub-123' }); const subWsQuery = createChainableQuery({ data: subWsRow, error: null }); @@ -1541,35 +1566,20 @@ describe('brands-storage', () => { })).to.be.rejectedWith('Failed to sync brand_sites: delete error'); }); - it('proceeds (delete unprotected) when the protected-rows SELECT fails silently', async () => { - // The protected-rows SELECT error is deliberately swallowed: a failed lookup - // must not block the brand edit. protectedSiteIds ends up empty, so the - // re-upserted row carries the URL's type (no serenity preservation) rather - // than throwing. This pins that silent-swallow contract. - const client = createCapturingClient({ - brands: [ - { data: { id: BRAND_ID, name: 'Test' }, error: null }, - { data: makeBrandRow({ name: 'Test' }), error: null }, - ], - sites: { data: [{ id: 'site-uuid-1', base_url: 'https://adobe.com' }], error: null }, - brand_sites: [ - { data: null, error: { message: 'select error' } }, // SELECT fails - { data: null, error: null }, // DELETE succeeds - { data: null, error: null }, // UPSERT succeeds - ], + it('throws (fails closed) when the protected-rows SELECT fails during syncBrandSites', async () => { + // A swallowed SELECT error would leave protectedSiteIds empty and let the + // re-upsert downgrade a serenity row's type, silently unprotecting a + // market-mirror link. Fail closed instead (consistent with delete/upsert). + const postgrestClient = createTableMockClient({ + brands: { data: { id: BRAND_ID, name: 'Test' }, error: null }, + brand_sites: { data: null, error: { message: 'select error' } }, // SELECT fails first }); - await upsertBrand({ + await expect(upsertBrand({ organizationId: ORG_ID, - brand: { name: 'Test', urls: [{ value: 'https://adobe.com', type: 'base' }] }, - postgrestClient: client, - }); - - const bsUpsert = client.capturedCalls.upsert.find((c) => c.table === 'brand_sites'); - expect(bsUpsert, 'a brand_sites upsert was issued').to.exist; - const row = (bsUpsert.row || []).find((r) => r.site_id === 'site-uuid-1'); - // Empty protectedSiteIds → type comes from the URL ('base'), not 'serenity'. - expect(row.type).to.equal('base'); + brand: { name: 'Test', urls: [{ value: 'https://test.com' }] }, + postgrestClient, + })).to.be.rejectedWith('Failed to sync brand_sites: cannot read protected rows: select error'); }); it('falls back to base URL when URL string is invalid in syncBrandSites', async () => { From 27b02fef5a0c432a7432555273148c889d39267e Mon Sep 17 00:00:00 2001 From: Rainer Friederich Date: Fri, 19 Jun 2026 11:57:34 +0200 Subject: [PATCH 4/6] refactor(serenity): tighten ensureMarketSite return contract + document invariant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses MysticatBot's two non-blocking nits on #2643 (already Approved): - ensureMarketSite now returns the site id ONLY when the brand_sites link is established; null otherwise (no client, or a failed link write — previously it returned the siteId, contradicting the JSDoc and the cross-org path). No caller depends on the value; this just makes the contract honest and consistent. - brands.js: explicit INVARIANT comment that the ensureMarketSite call must not throw (its catch releases the just-provisioned workspace). Co-Authored-By: Claude Opus 4.8 --- src/controllers/brands.js | 6 ++- src/support/serenity/site-linkage.js | 50 +++++++++++++--------- test/support/serenity/site-linkage.test.js | 10 +++-- 3 files changed, 40 insertions(+), 26 deletions(-) diff --git a/src/controllers/brands.js b/src/controllers/brands.js index e10e828c6f..1a4d88fe89 100644 --- a/src/controllers/brands.js +++ b/src/controllers/brands.js @@ -1480,8 +1480,10 @@ function BrandsController(ctx, log, env) { // When a Semrush sub-workspace + initial market were provisioned, mirror that // initial market as a SpaceCat Site (+ brand_sites link) keyed on the // market's domain, so the Semrush project has a resolvable site entity. - // Best-effort: never throws (a throw here would trip the workspace-release - // compensation below for a live brand). + // INVARIANT: ensureMarketSite MUST NOT throw — it sits inside the try/catch + // whose catch releases the just-provisioned workspace; a throw here would + // tear down a live brand's workspace. ensureMarketSite is best-effort by + // contract (its own catch-all swallows + logs), so this holds. if (hasText(provisionedWorkspaceId)) { await ensureMarketSite(context, { organizationId: spaceCatId, diff --git a/src/support/serenity/site-linkage.js b/src/support/serenity/site-linkage.js index 6601df776b..b660b1b43f 100644 --- a/src/support/serenity/site-linkage.js +++ b/src/support/serenity/site-linkage.js @@ -59,8 +59,10 @@ export const SERENITY_BRAND_SITE_TYPE = 'serenity'; * same base URL as the brand-create path. * @param {string} [opts.updatedBy] - audit actor for the brand_sites row. * @param {object} [opts.log] - logger. - * @returns {Promise} the linked site id, or null when skipped/failed - * (including the cross-org case, where the site exists but is NOT linked). + * @returns {Promise} the site id ONLY when the brand_sites link was + * established; null otherwise — bad input, data-access unavailable, cross-org, + * no postgrest client, or a failed link write (the site may exist in those + * cases, but a non-null return always means "linked"). */ export async function ensureMarketSite(ctx, { organizationId, @@ -118,24 +120,32 @@ export async function ensureMarketSite(ctx, { return null; } - if (postgrestClient?.from) { - const { error } = await postgrestClient - .from('brand_sites') - .upsert({ - organization_id: organizationId, - brand_id: brandId, - site_id: siteId, - paths: ['/'], - type: SERENITY_BRAND_SITE_TYPE, - updated_by: updatedBy, - }, { onConflict: 'brand_id,site_id' }); - if (error) { - // Non-fatal: the Site exists; the link can be re-ensured on a later - // market write. The marker keeps it safe from syncBrandSites once set. - log?.warn?.('ensureMarketSite: brand_sites link upsert failed (non-fatal)', { - brandId, siteId, error: error.message, - }); - } + if (!postgrestClient?.from) { + // Site ensured, but no client to write the brand_sites link → not linked. + log?.warn?.('ensureMarketSite: postgrest client unavailable; site ensured but not linked', { + brandId, siteId, domain, + }); + return null; + } + + const { error } = await postgrestClient + .from('brand_sites') + .upsert({ + organization_id: organizationId, + brand_id: brandId, + site_id: siteId, + paths: ['/'], + type: SERENITY_BRAND_SITE_TYPE, + updated_by: updatedBy, + }, { onConflict: 'brand_id,site_id' }); + if (error) { + // Non-fatal: the Site exists and the link can be re-ensured on a later + // market write. Return null (not siteId) — the link was NOT established, so + // a caller must not read a non-null return as a successful mirror. + log?.warn?.('ensureMarketSite: brand_sites link upsert failed (non-fatal)', { + brandId, siteId, error: error.message, + }); + return null; } return siteId; } catch (e) { diff --git a/test/support/serenity/site-linkage.test.js b/test/support/serenity/site-linkage.test.js index 9e3e591f32..d079e2a29f 100644 --- a/test/support/serenity/site-linkage.test.js +++ b/test/support/serenity/site-linkage.test.js @@ -106,23 +106,25 @@ describe('serenity site-linkage: ensureMarketSite', () => { expect(log.warn).to.have.been.calledOnce; }); - it('treats a brand_sites upsert error as non-fatal and still returns the site id', async () => { + it('treats a brand_sites upsert error as non-fatal but returns null (link not established)', async () => { Site.findByBaseURL.resolves(siteModel('site-9')); upsertStub.resolves({ error: { message: 'conflict' } }); const result = await ensureMarketSite(ctx, { organizationId: ORG, brandId: BRAND, domain: 'acme.com', log, }); - expect(result).to.equal('site-9'); + // Non-null means "linked"; a failed link write returns null. + expect(result).to.equal(null); expect(log.warn).to.have.been.calledOnce; }); - it('returns the site id without linking when no postgrest client is available', async () => { + it('returns null (site ensured, not linked) when no postgrest client is available', async () => { ctx.dataAccess.services = {}; // no postgrestClient Site.findByBaseURL.resolves(siteModel('site-9')); const result = await ensureMarketSite(ctx, { organizationId: ORG, brandId: BRAND, domain: 'acme.com', log, }); - expect(result).to.equal('site-9'); + expect(result).to.equal(null); + expect(log.warn).to.have.been.calledOnce; }); it('swallows a Site.create failure (returns null, logs error)', async () => { From a4436387bd59bdd94d0208ddd0327824849751ab Mon Sep 17 00:00:00 2001 From: Rainer Friederich Date: Mon, 22 Jun 2026 08:35:03 +0200 Subject: [PATCH 5/6] test(it): wire the held Serenity market-mirror seed (data-service v5.44.0) The IT harness previously pinned data-service v5.41.0, which predates the brand_sites_type_check migration that allows type='serenity'. Now that v5.44.0 ships that migration, unblock the held seed + assertions: - bump the IT data-service image v5.41.0 -> v5.44.0 - add a delivery_type='other' market-mirror Site (MARKET_SITE_1, ORG_1) and a type='serenity' brand_sites row linking it to BRAND_1 (seed-data/brand-sites.js, registered at seed Level 3 after brands + sites) - assert GET brand (single + list) EXCLUDES the serenity site from urls[]/siteIds - repoint the by-delivery-type "unmatched" case to aem_ams (other is now used) and add positive coverage that delivery_type=other returns the market-mirror site Full postgres IT suite green against v5.44.0 (547 passing). Co-Authored-By: Claude Opus 4.8 --- test/it/postgres/docker-compose.yml | 2 +- test/it/postgres/seed-data/brand-sites.js | 34 ++++++++++++++++++++++ test/it/postgres/seed-data/sites.js | 13 +++++++++ test/it/postgres/seed.js | 4 ++- test/it/shared/seed-ids.js | 6 ++++ test/it/shared/tests/brands.js | 35 ++++++++++++++++++++++- test/it/shared/tests/sites.js | 15 +++++++++- 7 files changed, 105 insertions(+), 4 deletions(-) create mode 100644 test/it/postgres/seed-data/brand-sites.js diff --git a/test/it/postgres/docker-compose.yml b/test/it/postgres/docker-compose.yml index a77eedb445..5eaedad9ef 100644 --- a/test/it/postgres/docker-compose.yml +++ b/test/it/postgres/docker-compose.yml @@ -41,7 +41,7 @@ services: retries: 10 data-service: - image: 682033462621.dkr.ecr.us-east-1.amazonaws.com/mysticat-data-service:v5.41.0 + image: 682033462621.dkr.ecr.us-east-1.amazonaws.com/mysticat-data-service:v5.44.0 container_name: spacecat-it-data-service depends_on: db: diff --git a/test/it/postgres/seed-data/brand-sites.js b/test/it/postgres/seed-data/brand-sites.js new file mode 100644 index 0000000000..86a4a64169 --- /dev/null +++ b/test/it/postgres/seed-data/brand-sites.js @@ -0,0 +1,34 @@ +/* + * Copyright 2026 Adobe. All rights reserved. + * This file is licensed to you under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. You may obtain a copy + * of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under + * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS + * OF ANY KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ + +/** + * Baseline brand_sites for IT tests. + * + * One row: BRAND_1 (ORG_1) linked to the Serenity market-mirror Site via a + * `brand_sites` row tagged type='serenity'. This is a pure backend linkage — + * the market's domain is NOT a brand URL (the brand is a shell with no domain + * of its own), so the GET-brand response must EXCLUDE it from urls[]/siteIds + * (see mapDbBrandToV2). The type='serenity' value requires the brand_sites + * CHECK constraint extended by mysticat-data-service migration in v5.44.0. + * + * Format: snake_case (PostgreSQL / PostgREST) + */ +export const brandSites = [ + { + organization_id: '11111111-1111-4111-b111-111111111111', + brand_id: 'ab111111-1111-4111-b111-111111111111', + site_id: '5e111111-1111-4111-b111-1111111111fe', + paths: ['/'], + type: 'serenity', + updated_by: 'seed', + }, +]; diff --git a/test/it/postgres/seed-data/sites.js b/test/it/postgres/seed-data/sites.js index 4481343c1c..1f3205f248 100644 --- a/test/it/postgres/seed-data/sites.js +++ b/test/it/postgres/seed-data/sites.js @@ -80,6 +80,19 @@ export const sites = [ is_live: true, name: 'Site Four (Delegate)', }, + // Serenity market-mirror Site (ORG_1): mirrors a Semrush market domain. + // delivery_type 'other' — a Semrush-managed market site is not an AEM target + // (matches ensureMarketSite). Linked to BRAND_1 via a type='serenity' + // brand_sites row (see seed-data/brand-sites.js) which must NOT surface in + // the brand's urls[] / siteIds. + { + id: '5e111111-1111-4111-b111-1111111111fe', + base_url: 'https://semrush-market.example.fr', + organization_id: '11111111-1111-4111-b111-111111111111', + delivery_type: 'other', + is_live: true, + name: 'Semrush Market Mirror (FR)', + }, // TEMPORARY: LLMO mode-resolution test sites — remove with resolveLlmoOnboardingMode legacy check { id: 'fd111111-1111-4111-b111-111111111111', diff --git a/test/it/postgres/seed.js b/test/it/postgres/seed.js index 8fda56a9e2..7ee63420a8 100644 --- a/test/it/postgres/seed.js +++ b/test/it/postgres/seed.js @@ -34,6 +34,7 @@ import { consumers } from './seed-data/consumers.js'; import { plgOnboardings } from './seed-data/plg-onboardings.js'; import { siteImsOrgAccesses } from './seed-data/site-ims-org-accesses.js'; import { brands } from './seed-data/brands.js'; +import { brandSites } from './seed-data/brand-sites.js'; import { projectionAudits } from './seed-data/projection-audits.js'; const POSTGREST_PORT = process.env.IT_POSTGREST_PORT || '3300'; @@ -135,12 +136,13 @@ async function seed() { insertRows('sentiment_topics', sentimentTopics), ]); - // Level 3: depend on opportunities, audits, topics + // Level 3: depend on opportunities, audits, topics, brands + sites await Promise.all([ insertRows('suggestions', suggestions), insertRows('fix_entities', fixes), insertRows('audit_urls', auditUrls), insertRows('sentiment_guidelines', sentimentGuidelines), + insertRows('brand_sites', brandSites), ]); // Level 4: depend on fix_entities + suggestions diff --git a/test/it/shared/seed-ids.js b/test/it/shared/seed-ids.js index 32c53474ba..8991f114f1 100644 --- a/test/it/shared/seed-ids.js +++ b/test/it/shared/seed-ids.js @@ -37,6 +37,12 @@ export const SITE_2_BASE_URL = 'https://site2.example.com'; export const SITE_3_ID = '55555555-5555-4555-9555-555555555555'; export const SITE_3_BASE_URL = 'https://site3-denied.example.com'; +// Serenity market-mirror Site (ORG_1): a Site that mirrors a Semrush market, +// linked to BRAND_1 via a `brand_sites` row tagged type='serenity'. It is a +// pure backend linkage — it must NOT surface in the brand's urls[] / siteIds. +export const MARKET_SITE_1_ID = '5e111111-1111-4111-b111-1111111111fe'; +export const MARKET_SITE_1_BASE_URL = 'https://semrush-market.example.fr'; + // ── Brands ── export const BRAND_1_ID = 'ab111111-1111-4111-b111-111111111111'; // ORG_1, "Test Brand" diff --git a/test/it/shared/tests/brands.js b/test/it/shared/tests/brands.js index d334eb785d..66667e3d77 100644 --- a/test/it/shared/tests/brands.js +++ b/test/it/shared/tests/brands.js @@ -11,7 +11,12 @@ */ import { expect } from 'chai'; -import { ORG_1_ID } from '../seed-ids.js'; +import { + ORG_1_ID, + BRAND_1_ID, + MARKET_SITE_1_ID, + MARKET_SITE_1_BASE_URL, +} from '../seed-ids.js'; export default function brandsTests(getHttpClient, resetData) { describe('Brands v2 claims guidance', () => { @@ -84,4 +89,32 @@ export default function brandsTests(getHttpClient, resetData) { expect(tooLong.status).to.equal(400); }); }); + + describe('Brands v2 Serenity market-mirror linkage', () => { + before(() => resetData()); + + it('excludes the Serenity market-mirror site from the brand urls[] and siteIds', async () => { + const http = getHttpClient(); + + // BRAND_1 is linked to the market-mirror Site (MARKET_SITE_1) via a + // brand_sites row tagged type='serenity'. The market's domain is NOT a + // brand URL (the brand is a shell with no domain of its own), so the row + // is a pure backend linkage and must not surface in the brand response. + const getRes = await http.admin.get(`/v2/orgs/${ORG_1_ID}/brands/${BRAND_1_ID}`); + expect(getRes.status).to.equal(200); + + expect(getRes.body.siteIds || []).to.not.include(MARKET_SITE_1_ID); + const urlValues = (getRes.body.urls || []).map((u) => u.value); + expect(urlValues).to.not.include(MARKET_SITE_1_BASE_URL); + + // The same exclusion must hold on the list endpoint. + const listRes = await http.admin.get(`/v2/orgs/${ORG_1_ID}/brands`); + expect(listRes.status).to.equal(200); + const listed = listRes.body.brands.find((brand) => brand.id === BRAND_1_ID); + expect(listed).to.be.an('object'); + expect(listed.siteIds || []).to.not.include(MARKET_SITE_1_ID); + const listedUrlValues = (listed.urls || []).map((u) => u.value); + expect(listedUrlValues).to.not.include(MARKET_SITE_1_BASE_URL); + }); + }); } diff --git a/test/it/shared/tests/sites.js b/test/it/shared/tests/sites.js index f681f6df91..37aecafc24 100644 --- a/test/it/shared/tests/sites.js +++ b/test/it/shared/tests/sites.js @@ -24,6 +24,8 @@ import { SITE_4_BASE_URL, SITE_LEGACY_LLMO_ID, SITE_NEW_LLMO_ID, + MARKET_SITE_1_ID, + MARKET_SITE_1_BASE_URL, NON_EXISTENT_SITE_ID, PROJECT_1_ID, PROJECT_2_ID, @@ -375,10 +377,21 @@ export default function siteTests(getHttpClient, resetData) { }); }); - it('admin: returns empty array for unmatched delivery type', async () => { + it('admin: returns the Serenity market-mirror site for delivery type other', async () => { const http = getHttpClient(); const res = await http.admin.get('/sites/by-delivery-type/other'); expect(res.status).to.equal(200); + // MARKET_SITE_1 (Semrush market mirror) is the only delivery_type=other site. + expect(res.body).to.be.an('array').with.lengthOf(1); + expect(res.body[0].id).to.equal(MARKET_SITE_1_ID); + expect(res.body[0].baseURL).to.equal(MARKET_SITE_1_BASE_URL); + expect(res.body[0].deliveryType).to.equal('other'); + }); + + it('admin: returns empty array for unmatched delivery type', async () => { + const http = getHttpClient(); + const res = await http.admin.get('/sites/by-delivery-type/aem_ams'); + expect(res.status).to.equal(200); expect(res.body).to.be.an('array').with.lengthOf(0); }); From 20a0b4a5c22656f6a6f783ea20482c4f701eaa82 Mon Sep 17 00:00:00 2001 From: Rainer Friederich Date: Mon, 22 Jun 2026 14:51:20 +0200 Subject: [PATCH 6/6] feat(serenity): optional prompt generation + all-or-nothing activation hardening Adds a generatePrompts flag (default false) that gates prompt/topic generation independently of project creation, and finalizes the all-or-nothing sub-workspace activation flow. - generatePrompts maps to handleCreateMarketSubworkspace's generateTopics. Project creation is gated on primary-URL presence (US/EN fallback when no market is supplied), NOT on the flag. No URL -> sub-workspace-only activation (bare brand anchored by its sub-workspace). generatePrompts requires a URL (400 otherwise). Models optional when not generating prompts; empty-units projects publish best-effort. Flag rides the pending_semrush_provisioning JSONB - no migration. - activate stays all-or-nothing: a brand flips active only when the sub-workspace, every market, the brand<->workspace link, and every brand_sites serenity mirror succeed; any failure keeps a pending brand pending with a 502. - fix: per-market modelIds were placed in the body arg on activation but read from the options arg, so stashed LLMs were silently dropped. Moved to the options arg. - test: stub ensureMarketSite in the serenity OpenAPI-contract fixture (it became a required activation gate). Co-Authored-By: Claude Opus 4.8 --- docs/openapi/schemas.yaml | 111 +++++- docs/openapi/serenity-api.yaml | 39 +- src/controllers/brands.js | 96 ++++- src/controllers/serenity.js | 347 ++++++++++++------ src/support/brands-storage.js | 50 ++- src/support/serenity/brand-provisioning.js | 38 +- test/controllers/brands.test.js | 166 ++++++++- test/controllers/serenity.test.js | 326 +++++++++++++--- test/openapi-contract/serenity-api.test.js | 6 + test/support/brands-storage.test.js | 67 ++++ .../serenity/brand-provisioning.test.js | 27 +- 11 files changed, 1034 insertions(+), 239 deletions(-) diff --git a/docs/openapi/schemas.yaml b/docs/openapi/schemas.yaml index 995769a601..9863ec1df5 100644 --- a/docs/openapi/schemas.yaml +++ b/docs/openapi/schemas.yaml @@ -3849,6 +3849,13 @@ Brand: type: string languageCode: type: string + modelIds: + type: array + items: + type: string + description: >- + AI model (LLM) ids to attach to this market's project at + activation. Optional; omitted when none were chosen. imsOrgId: description: The IMS Organization ID of the brand $ref: '#/ImsOrganizationId' @@ -7388,6 +7395,47 @@ V2BrandCompetitor: description: ISO-3166 country codes example: ['US'] +V2PendingSemrushProvisioning: + type: [object, 'null'] + description: >- + Deferred Semrush provisioning data for a pending (draft) brand: the primary + URL + initial markets (each with its chosen AI models/LLMs) collected at + "Save as pending" before the brand's sub-workspace + project exist, plus + whether activation should generate topics/prompts. Activation provisions each + market and removes it from the blob, nulling the field once none remain + (failed markets stay for retry). markets may be empty: with a primaryUrl a + single US/EN fallback project is provisioned; with neither a primaryUrl nor a + market a sub-workspace-only brand is provisioned. On a brand response this is + read-only and null for a non-pending brand. On a PATCH it is honored only + while the brand is (and stays) pending — the draft Markets tab appends a + market / edits a market's LLMs — and ignored otherwise. + properties: + primaryUrl: + type: [string, 'null'] + generatePrompts: + type: boolean + description: >- + Whether activation generates topics/prompts for the provisioned + project(s). Defaults to false (project created empty). Requires a + primaryUrl (and thus a project) when true. + markets: + type: array + items: + type: object + required: + - market + - languageCode + properties: + market: + type: string + languageCode: + type: string + modelIds: + type: array + items: + type: string + description: AI model (LLM) ids to attach to this market at activation. + V2Brand: type: object description: A brand managed via the v2 PostgREST-backed API @@ -7463,6 +7511,13 @@ V2Brand: type: string format: uuid description: IDs of sites linked to this brand (read-only, derived from brand_sites) + pendingSemrushProvisioning: + allOf: + - $ref: '#/V2PendingSemrushProvisioning' + readOnly: true + description: >- + Read-only deferred Semrush provisioning data for a pending (draft) + brand. Null for a non-pending brand. See V2PendingSemrushProvisioning. updatedAt: $ref: '#/DateTime' updatedBy: @@ -7624,6 +7679,13 @@ V2BrandUpdateInput: type: array items: $ref: '#/V2BrandCompetitor' + pendingSemrushProvisioning: + allOf: + - $ref: '#/V2PendingSemrushProvisioning' + description: >- + Deferred Semrush provisioning data. Honored only while the brand is (and + stays) pending — the draft Markets tab appends a market / edits a + market's LLMs; silently ignored on a non-pending brand. V2BrandListResponse: type: object @@ -10717,10 +10779,22 @@ SerenityActivateRequest: Activates a brand into sub-workspace mode: ensures the brand's own Semrush sub-workspace exists (creating + resourcing it on first call), then per supplied market creates-or-resumes a draft and publishes it once. - Markets are caller-supplied — reactivation re-supplies them (there is no - stored market memory). Sets the brand status to `active` once ≥1 market is live. - required: [brandDomain, brandNames, markets] + + All fields are optional. A pending (draft) brand activated from the wizard + sends an EMPTY body `{}` and every value is read from its stashed + `pending_semrush_provisioning` (primary URL, markets, generatePrompts). A + reactivation of an already-live brand re-supplies them in the body (the body + wins when present; there is no stored market memory otherwise). When no + primary URL/domain is available at all, a sub-workspace-only brand is + activated (no project). With a domain but no market, a single US/EN fallback + project is provisioned. properties: + generatePrompts: + type: boolean + description: >- + Whether to generate topics/prompts for the provisioned project(s). + Overrides the stashed value; defaults to the stash (else false). Requires + a primary URL/domain (a project) — true with no domain is rejected (400). brandDomain: type: string description: Primary domain for the brand's upstream projects. @@ -10754,6 +10828,15 @@ SerenityActivateRequest: name: type: string description: Optional display name for this market's upstream project. + modelIds: + type: array + items: + type: string + description: >- + Optional AI model (LLM) ids to attach to this market's project. + When omitted here, the markets stashed on the pending brand supply + them (a draft activated from the wizard carries per-market LLMs in + pending_semrush_provisioning). SerenityActivateResponse: type: object @@ -10766,8 +10849,24 @@ SerenityActivateResponse: type: string enum: [active, pending] description: | - `active` when at least one market went live; `pending` when none did - (the response is HTTP 207 in that case). Mirrors the persisted brand status. + All-or-nothing. `active` (HTTP 200) only when the FULL provisioning chain + succeeded — sub-workspace ensured, every market's project live, the brand + linked to its sub-workspace, AND every project mirrored as a Site + (`brand_sites` type=`serenity`). Otherwise a pending (draft) brand stays + `pending` and the response is HTTP 502 (incomplete activation; body carries + `error`/`message` and the per-market outcomes — retry to converge). Mirrors + the persisted brand status. + error: + type: string + description: | + Present only on an incomplete activation (HTTP 502, `status: pending`): a + stable token identifying the failure class (e.g. `serenityActivationIncomplete`). + message: + type: string + description: | + Present only on an incomplete activation (HTTP 502): a human-readable + reason naming the failed step (markets vs. site link). Never leaks the + upstream gateway URL. markets: type: array description: Per-market outcome, in request order. @@ -10781,7 +10880,7 @@ SerenityActivateResponse: type: string status: type: integer - description: Per-market HTTP-style status (201 live, 409 already-live, 4xx validation). + description: Per-market HTTP-style status (201 live, 409 already-live, 4xx/5xx failure). body: type: object additionalProperties: true diff --git a/docs/openapi/serenity-api.yaml b/docs/openapi/serenity-api.yaml index 16871e906f..428991e91f 100644 --- a/docs/openapi/serenity-api.yaml +++ b/docs/openapi/serenity-api.yaml @@ -474,10 +474,26 @@ v2-serenity-activate: it on first call and persisting `brands.semrush_workspace_id`, which flips the brand into sub-workspace mode), then per supplied market creates-or-resumes a draft and publishes it once. Markets are caller-supplied (reactivation - re-supplies them — there is no stored market memory). Idempotent on the - workspace; per-market it is 409-safe (an already-live slice is reported, - not duplicated). Returns 200 when ≥1 market went live (brand set `active`), - or 207 when none did (brand stays `pending`). + re-supplies them — there is no stored market memory); a pending (draft) + brand falls back to its stashed `pending_semrush_provisioning`. Idempotent on + the workspace; per-market it is 409-safe (an already-live slice is reported, + not duplicated). + + Project creation is gated on a primary URL/domain, NOT on prompt + generation: with no domain the brand is activated sub-workspace-only (no + project, HTTP 200, empty `markets`), anchored solely by its sub-workspace — + the bare "save & continue later" draft. With a domain but no market, a + single US/EN fallback project is provisioned. `generatePrompts` (body, else + stash, else false) decides whether topics/prompts are generated for the + provisioned project(s); it requires a domain, so true with no domain is a 400. + + ALL-OR-NOTHING: the brand flips to `active` (HTTP 200) only when the FULL + chain succeeds — sub-workspace ensured, EVERY market's project live, the + brand linked to its sub-workspace, AND every project mirrored as a Site + (`brand_sites` type=`serenity`). If ANY step fails, a pending brand STAYS + pending and the response is HTTP 502 (its stash + workspace pointer are kept + intact; retry converges idempotently). An already-active brand re-supplying + markets is never downgraded (a partial failure is reported as 207). operationId: activateSerenityBrand security: - ims_key: [] @@ -488,12 +504,14 @@ v2-serenity-activate: schema: { $ref: './schemas.yaml#/SerenityActivateRequest' } responses: '200': - description: At least one market is live; brand set active. + description: Full chain succeeded; brand set `active`. Body lists per-market outcomes. content: application/json: schema: { $ref: './schemas.yaml#/SerenityActivateResponse' } '207': - description: No market went live; brand left pending. Body lists per-market outcomes. + description: >- + An already-active brand re-supplied markets and at least one failed; the + brand is NOT downgraded (stays `active`). Body lists per-market outcomes. content: application/json: schema: { $ref: './schemas.yaml#/SerenityActivateResponse' } @@ -508,10 +526,15 @@ v2-serenity-activate: application/json: schema: { $ref: './schemas.yaml#/SerenityErrorResponse' } '502': - description: Upstream returned a non-2xx response. + description: >- + Incomplete activation — a pending (draft) brand's provisioning chain did + not fully complete (a market failed, or markets are live but could not be + linked as Sites). The brand STAYS `pending`; the body carries + `error`/`message` and the per-market outcomes so the caller can retry. + Also returned when an upstream call returns a non-2xx response. content: application/json: - schema: { $ref: './schemas.yaml#/SerenityErrorResponse' } + schema: { $ref: './schemas.yaml#/SerenityActivateResponse' } '500': { $ref: './responses.yaml#/500' } v2-serenity-deactivate: diff --git a/src/controllers/brands.js b/src/controllers/brands.js index 85bada00b7..05313128bc 100644 --- a/src/controllers/brands.js +++ b/src/controllers/brands.js @@ -1397,10 +1397,24 @@ function BrandsController(ctx, log, env) { // before picking its primary URL. const isPendingBrand = brandData.status === 'pending'; const { semrushMarket } = brandData; - if (isNonEmptyObject(semrushMarket)) { - const { market, languageCode } = semrushMarket; - if (!hasText(market) || !hasText(languageCode)) { - return badRequest('semrushMarket requires market and languageCode'); + const hasSemrushMarket = isNonEmptyObject(semrushMarket); + // generatePrompts (default false) gates topic/prompt generation ONLY. The + // wizard sends it as an explicit boolean for every Semrush-mode create, so + // its presence ALSO signals Semrush mode even when no market was picked — + // a bare "save and continue later" draft (location/language optional). + const generatePrompts = brandData.generatePrompts === true; + const isSemrushMode = hasSemrushMarket || typeof brandData.generatePrompts === 'boolean'; + if (isSemrushMode) { + let market; + let languageCode; + if (hasSemrushMarket) { + ({ market, languageCode } = semrushMarket); + if (!hasText(market) || !hasText(languageCode)) { + return badRequest('semrushMarket requires market and languageCode'); + } + } else if (generatePrompts) { + // Generating prompts needs a project, which needs a (market, language). + return badRequest('market and languageCode are required when generatePrompts is true'); } if (isPendingBrand) { // Defer provisioning: persist the chosen (market, languageCode) AND @@ -1410,17 +1424,38 @@ function BrandsController(ctx, log, env) { // URL otherwise lives only on the Semrush side, so a site-less draft // would have nowhere to keep it. The row lands as 'pending' because it // has no anchor (no site_id, no semrush_workspace_id) — see - // upsertBrand's anchor check. Models are collected at activation. + // upsertBrand's anchor check. const primaryUrl = (Array.isArray(brandData.urls) ? brandData.urls : []) .map((u) => (typeof u === 'string' ? u : u?.value)) .find(hasText) || null; - // TODO: the wizard creates a draft with a single market today, so we - // stash exactly one (market, languageCode). The activate flow already - // handles N stashed markets — if multi-market draft creation is ever - // added, build this `markets` array from all selected markets here. + // AI models (LLMs) the wizard collected. Unlike the direct-provision + // path they are NOT required here — a draft can be saved before the + // user picks any, and they can be edited per-market later from the + // Markets tab. Seed the initial market's modelIds with them when + // present so activation applies them; omit the key entirely when none + // were chosen (mirrors normalizePendingSemrushProvisioning). + const seedModelIds = Array.isArray(brandData.semrushModelIds) + ? brandData.semrushModelIds.filter(hasText) + : []; + // A no-prompt draft may carry NO market at all (location/language are + // optional then) — stash only the market actually picked. The activate + // flow already handles 0..N stashed markets: an empty list + a primary + // URL provisions a single US/EN fallback project, and an empty list + + // no URL provisions a sub-workspace-only brand. + // TODO: the wizard creates at most one market today; if multi-market + // draft creation is added, build this array from all selected markets. + const markets = []; + if (hasSemrushMarket) { + const initialMarket = { market, languageCode }; + if (seedModelIds.length > 0) { + initialMarket.modelIds = seedModelIds; + } + markets.push(initialMarket); + } brandData.pendingSemrushProvisioning = { primaryUrl, - markets: [{ market, languageCode }], + markets, + generatePrompts, }; } else { const brandDomain = brandDomainFromPayload(brandData); @@ -1428,12 +1463,15 @@ function BrandsController(ctx, log, env) { return badRequest('A primary URL is required to provision a Semrush brand'); } provisionedBrandDomain = brandDomain; - // The project needs at least one AI model (LLM) to track. The wizard - // collects them; reject a Semrush create that omits them. + // A prompt-generating project needs at least one AI model (LLM) to + // track. The wizard collects them; reject a prompt-generating Semrush + // create that omits them. With generatePrompts=false the project is + // created empty, so models are optional (it tracks nothing until the + // user adds them later). const modelIds = Array.isArray(brandData.semrushModelIds) ? brandData.semrushModelIds.filter(hasText) : []; - if (modelIds.length === 0) { + if (generatePrompts && modelIds.length === 0) { return badRequest('semrushModelIds must list at least one AI model to track'); } // Brand aliases drive branded/non-branded prompt classification. Accept @@ -1456,10 +1494,13 @@ function BrandsController(ctx, log, env) { spaceCatId, brandId: provisionedBrandId, brandName: brandData.name, + // market/languageCode may be undefined when generatePrompts=false and + // no market was picked — provisionBrandSubworkspace falls back to US/EN. market, languageCode, brandDomain, modelIds, + generateTopics: generatePrompts, brandAliases, brandUrlSources, // Competitors ("other brands to track") are merged into the initial @@ -1559,13 +1600,28 @@ function BrandsController(ctx, log, env) { // baseUrl is read-only (resolved from baseSiteId) — strip from updates. delete updates.baseUrl; - // pendingSemrushProvisioning is system-controlled: written only when a - // brand is saved as 'pending' (createBrandForOrg) and consumed/cleared by - // the activate flow. It is marked readOnly in the OpenAPI schema, but that - // is a doc hint with no runtime teeth — strip it here so a PATCH caller - // cannot inject a primaryUrl/markets that activation would later trust for - // Semrush provisioning. - delete updates.pendingSemrushProvisioning; + // pendingSemrushProvisioning is the deferred-provisioning staging blob for + // a *pending* (draft) brand. The draft UI mutates it via PATCH — the + // Markets tab appends a market / edits a market's LLMs before activation. + // Permit that ONLY while the brand is (and stays) pending: an active brand + // keeps its markets on the Semrush side, so a PATCH must never inject a + // primaryUrl/markets onto a live brand that activation would later trust. + // When the target isn't pending (or the same PATCH is flipping it to + // active — activation is the serenity endpoint's job, not PATCH's), strip + // it. Only pay for the status read when the field is actually present. + if (updates.pendingSemrushProvisioning !== undefined) { + const { data: currentBrand } = await postgrestClient + .from('brands') + .select('status') + .eq('organization_id', spaceCatId) + .eq('id', brandUuid) + .maybeSingle(); + const isPending = currentBrand?.status === 'pending' + && (updates.status === undefined || updates.status === 'pending'); + if (!isPending) { + delete updates.pendingSemrushProvisioning; + } + } // Capture the competitor list BEFORE the update so the Semrush re-sync can // compute which competitors were removed (old − new) — the only ones it diff --git a/src/controllers/serenity.js b/src/controllers/serenity.js index ab9f925cb7..c2230c7233 100644 --- a/src/controllers/serenity.js +++ b/src/controllers/serenity.js @@ -54,6 +54,8 @@ import { handleBulkDeletePromptsSubworkspace, } from '../support/serenity/handlers/prompts-subworkspace.js'; import { ensureSubworkspace, decommissionBrandWorkspace } from '../support/serenity/workspace-lifecycle.js'; +import { MAX_TOPICS_ON_CREATE } from '../support/serenity/brand-provisioning.js'; +import { STANDARD_PROMPT_TAGS, PROJECT_STANDARD_TAGS } from '../support/serenity/prompt-tags.js'; import AccessControlUtil from '../support/access-control-util.js'; import { resolveBrandUuid } from '../support/prompts-storage.js'; import { @@ -78,15 +80,6 @@ function safeError(msg) { return cleanupHeaderValue(String(msg || '')).slice(0, MAX_ERR_MSG_LEN); } -/** - * Stable identity for a market (market + languageCode), used to match a - * provisioned market against the deferred-provisioning stash. Case/space - * insensitive so 'US'/'us' and ' en'/'en' compare equal. - */ -function marketKey(market, languageCode) { - return `${String(market ?? '').trim().toLowerCase()}|${String(languageCode ?? '').trim().toLowerCase()}`; -} - /** * Extracts query params from the request URL. Does NOT fall back to * `context.data` (the request body) — body keys must never become query keys @@ -532,6 +525,9 @@ function SerenityController(context, log, env) { auth.brandUuid, ctx.dataAccess.services.postgrestClient, ); + // Optional prompt/topic generation for this market, defaulting to off so + // the endpoint's behavior is unchanged unless the caller opts in. + const genMarketTopics = (ctx.data || {}).generatePrompts === true; result = await handleCreateMarketSubworkspace( transport, brand, @@ -540,7 +536,15 @@ function SerenityController(context, log, env) { log, null, brandPointerReloader(ctx, auth.brandUuid), - { brandAliases, brandUrlSources, competitors }, + { + generateTopics: genMarketTopics, + topicCap: genMarketTopics ? MAX_TOPICS_ON_CREATE : 0, + standardTags: genMarketTopics ? STANDARD_PROMPT_TAGS : [], + projectTags: genMarketTopics ? PROJECT_STANDARD_TAGS : [], + brandAliases, + brandUrlSources, + competitors, + }, ); // Mirror this market as a SpaceCat Site (+ brand_sites link) keyed on the // market's own domain, once its Semrush project is created. Best-effort: @@ -784,28 +788,108 @@ function SerenityController(context, log, env) { const storedMarkets = Array.isArray(pendingSemrushProvisioning?.markets) ? pendingSemrushProvisioning.markets : []; - const markets = Array.isArray(body.markets) && body.markets.length > 0 - ? body.markets - : storedMarkets; - if (markets.length === 0) { - throw new ErrorWithStatusCode('markets must be a non-empty array', 400); - } - if (markets.length > MAX_MARKETS) { - throw new ErrorWithStatusCode(`markets must not exceed ${MAX_MARKETS} entries`, 400); - } + // Whether to generate topics/prompts for the provisioned project(s). Body + // overrides the stash; default false preserves the historical activate + // behavior (projects published without generated prompts). + const generatePrompts = typeof body.generatePrompts === 'boolean' + ? body.generatePrompts + : pendingSemrushProvisioning?.generatePrompts === true; + const wasPending = brand.getStatus?.() === 'pending'; // The Semrush project domain: the request's brandDomain, else derived from // the stashed draft primary URL (the wizard's "Save as pending" URL). + const suppliedUrlOrDomain = hasText(body.brandDomain) + || hasText(pendingSemrushProvisioning?.primaryUrl); const brandDomain = hasText(body.brandDomain) ? body.brandDomain : hostnameFromUrlString(pendingSemrushProvisioning?.primaryUrl); - // Fail fast with the same discipline as the direct create path - // (brands.js guards `if (!hasText(brandDomain)) return badRequest(...)`). - // A draft saved without a primary URL, activated without a body - // brandDomain, has no domain to provision against — a null would - // propagate into handleCreateMarketSubworkspace and surface as an opaque - // upstream error or an orphaned sub-workspace rather than a clear 400. + + // ----- Sub-workspace-only activation (no primary URL → no project) ----- + // A brand with no domain has nothing to provision a project against: just + // ensure its sub-workspace (which IS the active-brand anchor, persisted by + // ensureSubworkspace) and flip it active. This is the bare "save & continue + // later" draft; the user adds markets (projects) afterwards from the Markets + // tab. generatePrompts can't apply with no project, so reject the combo. if (!hasText(brandDomain)) { - throw new ErrorWithStatusCode('brandDomain is required to provision a Semrush market', 400); + // A URL/domain WAS supplied but did not resolve to a hostname → bad input, + // not a bare brand. Fail fast (a silent fallback would mask the typo and + // strand the user with a project-less brand they did not ask for). + if (suppliedUrlOrDomain) { + throw new ErrorWithStatusCode('brandDomain is required to provision a Semrush market', 400); + } + if (generatePrompts) { + throw new ErrorWithStatusCode('A primary URL is required to generate prompts', 400); + } + const bareWorkspaceId = await ensureSubworkspace( + transport, + brand, + auth.parentWorkspaceId, + 1, + log, + {}, + brandPointerReloader(ctx, auth.brandUuid), + ); + let bareSucceeded = true; + if (typeof brand.setStatus === 'function') { + brand.setStatus('active'); + } + if (hadPendingSemrushProvisioning + && typeof brand.setPendingSemrushProvisioning === 'function') { + brand.setPendingSemrushProvisioning(null); + } + try { + await brand.save(); + } catch (saveError) { + bareSucceeded = false; + log.error('serenity activate: SERENITY_ACTIVATE_SAVE_DIVERGENCE — sub-workspace ensured upstream but failed to persist active status', { + brandId: auth.brandUuid, + semrushWorkspaceId: bareWorkspaceId, + error: saveError?.message, + }); + } + log.info('serenity activate: completed (sub-workspace only)', { + brandId: auth.brandUuid, + semrushWorkspaceId: bareWorkspaceId, + fullySucceeded: bareSucceeded, + }); + if (bareSucceeded) { + return createResponse( + { brandId: auth.brandUuid, status: 'active', markets: [] }, + 200, + ); + } + // Save failed: a pending draft stays pending (retryable, idempotent — the + // sub-workspace 409s on retry); an already-active brand is left active + // (the flip was a no-op anyway). + if (wasPending) { + return createResponse( + { + brandId: auth.brandUuid, + status: 'pending', + error: 'serenityActivationIncomplete', + message: 'Sub-workspace provisioned but the active status could not be persisted.', + markets: [], + }, + 502, + ); + } + return createResponse( + { brandId: auth.brandUuid, status: 'active', markets: [] }, + 207, + ); + } + + // ----- Project activation (primary URL present) ----- + // Markets come from the body (reactivation), else the stash. A draft with a + // URL but no stashed market provisions a single US/EN fallback project — the + // same default brand-provisioning.js applies on the direct-create path. + const requestedMarkets = Array.isArray(body.markets) && body.markets.length > 0 + ? body.markets + : storedMarkets; + const markets = requestedMarkets.length > 0 + ? requestedMarkets + : [{ market: 'US', languageCode: 'en' }]; + if (markets.length > MAX_MARKETS) { + throw new ErrorWithStatusCode(`markets must not exceed ${MAX_MARKETS} entries`, 400); } // Brand aliases are brand-level: read once and apply to every market's // project (Semrush brand_names) in this batch. @@ -839,8 +923,6 @@ function SerenityController(context, log, env) { brandPointerReloader(ctx, auth.brandUuid), ); const results = []; - let anyLive = false; // ≥1 market is live (created now OR already live) - let anyFailed = false; // ≥1 market neither created nor already-live for (const m of markets) { const createBody = { market: m.market, @@ -850,6 +932,11 @@ function SerenityController(context, log, env) { brandDisplayName: body.brandDisplayName, name: m.name, }; + // AI models (LLMs) the draft staged for this market (or that the activate + // request supplied). handleCreateMarketSubworkspace reads them from its + // OPTIONS arg (NOT the body) and attaches them to the project before + // publish; omitted/empty → none attached. + const marketModelIds = Array.isArray(m.modelIds) ? m.modelIds : []; let r; try { // eslint-disable-next-line no-await-in-loop @@ -861,7 +948,25 @@ function SerenityController(context, log, env) { log, workspaceId, null, - { brandAliases, brandUrlSources, competitors }, + { + modelIds: marketModelIds, + // Generate topics/prompts only when the brand opted in. When false + // the project is published empty (no prompts) — today's default. + generateTopics: generatePrompts, + topicCap: generatePrompts ? MAX_TOPICS_ON_CREATE : 0, + standardTags: generatePrompts ? STANDARD_PROMPT_TAGS : [], + projectTags: generatePrompts ? PROJECT_STANDARD_TAGS : [], + // A project with neither models nor generated prompts publishes + // "empty units" → Semrush's disguised quota 405. Tolerate it + // (best-effort, leaves a draft) rather than failing activation; a + // project with models OR prompts has real units and must publish. + publishMode: marketModelIds.length > 0 || generatePrompts + ? 'require' + : 'best-effort', + brandAliases, + brandUrlSources, + competitors, + }, ); } catch (e) { // A single market failing must NOT abort the batch: markets already @@ -880,16 +985,10 @@ function SerenityController(context, log, env) { body: { error: 'serenityUpstreamError', message: 'Market activation failed' }, }; } - // 201 = created+published now; 409 = sliceExists (the market is already - // live upstream). Both mean the slice IS live, so both count toward - // brand-active and neither trips the partial-failure path — a full - // idempotent re-activate (every market already live → all 409s) is a - // complete success, not a 207/pending. - if (r.status === 201 || r.status === 409) { - anyLive = true; - } else { - anyFailed = true; - } + // 201 = created+published now; 409 = sliceExists (already live upstream). + // Both mean the slice IS live (a full idempotent re-activate where every + // market 409s is a complete success). The live/failed tally is derived + // from `results` after the loop (see allMarketsLive below). results.push({ market: m.market, languageCode: m.languageCode, @@ -898,58 +997,64 @@ function SerenityController(context, log, env) { }); } - // Per-market cleanup of the deferred-provisioning stash: drop every market - // that was just provisioned (201 = created now, 409 = already live), so a - // retry re-provisions ONLY the markets that still failed. When nothing - // remains, clear the whole blob (the draft is fully provisioned). Markets - // that failed this round stay stashed (with the primary URL) for retry. - const provisionedKeys = new Set( - results - .filter((r) => r.status === 201 || r.status === 409) - .map((r) => marketKey(r.market, r.languageCode)), - ); - const remainingMarkets = storedMarkets.filter( - (m) => !provisionedKeys.has(marketKey(m.market, m.languageCode)), - ); + // ALL-OR-NOTHING activation. The brand flips to 'active' ONLY when the + // full provisioning chain succeeded: + // 1. sub-workspace ensured (above; throws → caught → error response), + // 2. EVERY market's project published (status 201/409 — all live), + // 3. the brand is linked to its sub-workspace (semrushWorkspaceId, + // persisted by ensureSubworkspace above), AND + // 4. every provisioned market is mirrored as a Site + brand_sites row + // (type='serenity'). + // If ANY step fails, a brand that was pending STAYS pending — its stash and + // workspace pointer are left intact so a retry converges idempotently (live + // markets return 409; the site-link + stash-clear re-run) — and the + // response is an error. (An already-active brand re-supplying markets is + // never downgraded.) + const allMarketsLive = results.length > 0 + && results.every((r) => r.status === 201 || r.status === 409); + + // The brand_sites mirror is now a REQUIRED activation step (NOT + // best-effort): run it only once every market is live. Every market in + // this batch was provisioned against the single resolved `brandDomain` + // (body/stash primary URL), so one idempotent ensure on that domain links + // them all. A null return (any failure: bad input, cross-org, write error) + // keeps the brand pending below. + let siteLinked = false; + if (allMarketsLive) { + const linkedSiteId = await ensureMarketSite(ctx, { + // Optional-chained so a missing/throwing accessor can't 500 the call. + organizationId: brand.getOrganizationId?.(), + brandId: auth.brandUuid, + domain: brandDomain, + updatedBy: 'serenity-activate', + log, + }); + siteLinked = hasText(linkedSiteId); + } + + let fullySucceeded = allMarketsLive && siteLinked; - if (anyLive && typeof brand.setStatus === 'function') { - brand.setStatus('active'); - // Stash cleanup is intentionally coupled to this anyLive + setStatus - // guard so the flip-to-active and the stash trim happen in one - // brand.save() (atomic). If NOTHING went live (anyLive false) we skip - // both: the brand stays 'pending' with its stash intact, and a retry - // re-provisions — Semrush create is idempotent (a re-provisioned market - // returns 409, treated as success), so the coupling cannot strand a - // market or lose the stash. - // Update the stash to just the not-yet-provisioned markets (or null when - // all are done). Saved atomically with the status flip below. - const canSetStash = hadPendingSemrushProvisioning - && typeof brand.setPendingSemrushProvisioning === 'function'; - if (canSetStash) { - const stashPrimaryUrl = pendingSemrushProvisioning.primaryUrl ?? null; - const remainingStash = remainingMarkets.length > 0 - ? { primaryUrl: stashPrimaryUrl, markets: remainingMarkets } - : null; - brand.setPendingSemrushProvisioning(remainingStash); + if (fullySucceeded) { + if (typeof brand.setStatus === 'function') { + brand.setStatus('active'); + } + // Fully provisioned → clear the whole deferred-provisioning stash, + // saved atomically with the status flip. + if (hadPendingSemrushProvisioning + && typeof brand.setPendingSemrushProvisioning === 'function') { + brand.setPendingSemrushProvisioning(null); } try { await brand.save(); } catch (saveError) { - // Non-atomic seam — the mirror of deactivate's - // SERENITY_DEACTIVATE_SAVE_DIVERGENCE guard. The markets are already - // LIVE upstream (published in the loop above; the workspace pointer was - // persisted by ensureSubworkspace), but persisting the 'active' status - // flip failed: brands.status stays 'pending' while markets are live — - // divergent. A re-activate converges (idempotent: every live market - // returns 409), so this self-heals. Crucially, do NOT collapse to a 5xx - // via mapError — that would discard the per-market results telling the - // caller which markets went live. Emit a DISTINCT, greppable token so - // the orphaned status is alertable (not indistinguishable from an - // ordinary upstream error), force the partial-failure path so the caller - // sees a 207 instead of a bare 200 that hides the divergence, then fall - // through to return the multi-status body. - anyFailed = true; - log.error('serenity activate: SERENITY_ACTIVATE_SAVE_DIVERGENCE — markets live upstream but failed to persist active status', { + // Divergence seam: markets live + site linked upstream, but persisting + // the 'active' flip failed → the brand stays 'pending'. A re-activate + // converges (idempotent). Emit a DISTINCT, greppable token so the + // orphaned status is alertable, then fall through to the error response + // (do NOT collapse to a bare mapError 5xx — that discards the + // per-market results telling the caller what went live). + fullySucceeded = false; + log.error('serenity activate: SERENITY_ACTIVATE_SAVE_DIVERGENCE — markets live + site linked upstream but failed to persist active status', { brandId: auth.brandUuid, semrushWorkspaceId: workspaceId, marketsLive: results.filter((r) => r.status === 201 || r.status === 409).length, @@ -957,43 +1062,51 @@ function SerenityController(context, log, env) { }); } } - // Mirror the activated markets as a SpaceCat Site (+ brand_sites link) once - // at least one is live. Every market provisioned by this activation was - // created against the single resolved `brandDomain` (the body/stash primary - // URL), so one idempotent ensure on that domain covers them. Markets added - // later via createMarket carry their own domain and are mirrored there. - // Best-effort: never fails the activation. - if (anyLive) { - await ensureMarketSite(ctx, { - // Optional-chained so a missing/throwing accessor can't 500 an - // activation whose markets are already live upstream — best-effort. - organizationId: brand.getOrganizationId?.(), - brandId: auth.brandUuid, - domain: brandDomain, - updatedBy: 'serenity-activate', - log, - }); - } - // Success-level summary so a completed activation can be correlated with - // upstream state during incident investigation (counts + workspace). + + const marketsLiveCount = results.filter((r) => r.status === 201 || r.status === 409).length; log.info('serenity activate: completed', { brandId: auth.brandUuid, semrushWorkspaceId: workspaceId, - status: anyLive ? 'active' : 'pending', + fullySucceeded, + siteLinked, marketsTotal: results.length, - marketsLive: results.filter((r) => r.status === 201 || r.status === 409).length, - marketsFailed: results.filter((r) => !(r.status === 201 || r.status === 409)).length, + marketsLive: marketsLiveCount, + marketsFailed: results.length - marketsLiveCount, }); - // 207 Multi-Status whenever ANY market failed (even if others went live), - // so a caller keying off the HTTP status sees the partial failure instead - // of a bare 200. 200 only when every market is live. + + if (fullySucceeded) { + return createResponse( + { brandId: auth.brandUuid, status: 'active', markets: results }, + 200, + ); + } + + // Not fully succeeded. A pending-draft activation that did not complete + // every step STAYS pending and returns an ERROR (HTTP 502: the upstream + // provisioning chain is incomplete) naming the failed step, with the + // per-market results so the caller can show specifics and retry. + if (wasPending) { + const failureReason = !allMarketsLive + ? 'One or more markets failed to provision.' + : 'Markets were provisioned but could not be linked as sites (brand_sites).'; + return createResponse( + { + brandId: auth.brandUuid, + status: 'pending', + error: 'serenityActivationIncomplete', + message: failureReason, + markets: results, + }, + 502, + ); + } + + // An already-active brand re-supplying markets (reactivation) is never + // downgraded: a single failed market is reported as 207 Multi-Status while + // the brand remains active. return createResponse( - { - brandId: auth.brandUuid, - status: anyLive ? 'active' : 'pending', - markets: results, - }, - anyFailed ? 207 : 200, + { brandId: auth.brandUuid, status: 'active', markets: results }, + 207, ); } catch (e) { return mapError(e, log); diff --git a/src/support/brands-storage.js b/src/support/brands-storage.js index e5c653a653..654a7c4dcc 100644 --- a/src/support/brands-storage.js +++ b/src/support/brands-storage.js @@ -49,10 +49,13 @@ function normalizeNullableText(value, fieldName) { /** * Normalizes the deferred Semrush provisioning data of a pending (draft) brand * into the shape persisted in `brands.pending_semrush_provisioning` (a JSONB object - * `{ primaryUrl?, markets: [{ market, languageCode }] }`). These are the primary - * URL + initial market the add-brand wizard collected before a - * sub-workspace/project exist; activation reads them back to provision the real - * Semrush projects, then clears the column. + * `{ primaryUrl?, markets: [{ market, languageCode, modelIds? }], generatePrompts? }`). + * These are the primary URL + initial markets (each with its chosen AI models/LLMs) + * plus whether activation should generate topics/prompts — all collected by the + * add-brand wizard before a sub-workspace/project exist; activation reads them + * back to provision the real Semrush projects, then clears the column. + * The canonical shape is the `PendingSemrushProvisioning` type exported by + * `@adobe/spacecat-shared-data-access` (shared with project-elmo-ui). * * Returns `undefined` when the caller did not supply the field (leave the column * untouched), `null` when explicitly cleared or nothing useful remains, or the @@ -60,7 +63,7 @@ function normalizeNullableText(value, fieldName) { * * @param {unknown} value - `{ primaryUrl?, markets }`, null, or undefined. * @returns {{primaryUrl: (string|null), markets: Array<{market: string, - * languageCode: string}>}|null|undefined} + * languageCode: string, modelIds?: string[]}>}|null|undefined} */ function normalizePendingSemrushProvisioning(value) { if (value === undefined) { @@ -75,19 +78,42 @@ function normalizePendingSemrushProvisioning(value) { throw error; } const markets = (Array.isArray(value.markets) ? value.markets : []) - .map((m) => ({ - market: typeof m?.market === 'string' ? m.market.trim() : '', - languageCode: typeof m?.languageCode === 'string' ? m.languageCode.trim() : '', - })) + .map((m) => { + const market = { + market: typeof m?.market === 'string' ? m.market.trim() : '', + languageCode: typeof m?.languageCode === 'string' ? m.languageCode.trim() : '', + }; + // Per-market AI models (LLMs) chosen for this market; applied to the + // project at activation. Keep only non-empty strings; omit the key + // entirely when none remain so a cleared selection doesn't persist `[]`. + const modelIds = (Array.isArray(m?.modelIds) ? m.modelIds : []) + .map((id) => (typeof id === 'string' ? id.trim() : '')) + .filter(hasText); + if (modelIds.length > 0) { + market.modelIds = modelIds; + } + return market; + }) .filter((m) => hasText(m.market) && hasText(m.languageCode)); const primaryUrl = typeof value.primaryUrl === 'string' && hasText(value.primaryUrl) ? value.primaryUrl.trim() : null; - // Nothing worth persisting → treat as cleared. - if (markets.length === 0 && !hasText(primaryUrl)) { + // Whether activation should generate topics/prompts for the provisioned + // project(s). Only meaningful as an explicit boolean; absent means "legacy + // stash, leave generation off" and is omitted so the column stays minimal. + const hasGeneratePrompts = typeof value.generatePrompts === 'boolean'; + // Nothing worth persisting → treat as cleared. An explicit generatePrompts + // flag IS worth persisting on its own: a bare no-prompt draft (no URL, no + // market) still needs the stash so activation knows to provision a + // sub-workspace-only brand rather than treating it as a non-Semrush brand. + if (markets.length === 0 && !hasText(primaryUrl) && !hasGeneratePrompts) { return null; } - return { primaryUrl, markets }; + const result = { primaryUrl, markets }; + if (hasGeneratePrompts) { + result.generatePrompts = value.generatePrompts; + } + return result; } /** diff --git a/src/support/serenity/brand-provisioning.js b/src/support/serenity/brand-provisioning.js index 7a48825fa6..cd882b0584 100644 --- a/src/support/serenity/brand-provisioning.js +++ b/src/support/serenity/brand-provisioning.js @@ -86,6 +86,7 @@ export function initialMarketProjectName(market, languageCode) { export async function provisionBrandSubworkspace(context, { spaceCatId, brandId, brandName, market, languageCode, brandDomain, modelIds = [], brandAliases = [], brandUrlSources = null, competitors = [], + generateTopics = true, }, log = console) { if (!hasText(brandName)) { throw new ErrorWithStatusCode('brandName is required for Semrush provisioning', 400); @@ -93,12 +94,16 @@ export async function provisionBrandSubworkspace(context, { if (!hasText(brandId)) { throw new ErrorWithStatusCode('brandId is required for Semrush provisioning', 400); } - if (!hasText(market) || !hasText(languageCode)) { - throw new ErrorWithStatusCode('market and languageCode are required for Semrush provisioning', 400); - } if (!hasText(brandDomain)) { throw new ErrorWithStatusCode('brandDomain is required for Semrush provisioning', 400); } + // market/languageCode are OPTIONAL: a brand created WITHOUT prompt generation + // (generateTopics=false) may omit them. Fall back to the US/EN default slice so + // the project still has a valid (geo, language) to provision against. When + // generateTopics=true the caller (brands.js) still requires both, so a fallback + // never silently mislabels a prompt-generating project. + const resolvedMarket = hasText(market) ? market : 'US'; + const resolvedLanguageCode = hasText(languageCode) ? languageCode : 'en'; const parentWorkspaceId = await resolveWorkspaceId(context, spaceCatId); if (!hasText(parentWorkspaceId)) { @@ -152,31 +157,38 @@ export async function provisionBrandSubworkspace(context, { brandStub, parentWorkspaceId, { - market, - languageCode, + market: resolvedMarket, + languageCode: resolvedLanguageCode, brandDomain, brandNames: [brandName], brandDisplayName: brandName, - name: initialMarketProjectName(market, languageCode), + name: initialMarketProjectName(resolvedMarket, resolvedLanguageCode), }, log, // preResolvedWorkspaceId / reloadPointer: defaults (single-create path). null, null, - // Brand-create attaches the chosen LLMs, generates+attaches topics/prompts - // (top N by volume, tagged `topic:` + standard tags), then publishes. - // The child is carved a real allocation (CREATE_ALLOCATION) so prompts and - // publish have quota; a 405 here is a true over-quota and surfaces below. + // Brand-create attaches the chosen LLMs and, WHEN generateTopics is set, + // generates+attaches topics/prompts (top N by volume, tagged `topic:` + // + standard tags) before publishing. With generateTopics=false the project + // is created empty (no prompts); models are still attached when supplied. { modelIds, - generateTopics: true, - topicCap: MAX_TOPICS_ON_CREATE, + generateTopics, + topicCap: generateTopics ? MAX_TOPICS_ON_CREATE : 0, standardTags: STANDARD_PROMPT_TAGS, brandAliases, projectTags: PROJECT_STANDARD_TAGS, brandUrlSources, competitors, - publishMode: 'require', + // A project with neither models nor generated prompts would publish + // "empty units", which Semrush rejects with a disguised quota 405 + // (workspace doc §5). Tolerate that by leaving it a draft (best-effort) + // instead of failing the whole create; a project that has models OR + // prompts has real units and must publish (require). + publishMode: (Array.isArray(modelIds) && modelIds.length > 0) || generateTopics + ? 'require' + : 'best-effort', }, ); } catch (e) { diff --git a/test/controllers/brands.test.js b/test/controllers/brands.test.js index a90c1ed622..886fdb239b 100644 --- a/test/controllers/brands.test.js +++ b/test/controllers/brands.test.js @@ -4625,14 +4625,42 @@ describe('Brands Controller', () => { expect(provisionStub.called).to.equal(false); }); - it('returns 400 when semrushModelIds is missing or empty', async () => { + it('returns 400 when semrushModelIds is missing or empty and generatePrompts is true', async () => { const provisionStub = sinon.stub().resolves({ semrushWorkspaceId: 'ws-1' }); const controller = await buildController({ provisionBrandSubworkspace: provisionStub }); + // generatePrompts:true → a prompt-generating project, which needs a model. const response = await controller.createBrandForOrg({ ...context, params: { spaceCatId: ORGANIZATION_ID }, - data: { name: 'New Brand', urls: [{ value: 'https://acme.com' }], semrushMarket: { market: 'us', languageCode: 'en' } }, + data: { + name: 'New Brand', + urls: [{ value: 'https://acme.com' }], + semrushMarket: { market: 'us', languageCode: 'en' }, + generatePrompts: true, + }, + dataAccess: mockDataAccess, + attributes: { authInfo: { profile: { email: 'user@test.com' } } }, + }); + + expect(response.status).to.equal(400); + expect(provisionStub.called).to.equal(false); + }); + + it('returns 400 when generatePrompts is true but no market/language was supplied', async () => { + const provisionStub = sinon.stub().resolves({ semrushWorkspaceId: 'ws-1' }); + const controller = await buildController({ provisionBrandSubworkspace: provisionStub }); + + // generatePrompts true signals Semrush mode even with no semrushMarket, but + // generating prompts needs a project → market+language are required. + const response = await controller.createBrandForOrg({ + ...context, + params: { spaceCatId: ORGANIZATION_ID }, + data: { + name: 'New Brand', + urls: [{ value: 'https://acme.com' }], + generatePrompts: true, + }, dataAccess: mockDataAccess, attributes: { authInfo: { profile: { email: 'user@test.com' } } }, }); @@ -4641,6 +4669,33 @@ describe('Brands Controller', () => { expect(provisionStub.called).to.equal(false); }); + it('provisions WITHOUT models when generatePrompts is false (model-less project allowed)', async () => { + const provisionStub = sinon.stub().resolves({ semrushWorkspaceId: 'ws-1' }); + const upsertStub = sinon.stub().resolves({ id: 'forced-id', name: 'New Brand' }); + const controller = await buildController({ + provisionBrandSubworkspace: provisionStub, upsertBrand: upsertStub, + }); + + const response = await controller.createBrandForOrg({ + ...context, + params: { spaceCatId: ORGANIZATION_ID }, + data: { + name: 'New Brand', + urls: [{ value: 'https://acme.com' }], + semrushMarket: { market: 'us', languageCode: 'en' }, + generatePrompts: false, + }, + dataAccess: mockDataAccess, + attributes: { authInfo: { profile: { email: 'user@test.com' } } }, + }); + + expect(response.status).to.equal(201); + expect(provisionStub.called).to.equal(true); + // generateTopics is threaded through as false. + expect(provisionStub.firstCall.args[1].generateTopics).to.equal(false); + expect(provisionStub.firstCall.args[1].modelIds).to.deep.equal([]); + }); + it('returns 400 when no primary URL is present to derive a domain', async () => { const provisionStub = sinon.stub().resolves({ semrushWorkspaceId: 'ws-1' }); const controller = await buildController({ provisionBrandSubworkspace: provisionStub }); @@ -4703,6 +4758,7 @@ describe('Brands Controller', () => { expect(upsertArgs.brand.pendingSemrushProvisioning).to.deep.equal({ primaryUrl: null, markets: [{ market: 'us', languageCode: 'en' }], + generatePrompts: false, }); // A draft is never bound to a workspace at create time. expect(upsertArgs.semrushWorkspaceId).to.equal(null); @@ -4734,6 +4790,39 @@ describe('Brands Controller', () => { expect(upsertStub.firstCall.args[0].brand.pendingSemrushProvisioning).to.deep.equal({ primaryUrl: 'https://acme.com/path', markets: [{ market: 'us', languageCode: 'en' }], + generatePrompts: false, + }); + }); + + it('seeds the initial market modelIds from semrushModelIds on a pending draft (no provisioning)', async () => { + const provisionStub = sinon.stub().resolves({ semrushWorkspaceId: 'ws-1' }); + const upsertStub = sinon.stub().resolves({ id: 'draft-id', name: 'New Brand', status: 'pending' }); + const controller = await buildController({ + provisionBrandSubworkspace: provisionStub, upsertBrand: upsertStub, + }); + + const response = await controller.createBrandForOrg({ + ...context, + params: { spaceCatId: ORGANIZATION_ID }, + data: { + name: 'New Brand', + status: 'pending', + urls: [{ value: 'https://acme.com' }], + semrushMarket: { market: 'us', languageCode: 'en' }, + // The wizard's model picks: unlike the direct path they are optional + // for a draft, but when present they seed the initial market. + semrushModelIds: ['chatgpt', 'perplexity'], + }, + dataAccess: mockDataAccess, + attributes: { authInfo: { profile: { email: 'user@test.com' } } }, + }); + + expect(response.status).to.equal(201); + expect(provisionStub.called).to.equal(false); + expect(upsertStub.firstCall.args[0].brand.pendingSemrushProvisioning).to.deep.equal({ + primaryUrl: 'https://acme.com', + markets: [{ market: 'us', languageCode: 'en', modelIds: ['chatgpt', 'perplexity'] }], + generatePrompts: false, }); }); @@ -5008,7 +5097,9 @@ describe('Brands Controller', () => { expect(syncStub).to.not.have.been.called; }); - it('strips system-controlled pendingSemrushProvisioning from a PATCH (no runtime injection)', async () => { + it('strips pendingSemrushProvisioning from a PATCH when the brand is NOT pending (no runtime injection)', async () => { + // The describe-level postgrest mock resolves the brand with status:'active', + // so the pending-only guard strips the stash an attacker tried to inject. const updateBrandStub = sinon.stub().resolves({ id: BRAND_UUID, semrushWorkspaceId: null }); const controller = await buildUpdateController({ updateBrand: updateBrandStub, @@ -5038,6 +5129,75 @@ describe('Brands Controller', () => { expect(updates).to.not.have.property('pendingSemrushProvisioning'); }); + it('keeps pendingSemrushProvisioning on a PATCH when the brand IS pending (draft Markets-tab edit)', async () => { + // The draft Markets tab appends a market (with its LLMs) by PATCHing the + // stash; the pending-only guard must let it through for a pending brand. + mockDataAccess.services.postgrestClient.from = sandbox.stub().callsFake(() => ({ + select: sandbox.stub().returnsThis(), + eq: sandbox.stub().returnsThis(), + maybeSingle: sandbox.stub().resolves({ data: { status: 'pending' }, error: null }), + })); + const updateBrandStub = sinon.stub().resolves({ id: BRAND_UUID, semrushWorkspaceId: null }); + const controller = await buildUpdateController({ + updateBrand: updateBrandStub, + syncBrandUrlsAcrossMarkets: sinon.stub().resolves({}), + createSerenityTransport: sinon.stub(), + }); + + const stash = { + primaryUrl: 'https://acme.com', + markets: [{ market: 'us', languageCode: 'en', modelIds: ['chatgpt'] }], + }; + const response = await controller.updateBrandForOrg({ + ...context, + params: { spaceCatId: ORGANIZATION_ID, brandId: BRAND_UUID }, + data: { pendingSemrushProvisioning: stash }, + dataAccess: mockDataAccess, + pathInfo: { headers: { authorization: 'Bearer tok' } }, + attributes: { authInfo: { profile: { email: 'user@test.com' } } }, + }); + + expect(response.status).to.equal(200); + expect(updateBrandStub).to.have.been.calledOnce; + const { updates } = updateBrandStub.firstCall.args[0]; + expect(updates.pendingSemrushProvisioning).to.deep.equal(stash); + }); + + it('strips pendingSemrushProvisioning when a PATCH would flip a pending brand to active', async () => { + // Going active is the serenity activate endpoint's job, not PATCH's: a + // PATCH that sets status:'active' must not also carry a staging stash. + mockDataAccess.services.postgrestClient.from = sandbox.stub().callsFake(() => ({ + select: sandbox.stub().returnsThis(), + eq: sandbox.stub().returnsThis(), + maybeSingle: sandbox.stub().resolves({ data: { status: 'pending' }, error: null }), + })); + const updateBrandStub = sinon.stub().resolves({ id: BRAND_UUID, semrushWorkspaceId: null }); + const controller = await buildUpdateController({ + updateBrand: updateBrandStub, + syncBrandUrlsAcrossMarkets: sinon.stub().resolves({}), + createSerenityTransport: sinon.stub(), + }); + + const response = await controller.updateBrandForOrg({ + ...context, + params: { spaceCatId: ORGANIZATION_ID, brandId: BRAND_UUID }, + data: { + status: 'active', + pendingSemrushProvisioning: { + primaryUrl: 'https://acme.com', + markets: [{ market: 'us', languageCode: 'en' }], + }, + }, + dataAccess: mockDataAccess, + pathInfo: { headers: { authorization: 'Bearer tok' } }, + attributes: { authInfo: { profile: { email: 'user@test.com' } } }, + }); + + expect(response.status).to.equal(200); + const { updates } = updateBrandStub.firstCall.args[0]; + expect(updates).to.not.have.property('pendingSemrushProvisioning'); + }); + it('does NOT re-sync a flat-mode brand (no sub-workspace) even when URLs change', async () => { const updateBrandStub = sinon.stub().resolves({ id: BRAND_UUID, semrushWorkspaceId: null, urls: [], socialAccounts: [], earnedContent: [], diff --git a/test/controllers/serenity.test.js b/test/controllers/serenity.test.js index 6623eaeb96..3bfe177bcc 100644 --- a/test/controllers/serenity.test.js +++ b/test/controllers/serenity.test.js @@ -40,6 +40,10 @@ function makeBrandModel(overrides = {}) { getId: () => BRAND, getName: () => 'Test Brand', getOrganizationId: () => ORG, + // The activate flow is a pending (draft) brand being approved → active; the + // all-or-nothing path keys off this status (a non-pending brand is never + // downgraded on a partial failure). + getStatus: () => 'pending', getSemrushWorkspaceId: () => 'subworkspace-ws-1', setSemrushWorkspaceId: sinon.stub(), setStatus: sinon.stub(), @@ -902,9 +906,14 @@ describe('SerenityController', () => { })); expect(response.status).to.equal(201); expect(getBrandAliasNamesStub).to.have.been.calledOnceWith(BRAND); - // options object is the 8th arg (index 7). + // options object is the 8th arg (index 7). generatePrompts was not supplied, + // so topic generation defaults off (today's behavior is unchanged). expect(handlers.handleCreateMarketSubworkspace.firstCall.args[7]) .to.deep.equal({ + generateTopics: false, + topicCap: 0, + standardTags: [], + projectTags: [], brandAliases: ['Acme Inc', 'ACME'], brandUrlSources: { urls: [], socialAccounts: [], earnedContent: [] }, competitors: [], @@ -1110,7 +1119,7 @@ describe('SerenityController', () => { expect(opts).to.include({ organizationId: ORG, brandId: BRAND, domain: 'x.com' }); }); - it('activate does NOT mirror a Site when no market goes live', async () => { + it('activate does NOT mirror a Site (or flip active) when no market goes live — stays pending with a 502', async () => { handlers.handleCreateMarketSubworkspace.resolves({ status: 502, body: { error: 'serenityUpstreamError' } }); const brand = makeBrandModel(); const controller = SerenityController({ env: {} }, fakeLog(), {}); @@ -1118,8 +1127,12 @@ describe('SerenityController', () => { brand, data: { brandDomain: 'x.com', brandNames: ['X'], markets: [{ market: 'us', languageCode: 'en' }] }, })); - expect(response.status).to.equal(207); + // All-or-nothing: a market failed → no site mirror, brand stays pending, 502. + expect(response.status).to.equal(502); + const { status } = await readBody(response); + expect(status).to.equal('pending'); expect(ensureMarketSiteStub).to.not.have.been.called; + expect(brand.setStatus).to.not.have.been.called; }); it('activate falls back to the stashed pending_semrush_provisioning when the body omits markets + brandDomain', async () => { @@ -1156,24 +1169,54 @@ describe('SerenityController', () => { expect(brand.save).to.have.been.called; }); - it('activate 400s when the stash has markets but no primary URL and the body omits brandDomain (no domain to provision)', async () => { + it('activate passes each stashed market\'s modelIds into the options arg (LLMs applied at activation)', async () => { handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: {} }); const brand = makeBrandModel({ - // A draft saved before a primary URL was entered: markets present, no - // primaryUrl. hostnameFromUrlString(undefined) → null, so there is no - // domain to provision against → fail fast (mirrors the create path). getPendingSemrushProvisioning: () => ({ - markets: [{ market: 'us', languageCode: 'en' }], + primaryUrl: 'https://acme.com', + markets: [{ market: 'us', languageCode: 'en', modelIds: ['chatgpt', 'perplexity'] }], }), setPendingSemrushProvisioning: sinon.stub(), }); const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ brand, data: { brandNames: ['X'] } })); + expect(response.status).to.equal(200); + // modelIds are read from the OPTIONS arg (index 7), NOT the body (index 3) — + // handleCreateMarketSubworkspace destructures them from options. + const options = handlers.handleCreateMarketSubworkspace.firstCall.args[7]; + expect(options.modelIds).to.deep.equal(['chatgpt', 'perplexity']); + // models present but no prompts → real units → must publish. + expect(options.publishMode).to.equal('require'); + }); + + it('activate provisions a sub-workspace-only brand (200) when there is no primary URL, ignoring any stashed market', async () => { + // A draft saved before a primary URL was entered: a market may be stashed + // but there is no domain to provision a project against. Project creation is + // gated on a URL, so with none the brand activates sub-workspace-only — its + // sub-workspace IS the active anchor — and no project is created. + handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: {} }); + const setPendingSemrushProvisioning = sinon.stub(); + const brand = makeBrandModel({ + getPendingSemrushProvisioning: () => ({ + markets: [{ market: 'us', languageCode: 'en' }], + }), + setPendingSemrushProvisioning, + }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); const response = await controller.activate(fakeContext({ brand, data: { brandNames: ['X'] }, })); - expect(response.status).to.equal(400); + expect(response.status).to.equal(200); + const { status, markets } = await readBody(response); + expect(status).to.equal('active'); + expect(markets).to.deep.equal([]); + // No project: the market loop never runs; only the sub-workspace is ensured. expect(handlers.handleCreateMarketSubworkspace).to.not.have.been.called; + expect(ensureSubworkspaceStub).to.have.been.calledOnce; + expect(brand.setStatus).to.have.been.calledWith('active'); + // The whole stash is cleared on a fully-successful activation. + expect(setPendingSemrushProvisioning).to.have.been.calledWith(null); }); it('activate 400s when the stashed primary URL is unparseable and the body omits brandDomain', async () => { @@ -1196,6 +1239,94 @@ describe('SerenityController', () => { expect(handlers.handleCreateMarketSubworkspace).to.not.have.been.called; }); + it('activate threads generateTopics + topicCap + tags when the stash opts into prompts', async () => { + handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: {} }); + const brand = makeBrandModel({ + getPendingSemrushProvisioning: () => ({ + primaryUrl: 'https://acme.com', + markets: [{ market: 'us', languageCode: 'en' }], + generatePrompts: true, + }), + setPendingSemrushProvisioning: sinon.stub(), + }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ brand, data: { brandNames: ['X'] } })); + expect(response.status).to.equal(200); + const options = handlers.handleCreateMarketSubworkspace.firstCall.args[7]; + expect(options.generateTopics).to.equal(true); + expect(options.topicCap).to.be.greaterThan(0); + expect(options.standardTags).to.not.be.empty; + expect(options.projectTags).to.not.be.empty; + // generatePrompts → real units → must publish. + expect(options.publishMode).to.equal('require'); + }); + + it('activate lets generatePrompts in the body override a stash that opted out', async () => { + handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: {} }); + const brand = makeBrandModel({ + getPendingSemrushProvisioning: () => ({ + primaryUrl: 'https://acme.com', + markets: [{ market: 'us', languageCode: 'en' }], + generatePrompts: false, + }), + setPendingSemrushProvisioning: sinon.stub(), + }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, + data: { brandNames: ['X'], generatePrompts: true }, + })); + expect(response.status).to.equal(200); + const opts = handlers.handleCreateMarketSubworkspace.firstCall.args[7]; + expect(opts.generateTopics).to.equal(true); + }); + + it('activate 400s when generatePrompts is true but there is no primary URL (nothing to generate into)', async () => { + const brand = makeBrandModel({ + getPendingSemrushProvisioning: () => ({ markets: [], generatePrompts: false }), + setPendingSemrushProvisioning: sinon.stub(), + }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, + data: { brandNames: ['X'], generatePrompts: true }, + })); + expect(response.status).to.equal(400); + expect(handlers.handleCreateMarketSubworkspace).to.not.have.been.called; + expect(ensureSubworkspaceStub).to.not.have.been.called; + }); + + it('activate sub-workspace-only stays pending (502) when the status save fails', async () => { + const setPendingSemrushProvisioning = sinon.stub(); + const brand = makeBrandModel({ + getPendingSemrushProvisioning: () => ({ markets: [], generatePrompts: false }), + setPendingSemrushProvisioning, + save: sinon.stub().rejects(new Error('db down')), + }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ brand, data: { brandNames: ['X'] } })); + expect(response.status).to.equal(502); + const { status, error } = await readBody(response); + expect(status).to.equal('pending'); + expect(error).to.equal('serenityActivationIncomplete'); + // The sub-workspace WAS ensured (upstream) even though the flip didn't persist. + expect(ensureSubworkspaceStub).to.have.been.calledOnce; + }); + + it('activate sub-workspace-only on an already-active brand returns 207 (not downgraded) when the save fails', async () => { + const brand = makeBrandModel({ + getStatus: () => 'active', + getPendingSemrushProvisioning: () => null, + save: sinon.stub().rejects(new Error('db down')), + }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ brand, data: { brandNames: ['X'] } })); + expect(response.status).to.equal(207); + const { status } = await readBody(response); + expect(status).to.equal('active'); + expect(ensureSubworkspaceStub).to.have.been.calledOnce; + }); + it('activate prefers body markets + brandDomain over the stash, and clears the stash when its market is provisioned', async () => { handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: {} }); const setPendingSemrushProvisioning = sinon.stub(); @@ -1220,10 +1351,11 @@ describe('SerenityController', () => { expect(setPendingSemrushProvisioning).to.have.been.calledWith(null); }); - it('activate removes ONLY the provisioned markets from the stash, keeping failed ones for retry', async () => { + it('activate keeps the FULL stash and stays pending when any market fails (all-or-nothing, no partial trim)', async () => { // Stash has two draft markets; the first provisions (201), the second - // throws. The provisioned market is dropped; the failed one stays stashed - // (with the primary URL) so a retry re-provisions just that one. + // throws. All-or-nothing: the brand does NOT flip active and the stash is + // NOT trimmed — the whole blob is kept intact so a retry re-runs the full + // batch (the live market 409s; the failed one retries). handlers.handleCreateMarketSubworkspace .onFirstCall().resolves({ status: 201, body: {} }) .onSecondCall().rejects(new ErrorWithStatusCode('upstream boom', 502)); @@ -1241,14 +1373,13 @@ describe('SerenityController', () => { brand, data: { brandNames: ['X'] }, })); - // One live, one failed → 207. - expect(response.status).to.equal(207); - expect(setPendingSemrushProvisioning).to.have.been.calledOnceWith({ - primaryUrl: 'https://acme.com', - markets: [{ market: 'de', languageCode: 'de' }], - }); - // Brand still flips active (≥1 market live) even with markets remaining. - expect(brand.setStatus).to.have.been.calledWith('active'); + // One live, one failed → activation incomplete → 502, brand stays pending. + expect(response.status).to.equal(502); + const { status } = await readBody(response); + expect(status).to.equal('pending'); + // Stash untouched (no trim, no clear) and the brand never flips active. + expect(setPendingSemrushProvisioning).to.not.have.been.called; + expect(brand.setStatus).to.not.have.been.calledWith('active'); }); it('activate does NOT clear a stash on a brand that has none (no setPendingSemrushProvisioning call)', async () => { @@ -1276,7 +1407,14 @@ describe('SerenityController', () => { // Read once for the whole batch, not per market. expect(getBrandAliasNamesStub).to.have.been.calledOnceWith(BRAND); // Both market creates receive the same aliases in their options arg (index 7). + // No modelIds + no generatePrompts → empty units → best-effort publish. const expectedOpts = { + modelIds: [], + generateTopics: false, + topicCap: 0, + standardTags: [], + projectTags: [], + publishMode: 'best-effort', brandAliases: ['Acme Inc'], brandUrlSources: { urls: [], socialAccounts: [], earnedContent: [] }, competitors: [], @@ -1318,25 +1456,38 @@ describe('SerenityController', () => { expect(secondCall.args[7].competitors).to.deep.equal(competitors); }); - it('activate 400s on an empty markets array', async () => { + it('activate provisions a single US/EN fallback project for an empty markets array + a brandDomain', async () => { + // A URL but no market: project creation is gated on the URL, so a single + // US/EN fallback project is provisioned (matches the direct-create default). + handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: {} }); const controller = SerenityController({ env: {} }, fakeLog(), {}); - const response = await controller.activate(fakeContext({ data: { markets: [] } })); - expect(response.status).to.equal(400); + const response = await controller.activate(fakeContext({ + data: { markets: [], brandDomain: 'x.com', brandNames: ['X'] }, + })); + expect(response.status).to.equal(200); + expect(handlers.handleCreateMarketSubworkspace).to.have.been.calledOnce; + const createBody = handlers.handleCreateMarketSubworkspace.firstCall.args[3]; + expect(createBody.market).to.equal('US'); + expect(createBody.languageCode).to.equal('en'); }); it('activate 400s when the markets array exceeds the cap (and does not provision)', async () => { const markets = Array.from({ length: 51 }, (_, i) => ({ market: 'us', languageCode: `l${i}` })); const controller = SerenityController({ env: {} }, fakeLog(), {}); - const response = await controller.activate(fakeContext({ data: { markets } })); + // A brandDomain routes to the project path where the cap is enforced. + const response = await controller.activate(fakeContext({ + data: { markets, brandDomain: 'x.com', brandNames: ['X'] }, + })); expect(response.status).to.equal(400); // Bounded before any upstream work — never reaches ensureSubworkspace. expect(handlers.handleCreateMarketSubworkspace).to.not.have.been.called; }); - it('activate records a thrown market as failed and keeps the published one live (no abort)', async () => { + it('activate records a thrown market as failed without aborting the batch, but stays pending (all-or-nothing)', async () => { // Market 1 publishes (201, live upstream); market 2 throws. The batch must - // NOT abort - the brand goes active (≥1 live) but the HTTP status is 207 - // because ≥1 market failed, surfacing the partial failure to the caller. + // NOT abort - both markets are reported per-market. But all-or-nothing means + // a single failure keeps the brand pending and returns a 502 (the live + // market 409s on the next retry, which converges). handlers.handleCreateMarketSubworkspace .onFirstCall().resolves({ status: 201, body: {} }) .onSecondCall().rejects(new ErrorWithStatusCode('upstream boom', 502)); @@ -1350,16 +1501,18 @@ describe('SerenityController', () => { markets: [{ market: 'us', languageCode: 'en' }, { market: 'de', languageCode: 'de' }], }, })); - // partial failure -> 207, even though the brand is active (one market live). - expect(response.status).to.equal(207); - const { markets } = await readBody(response); + // incomplete activation -> 502, brand stays pending. + expect(response.status).to.equal(502); + const { status, markets } = await readBody(response); + expect(status).to.equal('pending'); // both markets reported; the throwing one becomes a 502 entry, no URL leak. expect(markets).to.have.length(2); expect(markets[0].status).to.equal(201); expect(markets[1].status).to.equal(502); expect(markets[1].body.message).to.equal('Market activation failed'); - expect(brand.setStatus).to.have.been.calledWith('active'); - expect(brand.save).to.have.been.called; + // Brand never flips active and is not re-saved on the failure path. + expect(brand.setStatus).to.not.have.been.calledWith('active'); + expect(brand.save).to.not.have.been.called; }); it('activate defaults a statusless throw to 502 in the per-market result', async () => { @@ -1374,8 +1527,8 @@ describe('SerenityController', () => { markets: [{ market: 'us', languageCode: 'en' }], }, })); - // every market failed (no 201) -> 207 multi-status, brand stays pending. - expect(response.status).to.equal(207); + // every market failed (no 201) -> 502, brand stays pending. + expect(response.status).to.equal(502); const { markets } = await readBody(response); expect(markets[0].status).to.equal(502); expect(brand.setStatus).to.not.have.been.called; @@ -1401,7 +1554,7 @@ describe('SerenityController', () => { expect(brand.setStatus).to.have.been.calledOnceWith('active'); }); - it('activate returns 207 and stays pending when every market genuinely fails', async () => { + it('activate returns 502 and stays pending when every market genuinely fails', async () => { // A real failure status (502), NOT 409 - a 409 sliceExists means the market // is already live and counts as success (see the all-409 re-activate test). handlers.handleCreateMarketSubworkspace.resolves({ status: 502, body: {} }); @@ -1411,7 +1564,9 @@ describe('SerenityController', () => { brand, data: { brandDomain: 'x.com', brandNames: ['X'], markets: [{ market: 'us', languageCode: 'en' }] }, })); - expect(response.status).to.equal(207); + expect(response.status).to.equal(502); + const { status } = await readBody(response); + expect(status).to.equal('pending'); expect(brand.setStatus).to.not.have.been.called; }); @@ -1436,12 +1591,11 @@ describe('SerenityController', () => { expect(brand.setStatus).to.have.been.calledWith('active'); }); - it('activate emits SERENITY_ACTIVATE_SAVE_DIVERGENCE and returns 207 (markets live) when the status save fails', async () => { - // The markets are already live upstream; a failed status-save must NOT - // collapse to a 5xx that discards the per-market results. The seam mirrors - // deactivate's divergence guard: emit a distinct, alertable token and - // surface the live markets as a 207 multi-status (not a bare 200 that hides - // the brands.status='pending' / markets-live divergence). + it('activate emits SERENITY_ACTIVATE_SAVE_DIVERGENCE and returns 502 (stays pending) when the status save fails', async () => { + // The markets are live + the site is linked upstream, but persisting the + // 'active' flip fails → the brand stays pending (divergence). The seam emits + // a distinct, alertable token and surfaces a 502 with the per-market results + // (not a bare 5xx via mapError that would discard them). A retry converges. handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: {} }); const brand = makeBrandModel(); brand.save = sinon.stub().rejects(new Error('db down')); @@ -1451,17 +1605,66 @@ describe('SerenityController', () => { brand, data: { brandDomain: 'x.com', brandNames: ['X'], markets: [{ market: 'us', languageCode: 'en' }] }, })); - // markets are live -> 207 partial (save divergence), never a 5xx or bare 200. - expect(response.status).to.equal(207); + // save diverged -> 502, brand stays pending, per-market results preserved. + expect(response.status).to.equal(502); const { status, markets } = await readBody(response); - expect(status).to.equal('active'); + expect(status).to.equal('pending'); expect(markets).to.have.length(1); expect(markets[0].status).to.equal(201); - expect(brand.setStatus).to.have.been.calledWith('active'); // distinct, greppable token so the orphaned status is alertable. expect(log.error).to.have.been.calledWithMatch('SERENITY_ACTIVATE_SAVE_DIVERGENCE'); }); + it('activate stays pending with a 502 when every market is live but the brand_sites link fails', async () => { + // The brand_sites mirror (type='serenity') is a REQUIRED activation step: + // even with every market live, a failed site link keeps the brand pending so + // a retry re-establishes the link. ensureMarketSite returns null on failure. + handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: {} }); + ensureMarketSiteStub.resolves(null); + const setPendingSemrushProvisioning = sinon.stub(); + const brand = makeBrandModel({ + getPendingSemrushProvisioning: () => ({ + primaryUrl: 'https://acme.com', + markets: [{ market: 'us', languageCode: 'en' }], + }), + setPendingSemrushProvisioning, + }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ brand, data: { brandNames: ['X'] } })); + // Markets live but not linked → activation incomplete → 502, stays pending. + expect(response.status).to.equal(502); + const { status } = await readBody(response); + expect(status).to.equal('pending'); + expect(ensureMarketSiteStub).to.have.been.calledOnce; + // Never flips active; the stash is preserved (not cleared) for retry. + expect(brand.setStatus).to.not.have.been.calledWith('active'); + expect(setPendingSemrushProvisioning).to.not.have.been.called; + }); + + it('activate does NOT downgrade an already-active brand on a partial failure (207, stays active)', async () => { + // Reactivation of a live brand re-supplying markets in the body: one market + // fails. All-or-nothing keeps a PENDING brand pending, but an already-active + // brand is never downgraded — it stays active and reports 207 Multi-Status. + handlers.handleCreateMarketSubworkspace + .onFirstCall().resolves({ status: 409, body: { error: 'sliceExists' } }) + .onSecondCall().resolves({ status: 502, body: { error: 'serenityUpstreamError' } }); + const brand = makeBrandModel({ getStatus: () => 'active' }); + const controller = SerenityController({ env: {} }, fakeLog(), {}); + const response = await controller.activate(fakeContext({ + brand, + data: { + brandDomain: 'x.com', + brandNames: ['X'], + markets: [{ market: 'us', languageCode: 'en' }, { market: 'de', languageCode: 'de' }], + }, + })); + expect(response.status).to.equal(207); + const { status } = await readBody(response); + expect(status).to.equal('active'); + // Not flipped (no fullySucceeded) and not downgraded — the brand row is left as-is. + expect(brand.setStatus).to.not.have.been.called; + }); + it('deactivate decommissions the subworkspace, clears the pointer, and sets the brand pending', async () => { const brand = makeBrandModel({ getSemrushWorkspaceId: () => 'subworkspace-ws-1' }); const controller = SerenityController({ env: {} }, fakeLog(), {}); @@ -1753,25 +1956,32 @@ describe('SerenityController', () => { expect(handlers.handleUpdateModelsSubworkspace.firstCall.args[2]).to.deep.equal({}); }); - // Lines 747-748: activate — `ctx.data || {}` fallback and - // `Array.isArray(body.markets) ? ... : []` else-branch when markets is not an - // array. Both paths produce markets.length === 0 → 400. - it('activate falls back to {} body when ctx.data is absent (markets empty → 400)', async () => { + // activate — `ctx.data || {}` fallback and the `Array.isArray(body.markets) + // ? ... : storedMarkets` else-branch when markets is not an array. With no + // primary URL these route to the sub-workspace-only activation (200). + it('activate falls back to {} body when ctx.data is absent (sub-workspace-only → 200)', async () => { + handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: {} }); const controller = SerenityController({ env: {} }, fakeLog(), {}); const ctx = fakeContext(); ctx.data = undefined; const response = await controller.activate(ctx); - expect(response.status).to.equal(400); - const body = await response.json(); - expect(body.message).to.match(/markets must be a non-empty array/); + expect(response.status).to.equal(200); + const { status, markets } = await readBody(response); + expect(status).to.equal('active'); + expect(markets).to.deep.equal([]); + expect(handlers.handleCreateMarketSubworkspace).to.not.have.been.called; }); - it('activate treats a non-array markets value as empty and returns 400', async () => { + it('activate treats a non-array markets value as empty, falling back to US/EN when a brandDomain is present', async () => { + handlers.handleCreateMarketSubworkspace.resolves({ status: 201, body: {} }); const controller = SerenityController({ env: {} }, fakeLog(), {}); - const response = await controller.activate(fakeContext({ data: { markets: 'not-an-array' } })); - expect(response.status).to.equal(400); - const body = await response.json(); - expect(body.message).to.match(/markets must be a non-empty array/); + const response = await controller.activate(fakeContext({ + data: { markets: 'not-an-array', brandDomain: 'x.com', brandNames: ['X'] }, + })); + expect(response.status).to.equal(200); + const createBody = handlers.handleCreateMarketSubworkspace.firstCall.args[3]; + expect(createBody.market).to.equal('US'); + expect(createBody.languageCode).to.equal('en'); }); // Line 907: deactivate — `(ctx.env || env)?` — the env fallback fires when diff --git a/test/openapi-contract/serenity-api.test.js b/test/openapi-contract/serenity-api.test.js index a6f25cf1f2..d6908c15b6 100644 --- a/test/openapi-contract/serenity-api.test.js +++ b/test/openapi-contract/serenity-api.test.js @@ -389,6 +389,12 @@ describe('OpenAPI contract — /serenity/* endpoints', function specSuite() { }), getBrandCompetitors: () => Promise.resolve([]), }, + // activate's all-or-nothing flip REQUIRES the brand_sites mirror to + // succeed; stub it to a site id so the documented 200 (full success) + // shape is exercised rather than the 207/502 partial-failure paths. + '../../src/support/serenity/site-linkage.js': { + ensureMarketSite: () => Promise.resolve('site-x'), + }, }, )).default; diff --git a/test/support/brands-storage.test.js b/test/support/brands-storage.test.js index 8046e7084f..b25195118a 100644 --- a/test/support/brands-storage.test.js +++ b/test/support/brands-storage.test.js @@ -1028,6 +1028,73 @@ describe('brands-storage', () => { }); }); + it('preserves per-market modelIds (trimmed, non-empty) and omits the key when empty', async () => { + const client = createCapturingClient({ + brands: [ + { data: null, error: null }, + { data: { id: BRAND_ID, name: 'Test' }, error: null }, + { data: makeBrandRow({ name: 'Test' }), error: null }, + ], + }); + + await upsertBrand({ + organizationId: ORG_ID, + brand: { + name: 'Test', + status: 'pending', + pendingSemrushProvisioning: { + primaryUrl: 'https://acme.com', + markets: [ + { market: 'US', languageCode: 'en', modelIds: [' chatgpt ', '', 'perplexity'] }, + { market: 'DE', languageCode: 'de', modelIds: [' '] }, + { market: 'FR', languageCode: 'fr' }, + ], + }, + }, + postgrestClient: client, + }); + + const brandsUpsert = client.capturedCalls.upsert.find((c) => c.table === 'brands'); + expect(brandsUpsert.row.pending_semrush_provisioning).to.deep.equal({ + primaryUrl: 'https://acme.com', + markets: [ + { market: 'US', languageCode: 'en', modelIds: ['chatgpt', 'perplexity'] }, + { market: 'DE', languageCode: 'de' }, + { market: 'FR', languageCode: 'fr' }, + ], + }); + }); + + it('preserves an explicit generatePrompts flag, keeping a bare stash (no URL, no market) alive', async () => { + const client = createCapturingClient({ + brands: [ + { data: null, error: null }, + { data: { id: BRAND_ID, name: 'Test' }, error: null }, + { data: makeBrandRow({ name: 'Test' }), error: null }, + ], + }); + + await upsertBrand({ + organizationId: ORG_ID, + brand: { + name: 'Test', + status: 'pending', + // A bare no-prompt draft: no primary URL, no market — yet the explicit + // generatePrompts flag must keep the stash from collapsing to null so + // activation knows to provision a sub-workspace-only brand. + pendingSemrushProvisioning: { generatePrompts: false, markets: [] }, + }, + postgrestClient: client, + }); + + const brandsUpsert = client.capturedCalls.upsert.find((c) => c.table === 'brands'); + expect(brandsUpsert.row.pending_semrush_provisioning).to.deep.equal({ + primaryUrl: null, + markets: [], + generatePrompts: false, + }); + }); + it('drops invalid markets and a blank primaryUrl from pending_semrush_provisioning, nulling it when empty', async () => { const client = createCapturingClient({ brands: [ diff --git a/test/support/serenity/brand-provisioning.test.js b/test/support/serenity/brand-provisioning.test.js index d60643177e..c57fec6709 100644 --- a/test/support/serenity/brand-provisioning.test.js +++ b/test/support/serenity/brand-provisioning.test.js @@ -118,6 +118,29 @@ describe('provisionBrandSubworkspace', () => { expect(brandStub.getSemrushWorkspaceId()).to.equal(undefined); }); + it('falls back to US/EN and publishes best-effort when generateTopics is false with no market or models', async () => { + const { provisionBrandSubworkspace } = await loadModule({ + resolveWorkspaceId, handleCreateMarketSubworkspace, + }); + await provisionBrandSubworkspace(buildContext(), { + ...baseParams, + market: '', + languageCode: '', + modelIds: [], + generateTopics: false, + }); + const { args } = handleCreateMarketSubworkspace.firstCall; + const [, , , body, , , , options] = args; + // No market/language supplied → US/EN default slice. + expect(body.market).to.equal('US'); + expect(body.languageCode).to.equal('en'); + expect(body.name).to.equal('US - EN'); + // No prompts + no models → empty units → best-effort publish (leaves a draft). + expect(options.generateTopics).to.equal(false); + expect(options.topicCap).to.equal(0); + expect(options.publishMode).to.equal('best-effort'); + }); + it('forwards brandAliases to the handler for branded prompt classification', async () => { const { provisionBrandSubworkspace } = await loadModule({ resolveWorkspaceId, handleCreateMarketSubworkspace, @@ -214,11 +237,11 @@ describe('provisionBrandSubworkspace', () => { const { provisionBrandSubworkspace } = await loadModule({ resolveWorkspaceId, handleCreateMarketSubworkspace, }); + // market/languageCode are NOT in this list: they are optional (a no-prompt + // brand may omit them, falling back to US/EN) — see the fallback test below. for (const bad of [ { ...baseParams, brandName: '' }, { ...baseParams, brandId: '' }, - { ...baseParams, market: '' }, - { ...baseParams, languageCode: '' }, { ...baseParams, brandDomain: '' }, ]) { // eslint-disable-next-line no-await-in-loop