-
Notifications
You must be signed in to change notification settings - Fork 475
fix(update_project): numeric field values now correctly infer NUMBER type #49073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1800,6 +1800,173 @@ describe("updateProject", () => { | |
| expect(updateCalls[0][1].value).toEqual({ text: "high" }); | ||
| }); | ||
|
|
||
| it("creates a new NUMBER field when field doesn't exist and value is a finite number", async () => { | ||
| const projectUrl = "https://github.com/orgs/testowner/projects/60"; | ||
| const output = { | ||
| type: "update_project", | ||
| project: projectUrl, | ||
| content_type: "issue", | ||
| content_number: 101, | ||
| fields: { | ||
| comment_count: 30, | ||
| }, | ||
| }; | ||
|
|
||
| queueResponses([ | ||
| repoResponse(), | ||
| viewerResponse(), | ||
| orgProjectV2Response(projectUrl, 60, "project-create-number-field"), | ||
| issueResponse("issue-id-101"), | ||
| existingItemResponse("issue-id-101", "item-create-number-field"), | ||
| // No existing fields - will need to create as NUMBER | ||
| fieldsResponse([]), | ||
| // Response for creating the NUMBER field | ||
| { | ||
| createProjectV2Field: { | ||
| projectV2Field: { | ||
| id: "field-comment-count", | ||
| name: "Comment Count", | ||
| dataType: "NUMBER", | ||
| }, | ||
| }, | ||
| }, | ||
| updateFieldValueResponse(), | ||
| ]); | ||
|
|
||
| await updateProject(output); | ||
|
|
||
| // Verify that the NUMBER field was created | ||
| const createCalls = mockGithub.graphql.mock.calls.filter(([query]) => query.includes("createProjectV2Field")); | ||
| expect(createCalls.length).toBe(1); | ||
| expect(createCalls[0][1].dataType).toBe("NUMBER"); | ||
| expect(createCalls[0][1].name).toBe("Comment Count"); | ||
| expect(createCalls[0][1].options).toBeUndefined(); | ||
|
|
||
| // Verify the field value was set as a number | ||
| const updateCalls = mockGithub.graphql.mock.calls.filter(([query]) => query.includes("updateProjectV2ItemFieldValue")); | ||
| expect(updateCalls.length).toBe(1); | ||
| expect(updateCalls[0][1].value).toEqual({ number: 30 }); | ||
|
|
||
| // Verify no type mismatch warning was emitted | ||
| expect(mockCore.warning).not.toHaveBeenCalledWith(expect.stringContaining("Field type mismatch")); | ||
| }); | ||
|
|
||
| it("creates a NUMBER field for zero value and updates without warning", async () => { | ||
| const projectUrl = "https://github.com/orgs/testowner/projects/60"; | ||
| const output = { | ||
| type: "update_project", | ||
| project: projectUrl, | ||
| content_type: "issue", | ||
| content_number: 102, | ||
| fields: { | ||
| comment_count: 0, | ||
| }, | ||
| }; | ||
|
|
||
| queueResponses([ | ||
| repoResponse(), | ||
| viewerResponse(), | ||
| orgProjectV2Response(projectUrl, 60, "project-create-number-field-zero"), | ||
| issueResponse("issue-id-102"), | ||
| existingItemResponse("issue-id-102", "item-create-number-field-zero"), | ||
| fieldsResponse([]), | ||
| { | ||
| createProjectV2Field: { | ||
| projectV2Field: { | ||
| id: "field-comment-count-zero", | ||
| name: "Comment Count", | ||
| dataType: "NUMBER", | ||
| }, | ||
| }, | ||
| }, | ||
| updateFieldValueResponse(), | ||
| ]); | ||
|
|
||
| await updateProject(output); | ||
|
|
||
| const updateCalls = mockGithub.graphql.mock.calls.filter(([query]) => query.includes("updateProjectV2ItemFieldValue")); | ||
| expect(updateCalls.length).toBe(1); | ||
| expect(updateCalls[0][1].value).toEqual({ number: 0 }); | ||
|
|
||
| expect(mockCore.warning).not.toHaveBeenCalledWith(expect.stringContaining("Field type mismatch")); | ||
| }); | ||
|
|
||
| it("updates an existing NUMBER field with a numeric value without spurious type mismatch warning", async () => { | ||
| const projectUrl = "https://github.com/orgs/testowner/projects/60"; | ||
| const output = { | ||
| type: "update_project", | ||
| project: projectUrl, | ||
| content_type: "issue", | ||
| content_number: 103, | ||
| fields: { | ||
| "Comment Count": 30, | ||
| }, | ||
| }; | ||
|
|
||
| queueResponses([ | ||
| repoResponse(), | ||
| viewerResponse(), | ||
| orgProjectV2Response(projectUrl, 60, "project-existing-number-field"), | ||
| issueResponse("issue-id-103"), | ||
| existingItemResponse("issue-id-103", "item-existing-number-field"), | ||
| fieldsResponse([{ id: "field-comment-count", name: "Comment Count", dataType: "NUMBER" }]), | ||
| updateFieldValueResponse(), | ||
| ]); | ||
|
|
||
| await updateProject(output); | ||
|
|
||
| const updateCall = mockGithub.graphql.mock.calls.find(([query]) => query.includes("updateProjectV2ItemFieldValue")); | ||
| expect(updateCall).toBeDefined(); | ||
| expect(updateCall[1].value).toEqual({ number: 30 }); | ||
|
|
||
| // No spurious type mismatch warning should be logged | ||
| expect(mockCore.warning).not.toHaveBeenCalledWith(expect.stringContaining("Field type mismatch")); | ||
| }); | ||
|
|
||
| it("creates a NUMBER field when field name contains 'date' but value is numeric", async () => { | ||
| const projectUrl = "https://github.com/orgs/testowner/projects/60"; | ||
| const output = { | ||
| type: "update_project", | ||
| project: projectUrl, | ||
| content_type: "issue", | ||
| content_number: 104, | ||
| fields: { | ||
| update_date_count: 30, | ||
| }, | ||
| }; | ||
|
|
||
| queueResponses([ | ||
| repoResponse(), | ||
| viewerResponse(), | ||
| orgProjectV2Response(projectUrl, 60, "project-date-name-number-field"), | ||
| issueResponse("issue-id-104"), | ||
| existingItemResponse("issue-id-104", "item-date-name-number-field"), | ||
| fieldsResponse([]), | ||
| { | ||
| createProjectV2Field: { | ||
| projectV2Field: { | ||
| id: "field-update-date-count", | ||
| name: "Update Date Count", | ||
| dataType: "NUMBER", | ||
| }, | ||
| }, | ||
| }, | ||
| updateFieldValueResponse(), | ||
| ]); | ||
|
|
||
| await updateProject(output); | ||
|
|
||
| const createCalls = mockGithub.graphql.mock.calls.filter(([query]) => query.includes("createProjectV2Field")); | ||
| expect(createCalls.length).toBe(1); | ||
| expect(createCalls[0][1].dataType).toBe("NUMBER"); | ||
|
|
||
| const updateCalls = mockGithub.graphql.mock.calls.filter(([query]) => query.includes("updateProjectV2ItemFieldValue")); | ||
| expect(updateCalls.length).toBe(1); | ||
| expect(updateCalls[0][1].value).toEqual({ number: 30 }); | ||
|
|
||
| expect(mockCore.warning).not.toHaveBeenCalledWith(expect.stringContaining("looks like a date field")); | ||
| }); | ||
|
|
||
| it("should reject update_project message with missing project field", async () => { | ||
| const messageHandler = await updateProjectHandlerFactory({}); | ||
|
|
||
|
|
@@ -2454,8 +2621,20 @@ describe("inferFieldDataType", () => { | |
| expect(inferFieldDataType("priority", "High", datePattern)).toBe("SINGLE_SELECT"); | ||
| }); | ||
|
|
||
| it("returns SINGLE_SELECT for numeric field value", () => { | ||
| expect(inferFieldDataType("score", 42, datePattern)).toBe("SINGLE_SELECT"); | ||
| it("returns NUMBER for finite numeric field value", () => { | ||
| expect(inferFieldDataType("score", 42, datePattern)).toBe("NUMBER"); | ||
| }); | ||
|
|
||
| it("returns NUMBER for zero value", () => { | ||
| expect(inferFieldDataType("count", 0, datePattern)).toBe("NUMBER"); | ||
| }); | ||
|
|
||
| it("returns NUMBER for negative numeric field value", () => { | ||
| expect(inferFieldDataType("delta", -5, datePattern)).toBe("NUMBER"); | ||
| }); | ||
|
|
||
| it("returns SINGLE_SELECT for non-finite numeric value (Infinity)", () => { | ||
| expect(inferFieldDataType("score", Infinity, datePattern)).toBe("SINGLE_SELECT"); | ||
| }); | ||
|
|
||
| it("returns SINGLE_SELECT for boolean field value", () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] Missing test: 💡 Suggested testit('returns SINGLE_SELECT for NaN', () => {
expect(inferFieldDataType('score', NaN, datePattern)).toBe('SINGLE_SELECT');
});
@copilot please address this. |
||
|
|
@@ -2469,4 +2648,8 @@ describe("inferFieldDataType", () => { | |
| it("returns SINGLE_SELECT for date field name with invalid date format", () => { | ||
| expect(inferFieldDataType("end_date", "2024/06/30", datePattern)).toBe("SINGLE_SELECT"); | ||
| }); | ||
|
|
||
| it("returns NUMBER for a field name containing 'date' with a numeric value", () => { | ||
| expect(inferFieldDataType("update_date_count", 30, datePattern)).toBe("NUMBER"); | ||
| }); | ||
| }); | ||
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.
Any numeric YAML value is now inferred as
NUMBER, which is a breaking regression for pre-existingSINGLE_SELECTfields that legitimately hold numeric-looking option labels.💡 Details
Before this PR, any finite number fell through to
SINGLE_SELECT. NowinferFieldDataTypeunconditionally returnsNUMBERfortypeof value === "number" && Number.isFinite(value), with no way to opt back intoSINGLE_SELECTfor numeric values.Consider a workflow with an existing
SINGLE_SELECTfield namedPrioritywhose options are1,2,3(numeric labels), configured withpriority: 2. Previously this worked with no warning. After this change,expectedDataTypebecomesNUMBER, andcheckFieldTypeMismatch(fieldName, field, "NUMBER")will fire aField type mismatch for "Priority": Expected NUMBER but found SINGLE_SELECTwarning on every single run, even though the field type is correct and intentional.The fix in the PR description is scoped to "missing field creates the wrong type" and "existing NUMBER field produces spurious warning" — but it introduces the mirror-image bug for existing SINGLE_SELECT fields with numeric values, which isn't covered by any new test.
Suggested fix: only infer
NUMBERwhen there's no existing field, or when the existing field's dataType is unknown — i.e. pass the existing field (or its dataType) intoinferFieldDataTypeand prefer the already-established type over re-inferring from the value when a field already exists.