Refactor duplicated issue/PR update payload normalization into shared helper #40624
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| // @ts-check | ||
| /// <reference types="@actions/github-script" /> | ||
|
|
||
| const { sanitizeTitle } = require("./sanitize_title.cjs"); | ||
| const { parseBoolTemplatable } = require("./templatable.cjs"); | ||
|
|
||
| /** | ||
| * Build shared update payload fields for issue/PR update handlers. | ||
| * | ||
| * @param {Object} item | ||
| * @param {Object} config | ||
| * @param {Object} options | ||
| * @param {boolean} [options.allowTitle=true] | ||
| * @param {string} [options.defaultOperation] - Required when item.body may be present; used as fallback operation if item.operation and configDefaultOperation are both absent. | ||
| * @param {string | undefined} [options.configDefaultOperation] | ||
| * @param {boolean} [options.includeBodyInApiData=false] | ||
| * @param {(() => void) | undefined} [options.onBodyDisallowed] | ||
| * @returns {{updateData: Object, hasCommonUpdates: boolean}} | ||
| */ | ||
| function buildCommonEntityUpdateData(item, config, options = {}) { | ||
| const { allowTitle = true, defaultOperation, configDefaultOperation, includeBodyInApiData = false, onBodyDisallowed } = options; | ||
|
|
||
| const updateData = {}; | ||
| let hasCommonUpdates = false; | ||
|
|
||
| if (allowTitle && item.title !== undefined) { | ||
| updateData.title = sanitizeTitle(item.title); | ||
| hasCommonUpdates = true; | ||
| } | ||
|
|
||
| const canUpdateBody = config.allow_body !== false; | ||
| if (item.body !== undefined && canUpdateBody) { | ||
| const resolvedOperation = item.operation || configDefaultOperation || defaultOperation; | ||
| if (!resolvedOperation) { | ||
| throw new Error("buildCommonEntityUpdateData: defaultOperation is required when body may be present"); | ||
| } | ||
| updateData._operation = resolvedOperation; | ||
| updateData._rawBody = item.body; | ||
| if (includeBodyInApiData) { | ||
| updateData.body = item.body; | ||
| } | ||
| hasCommonUpdates = true; | ||
| } else if (item.body !== undefined && !canUpdateBody && typeof onBodyDisallowed === "function") { | ||
| onBodyDisallowed(); | ||
| } | ||
|
|
||
| // Always populate _includeFooter: downstream executeUpdate reads it regardless of | ||
| // whether title/body changed, matching pre-refactor behavior in both callers. | ||
| updateData._includeFooter = parseBoolTemplatable(config.footer, true); | ||
|
|
||
| return { updateData, hasCommonUpdates }; | ||
| } | ||
|
|
||
| module.exports = { buildCommonEntityUpdateData }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| import { describe, it, expect, vi } from "vitest"; | ||
| const { buildCommonEntityUpdateData } = require("./update_entity_helpers.cjs"); | ||
|
|
||
| describe("update_entity_helpers.cjs - buildCommonEntityUpdateData", () => { | ||
| it("returns hasCommonUpdates true and populates title, body fields, and footer when title and body are provided", () => { | ||
| const result = buildCommonEntityUpdateData( | ||
| { title: "New title", body: "Body text" }, | ||
| {}, | ||
| { | ||
| defaultOperation: "append", | ||
| } | ||
| ); | ||
|
|
||
| expect(result.updateData.title).toBe("New title"); | ||
| expect(result.updateData._operation).toBe("append"); | ||
| expect(result.updateData._rawBody).toBe("Body text"); | ||
| expect(result.updateData._includeFooter).toBe(true); | ||
| expect(result.hasCommonUpdates).toBe(true); | ||
| }); | ||
|
|
||
| it("prefers configDefaultOperation over defaultOperation for body operation", () => { | ||
| const result = buildCommonEntityUpdateData( | ||
|
Contributor
There was a problem hiding this comment.
💡 Suggested additionThe resolution chain it("item.operation takes precedence over configDefaultOperation and defaultOperation", () => {
const result = buildCommonEntityUpdateData(
{ body: "Body text", operation: "prepend" },
{},
{
defaultOperation: "append",
configDefaultOperation: "replace",
}
);
expect(result.updateData._operation).toBe("prepend");
});Without this, a regression that swaps the precedence order (e.g.,
Contributor
Author
There was a problem hiding this comment. Added |
||
| { body: "Body text" }, | ||
| {}, | ||
| { | ||
| defaultOperation: "append", | ||
| configDefaultOperation: "replace", | ||
| } | ||
| ); | ||
|
|
||
| expect(result.updateData._operation).toBe("replace"); | ||
| }); | ||
|
|
||
| it("includes body in api data when includeBodyInApiData is true", () => { | ||
| const result = buildCommonEntityUpdateData( | ||
| { body: "Body text" }, | ||
| {}, | ||
| { | ||
| defaultOperation: "append", | ||
| includeBodyInApiData: true, | ||
| } | ||
| ); | ||
|
|
||
| expect(result.updateData.body).toBe("Body text"); | ||
| }); | ||
|
|
||
| it("item.operation takes precedence over configDefaultOperation and defaultOperation", () => { | ||
| const result = buildCommonEntityUpdateData( | ||
| { body: "Body text", operation: "prepend" }, | ||
| {}, | ||
| { | ||
| defaultOperation: "append", | ||
| configDefaultOperation: "replace", | ||
| } | ||
| ); | ||
|
|
||
| expect(result.updateData._operation).toBe("prepend"); | ||
| }); | ||
|
|
||
| it("skips title when allowTitle is false and does not set hasCommonUpdates", () => { | ||
| const result = buildCommonEntityUpdateData({ title: "Should be ignored" }, {}, { allowTitle: false, defaultOperation: "append" }); | ||
|
|
||
| expect(result.updateData.title).toBeUndefined(); | ||
| expect(result.hasCommonUpdates).toBe(false); | ||
| }); | ||
|
|
||
| it("invokes onBodyDisallowed when body updates are blocked", () => { | ||
|
Contributor
There was a problem hiding this comment.
💡 Suggested addition
it("blocks title update when allowTitle is false and does not set hasCommonUpdates", () => {
const result = buildCommonEntityUpdateData(
{ title: "Should be ignored" },
{},
{ allowTitle: false, defaultOperation: "append" }
);
expect(result.updateData.title).toBeUndefined();
expect(result.hasCommonUpdates).toBe(false);
});
Contributor
Author
There was a problem hiding this comment. Covered by the same |
||
| const onBodyDisallowed = vi.fn(); | ||
|
|
||
| const result = buildCommonEntityUpdateData( | ||
| { body: "Body text" }, | ||
| { allow_body: false }, | ||
| { | ||
| defaultOperation: "append", | ||
| onBodyDisallowed, | ||
| } | ||
| ); | ||
|
|
||
| expect(onBodyDisallowed).toHaveBeenCalledTimes(1); | ||
| expect(result.updateData._rawBody).toBeUndefined(); | ||
| expect(result.hasCommonUpdates).toBe(false); | ||
| }); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. [/tdd] Missing The 💡 Suggested testit('skips title when allowTitle is false', () => {
const result = buildCommonEntityUpdateData(
{ title: 'Should not appear' },
{},
{ allowTitle: false }
);
expect(result.updateData.title).toBeUndefined();
expect(result.hasCommonUpdates).toBe(false);
});This is the primary guard used by
Contributor
Author
There was a problem hiding this comment. Added |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/zoom-out]
_includeFooteris always written toupdateData— even when both title and body are absent andhasCommonUpdateswill befalse. This matches pre-refactor behavior in both callers (intentional), but a brief comment here would protect against a future maintainer wrapping it in anif (hasCommonUpdates)guard and inadvertently breaking the footer on no-op calls.💡 Suggested inline comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the inline comment in 8cf7e35.