From 758cc3b592e88ef4e78ffe9b39a9cc4f88fdefcf Mon Sep 17 00:00:00 2001 From: DerekAgility Date: Thu, 15 Oct 2026 20:05:17 -0400 Subject: [PATCH 1/2] PROD-2729: carry page GEO fields through sync; retry without them on a GEO validation 400 Pages already spread the pulled page into the savePage payload, so `geo` and the new `seo` keys (title, canonicalUrl, robots, ogImage) pass through untouched, and the content batch processor's null-`seo` default only sets the five older keys. Tests now pin both, since an explicit null on save clears a stored value. When the Management API rejects a page's GEO data with a 400, the page is saved again without the offending properties (deleted, never nulled, so the target keeps its own values). The page counts as a success and the run summary carries a "GEO not copied" warning with code `geoNotCopied` and the API's message. Other 400s, non-400 errors and a failing retry behave as before. Also: verify compares `geo` on pages, and the run summary's page warnings heading no longer says "Component" only. Adds an integration round trip (static, dynamic and invalid-GEO fixture pages) that skips unless two instances and the fixture page IDs are configured. Co-Authored-By: Claude Opus 5.5 --- src/core/push.ts | 2 +- .../tests/content-batch-processor.test.ts | 72 +++++- src/lib/pushers/page-pusher/geo-fields.ts | 131 +++++++++++ src/lib/pushers/page-pusher/process-page.ts | 64 ++++-- .../page-pusher/tests/geo-fields.test.ts | 106 +++++++++ .../page-pusher/tests/process-page.test.ts | 198 +++++++++++++++++ src/lib/verify/check-page-zones.ts | 3 +- src/lib/verify/tests/check-page-zones.test.ts | 20 ++ .../geo/geo-roundtrip.integration.test.ts | 209 ++++++++++++++++++ src/types/sourceData.ts | 1 + 10 files changed, 787 insertions(+), 19 deletions(-) create mode 100644 src/lib/pushers/page-pusher/geo-fields.ts create mode 100644 src/lib/pushers/page-pusher/tests/geo-fields.test.ts create mode 100644 src/tests/geo/geo-roundtrip.integration.test.ts diff --git a/src/core/push.ts b/src/core/push.ts index 1143765..ebfac24 100644 --- a/src/core/push.ts +++ b/src/core/push.ts @@ -343,7 +343,7 @@ export class Push { // modules were left off the pushed page. Non-blocking: the modules return automatically // on a future push of the page once the content syncs. if (syncWarningDetails.length > 0) { - console.log(ansiColors.yellow(`\n Page Component Warnings (non-blocking, ${syncWarningDetails.length}):`)); + console.log(ansiColors.yellow(`\n Page Warnings (non-blocking, ${syncWarningDetails.length}):`)); syncWarningDetails.forEach(({ name, error, pageID, contentID, guid, locale }) => { const prefix = guid && locale ? `[${guid}][${locale}]` : guid ? `[${guid}]` : ""; console.log(ansiColors.yellow(` ${prefix} • ${name}: ${error}`)); diff --git a/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts b/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts index 38b26e0..477e16c 100644 --- a/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts +++ b/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts @@ -600,9 +600,7 @@ describe("ContentBatchProcessor.prepareContentPayloads — stale container mappi expect(result.skippedCount).toBe(0); expect(result.failedItems).toHaveLength(1); expect(result.failedItems[0].error).toEqual(expect.stringContaining("no longer exists on the target")); - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining("no longer exists on the target") - ); + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("no longer exists on the target")); }); it("ships the payload normally when the target container still exists", async () => { @@ -623,6 +621,74 @@ describe("ContentBatchProcessor.prepareContentPayloads — stale container mappi expect(result.prepared[0].source).toBe(item); expect(result.prepared[0].payload.properties.referenceName).toBe("ref-1"); }); + + // PROD-2729: GEO fields. On save, an omitted property keeps the target's value and null clears + // it, so the payload must carry `geo` and the GEO seo keys as pulled and never default them. + describe("GEO fields (PROD-2729)", () => { + async function prepareOne(sourceGuid: string, targetGuid: string, item: any) { + seedModelAndContainerMapping(tmpDir, sourceGuid, targetGuid, { createTargetContainerFile: true }); + const referenceMapper = new ContentItemMapper(sourceGuid, targetGuid, "en-us"); + const processor = new ContentBatchProcessor(makeConfig({ sourceGuid, targetGuid, referenceMapper })); + item.properties.referenceName = "ref-1"; + item.properties.definitionName = "TestModel"; + const result = await (processor as any).prepareContentPayloads([item], sourceGuid, targetGuid, makeLogger()); + expect(result.prepared).toHaveLength(1); + return result.prepared[0].payload; + } + + it("passes geo and the GEO seo keys through untouched", async () => { + const item = makeContentItem(1); + item.seo = { + metaDescription: "desc", + metaKeywords: null, + metaHTML: null, + menuVisible: true, + sitemapVisible: true, + title: "Independent title", + canonicalUrl: "https://example.com/a", + robots: { index: false, follow: true }, + ogImage: "https://cdn.example.com/og.png", + }; + item.geo = { + schemaType: "AboutPage", + summary: "Answer first.", + structuredData: { "@type": "AboutPage", name: "About" }, + ai: { llmsInclude: "include", crawl: "inherit" }, + }; + + const payload = await prepareOne("geo-pass-src", "geo-pass-tgt", item); + + expect(payload.seo).toEqual(item.seo); + expect(payload.geo).toEqual(item.geo); + }); + + it("defaults only the five existing seo keys when seo is null, and keeps geo", async () => { + const item = makeContentItem(1); + item.seo = null; + item.geo = { schemaType: "FAQPage", summary: "Kept." }; + + const payload = await prepareOne("geo-nullseo-src", "geo-nullseo-tgt", item); + + expect(payload.seo).toEqual({ + metaDescription: null, + metaKeywords: null, + metaHTML: null, + menuVisible: null, + sitemapVisible: null, + }); + // The GEO seo keys are omitted (keep the target's value), never sent as null (clear it). + for (const key of ["title", "canonicalUrl", "robots", "ogImage"]) { + expect(payload.seo).not.toHaveProperty(key); + } + expect(payload.geo).toEqual({ schemaType: "FAQPage", summary: "Kept." }); + }); + + it("omits geo (rather than sending null) when the pulled item has none", async () => { + const item = makeContentItem(1); + const payload = await prepareOne("geo-none-src", "geo-none-tgt", item); + expect(payload).not.toHaveProperty("geo"); + }); + }); }); // ─── PROD-2542: payload/source alignment ────────────────────────────────────── diff --git a/src/lib/pushers/page-pusher/geo-fields.ts b/src/lib/pushers/page-pusher/geo-fields.ts new file mode 100644 index 0000000..28ca051 --- /dev/null +++ b/src/lib/pushers/page-pusher/geo-fields.ts @@ -0,0 +1,131 @@ +/** + * PROD-2729: page GEO/AEO fields during a sync. + * + * A page can carry a top-level `geo` block (schemaType, summary, structuredData, ai) and four + * newer `seo` keys (title, canonicalUrl, robots, ogImage). The CLI needs no feature work for them: + * they arrive on the pulled page and are spread into the savePage payload untouched. Two rules + * matter on the write side, both from the Management API's three-state contract: + * + * - an omitted property keeps what the target has stored, and + * - an explicit `null` clears it. + * + * So the CLI must never default these properties to `null`, and when it has to leave GEO data out + * it deletes the keys rather than nulling them. + * + * The Management API rejects invalid GEO input (e.g. malformed `geo.structuredData`) with a 400 + * whose message names each offending field path ("geo.structuredData: is not valid JSON (...)"). + * Data saved through a path that doesn't validate can therefore fail when synced to another + * instance. One bad JSON-LD block must never fail a sync, so the page is saved again without the + * offending GEO properties (the target keeps its own values) and the skip is reported as a warning. + */ + +/** The `seo` keys added for GEO/AEO. The five older keys are not GEO fields. */ +export const GEO_SEO_KEYS = ["title", "canonicalUrl", "robots", "ogImage"] as const; + +/** Stable warning code for the run summary / --jsonSummary. */ +export const GEO_NOT_COPIED_CODE = "geoNotCopied"; + +export interface GeoValidationError { + /** The validation message from the Management API. */ + message: string; + /** + * What to leave out of the retry: "geo" for anything under `geo.*`, or "seo." for one of + * GEO_SEO_KEYS. Deduplicated, in first-seen order. + */ + strip: string[]; + /** True when every offending path is `geo.structuredData`. */ + structuredDataOnly: boolean; +} + +const GEO_PATH = + /(?:^|[\s"'$.[])((?:geo(?:\.[A-Za-z]+)*)|(?:seo\.(?:title|canonicalUrl|robots|ogImage)(?:\.[A-Za-z]+)*))\s*:/gi; + +function canonicalSeoKey(key: string): string | undefined { + return GEO_SEO_KEYS.find((k) => k.toLowerCase() === key.toLowerCase()); +} + +/** Map one field path ("geo.ai.crawl", "$.seo.Title") to what the retry leaves out. */ +function stripTargetForPath(rawPath: string): { strip: string; path: string } | undefined { + const path = rawPath.replace(/^\$\./, ""); + const parts = path.split("."); + if (parts[0].toLowerCase() === "geo") return { strip: "geo", path: path.toLowerCase() }; + if (parts[0].toLowerCase() === "seo" && parts[1]) { + const key = canonicalSeoKey(parts[1]); + if (key) return { strip: `seo.${key}`, path: path.toLowerCase() }; + } + return undefined; +} + +/** The HTTP response behind an error thrown by @agility/management-sdk (it wraps the AxiosError). */ +function responseOf(error: any): any { + return error?.innerError?.response ?? error?.response ?? error?.innerError?.innerError?.response; +} + +/** + * If `error` is a Management API 400 rejecting GEO fields, describe it; otherwise undefined. + * + * Handles both 400 shapes the Management API produces: + * - its own validation: `{ statusCode: 400, exceptionMessage: "geo.structuredData: ... geo.schemaType: ..." }` + * - ASP.NET model binding: `{ status: 400, title: "...", errors: { "$.geo.schemaType": ["..."] } }` + */ +export function getGeoValidationError(error: any): GeoValidationError | undefined { + const response = responseOf(error); + if (!response || response.status !== 400) return undefined; + + const data = response.data; + const pieces: string[] = []; + if (typeof data === "string") pieces.push(data); + if (data && typeof data === "object") { + if (typeof data.exceptionMessage === "string") pieces.push(data.exceptionMessage); + if (typeof data.message === "string") pieces.push(data.message); + if (data.errors && typeof data.errors === "object") { + for (const [key, value] of Object.entries(data.errors)) { + const messages = Array.isArray(value) ? value.join(" ") : String(value); + pieces.push(`${key}: ${messages}`); + } + } + } + const message = pieces.join(" ").trim(); + if (!message) return undefined; + + const strip: string[] = []; + const paths: string[] = []; + const pattern = new RegExp(GEO_PATH.source, GEO_PATH.flags); + let match: RegExpExecArray | null; + while ((match = pattern.exec(message)) !== null) { + const target = stripTargetForPath(match[1]); + if (!target) continue; + paths.push(target.path); + if (!strip.includes(target.strip)) strip.push(target.strip); + } + if (strip.length === 0) return undefined; + + return { + message, + strip, + structuredDataOnly: paths.every((p) => p.startsWith("geo.structureddata")), + }; +} + +/** + * A copy of `payload` without the GEO properties named in `strip`. The keys are deleted, never + * set to null: omitted means "keep what the target has", null would clear it. + */ +export function withoutGeoFields>(payload: T, strip: string[]): T { + const copy: any = { ...payload }; + for (const entry of strip) { + if (entry === "geo") { + delete copy.geo; + } else if (entry.startsWith("seo.") && copy.seo && typeof copy.seo === "object") { + copy.seo = { ...copy.seo }; + delete copy.seo[entry.slice(4)]; + } + } + return copy; +} + +/** The warning text recorded for a page whose GEO data was not copied. */ +export function geoNotCopiedMessage(validation: GeoValidationError): string { + const what = validation.structuredDataOnly ? "invalid structured data" : "invalid GEO fields"; + return `GEO not copied: ${what} (${validation.strip.join(", ")} left out; the target keeps its existing values). ${validation.message}`; +} diff --git a/src/lib/pushers/page-pusher/process-page.ts b/src/lib/pushers/page-pusher/process-page.ts index 927c19d..0afdfb8 100644 --- a/src/lib/pushers/page-pusher/process-page.ts +++ b/src/lib/pushers/page-pusher/process-page.ts @@ -17,6 +17,7 @@ import { } from "core/state"; import { FailureDetail, PageModuleExtended } from "types/sourceData"; import { preflightReport } from "../../preflight/preflight-report"; +import { GEO_NOT_COPIED_CODE, geoNotCopiedMessage, getGeoValidationError, withoutGeoFields } from "./geo-fields"; interface Props { channel: string; @@ -397,7 +398,10 @@ export async function processPage({ const pageJSON = JSON.stringify(page, null, 2); const pageCopy = JSON.parse(pageJSON) as mgmtApi.PageItem; // Create a copy to avoid modifying original - const payload: any = { + // PROD-2729: the spread carries `geo` and the GEO `seo` keys (title, canonicalUrl, robots, + // ogImage) through untouched. Nothing here may default them to null: on save, an omitted + // property keeps the target's value but null clears it. + let payload: any = { ...pageCopy, // If local target file is missing but mapping exists, use the known target container ID // to force an UPDATE instead of INSERT (prevents duplicate name server errors) @@ -440,17 +444,46 @@ export async function processPage({ const otherLocale = mappingToOtherLocale ? mappingToOtherLocale.OtherLanguageCode : null; // Save the page with returnBatchID flag for consistent batch processing - const savePageResponse = await apiClient.pageMethods.savePage( - payload, - targetGuid, - locale, - parentIDArg, - placeBeforeIDArg, - true, - pageIDInOtherLocale, - otherLocale, - true - ); + const savePage = (body: any) => + apiClient.pageMethods.savePage( + body, + targetGuid, + locale, + parentIDArg, + placeBeforeIDArg, + true, + pageIDInOtherLocale, + otherLocale, + true + ); + + // PROD-2729: a non-blocking notice when the page's GEO data couldn't be copied. + const geoWarnings: FailureDetail[] = []; + let savePageResponse: any; + try { + savePageResponse = await savePage(payload); + } catch (saveError: any) { + // The Management API rejects invalid GEO input (e.g. malformed structuredData) with a 400. + // Keep its validation strict and save the page again without the offending GEO properties, + // so the target keeps its own values and one bad JSON-LD block never fails the sync. + const geoValidation = getGeoValidationError(saveError); + if (!geoValidation) throw saveError; + + const message = geoNotCopiedMessage(geoValidation); + payload = withoutGeoFields(payload, geoValidation.strip); + savePageResponse = await savePage(payload); + + geoWarnings.push({ + name: page.name || `Page ${page.pageID}`, + error: message, + type: "page", + pageID: page.pageID, + guid: sourceGuid, + locale, + code: GEO_NOT_COPIED_CODE, + }); + logger.page.skipped(page, message, locale, channel, targetGuid); + } // Process the response - with returnBatchID=true, we should always get a batch ID if (Array.isArray(savePageResponse) && savePageResponse.length > 0) { @@ -558,7 +591,9 @@ export async function processPage({ logger.page.created(page, "created", locale, channel, targetGuid); } // PROD-2316: surface any dropped-module notices alongside the success. - return { status: "success", warnings: droppedModules.length > 0 ? droppedModules : undefined }; + // PROD-2729: and any GEO data left out of the save. + const pageWarnings = [...droppedModules, ...geoWarnings]; + return { status: "success", warnings: pageWarnings.length > 0 ? pageWarnings : undefined }; } else { let errorMsg: string; if (batchFailedItems.length > 0 && batchFailedItems[0].error) { @@ -580,7 +615,8 @@ export async function processPage({ channel, targetGuid ); - return { status: "failure", error: errorMsg, warnings: droppedModules.length > 0 ? droppedModules : undefined }; + const pageWarnings = [...droppedModules, ...geoWarnings]; + return { status: "failure", error: errorMsg, warnings: pageWarnings.length > 0 ? pageWarnings : undefined }; } } else { const errorMsg = "Unexpected response format"; diff --git a/src/lib/pushers/page-pusher/tests/geo-fields.test.ts b/src/lib/pushers/page-pusher/tests/geo-fields.test.ts new file mode 100644 index 0000000..577c5a9 --- /dev/null +++ b/src/lib/pushers/page-pusher/tests/geo-fields.test.ts @@ -0,0 +1,106 @@ +import { getGeoValidationError, withoutGeoFields, geoNotCopiedMessage } from "../geo-fields"; + +// @agility/management-sdk wraps the AxiosError in its own Exception (`innerError`). +function sdkError(status: number, data: any): any { + return { message: "Unable to create page.", innerError: { response: { status, data } } }; +} + +describe("getGeoValidationError (PROD-2729)", () => { + it("reads the Management API's own 400 (exceptionMessage with field paths)", () => { + const result = getGeoValidationError( + sdkError(400, { + statusCode: 400, + exceptionMessage: + "geo.schemaType: must be one of WebPage, AboutPage (was 'Article'). geo.structuredData: must be a JSON object or array.", + additionalParameters: null, + }) + ); + expect(result).toEqual({ + message: expect.stringContaining("geo.schemaType"), + strip: ["geo"], + structuredDataOnly: false, + }); + }); + + it("flags a structuredData-only rejection", () => { + const result = getGeoValidationError( + sdkError(400, { exceptionMessage: "geo.structuredData: is not valid JSON (bad token)." }) + ); + expect(result?.structuredDataOnly).toBe(true); + expect(result?.strip).toEqual(["geo"]); + }); + + it("maps GEO seo keys to their own strip entries, and ignores older seo keys", () => { + const result = getGeoValidationError( + sdkError(400, { + exceptionMessage: + "seo.title: must be at most 300 characters (was 412). seo.ogImage: must be an absolute http or https URL, or a path starting with '/'. geo.ai.crawl: must be one of inherit, block (was 'x').", + }) + ); + expect(result?.strip).toEqual(["seo.title", "seo.ogImage", "geo"]); + }); + + it("reads an ASP.NET model-binding 400 (errors keyed by JSON path)", () => { + const result = getGeoValidationError( + sdkError(400, { + title: "One or more validation errors occurred.", + status: 400, + errors: { "$.geo.schemaType": ["The JSON value could not be converted to System.String."] }, + }) + ); + expect(result?.strip).toEqual(["geo"]); + }); + + it("also accepts an AxiosError thrown directly", () => { + const result = getGeoValidationError({ + response: { status: 400, data: "geo.summary: must be at most 2000 characters (was 2400)." }, + }); + expect(result?.strip).toEqual(["geo"]); + }); + + it("returns undefined for a 400 that doesn't name a GEO field", () => { + expect( + getGeoValidationError(sdkError(400, { exceptionMessage: "seo.metaDescription: too long." })) + ).toBeUndefined(); + expect(getGeoValidationError(sdkError(400, { exceptionMessage: "Invalid page name." }))).toBeUndefined(); + }); + + it("returns undefined for other statuses and plain errors", () => { + expect(getGeoValidationError(sdkError(500, { exceptionMessage: "geo.summary: boom" }))).toBeUndefined(); + expect(getGeoValidationError(new Error("geo.summary: network"))).toBeUndefined(); + expect(getGeoValidationError(undefined)).toBeUndefined(); + }); +}); + +describe("withoutGeoFields (PROD-2729)", () => { + const payload = { + pageID: 5, + seo: { metaDescription: "d", title: "t", canonicalUrl: "/c", robots: { index: true, follow: true } }, + geo: { schemaType: "WebPage" }, + }; + + it("deletes the named keys instead of nulling them, without mutating the input", () => { + const result = withoutGeoFields(payload, ["geo", "seo.title"]); + expect(result).not.toHaveProperty("geo"); + expect(result.seo).toEqual({ metaDescription: "d", canonicalUrl: "/c", robots: { index: true, follow: true } }); + expect(payload.geo).toEqual({ schemaType: "WebPage" }); + expect(payload.seo.title).toBe("t"); + }); + + it("copes with a payload that has no seo", () => { + expect(withoutGeoFields({ pageID: 1 } as any, ["seo.title"])).toEqual({ pageID: 1 }); + }); +}); + +describe("geoNotCopiedMessage (PROD-2729)", () => { + it("names the reason, what was left out, and the API's message", () => { + const message = geoNotCopiedMessage({ + message: "geo.structuredData: is not valid JSON.", + strip: ["geo"], + structuredDataOnly: true, + }); + expect(message).toMatch(/^GEO not copied: invalid structured data/); + expect(message).toContain("geo left out"); + expect(message).toContain("geo.structuredData: is not valid JSON."); + }); +}); diff --git a/src/lib/pushers/page-pusher/tests/process-page.test.ts b/src/lib/pushers/page-pusher/tests/process-page.test.ts index d1af91b..16ede16 100644 --- a/src/lib/pushers/page-pusher/tests/process-page.test.ts +++ b/src/lib/pushers/page-pusher/tests/process-page.test.ts @@ -806,3 +806,201 @@ describe("countModulesReferencingFailedContent (PROD-2542)", () => { expect(countModulesReferencingFailedContent(makePage({ zones: undefined }), "en-us")).toBe(0); }); }); + +// ─── PROD-2729: GEO fields ──────────────────────────────────────────────────── + +describe("processPage — GEO fields (PROD-2729)", () => { + const GEO = { + schemaType: "AboutPage", + summary: "Answer first.", + structuredData: { "@type": "AboutPage", name: "About us" }, + ai: { llmsInclude: "include", crawl: "inherit" }, + }; + const SEO = { + metaDescription: "desc", + metaKeywords: "", + metaHTML: "", + menuVisible: true, + sitemapVisible: true, + title: "Independent title", + canonicalUrl: "https://example.com/about", + robots: { index: false, follow: true }, + ogImage: "https://cdn.example.com/og.png", + }; + + // What @agility/management-sdk throws for a non-2xx savePage: its Exception wrapping the AxiosError. + function sdkError(status: number, data: any): Error { + const axiosError: any = new Error(`Request failed with status code ${status}`); + axiosError.response = { status, data }; + const err: any = new Error(`Unable to create page. AxiosError: Request failed with status code ${status}`); + err.innerError = axiosError; + return err; + } + + function geoValidation400(exceptionMessage: string): Error { + return sdkError(400, { statusCode: 400, exceptionMessage, additionalParameters: null }); + } + + beforeEach(() => { + const { TemplateMapper } = require("lib/mappers/template-mapper"); + TemplateMapper.mockImplementation(() => ({ + getTemplateMappingByPageTemplateName: jest.fn().mockReturnValue({ ref: "Main" }), + getMappedEntity: jest.fn().mockReturnValue({ contentSectionDefinitions: [] }), + })); + mockExtract.mockReturnValue({ + successfulItems: [{ newId: 200, newItem: { processedItemVersionID: 5 } }], + failedItems: [], + }); + }); + + it("passes geo and the GEO seo keys through to savePage untouched", async () => { + const apiClient = makeApiClient(); + const page = makePage({ seo: SEO, geo: GEO }); + + const result = await processPage(makeProps({ apiClient, page })); + + expect(result.status).toBe("success"); + expect(result.warnings).toBeUndefined(); + const savedPayload = (apiClient.pageMethods.savePage as jest.Mock).mock.calls[0][0]; + expect(savedPayload.seo).toEqual(SEO); + expect(savedPayload.geo).toEqual(GEO); + }); + + it("passes a dynamic page's geo through as well", async () => { + const apiClient = makeApiClient(); + const page = makePage({ + pageType: "static", + dynamic: { referenceName: "posts", fieldName: "slug", titleFormula: "title" }, + seo: SEO, + geo: { schemaType: "CollectionPage" }, + }); + + const result = await processPage(makeProps({ apiClient, page })); + + expect(result.status).toBe("success"); + const savedPayload = (apiClient.pageMethods.savePage as jest.Mock).mock.calls[0][0]; + expect(savedPayload.geo).toEqual({ schemaType: "CollectionPage" }); + expect(savedPayload.seo.title).toBe("Independent title"); + }); + + it("does not add geo or GEO seo keys (as null or otherwise) when the pulled page has none", async () => { + const apiClient = makeApiClient(); + const page = makePage({ + seo: { metaDescription: "d", metaKeywords: "", metaHTML: "", menuVisible: true, sitemapVisible: true }, + }); + + await processPage(makeProps({ apiClient, page })); + + const savedPayload = (apiClient.pageMethods.savePage as jest.Mock).mock.calls[0][0]; + expect(savedPayload).not.toHaveProperty("geo"); + for (const key of ["title", "canonicalUrl", "robots", "ogImage"]) { + expect(savedPayload.seo).not.toHaveProperty(key); + } + }); + + it("retries without geo on a GEO validation 400, keeps the page a success and reports it", async () => { + const apiClient = makeApiClient(); + apiClient.pageMethods.savePage = jest + .fn() + .mockRejectedValueOnce( + geoValidation400("geo.structuredData: is not valid JSON ('o' is an invalid start of a value.).") + ) + .mockResolvedValueOnce([100]); + const logger = makeLogger(); + const page = makePage({ pageID: 7, name: "About", seo: SEO, geo: { ...GEO, structuredData: "{oops" } }); + + const result = await processPage(makeProps({ apiClient, page, logger })); + + expect(result.status).toBe("success"); + const calls = (apiClient.pageMethods.savePage as jest.Mock).mock.calls; + expect(calls).toHaveLength(2); + expect(calls[0][0].geo).toBeDefined(); + // The retry omits geo (keep the target's value); it must not send geo: null (clear it). + expect(calls[1][0]).not.toHaveProperty("geo"); + // Only the offending block is left out: the valid GEO seo keys still copy. + expect(calls[1][0].seo).toEqual(SEO); + // Every other savePage argument is unchanged on the retry. + expect(calls[1].slice(1)).toEqual(calls[0].slice(1)); + + expect(result.warnings).toHaveLength(1); + expect(result.warnings![0]).toEqual( + expect.objectContaining({ + name: "About", + type: "page", + pageID: 7, + locale: "en-us", + guid: "src", + code: "geoNotCopied", + }) + ); + expect(result.warnings![0].error).toContain("GEO not copied: invalid structured data"); + expect(result.warnings![0].error).toContain("geo.structuredData: is not valid JSON"); + expect(logger.page.skipped).toHaveBeenCalledWith( + page, + expect.stringContaining("GEO not copied"), + "en-us", + "website", + "tgt" + ); + expect(logger.page.error).not.toHaveBeenCalled(); + }); + + it("leaves out only the named GEO seo key when that is what the 400 rejects", async () => { + const apiClient = makeApiClient(); + apiClient.pageMethods.savePage = jest + .fn() + .mockRejectedValueOnce( + geoValidation400("seo.canonicalUrl: must be an absolute http or https URL, or a path starting with '/'.") + ) + .mockResolvedValueOnce([100]); + const page = makePage({ seo: { ...SEO, canonicalUrl: "not a url" }, geo: GEO }); + + const result = await processPage(makeProps({ apiClient, page })); + + expect(result.status).toBe("success"); + const retry = (apiClient.pageMethods.savePage as jest.Mock).mock.calls[1][0]; + expect(retry.geo).toEqual(GEO); + expect(retry.seo).not.toHaveProperty("canonicalUrl"); + expect(retry.seo.title).toBe("Independent title"); + expect(result.warnings![0].error).toContain("GEO not copied: invalid GEO fields"); + }); + + it("does not retry a 400 that isn't about GEO fields", async () => { + const apiClient = makeApiClient(); + apiClient.pageMethods.savePage = jest + .fn() + .mockRejectedValue(sdkError(400, { statusCode: 400, exceptionMessage: "The page name is required." })); + const page = makePage({ seo: SEO, geo: GEO }); + + const result = await processPage(makeProps({ apiClient, page })); + + expect(result.status).toBe("failure"); + expect(apiClient.pageMethods.savePage).toHaveBeenCalledTimes(1); + }); + + it("does not retry a non-400 error even if it mentions geo", async () => { + const apiClient = makeApiClient(); + apiClient.pageMethods.savePage = jest + .fn() + .mockRejectedValue(sdkError(500, { statusCode: 500, exceptionMessage: "geo.summary: boom" })); + + const result = await processPage(makeProps({ apiClient, page: makePage({ geo: GEO }) })); + + expect(result.status).toBe("failure"); + expect(apiClient.pageMethods.savePage).toHaveBeenCalledTimes(1); + }); + + it("fails the page (once) when the retry without geo fails too", async () => { + const apiClient = makeApiClient(); + apiClient.pageMethods.savePage = jest + .fn() + .mockRejectedValueOnce(geoValidation400("geo.schemaType: must be one of WebPage, AboutPage (was 'Article').")) + .mockRejectedValueOnce(new Error("network error")); + + const result = await processPage(makeProps({ apiClient, page: makePage({ geo: { schemaType: "Article" } }) })); + + expect(result.status).toBe("failure"); + expect(result.error).toContain("network error"); + expect(apiClient.pageMethods.savePage).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/lib/verify/check-page-zones.ts b/src/lib/verify/check-page-zones.ts index 5121bd0..56d8ee8 100644 --- a/src/lib/verify/check-page-zones.ts +++ b/src/lib/verify/check-page-zones.ts @@ -18,7 +18,7 @@ * a sourceDefect and left out of the expectation. * * Page fields compared: title, menuText, name, pageType, template (through the template mapping), - * redirectUrl, visible, seo, scripts, securePage. A different workflow state is a warning + * redirectUrl, visible, seo, geo (PROD-2729), scripts, securePage. A different workflow state is a warning * (publishStateDifference), as for content. */ import { Finding, FindingCollector } from "./verify-report"; @@ -38,6 +38,7 @@ export const COMPARED_PAGE_FIELDS = [ "redirectUrl", "visible", "seo", + "geo", "scripts", "securePage", ] as const; diff --git a/src/lib/verify/tests/check-page-zones.test.ts b/src/lib/verify/tests/check-page-zones.test.ts index 05c7926..96a5a4a 100644 --- a/src/lib/verify/tests/check-page-zones.test.ts +++ b/src/lib/verify/tests/check-page-zones.test.ts @@ -228,4 +228,24 @@ describe("checkPages — page fields", () => { }); expect(findings).toEqual([]); }); + + // PROD-2729: geo and the GEO seo keys are part of a page, so a sync that didn't copy them shows. + it("compares geo and the GEO seo keys", () => { + const geo = { schemaType: "AboutPage", structuredData: { "@type": "AboutPage" } }; + const seo = { metaDescription: "d", title: "T", robots: { index: false, follow: true } }; + expect( + run({ sourceZones: {}, targetZones: {}, sourcePage: { seo, geo }, targetPage: { seo, geo } }).findings + ).toEqual([]); + + const { findings } = run({ + sourceZones: {}, + targetZones: {}, + sourcePage: { seo, geo }, + targetPage: { seo: { metaDescription: "d" } }, + }); + expect(findings.map((f) => [f.category, f.path])).toEqual([ + ["pageFieldMismatch", "seo"], + ["pageFieldMismatch", "geo"], + ]); + }); }); diff --git a/src/tests/geo/geo-roundtrip.integration.test.ts b/src/tests/geo/geo-roundtrip.integration.test.ts new file mode 100644 index 0000000..18377c1 --- /dev/null +++ b/src/tests/geo/geo-roundtrip.integration.test.ts @@ -0,0 +1,209 @@ +/** + * PROD-2729: round-trip page GEO/AEO fields between two instances with a real `agility sync`. + * + * What it proves: + * 1. A static page and a dynamic page carrying `geo` and the GEO `seo` keys (title, canonicalUrl, + * robots, ogImage) arrive on the target with the same values. + * 2. A page whose GEO data the Management API rejects (e.g. invalid structuredData) does not fail + * the sync: the page is saved without it, the target keeps its existing GEO data, and the run + * summary carries a `geoNotCopied` warning for that page. + * + * It can only pass once Phase 1 rollout stage 3 is on the environment the instances live on: the + * CLI pulls pages through the Fetch API, which only carries GEO data from stage 3, and the + * Management API must accept and return it (stage 2, with both instances in + * `AppSettings:GeoFieldsInstances`). It SKIPS unless every required variable below is set. + * + * Fixtures (set up by hand on the SOURCE instance, then published, because the test never writes + * to the source): + * - GEO_STATIC_PAGE_ID: a static page with `geo` and at least one GEO seo key set. + * - GEO_DYNAMIC_PAGE_ID: a dynamic page with the same. + * - GEO_INVALID_PAGE_ID: a page whose GEO data the Management API rejects, e.g. structuredData + * that isn't a JSON object/array, or schemaType "Article". The Management API won't save that, + * so write it through a path that doesn't validate (the classic editor, or the DB on dev). + * + * Required env vars: + * - AGILITY_TOKEN personal access token with access to both instances + * - GEO_SOURCE_GUID source instance GUID (dev, e.g. "xxxxxxxx-d") + * - GEO_TARGET_GUID target instance GUID (dev) + * - GEO_STATIC_PAGE_ID, GEO_DYNAMIC_PAGE_ID, GEO_INVALID_PAGE_ID source page IDs + * Optional: GEO_LOCALE (default en-us), GEO_CHANNEL (default website), GEO_WORK_DIR (where the CLI + * keeps its files and mappings between runs; default a fixed folder under the OS temp dir). + * + * Run: npm run build && npm run test:integration -- src/tests/geo + */ +import { spawnSync } from "child_process"; +import * as fs from "fs"; +import * as os from "os"; +import * as path from "path"; +import * as mgmtApi from "@agility/management-sdk"; +import { Auth } from "core/auth"; +import { resetState, setState } from "core/state"; +import { PageMapper } from "lib/mappers/page-mapper"; + +const env = process.env; +const TOKEN = env.AGILITY_TOKEN; +const SOURCE = env.GEO_SOURCE_GUID; +const TARGET = env.GEO_TARGET_GUID; +const LOCALE = env.GEO_LOCALE || "en-us"; +const CHANNEL = env.GEO_CHANNEL || "website"; +const STATIC_PAGE_ID = Number(env.GEO_STATIC_PAGE_ID); +const DYNAMIC_PAGE_ID = Number(env.GEO_DYNAMIC_PAGE_ID); +const INVALID_PAGE_ID = Number(env.GEO_INVALID_PAGE_ID); + +const configured = !!TOKEN && !!SOURCE && !!TARGET && STATIC_PAGE_ID > 0 && DYNAMIC_PAGE_ID > 0 && INVALID_PAGE_ID > 0; + +const describeIfConfigured = configured ? describe : describe.skip; + +const GEO_SEO_KEYS = ["title", "canonicalUrl", "robots", "ogImage"]; +const CLI = path.resolve(__dirname, "../../../dist/index.js"); + +/** The GEO part of a page: `geo` plus the GEO seo keys, with unset keys left out. */ +function geoPart(page: any): { geo: any; seo: Record } { + const seo: Record = {}; + for (const key of GEO_SEO_KEYS) { + if (page?.seo?.[key] !== undefined && page?.seo?.[key] !== null) seo[key] = page.seo[key]; + } + return { geo: page?.geo ?? null, seo }; +} + +describeIfConfigured("GEO fields round trip (PROD-2729)", () => { + jest.setTimeout(20 * 60 * 1000); + + const auth = new Auth(); + let workDir: string; + let source: mgmtApi.ApiClient; + let target: mgmtApi.ApiClient; + + function client(guid: string): mgmtApi.ApiClient { + return new mgmtApi.ApiClient({ token: TOKEN!, baseUrl: auth.determineBaseUrl(guid) } as mgmtApi.Options); + } + + /** The source page as the CLI will pull it: from the Fetch API (preview), not the Management API. */ + async function fetchSourcePage(pageID: number): Promise { + const keyResponse = await fetch(`${auth.determineBaseUrl(SOURCE)}/oauth/GetPreviewKey?guid=${SOURCE}`, { + headers: { Authorization: `Bearer ${TOKEN}`, "Cache-Control": "no-cache" }, + }); + if (!keyResponse.ok) throw new Error(`GetPreviewKey failed: HTTP ${keyResponse.status}`); + const key = (await keyResponse.text()).replace(/^"|"$/g, ""); + const response = await fetch( + `${auth.determineFetchUrl(SOURCE)}/${SOURCE}/preview/${LOCALE}/page/${pageID}?contentLinkDepth=0`, + { headers: { APIKey: key } } + ); + if (!response.ok) throw new Error(`Fetch API page ${pageID}: HTTP ${response.status}`); + return response.json(); + } + + function targetPageID(sourcePageID: number): number | undefined { + setState({ rootPath: path.join(workDir, "agility-files") }); + const mapping = new PageMapper(SOURCE!, TARGET!, LOCALE).getPageMappingByPageID(sourcePageID, "source"); + return mapping?.targetPageID > 0 ? mapping.targetPageID : undefined; + } + + function runSync(summaryPath: string) { + fs.rmSync(summaryPath, { force: true }); + const result = spawnSync( + process.execPath, + [ + CLI, + "sync", + `--sourceGuid=${SOURCE}`, + `--targetGuid=${TARGET}`, + `--token=${TOKEN}`, + `--locales=${LOCALE}`, + `--channel=${CHANNEL}`, + `--pages=${[STATIC_PAGE_ID, DYNAMIC_PAGE_ID, INVALID_PAGE_ID].join(",")}`, + // The fixtures may have been synced before; overwrite so a target-side edit can't skip them. + "--overwrite", + "--headless", + `--jsonSummary=${summaryPath}`, + ], + { cwd: workDir, encoding: "utf8", env: { ...process.env }, maxBuffer: 64 * 1024 * 1024 } + ); + if (!fs.existsSync(summaryPath)) { + throw new Error(`sync wrote no summary (exit ${result.status}).\n${result.stdout}\n${result.stderr}`); + } + return { exitCode: result.status, summary: JSON.parse(fs.readFileSync(summaryPath, "utf8")) }; + } + + let sourcePages: Record; + let invalidTargetBefore: { geo: any; seo: Record }; + let run: { exitCode: number | null; summary: any }; + + beforeAll(async () => { + if (!fs.existsSync(CLI)) throw new Error(`Build the CLI first (npm run build): ${CLI} is missing`); + resetState(); + // A stable folder, kept between runs: the CLI's mapping files live in it, and without them + // every run would create the fixture pages on the target again. + workDir = env.GEO_WORK_DIR || path.join(os.tmpdir(), `agility-geo-roundtrip-${SOURCE}-${TARGET}`); + fs.mkdirSync(workDir, { recursive: true }); + source = client(SOURCE!); + target = client(TARGET!); + + // Precondition: the source pages carry GEO data in the Fetch API output (stage 3). + sourcePages = {}; + for (const id of [STATIC_PAGE_ID, DYNAMIC_PAGE_ID, INVALID_PAGE_ID]) { + sourcePages[id] = await fetchSourcePage(id); + if (!sourcePages[id]?.geo) { + throw new Error( + `Source page ${id} has no geo in the Fetch API output. Is stage 3 deployed, and was the page re-published after its GEO data was saved?` + ); + } + } + // Sanity: the dynamic fixture really is dynamic, as the Management API sees it. + const dynamicPage = await source.pageMethods.getPage(DYNAMIC_PAGE_ID, SOURCE!, LOCALE); + if (!dynamicPage?.dynamic?.referenceName) { + throw new Error(`GEO_DYNAMIC_PAGE_ID ${DYNAMIC_PAGE_ID} is not a dynamic page`); + } + + // What the target has for the invalid fixture before this run. On a first run the page is + // created by this sync, so it starts with no GEO data. + const previousTargetID = targetPageID(INVALID_PAGE_ID); + invalidTargetBefore = previousTargetID + ? geoPart(await target.pageMethods.getPage(previousTargetID, TARGET!, LOCALE)) + : { geo: null, seo: {} }; + + run = runSync(path.join(workDir, "summary.json")); + }); + + it("does not fail any of the fixture pages", () => { + const failedIDs = (run.summary.failures || []).filter((f: any) => f.type === "page").map((f: any) => f.pageID); + expect(failedIDs).not.toContain(STATIC_PAGE_ID); + expect(failedIDs).not.toContain(DYNAMIC_PAGE_ID); + expect(failedIDs).not.toContain(INVALID_PAGE_ID); + }); + + it.each([ + ["static", () => STATIC_PAGE_ID], + ["dynamic", () => DYNAMIC_PAGE_ID], + ])("copies geo and the GEO seo keys for the %s page", async (_kind, idOf) => { + const sourceID = idOf(); + const targetID = targetPageID(sourceID); + expect(targetID).toBeDefined(); + + const targetPage = await target.pageMethods.getPage(targetID!, TARGET!, LOCALE); + expect(geoPart(targetPage)).toEqual(geoPart(sourcePages[sourceID])); + }); + + it("reports the invalid page as 'GEO not copied' and keeps the target's GEO data", async () => { + const warning = (run.summary.warnings || []).find( + (w: any) => w.code === "geoNotCopied" && w.pageID === INVALID_PAGE_ID + ); + expect(warning).toBeDefined(); + expect(warning.error).toMatch(/^GEO not copied: /); + + // The page itself still synced. + const targetID = targetPageID(INVALID_PAGE_ID); + expect(targetID).toBeDefined(); + const after = geoPart(await target.pageMethods.getPage(targetID!, TARGET!, LOCALE)); + // The rejected values never reach the target. + expect(after).not.toEqual(geoPart(sourcePages[INVALID_PAGE_ID])); + expect(after).toEqual(invalidTargetBefore); + }); +}); + +if (!configured) { + // Keep jest from reporting "Your test suite must contain at least one test". + describe("GEO fields round trip (PROD-2729)", () => { + it.skip("needs AGILITY_TOKEN, GEO_SOURCE_GUID, GEO_TARGET_GUID and the three GEO_*_PAGE_ID fixtures", () => {}); + }); +} diff --git a/src/types/sourceData.ts b/src/types/sourceData.ts index c2e1be8..9f4b31c 100644 --- a/src/types/sourceData.ts +++ b/src/types/sourceData.ts @@ -56,6 +56,7 @@ export interface FailureDetail { locale?: string; // Locale code model?: string; // PROD-2542: model of a content item that failed client-side validation fields?: string[]; // PROD-2542: required fields it was missing, as "name (Type)" + code?: string; // Stable warning code, e.g. "geoNotCopied" (PROD-2729) } /** From 8c03e58de5b30f1672604f0a4232f471be88559d Mon Sep 17 00:00:00 2001 From: DerekAgility Date: Fri, 16 Oct 2026 11:41:35 -0400 Subject: [PATCH 2/2] PROD-2729: carry dynamic-list item GEO through sync; per-item GEO 400 retry; verify item geo The MVP's item half. - Content items already spread the pulled item into the save payload, so `geo` passes through; the null-`seo` default still fills only the five older keys and never sets `geo` (comment added; tests from the page half pin it). - Item saves go through POST item/multi, which validates every item's GEO before queueing and rejects the WHOLE batch with one 400 naming "items[i].geo..." (or "$[i].geo..." from model binding). The batch processor now saves the batch again with the offending GEO properties deleted (never nulled) from the named items only, and reports each as a `geoNotCopied` warning. An unindexed message strips from every item that carries the named keys; a non-GEO 400, nothing to strip, or a failing retry fail the batch as before. Warnings reach the run summary through pushContent's warningDetails; the summary heading is now "Warnings". - `agility verify` compares an item's `geo` (fieldMismatch, path "geo"). - The skipped round-trip integration test gains an independent item block (GEO_ITEM_CONTAINER, GEO_ITEM_ID, optional GEO_INVALID_ITEM_ID), run as a sync scoped with --containers. Co-Authored-By: Claude Opus 5.5 --- src/core/push.ts | 5 +- .../content-pusher/content-batch-processor.ts | 81 +++++++++- .../pushers/content-pusher/content-pusher.ts | 7 + .../tests/content-batch-processor.test.ts | 143 ++++++++++++++++++ src/lib/pushers/content-pusher/util/types.ts | 2 + src/lib/pushers/page-pusher/geo-fields.ts | 58 ++++++- .../page-pusher/tests/geo-fields.test.ts | 65 +++++++- src/lib/verify/check-content.ts | 20 ++- src/lib/verify/tests/check-content.test.ts | 23 +++ .../geo/geo-roundtrip.integration.test.ts | 134 +++++++++++++++- 10 files changed, 518 insertions(+), 20 deletions(-) diff --git a/src/core/push.ts b/src/core/push.ts index ebfac24..a624c3d 100644 --- a/src/core/push.ts +++ b/src/core/push.ts @@ -341,9 +341,10 @@ export class Push { // PROD-2316: dropped-module notices — a page pushed successfully, but one or more of // its modules referenced content that couldn't be resolved on the target, so those // modules were left off the pushed page. Non-blocking: the modules return automatically - // on a future push of the page once the content syncs. + // on a future push of the page once the content syncs. PROD-2729: also pages and content + // items whose GEO data the target rejected and that were saved without it. if (syncWarningDetails.length > 0) { - console.log(ansiColors.yellow(`\n Page Warnings (non-blocking, ${syncWarningDetails.length}):`)); + console.log(ansiColors.yellow(`\n Warnings (non-blocking, ${syncWarningDetails.length}):`)); syncWarningDetails.forEach(({ name, error, pageID, contentID, guid, locale }) => { const prefix = guid && locale ? `[${guid}][${locale}]` : guid ? `[${guid}]` : ""; console.log(ansiColors.yellow(` ${prefix} • ${name}: ${error}`)); diff --git a/src/lib/pushers/content-pusher/content-batch-processor.ts b/src/lib/pushers/content-pusher/content-batch-processor.ts index 58047e2..0dc3035 100644 --- a/src/lib/pushers/content-pusher/content-batch-processor.ts +++ b/src/lib/pushers/content-pusher/content-batch-processor.ts @@ -20,6 +20,13 @@ import { fileOperations } from "core/fileOperations"; import { collectUnresolvedContentReferences } from "./util/has-unresolved-content-references"; import { Logs } from "core/logs"; import { state, getFailedContent } from "core/state"; +import { FailureDetail } from "types/sourceData"; +import { + GEO_NOT_COPIED_CODE, + geoNotCopiedMessage, + getItemGeoValidationErrors, + withoutGeoFields, +} from "../page-pusher/geo-fields"; /** * PROD-2542: one source item and the payload built from it. Prepared items travel as pairs so the @@ -122,6 +129,7 @@ export class ContentBatchProcessor { let totalSkippedCount = 0; const allSuccessfulItems: BatchSuccessItem[] = []; const allFailedItems: BatchFailedItem[] = []; + const allWarnings: FailureDetail[] = []; const startTime = Date.now(); for (let i = 0; i < contentBatches.length; i++) { @@ -171,16 +179,37 @@ export class ContentBatchProcessor { continue; } // Both lists derive from the same paired array, so index i of one is index i of the other. - const contentPayloads = prepared.map((p) => p.payload); + let contentPayloads = prepared.map((p) => p.payload); const includedItems = prepared.map((p) => p.source); // Execute bulk upload using saveContentItems API with returnBatchID flag - const batchIDResult = await this.config.apiClient.contentMethods.saveContentItems( - contentPayloads, - this.config.targetGuid, - this.config.locale, - true // returnBatchID flag - ); + const saveBatch = (payloads: any[]) => + this.config.apiClient.contentMethods.saveContentItems( + payloads, + this.config.targetGuid, + this.config.locale, + true // returnBatchID flag + ); + + let batchIDResult: any; + try { + batchIDResult = await saveBatch(contentPayloads); + } catch (saveError: any) { + // PROD-2729: the Management API validates every item's GEO data before queueing and + // rejects the WHOLE request with one 400 naming "items[i].geo.…". One bad JSON-LD block + // must not fail a batch of up to 100 items, so save again with the offending GEO + // properties left out of the named items (deleted, never nulled: the target keeps its + // own values) and report each as a non-blocking warning. + const retry = this.withoutRejectedGeo(saveError, prepared); + if (!retry) throw saveError; + contentPayloads = retry.payloads; + prepared.forEach((p, idx) => (p.payload = contentPayloads[idx])); + batchIDResult = await saveBatch(contentPayloads); + allWarnings.push(...retry.warnings); + retry.warnings.forEach((w) => + console.warn(ansiColors.yellow(`⚠️ ${w.name} (content ${w.contentID}): ${w.error}`)) + ); + } // Extract batch ID from array response const batchID = Array.isArray(batchIDResult) ? batchIDResult[0] : batchIDResult; @@ -322,9 +351,43 @@ export class ContentBatchProcessor { successfulItems: allSuccessfulItems, failedItems: allFailedItems, publishableIds: publishableItems.map((item) => item.newContentId), + warnings: allWarnings.length > 0 ? allWarnings : undefined, }; } + /** + * PROD-2729: if `error` is the Management API's 400 rejecting GEO data in a batch save, the + * payloads to send again (offending GEO properties deleted from the items it names) and one + * `geoNotCopied` warning per item that changed. Undefined when the error isn't a GEO 400, or when + * no payload carries the named properties (a retry would fail the same way). + */ + private withoutRejectedGeo( + error: any, + prepared: PreparedContentPayload[] + ): { payloads: any[]; warnings: FailureDetail[] } | undefined { + const byIndex = getItemGeoValidationErrors(error, prepared.length); + if (!byIndex) return undefined; + + const warnings: FailureDetail[] = []; + const payloads = prepared.map((p, idx) => { + const validation = byIndex.get(idx); + if (!validation) return p.payload; + const stripped = withoutGeoFields(p.payload, validation.strip); + if (JSON.stringify(stripped) === JSON.stringify(p.payload)) return p.payload; + warnings.push({ + name: p.source?.properties?.referenceName || `Content ${p.source?.contentID}`, + error: geoNotCopiedMessage(validation), + type: "content", + contentID: p.source?.contentID, + guid: this.config.sourceGuid, + locale: this.config.locale, + code: GEO_NOT_COPIED_CODE, + }); + return stripped; + }); + return warnings.length > 0 ? { payloads, warnings } : undefined; + } + /** * Create batches of content items for bulk processing */ @@ -564,6 +627,10 @@ export class ContentBatchProcessor { const defaultScripts = { top: null, bottom: null }; // STEP 8: Create payload using EXACT original logic + // PROD-2729: the spread carries the item's `geo` (dynamic page list items) and any GEO + // `seo` keys (title, canonicalUrl, robots, ogImage) through as pulled. `defaultSeo` only + // fills the five older keys when `seo` is null, and nothing here may set `geo` (or a GEO + // key) to null: on save an omitted property keeps the target's value, null clears it. const payload = { ...contentItem, // Start with original content item contentID: existingContentID, diff --git a/src/lib/pushers/content-pusher/content-pusher.ts b/src/lib/pushers/content-pusher/content-pusher.ts index 1c2c076..0ae3abc 100644 --- a/src/lib/pushers/content-pusher/content-pusher.ts +++ b/src/lib/pushers/content-pusher/content-pusher.ts @@ -1,5 +1,6 @@ // Removed finder imports - using mapper directly import ansiColors from "ansi-colors"; +import { FailureDetail } from "types/sourceData"; // Removed ContentBatchProcessor import - individual pusher only handles individual processing import { getLoggerForGuid, state, registerFailedContent } from "core/state"; import { ContentItemMapper } from "lib/mappers/content-item-mapper"; @@ -68,6 +69,8 @@ export async function pushContent(sourceData: ContentItem[], targetData: Content model?: string; fields?: string[]; }> = []; + // PROD-2729: non-blocking notices (an item's GEO data left out of the save). + const allWarningDetails: FailureDetail[] = []; // PROD-2542: items the filter rejected client-side (missing required field on the target model). // Register each in the failed-content registry right away, so the next pass's parents and the @@ -151,6 +154,7 @@ export async function pushContent(sourceData: ContentItem[], targetData: Content totalSkipped += filteredLinkedContentItems.skippedCount; totalSkipped += linkedResult.skippedCount; allPublishableIds.push(...linkedResult.publishableIds); + if (linkedResult.warnings) allWarningDetails.push(...linkedResult.warnings); // Collect failure details for error summary and register in failed content registry if (linkedResult.failedItems && linkedResult.failedItems.length > 0) { linkedResult.failedItems.forEach((item) => { @@ -214,6 +218,7 @@ export async function pushContent(sourceData: ContentItem[], targetData: Content totalSkipped += filteredNormalContentItems.skippedCount; totalSkipped += normalResult.skippedCount; allPublishableIds.push(...normalResult.publishableIds); + if (normalResult.warnings) allWarningDetails.push(...normalResult.warnings); // Collect failure details for error summary and register in failed content registry if (normalResult.failedItems && normalResult.failedItems.length > 0) { normalResult.failedItems.forEach((item) => { @@ -244,6 +249,7 @@ export async function pushContent(sourceData: ContentItem[], targetData: Content skipped: totalSkipped, publishableIds: allPublishableIds, failureDetails: allFailureDetails, + warningDetails: allWarningDetails, }; } catch (batchError: any) { console.error(ansiColors.red(`❌ Batch processing failed: ${batchError.message}`)); @@ -254,6 +260,7 @@ export async function pushContent(sourceData: ContentItem[], targetData: Content skipped: totalSkipped, publishableIds: allPublishableIds, failureDetails: [...allFailureDetails, { name: "Batch processing", error: batchError.message }], + warningDetails: allWarningDetails, }; } } diff --git a/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts b/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts index 477e16c..3cc4bf9 100644 --- a/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts +++ b/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts @@ -245,6 +245,149 @@ describe("ContentBatchProcessor.processBatches — successful batch", () => { }); }); +// ─── processBatches — GEO 400 retry (PROD-2729) ────────────────────────────── + +describe("ContentBatchProcessor.processBatches — GEO validation 400 (PROD-2729)", () => { + // @agility/management-sdk wraps the AxiosError in its own Exception (`innerError`). + function geo400(exceptionMessage: string): any { + const err: any = new Error("Unable to create contents."); + err.innerError = { response: { status: 400, data: { statusCode: 400, exceptionMessage } } }; + return err; + } + + function preparedWithGeo(items: any[]): Array<{ source: any; payload: any }> { + return items.map((source) => ({ + source, + payload: { + contentID: -1, + fields: source.fields, + seo: source.seo, + ...(source.geo !== undefined ? { geo: source.geo } : {}), + }, + })); + } + + it("saves the batch again without the GEO of the items the 400 names, and warns for each", async () => { + const save = jest + .fn() + .mockRejectedValueOnce(geo400("items[1].geo.structuredData: is not valid JSON (bad token).")) + .mockResolvedValueOnce([42]); + const processor = new ContentBatchProcessor(makeConfig({ apiClient: makeApiClient(save) })); + const item1 = { ...makeContentItem(1), geo: { summary: "Fine." } }; + const item2 = { ...makeContentItem(2), geo: { summary: "Bad JSON-LD here.", structuredData: "{" } }; + jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ + prepared: preparedWithGeo([item1, item2]), + skippedCount: 0, + failedItems: [], + }); + mockExtract.mockReturnValue({ + successfulItems: [ + { originalItem: item1, newItem: { itemID: 101 }, newId: 101 }, + { originalItem: item2, newItem: { itemID: 102 }, newId: 102 }, + ], + failedItems: [], + }); + + const result = await processor.processBatches([item1, item2], makeLogger(), "Test"); + + expect(save).toHaveBeenCalledTimes(2); + const retried = save.mock.calls[1][0]; + expect(retried[0].geo).toEqual({ summary: "Fine." }); // not named: untouched + expect(retried[1]).not.toHaveProperty("geo"); // deleted, never nulled + // Polling matches results against the payloads that were actually sent. + expect(mockPoll.mock.calls[mockPoll.mock.calls.length - 1][3]).toBe(retried); + expect(result.successCount).toBe(2); + expect(result.failureCount).toBe(0); + expect(result.warnings).toEqual([ + expect.objectContaining({ + name: "ref-2", + type: "content", + contentID: 2, + code: "geoNotCopied", + error: expect.stringContaining("GEO not copied: invalid structured data"), + }), + ]); + }); + + it("strips only the named seo key when the 400 names a GEO seo field", async () => { + const save = jest + .fn() + .mockRejectedValueOnce(geo400("items[0].seo.canonicalUrl: must be an absolute URL.")) + .mockResolvedValueOnce([42]); + const processor = new ContentBatchProcessor(makeConfig({ apiClient: makeApiClient(save) })); + const item = { + ...makeContentItem(1), + seo: { metaDescription: "d", canonicalUrl: "nope" }, + geo: { summary: "Kept." }, + }; + jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ + prepared: preparedWithGeo([item]), + skippedCount: 0, + failedItems: [], + }); + + const result = await processor.processBatches([item], makeLogger(), "Test"); + + const retried = save.mock.calls[1][0][0]; + expect(retried.seo).toEqual({ metaDescription: "d" }); + expect(retried.geo).toEqual({ summary: "Kept." }); + expect(result.warnings).toHaveLength(1); + }); + + it("fails the batch as before when the 400 is not about GEO", async () => { + const save = jest.fn().mockRejectedValue(geo400("fields.title: is required.")); + const processor = new ContentBatchProcessor(makeConfig({ apiClient: makeApiClient(save) })); + const item = { ...makeContentItem(1), geo: { summary: "x" } }; + jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ + prepared: preparedWithGeo([item]), + skippedCount: 0, + failedItems: [], + }); + + const result = await processor.processBatches([item], makeLogger(), "Test"); + + expect(save).toHaveBeenCalledTimes(1); + expect(result.failureCount).toBe(1); + expect(result.warnings).toBeUndefined(); + }); + + it("does not retry when no item carries the rejected GEO data", async () => { + const save = jest.fn().mockRejectedValue(geo400("items[0].geo.schemaType: must be one of WebPage.")); + const processor = new ContentBatchProcessor(makeConfig({ apiClient: makeApiClient(save) })); + const item = makeContentItem(1); + jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ + prepared: preparedWithGeo([item]), + skippedCount: 0, + failedItems: [], + }); + + const result = await processor.processBatches([item], makeLogger(), "Test"); + + expect(save).toHaveBeenCalledTimes(1); + expect(result.failureCount).toBe(1); + }); + + it("fails the batch when the retry fails too", async () => { + const save = jest + .fn() + .mockRejectedValueOnce(geo400("items[0].geo.structuredData: is not valid JSON.")) + .mockRejectedValueOnce(new Error("Request failed with status code 500")); + const processor = new ContentBatchProcessor(makeConfig({ apiClient: makeApiClient(save) })); + const item = { ...makeContentItem(1), geo: { structuredData: "{" } }; + jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ + prepared: preparedWithGeo([item]), + skippedCount: 0, + failedItems: [], + }); + + const result = await processor.processBatches([item], makeLogger(), "Test"); + + expect(save).toHaveBeenCalledTimes(2); + expect(result.failureCount).toBe(1); + expect(result.failedItems[0].error).toContain("status code 500"); + }); +}); + // ─── processBatches — publishableIds filtering ──────────────────────────────── describe("ContentBatchProcessor.processBatches — publishableIds", () => { diff --git a/src/lib/pushers/content-pusher/util/types.ts b/src/lib/pushers/content-pusher/util/types.ts index 39c057c..353ebbb 100644 --- a/src/lib/pushers/content-pusher/util/types.ts +++ b/src/lib/pushers/content-pusher/util/types.ts @@ -1,5 +1,6 @@ import * as mgmtApi from "@agility/management-sdk"; import { ContentItemMapper } from "lib/mappers/content-item-mapper"; +import { FailureDetail } from "types/sourceData"; /** * Configuration for content batch processing @@ -27,6 +28,7 @@ export interface BatchProcessingResult { successfulItems: BatchSuccessItem[]; failedItems: BatchFailedItem[]; publishableIds: number[]; // Target content IDs for workflow operations + warnings?: FailureDetail[]; // Non-blocking notices, e.g. an item's GEO data left out (PROD-2729) } /** diff --git a/src/lib/pushers/page-pusher/geo-fields.ts b/src/lib/pushers/page-pusher/geo-fields.ts index 28ca051..7213976 100644 --- a/src/lib/pushers/page-pusher/geo-fields.ts +++ b/src/lib/pushers/page-pusher/geo-fields.ts @@ -88,6 +88,55 @@ export function getGeoValidationError(error: any): GeoValidationError | undefine const message = pieces.join(" ").trim(); if (!message) return undefined; + const parsed = parseGeoPaths(message); + if (!parsed) return undefined; + return { message, ...parsed }; +} + +/** + * Content items (the MVP's item half): `POST {locale}/item/multi` validates every item's GEO data + * before queueing the batch and rejects the whole request with one 400 whose message prefixes each + * path with the item's index ("items[3].geo.structuredData: ..."; ASP.NET model binding says + * "$[3].geo.schemaType"); a single-item save has no prefix. + * Returns the validation errors per payload index, or undefined when the error isn't a GEO 400. + * + * When the message names no index for a multi-item request, every item is attributed the whole + * error: the caller then leaves the named properties out of every item that carries them, which + * only ever keeps the target's values. + */ +export function getItemGeoValidationErrors(error: any, itemCount: number): Map | undefined { + const overall = getGeoValidationError(error); + if (!overall) return undefined; + + const byIndex = new Map(); + const segments = overall.message.split(/(?=(?:items|\$)\[\d+\]\.)/); + for (const segment of segments) { + const indexMatch = /^(?:items|\$)\[(\d+)\]\./.exec(segment); + if (!indexMatch) continue; + const index = parseInt(indexMatch[1], 10); + if (index < 0 || index >= itemCount) continue; + const parsed = parseGeoPaths(segment.slice(indexMatch[0].length)); + if (!parsed) continue; + const existing = byIndex.get(index); + if (existing) { + existing.message = `${existing.message} ${segment.trim()}`; + parsed.strip.forEach((s) => existing.strip.includes(s) || existing.strip.push(s)); + existing.structuredDataOnly = existing.structuredDataOnly && parsed.structuredDataOnly; + } else { + byIndex.set(index, { + message: segment.trim(), + strip: parsed.strip, + structuredDataOnly: parsed.structuredDataOnly, + }); + } + } + if (byIndex.size > 0) return byIndex; + + for (let i = 0; i < itemCount; i++) byIndex.set(i, { ...overall, strip: [...overall.strip] }); + return byIndex; +} + +function parseGeoPaths(message: string): Omit | undefined { const strip: string[] = []; const paths: string[] = []; const pattern = new RegExp(GEO_PATH.source, GEO_PATH.flags); @@ -99,12 +148,7 @@ export function getGeoValidationError(error: any): GeoValidationError | undefine if (!strip.includes(target.strip)) strip.push(target.strip); } if (strip.length === 0) return undefined; - - return { - message, - strip, - structuredDataOnly: paths.every((p) => p.startsWith("geo.structureddata")), - }; + return { strip, structuredDataOnly: paths.every((p) => p.startsWith("geo.structureddata")) }; } /** @@ -124,7 +168,7 @@ export function withoutGeoFields>(payload: T, stri return copy; } -/** The warning text recorded for a page whose GEO data was not copied. */ +/** The warning text recorded for a page or content item whose GEO data was not copied. */ export function geoNotCopiedMessage(validation: GeoValidationError): string { const what = validation.structuredDataOnly ? "invalid structured data" : "invalid GEO fields"; return `GEO not copied: ${what} (${validation.strip.join(", ")} left out; the target keeps its existing values). ${validation.message}`; diff --git a/src/lib/pushers/page-pusher/tests/geo-fields.test.ts b/src/lib/pushers/page-pusher/tests/geo-fields.test.ts index 577c5a9..81bcb45 100644 --- a/src/lib/pushers/page-pusher/tests/geo-fields.test.ts +++ b/src/lib/pushers/page-pusher/tests/geo-fields.test.ts @@ -1,4 +1,9 @@ -import { getGeoValidationError, withoutGeoFields, geoNotCopiedMessage } from "../geo-fields"; +import { + getGeoValidationError, + getItemGeoValidationErrors, + withoutGeoFields, + geoNotCopiedMessage, +} from "../geo-fields"; // @agility/management-sdk wraps the AxiosError in its own Exception (`innerError`). function sdkError(status: number, data: any): any { @@ -104,3 +109,61 @@ describe("geoNotCopiedMessage (PROD-2729)", () => { expect(message).toContain("geo.structuredData: is not valid JSON."); }); }); + +describe("getItemGeoValidationErrors (PROD-2729, content items)", () => { + it("attributes each items[i] path to its item", () => { + const result = getItemGeoValidationErrors( + sdkError(400, { + exceptionMessage: + "items[0].geo.schemaType: must be one of WebPage, AboutPage (was 'Article'). items[2].geo.structuredData: is not valid JSON. items[2].seo.ogImage: must be an absolute URL.", + }), + 3 + )!; + expect(Array.from(result.keys())).toEqual([0, 2]); + expect(result.get(0)).toEqual({ + message: expect.stringContaining("items[0].geo.schemaType"), + strip: ["geo"], + structuredDataOnly: false, + }); + expect(result.get(2)!.strip).toEqual(["geo", "seo.ogImage"]); + expect(result.get(2)!.message).not.toContain("items[0]"); + expect(result.get(2)!.structuredDataOnly).toBe(false); + }); + + it("reads ASP.NET model-binding paths ($[i].geo...)", () => { + const result = getItemGeoValidationErrors( + sdkError(400, { status: 400, errors: { "$[1].geo.ai.crawl": ["The JSON value could not be converted."] } }), + 2 + )!; + expect(Array.from(result.keys())).toEqual([1]); + expect(result.get(1)!.strip).toEqual(["geo"]); + }); + + it("attributes an unprefixed error to the only item of a single-item save", () => { + const result = getItemGeoValidationErrors( + sdkError(400, { exceptionMessage: "geo.structuredData: is not valid JSON." }), + 1 + )!; + expect(result.get(0)).toEqual(expect.objectContaining({ strip: ["geo"], structuredDataOnly: true })); + }); + + it("attributes an unprefixed error to every item of a multi-item save", () => { + const result = getItemGeoValidationErrors(sdkError(400, { exceptionMessage: "geo.summary: is too long." }), 3)!; + expect(Array.from(result.keys())).toEqual([0, 1, 2]); + }); + + it("ignores indexes outside the batch, non-GEO 400s and other statuses", () => { + expect( + getItemGeoValidationErrors(sdkError(400, { exceptionMessage: "items[0].fields.title: is required." }), 1) + ).toBeUndefined(); + expect( + getItemGeoValidationErrors(sdkError(500, { exceptionMessage: "items[0].geo.summary: boom" }), 1) + ).toBeUndefined(); + const out = getItemGeoValidationErrors( + sdkError(400, { exceptionMessage: "items[5].geo.summary: is too long." }), + 2 + )!; + // No index in range, so it falls back to every item (stripping only ever keeps the target's values). + expect(Array.from(out.keys())).toEqual([0, 1]); + }); +}); diff --git a/src/lib/verify/check-content.ts b/src/lib/verify/check-content.ts index 4067c79..5d854ff 100644 --- a/src/lib/verify/check-content.ts +++ b/src/lib/verify/check-content.ts @@ -5,7 +5,9 @@ * - contentModelMismatch the target item is of a different model (through the model mapping) * - contentContainerMismatch the target item sits in a different container — skipped for items * placed on a page, whose container name is auto-generated - * - fieldMismatch one per differing field, compared in canonical form + * - fieldMismatch one per differing field, compared in canonical form; also path + * "geo" when the item's own GEO data differs (dynamic page list items, + * PROD-2729) * - sourceAssetUrlInTarget a target field still points at an asset in the SOURCE instance * - publishStateDifference (warning) the two items are in different workflow states * Per container / model (unscoped runs only): @@ -18,7 +20,7 @@ import { Finding, FindingCollector } from "./verify-report"; import { ComparablePair, contentSourceDefect } from "./check-mapping-integrity"; import { sourceCanonicalContext, targetCanonicalContext, VerifyContext } from "./verify-context"; -import { canonicalFields, diffCanonical } from "./canonicalize"; +import { canonicalEquals, canonicalFields, canonicalValue, describeDifference, diffCanonical } from "./canonicalize"; function lower(value: any): string { return typeof value === "string" ? value.toLowerCase() : ""; @@ -87,6 +89,20 @@ export function checkContent(ctx: VerifyContext, pairs: ComparablePair[], collec }); }); + // PROD-2729: an item in a dynamic page list carries its own `geo`; a sync that didn't copy it + // (or a target that rejected it) shows here. Absent on both sides compares equal. + const expectedGeo = canonicalValue(source?.geo, sourceCtx); + const actualGeo = canonicalValue(target?.geo, targetCtx); + if (!canonicalEquals(expectedGeo, actualGeo)) { + findings.push({ + ...base, + category: "fieldMismatch", + severity: "failure", + path: "geo", + ...describeDifference(expectedGeo, actualGeo), + }); + } + const leaked = new Map(); walkStrings(target?.fields, "fields", (text, path) => { const urls = ctx.assets.findSourceUrls(text); diff --git a/src/lib/verify/tests/check-content.test.ts b/src/lib/verify/tests/check-content.test.ts index 863e9e6..8831584 100644 --- a/src/lib/verify/tests/check-content.test.ts +++ b/src/lib/verify/tests/check-content.test.ts @@ -61,6 +61,29 @@ describe("checkContent", () => { expect(collector.talliesFor(LOCALE).content.mismatched).toBe(1); }); + // PROD-2729: a dynamic page list item's own GEO data. + it("compares an item's geo", () => { + const geo = { schemaType: "ItemPage", summary: "In one sentence.", structuredData: { "@type": "BlogPosting" } }; + const withGeo = (id: number, g: any) => ({ ...item(id, { title: "A" }), ...(g !== undefined ? { geo: g } : {}) }); + + expect(run([withGeo(1, geo)], [withGeo(101, geo)], [[1, 101]]).findings).toEqual([]); + expect(run([withGeo(1, undefined)], [withGeo(101, undefined)], [[1, 101]]).findings).toEqual([]); + + const missing = run([withGeo(1, geo)], [withGeo(101, undefined)], [[1, 101]]); + expect(missing.findings).toEqual([ + expect.objectContaining({ + category: "fieldMismatch", + path: "geo", + sourceID: 1, + targetID: 101, + actual: undefined, + }), + ]); + + const differs = run([withGeo(1, geo)], [withGeo(101, { ...geo, summary: "Other." })], [[1, 101]]); + expect(differs.findings.map((f) => f.path)).toEqual(["geo"]); + }); + it("translates linked content IDs before comparing", () => { const collector = run( [item(1, { title: "A" }), item(2, { title: "B", related: { contentid: 1, fulllist: false } })], diff --git a/src/tests/geo/geo-roundtrip.integration.test.ts b/src/tests/geo/geo-roundtrip.integration.test.ts index 18377c1..eafbc25 100644 --- a/src/tests/geo/geo-roundtrip.integration.test.ts +++ b/src/tests/geo/geo-roundtrip.integration.test.ts @@ -21,11 +21,23 @@ * that isn't a JSON object/array, or schemaType "Article". The Management API won't save that, * so write it through a path that doesn't validate (the classic editor, or the DB on dev). * + * Dynamic-list items (the MVP's item half), a second, independent block run with a sync scoped to + * one container: + * 3. An item in a list that backs a dynamic page, carrying `geo`, arrives on the target with the + * same `geo`. + * 4. Optionally, an item in the same list whose GEO data the Management API rejects is saved + * without it (the target keeps its own), with a `geoNotCopied` warning, and the rest of its + * batch is not failed. + * Fixtures: GEO_ITEM_CONTAINER (the list's reference name; keep it small, the whole list is synced), + * GEO_ITEM_ID (an item in it with `geo` set) and, optionally, GEO_INVALID_ITEM_ID (an item in it + * with invalid GEO data, written the same way as GEO_INVALID_PAGE_ID). + * * Required env vars: * - AGILITY_TOKEN personal access token with access to both instances * - GEO_SOURCE_GUID source instance GUID (dev, e.g. "xxxxxxxx-d") * - GEO_TARGET_GUID target instance GUID (dev) - * - GEO_STATIC_PAGE_ID, GEO_DYNAMIC_PAGE_ID, GEO_INVALID_PAGE_ID source page IDs + * - GEO_STATIC_PAGE_ID, GEO_DYNAMIC_PAGE_ID, GEO_INVALID_PAGE_ID source page IDs (page block) + * - GEO_ITEM_CONTAINER, GEO_ITEM_ID (item block; each block skips on its own) * Optional: GEO_LOCALE (default en-us), GEO_CHANNEL (default website), GEO_WORK_DIR (where the CLI * keeps its files and mappings between runs; default a fixed folder under the OS temp dir). * @@ -39,6 +51,7 @@ import * as mgmtApi from "@agility/management-sdk"; import { Auth } from "core/auth"; import { resetState, setState } from "core/state"; import { PageMapper } from "lib/mappers/page-mapper"; +import { ContentItemMapper } from "lib/mappers/content-item-mapper"; const env = process.env; const TOKEN = env.AGILITY_TOKEN; @@ -50,7 +63,12 @@ const STATIC_PAGE_ID = Number(env.GEO_STATIC_PAGE_ID); const DYNAMIC_PAGE_ID = Number(env.GEO_DYNAMIC_PAGE_ID); const INVALID_PAGE_ID = Number(env.GEO_INVALID_PAGE_ID); +const ITEM_CONTAINER = env.GEO_ITEM_CONTAINER; +const ITEM_ID = Number(env.GEO_ITEM_ID); +const INVALID_ITEM_ID = Number(env.GEO_INVALID_ITEM_ID) || 0; + const configured = !!TOKEN && !!SOURCE && !!TARGET && STATIC_PAGE_ID > 0 && DYNAMIC_PAGE_ID > 0 && INVALID_PAGE_ID > 0; +const itemsConfigured = !!TOKEN && !!SOURCE && !!TARGET && !!ITEM_CONTAINER && ITEM_ID > 0; const describeIfConfigured = configured ? describe : describe.skip; @@ -207,3 +225,117 @@ if (!configured) { it.skip("needs AGILITY_TOKEN, GEO_SOURCE_GUID, GEO_TARGET_GUID and the three GEO_*_PAGE_ID fixtures", () => {}); }); } + +(itemsConfigured ? describe : describe.skip)("GEO fields round trip, dynamic-list items (PROD-2729)", () => { + jest.setTimeout(20 * 60 * 1000); + + const auth = new Auth(); + let workDir: string; + let target: mgmtApi.ApiClient; + let sourceItems: Record; + let invalidTargetBefore: any; + let run: { exitCode: number | null; summary: any }; + + /** The source item as the CLI will pull it: from the Fetch API (preview). */ + async function fetchSourceItem(contentID: number): Promise { + const keyResponse = await fetch(`${auth.determineBaseUrl(SOURCE)}/oauth/GetPreviewKey?guid=${SOURCE}`, { + headers: { Authorization: `Bearer ${TOKEN}`, "Cache-Control": "no-cache" }, + }); + if (!keyResponse.ok) throw new Error(`GetPreviewKey failed: HTTP ${keyResponse.status}`); + const key = (await keyResponse.text()).replace(/^"|"$/g, ""); + const response = await fetch( + `${auth.determineFetchUrl(SOURCE)}/${SOURCE}/preview/${LOCALE}/item/${contentID}?contentLinkDepth=0`, + { headers: { APIKey: key } } + ); + if (!response.ok) throw new Error(`Fetch API item ${contentID}: HTTP ${response.status}`); + return response.json(); + } + + function targetContentID(sourceContentID: number): number | undefined { + setState({ rootPath: path.join(workDir, "agility-files") }); + const mapping = new ContentItemMapper(SOURCE!, TARGET!, LOCALE).getContentItemMappingByContentID( + sourceContentID, + "source" + ); + return mapping && mapping.targetContentID > 0 ? mapping.targetContentID : undefined; + } + + beforeAll(async () => { + if (!fs.existsSync(CLI)) throw new Error(`Build the CLI first (npm run build): ${CLI} is missing`); + resetState(); + workDir = env.GEO_WORK_DIR || path.join(os.tmpdir(), `agility-geo-roundtrip-${SOURCE}-${TARGET}`); + fs.mkdirSync(workDir, { recursive: true }); + target = new mgmtApi.ApiClient({ token: TOKEN!, baseUrl: auth.determineBaseUrl(TARGET!) } as mgmtApi.Options); + + // Precondition: the source items carry GEO data in the Fetch API output. + sourceItems = {}; + for (const id of [ITEM_ID, INVALID_ITEM_ID].filter((x) => x > 0)) { + sourceItems[id] = await fetchSourceItem(id); + if (!sourceItems[id]?.geo) { + throw new Error( + `Source item ${id} has no geo in the Fetch API output. Is the Fetch API item half deployed, is the item in a dynamic page list, and was it re-published after its GEO data was saved?` + ); + } + } + + const previousInvalid = INVALID_ITEM_ID > 0 ? targetContentID(INVALID_ITEM_ID) : undefined; + invalidTargetBefore = previousInvalid + ? (((await target.contentMethods.getContentItem(previousInvalid, TARGET!, LOCALE)) as any)?.geo ?? null) + : null; + + const summaryPath = path.join(workDir, "summary-items.json"); + fs.rmSync(summaryPath, { force: true }); + const result = spawnSync( + process.execPath, + [ + CLI, + "sync", + `--sourceGuid=${SOURCE}`, + `--targetGuid=${TARGET}`, + `--token=${TOKEN}`, + `--locales=${LOCALE}`, + `--containers=${ITEM_CONTAINER}`, + "--overwrite", + "--headless", + `--jsonSummary=${summaryPath}`, + ], + { cwd: workDir, encoding: "utf8", env: { ...process.env }, maxBuffer: 64 * 1024 * 1024 } + ); + if (!fs.existsSync(summaryPath)) { + throw new Error(`sync wrote no summary (exit ${result.status}).\n${result.stdout}\n${result.stderr}`); + } + run = { exitCode: result.status, summary: JSON.parse(fs.readFileSync(summaryPath, "utf8")) }; + }); + + it("does not fail the fixture items", () => { + const failedIDs = (run.summary.failures || []) + .filter((f: any) => f.type === "content") + .map((f: any) => f.contentID); + expect(failedIDs).not.toContain(ITEM_ID); + if (INVALID_ITEM_ID > 0) expect(failedIDs).not.toContain(INVALID_ITEM_ID); + }); + + it("copies the item's geo", async () => { + const targetID = targetContentID(ITEM_ID); + expect(targetID).toBeDefined(); + const targetItem: any = await target.contentMethods.getContentItem(targetID!, TARGET!, LOCALE); + expect(targetItem?.geo ?? null).toEqual(sourceItems[ITEM_ID].geo); + }); + + (INVALID_ITEM_ID > 0 ? it : it.skip)( + "reports the invalid item as 'GEO not copied' and keeps the target's GEO data", + async () => { + const warning = (run.summary.warnings || []).find( + (w: any) => w.code === "geoNotCopied" && w.contentID === INVALID_ITEM_ID + ); + expect(warning).toBeDefined(); + expect(warning.error).toMatch(/^GEO not copied: /); + + const targetID = targetContentID(INVALID_ITEM_ID); + expect(targetID).toBeDefined(); + const after: any = await target.contentMethods.getContentItem(targetID!, TARGET!, LOCALE); + expect(after?.geo ?? null).not.toEqual(sourceItems[INVALID_ITEM_ID].geo); + expect(after?.geo ?? null).toEqual(invalidTargetBefore); + } + ); +});