Skip to content

[Exch-13810] return buyer exts in response to dsp#18

Closed
Kacper Fus (Fuska1) wants to merge 14 commits intomasterfrom
EXCH-13810-Return-buyer-exts-in-response-to-DSP
Closed

[Exch-13810] return buyer exts in response to dsp#18
Kacper Fus (Fuska1) wants to merge 14 commits intomasterfrom
EXCH-13810-Return-buyer-exts-in-response-to-DSP

Conversation

@Fuska1
Copy link
Copy Markdown

No description provided.

@Fuska1 Kacper Fus (Fuska1) marked this pull request as ready for review August 28, 2025 10:35
@Fuska1 Kacper Fus (Fuska1) marked this pull request as draft August 28, 2025 10:59
@Fuska1 Kacper Fus (Fuska1) marked this pull request as ready for review August 28, 2025 11:12
Comment thread adapters/openx/openx.go Outdated
Comment thread adapters/openx/openx.go Outdated
Comment thread adapters/openx/openxtest/exemplary/bid-ext-meta.json Outdated
Comment thread adapters/openx/openx.go Outdated
Comment thread adapters/openx/openx_test.go Outdated
Comment thread adapters/openx/openx.go Outdated
Comment on lines +37 to +39
DspId *string `json:"dsp_id,omitempty"`
BrandId *string `json:"brand_id,omitempty"`
BuyerId *string `json:"buyer_id,omitempty"`
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

minor

not critical, but I don't think we need pointers here, more common to define just strings and check for empty string

Suggested change
DspId *string `json:"dsp_id,omitempty"`
BrandId *string `json:"brand_id,omitempty"`
BuyerId *string `json:"buyer_id,omitempty"`
DspId string `json:"dsp_id,omitempty"`
BrandId string `json:"brand_id,omitempty"`
BuyerId string `json:"buyer_id,omitempty"`

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

changed, thanks for suggestion

Comment thread adapters/openx/openx_test.go Outdated
func TestGetBidMeta(t *testing.T) {
buyerId := "123"
dspId := "456"
bradId := "789"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
bradId := "789"
brandId := "789"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

I am also wondering what other types of test are available to additionally verify the feature?

Comment thread adapters/openx/openx.go
}
}

func getBuyerIdFromExt(ext *oxBidExt) int {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this function will work correctly
but still it's best practice to check for an empty value before passing value to strconv.Atoi() to avoid spawning an error each time there is no value available in json

Suggested change
func getBuyerIdFromExt(ext *oxBidExt) int {
func getBuyerIdFromExt(ext *oxBidExt) int {
if ext.BuyerId == "" {
return 0
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

added, thanks for the suggestion

@Fuska1
Copy link
Copy Markdown
Author

LGTM

I am also wondering what other types of test are available to additionally verify the feature?

At the moment, this is the only way I know to test this functionality before deployment. Unfortunately, in the tests that use JSON files, the bid is compared with the expectedBidResponse from TypedBid.Bid, which in our case will never be equal because we add values to meta. Also other adapters test similar features this way.

Comment thread adapters/openx/openx_test.go Outdated
Comment thread adapters/openx/openx_test.go Outdated
Comment thread adapters/openx/openx_test.go Outdated
@Fuska1
Copy link
Copy Markdown
Author

merged in prebid#4507 (comment)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants