-
Notifications
You must be signed in to change notification settings - Fork 733
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: ResetDigital #3766
base: master
Are you sure you want to change the base?
Conversation
no need to include `required` if its empty _Originally posted by @onkarvhanumante in prebid#3452 (comment)
} | ||
|
||
func getBidType(imp openrtb2.Imp) (openrtb_ext.BidType, error) { | ||
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.
Implemented as suggested
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.
Implemented as suggested
could you point out or link where MType
changes are implemented?
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.
Sorry, it was addressed on the point that we support only single format bids, so we could assume the anti pattern. Anyway, it would be more advisable to change to the normal pattern?
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.
Anyway, it would be more advisable to change to the normal pattern?
Prebid team recommends using MType field. But if it's not doable then current change suffices single format bid. @bruno-siira should mention in Bidder docs that adapter expects only single format bids in the incoming request
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.
When we're talking about the Bidder Docs what is this file exacly @onkarvhanumante
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.
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report
|
Implementation of suggestions
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report
|
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.
We have now a Builder
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.
There is a schema now
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report
|
} | ||
|
||
func (a *adapter) MakeBids(internalRequest *openrtb2.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) { | ||
if response.StatusCode == http.StatusNoContent || response.StatusCode == http.StatusBadRequest || response.StatusCode != http.StatusOK { |
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.
code path is not covered. should add json tests to improve coverage
bidMap := bidData.(map[string]interface{}) | ||
bid := getBidFromResponse(bidMap) | ||
if bid == 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.
code path is not covered. should add json tests to improve coverage
|
||
bidTypes, err := getBidTypes(internalRequest.Imp[0]) | ||
if err != nil { | ||
return nil, []error{err} |
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.
code path is not covered. should add json tests to improve coverage
} | ||
} | ||
|
||
if bid.Price == 0 { |
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.
code path is not covered. should add json tests to improve coverage
if value, ok := requestData["cpm"].(float64); ok { | ||
return value | ||
} | ||
return 0.0 // Default value if "cpm" doesn't exist or is not a float64 |
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.
code path is not covered. should add json tests to improve coverage
} | ||
if imp.Video != nil { | ||
bidTypes = append(bidTypes, openrtb_ext.BidTypeVideo) | ||
} | ||
if imp.Audio != nil { | ||
bidTypes = append(bidTypes, openrtb_ext.BidTypeAudio) | ||
} | ||
if len(bidTypes) == 0 { | ||
return nil, fmt.Errorf("failed to find matching imp for bid %s", imp.ID) | ||
} |
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.
code path is not covered. should add json tests to improve coverage
return openrtb_ext.BidTypeVideo, nil | ||
} else if imp.Audio != nil { | ||
return openrtb_ext.BidTypeAudio, nil | ||
} | ||
|
||
return "", fmt.Errorf("failed to find matching imp for bid %s", imp.ID) |
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.
code path is not covered. should add json tests to improve coverage
@bruno-siira please add or link bidder docs PR for adapter. |
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report
|
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report
|
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.
@bruno-siira few open comments. PTAL
…xcluded. Can be rewritten as below:
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report
|
if response.StatusCode == http.StatusNoContent || response.StatusCode == http.StatusBadRequest || response.StatusCode != http.StatusOK { | ||
if adapters.IsResponseStatusCodeNoContent(response) { | ||
return nil, nil | ||
} | ||
|
||
if err := adapters.CheckResponseStatusCodeForErrors(response); err != nil { | ||
return nil, []error{err} | ||
} |
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.
should add json exemplary tests for above code path
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report
|
// MaximumBids is the maximum number of bids that can be returned by this adapter. | ||
const maxBids = 1 | ||
|
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.
maxBids
is unused. can remove this var
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report
|
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.
Please add the following exemplary tests:
resetdigitaltest/exemplary/simple-video.json
resetdigitaltest/exemplary/simple-audio.json
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.
Please delete the directory resetdigitaltest/examplary
and move all of the tests in that directory to a new directory called resetdigitaltest/supplemental
with the following names:
missing-cur.json
missing-site.json
multi-format.json
request-error.json
Please also add the following JSON tests to the supplemental directory:
missing-device.json
multi-imp.json
missing-banner-height-width.json
missing-video-height-width.json
invalid-bid-width.json
invalid-bid-height.json
invalid-bid-type-native.json
For each of these tests I would copy one of your exemplary tests and make the minimum modifications to exercise the use case.
func getHeaders(request *openrtb2.BidRequest) http.Header { | ||
headers := http.Header{} | ||
|
||
if request != nil && request.Device != nil && request.Site != nil { // what about request.App? Do we need to do something different with Referrer in the app case assuming we care about app? |
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.
As specified in the comment, how are you handling app requests? Do you support app? If so, please update your yaml file declaring what formats you support for app. If you don't support it, let us know and delete this comment.
|
||
func getBidFromResponse(bidResponse *resetDigitalBid) (*openrtb2.Bid, error) { | ||
if bidResponse.CPM == 0 { | ||
// brian to check how to report this |
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.
I will look into this and get back to you.
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.
You don't need this check. Core will discard a zero dollar bid as long as it isn't associated with a deal.
bid.W = i | ||
} else if err != nil { | ||
return nil, err | ||
} // the error should be returned here if ParseInt fails |
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.
Delete comment.
if i, err := strconv.ParseInt(bidResponse.H, 10, 64); err == nil && i > 0 { | ||
bid.H = i | ||
} else if err != nil { | ||
return nil, err | ||
} // the error should be returned here if ParseInt fails |
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.
A better way to write this would be:
h, err := strconv.ParseInt(bidResponse.H, 10, 64)
if err != nil {
return nil, err
}
bid.H = h
This matches the current behavior you have. If the height is successfully converted to an int but is zero, what do you want to do?
// handle the error | ||
if bid == nil { | ||
// it would be better to return an error here | ||
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.
The comments left here in the code need to be addressed. We discussed returning an error so you might want to change this block to something like:
if err != nil || bid == nil {
errs = append(errs, err)
continue
}
|
||
bidType, err := GetMediaTypeForImp(requestImps, bid.ImpID) | ||
if err != nil { | ||
return nil, []error{err} |
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.
Does it make more sense to append the error and continue processing the rest of the bids?
If so, this should be:
if err != nil {
errs = append(errs, err)
continue
}
func processDataFromRequest(requestData *openrtb2.BidRequest, imp openrtb2.Imp, bidType openrtb_ext.BidType) (resetDigitalRequest, error) { | ||
var reqData resetDigitalRequest | ||
|
||
if requestData.Site != 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.
Do you need to handle App data?
if request != nil && request.Device != nil && request.Site != nil { // what about request.App? Do we need to do something different with Referrer in the app case assuming we care about app? | ||
addNonEmptyHeaders(&headers, map[string]string{ | ||
"Referer": request.Site.Page, | ||
"Accept-Language": request.Device.Language, | ||
"User-Agent": request.Device.UA, | ||
"X-Forwarded-For": request.Device.IP, | ||
"X-Real-Ip": request.Device.IP, | ||
"Content-Type": "application/json;charset=utf-8", | ||
"Accept": "application/json", | ||
}) | ||
} |
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.
This conditional only adds the headers if both site and device are not nil.
Is it possible that one of those is present but the other is nil?
If so, you should probably have two separate calls to addNonEmptyHeaders
where one is wrapped in a if request != nil && request.Device != nil
conditional and the other in a if request != nil && request.Site != nil
conditional.
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report
|
This PR adds a new adapter for Reset Digital to the Prebid Server (Go version). The adapter enables Prebid Server to communicate with Reset Digital for real-time advertising auctions.
Changes:
Added the Reset Digital adapter in adapters/resetdigital/.
Implemented necessary methods for the adapter.
Added unit tests for the adapter in adapters/resetdigital/resetdigital_test.go.
Updated the documentation to include the configuration for the Reset Digital adapter.
Testing:
Unit tests have been written and verified to ensure the adapter works correctly.
Manual testing has been performed to verify the integration with Reset Digital.
Notes:
This system is based on single bid, so it's based on that premise.
Related Issues: