Skip to content
Merged
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
42 changes: 33 additions & 9 deletions actions/setup/js/update_entity_helpers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,42 @@
const { sanitizeTitle } = require("./sanitize_title.cjs");
const { parseBoolTemplatable } = require("./templatable.cjs");

/**
* @typedef {{ title?: string, body?: string, operation?: string }} EntityUpdateItem
Comment thread
pelikhan marked this conversation as resolved.
*/

/**
* @typedef {{ allow_body?: boolean, footer?: boolean | string }} EntityUpdateConfig
*/

/**
* @typedef {{ _includeFooter: boolean, title?: string, _operation?: string, _rawBody?: string, body?: string }} EntityUpdateDataBase
*/

/**
* @typedef {EntityUpdateDataBase & { [key: string]: any }} EntityUpdateData
*/

/**
* @typedef {{ updateData: EntityUpdateData, hasCommonUpdates: boolean }} EntityUpdateResult
*/

/**
* 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}}
* `options.defaultOperation` is required when `item.body` may be present;
* used as fallback when `item.operation` and `configDefaultOperation` are both absent.
*
* @param {EntityUpdateItem} item
* @param {EntityUpdateConfig} config
* @param {{
* allowTitle?: boolean,
* defaultOperation?: string,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

defaultOperation lost its critical behavioral note in the refactoring — the old JSDoc explicitly warned: "Required when item.body may be present; used as fallback operation if item.operation and configDefaultOperation are both absent." The new inline type just shows defaultOperation?: string, silently dropping the constraint that explains the runtime throw in the function body.

💡 Suggested fix

Restore the explanation as an inline JSDoc comment:

 * `@param` {{
 *   allowTitle?: boolean,
 *   /** Required when item.body may be present; fallback when item.operation and configDefaultOperation are both absent. */
 *   defaultOperation?: string,
 *   configDefaultOperation?: string,
 *   includeBodyInApiData?: boolean,
 *   onBodyDisallowed?: (() => void),
 * }} [options]

Without this note, callers who omit defaultOperation and later pass a body will hit an opaque runtime error with no guidance from hover docs.

* configDefaultOperation?: string,
* includeBodyInApiData?: boolean,
* onBodyDisallowed?: (() => void),
* }} [options]
* @returns {EntityUpdateResult}
*/
function buildCommonEntityUpdateData(item, config, options = {}) {
const { allowTitle = true, defaultOperation, configDefaultOperation, includeBodyInApiData = false, onBodyDisallowed } = options;
Expand Down
49 changes: 49 additions & 0 deletions actions/setup/js/update_entity_helpers.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,53 @@ describe("update_entity_helpers.cjs - buildCommonEntityUpdateData", () => {
expect(result.updateData._rawBody).toBeUndefined();
expect(result.hasCommonUpdates).toBe(false);
});

it("throws when body is present but no operation is resolvable", () => {
expect(() => buildCommonEntityUpdateData({ body: "Body text" }, {})).toThrow("buildCommonEntityUpdateData: defaultOperation is required when body may be present");
});

it("returns hasCommonUpdates false when neither title nor body is present", () => {
const result = buildCommonEntityUpdateData({}, {});

expect(result.hasCommonUpdates).toBe(false);
expect(result.updateData.title).toBeUndefined();
expect(result.updateData._rawBody).toBeUndefined();
expect(result.updateData._includeFooter).toBe(true);
});

it("populates _includeFooter false when config.footer is false", () => {
const result = buildCommonEntityUpdateData({}, { footer: false });

expect(result.updateData._includeFooter).toBe(false);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/tdd] The new footer test covers footer: false (boolean), but parseBoolTemplatable specifically handles the string "false" as a key case for resolved GitHub Actions expressions.

Adding a parallel test for config.footer = "false" would validate the templatable contract that the typedef's any type is designed to accommodate.

💡 Suggested additional test
it('populates _includeFooter false when config.footer is the string "false"', () => {
  const result = buildCommonEntityUpdateData({}, { footer: 'false' });

  expect(result.updateData._includeFooter).toBe(false);
});

This is the scenario that arises when a GitHub Actions workflow passes footer: ${{ inputs.include-footer }} and the runner resolves it to the string "false".

});

it('populates _includeFooter false when config.footer is the string "false"', () => {
const result = buildCommonEntityUpdateData({}, { footer: "false" });

expect(result.updateData._includeFooter).toBe(false);
});

it("does not include body in updateData.body by default when includeBodyInApiData is omitted", () => {
const result = buildCommonEntityUpdateData({ body: "Body text" }, {}, { defaultOperation: "append" });

expect(result.updateData._rawBody).toBe("Body text");
expect(result.updateData.body).toBeUndefined();
});

it("handles title-only update without body operation", () => {
const result = buildCommonEntityUpdateData({ title: "Only title" }, {});

expect(result.updateData.title).toBe("Only title");
expect(result.updateData._operation).toBeUndefined();
expect(result.updateData._rawBody).toBeUndefined();
expect(result.hasCommonUpdates).toBe(true);
});

it("does not call onBodyDisallowed when body is absent even if allow_body is false", () => {
const onBodyDisallowed = vi.fn();

buildCommonEntityUpdateData({}, { allow_body: false }, { onBodyDisallowed });

expect(onBodyDisallowed).not.toHaveBeenCalled();
});
});
Loading