Conversation
adapters/beop/beop.go
Outdated
| continue | ||
| } | ||
| bidResponseFinal.Bids = append(bidResponseFinal.Bids, &adapters.TypedBid{ | ||
| Bid: &bid, |
There was a problem hiding this comment.
Found incorrect assignment made to Bid. bid variable receives a new value in each iteration of range loop. Assigning the address of bid (&bid) to Bid will result in a pointer that always points to the same memory address with the value of the last iteration. This can lead to unexpected behavior or incorrect results. Refer https://go.dev/play/p/9ZS1f-5h4qS
Consider using an index variable in the seatBids.Bid loop as shown below
for _, seatBid := range response.SeatBid {
for i := range seatBids.Bid {
...
responseBid := &adapters.TypedBid{
Bid: &seatBids.Bid[i],
...
}
...
...
}
}
There was a problem hiding this comment.
Please take a look at this comment
adapters/beop/beop.go
Outdated
| var bidExt openrtb_ext.ExtBid | ||
| err := jsonutil.Unmarshal(bid.Ext, &bidExt) | ||
| if err == nil && bidExt.Prebid != nil { | ||
| return openrtb_ext.ParseBidType(string(bidExt.Prebid.Type)) |
There was a problem hiding this comment.
Consider this as a suggestion. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, recommends implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
Code coverage summaryNote:
beopRefer here for heat map coverage report |
Code coverage summaryNote:
beopRefer here for heat map coverage report |
|
Hello @bsardo, hope you are good 🙏 |
|
Can you please create a documentation PR and link it in the description? |
| @@ -0,0 +1,33 @@ | |||
| { | |||
There was a problem hiding this comment.
I do not see tests related to the bidder params. Can you please add that under beop adapters folder with file name params_test.go?
There was a problem hiding this comment.
Did you want a test with missing param for example?
There was a problem hiding this comment.
Hi, what i meant was adding tests for valid and invalid params in a dedicated file adapters/beop/params_test.go
Please take a look at example - https://github.com/prebid/prebid-server/blob/master/adapters/adverxo/params_test.go
adapters/beop/beop.go
Outdated
| Message: "ext.bidder not provided", | ||
| } | ||
| } | ||
| if beopExt.BeopPublisherID == "" && beopExt.BeopNetworkID == "" { |
There was a problem hiding this comment.
Not sure if we need this validation since if the param validation fails, it the adapter will never be called. In the bidder params configuration, we have already provided that either pid or nid, ntpnid needs to be provided
There was a problem hiding this comment.
Oh I didn't know that, so yeah we don't need extra validation.
adapters/beop/beop.go
Outdated
| continue | ||
| } | ||
| bidResponseFinal.Bids = append(bidResponseFinal.Bids, &adapters.TypedBid{ | ||
| Bid: &bid, |
There was a problem hiding this comment.
Please take a look at this comment
| } | ||
| } | ||
| var beopExt openrtb_ext.ExtImpBeop | ||
| if err := jsonutil.Unmarshal(bidderExt.Bidder, &beopExt); err != nil { |
There was a problem hiding this comment.
I think we can add coverage for unmarshalling errors in this code path
adapters/beop/beop.go
Outdated
| url, err := url.Parse(a.endpoint) | ||
| if err != nil { | ||
| return "", &errortypes.Warning{ | ||
| Message: "Failed to parse endpoint", |
There was a problem hiding this comment.
Do we need to do this validation since endpoint is supplanted from the config by Prebid ?
adapters/beop/beop.go
Outdated
| case openrtb2.MarkupVideo: | ||
| bidType = openrtb_ext.BidTypeVideo | ||
| case openrtb2.MarkupAudio: | ||
| bidType = openrtb_ext.BidTypeAudio |
There was a problem hiding this comment.
Please add coverage for BidType - Audio, MarkupNative and invalid type
There was a problem hiding this comment.
Since the beop.yaml allow only banner and video I will remove the extra cases.
Code coverage summaryNote:
beopRefer here for heat map coverage report |
|
Please find here the DOC PR : prebid/prebid.github.io#6432 |
Code coverage summaryNote:
beopRefer here for heat map coverage report |
| responseData *adapters.ResponseData) ( | ||
| *adapters.BidderResponse, []error, | ||
| ) { | ||
| if responseData.StatusCode == http.StatusNoContent { |
| endpoint: "https://hb.collectiveaudience.co/rtb/bid" | ||
| endpointCompression: gzip | ||
| maintainer: | ||
| email: tech@collectiveaudience.co |
There was a problem hiding this comment.
Sent a verification mail on this mailbox. Please acknowledge over the mailbox with "received"
Thanks
Code coverage summaryNote:
beopRefer here for heat map coverage report |
karwaankit32
left a comment
There was a problem hiding this comment.
Acknowledging that i received a reply over the mailbox
Type of change
Documention PR: prebid/prebid.github.io#6432