-
Notifications
You must be signed in to change notification settings - Fork 722
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
New Adapter: Readpeak #3610
New Adapter: Readpeak #3610
Conversation
adapters/readpeak/readpeak.go
Outdated
func getMediaType(impID string, imps []openrtb2.Imp) (openrtb_ext.BidType, error) { | ||
for _, imp := range imps { | ||
if imp.ID == impID { | ||
if imp.Banner != nil { |
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.
Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeBanner, nil. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
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.
@readpeak-user PTAL at above comment and provide feedback on using Mtype
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.
Fixed to use mtype from bid response
Code coverage summaryNote:
readpeakRefer here for heat map coverage report
|
tests are failing on PR with following error:
run gofmt on above files to fix errors reported by test suite |
@@ -0,0 +1,15 @@ | |||
endpoint: https://dsp.readpeak.com/header/prebid |
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.
endpoint is reachable
curl -i --location --request POST 'https://dsp.readpeak.com/header/prebid'
HTTP/2 204
date: Fri, 05 Apr 2024 10:11:33 GMT
- EEA | ||
maintainer: | ||
email: devteam@readpeak.com | ||
gvlVendorID: 290 |
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.
verified gvl id
curl https://vendor-list.consensu.org/v2/vendor-list.json | jq '.vendors."290"'
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 430k 100 430k 0 0 730k 0 --:--:-- --:--:-- --:--:-- 729k
{
"id": 290,
"name": "Readpeak Oy",
"purposes": [
1,
3,
4
],
"legIntPurposes": [
2,
7,
10
],
"flexiblePurposes": [
2,
7,
10
],
"specialPurposes": [
1,
2
],
"features": [
3
],
"specialFeatures": [],
"policyUrl": "https://www.readpeak.com/terms/readpeak-privacy-policy",
"cookieMaxAgeSeconds": 7776000,
"usesCookies": true,
"cookieRefresh": true,
"usesNonCookieAccess": false,
"deviceStorageDisclosureUrl": "https://static.readpeak.com/tcf/deviceStorage.json"
}
adapters/readpeak/readpeak.go
Outdated
bidResponse := adapters.NewBidderResponseWithBidsCapacity(len(request.Imp)) | ||
bidResponse.Currency = response.Cur |
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.
by default line 130 will initialize bidResponse.Currency
as USD
reassigning bidResponse.Currency
on line 131 without checking response.Cur
length may overwrite bidResponse.Currency
with blank value
code block can be refactored as below
bidResponse := adapters.NewBidderResponseWithBidsCapacity(len(request.Imp))
if len(response.Cur) != 0 {
bidResponse.Currency = response.Cur
}
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.
Fixed
Code coverage summaryNote:
readpeakRefer here for heat map coverage report
|
@readpeak-user Pr checks are failing as you would need to run gofmt in your code files. |
Code coverage summaryNote:
readpeakRefer here for heat map coverage report
|
adapters/readpeak/readpeak.go
Outdated
if requestCopy.Imp[i].Native == nil && requestCopy.Imp[i].Banner == nil { | ||
continue | ||
} |
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.
static/bidder-info/readpeak.yaml
specifies either banner
or Native
is required.
prebid server core will ensure either banner
or native
is present else will return error before adapter code is being invoked
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.
Removed the check
adapters/readpeak/readpeak.go
Outdated
if responseData.StatusCode == http.StatusNoContent { | ||
return nil, nil | ||
} |
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.
prebid-server/adapters/response.go
Lines 26 to 28 in ec729e6
func IsResponseStatusCodeNoContent(response *ResponseData) bool { | |
return response.StatusCode == http.StatusNoContent | |
} |
could make use of response util here
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.
Used util function
@readpeak-user test suites are failing with following error
|
Code coverage summaryNote:
readpeakRefer here for heat map coverage report
|
@onkarvhanumante I have not been able to figure out why those tests fail. Any ideas? How can I run these tests locally? |
@readpeak-user requesting to merge prebid-server/adapters/pubmatic/pubmatic.go Lines 175 to 182 in 7635519
|
Code coverage summaryNote:
readpeakRefer here for heat map coverage report
|
Thanks @onkarvhanumante. Should be fixed now. |
Adds s2s adapter for Readpeak.
First time writing Go so please check carefully :)
Link to document PR prebid/prebid.github.io#5242