Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,20 @@ export const matchingDraftPlanParams = ({
upsertMatchesDraftEntry({ upsertProductPlan, planParams }),
);

/**
* A request-level draft claims every row, leaving `rowCanReceiveMigrationDraft`
* and the diff to decide which actually get one — a caller pushing a whole
* catalog cannot know which plans hold customers.
*/
export const upsertClaimsMigrationDraft = ({
upsertProductPlan,
params,
}: {
upsertProductPlan: UpsertProductPlan;
params: UpdateCatalogParams;
}): boolean => matchingDraftPlanParams({ upsertProductPlan, params }) != null;
}): boolean =>
params.migration?.draft === true ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: A request-level migration.draft claims every row and short-circuits per-plan flags, so no plan entry can opt a row back out of drafting. The schema comment in this PR calls the field "overridable per plan," but params.migration?.draft === true || runs before matchingDraftPlanParams, so migration: { draft: false } on a plan is ignored once the request-level flag is set. A config client that wants to exclude a specific plan from a request-wide draft has no way to do so.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/catalogV2/actions/updateCatalog/compute/computeMigrationDraftPlans/matchingDraftPlanParams.ts, line 87:

<comment>A request-level `migration.draft` claims every row and short-circuits per-plan flags, so no plan entry can opt a row back out of drafting. The schema comment in this PR calls the field "overridable per plan," but `params.migration?.draft === true ||` runs before `matchingDraftPlanParams`, so `migration: { draft: false }` on a plan is ignored once the request-level flag is set. A config client that wants to exclude a specific plan from a request-wide draft has no way to do so.</comment>

<file context>
@@ -72,13 +72,20 @@ export const matchingDraftPlanParams = ({
 	params: UpdateCatalogParams;
-}): boolean => matchingDraftPlanParams({ upsertProductPlan, params }) != null;
+}): boolean =>
+	params.migration?.draft === true ||
+	matchingDraftPlanParams({ upsertProductPlan, params }) != null;
 
</file context>

matchingDraftPlanParams({ upsertProductPlan, params }) != null;
Comment on lines +87 to +88

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Honor per-plan opt-outs from catalog migration drafts

When a request sets catalog-level migration.draft: true but a plan explicitly sets migration: { draft: false }, this unconditional OR still claims that plan's rows. This violates the documented per-plan override behavior and creates migration drafts for customered edits that explicitly opted out; resolve the matching plan's boolean first and fall back to the request-level default only when it is unset.

Useful? React with 👍 / 👎.


export const includeCustomForMigrationDraft = ({
upsertProductPlan,
Expand All @@ -88,4 +95,4 @@ export const includeCustomForMigrationDraft = ({
params: UpdateCatalogParams;
}): boolean =>
matchingDraftPlanParams({ upsertProductPlan, params })?.migration
?.include_custom === true;
?.include_custom === true || params.migration?.include_custom === true;
Comment on lines 97 to +98

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Honor per-plan include_custom overrides

When catalog-level include_custom is true and a claimed plan explicitly specifies include_custom: false, the OR still returns true, so that plan's draft includes customized customers despite its override. Use the matched plan's value when present and only fall back to the catalog-level value when it is undefined.

Useful? React with 👍 / 👎.

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import type { FullProduct } from "@autumn/shared";

export type OrphanedActivePointer = {
planId: string;
version: number;
};

const rowsByPlanId = ({
products,
}: {
products: FullProduct[];
}): Map<string, FullProduct[]> => {
const byPlanId = new Map<string, FullProduct[]>();
for (const product of products) {
const group = byPlanId.get(product.id) ?? [];
group.push(product);
byPlanId.set(product.id, group);
}
return byPlanId;
};

/**
* Projected plans still holding a live version while the version customers
* attach to is archived. Archiving every version is a plan going away and is
* left alone.
*/
export const detectOrphanedActivePointers = ({
products,
}: {
products: FullProduct[];
}): OrphanedActivePointer[] => {
const orphaned: OrphanedActivePointer[] = [];

for (const [planId, rows] of rowsByPlanId({ products })) {
const survivesArchive = rows.some((row) => !row.archived);
if (!survivesArchive) continue;

const activeRow = rows.find((row) => row.active);
if (!activeRow?.archived) continue;

orphaned.push({ planId, version: activeRow.version });
}

return orphaned;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { ErrCode, RecaseError } from "@autumn/shared";
import { detectOrphanedActivePointers } from "@/internal/catalogV2/actions/updateCatalog/errors/detectOrphanedActivePointers";
import type { UpdateCatalogPlan } from "@/internal/catalogV2/actions/updateCatalog/types/updateCatalogPlan";

/** A plan keeping live versions must keep a live active one to attach to. */
export const handleActivePointerErrors = ({
updateCatalogPlan,
}: {
updateCatalogPlan: UpdateCatalogPlan;
}): void => {
const [orphaned] = detectOrphanedActivePointers({
products: updateCatalogPlan.projected.products,
});
if (!orphaned) return;

throw new RecaseError({
message: `Cannot archive version ${orphaned.version} of plan ${orphaned.planId} while it is the active version and other versions remain. Promote another version first, or archive the whole plan.`,
code: ErrCode.InvalidRequest,
statusCode: 400,
});
};
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
} from "@autumn/shared";
import type { AutumnContext } from "@/honoUtils/HonoEnv";
import { assertInternalIdAgrees } from "@/internal/catalogV2/actions/updateCatalog/errors/assertInternalIdAgrees";
import { handleActivePointerErrors } from "@/internal/catalogV2/actions/updateCatalog/errors/handleActivePointerErrors";
import { handleDeclaredVariantAnchorErrors } from "@/internal/catalogV2/actions/updateCatalog/errors/handleDeclaredVariantAnchorErrors";
import { handleLicenseAnchorLifecycleErrors } from "@/internal/catalogV2/actions/updateCatalog/errors/handleLicenseAnchorLifecycleErrors";
import { handleRemoveFeatureErrors } from "@/internal/catalogV2/actions/updateCatalog/errors/handleRemoveFeatureErrors/handleRemoveFeatureErrors";
Expand Down Expand Up @@ -177,6 +178,7 @@ export const handleUpdateCatalogErrors = async ({
});
handleUpsertProductVersionSlugErrors({ updateCatalogPlan });
handleUpsertProductActiveErrors({ params });
handleActivePointerErrors({ updateCatalogPlan });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Scope active-pointer validation to newly invalidated plans

When an organization already contains an archived active version beside a live sibling—a state the existing pin-archive flow deliberately persisted and version-identity-remove.test.ts asserts—projected.products carries that state into every subsequent catalog request. Calling this validator unconditionally therefore makes even unrelated feature or plan updates fail with 400 after deployment; compare against the original state or restrict the check to plans whose pointer/archive state this request changes.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: detectOrphanedActivePointers only catches the case where the active version remains in the projection as archived. When a live active version is removed as a single (non-all_versions) version with no customers and no rewards, stampRemoveWillArchive hard-deletes it (willArchive=false), so projectCatalog drops the row entirely. The plan's surviving versions then have no active row, activeRow is undefined, and the orphaned active pointer passes this check — exactly the state the feature docstring says must be prevented. Consider flagging plans that have a surviving non-archived version but no active row. Note the primary archive path (active version removed with a surviving version) is detected correctly.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/catalogV2/actions/updateCatalog/errors/handleUpdateCatalogErrors.ts, line 181:

<comment>detectOrphanedActivePointers only catches the case where the active version remains in the projection as archived. When a live active version is removed as a single (non-all_versions) version with no customers and no rewards, stampRemoveWillArchive hard-deletes it (willArchive=false), so projectCatalog drops the row entirely. The plan's surviving versions then have no active row, activeRow is undefined, and the orphaned active pointer passes this check — exactly the state the feature docstring says must be prevented. Consider flagging plans that have a surviving non-archived version but no active row. Note the primary archive path (active version removed with a surviving version) is detected correctly.</comment>

<file context>
@@ -177,6 +178,7 @@ export const handleUpdateCatalogErrors = async ({
 	});
 	handleUpsertProductVersionSlugErrors({ updateCatalogPlan });
 	handleUpsertProductActiveErrors({ params });
+	handleActivePointerErrors({ updateCatalogPlan });
 	handleUpsertProductErrors({
 		updateCatalogPlan,
</file context>

handleUpsertProductErrors({
updateCatalogPlan,
productStatesContext: catalogContext.productStatesContext,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
/**
* catalogV2.update — the server decides which rows need a migration draft.
*
* A draft has only ever appeared when the caller set `migration: { draft: true }`
* on the very entry it wanted drafted, which means the caller had to work out
* which plans carry customers. A config-file client cannot: it pushes the whole
* catalog and knows nothing about who is on what. Request-level `migration`
* says "draft wherever one is warranted" and hands that judgement to the
* server, which already answers it per row.
*
* Contract:
* E1 a request-level draft covers a customered row no entry named
* E2 the same push drafts nothing for a plan with no customers
* E3 both edits still land in place — neither mints a version
* E4 without the flag nothing drafts, so existing callers are untouched
*
* Red (current): the claim is per-entry only, so a request-level flag is
* ignored and E1 finds no migration at all.
* Green (after): the request-level flag claims every row, and the existing row
* gate — customers, not a mint, neither side archived, non-empty diff —
* decides which of them actually produces one.
*/

import { test } from "bun:test";
import { ResetInterval } from "@autumn/shared";
import { TestFeature } from "@tests/setup/v2Features.js";
import { initScenario } from "@tests/utils/testInitUtils/initScenario.js";
import chalk from "chalk";
import { uniqueTestId } from "../../utils/uniqueTestId.js";
import { cleanupPlanCustomerRefs } from "../utils/cleanupPlanCustomerRefs.js";
import {
deleteDbPlans,
expectCatalogPlansCorrect,
} from "../utils/expectCatalogPlans.js";
import {
deleteMigrations,
expectUpdateMigrations,
} from "./utils/expectMigrationDrafts.js";
import { seedVersionableCustomer } from "./utils/seedVersionableCustomer.js";

const messagesItem = ({ included }: { included: number }) => ({
feature_id: TestFeature.Messages,
included,
reset: { interval: ResetInterval.Month },
});

test.concurrent(
`${chalk.yellowBright("catalogV2 migration: a request-level draft covers the customered row only")}`,
async () => {
const { autumnV2_3, ctx } = await initScenario({ setup: [], actions: [] });
const customeredId = uniqueTestId("cv2_auto_held");
const quietId = uniqueTestId("cv2_auto_quiet");
await deleteDbPlans({ ctx, planIds: [customeredId, quietId] });

try {
await autumnV2_3.catalogV2.update({
plans: [
{
plan_id: customeredId,
name: "Held",
items: [messagesItem({ included: 100 })],
},
{
plan_id: quietId,
name: "Quiet",
items: [messagesItem({ included: 100 })],
},
],
});
await seedVersionableCustomer({ ctx, planId: customeredId, version: 1 });

// Neither entry asks for a draft — the request as a whole does, which is
// all a config push can say. Only `customeredId` has anyone to move.
const response = await autumnV2_3.catalogV2.update({
migration: { draft: true },
plans: [
{ plan_id: customeredId, items: [messagesItem({ included: 250 })] },
{ plan_id: quietId, items: [messagesItem({ included: 250 })] },
],
});

// E1 + E2: exactly one draft, and it names the customered row.
expectUpdateMigrations({
response,
plans: [[{ plan_id: customeredId, versions: [1] }]],
});

// E3: a draft is what happens INSTEAD of blocking, so both edits applied
// to the row that was already there.
await expectCatalogPlansCorrect({
autumn: autumnV2_3,
expected: [
{
id: customeredId,
version: 1,
allowances: { [TestFeature.Messages]: 250 },
},
{
id: quietId,
version: 1,
allowances: { [TestFeature.Messages]: 250 },
},
],
});
} finally {
await deleteMigrations({ ctx, ids: [customeredId] });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: The migration draft persists with an id of {scope}-update-{uid} (see buildMigrationDraftId), never the plan id, but finally calls deleteMigrations({ ctx, ids: [customeredId] }) / [planId]. Since every sibling draft test (billing-flag, customize-buckets, draft-guards, filter-collapse) deletes via response.migrations![0]!.id, passing the plan id here deletes nothing, and the draft row created by the request-level flag leaks in the test DB. Capture the migration id from the response (as the other tests do) and delete that.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/tests/integration/catalog-v2/plans/migrations/auto-draft.test.ts, line 106:

<comment>The migration draft persists with an id of `{scope}-update-{uid}` (see buildMigrationDraftId), never the plan id, but `finally` calls `deleteMigrations({ ctx, ids: [customeredId] })` / `[planId]`. Since every sibling draft test (billing-flag, customize-buckets, draft-guards, filter-collapse) deletes via `response.migrations![0]!.id`, passing the plan id here deletes nothing, and the draft row created by the request-level flag leaks in the test DB. Capture the migration id from the response (as the other tests do) and delete that.</comment>

<file context>
@@ -0,0 +1,158 @@
+				],
+			});
+		} finally {
+			await deleteMigrations({ ctx, ids: [customeredId] });
+			await cleanupPlanCustomerRefs({
+				ctx,
</file context>

await cleanupPlanCustomerRefs({
ctx,
planIds: [customeredId, quietId],
});
await deleteDbPlans({ ctx, planIds: [customeredId, quietId] });
}
},
);

test.concurrent(
`${chalk.yellowBright("catalogV2 migration: a push with no migration params still drafts nothing")}`,
async () => {
const { autumnV2_3, ctx } = await initScenario({ setup: [], actions: [] });
const planId = uniqueTestId("cv2_auto_optout");
await deleteDbPlans({ ctx, planIds: [planId] });

try {
await autumnV2_3.catalogV2.update({
plans: [
{
plan_id: planId,
name: "Opt Out",
items: [messagesItem({ included: 100 })],
},
],
});
await seedVersionableCustomer({ ctx, planId, version: 1 });

// E4: the shape every existing caller sends. The row would qualify for a
// draft on every count except that nothing asked for one.
const response = await autumnV2_3.catalogV2.update({
plans: [{ plan_id: planId, items: [messagesItem({ included: 250 })] }],
});

expectUpdateMigrations({ response, plans: [] });
await expectCatalogPlansCorrect({
autumn: autumnV2_3,
expected: [
{
id: planId,
version: 1,
allowances: { [TestFeature.Messages]: 250 },
},
],
});
} finally {
await deleteMigrations({ ctx, ids: [planId] });
await cleanupPlanCustomerRefs({ ctx, planIds: [planId] });
await deleteDbPlans({ ctx, planIds: [planId] });
}
},
);
Loading
Loading