Skip to content

fix(ms-bing-capi): item price should be number not integer - #3859

Open
scottlepich-lz wants to merge 6 commits into
segmentio:mainfrom
scottlepich-lz:fix/ms-bing-capi-item-price-number
Open

fix(ms-bing-capi): item price should be number not integer#3859
scottlepich-lz wants to merge 6 commits into
segmentio:mainfrom
scottlepich-lz:fix/ms-bing-capi-item-price-number

Conversation

@scottlepich-lz

Copy link
Copy Markdown

Summary

The Microsoft Bing CAPI destination's items InputField defines the per-item price property with type: 'integer'. This is too strict: Microsoft's own UET Conversions API accepts decimal item prices in whole currency units.

Because of this, Segment's mapping-kit rejects perfectly valid decimal prices (e.g. 9.99) with:

400: Item Price must be an integer but it was a number

Since mapping validation fails, the entire event is dropped — not just the price field. Any storefront selling at non-round prices (which is essentially all of them) silently loses conversion events.

Evidence (Microsoft docs)

Microsoft's official UET Conversions API integration guide documents price as a decimal:

So integer is incorrect; the correct type is number.

Fix

Change the items[].price field from type: 'integer' to type: 'number'. quantity is left as integer (correct for quantity). Nothing else is changed.

generated-types.ts is unchanged, because both integer and number map to the TypeScript type number. Codegen (generate:types) was run for the destination and produced no diff.

Testing

  • Added a unit test in sendEvent/__tests__/sendEvent.test.ts proving a decimal item price (9.99) is now accepted and forwarded to the request payload without a validation error.
  • Verified the new test fails against the old type: 'integer' with the exact error Item Price must be an integer but it was a number, and passes with type: 'number'.
  • Ran the scoped test suite: 16 passed, 16 total (jest src/destinations/ms-bing-capi).
  • Ran typecheck and eslint for the touched destination — clean.

Follow-up observation (out of scope, not changed here)

While in this file I noticed a separate latent bug: the items field's default mapping writes to key item_price, but the defined property is price. Since the field is additionalProperties: false, item_price is silently dropped, so the default mapping never sends a price at all. I've deliberately left this out of this PR to keep it focused on the type fix; flagging it as a follow-up.

🤖 Generated with Claude Code

The `items` field defined per-item `price` with `type: 'integer'`, which
is too strict. Microsoft's UET Conversions API accepts DECIMAL item
prices in whole currency units — their official docs show the `price`
parameter example as 25.1 and every JSON sample uses decimals such as
"price": 25.1 and "price": 27.3
(https://learn.microsoft.com/en-us/advertising/guides/uet-conversion-api-integration).

With `type: 'integer'`, Segment's mapping-kit rejected valid decimal
prices (e.g. 9.99) with "400: Item Price must be an integer but it was a
number", dropping the entire event. Changing the type to `number` allows
decimal prices through. `quantity` remains an integer.

generated-types.ts is unchanged since both integer and number map to the
TypeScript type `number`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 6, 2026 23:25
@scottlepich-lz
scottlepich-lz requested a review from a team as a code owner July 6, 2026 23:25

Copilot AI left a comment

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.

Pull request overview

Note

Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.

Updates the Microsoft Bing CAPI destination schema to allow decimal item prices and adds a regression test to ensure decimal prices pass mapping validation and are forwarded to Bing.

Changes:

  • Relaxed items[].price validation from integer to number in the action field schema.
  • Added a unit test confirming a decimal item price (9.99) is accepted and appears in the outgoing request payload.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
packages/destination-actions/src/destinations/ms-bing-capi/sendEvent/fields.ts Loosens schema validation for item price to accept decimals.
packages/destination-actions/src/destinations/ms-bing-capi/sendEvent/tests/sendEvent.test.ts Adds regression coverage for decimal item prices being forwarded successfully.

.post(`/v1/${settings.UetTag}/events`, (body: any) => {
const items = body.data[0].customData.items
expect(items).toHaveLength(1)
expect(items[0].price).toBe(9.99)
Copilot AI review requested due to automatic review settings July 20, 2026 08:25

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

.post(`/v1/${settings.UetTag}/events`, (body: any) => {
const items = body.data[0].customData.items
expect(items).toHaveLength(1)
expect(items[0].price).toBe(9.99)
Copilot AI review requested due to automatic review settings July 21, 2026 05:40

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

.post(`/v1/${settings.UetTag}/events`, (body: any) => {
const items = body.data[0].customData.items
expect(items).toHaveLength(1)
expect(items[0].price).toBe(9.99)
Comment on lines 196 to 200
price: {
label: 'Item Price',
description: 'The price of the item, after discounts.',
type: 'integer'
type: 'number'
},
scottlepich-lz and others added 2 commits July 29, 2026 12:40
The sendEvent action sends events with `continueOnValidationError: true`, so
Microsoft's CAPI accepts an event (HTTP 200, `eventsReceived: 1`) even when it
has non-fatal issues, and reports those issues as entries in
`error.details[]` flagged with `"isWarning": true`.

The batch response handler (`performBatch`) matched `error.details[]` by
`index` only and treated ANY matching detail as a hard failure. As a result,
a batched event whose only detail was a warning was marked `status: 400` and
reported as a failed delivery, even though Microsoft had accepted it. This
caused real, silent delivery failures for any batched event that triggered a
Microsoft warning. (Single `perform` returns the raw 200, so the bug only
manifested in batching.)

Fix: only treat a detail as a failure when it is NOT a warning
(`detail.index === index && !detail.isWarning`). A warning-only event is now
marked SUCCESS (200); an event with a real (non-warning) error at its index is
still marked 400. `types.ts` is extended to model the `isWarning` and
`errorCode` fields the API actually returns.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The sendEvent `items.price` field type was changed from `integer` to
`number` in fields.ts, but metadata.json was not regenerated. Running
`yarn generate:metadata-payload` updates the Item Price field type to
`number`, which fixes the failing "Assert metadata payloads are
up-to-date" CI check.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 29, 2026 21:33

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

packages/destination-actions/src/destinations/ms-bing-capi/sendEvent/index.ts:75

  • The PR title/summary focuses on changing items[].price from integer -> number, but this change also modifies batch error semantics by treating isWarning: true details as success. That’s a behavioral change worth explicitly documenting in the PR description (or splitting into a separate PR) so reviewers and release notes capture the expanded scope.
    const error = details.find((detail) => detail.index === index && !detail.isWarning)

packages/destination-actions/src/destinations/ms-bing-capi/sendEvent/index.ts:75

  • This change drops warning-only details entirely from per-index results. While that avoids incorrectly failing accepted events, it also removes potentially useful diagnostics (e.g., partial field acceptance). Consider surfacing warning details somewhere non-fatal (e.g., attach warnings to the success response payload if supported, or emit debug-level logs/metrics) so operators can detect degraded payload quality without event drops.
    const error = details.find((detail) => detail.index === index && !detail.isWarning)

Copilot AI review requested due to automatic review settings August 3, 2026 18:06

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

packages/destination-actions/src/destinations/ms-bing-capi/sendEvent/index.ts:75

  • The PR title/description focus on changing items[].price from integer to number, but this change also alters batch error handling by ignoring isWarning: true details. Please either (a) update the PR title/description to explicitly include the warning-handling behavior change, or (b) split this into a separate PR to keep scope aligned.
    const error = details.find((detail) => detail.index === index && !detail.isWarning)

packages/destination-actions/src/destinations/ms-bing-capi/sendEvent/index.ts:75

  • Using !detail.isWarning relies on truthiness and is a bit opaque given isWarning is optional. For clarity (and to preserve the current behavior where undefined is treated as a real error), consider making the intent explicit with a comparison like detail.isWarning !== true.
    const error = details.find((detail) => detail.index === index && !detail.isWarning)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants