Vrtcal adapter: Added Native Support #2956
Conversation
gargcreation1992
changed the title
| site: | ||
| mediaTypes: | ||
| - banner | ||
| - video | ||
| - native |
There was a problem hiding this comment.
Requesting you to please add the json test case for this change.
There was a problem hiding this comment.
Native test cases added
adapters/vrtcal/vrtcal.go
Outdated
|
|
||
| if imp.Native != nil { | ||
| return openrtb_ext.BidTypeNative, nil | ||
| } |
There was a problem hiding this comment.
Warning - This is an anti-pattern. This code assumes that for multi-format bid request, mediatype will be set to type by default. I would recommend to set the mediaType based on bidder response. Your bidder should be able to set openrtb2.Bid.MType field and use that field here to get mediaTypeForImp. Here is an example on how this is done. Even if your bidder does not support multi-format request, I would highly encourage you to make this change.
There was a problem hiding this comment.
Thank you for the feedback...We've updated to support the 2.6 mtype parameter when present....We've also included code to still support the pre 2.6 non passed mtype (being it wasnt in the oRTB standard until 2.6).
adapters/vrtcal/vrtcal.go
Outdated
| @@ -101,10 +101,17 @@ func getReturnTypeForImp(impID string, imps []openrtb2.Imp) (openrtb_ext.BidType | |||
| return openrtb_ext.BidTypeBanner, nil | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
nitpick - Please remove this blank line.
adapters/vrtcal/vrtcal.go
Outdated
| if imp.Video != nil { | ||
| return openrtb_ext.BidTypeVideo, nil | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
nit - there is already a blank line above line 109. Please remove this blank line.
adapters/vrtcal/vrtcal.go
Outdated
| return openrtb_ext.BidTypeNative, nil | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
nitpick - Please remove this blank line.
|
@vrtcal-dev Requesting you to please address the open comments. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
adapters/vrtcal/vrtcal.go
Outdated
| @@ -94,25 +94,37 @@ func Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server co | |||
| return bidder, nil | |||
| } | |||
|
|
|||
| func getReturnTypeForImp(impID string, imps []openrtb2.Imp) (openrtb_ext.BidType, error) { | |||
| func getReturnTypeForImp(MType openrtb2.MarkupType, impID string, imps []openrtb2.Imp) (openrtb_ext.BidType, error) { | |||
There was a problem hiding this comment.
should use camelCase style for var. MType -> mType
adapters/vrtcal/vrtcal.go
Outdated
|
|
||
| return "", &errortypes.BadServerResponse{ | ||
| Message: fmt.Sprintf("Unsupported return type for ID: \"%s\"", impID), | ||
| if mType != 0 { |
There was a problem hiding this comment.
This part of code does not have to be in for loop. This function argument is having mType as input that is set on seatBid.Bid[i] which can only hold one impression. I would suggest to remove this for loop.
There was a problem hiding this comment.
Cleaned up this function to reflect your suggestions and updated related test cases.
adapters/vrtcal/vrtcal.go
Outdated
| return "", &errortypes.BadServerResponse{ | ||
| Message: fmt.Sprintf("Unsupported return type for ID (mtype present): \"%s\"", impID)} | ||
| } | ||
| } else { //Pre-2.6 support when mtype is not passed |
There was a problem hiding this comment.
If your bidder supports MType and you are setting adapters.TypedBid.BidType - I do not think you need this fallback here. I suggest to remove this fallback check and use single source of truth to define adapters.TypedBid.BidType.
There was a problem hiding this comment.
Cleaned up this function to reflect your suggestions and updated related test cases.
No description provided.