New Adapter: ResetDigital #3766
Conversation
no need to include `required` if its empty _Originally posted by @onkarvhanumante in prebid#3452 (comment)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| func getBidType(imp openrtb2.Imp) (openrtb_ext.BidType, error) { | ||
| if imp.Banner != nil { |
There was a problem hiding this comment.
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.
Implemented as suggested
There was a problem hiding this comment.
Implemented as suggested
could you point out or link where MType changes are implemented?
There was a problem hiding this comment.
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.
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.
When we're talking about the Bidder Docs what is this file exacly @onkarvhanumante
There was a problem hiding this comment.
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
Implementation of suggestions
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
| } | ||
| if reqImp.Audio != nil { | ||
| return openrtb_ext.BidTypeAudio | ||
| } |
There was a problem hiding this comment.
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.BidTypeAudio. 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.
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
|
@bsardo, this PR is once again ready for review. |
Pr review comments 12 16 24
| } | ||
|
|
||
| func getBidType(imp openrtb2.Imp) (openrtb_ext.BidType, error) { | ||
| if imp.Banner != nil { |
There was a problem hiding this comment.
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.
| return openrtb_ext.BidTypeVideo, nil | ||
| } else if imp.Audio != nil { | ||
| return openrtb_ext.BidTypeAudio, nil | ||
| } |
There was a problem hiding this comment.
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.BidTypeVideo, 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.
| return openrtb_ext.BidTypeBanner, nil | ||
| } else if imp.Video != nil { | ||
| return openrtb_ext.BidTypeVideo, nil | ||
| } else if imp.Audio != nil { |
There was a problem hiding this comment.
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.BidTypeAudio, 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.
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
| bidResponse := adapters.NewBidderResponseWithBidsCapacity(len(request.Imp)) | ||
| // Ensure there is exactly one bid in the response | ||
| if len(response.Bids) != 1 { | ||
| return nil, []error{fmt.Errorf("expected exactly one bid in the response, but got %d", len(response.Bids))} |
There was a problem hiding this comment.
Please achieve code coverage of this error case by adding a supplemental JSON test (e.g. multiple-bids.json) that contains more than one bid.
| // Map the incoming impression to its ID for media type determination | ||
| requestImp, found := findRequestImpByID(request.Imp, resetDigitalBid.ImpID) | ||
| if !found { | ||
| return nil, []error{fmt.Errorf("no matching impression found for ImpID %s", resetDigitalBid.ImpID)} |
There was a problem hiding this comment.
Please achieve code coverage of this error case by adding a supplemental JSON test (e.g. imp-id-mismatch.json) with a bid response imp id that does not match the bid request imp id.
| @@ -207,51 +207,65 @@ func processDataFromRequest(requestData *openrtb2.BidRequest, imp openrtb2.Imp, | |||
| } | |||
|
|
|||
| func (a *adapter) MakeBids(request *openrtb2.BidRequest, requestData *adapters.RequestData, responseData *adapters.ResponseData) (*adapters.BidderResponse, []error) { | |||
| // Return early if the response contains no content | |||
There was a problem hiding this comment.
Nitpick: I suggest removing all of the comments in this function to tighten this up. IMO your code is self explanatory now. You can leave it of course if you think it will benefit your developers.
| if err := adapters.CheckResponseStatusCodeForErrors(responseData); err != nil { | ||
| return nil, []error{err} | ||
| } | ||
|
|
||
| // Parse the response body into a single bid response | ||
| var response resetDigitalBidResponse | ||
| if err := json.Unmarshal(responseData.Body, &response); err != nil { | ||
| return nil, []error{err} |
There was a problem hiding this comment.
Please achieve code coverage of this error case by adding a supplemental JSON test (e.g. malformed-response.json) that contains a malformed response body. This can be achieved by simply setting your mock response body to a string instead of an object.
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
| return openrtb_ext.BidTypeVideo, nil | ||
| } else if imp.Audio != nil { | ||
| return openrtb_ext.BidTypeAudio, nil | ||
| } |
There was a problem hiding this comment.
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.
| return openrtb_ext.BidTypeVideo, nil | ||
| } else if imp.Audio != nil { | ||
| return openrtb_ext.BidTypeAudio, nil | ||
| } |
There was a problem hiding this comment.
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.BidTypeVideo, 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.
| return openrtb_ext.BidTypeVideo, nil | ||
| } else if imp.Audio != nil { | ||
| return openrtb_ext.BidTypeAudio, nil | ||
| } |
There was a problem hiding this comment.
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.BidTypeAudio, 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.
|
|
||
| func GetMediaTypeForImp(reqImp openrtb2.Imp) openrtb_ext.BidType { | ||
|
|
||
| if reqImp.Video != nil { |
There was a problem hiding this comment.
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.BidTypeVideo. 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.
| if reqImp.Video != nil { | ||
| return openrtb_ext.BidTypeVideo | ||
| } | ||
| if reqImp.Audio != nil { |
There was a problem hiding this comment.
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.BidTypeAudio. 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.
|
Ready for review again, @bsardo, please! |
|
@bruno-siira - are there any differences between this and the PBS-Java version of your adapter or is this a direct port? |
Co-authored-by: Bruno Jacinto <b.jacinto@gmail.com> Co-authored-by: Emmanuel <eogbonna097@gmail.com> Co-authored-by: dirk-rd <dirk@resetdigital.co>
Co-authored-by: Bruno Jacinto <b.jacinto@gmail.com> Co-authored-by: Emmanuel <eogbonna097@gmail.com> Co-authored-by: dirk-rd <dirk@resetdigital.co>
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: