PROD-2350: match page zones and template sections by ID, not position/name

src/lib/mappers/section-mapper.ts

@@ -0,0 +1,117 @@
+import { fileOperations } from "../../core";
+import * as mgmtApi from "@agility/management-sdk";
+
+interface SectionMapping {
+  sourceGuid: string;
+  targetGuid: string;
+  sourcePageItemTemplateID: number;
+  targetPageItemTemplateID: number;
+  sourceReferenceName?: string;
+  targetReferenceName?: string;
+}
+
+// PROD-2350: content section definitions (page zones) were being matched between source and
+// target templates by reference name (or worse, by array position) every time a page or template
+// was pushed. That breaks whenever a section is renamed or reordered on either side. This mapper
+// persists the source/target pageItemTemplateID pairing once (established in template-pusher.ts
+// from the confirmed API response), so later lookups are a stable ID join instead of a repeated
+// guess against possibly-stale names.
+export class SectionMapper {
+  private fileOps: fileOperations;
+  private sourceGuid: string;
+  private targetGuid: string;
+  private mappings: SectionMapping[];
+  private directory: string;
+
+  constructor(sourceGuid: string, targetGuid: string) {
+    this.sourceGuid = sourceGuid;
+    this.targetGuid = targetGuid;
+    this.directory = "sections";
+    // this will provide access to the /agility-files/{GUID} folder
+    this.fileOps = new fileOperations(targetGuid);
+    this.mappings = this.loadMapping();
+  }
+
+  getSectionMapping(
+    section: mgmtApi.ContentSectionDefinition,
+    type: "source" | "target"
+  ): SectionMapping | null {
+    if (!section) return null;
+    const mapping = this.mappings.find((m: SectionMapping) =>
+      type === "source"
+        ? m.sourcePageItemTemplateID === section.pageItemTemplateID
+        : m.targetPageItemTemplateID === section.pageItemTemplateID
+    );
+    if (!mapping) return null;
+    return mapping;
+  }
+
+  getSectionMappingByID(pageItemTemplateID: number, type: "source" | "target"): SectionMapping | null {
+    const mapping = this.mappings.find((m: SectionMapping) =>
+      type === "source"
+        ? m.sourcePageItemTemplateID === pageItemTemplateID
+        : m.targetPageItemTemplateID === pageItemTemplateID
+    );
+    if (!mapping) return null;
+    return mapping;
+  }
+
+  addMapping(
+    sourceSection: mgmtApi.ContentSectionDefinition,
+    targetSection: mgmtApi.ContentSectionDefinition
+  ) {
+    const targetMapping = this.getSectionMapping(targetSection, "target");
+    const sourceMapping = this.getSectionMapping(sourceSection, "source");
+
+    if (targetMapping && sourceMapping && targetMapping !== sourceMapping) {
+      throw new Error(
+        `Invalid Mappings detected! Source pageItemTemplateID: ${sourceSection.pageItemTemplateID}, Target pageItemTemplateID: ${targetSection.pageItemTemplateID}`
+      );
+    }
+
+    if (targetMapping) {
+      this.updateMapping(sourceSection, targetSection, targetMapping);
+    } else {
+      const newMapping: SectionMapping = {
+        sourceGuid: this.sourceGuid,
+        targetGuid: this.targetGuid,
+        sourcePageItemTemplateID: sourceSection.pageItemTemplateID,
+        targetPageItemTemplateID: targetSection.pageItemTemplateID,
+        sourceReferenceName: sourceSection.pageItemTemplateReferenceName,
+        targetReferenceName: targetSection.pageItemTemplateReferenceName,
+      };
+
+      this.mappings.push(newMapping);
+    }
+
+    this.saveMapping();
+  }
+
+  updateMapping(
+    sourceSection: mgmtApi.ContentSectionDefinition,
+    targetSection: mgmtApi.ContentSectionDefinition,
+    mapping: SectionMapping
+  ) {
+    if (targetSection.pageItemTemplateID !== mapping.targetPageItemTemplateID) {
+      throw new Error(
+        `Invalid items trying to be mapped! Source pageItemTemplateID: ${sourceSection.pageItemTemplateID}, Target pageItemTemplateID: ${targetSection.pageItemTemplateID}`
+      );
+    }
+    mapping.sourceGuid = this.sourceGuid;
+    mapping.targetGuid = this.targetGuid;
+    mapping.sourcePageItemTemplateID = sourceSection.pageItemTemplateID;
+    mapping.targetPageItemTemplateID = targetSection.pageItemTemplateID;
+    mapping.sourceReferenceName = sourceSection.pageItemTemplateReferenceName;
+    mapping.targetReferenceName = targetSection.pageItemTemplateReferenceName;
+    this.saveMapping();
+  }
+
+  loadMapping() {
+    const mapping = this.fileOps.getMappingFile(this.directory, this.sourceGuid, this.targetGuid);
+    return mapping;
+  }
+
+  saveMapping() {
+    this.fileOps.saveMappingFile(this.mappings, this.directory, this.sourceGuid, this.targetGuid);
+  }
+}

src/lib/mappers/template-mapper.ts

@@ -1,5 +1,6 @@
 import { fileOperations } from "../../core";
 import * as mgmtApi from "@agility/management-sdk";
+import { SectionMapper } from "./section-mapper";
 interface TemplateMapping {
   sourceGuid: string;
   targetGuid: string;
@@ -9,24 +10,6 @@ interface TemplateMapping {
   targetPageTemplateName: string;
 }
 
-// Templates have no lastModifiedDate, so change detection compares the source and
-// target structure directly. Per-instance IDs (pageItemTemplateID, pageTemplateID,
-// contentViewID, contentDefinitionID, itemContainerID) are excluded — they always
-// differ between instances. Sections are sorted by reference name so payload
-// ordering doesn't matter; itemOrder still captures real reordering.
-function normalizeTemplate(template: mgmtApi.PageModel): string {
-  const sections = (template.contentSectionDefinitions || [])
-    .map((def) => ({
-      name: def.pageItemTemplateName ?? null,
-      referenceName: def.pageItemTemplateReferenceName ?? null,
-      type: def.pageItemTemplateType ?? null,
-      itemOrder: def.itemOrder ?? null,
-    }))
-    .sort((a, b) => String(a.referenceName).localeCompare(String(b.referenceName)));
-
-  return JSON.stringify({ pageTemplateName: template.pageTemplateName ?? null, sections });
-}
-
 export class TemplateMapper {
   private fileOps: fileOperations;
   private sourceGuid: string;
@@ -133,8 +116,61 @@ export class TemplateMapper {
     this.fileOps.saveMappingFile(this.mappings, this.directory, this.sourceGuid, this.targetGuid);
   }
 
-  hasTemplateChanged(sourceTemplate: mgmtApi.PageModel | null, targetTemplate: mgmtApi.PageModel | null): boolean {
+  // Templates have no lastModifiedDate, so change detection compares the source and target
+  // structure directly. Per-instance IDs (pageItemTemplateID, pageTemplateID, contentViewID,
+  // contentDefinitionID, itemContainerID) are excluded — they always differ between instances.
+  //
+  // PROD-2350: pair each source section with its target counterpart by the persisted
+  // pageItemTemplateID mapping when one is available — that's the actual identity, not a
+  // name-based guess — and only fall back to a reference-name match when no mapping exists yet
+  // (bootstrap, or sectionMapper not supplied). Matching purely by reference name can silently
+  // miss a real change (e.g. two sections trade reference names and item orders at once) since
+  // it re-derives the pairing from a value that isn't guaranteed to still identify the section.
+  hasTemplateChanged(
+    sourceTemplate: mgmtApi.PageModel | null,
+    targetTemplate: mgmtApi.PageModel | null,
+    sectionMapper?: SectionMapper | null
+  ): boolean {
     if (!sourceTemplate || !targetTemplate) return false;
-    return normalizeTemplate(sourceTemplate) !== normalizeTemplate(targetTemplate);
+
+    if ((sourceTemplate.pageTemplateName ?? null) !== (targetTemplate.pageTemplateName ?? null)) return true;
+
+    const sourceSections = sourceTemplate.contentSectionDefinitions || [];
+    const targetSections = targetTemplate.contentSectionDefinitions || [];
+
+    if (sourceSections.length !== targetSections.length) return true;
+
+    for (const sourceSection of sourceSections) {
+      if (!sourceSection) return true;
+
+      let targetSection: mgmtApi.ContentSectionDefinition | null = null;
+
+      if (sectionMapper && sourceSection.pageItemTemplateID != null) {
+        const sectionMapping = sectionMapper.getSectionMappingByID(sourceSection.pageItemTemplateID, "source");
+        if (sectionMapping) {
+          targetSection =
+            targetSections.find((t) => t?.pageItemTemplateID === sectionMapping.targetPageItemTemplateID) ?? null;
+        }
+      }
+
+      if (!targetSection) {
+        targetSection =
+          targetSections.find(
+            (t) => t?.pageItemTemplateReferenceName === sourceSection.pageItemTemplateReferenceName
+          ) ?? null;
+      }
+
+      if (!targetSection) return true;
+
+      const sectionChanged =
+        (sourceSection.pageItemTemplateName ?? null) !== (targetSection.pageItemTemplateName ?? null) ||
+        (sourceSection.pageItemTemplateReferenceName ?? null) !== (targetSection.pageItemTemplateReferenceName ?? null) ||
+        (sourceSection.pageItemTemplateType ?? null) !== (targetSection.pageItemTemplateType ?? null) ||
+        (sourceSection.itemOrder ?? null) !== (targetSection.itemOrder ?? null);
+
+      if (sectionChanged) return true;
+    }
+
+    return false;
   }
 }

src/lib/mappers/tests/section-mapper.test.ts

@@ -0,0 +1,158 @@
+import * as fs from "fs";
+import * as os from "os";
+import * as path from "path";
+import { resetState, setState } from "core/state";
+import { SectionMapper } from "lib/mappers/section-mapper";
+
+let tmpDir: string;
+
+beforeAll(() => {
+  tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "agility-section-mapper-"));
+});
+
+afterAll(() => {
+  fs.rmSync(tmpDir, { recursive: true, force: true });
+});
+
+beforeEach(() => {
+  resetState();
+  setState({ rootPath: tmpDir });
+  jest.spyOn(console, "log").mockImplementation(() => {});
+  jest.spyOn(console, "warn").mockImplementation(() => {});
+  jest.spyOn(console, "error").mockImplementation(() => {});
+});
+
+afterEach(() => {
+  jest.restoreAllMocks();
+});
+
+let testCounter = 0;
+let currentSrc: string;
+let currentTgt: string;
+
+function makeMapper(): SectionMapper {
+  testCounter++;
+  currentSrc = `src-${testCounter}`;
+  currentTgt = `tgt-${testCounter}`;
+  return new SectionMapper(currentSrc, currentTgt);
+}
+
+function makeSection(overrides: Record<string, any> = {}): any {
+  return {
+    pageItemTemplateID: 1,
+    pageItemTemplateReferenceName: "Main",
+    itemOrder: 0,
+    contentViewID: 100,
+    ...overrides,
+  };
+}
+
+// ─── constructor ──────────────────────────────────────────────────────────────
+
+describe("SectionMapper constructor", () => {
+  it("constructs without throwing", () => {
+    expect(() => makeMapper()).not.toThrow();
+  });
+});
+
+// ─── getSectionMapping ────────────────────────────────────────────────────────
+
+describe("SectionMapper.getSectionMapping", () => {
+  it("returns null when section is null", () => {
+    const mapper = makeMapper();
+    expect(mapper.getSectionMapping(null as any, "source")).toBeNull();
+  });
+
+  it("returns null when no mapping exists", () => {
+    const mapper = makeMapper();
+    expect(mapper.getSectionMapping(makeSection({ pageItemTemplateID: 999 }), "source")).toBeNull();
+  });
+
+  it("finds mapping by source pageItemTemplateID after addMapping", () => {
+    const mapper = makeMapper();
+    mapper.addMapping(
+      makeSection({ pageItemTemplateID: 10, pageItemTemplateReferenceName: "SourceMain" }),
+      makeSection({ pageItemTemplateID: 20, pageItemTemplateReferenceName: "TargetMain" })
+    );
+    const found = mapper.getSectionMapping(makeSection({ pageItemTemplateID: 10 }), "source");
+    expect(found).not.toBeNull();
+    expect(found!.targetPageItemTemplateID).toBe(20);
+  });
+
+  it("finds mapping by target pageItemTemplateID after addMapping", () => {
+    const mapper = makeMapper();
+    mapper.addMapping(makeSection({ pageItemTemplateID: 10 }), makeSection({ pageItemTemplateID: 20 }));
+    const found = mapper.getSectionMapping(makeSection({ pageItemTemplateID: 20 }), "target");
+    expect(found!.sourcePageItemTemplateID).toBe(10);
+  });
+});
+
+// ─── getSectionMappingByID ────────────────────────────────────────────────────
+
+describe("SectionMapper.getSectionMappingByID", () => {
+  it("returns null for unknown ID", () => {
+    const mapper = makeMapper();
+    expect(mapper.getSectionMappingByID(999, "source")).toBeNull();
+  });
+
+  it("returns mapping by source ID", () => {
+    const mapper = makeMapper();
+    mapper.addMapping(makeSection({ pageItemTemplateID: 5 }), makeSection({ pageItemTemplateID: 6 }));
+    expect(mapper.getSectionMappingByID(5, "source")).not.toBeNull();
+  });
+
+  it("returns mapping by target ID", () => {
+    const mapper = makeMapper();
+    mapper.addMapping(makeSection({ pageItemTemplateID: 5 }), makeSection({ pageItemTemplateID: 6 }));
+    expect(mapper.getSectionMappingByID(6, "target")).not.toBeNull();
+  });
+
+  // PROD-2350: the whole point of the ID mapping — a rename on either side shouldn't break
+  // the lookup once the mapping has been established.
+  it("still resolves after the source section is renamed", () => {
+    const mapper = makeMapper();
+    mapper.addMapping(
+      makeSection({ pageItemTemplateID: 5, pageItemTemplateReferenceName: "OldName" }),
+      makeSection({ pageItemTemplateID: 6, pageItemTemplateReferenceName: "Main" })
+    );
+    const found = mapper.getSectionMappingByID(5, "source");
+    expect(found!.targetPageItemTemplateID).toBe(6);
+  });
+});
+
+// ─── addMapping / updateMapping ───────────────────────────────────────────────
+
+describe("SectionMapper.addMapping", () => {
+  it("adds a new mapping", () => {
+    const mapper = makeMapper();
+    mapper.addMapping(makeSection({ pageItemTemplateID: 10 }), makeSection({ pageItemTemplateID: 20 }));
+    expect(mapper.getSectionMappingByID(20, "target")).not.toBeNull();
+  });
+
+  it("updates existing mapping when target already exists", () => {
+    const mapper = makeMapper();
+    const tgt = makeSection({ pageItemTemplateID: 20 });
+    mapper.addMapping(makeSection({ pageItemTemplateID: 10, pageItemTemplateReferenceName: "Old" }), tgt);
+    mapper.addMapping(makeSection({ pageItemTemplateID: 11, pageItemTemplateReferenceName: "New" }), tgt);
+    const found = mapper.getSectionMappingByID(20, "target")!;
+    expect(found.sourcePageItemTemplateID).toBe(11);
+    expect(found.sourceReferenceName).toBe("New");
+  });
+
+  it("throws when source and target already point at different mappings", () => {
+    const mapper = makeMapper();
+    mapper.addMapping(makeSection({ pageItemTemplateID: 1 }), makeSection({ pageItemTemplateID: 2 }));
+    mapper.addMapping(makeSection({ pageItemTemplateID: 3 }), makeSection({ pageItemTemplateID: 4 }));
+    expect(() =>
+      mapper.addMapping(makeSection({ pageItemTemplateID: 1 }), makeSection({ pageItemTemplateID: 4 }))
+    ).toThrow(/Invalid Mappings detected/);
+  });
+
+  it("persists across mapper instances for the same guid pair", () => {
+    const mapper = makeMapper();
+    mapper.addMapping(makeSection({ pageItemTemplateID: 10 }), makeSection({ pageItemTemplateID: 20 }));
+
+    const reloaded = new SectionMapper(currentSrc, currentTgt);
+    expect(reloaded.getSectionMappingByID(10, "source")).not.toBeNull();
+  });
+});